| Message ID | 20200520220023.GB3648@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Protocol v2 in v2.27 (Re: Re* [ANNOUNCE] Git v2.27.0-rc1) | expand |
Jonathan Nieder <jrnieder@gmail.com> writes: > Speaking of which, should we enable protocol v2 by default for people > with feature.experimental enabled, like this? It is an excellent idea, but is something that had to have been proposed before -rc0 to be in the upcoming release, no? The patch looks good; do we have the master list of things that are under control of feature.experimental knob in the documentation, or is it a feature that we do not have to have a centralized control? > diff --git c/Documentation/config/protocol.txt i/Documentation/config/protocol.txt > index 0b40141613e..c46e9b3d00a 100644 > --- c/Documentation/config/protocol.txt > +++ i/Documentation/config/protocol.txt > @@ -48,7 +48,8 @@ protocol.version:: > If set, clients will attempt to communicate with a server > using the specified protocol version. If the server does > not support it, communication falls back to version 0. > - If unset, the default is `0`. > + If unset, the default is `0`, unless `feature.experimental` > + is enabled, in which case the default is `2`. > Supported versions: > + > -- > diff --git c/protocol.c i/protocol.c > index d390391ebac..d1dd3424bba 100644 > --- c/protocol.c > +++ i/protocol.c > @@ -17,6 +17,7 @@ static enum protocol_version parse_protocol_version(const char *value) > enum protocol_version get_protocol_version_config(void) > { > const char *value; > + int val; > const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION"; > const char *git_test_v; > > @@ -30,6 +31,9 @@ enum protocol_version get_protocol_version_config(void) > return version; > } > > + if (!git_config_get_bool("feature.experimental", &val) && val) > + return protocol_v2; > + > git_test_v = getenv(git_test_k); > if (git_test_v && *git_test_v) { > enum protocol_version env = parse_protocol_version(git_test_v);
On Wed, May 20, 2020 at 04:59:29PM -0700, Junio C Hamano wrote: > Jonathan Nieder <jrnieder@gmail.com> writes: > > > Speaking of which, should we enable protocol v2 by default for people > > with feature.experimental enabled, like this? > > It is an excellent idea, but is something that had to have been > proposed before -rc0 to be in the upcoming release, no? Yeah, my original email was almost "hey, did we make a conscious decision to keep v2 reverted after we found the root cause?". But I think it is too late in the -rc cycle to switch now (which is kind of a shame, because we won't really get good data on whether the in_vain fix helped the Linux folks, since it seems clear that most of them are not switching protocol.version themselves). The feature.experimental one we could be a bit more cavalier with, though. I dunno. (I guess you could argue that since v2.26.2 shipped with protocol v2, we could already be pretty cavalier with _leaving_ it on in the new release, but I think as a general principle -rc2 is not the right time to go flipping defaults). -Peff
Junio C Hamano wrote: > Jonathan Nieder <jrnieder@gmail.com> writes: >> Speaking of which, should we enable protocol v2 by default for people >> with feature.experimental enabled, like this? > > It is an excellent idea, but is something that had to have been > proposed before -rc0 to be in the upcoming release, no? I would love to go back in time, but I only have the present to work with. Here, I'm hoping it makes the switch of default back to v0 less of a regression for some interested users. > The patch looks good; do we have the master list of things that are > under control of feature.experimental knob in the documentation, or > is it a feature that we do not have to have a centralized control? Good catch. Here's an updated patch with (1) a commit message and (2) that doc update. -- >8 -- Subject: config: let feature.experimental imply protocol.version=2 Git 2.26 used protocol v2 as its default protocol, but soon after release, users noticed that the protocol v2 negotiation code was prone to fail when fetching from some remotes that are far ahead of others (such as linux-next.git versus Linus's linux.git). That has been fixed by 0b07eecf6ed (Merge branch 'jt/v2-fetch-nego-fix', 2020-05-01), but to be cautious, we are using protocol v0 as the default in 2.27 to buy some time for any other unanticipated issues to surface. To that end, let's ensure that users requesting the bleeding edge using the feature.experimental flag *do* get protocol v2. This way, we can gain experience with a wider audience for the new protocol version and be more confident when it is time to enable it by default for all users in some future Git version. Implementation note: this isn't with the rest of the feature.experimental options in repo-settings.c because those are tied to a repository object, whereas this code path is used for operations like "git ls-remote" that do not require a repository. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Thanks, Jonathan Documentation/config/feature.txt | 4 ++++ Documentation/config/protocol.txt | 3 ++- protocol.c | 4 ++++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/Documentation/config/feature.txt b/Documentation/config/feature.txt index 4e3a5c0cebc..28c33602d52 100644 --- a/Documentation/config/feature.txt +++ b/Documentation/config/feature.txt @@ -22,6 +22,10 @@ existing commit-graph file(s). Occasionally, these files will merge and the write may take longer. Having an updated commit-graph file helps performance of many Git commands, including `git merge-base`, `git push -f`, and `git log --graph`. ++ +* `protocol.version=2` speeds up fetches from repositories with many refs by +allowing the client to specify which refs to list before the server lists +them. feature.manyFiles:: Enable config options that optimize for repos with many files in the diff --git a/Documentation/config/protocol.txt b/Documentation/config/protocol.txt index 0b40141613e..c46e9b3d00a 100644 --- a/Documentation/config/protocol.txt +++ b/Documentation/config/protocol.txt @@ -48,7 +48,8 @@ protocol.version:: If set, clients will attempt to communicate with a server using the specified protocol version. If the server does not support it, communication falls back to version 0. - If unset, the default is `0`. + If unset, the default is `0`, unless `feature.experimental` + is enabled, in which case the default is `2`. Supported versions: + -- diff --git a/protocol.c b/protocol.c index d390391ebac..d1dd3424bba 100644 --- a/protocol.c +++ b/protocol.c @@ -17,6 +17,7 @@ static enum protocol_version parse_protocol_version(const char *value) enum protocol_version get_protocol_version_config(void) { const char *value; + int val; const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION"; const char *git_test_v; @@ -30,6 +31,9 @@ enum protocol_version get_protocol_version_config(void) return version; } + if (!git_config_get_bool("feature.experimental", &val) && val) + return protocol_v2; + git_test_v = getenv(git_test_k); if (git_test_v && *git_test_v) { enum protocol_version env = parse_protocol_version(git_test_v);
Sorry to hijack the thread, but hopefully this is not completely offtopic either, regarding v2 support. Is 1aa8dded3a (connect: don't request v2 when pushing, 2018-03-15) still relevant? Carlo CC +Brandon to help answer
Hi, Carlo Marcelo Arenas Belón wrote: > Sorry to hijack the thread, but hopefully this is not completely offtopic > either, regarding v2 support. Changing subject line. > Is 1aa8dded3a (connect: don't request v2 when pushing, 2018-03-15) still > relevant? Can you elaborate a bit? Are you asking whether we should revert that patch for git 2.28? Protocol v2 for push still does not exist, but we have plans to introduce it soon (reusing more code from v0 push, so this would hopefully have fewer teething pains than v2 for fetch). Thanks, Jonathan
On Wed, May 20, 2020 at 07:45:58PM -0700, Jonathan Nieder wrote: > > Protocol v2 for push still does not exist, but we > have plans to introduce it soon (reusing more code from v0 push, so > this would hopefully have fewer teething pains than v2 for fetch). Thanks for the explanation; I wasn't aware push was indeed still not implemented and was indeed puzzled by that code, which I found while looking at some unrelated issue. Carlo
On 5/20/2020 10:15 PM, Jonathan Nieder wrote: > Junio C Hamano wrote: >> Jonathan Nieder <jrnieder@gmail.com> writes: > >>> Speaking of which, should we enable protocol v2 by default for people >>> with feature.experimental enabled, like this? >> >> It is an excellent idea, but is something that had to have been >> proposed before -rc0 to be in the upcoming release, no? > > I would love to go back in time, but I only have the present to work > with. Here, I'm hoping it makes the switch of default back to v0 less > of a regression for some interested users. > >> The patch looks good; do we have the master list of things that are >> under control of feature.experimental knob in the documentation, or >> is it a feature that we do not have to have a centralized control? > > Good catch. Here's an updated patch with (1) a commit message and (2) > that doc update. > > -- >8 -- > Subject: config: let feature.experimental imply protocol.version=2 For some reason, I thought protocol v2 was part of feature.experimental at some point, but it doesn't appear to be so, or ever even part of the patch series. > Git 2.26 used protocol v2 as its default protocol, but soon after > release, users noticed that the protocol v2 negotiation code was prone > to fail when fetching from some remotes that are far ahead of others > (such as linux-next.git versus Linus's linux.git). That has been > fixed by 0b07eecf6ed (Merge branch 'jt/v2-fetch-nego-fix', > 2020-05-01), Should we really mention the merge here? Is it because the "fix" is spread across two commits? Why not say the tip of the topic? 2f0a093 (fetch-pack: in protocol v2, reset in_vain upon ACK, 2020-05-27) > but to be cautious, we are using protocol v0 as the > default in 2.27 to buy some time for any other unanticipated issues to > surface. > > To that end, let's ensure that users requesting the bleeding edge > using the feature.experimental flag *do* get protocol v2. This way, > we can gain experience with a wider audience for the new protocol > version and be more confident when it is time to enable it by default > for all users in some future Git version. > > Implementation note: this isn't with the rest of the > feature.experimental options in repo-settings.c because those are tied > to a repository object, whereas this code path is used for operations > like "git ls-remote" that do not require a repository. (This may be why protocol.version wasn't included in the first place.) > +* `protocol.version=2` speeds up fetches from repositories with many refs by > +allowing the client to specify which refs to list before the server lists > +them. This is the appropriate tone for this section: it's more about _why_ than _what_. > --- a/Documentation/config/protocol.txt > +++ b/Documentation/config/protocol.txt > @@ -48,7 +48,8 @@ protocol.version:: > If set, clients will attempt to communicate with a server > using the specified protocol version. If the server does > not support it, communication falls back to version 0. > - If unset, the default is `0`. > + If unset, the default is `0`, unless `feature.experimental` > + is enabled, in which case the default is `2`. Looks good. > @@ -17,6 +17,7 @@ static enum protocol_version parse_protocol_version(const char *value) > enum protocol_version get_protocol_version_config(void) > { > const char *value; > + int val; > const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION"; > const char *git_test_v; > > @@ -30,6 +31,9 @@ enum protocol_version get_protocol_version_config(void) > return version; > } > > + if (!git_config_get_bool("feature.experimental", &val) && val) > + return protocol_v2; > + > git_test_v = getenv(git_test_k); > if (git_test_v && *git_test_v) { > enum protocol_version env = parse_protocol_version(git_test_v); > The context around this change is that we check "protocol.version" explicitly before checking "feature.experimental", so that explicit setting wins. Then, feature.experimental overrides the GIT_TEST_PROTOCOL_VERSION environment variable. LGTM. -Stolee
Jonathan Nieder <jrnieder@gmail.com> writes: >> The patch looks good; do we have the master list of things that are >> under control of feature.experimental knob in the documentation, or >> is it a feature that we do not have to have a centralized control? > > Good catch. Here's an updated patch with (1) a commit message and (2) > that doc update. Heh, that wasn't a rhetorical question but me being just too tired and lazy to go hunt for the list ;-) The addition to the section in this patch looks good, too. Will queue. Thanks.
Derrick Stolee <stolee@gmail.com> writes: >> Git 2.26 used protocol v2 as its default protocol, but soon after >> release, users noticed that the protocol v2 negotiation code was prone >> to fail when fetching from some remotes that are far ahead of others >> (such as linux-next.git versus Linus's linux.git). That has been >> fixed by 0b07eecf6ed (Merge branch 'jt/v2-fetch-nego-fix', >> 2020-05-01), > > Should we really mention the merge here? Is it because the "fix" is > spread across two commits? Why not say the tip of the topic? Strictly speaking, the tip of the topic is sufficient. You can use a third-party script "git when-merged" to ask "which exact merge gave us this commit in the mainline?" when given only the tip of the topic, which is what I often end up doing. Writing the merge result reduces the need for doing the same "reverse" look-up. On the other hand, going the other way to find the then-current tip of the topic from the result of the merge is much easier; you can ask the merge result what its second parent is. The rest of your review I can entirely agree with. Thanks, always, for a prompt and well written review.
diff --git c/Documentation/config/protocol.txt i/Documentation/config/protocol.txt index 0b40141613e..c46e9b3d00a 100644 --- c/Documentation/config/protocol.txt +++ i/Documentation/config/protocol.txt @@ -48,7 +48,8 @@ protocol.version:: If set, clients will attempt to communicate with a server using the specified protocol version. If the server does not support it, communication falls back to version 0. - If unset, the default is `0`. + If unset, the default is `0`, unless `feature.experimental` + is enabled, in which case the default is `2`. Supported versions: + -- diff --git c/protocol.c i/protocol.c index d390391ebac..d1dd3424bba 100644 --- c/protocol.c +++ i/protocol.c @@ -17,6 +17,7 @@ static enum protocol_version parse_protocol_version(const char *value) enum protocol_version get_protocol_version_config(void) { const char *value; + int val; const char *git_test_k = "GIT_TEST_PROTOCOL_VERSION"; const char *git_test_v; @@ -30,6 +31,9 @@ enum protocol_version get_protocol_version_config(void) return version; } + if (!git_config_get_bool("feature.experimental", &val) && val) + return protocol_v2; + git_test_v = getenv(git_test_k); if (git_test_v && *git_test_v) { enum protocol_version env = parse_protocol_version(git_test_v);