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@xxxxxxxxxxxxxxxxx/ > 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
