On 5/20/2020 10:15 PM, Jonathan Nieder wrote:
> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@xxxxxxxxx> 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
