[PATCH] git_connect: allow passing arguments to ssh in GIT_SSH_ARGS

It may be impractical to install a wrapper script for ssh
when additional parameters need to be passed. Provide an
alternative way of specifying these by means of the GIT_SSH_ARGS
environment variable. Arguments are whitespace delimited following
usual shell conventions; embedded whitespace can be enclosed
in quotes, or escaped with a backslash.

Signed-off-by: Thomas Quinot <redacted>
---

Dear fellow GIT developers,

I hope I won't stray too far away from established procedures
with my first contribution to git. This patch adds support
for a GIT_SSH_ARGS environment variable, providing a way
of specifying ssh arguments without having to create a
wrapper script.

Thomas.

 Documentation/git.txt | 11 +++++++++--
 connect.c             | 22 ++++++++++++++++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 9202010..3ac7b5b 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt

@@ -887,13 +887,20 @@ other
 	than the default SSH port.
 +
 To pass options to the program that you want to list in GIT_SSH
-you will need to wrap the program and options into a shell script,
-then set GIT_SSH to refer to the shell script.
+you can either wrap the program and options into a shell script,
+then set GIT_SSH to refer to the shell script, or use the
+GIT_SSH_ARGS environment variable (see below).
 +
 Usually it is easier to configure any desired options through your
 personal `.ssh/config` file.  Please consult your ssh documentation
 for further details.
 
+'GIT_SSH_ARGS'::
+	This environment variables provides additional arguments to be
+	passed to GIT_SSH. Arguments are split by spaces, the usual shell
+	quoting and escaping is supported. Quote pairs or a backslash can
+	be used to quote them.
+
 'GIT_ASKPASS'::
 	If this environment variable is set, then Git commands which need to
 	acquire passwords or passphrases (e.g. for HTTP or IMAP authentication)

diff --git a/connect.c b/connect.c
index d47d0ec..3d4b182 100644
--- a/connect.c
+++ b/connect.c

@@ -701,9 +701,15 @@ struct child_process *git_connect(int fd[2], const char *url,
 		conn->in = conn->out = -1;
 		if (protocol == PROTO_SSH) {
 			const char *ssh = getenv("GIT_SSH");
+			const char *ssh_args_env = getenv("GIT_SSH_ARGS");
+			char *ssh_args = ssh_args_env ?
+				xstrdup(ssh_args_env) : NULL;
 			int putty = ssh && strcasestr(ssh, "plink");
 			char *ssh_host = hostandport;
 			const char *port = NULL;
+			const char **argv;
+			int argc;
+
 			get_host_and_port(&ssh_host, &port);
 			port = get_port_numeric(port);
 

@@ -717,6 +723,22 @@ struct child_process *git_connect(int fd[2], const char *url,
 				argv_array_push(&conn->args, putty ? "-P" : "-p");
 				argv_array_push(&conn->args, port);
 			}
+
+			if (ssh_args) {
+				argc = split_cmdline(ssh_args, &argv);
+				if (argc < 0) {
+					free(ssh_args);
+					die("invalid GIT_SSH_ARGS '%s': %s",
+					    ssh_args_env,
+					    split_cmdline_strerror(argc));
+				}
+
+				while (argc--)
+					argv_array_push(&conn->args, *argv++);
+				free(ssh_args);
+				free(argv);
+			}
+
 			argv_array_push(&conn->args, ssh_host);
 		} else {
 			/* remove repo-local variables from the environment */

-- 
1.9.2

On Sat, Nov 08, 2014 at 11:44:39AM +0100, Thomas Quinot wrote:

It may be impractical to install a wrapper script for ssh
when additional parameters need to be passed. Provide an
alternative way of specifying these by means of the GIT_SSH_ARGS
environment variable. Arguments are whitespace delimited following
usual shell conventions; embedded whitespace can be enclosed
in quotes, or escaped with a backslash.

This has bugged me before, too. Almost every sub-program invoked by git
is done so using a shell, so you can put in arbitrary arguments or even
snippets of shell code. But not GIT_SSH.

I think the original reason was to match other programs with similar
variables. At this point, we can't just change it on a whim; the quoting
requirements are different and it would break people's setups. So your
approach of adding a new variable is good (the other option is figuring
out a long-term transition plan).

However, I'm not sure adding a separate variable for options is the best
we can do. Besides being a bit clunky, it has two big shortcomings:

  1. It's not consistent with other git variables that use the shell
     (e.g., GIT_PAGER).

  2. It's not as flexible as a shell; you can't specify "$HOME/bin/ssh"
     or other even more esoteric things, like dynamically tweaking
     the options based on the environment.

What do you think of adding an alternate variable that is not ssh
_arguments_, but rather just a full shell command for running ssh?
I'm not sure what it could be called (GIT_SSH_SH is probably too
confusing).

I hope I won't stray too far away from established procedures
with my first contribution to git. This patch adds support
for a GIT_SSH_ARGS environment variable, providing a way
of specifying ssh arguments without having to create a
wrapper script.

The formatting and whatnot of your submission looks fine to me. The
patch itself looks reasonable, though I didn't look too closely after
making my comments above. :)

