[PATCH] protocol: treat unrecognized protocol.version setting as 0
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 --- 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= \ + clone "$GIT_DAEMON_URL/parent" v 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
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 > --- > 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
On Wed, Feb 28, 2018 at 8:02 AM, Brandon Williams 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
On 02/28, Duy Nguyen wrote: > On Wed, Feb 28, 2018 at 8:02 AM, Brandon Williams 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
Duy Nguyen wrote: > On Wed, Feb 28, 2018 at 8:02 AM, Brandon Williams 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
Jonathan Nieder 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
Jonathan Nieder 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 writes: > Jonathan Nieder 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
Hi, Junio C Hamano wrote: > Jonathan Nieder 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