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

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

[Jonathan Nieder]

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.
After all, future Git versions might add even more protocol versions,
and using two different Git versions with the same Git repo, machine,
or home directory should not cripple the older Git version just
because of a parameter that is only understood by a more recent Git
version.

So ignore the unrecognized value.  It may be useful for spell checking
(for instance, if I put "version = v1" intending "version = 1") to
warn about such settings, but this patch does not, since at least in
these early days for protocol v2 it is expected for configurations
that want to opportunistically use protocol v2 if available not to be
unusual.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Google has been running with a patch like this internally for a while,
since we have been changing the protocol.version number to a new value
like 20180226 each time a minor tweak to the protocolv2 RFC occured.

The bit I have doubts about is whether to warn.  What do you think?

Thanks,
Jonathan

 protocol.c             |  8 ++------
 t/t5700-protocol-v1.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/protocol.c b/protocol.c
index 43012b7eb6..ce9c634a3a 100644
--- a/protocol.c
+++ b/protocol.c
@@ -17,12 +17,8 @@ enum protocol_version get_protocol_version_config(void)
 	const char *value;
 	if (!git_config_get_string_const("protocol.version", &value)) {
 		enum protocol_version version = parse_protocol_version(value);
-
-		if (version == protocol_unknown_version)
-			die("unknown value for config 'protocol.version': %s",
-			    value);
-
-		return version;
+		if (version != protocol_unknown_version)
+			return version;
 	}
 
 	return protocol_v0;
diff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh
index ba86a44eb1..c35767ab01 100755
--- a/t/t5700-protocol-v1.sh
+++ b/t/t5700-protocol-v1.sh
@@ -31,6 +31,18 @@ test_expect_success 'clone with git:// using protocol v1' '
 	grep "clone< version 1" log
 '
 
+test_expect_success 'unrecognized protocol versions fall back to v0' '
+	GIT_TRACE_PACKET=1 git -c protocol.version=9999 \
+		clone "$GIT_DAEMON_URL/parent" v9999 2>log &&
+
+	git -C daemon_child log -1 --format=%s >actual &&
+	git -C "$daemon_parent" log -1 --format=%s >expect &&
+	test_cmp expect actual &&
+
+	# Client requested and server responded using protocol v0
+	! grep version log
+'
+
 test_expect_success 'fetch with git:// using protocol v1' '
 	test_commit -C "$daemon_parent" two &&
 
-- 
2.16.2.395.g2e18187dfd

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

[Brandon Williams]

On 02/27, Jonathan Nieder wrote:
> 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.
> After all, future Git versions might add even more protocol versions,
> and using two different Git versions with the same Git repo, machine,
> or home directory should not cripple the older Git version just
> because of a parameter that is only understood by a more recent Git
> version.
> 
> So ignore the unrecognized value.  It may be useful for spell checking
> (for instance, if I put "version = v1" intending "version = 1") to
> warn about such settings, but this patch does not, since at least in
> these early days for protocol v2 it is expected for configurations
> that want to opportunistically use protocol v2 if available not to be
> unusual.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Google has been running with a patch like this internally for a while,
> since we have been changing the protocol.version number to a new value
> like 20180226 each time a minor tweak to the protocolv2 RFC occured.
> 
> The bit I have doubts about is whether to warn.  What do you think?

Patch looks good to me.  And I don't have a strong preference either way
for whether to warn or not.


-- 
Brandon Williams

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

[Duy Nguyen]

On Wed, Feb 28, 2018 at 8:02 AM, Brandon Williams <bmwill@google.com> wrote:
> On 02/27, Jonathan Nieder wrote:
>> 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.
>> After all, future Git versions might add even more protocol versions,
>> and using two different Git versions with the same Git repo, machine,
>> or home directory should not cripple the older Git version just
>> because of a parameter that is only understood by a more recent Git
>> version.

I wonder if it's better to specify multiple versions. If v2 is not
recognized by this git but v0 is, then it can pick that up. But if you
explicitly tell it to choose between v2 and v3 only and it does not
understand either, then it dies. Not sure if this is a good idea
though.
-- 
Duy

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

[Brandon Williams]

On 02/28, Duy Nguyen wrote:
> On Wed, Feb 28, 2018 at 8:02 AM, Brandon Williams <bmwill@google.com> wrote:
> > On 02/27, Jonathan Nieder wrote:
> >> 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.
> >> After all, future Git versions might add even more protocol versions,
> >> and using two different Git versions with the same Git repo, machine,
> >> or home directory should not cripple the older Git version just
> >> because of a parameter that is only understood by a more recent Git
> >> version.
> 
> I wonder if it's better to specify multiple versions. If v2 is not
> recognized by this git but v0 is, then it can pick that up. But if you
> explicitly tell it to choose between v2 and v3 only and it does not
> understand either, then it dies. Not sure if this is a good idea
> though.

