threads / patch / 37904

patchgit_connect: allow passing arguments to ssh in GIT_SSH_ARGS

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

## tl;dr

15 messages between Nov 8, 2014 and Nov 10, 2014. Diffs are folded; open one to read it.

replies: 14people: 3as markdown or json

Thomas Quinot· Nov 8, 2014, 10:44 UTC · lore

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 <thomas@quinot.org>
---
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(-)
Show changes to 2 files +31 −2

Documentation/git.txt, connect.c

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
Jeff King· Nov 8, 2014, 11:09 UTC · re: Thomas Quinot · lore

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

On Sat, Nov 08, 2014 at 11:44:39AM +0100, Thomas Quinot wrote:
Show 6 quoted lines
> 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).

Show 5 quoted lines
> 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
Thomas Quinot· Nov 8, 2014, 12:35 UTC · re: Jeff King · lore

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

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.
Thomas Quinot· Nov 8, 2014, 14:27 UTC · re: Thomas Quinot · lore

[PATCH v2 1/2] git_connect: set ssh shell command in GIT_SSH_CMD

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 <thomas@quinot.org>
---

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(-)
Show changes to 2 files +24 −15

Documentation/git.txt, connect.c

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
Jeff King· Nov 9, 2014, 09:51 UTC · re: Thomas Quinot · lore

Re: [PATCH v2 1/2] git_connect: set ssh shell command in GIT_SSH_CMD

On Sat, Nov 08, 2014 at 03:27:53PM +0100, Thomas Quinot wrote:
Show 5 quoted lines
> 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.

Show 8 quoted lines
> 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
Thomas Quinot· Nov 9, 2014, 12:39 UTC · re: Jeff King · lore

Re: [PATCH v2 1/2] git_connect: set ssh shell command in GIT_SSH_CMD

* 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.
Thomas Quinot· Nov 9, 2014, 13:24 UTC · re: Thomas Quinot · lore

[PATCH v3] git_connect: set ssh shell command in GIT_SSH_CMD

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 <thomas@quinot.org>
---
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(-)
Show changes to 2 files +25 −15

Documentation/git.txt, connect.c

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
Junio C Hamano· Nov 9, 2014, 17:32 UTC · re: Thomas Quinot · lore

Re: [PATCH v2 1/2] git_connect: set ssh shell command in GIT_SSH_CMD

Thomas Quinot <thomas@quinot.org> writes:
> Could also be GIT_SSH_SHELLCMD, to denote that this is a *shell*
> command...

Whatever. I loathe the CMD abbreviation, though. Why spell out SHELL but not COMMAND? I.e. GIT_SSH_[SHELL_]COMMAND

Show 7 quoted lines
>> 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...)

It does not make sense to special case "plink" at the level of this mechanism where the spawned command line does not even have to invoke any "ssh" and its derivatives, so I fully agree that choosing between -P and -p should be the responsibility of whoever is using this mechanism.

Thanks.
Thomas Quinot· Nov 9, 2014, 17:47 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 1/2] git_connect: set ssh shell command in GIT_SSH_CMD

* Junio C Hamano, 2014-11-09 :
> Whatever.  I loathe the CMD abbreviation, though.  Why spell out
> SHELL but not COMMAND?  I.e. GIT_SSH_[SHELL_]COMMAND
No strong opinion :-) GIT_SSH_COMMAND looks just fine to me.
(GIT_SSH_SHELL_COMMAND starts to feel a bit long...)
 
Thomas.
Junio C Hamano· Nov 9, 2014, 17:58 UTC · re: Thomas Quinot · lore

Re: [PATCH v2 1/2] git_connect: set ssh shell command in GIT_SSH_CMD

