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

[Josh Steadmon <steadmon@google.com>]

On 2018.12.14 21:20, Ævar Arnfjörð Bjarmason wrote:
> 
> 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.

Ack. I'll make sure that V6 calls this out in the commit message.


> 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.

Sorry if I'm being dense, but can you point out more specifically what
is wrong here? I don't see anything in t5400 that would be affected by
this; the final test case ("receive-pack de-dupes .have lines") is the
only one that does any tracing, and it strips out everything that's not
a ref advertisement before checking the output. Sorry if I'm missing
something obvious.


> 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.

I am not an expert on this part of the code, but it seems to be
intended. Later on in the function, there are BUG()s that state that
VARIANT_AUTO should not be passed to the push_ssh_options() function.
And this only changes the behavior when version=0. For protocol versions
1 & 2, VARIANT_AUTO never had SendEnv=GIT_PROTOCOL added to the command
line. Again, sorry if I'm missing some implication here.


Thanks,
Josh