Hi, Jonathan Tan wrote: > Server options were added in commit 5e3548ef16 ("fetch: send server > options when using protocol v2", 2018-04-24), supported only for > protocol version 2. Add a warning if server options are specified for > the user if a legacy protocol is used instead. > > An effort is made to avoid printing the same warning more than once by > clearing transport->server_options after the warning, but this does not > fully avoid double-printing (for example, when backfulling tags using > another fetch with a non-reusable transport). > > Signed-off-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx> > --- > t/t5702-protocol-v2.sh | 17 +++++++++++++++++ > transport.c | 8 ++++++++ > 2 files changed, 25 insertions(+) Thanks for writing this. I'd be in favor of making this die(). Compare what we already do in push: if (args->push_options && !push_options_supported) die(_("the receiving end does not support push options")); What happens in the case of push with protocol v0, where server options are supported? [...] > --- a/transport.c > +++ b/transport.c > @@ -252,6 +252,12 @@ static int connect_setup(struct transport *transport, int for_push) > return 0; > } > > +static void warn_server_options_unsupported(struct transport *transport) > +{ > + warning(_("Ignoring server options because protocol version does not support it")); nits: - error and warning messages tend to use lowercase - the user running into this may want to know how to switch to a better protocol version. Is there e.g. a manpage we can point them to? For example: fatal: server options require protocol version 2 or later hint: see protocol.version in "git help config" for more details > + transport->server_options = NULL; Should this use a static to also warn only once in the tag-catchup case you mentioned? > +} > + > /* > * Obtains the protocol version from the transport and writes it to > * transport->data->version, first connecting if not already connected. > @@ -286,6 +292,7 @@ static struct ref *handshake(struct transport *transport, int for_push, > break; > case protocol_v1: > case protocol_v0: > + warn_server_options_unsupported(transport); > get_remote_heads(&reader, &refs, > for_push ? REF_NORMAL : 0, > &data->extra_have, > @@ -363,6 +370,7 @@ static int fetch_refs_via_pack(struct transport *transport, > break; > case protocol_v1: > case protocol_v0: > + warn_server_options_unsupported(transport); > refs = fetch_pack(&args, data->fd, data->conn, Looks like this only affects fetch, so the question above about push is only about the commit message. With whatever subset of the suggested changes makes sense, Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx> Thanks.