-Peff

Jeff,

Thanks for your feedback!

* Jeff King, 2014-11-08 :

What do you think of adding an alternate variable that is not ssh
_arguments_, but rather just a full shell command for running ssh?
I'm not sure what it could be called (GIT_SSH_SH is probably too
confusing).

Interesting idea, I had not thought of this, but I agree that it looks
nicer and more flexible from the user's point of view. In addition, it
turns out that it makes the implementation even simpler! I suggest
'GIT_SSH_CMD' for the variable name; patch v2 coming...

Thomas.

It may be impractical to install a wrapper script for GIT_SSH
when additional parameters need to be passed. Provide an alternative
way of specifying a shell command to be run, including command line
arguments, by means of the GIT_SSH_CMD environment variable, which
behaves like GIT_SSH but is passed to the shell.

Signed-off-by: Thomas Quinot <redacted>
---

As suggested by Jeff, here is a second version introducing a GIT_SSH_CMD
variable that overrides GIT_SSH, and is processed by the shell.

Note that with this first patch only, the special processing for PuTTY/plink
looks at the whole command in that case. I deliberately left it that way,
even though one might imagine a case where this would cause a false positive,
e.g. if the user's login name includes the string 'plink':
   GIT_SSH_CMD="ssh -l fooplink"

The work-around in this case would be to write:
   GIT_SSH_CMD="ssh -l foop''link"

A second patch, coming in a followup message, pre-splits $GIT_SSH_CMD
and ensures that we look for "plink"/"tortoiseplink" only in the argv[0]
element.

Thomas.

 Documentation/git.txt | 26 ++++++++++++++------------
 connect.c             | 13 ++++++++++---
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 9202010..519711d 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt

@@ -876,19 +876,21 @@ other
 	and the `core.editor` option in linkgit:git-config[1].
 
 'GIT_SSH'::
-	If this environment variable is set then 'git fetch'
-	and 'git push' will use this command instead
-	of 'ssh' when they need to connect to a remote system.
-	The '$GIT_SSH' 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.
+'GIT_SSH_CMD'::
+	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.
 +
-To pass options to the program that you want to list in GIT_SSH
-you will need to wrap the program and options into a shell script,
-then set GIT_SSH to refer to the shell script.
+`$GIT_SSH_CMD` takes precedence over `$GIT_SSH`, and is interpreted
+by the shell, which allows additional arguments to be included.
+`$GIT_SSH` on the other hand must be just the path to a program
+(which can be a wrapper shell script, if additional arguments are
+needed).
 +
 Usually it is easier to configure any desired options through your
 personal `.ssh/config` file.  Please consult your ssh documentation

diff --git a/connect.c b/connect.c
index d47d0ec..ecb1821 100644
--- a/connect.c
+++ b/connect.c

