Re: Protocol v2 in v2.27 (Re: Re* [ANNOUNCE] Git v2.27.0-rc1)

[Derrick Stolee <stolee@gmail.com>]

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