I mean that's definitely a possibility, but I don't think its worth the
effort to get that working until we actually need it.  I'm hoping we
really don't bump version numbers often.

-- 
Brandon Williams

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

[Jonathan Nieder]

Duy Nguyen wrote:
> On Wed, Feb 28, 2018 at 8:02 AM, Brandon Williams <bmwill@google.com> wrote:
>> On 02/27, Jonathan Nieder wrote:

>>> 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.
>>> After all, future Git versions might add even more protocol versions,
>>> and using two different Git versions with the same Git repo, machine,
>>> or home directory should not cripple the older Git version just
>>> because of a parameter that is only understood by a more recent Git
>>> version.
>
> I wonder if it's better to specify multiple versions. If v2 is not
> recognized by this git but v0 is, then it can pick that up. But if you
> explicitly tell it to choose between v2 and v3 only and it does not
> understand either, then it dies. Not sure if this is a good idea
> though.

Interesting thought.  Something roughly like this (on top of the patch
this is a reply to)?

diff --git i/protocol.c w/protocol.c
index ce9c634a3a..6aa8857a11 100644
--- i/protocol.c
+++ w/protocol.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "string-list.h"
 #include "config.h"
 #include "protocol.h"
 
@@ -14,14 +15,18 @@ static enum protocol_version parse_protocol_version(const char *value)
 
 enum protocol_version get_protocol_version_config(void)
 {
-	const char *value;
-	if (!git_config_get_string_const("protocol.version", &value)) {
-		enum protocol_version version = parse_protocol_version(value);
-		if (version != protocol_unknown_version)
-			return version;
+	const struct string_list *values;
+	const struct string_list_item *value;
+	enum protocol_version result = protocol_v0;
+
+	values = git_config_get_value_multi("protocol.version");
+	for_each_string_list_item(value, values) {
+		enum protocol_version v = parse_protocol_version(value->string);
+		if (v != protocol_unknown_version)
+			result = v;
 	}
 
-	return protocol_v0;
+	return result;
 }
 
 enum protocol_version determine_protocol_version_server(void)

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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).

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> I wonder if it's better to specify multiple versions. If v2 is not
>> recognized by this git but v0 is, then it can pick that up. But if you
>> explicitly tell it to choose between v2 and v3 only and it does not
>> understand either, then it dies. Not sure if this is a good idea
>> though.
>
> Interesting thought.  Something roughly like this (on top of the patch
> this is a reply to)?

I am OK with that, i.e. allow the user to tell us what is acceptable
and pick from them, as long as the blind "we do not know so let's
fall back to v0" is removed.



>
> diff --git i/protocol.c w/protocol.c
> index ce9c634a3a..6aa8857a11 100644
> --- i/protocol.c
> +++ w/protocol.c
> @@ -1,4 +1,5 @@
>  #include "cache.h"
> +#include "string-list.h"
>  #include "config.h"
>  #include "protocol.h"
>  
> @@ -14,14 +15,18 @@ static enum protocol_version parse_protocol_version(const char *value)
>  
>  enum protocol_version get_protocol_version_config(void)
>  {
> -	const char *value;
> -	if (!git_config_get_string_const("protocol.version", &value)) {
> -		enum protocol_version version = parse_protocol_version(value);
> -		if (version != protocol_unknown_version)
> -			return version;
> +	const struct string_list *values;
> +	const struct string_list_item *value;
> +	enum protocol_version result = protocol_v0;
> +
> +	values = git_config_get_value_multi("protocol.version");
> +	for_each_string_list_item(value, values) {
> +		enum protocol_version v = parse_protocol_version(value->string);
> +		if (v != protocol_unknown_version)
> +			result = v;
>  	}
>  
> -	return protocol_v0;
> +	return result;
>  }
>  
>  enum protocol_version determine_protocol_version_server(void)

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> I wonder if it's better to specify multiple versions. If v2 is not
>>> recognized by this git but v0 is, then it can pick that up. But if you
>>> explicitly tell it to choose between v2 and v3 only and it does not
>>> understand either, then it dies. Not sure if this is a good idea
>>> though.
>>
>> Interesting thought.  Something roughly like this (on top of the patch
>> this is a reply to)?
>
> I am OK with that, i.e. allow the user to tell us what is acceptable
> and pick from them, as long as the blind "we do not know so let's
> fall back to v0" is removed.

Sorry, but "as long as" was a bit too strong.  I am not convinced
that the "treat unrecognized one as if it were 0" is a good idea for
the reasons I gave in the my earlier response, but I am not yet
convinced that erroring out is the best solution, either.

So s/as long as/but it may be better if/, perhaps.

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

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> 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