On 10/03, Jonathan Nieder wrote: > Hi, > > Brandon Williams wrote: > > > When using the 'ssh' transport, the '-o' option is used to specify an > > environment variable which should be set on the remote end. This allows > > git to send additional information when contacting the server, > > requesting the use of a different protocol version via the > > 'GIT_PROTOCOL' environment variable like so: "-o SendEnv=GIT_PROTOCOL" > > > > Unfortunately not all ssh variants support the sending of environment > > variables to the remote end. To account for this, only use the '-o' > > option for ssh variants which are OpenSSH compliant. This is done by > > checking that the basename of the ssh command is 'ssh' or the ssh > > variant is overridden to be 'ssh' (via the ssh.variant config). > > This also affects -p (port), right?
Yeah I'll add a comment in the commit msg indicating that options like -p and -4 -6 are are only supported by some variants. > > What happens if I specify a ssh://host:port/path URL and the SSH > implementation is of 'simple' type? The port would only be sent if your ssh command supported it. > > > Previously if an ssh command's basename wasn't 'plink' or > > Git's commit messages use the present tense to describe the current > state of the code, so this is "Currently". :) I'll fix this :) > > > 'tortoiseplink' git assumed that the command was an OpenSSH variant. > > Since user configured ssh commands may not be OpenSSH compliant, tighten > > this constraint and assume a variant of 'simple' if the basename of the > > command doesn't match the variants known to git. The new ssh variant > > 'simple' will only have the host and command to execute ([username@]host > > command) passed as parameters to the ssh command. > > > > Update the Documentation to better reflect the command-line options sent > > to ssh commands based on their variant. > > > > Reported-by: Jeffrey Yasskin <jyass...@google.com> > > Signed-off-by: Brandon Williams <bmw...@google.com> > > Thanks for working on this. > > For background, the GIT_SSH implementation that motivated this is > https://github.com/travis-ci/dpl/blob/6c3fddfda1f2a85944c544446b068bac0a77c049/lib/dpl/provider.rb#L215, > which does not support -p or -4/-6, either. > > > --- > > Documentation/config.txt | 27 ++++++++++-- > > Documentation/git.txt | 9 ++-- > > connect.c | 107 > > ++++++++++++++++++++++++++--------------------- > > t/t5601-clone.sh | 9 ++-- > > t/t5700-protocol-v1.sh | 2 + > > 5 files changed, 95 insertions(+), 59 deletions(-) > [...] > > --- a/connect.c > > +++ b/connect.c > > @@ -776,37 +776,44 @@ static const char *get_ssh_command(void) > [...] > > +static enum ssh_variant determine_ssh_variant(const char *ssh_command, > > + int is_cmdline) > [...] > > - if (!strcasecmp(variant, "plink") || > > - !strcasecmp(variant, "plink.exe")) > > - *port_option = 'P'; > > + if (!strcasecmp(variant, "ssh")) > > + ssh_variant = VARIANT_SSH; > > Could this handle ssh.exe, too? Yeah I'll add the additional comparison. > > [...] > > --- a/t/t5601-clone.sh > > +++ b/t/t5601-clone.sh > > Can this get tests for the new defaulting behavior? E.g. > > - default is "simple" > - how "simple" treats an ssh://host:port/path URL > - how "simple" treats ipv4/ipv6 switching > - ssh defaults to "ssh" > - if GIT_SSH=ssh, can set ssh.variant to "simple" to force the "simple" > mode I'll look to adding a few more tests. > > One other worry: this (intentionally) changes the behavior of a > previously-working GIT_SSH=ssh-wrapper that wants to support > OpenSSH-style options but does not declare ssh.variant=ssh. When > discovering this change, what should the author of such an ssh-wrapper > do? > > They could instruct their users to set ssh.variant or GIT_SSH_VARIANT > to "ssh", but then they are at the mercy of future additional options > supported by OpenSSH we may want to start using in the future (e.g., > maybe we will start passing "--" to separate options from the > hostname). So this is not a futureproof option for them. > > They could take the new default behavior or instruct their users to > set ssh.variant or GIT_SSH_VARIANT to "simple", but then they lose > support for handling alternate ports, ipv4/ipv6, and specifying -o > SendEnv to propagate GIT_PROTOCOL or other envvars. They can handle > GIT_PROTOCOL propagation manually, but losing port support seems like > a heavy cost. > > They could send a patch to define yet another variant that is > forward-compatible, for example using an interface similar to what > git-credential(1) uses. Then they can set GIT_SSH to their > OpenSSH-style helper and GIT_FANCY_NEW_SSH to their more modern > helper, so that old Git versions could use GIT_SSH and new Git > versions could use GIT_FANCY_NEW_SSH. This might be their best > option. It feels odd to say that their only good way forward is to > send a patch, but on the other hand someone with such an itch is > likely to be in the best position to define an appropriate interface. > > They could send a documentation patch to make more promises about the > commandline used in OpenSSH mode: e.g. setting a rule in advance about > which options can take an argument so that they can properly parse an > OpenSSH command line in a future-compatible way. > > Or they could send a patch to allow passing the port in "simple" > mode, for example using an environment variable. > > Am I missing another option? What advice do we give to this person? > > Thanks, > Jonathan -- Brandon Williams
