Re: [PATCH v5 1/1] protocol: advertise multiple supported versions

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Nov 16 2018, Josh Steadmon wrote:

I started looking at this to address
https://public-inbox.org/git/nycvar.QRO.7.76.6.1812141318520.43@tvgsbejvaqbjf.bet/
and was about to send a re-roll of my own series, but then...

> Currently the client advertises that it supports the wire protocol
> version set in the protocol.version config. However, not all services
> support the same set of protocol versions. For example, git-receive-pack
> supports v1 and v0, but not v2. If a client connects to git-receive-pack
> and requests v2, it will instead be downgraded to v0. Other services,
> such as git-upload-archive, do not do any version negotiation checks.
>
> This patch creates a protocol version registry. Individual client and
> server programs register all the protocol versions they support prior to
> communicating with a remote instance. Versions should be listed in
> preference order; the version specified in protocol.version will
> automatically be moved to the front of the registry.
>
> The protocol version registry is passed to remote helpers via the
> GIT_PROTOCOL environment variable.
>
> Clients now advertise the full list of registered versions. Servers
> select the first allowed version from this advertisement.
>
> Additionally, remove special cases around advertising version=0.
> Previously we avoided adding version negotiation fields in server
> responses if it looked like the client wanted v0. However, including
> these fields does not change behavior, so it's better to have simpler
> code.

...this paragraph is new in your v5, from the cover letter: "Changes
from V4: remove special cases around advertising version=0". However as
seen in the code & tests:

