Re: [PATCH] protocol: treat unrecognized protocol.version setting as 0

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 28, 2018, 18:07 UTC
Message-ID: <20180228180734.GD251290@aiede.svl.corp.google.com>
In-Reply-To: <xmqqbmg9m260.fsf@gitster-ct.c.googlers.com>

Hi,

Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:

Show 26 quoted lines

>> If I share my .gitconfig or .git/config file between multiple machines
>> (or between multiple Git versions on a single machine) and set
>>
>> 	[protocol]
>> 		version = 2
>>
>> then running "git fetch" with a Git version that does not support
>> protocol v2 errors out with
>>
>> 	fatal: unknown value for config 'protocol.version': 2
>>
>> In the spirit of v1.7.6-rc0~77^2~1 (Improve error handling when
>> parsing dirstat parameters, 2011-04-29), it is better to (perhaps
>> after warning the user) ignore the unrecognized protocol version.
>
> I do not agree with the analogy at all.
>
> Showing dirstat with different tweaks than the user expected to see
> is a local and read-only thing.  Talking to the other side over a
> protocol the user explicitly wanted to avoid (e.g. imagine the case
> where your upstream's protocol version 1 implementation is
> critically buggy and you want to use version 2 if you talk with
> them) by accident is a more grave error, possibly affecting the
> other side that you may not have enough power to recover from
> (e.g. damaging the remote repository to which you only have push
> access and not interactive shell).

Fair enough about the analogy being a poor one.

I disagree with characterizing using protocol v0 as a grave error, because the scenario seems far-fetched to me. I can imagine the opposite scenario, wanting to use protocol v0 because a server's implementation of v2 is buggy (for example, producing wrong negotiation results and wasting bandwidth, or erroring out for a request that should not be an error). I am having trouble imagining a broken v0 implementation doing anything worse than being slow or erroring out.

That said, erroring out to catch spelling errors is nice and helpful, so I would be okay with continuing to apply this as a local patch and it not landing upstream.

The following third way would also be possible, but I'm pretty sure I don't like it:

- As Duy suggested, allowing multiple 'protocol.version' settings.
  The last setting that the Git implementation recognizes wins.

- If there is at least one 'protocol.version' setting but the Git
  implementation doesn't recognize any of them, error out.

The reason I don't like it is that it doesn't make your example case work significantly better. If I have

	[protocol]
		version = 1

in my ~/.gitconfig and

	[protocol]
		version = v2

in .git/config, then that means I intended to use protocol v2 and misspelled it, but this heuristic would ignore the v2 and fall back to version 1.

As a side note, the exact same problem would happen if I make a typo in the config key:

	[protocol]
		vesion = 2

Treating unrecognized values from the growing set of values as an error while not treating unrecognized keys from the growing set of keys as an error feels odd to me. I think it would be useful to document whatever we decide about this subject in Documentation/CodingGuidelines.

Thanks, Jonathan