{"thread":{"id":"47940","subject":"[PATCH] protocol: treat unrecognized protocol.version setting as 0","startedAt":"2018-02-28T00:51:12Z","lastAt":"2018-02-28T18:07:43Z","messageCount":9,"participants":["Jonathan Nieder","Brandon Williams","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"340501","messageId":"20180228005059.GA251290@aiede.svl.corp.google.com","threadId":"47940","inReplyTo":null,"subject":"[PATCH] protocol: treat unrecognized protocol.version setting as 0","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-02-28T00:50:59Z","receivedAt":"2018-02-28T00:51:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"If I share my .gitconfig or .git/config file between multiple machines\n(or between multiple Git versions on a single machine) and set\n\n\t[protocol]\n\t\tversion = 2\n\nthen running \"git fetch\" with a Git version that does not support\nprotocol v2 errors out with\n\n\tfatal: unknown value for config 'protocol.version': 2\n\nIn the spirit of v1.7.6-rc0~77^2~1 (Improve error handling when\nparsing dirstat parameters, 2011-04-29), it is better to (perhaps\nafter warning the user) ignore the unrecognized protocol version.\nAfter all, future Git versions might add even more protocol versions,\nand using two different Git versions with the same Git repo, machine,\nor home directory should not cripple the older Git version just\nbecause of a parameter that is only understood by a more recent Git\nversion.\n\nSo ignore the unrecognized value.  It may be useful for spell checking\n(for instance, if I put \"version = v1\" intending \"version = 1\") to\nwarn about such settings, but this patch does not, since at least in\nthese early days for protocol v2 it is expected for configurations\nthat want to opportunistically use protocol v2 if available not to be\nunusual.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nGoogle has been running with a patch like this internally for a while,\nsince we have been changing the protocol.version number to a new value\nlike 20180226 each time a minor tweak to the protocolv2 RFC occured.\n\nThe bit I have doubts about is whether to warn.  What do you think?\n\nThanks,\nJonathan\n\n protocol.c             |  8 ++------\n t/t5700-protocol-v1.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/protocol.c b/protocol.c\nindex 43012b7eb6..ce9c634a3a 100644\n--- a/protocol.c\n+++ b/protocol.c\n@@ -17,12 +17,8 @@ enum protocol_version get_protocol_version_config(void)\n \tconst char *value;\n \tif (!git_config_get_string_const(\"protocol.version\", &value)) {\n \t\tenum protocol_version version = parse_protocol_version(value);\n-\n-\t\tif (version == protocol_unknown_version)\n-\t\t\tdie(\"unknown value for config 'protocol.version': %s\",\n-\t\t\t    value);\n-\n-\t\treturn version;\n+\t\tif (version != protocol_unknown_version)\n+\t\t\treturn version;\n \t}\n \n \treturn protocol_v0;\ndiff --git a/t/t5700-protocol-v1.sh b/t/t5700-protocol-v1.sh\nindex ba86a44eb1..c35767ab01 100755\n--- a/t/t5700-protocol-v1.sh\n+++ b/t/t5700-protocol-v1.sh\n@@ -31,6 +31,18 @@ test_expect_success 'clone with git:// using protocol v1' '\n \tgrep \"clone< version 1\" log\n '\n \n+test_expect_success 'unrecognized protocol versions fall back to v0' '\n+\tGIT_TRACE_PACKET=1 git -c protocol.version=9999 \\\n+\t\tclone \"$GIT_DAEMON_URL/parent\" v9999 2>log &&\n+\n+\tgit -C daemon_child log -1 --format=%s >actual &&\n+\tgit -C \"$daemon_parent\" log -1 --format=%s >expect &&\n+\ttest_cmp expect actual &&\n+\n+\t# Client requested and server responded using protocol v0\n+\t! grep version log\n+'\n+\n test_expect_success 'fetch with git:// using protocol v1' '\n \ttest_commit -C \"$daemon_parent\" two &&\n \n-- \n2.16.2.395.g2e18187dfd\n\n"},{"id":"340503","messageId":"20180228010233.GA45342@google.com","threadId":"47940","inReplyTo":"20180228005059.GA251290@aiede.svl.corp.google.com","subject":"Re: [PATCH] protocol: treat unrecognized protocol.version setting as 0","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-02-28T01:02:33Z","receivedAt":"2018-02-28T01:02:41Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/27, Jonathan Nieder wrote:\n> If I share my .gitconfig or .git/config file between multiple machines\n> (or between multiple Git versions on a single machine) and set\n> \n> \t[protocol]\n> \t\tversion = 2\n> \n> then running \"git fetch\" with a Git version that does not support\n> protocol v2 errors out with\n> \n> \tfatal: unknown value for config 'protocol.version': 2\n> \n> In the spirit of v1.7.6-rc0~77^2~1 (Improve error handling when\n> parsing dirstat parameters, 2011-04-29), it is better to (perhaps\n> after warning the user) ignore the unrecognized protocol version.\n> After all, future Git versions might add even more protocol versions,\n> and using two different Git versions with the same Git repo, machine,\n> or home directory should not cripple the older Git version just\n> because of a parameter that is only understood by a more recent Git\n> version.\n> \n> So ignore the unrecognized value.  It may be useful for spell checking\n> (for instance, if I put \"version = v1\" intending \"version = 1\") to\n> warn about such settings, but this patch does not, since at least in\n> these early days for protocol v2 it is expected for configurations\n> that want to opportunistically use protocol v2 if available not to be\n> unusual.\n> \n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> Google has been running with a patch like this internally for a while,\n> since we have been changing the protocol.version number to a new value\n> like 20180226 each time a minor tweak to the protocolv2 RFC occured.\n> \n> The bit I have doubts about is whether to warn.  What do you think?\n\nPatch looks good to me.  And I don't have a strong preference either way\nfor whether to warn or not.\n\n\n-- \nBrandon Williams\n"},{"id":"340517","messageId":"CACsJy8Cn2hcodoR8ksRBY9qf7MmJaP+KAzYqv6seeR9s-Be8Hw@mail.gmail.com","threadId":"47940","inReplyTo":"20180228010233.GA45342@google.com","subject":"Re: [PATCH] protocol: treat unrecognized protocol.version setting as 0","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-28T01:10:35Z","receivedAt":"2018-02-28T01:11:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 28, 2018 at 8:02 AM, Brandon Williams <bmwill@google.com> wrote:\n> On 02/27, Jonathan Nieder wrote:\n>> If I share my .gitconfig or .git/config file between multiple machines\n>> (or between multiple Git versions on a single machine) and set\n>>\n>>       [protocol]\n>>               version = 2\n>>\n>> then running \"git fetch\" with a Git version that does not support\n>> protocol v2 errors out with\n>>\n>>       fatal: unknown value for config 'protocol.version': 2\n>>\n>> In the spirit of v1.7.6-rc0~77^2~1 (Improve error handling when\n>> parsing dirstat parameters, 2011-04-29), it is better to (perhaps\n>> after warning the user) ignore the unrecognized protocol version.\n>> After all, future Git versions might add even more protocol versions,\n>> and using two different Git versions with the same Git repo, machine,\n>> or home directory should not cripple the older Git version just\n>> because of a parameter that is only understood by a more recent Git\n>> version.\n\nI wonder if it's better to specify multiple versions. If v2 is not\nrecognized by this git but v0 is, then it can pick that up. But if you\nexplicitly tell it to choose between v2 and v3 only and it does not\nunderstand either, then it dies. Not sure if this is a good idea\nthough.\n-- \nDuy\n"},{"id":"340518","messageId":"20180228011604.GC45342@google.com","threadId":"47940","inReplyTo":"CACsJy8Cn2hcodoR8ksRBY9qf7MmJaP+KAzYqv6seeR9s-Be8Hw@mail.gmail.com","subject":"Re: [PATCH] protocol: treat unrecognized protocol.version setting as 0","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-02-28T01:16:04Z","receivedAt":"2018-02-28T01:16:12Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/28, Duy Nguyen wrote:\n> On Wed, Feb 28, 2018 at 8:02 AM, Brandon Williams <bmwill@google.com> wrote:\n> > On 02/27, Jonathan Nieder wrote:\n> >> If I share my .gitconfig or .git/config file between multiple machines\n> >> (or between multiple Git versions on a single machine) and set\n> >>\n> >>       [protocol]\n> >>               version = 2\n> >>\n> >> then running \"git fetch\" with a Git version that does not support\n> >> protocol v2 errors out with\n> >>\n> >>       fatal: unknown value for config 'protocol.version': 2\n> >>\n> >> In the spirit of v1.7.6-rc0~77^2~1 (Improve error handling when\n> >> parsing dirstat parameters, 2011-04-29), it is better to (perhaps\n> >> after warning the user) ignore the unrecognized protocol version.\n> >> After all, future Git versions might add even more protocol versions,\n> >> and using two different Git versions with the same Git repo, machine,\n> >> or home directory should not cripple the older Git version just\n> >> because of a parameter that is only understood by a more recent Git\n> >> version.\n> \n> I wonder if it's better to specify multiple versions. If v2 is not\n> recognized by this git but v0 is, then it can pick that up. But if you\n> explicitly tell it to choose between v2 and v3 only and it does not\n> understand either, then it dies. Not sure if this is a good idea\n> though.\n\nI mean that's definitely a possibility, but I don't think its worth the\neffort to get that working until we actually need it.  I'm hoping we\nreally don't bump version numbers often.\n\n-- \nBrandon Williams\n"},{"id":"340519","messageId":"20180228012207.GB251290@aiede.svl.corp.google.com","threadId":"47940","inReplyTo":"CACsJy8Cn2hcodoR8ksRBY9qf7MmJaP+KAzYqv6seeR9s-Be8Hw@mail.gmail.com","subject":"Re: [PATCH] protocol: treat unrecognized protocol.version setting as 0","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-02-28T01:22:07Z","receivedAt":"2018-02-28T01:22:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Duy Nguyen wrote:\n> On Wed, Feb 28, 2018 at 8:02 AM, Brandon Williams <bmwill@google.com> wrote:\n>> On 02/27, Jonathan Nieder wrote:\n\n>>> If I share my .gitconfig or .git/config file between multiple machines\n>>> (or between multiple Git versions on a single machine) and set\n>>>\n>>>       [protocol]\n>>>               version = 2\n>>>\n>>> then running \"git fetch\" with a Git version that does not support\n>>> protocol v2 errors out with\n>>>\n>>>       fatal: unknown value for config 'protocol.version': 2\n>>>\n>>> In the spirit of v1.7.6-rc0~77^2~1 (Improve error handling when\n>>> parsing dirstat parameters, 2011-04-29), it is better to (perhaps\n>>> after warning the user) ignore the unrecognized protocol version.\n>>> After all, future Git versions might add even more protocol versions,\n>>> and using two different Git versions with the same Git repo, machine,\n>>> or home directory should not cripple the older Git version just\n>>> because of a parameter that is only understood by a more recent Git\n>>> version.\n>\n> I wonder if it's better to specify multiple versions. If v2 is not\n> recognized by this git but v0 is, then it can pick that up. But if you\n> explicitly tell it to choose between v2 and v3 only and it does not\n> understand either, then it dies. Not sure if this is a good idea\n> though.\n\nInteresting thought.  Something roughly like this (on top of the patch\nthis is a reply to)?\n\ndiff --git i/protocol.c w/protocol.c\nindex ce9c634a3a..6aa8857a11 100644\n--- i/protocol.c\n+++ w/protocol.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"string-list.h\"\n #include \"config.h\"\n #include \"protocol.h\"\n \n@@ -14,14 +15,18 @@ static enum protocol_version parse_protocol_version(const char *value)\n \n enum protocol_version get_protocol_version_config(void)\n {\n-\tconst char *value;\n-\tif (!git_config_get_string_const(\"protocol.version\", &value)) {\n-\t\tenum protocol_version version = parse_protocol_version(value);\n-\t\tif (version != protocol_unknown_version)\n-\t\t\treturn version;\n+\tconst struct string_list *values;\n+\tconst struct string_list_item *value;\n+\tenum protocol_version result = protocol_v0;\n+\n+\tvalues = git_config_get_value_multi(\"protocol.version\");\n+\tfor_each_string_list_item(value, values) {\n+\t\tenum protocol_version v = parse_protocol_version(value->string);\n+\t\tif (v != protocol_unknown_version)\n+\t\t\tresult = v;\n \t}\n \n-\treturn protocol_v0;\n+\treturn result;\n }\n \n enum protocol_version determine_protocol_version_server(void)\n"},{"id":"340581","messageId":"xmqqbmg9m260.fsf@gitster-ct.c.googlers.com","threadId":"47940","inReplyTo":"20180228005059.GA251290@aiede.svl.corp.google.com","subject":"Re: [PATCH] protocol: treat unrecognized protocol.version setting as 0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-28T17:53:11Z","receivedAt":"2018-02-28T17:53:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> If I share my .gitconfig or .git/config file between multiple machines\n> (or between multiple Git versions on a single machine) and set\n>\n> \t[protocol]\n> \t\tversion = 2\n>\n> then running \"git fetch\" with a Git version that does not support\n> protocol v2 errors out with\n>\n> \tfatal: unknown value for config 'protocol.version': 2\n>\n> In the spirit of v1.7.6-rc0~77^2~1 (Improve error handling when\n> parsing dirstat parameters, 2011-04-29), it is better to (perhaps\n> after warning the user) ignore the unrecognized protocol version.\n\nI do not agree with the analogy at all.  \n\nShowing dirstat with different tweaks than the user expected to see\nis a local and read-only thing.  Talking to the other side over a\nprotocol the user explicitly wanted to avoid (e.g. imagine the case\nwhere your upstream's protocol version 1 implementation is\ncritically buggy and you want to use version 2 if you talk with\nthem) by accident is a more grave error, possibly affecting the\nother side that you may not have enough power to recover from\n(e.g. damaging the remote repository to which you only have push\naccess and not interactive shell).\n\n"},{"id":"340582","messageId":"xmqq7eqxm22w.fsf@gitster-ct.c.googlers.com","threadId":"47940","inReplyTo":"20180228012207.GB251290@aiede.svl.corp.google.com","subject":"Re: [PATCH] protocol: treat unrecognized protocol.version setting as 0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-28T17:55:03Z","receivedAt":"2018-02-28T17:55:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> I wonder if it's better to specify multiple versions. If v2 is not\n>> recognized by this git but v0 is, then it can pick that up. But if you\n>> explicitly tell it to choose between v2 and v3 only and it does not\n>> understand either, then it dies. Not sure if this is a good idea\n>> though.\n>\n> Interesting thought.  Something roughly like this (on top of the patch\n> this is a reply to)?\n\nI am OK with that, i.e. allow the user to tell us what is acceptable\nand pick from them, as long as the blind \"we do not know so let's\nfall back to v0\" is removed.\n\n\n\n>\n> diff --git i/protocol.c w/protocol.c\n> index ce9c634a3a..6aa8857a11 100644\n> --- i/protocol.c\n> +++ w/protocol.c\n> @@ -1,4 +1,5 @@\n>  #include \"cache.h\"\n> +#include \"string-list.h\"\n>  #include \"config.h\"\n>  #include \"protocol.h\"\n>  \n> @@ -14,14 +15,18 @@ static enum protocol_version parse_protocol_version(const char *value)\n>  \n>  enum protocol_version get_protocol_version_config(void)\n>  {\n> -\tconst char *value;\n> -\tif (!git_config_get_string_const(\"protocol.version\", &value)) {\n> -\t\tenum protocol_version version = parse_protocol_version(value);\n> -\t\tif (version != protocol_unknown_version)\n> -\t\t\treturn version;\n> +\tconst struct string_list *values;\n> +\tconst struct string_list_item *value;\n> +\tenum protocol_version result = protocol_v0;\n> +\n> +\tvalues = git_config_get_value_multi(\"protocol.version\");\n> +\tfor_each_string_list_item(value, values) {\n> +\t\tenum protocol_version v = parse_protocol_version(value->string);\n> +\t\tif (v != protocol_unknown_version)\n> +\t\t\tresult = v;\n>  \t}\n>  \n> -\treturn protocol_v0;\n> +\treturn result;\n>  }\n>  \n>  enum protocol_version determine_protocol_version_server(void)\n"},{"id":"340585","messageId":"xmqqtvu1kmzo.fsf@gitster-ct.c.googlers.com","threadId":"47940","inReplyTo":"xmqq7eqxm22w.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] protocol: treat unrecognized protocol.version setting as 0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-28T18:06:19Z","receivedAt":"2018-02-28T18:06:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n>\n>>> I wonder if it's better to specify multiple versions. If v2 is not\n>>> recognized by this git but v0 is, then it can pick that up. But if you\n>>> explicitly tell it to choose between v2 and v3 only and it does not\n>>> understand either, then it dies. Not sure if this is a good idea\n>>> though.\n>>\n>> Interesting thought.  Something roughly like this (on top of the patch\n>> this is a reply to)?\n>\n> I am OK with that, i.e. allow the user to tell us what is acceptable\n> and pick from them, as long as the blind \"we do not know so let's\n> fall back to v0\" is removed.\n\nSorry, but \"as long as\" was a bit too strong.  I am not convinced\nthat the \"treat unrecognized one as if it were 0\" is a good idea for\nthe reasons I gave in the my earlier response, but I am not yet\nconvinced that erroring out is the best solution, either.\n\nSo s/as long as/but it may be better if/, perhaps.\n"},{"id":"340586","messageId":"20180228180734.GD251290@aiede.svl.corp.google.com","threadId":"47940","inReplyTo":"xmqqbmg9m260.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] protocol: treat unrecognized protocol.version setting as 0","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-02-28T18:07:34Z","receivedAt":"2018-02-28T18:07:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> If I share my .gitconfig or .git/config file between multiple machines\n>> (or between multiple Git versions on a single machine) and set\n>>\n>> \t[protocol]\n>> \t\tversion = 2\n>>\n>> then running \"git fetch\" with a Git version that does not support\n>> protocol v2 errors out with\n>>\n>> \tfatal: unknown value for config 'protocol.version': 2\n>>\n>> In the spirit of v1.7.6-rc0~77^2~1 (Improve error handling when\n>> parsing dirstat parameters, 2011-04-29), it is better to (perhaps\n>> after warning the user) ignore the unrecognized protocol version.\n>\n> I do not agree with the analogy at all.\n>\n> Showing dirstat with different tweaks than the user expected to see\n> is a local and read-only thing.  Talking to the other side over a\n> protocol the user explicitly wanted to avoid (e.g. imagine the case\n> where your upstream's protocol version 1 implementation is\n> critically buggy and you want to use version 2 if you talk with\n> them) by accident is a more grave error, possibly affecting the\n> other side that you may not have enough power to recover from\n> (e.g. damaging the remote repository to which you only have push\n> access and not interactive shell).\n\nFair enough about the analogy being a poor one.\n\nI disagree with characterizing using protocol v0 as a grave error,\nbecause the scenario seems far-fetched to me.  I can imagine the\nopposite scenario, wanting to use protocol v0 because a server's\nimplementation of v2 is buggy (for example, producing wrong\nnegotiation results and wasting bandwidth, or erroring out for a\nrequest that should not be an error).  I am having trouble imagining a\nbroken v0 implementation doing anything worse than being slow or\nerroring out.\n\nThat said, erroring out to catch spelling errors is nice and helpful,\nso I would be okay with continuing to apply this as a local patch and\nit not landing upstream.\n\nThe following third way would also be possible, but I'm pretty sure I\ndon't like it:\n\n- As Duy suggested, allowing multiple 'protocol.version' settings.\n  The last setting that the Git implementation recognizes wins.\n\n- If there is at least one 'protocol.version' setting but the Git\n  implementation doesn't recognize any of them, error out.\n\nThe reason I don't like it is that it doesn't make your example case\nwork significantly better.  If I have\n\n\t[protocol]\n\t\tversion = 1\n\nin my ~/.gitconfig and\n\n\t[protocol]\n\t\tversion = v2\n\nin .git/config, then that means I intended to use protocol v2 and\nmisspelled it, but this heuristic would ignore the v2 and fall back to\nversion 1.\n\nAs a side note, the exact same problem would happen if I make a typo\nin the config key:\n\n\t[protocol]\n\t\tvesion = 2\n\nTreating unrecognized values from the growing set of values as an\nerror while not treating unrecognized keys from the growing set of\nkeys as an error feels odd to me.  I think it would be useful to\ndocument whatever we decide about this subject in\nDocumentation/CodingGuidelines.\n\nThanks,\nJonathan\n"}]}