[PATCH v2] Ensure that SSH runs in non-interactive mode

OpenSSH has the nice feature that it sets the IP TOS value of its
connection depending on usage. When used in interactive mode, it
is set to Minimize-Delay, and other wise to Maximize-Throughput. Its
usage by Git is best served by Maximize-Throughput, for obvious
reasons.

However, it seems to use a DWIM heuristic for detecting interactive
mode. The current implementation enters interactive mode if either
a PTY is allocated or X11 forwarding is enabled, and even though Git
SSH:ing does not allocate a PTY, X11 forwarding is often turned on
by default.

This patch allows the Git config file to specify the SSH command to
use and its parameters in a rather flexible manner. It should also be
enough to configure Git to use other SSH implementations than OpenSSH.

Signed-off-by: Fredrik Tolf <redacted>
---

I'm following my previous SSH patch up with this one, which should at
least solve the problems discussed, and probably some more. If anything,
it might be considered a bit overkill for the problem at hand.

I assume it might have to be documented as well, if people approve of it.

 connect.c |  130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 117 insertions(+), 13 deletions(-)

diff --git a/connect.c b/connect.c
index 574f42f..46379fa 100644
--- a/connect.c
+++ b/connect.c

@@ -474,6 +474,114 @@ char *get_port(char *host)
 	return NULL;
 }
 
+static char *git_ssh_command;
+static struct {
+	char *name;
+	char *exp;
+} ssh_templates[] = {
+	{"openssh", "ssh -xT %P-p %p %h"},
+	{"plink", "plink %P-P %p %h"},
+	{NULL, NULL}
+};
+
+static int git_ssh_command_options(const char *var, const char *value, void *cb)
+{
+	int i;
+	
+	if(!strcmp(var, "core.sshcommand")) {
+		if(git_ssh_command)
+			return 0;
+		if(!value)
+			return config_error_nonbool(var);
+		if(strchr(value, ' ')) {
+			git_ssh_command = xstrdup(value);
+		} else {
+			for(i = 0; ssh_templates[i].name; i++) {
+				if(!strcmp(ssh_templates[i].name, value)) {
+					git_ssh_command = xstrdup(ssh_templates[i].exp);
+					break;
+				}
+			}
+			if(git_ssh_command == NULL)
+				git_ssh_command = xstrdup(value);
+		}
+	}
+	
+	return git_default_config(var, value, cb);
+}
+
+static char *ssh_arg_subst(char *arg, const char *host, const char *port)
+{
+	if(!strncmp(arg, "%P", 2)) {
+		if(!port)
+			return NULL;
+		return xstrdup(arg + 2);
+	} else if(!strcmp(arg, "%p")) {
+		if(!port)
+			return NULL;
+		return xstrdup(port);
+	} else if(!strcmp(arg, "%h")) {
+		return xstrdup(host);
+	}
+	return arg;
+}
+
+static const char **setup_ssh_command(const char *host, const char *port, int extra_args)
+{
+	char **argv;
+	char *tok, *buf;
+	int i, sz;
+	
+	if((buf = getenv("GIT_SSH")) != NULL) {
+		if(port) {
+			argv = xcalloc(5 + extra_args, sizeof(*argv));
+			argv[0] = xstrdup(buf);
+			argv[1] = xstrdup("-p");
+			argv[2] = xstrdup(port);
+			argv[3] = xstrdup(host);
+		} else {
+			argv = xcalloc(3 + extra_args, sizeof(*argv));
+			argv[0] = xstrdup(buf);
+			argv[1] = xstrdup(host);
+		}
+		return (const char **)argv;
+	}
+
+	git_config(git_ssh_command_options, NULL);
+	if(git_ssh_command == NULL)
+		buf = xstrdup(ssh_templates[0].exp);
+	else
+		buf = xstrdup(git_ssh_command);
+	
+	argv = xmalloc((sz = 5) * sizeof(*argv));
+	for(i = 0, tok = strtok(buf, " "); tok != NULL; tok = strtok(NULL, " ")) {
+		argv[i++] = xstrdup(tok);
+		if(sz - i < 1)
+			argv = xrealloc(argv, (sz += 5) * sizeof(*argv));
+	}
+	argv[i] = NULL;
+	argv = xrealloc(argv, ((sz = i) + extra_args + 1) * sizeof(*argv));
+	free(buf);
+	
+	for(i = 0; i < sz;) {
+		buf = ssh_arg_subst(argv[i], host, port);
+		if(buf == argv[i]) {
+			i++;
+			continue;
+		} else if(buf == NULL) {
+			free(argv[i]);
+			memmove(argv + i, argv + i + 1, sizeof(*argv) * (sz-- - i));
+			continue;
+		} else {
+			free(argv[i]);
+			argv[i] = buf;
+			i++;
+		}
+	}
+
+	return (const char **)argv;
+}
+
 static struct child_process no_fork;
 
 /*

@@ -596,19 +704,12 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 		die("command line too long");
 
 	conn->in = conn->out = -1;
-	conn->argv = arg = xcalloc(6, sizeof(*arg));
 	if (protocol == PROTO_SSH) {
-		const char *ssh = getenv("GIT_SSH");
-		if (!ssh) ssh = "ssh";
-
-		*arg++ = ssh;
-		if (port) {
-			*arg++ = "-p";
-			*arg++ = port;
-		}
-		*arg++ = host;
+		conn->argv = setup_ssh_command(host, port, 1);
+		for(arg = conn->argv; *arg; arg++);
 	}
 	else {
+		conn->argv = arg = xcalloc(6, sizeof(*arg));
 		/* remove these from the environment */
 		const char *env[] = {
 			ALTERNATE_DB_ENVIRONMENT,

@@ -620,10 +721,10 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 			NULL
 		};
 		conn->env = env;
