Re: [PATCH v3 10/10] ssh: introduce a 'simple' ssh variant
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 > > Signed-off-by: Brandon Williams > > Thanks for working on this. > > For background, the GIT_SSH implementation that motivated this is > https://github.com/travis-ci/dpl/blob/6c3fddfda1f2a85944c56b068bac0a77c049/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 th
Re: [PATCH v3 10/10] ssh: introduce a 'simple' ssh variant
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? What happens if I specify a ssh://host:port/path URL and the SSH implementation is of 'simple' type? > 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". :) > '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 > Signed-off-by: Brandon Williams Thanks for working on this. For background, the GIT_SSH implementation that motivated this is https://github.com/travis-ci/dpl/blob/6c3fddfda1f2a85944c56b068bac0a77c049/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? [...] > --- 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 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 d
[PATCH v3 10/10] ssh: introduce a 'simple' ssh variant
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).
Previously if an ssh command's basename wasn't 'plink' or
'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
Signed-off-by: Brandon Williams
---
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(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index b78747abc..0460af37e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2084,12 +2084,31 @@ ssh.variant::
Depending on the value of the environment variables `GIT_SSH` or
`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
auto-detects whether to adjust its command-line parameters for use
- with plink or tortoiseplink, as opposed to the default (OpenSSH).
+ with ssh (OpenSSH), plink or tortoiseplink, as opposed to the default
+ (simple).
+
The config variable `ssh.variant` can be set to override this auto-detection;
-valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
-will be treated as normal ssh. This setting can be overridden via the
-environment variable `GIT_SSH_VARIANT`.
+valid values are `ssh`, `simple`, `plink`, `putty` or `tortoiseplink`. Any
+other value will be treated as normal ssh. This setting can be overridden via
+the environment variable `GIT_SSH_VARIANT`.
++
+The current command-line parameters used for each variant are as
+follows:
++
+--
+
+* `ssh` - [-p port] [-4] [-6] [-o option] [username@]host command
+
+* `simple` - [username@]host command
+
+* `plink` or `putty` - [-P port] [-4] [-6] [username@]host command
+
+* `tortoiseplink` - [-P port] [-4] [-6] -batch [username@]host command
+
+--
++
+Except for the `simple` variant, command-line parameters are likely to
+change as git gains new features.
i18n.commitEncoding::
Character encoding the commit messages are stored in; Git itself
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7518ea3af..8bc3f2147 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -518,11 +518,10 @@ other
If either of these environment variables is set then 'git fetch'
and 'git push' will use the specified command instead of 'ssh'
when they need to connect to a remote system.
- The command will be given exactly two or four arguments: the
- 'username@host' (or just 'host') from the URL and the shell
- command to execute on that remote system, optionally preceded by
- `-p` (literally) and the 'port' from the URL when it specifies
- something other than the default SSH port.
+ The command-line parameters passed to the configured command are
+ determined by the ssh variant. See `ssh.variant` option in
+ linkgit:git-config[1] for details.
+
+
`$GIT_SSH_COMMAND` takes precedence over `$GIT_SSH`, and is interpreted
by the shell, which allows additional arguments to be included.
diff --git a/connect.c b/connect.c
index b8695a2fa..65cee49b6 100644
--- a/connect.c
+++ b/connect.c
@@ -776,37 +776,44 @@ static const char *get_ssh_command(void)
return NULL;
}
-static int override_ssh_variant(int *port_option, int *needs_batch)
+enum ssh_variant {
+ VARIANT_SIMPLE,
+ VARIANT_SSH,
+ VARIANT_PLINK,
+ VARIANT_PUTTY,
+ VARIANT_TORTOISEPLINK,
+};
+
+static int override_ssh_variant(enum ssh_variant *ssh_variant)
{
- char *variant;
+ const char *variant = getenv("GIT_SSH_VARIANT");
- variant = xstrdup_or_null(getenv("GIT_SSH_VARIANT"));
- if (!variant &&
- git_config_get_string("ssh.variant", &variant))
+ if (!variant && git_config_get_
