Re: [PATCH v3 10/10] ssh: introduce a 'simple' ssh variant

[Brandon Williams <bmwill@google.com>]

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 <jyasskin@google.com>
> > Signed-off-by: Brandon Williams <bmwill@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