-		*arg++ = "sh";
-		*arg++ = "-c";
+		*arg++ = xstrdup("sh");
+		*arg++ = xstrdup("-c");
 	}
-	*arg++ = cmd.buf;
+	*arg++ = xstrdup(cmd.buf);
 	*arg = NULL;
 
 	if (start_command(conn))

@@ -640,11 +741,14 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 
 int finish_connect(struct child_process *conn)
 {
+	const char **argp;
 	int code;
 	if (!conn || conn == &no_fork)
 		return 0;
 
 	code = finish_command(conn);
+	for(argp = conn->argv; *argp; argp++)
+		free((void *)*argp);
 	free(conn->argv);
 	free(conn);
 	return code;

-- 
1.5.6.2

Hi,

On Mon, 21 Jul 2008, Fredrik Tolf wrote:

I'm following my previous SSH patch up with this one, which should at
least solve the problems discussed, and probably some more. If anything,
it might be considered a bit overkill for the problem at hand.

I am not assuming it is overkill, but since you do not reuse functions 
such as strbuf_expand() and split_cmdline(), your patch ends up pretty 
large.

And since you use very short and undescriptive variable names, with ugly 
assignments inside arithmetic expressions, I will be less likely 
reviewing it in detail.

I assume it might have to be documented as well, if people approve of it.

Catch 22.  Since you have not documented what %P should be useful for, 
people might not approve of the patch, because they do not understand what
it is supposed to do.

People like me,
Dscho

On Mon, 2008-07-21 at 03:15 +0200, Johannes Schindelin wrote:

Hi,

On Mon, 21 Jul 2008, Fredrik Tolf wrote:

quoted

I'm following my previous SSH patch up with this one, which should at
least solve the problems discussed, and probably some more. If anything,
it might be considered a bit overkill for the problem at hand.

I am not assuming it is overkill, but since you do not reuse functions 
such as strbuf_expand() and split_cmdline(), your patch ends up pretty 
large.

Thanks! I didn't know that there was a split_cmdline. I will use it and
resubmit.

And since you use very short and undescriptive variable names, with ugly 
assignments inside arithmetic expressions, I will be less likely 
reviewing it in detail.

I actually use those assignments for clarity. IMO, it is more clear to
have one line which clearly initializes a buffer (and from which the
exact details of the buffer initialization can still be read), than to
litter an algorithm with lots of auxiliary lines that obfuscate what the
code really does.

Seeing how you disagree, though, I'll change that too when I'm at it.

quoted

I assume it might have to be documented as well, if people approve of it.

Catch 22.  Since you have not documented what %P should be useful for, 
people might not approve of the patch, because they do not understand what
it is supposed to do.

Yes, that would be a problem. Here's some makeshift documentation:

The string specified in core.sshcommand is first checked if it matches
any of the built-in templates, in which case it is expanded (I've added
the templates "openssh" and "plink" by default). When used, the string
is split into words, each of which is processed as follows:

* If a word is %p, it is replaced by the port number, if specified.
  If the port number is not specified, the word is deleted.
* If a word is %h, it is replaced by the remote host name.
* If a word begins with %P, it is deleted if no port number is
  specified. This is to allow for specifying different port number
  flags for different SSH implementations. The syntax is a bit ugly,
  but I cannot really think of anything that would look better.
  If a port number has been specified, the leading %P is simply deleted.

The result is used, along with the command to run on the remote side, as
the SSH command line.

Fredrik Tolf

Fredrik Tolf [off-list ref] writes:

[...] Here's some makeshift documentation:

The string specified in core.sshcommand is first checked if it matches
any of the built-in templates, in which case it is expanded (I've added
the templates "openssh" and "plink" by default). When used, the string
is split into words, each of which is processed as follows:

* If a word is %p, it is replaced by the port number, if specified.
  If the port number is not specified, the word is deleted.
* If a word is %h, it is replaced by the remote host name.
* If a word begins with %P, it is deleted if no port number is
  specified. This is to allow for specifying different port number
  flags for different SSH implementations. The syntax is a bit ugly,
  but I cannot really think of anything that would look better.
  If a port number has been specified, the leading %P is simply deleted.

There is a syntax which would look better, but perhaps it is a bit
overkill in this situation.  Namely use either shell conditional
expansion:

  ${p:+-P $p}

or syntax used in RPM spec macros

  %{?p:-P %p}

(and there is complementing %{!?<var>:<expansion>} in RPM spec macro
language).
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Hi,

On Mon, 21 Jul 2008, Jakub Narebski wrote:

Fredrik Tolf [off-list ref] writes:

quoted

[...] Here's some makeshift documentation:

The string specified in core.sshcommand is first checked if it matches
any of the built-in templates, in which case it is expanded (I've added
the templates "openssh" and "plink" by default). When used, the string
is split into words, each of which is processed as follows:

* If a word is %p, it is replaced by the port number, if specified.
  If the port number is not specified, the word is deleted.
* If a word is %h, it is replaced by the remote host name.
* If a word begins with %P, it is deleted if no port number is
  specified. This is to allow for specifying different port number
  flags for different SSH implementations. The syntax is a bit ugly,
  but I cannot really think of anything that would look better.
  If a port number has been specified, the leading %P is simply deleted.

There is a syntax which would look better, but perhaps it is a bit
overkill in this situation.  Namely use either shell conditional
expansion:

  ${p:+-P $p}

or syntax used in RPM spec macros

  %{?p:-P %p}

(and there is complementing %{!?<var>:<expansion>} in RPM spec macro
language).

Yes, this is overkill.  I would even have passed the port argument 
_always_, since the port 22 for ssh is as likely to change as hell will 
not freeze over.  Actually, I am not so sure about the latter.

Ciao,
Dscho

On Mon, Jul 21, 2008 at 12:56:14PM +0200, Johannes Schindelin wrote:

quoted

or syntax used in RPM spec macros

  %{?p:-P %p}

(and there is complementing %{!?<var>:<expansion>} in RPM spec macro
language).

Yes, this is overkill.  I would even have passed the port argument 
_always_, since the port 22 for ssh is as likely to change as hell will 
not freeze over.  Actually, I am not so sure about the latter.

But keep in mind that "-p 22" on the command line _overrides_ what the
user has in their ssh config, so it is not a good idea to pass it all
the time.

-Peff

Hi,

On Mon, 21 Jul 2008, Jeff King wrote:

On Mon, Jul 21, 2008 at 12:56:14PM +0200, Johannes Schindelin wrote:

quoted

quoted

or syntax used in RPM spec macros

  %{?p:-P %p}

(and there is complementing %{!?<var>:<expansion>} in RPM spec macro
language).

Yes, this is overkill.  I would even have passed the port argument 
_always_, since the port 22 for ssh is as likely to change as hell will 
not freeze over.  Actually, I am not so sure about the latter.

But keep in mind that "-p 22" on the command line _overrides_ what the
user has in their ssh config, so it is not a good idea to pass it all
the time.

Oh, good point.  So we might need something distasteful as what Jakub 
proposed... Hrmpf.

Ciao,
Dscho

On Mon, 2008-07-21 at 12:22 +0100, Johannes Schindelin wrote:

Hi,

On Mon, 21 Jul 2008, Jeff King wrote:

quoted

On Mon, Jul 21, 2008 at 12:56:14PM +0200, Johannes Schindelin wrote:

quoted

quoted

or syntax used in RPM spec macros

  %{?p:-P %p}

(and there is complementing %{!?<var>:<expansion>} in RPM spec macro
language).

Yes, this is overkill.  I would even have passed the port argument 
_always_, since the port 22 for ssh is as likely to change as hell will 
not freeze over.  Actually, I am not so sure about the latter.

But keep in mind that "-p 22" on the command line _overrides_ what the
user has in their ssh config, so it is not a good idea to pass it all
the time.

Oh, good point.  So we might need something distasteful as what Jakub 
proposed... Hrmpf.

Two suggestions:

 * Use %P as I suggested. I know it is kind of ugly, but I might still
   consider it less ugly than adding full shell-style substitution to
   the code. Especially considering that it would almost only have to be
   used inside of Git. Users outside of the Git source would only very
   seldomly have to touch it. (It would essentially only be those users
   who both use alternative SSH ports *and* has an non-standard default
   in their ssh_config *and* use a custom SSH command...)
 * Put the SSH port number and host name in the environment and call
   "/bin/sh -c" with the textual concatenation of the SSH command and
   the command that Git wants to call. While a bit more ugly than
   handling word splitting internally, I don't really think that it has
   any real ill effects, seeing how the Git command is word-split on the
   remote side anyhow (I think that has to be SSH's most stupid
   "feature").

Fredrik Tolf