@@ -700,16 +700,23 @@ struct child_process *git_connect(int fd[2], const char *url,
 
 		conn->in = conn->out = -1;
 		if (protocol == PROTO_SSH) {
-			const char *ssh = getenv("GIT_SSH");
-			int putty = ssh && strcasestr(ssh, "plink");
+			const char *ssh;
+			int putty;
 			char *ssh_host = hostandport;
 			const char *port = NULL;
 			get_host_and_port(&ssh_host, &port);
 			port = get_port_numeric(port);
 
-			if (!ssh) ssh = "ssh";
+			ssh = getenv("GIT_SSH_CMD");
+			if (ssh)
+				conn->use_shell = 1;
+			else {
+				ssh = getenv("GIT_SSH");
+				if (!ssh) ssh = "ssh";
+			}
 
 			argv_array_push(&conn->args, ssh);
+			putty = strcasestr(ssh, "plink");
 			if (putty && !strcasestr(ssh, "tortoiseplink"))
 				argv_array_push(&conn->args, "-batch");
 			if (port) {

-- 
2.1.2

git_connect includes circuitry to use specific command line arguments
when using PuTTY's plink. This circuitry is enabled based on the
presence of the "plink" and "tortoiseplink" substrings in the name
of the SSH program to use. When using GIT_SSH_CMD to specify a complete
shell command to run for ssh connections, look for these substrings only
in the program name (not in subsequent arguments).

Signed-off-by: Thomas Quinot <redacted>
---
 connect.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/connect.c b/connect.c
index ecb1821..fcb928a 100644
--- a/connect.c
+++ b/connect.c

@@ -700,22 +700,42 @@ struct child_process *git_connect(int fd[2], const char *url,
 
 		conn->in = conn->out = -1;
 		if (protocol == PROTO_SSH) {
-			const char *ssh;
-			int putty;
+			const char *ssh_cmd, *ssh;
+			char *ssh_cmd_var = NULL;
+			char *putty;
 			char *ssh_host = hostandport;
 			const char *port = NULL;
 			get_host_and_port(&ssh_host, &port);
 			port = get_port_numeric(port);
 
-			ssh = getenv("GIT_SSH_CMD");
-			if (ssh)
+			ssh_cmd = getenv("GIT_SSH_CMD");
+			if (ssh_cmd) {
+				/*
+				 * Split command line to check for plink in
+				 * ssh_argv[0].
+				 */
+				const char **ssh_argv;
+				ssh_cmd_var = xstrdup(ssh_cmd);
+				int ssh_argc =
+				    split_cmdline(ssh_cmd_var, &ssh_argv);
+				if (ssh_argv < 0) {
+					free(ssh_argv);
+					free(ssh_cmd_var);
+					die("invalid GIT_SSH_CMD '%s': %s",
+					    ssh,
+					    split_cmdline_strerror(ssh_argc));
+				}
+				ssh = ssh_argv[0];
+				free(ssh_argv);
+
 				conn->use_shell = 1;
-			else {
+				argv_array_push(&conn->args, ssh_cmd);
+			} else {
 				ssh = getenv("GIT_SSH");
 				if (!ssh) ssh = "ssh";
+				argv_array_push(&conn->args, ssh);
 			}
 
-			argv_array_push(&conn->args, ssh);
 			putty = strcasestr(ssh, "plink");
 			if (putty && !strcasestr(ssh, "tortoiseplink"))
 				argv_array_push(&conn->args, "-batch");

@@ -725,6 +745,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 				argv_array_push(&conn->args, port);
 			}
 			argv_array_push(&conn->args, ssh_host);
+			if (ssh_cmd_var) free(ssh_cmd_var);
 		} else {
 			/* remove repo-local variables from the environment */
 			conn->env = local_repo_env;

-- 
2.1.2

On Sat, Nov 08, 2014 at 03:27:53PM +0100, Thomas Quinot wrote:

It may be impractical to install a wrapper script for GIT_SSH
when additional parameters need to be passed. Provide an alternative
way of specifying a shell command to be run, including command line
arguments, by means of the GIT_SSH_CMD environment variable, which
behaves like GIT_SSH but is passed to the shell.

Thanks, I like this much better. The name GIT_SSH_CMD is not too bad.
Personally, of the two (GIT_SSH and GIT_SSH_CMD) I would expect the
"_CMD" form to be the one that does not use the shell. But I do not
really have a better suggestion for the name, so perhaps it's OK.

Note that with this first patch only, the special processing for PuTTY/plink
looks at the whole command in that case. I deliberately left it that way,
even though one might imagine a case where this would cause a false positive,
e.g. if the user's login name includes the string 'plink':
   GIT_SSH_CMD="ssh -l fooplink"

The work-around in this case would be to write:
   GIT_SSH_CMD="ssh -l foop''link"

I'd be tempted to say that the "plink" magic does not need to kick in at
all for GIT_SSH_CMD. There are simply too many corner cases in trying to
guess what the shell is going to run. For example:

A second patch, coming in a followup message, pre-splits $GIT_SSH_CMD
and ensures that we look for "plink"/"tortoiseplink" only in the argv[0]
element.

I don't think you can pre-split accurately. Since we promise to pass the
result to the shell, the user gets to write whatever they want. Even:

  GIT_SSH_CMD='f() {
    if test "$(hostname)" = "some-machine"; then
      some-special-ssh "$@"
    else
      ssh "$@"
  }
  f'

Parsing complications aside, you cannot even know in git which ssh is
going to be run until the shell code is executed. I think either we have
to leave the responsibility for munging "-p" into "-P" on the side of
the user's shell snippet (and remember that they can still use GIT_SSH
as usual, so we are not making anything _worse_ for them here), or we
have to provide some unambiguous way for them to signal which calling
convention we want ($GIT_SSH_CMD_IS_PLINK or something).

-Peff

* Jeff King, 2014-11-09 :

Thanks, I like this much better. The name GIT_SSH_CMD is not too bad.
Personally, of the two (GIT_SSH and GIT_SSH_CMD) I would expect the
"_CMD" form to be the one that does not use the shell.

Right, except of course we're stuck with the compatibility issue in any
case.

But I do not really have a better suggestion for the name, so perhaps
it's OK.

Could also be GIT_SSH_SHELLCMD, to denote that this is a *shell*
command...

Parsing complications aside, you cannot even know in git which ssh is
going to be run until the shell code is executed. I think either we have
to leave the responsibility for munging "-p" into "-P" on the side of
the user's shell snippet

That sounds like a reasonable approach, and leaves us with the simpler
change (that splitting circuitry was admittely a bit awkward...)

Thomas.

It may be impractical to install a wrapper script for GIT_SSH
when additional parameters need to be passed. Provide an alternative
way of specifying a shell command to be run, including command line
arguments, by means of the GIT_SSH_CMD environment variable, which
behaves like GIT_SSH but is passed to the shell.

The special circuitry to modify parameters in the case of using
PuTTY's plink/tortoiseplink is activated only when using GIT_SSH;
in the case of using GIT_SSH_CMD, it is deliberately left up to the
user to make any required adaptation before calling the underlying
ssh implementation.

Signed-off-by: Thomas Quinot <redacted>
---
Third version: disabling the complicated plink circuitry altogether
when using GIT_SSH_CMD, as discussed. This keeps the change
very simple, which is nice.

Keepin GIT_SSH_CMD for the variable name for the time being, as
I'm not very enthusiastic with GIT_SSH_SHELLCMD (more explicit,
but too long for my taste). No strong opinion on that point
though.

Thomas.

 Documentation/git.txt | 26 ++++++++++++++------------
 connect.c             | 14 +++++++++++---
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 9202010..519711d 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt

@@ -876,19 +876,21 @@ other
 	and the `core.editor` option in linkgit:git-config[1].
 
 'GIT_SSH'::
-	If this environment variable is set then 'git fetch'
-	and 'git push' will use this command instead
-	of 'ssh' when they need to connect to a remote system.
-	The '$GIT_SSH' 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.
+'GIT_SSH_CMD'::
+	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.
 +
-To pass options to the program that you want to list in GIT_SSH
-you will need to wrap the program and options into a shell script,
-then set GIT_SSH to refer to the shell script.
+`$GIT_SSH_CMD` takes precedence over `$GIT_SSH`, and is interpreted
+by the shell, which allows additional arguments to be included.
+`$GIT_SSH` on the other hand must be just the path to a program
+(which can be a wrapper shell script, if additional arguments are
+needed).
 +
 Usually it is easier to configure any desired options through your
 personal `.ssh/config` file.  Please consult your ssh documentation

diff --git a/connect.c b/connect.c
index d47d0ec..7fe5268 100644
--- a/connect.c
+++ b/connect.c

@@ -700,14 +700,22 @@ struct child_process *git_connect(int fd[2], const char *url,
 
 		conn->in = conn->out = -1;
 		if (protocol == PROTO_SSH) {
-			const char *ssh = getenv("GIT_SSH");
-			int putty = ssh && strcasestr(ssh, "plink");
+			const char *ssh;
+			int putty;
 			char *ssh_host = hostandport;
 			const char *port = NULL;
 			get_host_and_port(&ssh_host, &port);
 			port = get_port_numeric(port);
 
-			if (!ssh) ssh = "ssh";
+			ssh = getenv("GIT_SSH_CMD");
+			if (ssh) {
+				conn->use_shell = 1;
+				putty = 0;
+			} else {
+				ssh = getenv("GIT_SSH");
+				if (!ssh) ssh = "ssh";
+				putty = strcasestr(ssh, "plink") != NULL;
+			}
 
 			argv_array_push(&conn->args, ssh);
 			if (putty && !strcasestr(ssh, "tortoiseplink"))

-- 
2.1.2