> [...]
>  static void push_ssh_options(struct argv_array *args, struct argv_array *env,
>  			     enum ssh_variant variant, const char *port,
> -			     enum protocol_version version, int flags)
> +			     const struct strbuf *version_advert, int flags)
>  {
> -	if (variant == VARIANT_SSH &&
> -	    version > 0) {
> +	if (variant == VARIANT_SSH) {
>  		argv_array_push(args, "-o");
>  		argv_array_push(args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT);
> -		argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
> -				 version);
> +		argv_array_pushf(env, GIT_PROTOCOL_ENVIRONMENT "=%s",
> +				 version_advert->buf);
>  	}
> [...]
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -346,7 +346,7 @@ expect_ssh () {
>
>  test_expect_success 'clone myhost:src uses ssh' '
>  	git clone myhost:src ssh-clone &&
> -	expect_ssh myhost src
> +	expect_ssh "-o SendEnv=GIT_PROTOCOL" myhost src
>  '
>
>  test_expect_success !MINGW,!CYGWIN 'clone local path foo:bar' '
> @@ -357,12 +357,12 @@ test_expect_success !MINGW,!CYGWIN 'clone local path foo:bar' '
>
>  test_expect_success 'bracketed hostnames are still ssh' '
>  	git clone "[myhost:123]:src" ssh-bracket-clone &&
> -	expect_ssh "-p 123" myhost src
> +	expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
>  '
>
>  test_expect_success 'OpenSSH variant passes -4' '
>  	git clone -4 "[myhost:123]:src" ssh-ipv4-clone &&
> -	expect_ssh "-4 -p 123" myhost src
> +	expect_ssh "-o SendEnv=GIT_PROTOCOL -4 -p 123" myhost src
>  '
>
>  test_expect_success 'variant can be overridden' '
> @@ -406,7 +406,7 @@ test_expect_success 'OpenSSH-like uplink is treated as ssh' '
>  	GIT_SSH="$TRASH_DIRECTORY/uplink" &&
>  	test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\"" &&
>  	git clone "[myhost:123]:src" ssh-bracket-clone-sshlike-uplink &&
> -	expect_ssh "-p 123" myhost src
> +	expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
>  '
>
>  test_expect_success 'plink is treated specially (as putty)' '
> @@ -446,14 +446,14 @@ test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
>  	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
>  	GIT_SSH_VARIANT=ssh \
>  	git clone "[myhost:123]:src" ssh-bracket-clone-variant-1 &&
> -	expect_ssh "-p 123" myhost src
> +	expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
>  '
>
>  test_expect_success 'ssh.variant overrides plink detection' '
>  	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
>  	git -c ssh.variant=ssh \
>  		clone "[myhost:123]:src" ssh-bracket-clone-variant-2 &&
> -	expect_ssh "-p 123" myhost src
> +	expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
>  '
>
>  test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
> @@ -488,7 +488,7 @@ test_clone_url () {
>  }
>
>  test_expect_success !MINGW 'clone c:temp is ssl' '
> -	test_clone_url c:temp c temp
> +	test_clone_url c:temp "-o SendEnv=GIT_PROTOCOL" c temp
>  '
>
>  test_expect_success MINGW 'clone c:temp is dos drive' '
> @@ -499,7 +499,7 @@ test_expect_success MINGW 'clone c:temp is dos drive' '
>  for repo in rep rep/home/project 123
>  do
>  	test_expect_success "clone host:$repo" '
> -		test_clone_url host:$repo host $repo
> +		test_clone_url host:$repo "-o SendEnv=GIT_PROTOCOL" host $repo
>  	'
>  done
>
> @@ -507,16 +507,16 @@ done
>  for repo in rep rep/home/project 123
>  do
>  	test_expect_success "clone [::1]:$repo" '
> -		test_clone_url [::1]:$repo ::1 "$repo"
> +		test_clone_url [::1]:$repo "-o SendEnv=GIT_PROTOCOL" ::1 "$repo"
>  	'
>  done
>  #home directory
>  test_expect_success "clone host:/~repo" '
> -	test_clone_url host:/~repo host "~repo"
> +	test_clone_url host:/~repo "-o SendEnv=GIT_PROTOCOL" host "~repo"
>  '
>
>  test_expect_success "clone [::1]:/~repo" '
> -	test_clone_url [::1]:/~repo ::1 "~repo"
> +	test_clone_url [::1]:/~repo "-o SendEnv=GIT_PROTOCOL" ::1 "~repo"
>  '
>
>  # Corner cases
> @@ -532,22 +532,22 @@ done
>  for tcol in "" :
>  do
>  	test_expect_success "clone ssh://host.xz$tcol/home/user/repo" '
> -		test_clone_url "ssh://host.xz$tcol/home/user/repo" host.xz /home/user/repo
> +		test_clone_url "ssh://host.xz$tcol/home/user/repo" "-o SendEnv=GIT_PROTOCOL" host.xz /home/user/repo
>  	'
>  	# from home directory
>  	test_expect_success "clone ssh://host.xz$tcol/~repo" '
> -	test_clone_url "ssh://host.xz$tcol/~repo" host.xz "~repo"
> +	test_clone_url "ssh://host.xz$tcol/~repo" "-o SendEnv=GIT_PROTOCOL" host.xz "~repo"
>  '
>  done
>
>  # with port number
>  test_expect_success 'clone ssh://host.xz:22/home/user/repo' '
> -	test_clone_url "ssh://host.xz:22/home/user/repo" "-p 22 host.xz" "/home/user/repo"
> +	test_clone_url "ssh://host.xz:22/home/user/repo" "-o SendEnv=GIT_PROTOCOL -p 22 host.xz" "/home/user/repo"
>  '
>
>  # from home directory with port number
>  test_expect_success 'clone ssh://host.xz:22/~repo' '
> -	test_clone_url "ssh://host.xz:22/~repo" "-p 22 host.xz" "~repo"
> +	test_clone_url "ssh://host.xz:22/~repo" "-o SendEnv=GIT_PROTOCOL -p 22 host.xz" "~repo"
>  '
>
>  #IPv6
> @@ -555,7 +555,7 @@ for tuah in ::1 [::1] [::1]: user@::1 user@[::1] user@[::1]: [user@::1] [user@::
>  do
>  	ehost=$(echo $tuah | sed -e "s/1]:/1]/" | tr -d "[]")
>  	test_expect_success "clone ssh://$tuah/home/user/repo" "
> -	  test_clone_url ssh://$tuah/home/user/repo $ehost /home/user/repo
> +	  test_clone_url ssh://$tuah/home/user/repo '-o SendEnv=GIT_PROTOCOL' $ehost /home/user/repo
>  	"
>  done
>
> @@ -564,7 +564,7 @@ for tuah in ::1 [::1] user@::1 user@[::1] [user@::1]
>  do
>  	euah=$(echo $tuah | tr -d "[]")
>  	test_expect_success "clone ssh://$tuah/~repo" "
> -	  test_clone_url ssh://$tuah/~repo $euah '~repo'
> +	  test_clone_url ssh://$tuah/~repo '-o SendEnv=GIT_PROTOCOL' $euah '~repo'
>  	"
>  done
>
> @@ -573,7 +573,7 @@ for tuah in [::1] user@[::1] [user@::1]
>  do
>  	euah=$(echo $tuah | tr -d "[]")
>  	test_expect_success "clone ssh://$tuah:22/home/user/repo" "
> -	  test_clone_url ssh://$tuah:22/home/user/repo '-p 22' $euah /home/user/repo
> +	  test_clone_url ssh://$tuah:22/home/user/repo '-o SendEnv=GIT_PROTOCOL -p 22' $euah /home/user/repo
>  	"
>  done
>
> @@ -582,7 +582,7 @@ for tuah in [::1] user@[::1] [user@::1]
>  do
>  	euah=$(echo $tuah | tr -d "[]")
>  	test_expect_success "clone ssh://$tuah:22/~repo" "
> -	  test_clone_url ssh://$tuah:22/~repo '-p 22' $euah '~repo'
> +	  test_clone_url ssh://$tuah:22/~repo '-o SendEnv=GIT_PROTOCOL -p 22' $euah '~repo'
>  	"
>  done

...so now we're unconditionally going to SendEnv=GIT_PROTOCOL to "ssh"
invocations. I don't have an issue with this, but given the change in
the commit message this seems to have snuck under the radar. You're just
talking about always including the version in server responses, nothing
about the client always needing SendEnv=GIT_PROTOCOL now even with v0.

If the server always sends the version now, why don't you need to amend
the same t5400-send-pack.sh tests as in my "tests: mark & fix tests
broken under GIT_TEST_PROTOCOL_VERSION=1"? There's one that spews out
"version" there under my GIT_TEST_PROTOCOL_VERSION=1.

I was worried about this breaking GIT_SSH_COMMAND, but then I see due to
an interaction with picking "what ssh implementation?" we don't pass "-G
-o SendEnv=GIT_PROTOCOL" at all when I have a GIT_SSH_COMMAND, but *do*
pass it to my normal /usr/bin/ssh. Is this intended? Now if I have a
GIT_SSH_COMMAND that expects to wrap openssh I need to pass "-c
ssh.variant=ssh", because "-c ssh.variant=auto" will now omit these new
arguments.