Thomas Quinot <thomas@quinot.org> writes:
Show 7 quoted lines
> * Junio C Hamano, 2014-11-09 :
>
>> Whatever.  I loathe the CMD abbreviation, though.  Why spell out
>> SHELL but not COMMAND?  I.e. GIT_SSH_[SHELL_]COMMAND
>
> No strong opinion :-) GIT_SSH_COMMAND looks just fine to me.
> (GIT_SSH_SHELL_COMMAND starts to feel a bit long...)
Yeah I agree.
Thomas Quinot· Nov 9, 2014, 22:42 UTC · re: Junio C Hamano · lore

[PATCH v4] git_connect: set ssh shell command in GIT_SSH_COMMAND

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_COMMAND 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_COMMAND, it is deliberately left up to the user to make any required parameters adaptation before calling the underlying ssh implementation.

Signed-off-by: Thomas Quinot <thomas@quinot.org>
---

Amended patch as per discussion with Junio: change variable name to GIT_SSH_COMMAND, keep plink special circuitry disabled for this case (leaving it enabled only when using GIT_SSH, thus preserving compatibility with legacy usage).

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

Documentation/git.txt, connect.c

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 9202010..6a25ba3 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_COMMAND'::
+	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_COMMAND` 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..b8bc2ee 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_COMMAND");
+			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
Jeff King· Nov 10, 2014, 07:11 UTC · re: Thomas Quinot · lore

Re: [PATCH v4] git_connect: set ssh shell command in GIT_SSH_COMMAND

On Sun, Nov 09, 2014 at 11:42:32PM +0100, Thomas Quinot wrote:
Show 19 quoted lines
> 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_COMMAND 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_COMMAND, it is deliberately left up to
> the user to make any required parameters adaptation before calling
> the underlying ssh implementation.
> 
> Signed-off-by: Thomas Quinot <thomas@quinot.org>
> ---
> 
> Amended patch as per discussion with Junio: change variable name
> to GIT_SSH_COMMAND, keep plink special circuitry disabled for
> this case (leaving it enabled only when using GIT_SSH, thus
> preserving compatibility with legacy usage).
I think this version looks good. Thanks for working on it.

Two style micro-nits (that I do not think even merit a re-roll by themselves, but Junio may want to mark up as he applies):

> +			} else {
> +				ssh = getenv("GIT_SSH");
> +				if (!ssh) ssh = "ssh";

You did not even introduce this line, but only reindented it. However, our code style usually would write this as:

	if (!ssh)
		ssh = "ssh";
> +				putty = strcasestr(ssh, "plink") != NULL;
We would usually write this as:
	putty = !!strcasestr(ssh, "plink");
-Peff
Thomas Quinot· Nov 10, 2014, 08:28 UTC · re: Jeff King · lore

Re: [PATCH v4] git_connect: set ssh shell command in GIT_SSH_COMMAND

* Jeff King, 2014-11-10 :
> I think this version looks good. Thanks for working on it.
Thanks!
> Two style micro-nits (that I do not think even merit a re-roll by
> themselves, but Junio may want to mark up as he applies):

OK, committed locally, I can resend a PATCH v5 if that's more convenient -- do let me know.

Thomas.
Junio C Hamano· Nov 10, 2014, 16:54 UTC · re: Thomas Quinot · lore

Re: [PATCH v4] git_connect: set ssh shell command in GIT_SSH_COMMAND

Thomas Quinot <thomas@quinot.org> writes:
Show 11 quoted lines
> * Jeff King, 2014-11-10 :
>
>> I think this version looks good. Thanks for working on it.
>
> Thanks!
>
>> Two style micro-nits (that I do not think even merit a re-roll by
>> themselves, but Junio may want to mark up as he applies):
>
> OK, committed locally, I can resend a PATCH v5 if that's more
> convenient -- do let me know.
Locally tweaked while queuing.
Thanks both.
Thomas Quinot· Nov 8, 2014, 14:29 UTC · re: Thomas Quinot · lore

[PATCH v2 2/2] git_connect: when using GIT_SSH_CMD, check for plink in argv[0] only

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 <thomas@quinot.org>
---
 connect.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)
Show changes to connect.c +27 −6
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

← back to recent threads