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

[Josh Steadmon <steadmon@google.com>]

On 2018.12.14 23:39, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Dec 14 2018, Josh Steadmon wrote:
> 
> > 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 think I'm just misunderstanding this bit:
> 
>     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.

Ah yes, that is a bad description, which I will fix in V6. It should
replace "server responses" to instead be "client requests". Sorry for
the confusion, that was a really silly mistake for me to make.

Basically, in the current master, clients will not add a version
advertisement to their request if their only supported version is 0.
With this patch, they will always include a version advertisement.

> I meant that if you pick the series I have up to "tests: add a special
> setup that sets protocol.version", which is at c6f33984fc in a WIP
> branch in github.com/avar/git.git and run:
> 
>     $ GIT_TEST_PROTOCOL_VERSION=1 ./t5400-send-pack.sh -v -i -x
> 
> It'll fail with:
> 
>     + test_cmp expect refs
>     + diff -u expect refs
>     --- expect      2018-12-14 22:26:52.485411992 +0000
>     +++ refs        2018-12-14 22:26:52.713412148 +0000
>     @@ -1,3 +1,4 @@
>     +version 1
>      5285e1710002a06a379056b0d21357a999f3c642 refs/heads/master
>      5285e1710002a06a379056b0d21357a999f3c642 refs/remotes/origin/HEAD
>      5285e1710002a06a379056b0d21357a999f3c642 refs/remotes/origin/master
>     error: last command exited with $?=1
>     not ok 16 - receive-pack de-dupes .have lines

I believe the reason that my series doesn't fail on this test while
yours does with GIT_TEST_PROTOCOL_VERSION=1 is because the "fork" repo
in the test is a clone from the local filesystem. So in my case the
version negotiation differences are not visible in the trace output of
"git push" (since the advert should be in an environment variable in
this case).

> And I read your commit message to mean "v0 clients also support v1
> responses with the version in them, so let's just always send it". But I
> think this is my confusion (but I still don't know what it *really*
> means).

Yeah again, a dumb mistake in the description on my part. We are only
changing whether the client sends a version advertisement. If everything
else is equal, there should be no behavior changes apart from the
version advertisement.

> >> 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.
> 
> I'm wondering if we need to worry about some new odd interactions
> between client/servers when using GIT_SSH* wrappers, maybe not, but
> e.g. as opposed to 0da0e49ba1 ("ssh: 'auto' variant to select between
> 'ssh' and 'simple'", 2017-11-20) which noted a similar change had been
> tested at Google internally (and I read it as Google using GIT_SSH* on
> workstations) your commit doesn't make any mention of this case being
> tested / considered.
> 
> So do we need to worry about some new interaction here? Maybe not. Just
> something for people more experienced with the ssh integration to chime
> in on.

After taking a look at 0da0e49ba1 (thanks for the find) and reading
fill_ssh_args(), it seems to me that this is the intended behavior.
ssh.variant=auto means that we will run an initial "$ssh_command -G" to
see if the wrapper seems to handle OpenSSH arguments; if so we will
switch the variant to VARIANT_SSH and run again with the proper options
(including SendEnv=GIT_PROTOCOL).