{"thread":{"id":"14553","subject":"[PATCH v2] Ensure that SSH runs in non-interactive mode","startedAt":"2008-07-21T00:00:32Z","lastAt":"2008-07-21T13:04:09Z","messageCount":8,"participants":["Fredrik Tolf","Johannes Schindelin","Jakub Narebski","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"84106","messageId":"1216598432-18553-1-git-send-email-fredrik@dolda2000.com","threadId":"14553","inReplyTo":null,"subject":"[PATCH v2] Ensure that SSH runs in non-interactive mode","fromName":"Fredrik Tolf","fromEmail":"fredrik@dolda2000.com","sentAt":"2008-07-21T00:00:32Z","receivedAt":"2008-07-21T00:00:32Z","isPatch":true,"sender":{"key":"fredrik@dolda2000.com","avatar":null},"body":"OpenSSH has the nice feature that it sets the IP TOS value of its\nconnection depending on usage. When used in interactive mode, it\nis set to Minimize-Delay, and other wise to Maximize-Throughput. Its\nusage by Git is best served by Maximize-Throughput, for obvious\nreasons.\n\nHowever, it seems to use a DWIM heuristic for detecting interactive\nmode. The current implementation enters interactive mode if either\na PTY is allocated or X11 forwarding is enabled, and even though Git\nSSH:ing does not allocate a PTY, X11 forwarding is often turned on\nby default.\n\nThis patch allows the Git config file to specify the SSH command to\nuse and its parameters in a rather flexible manner. It should also be\nenough to configure Git to use other SSH implementations than OpenSSH.\n\nSigned-off-by: Fredrik Tolf <fredrik@dolda2000.com>\n---\n\nI'm following my previous SSH patch up with this one, which should at\nleast solve the problems discussed, and probably some more. If anything,\nit might be considered a bit overkill for the problem at hand.\n\nI assume it might have to be documented as well, if people approve of it.\n\n connect.c |  130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------\n 1 files changed, 117 insertions(+), 13 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 574f42f..46379fa 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -474,6 +474,114 @@ char *get_port(char *host)\n \treturn NULL;\n }\n \n+static char *git_ssh_command;\n+static struct {\n+\tchar *name;\n+\tchar *exp;\n+} ssh_templates[] = {\n+\t{\"openssh\", \"ssh -xT %P-p %p %h\"},\n+\t{\"plink\", \"plink %P-P %p %h\"},\n+\t{NULL, NULL}\n+};\n+\n+static int git_ssh_command_options(const char *var, const char *value, void *cb)\n+{\n+\tint i;\n+\t\n+\tif(!strcmp(var, \"core.sshcommand\")) {\n+\t\tif(git_ssh_command)\n+\t\t\treturn 0;\n+\t\tif(!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\tif(strchr(value, ' ')) {\n+\t\t\tgit_ssh_command = xstrdup(value);\n+\t\t} else {\n+\t\t\tfor(i = 0; ssh_templates[i].name; i++) {\n+\t\t\t\tif(!strcmp(ssh_templates[i].name, value)) {\n+\t\t\t\t\tgit_ssh_command = xstrdup(ssh_templates[i].exp);\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t\tif(git_ssh_command == NULL)\n+\t\t\t\tgit_ssh_command = xstrdup(value);\n+\t\t}\n+\t}\n+\t\n+\treturn git_default_config(var, value, cb);\n+}\n+\n+static char *ssh_arg_subst(char *arg, const char *host, const char *port)\n+{\n+\tif(!strncmp(arg, \"%P\", 2)) {\n+\t\tif(!port)\n+\t\t\treturn NULL;\n+\t\treturn xstrdup(arg + 2);\n+\t} else if(!strcmp(arg, \"%p\")) {\n+\t\tif(!port)\n+\t\t\treturn NULL;\n+\t\treturn xstrdup(port);\n+\t} else if(!strcmp(arg, \"%h\")) {\n+\t\treturn xstrdup(host);\n+\t}\n+\treturn arg;\n+}\n+\n+static const char **setup_ssh_command(const char *host, const char *port, int extra_args)\n+{\n+\tchar **argv;\n+\tchar *tok, *buf;\n+\tint i, sz;\n+\t\n+\tif((buf = getenv(\"GIT_SSH\")) != NULL) {\n+\t\tif(port) {\n+\t\t\targv = xcalloc(5 + extra_args, sizeof(*argv));\n+\t\t\targv[0] = xstrdup(buf);\n+\t\t\targv[1] = xstrdup(\"-p\");\n+\t\t\targv[2] = xstrdup(port);\n+\t\t\targv[3] = xstrdup(host);\n+\t\t} else {\n+\t\t\targv = xcalloc(3 + extra_args, sizeof(*argv));\n+\t\t\targv[0] = xstrdup(buf);\n+\t\t\targv[1] = xstrdup(host);\n+\t\t}\n+\t\treturn (const char **)argv;\n+\t}\n+\n+\tgit_config(git_ssh_command_options, NULL);\n+\tif(git_ssh_command == NULL)\n+\t\tbuf = xstrdup(ssh_templates[0].exp);\n+\telse\n+\t\tbuf = xstrdup(git_ssh_command);\n+\t\n+\targv = xmalloc((sz = 5) * sizeof(*argv));\n+\tfor(i = 0, tok = strtok(buf, \" \"); tok != NULL; tok = strtok(NULL, \" \")) {\n+\t\targv[i++] = xstrdup(tok);\n+\t\tif(sz - i < 1)\n+\t\t\targv = xrealloc(argv, (sz += 5) * sizeof(*argv));\n+\t}\n+\targv[i] = NULL;\n+\targv = xrealloc(argv, ((sz = i) + extra_args + 1) * sizeof(*argv));\n+\tfree(buf);\n+\t\n+\tfor(i = 0; i < sz;) {\n+\t\tbuf = ssh_arg_subst(argv[i], host, port);\n+\t\tif(buf == argv[i]) {\n+\t\t\ti++;\n+\t\t\tcontinue;\n+\t\t} else if(buf == NULL) {\n+\t\t\tfree(argv[i]);\n+\t\t\tmemmove(argv + i, argv + i + 1, sizeof(*argv) * (sz-- - i));\n+\t\t\tcontinue;\n+\t\t} else {\n+\t\t\tfree(argv[i]);\n+\t\t\targv[i] = buf;\n+\t\t\ti++;\n+\t\t}\n+\t}\n+\n+\treturn (const char **)argv;\n+}\n+\n static struct child_process no_fork;\n \n /*\n@@ -596,19 +704,12 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t\tdie(\"command line too long\");\n \n \tconn->in = conn->out = -1;\n-\tconn->argv = arg = xcalloc(6, sizeof(*arg));\n \tif (protocol == PROTO_SSH) {\n-\t\tconst char *ssh = getenv(\"GIT_SSH\");\n-\t\tif (!ssh) ssh = \"ssh\";\n-\n-\t\t*arg++ = ssh;\n-\t\tif (port) {\n-\t\t\t*arg++ = \"-p\";\n-\t\t\t*arg++ = port;\n-\t\t}\n-\t\t*arg++ = host;\n+\t\tconn->argv = setup_ssh_command(host, port, 1);\n+\t\tfor(arg = conn->argv; *arg; arg++);\n \t}\n \telse {\n+\t\tconn->argv = arg = xcalloc(6, sizeof(*arg));\n \t\t/* remove these from the environment */\n \t\tconst char *env[] = {\n \t\t\tALTERNATE_DB_ENVIRONMENT,\n@@ -620,10 +721,10 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t\t\tNULL\n \t\t};\n \t\tconn->env = env;\n-\t\t*arg++ = \"sh\";\n-\t\t*arg++ = \"-c\";\n+\t\t*arg++ = xstrdup(\"sh\");\n+\t\t*arg++ = xstrdup(\"-c\");\n \t}\n-\t*arg++ = cmd.buf;\n+\t*arg++ = xstrdup(cmd.buf);\n \t*arg = NULL;\n \n \tif (start_command(conn))\n@@ -640,11 +741,14 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \n int finish_connect(struct child_process *conn)\n {\n+\tconst char **argp;\n \tint code;\n \tif (!conn || conn == &no_fork)\n \t\treturn 0;\n \n \tcode = finish_command(conn);\n+\tfor(argp = conn->argv; *argp; argp++)\n+\t\tfree((void *)*argp);\n \tfree(conn->argv);\n \tfree(conn);\n \treturn code;\n-- \n1.5.6.2\n"},{"id":"84122","messageId":"alpine.DEB.1.00.0807210310330.3305@eeepc-johanness","threadId":"14553","inReplyTo":"1216598432-18553-1-git-send-email-fredrik@dolda2000.com","subject":"Re: [PATCH v2] Ensure that SSH runs in non-interactive mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-21T01:15:52Z","receivedAt":"2008-07-21T01:15:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 21 Jul 2008, Fredrik Tolf wrote:\n\n> I'm following my previous SSH patch up with this one, which should at\n> least solve the problems discussed, and probably some more. If anything,\n> it might be considered a bit overkill for the problem at hand.\n\nI am not assuming it is overkill, but since you do not reuse functions \nsuch as strbuf_expand() and split_cmdline(), your patch ends up pretty \nlarge.\n\nAnd since you use very short and undescriptive variable names, with ugly \nassignments inside arithmetic expressions, I will be less likely \nreviewing it in detail.\n\n> I assume it might have to be documented as well, if people approve of it.\n\nCatch 22.  Since you have not documented what %P should be useful for, \npeople might not approve of the patch, because they do not understand what\nit is supposed to do.\n\nPeople like me,\nDscho\n"},{"id":"84129","messageId":"1216604693.3673.20.camel@pc7.dolda2000.com","threadId":"14553","inReplyTo":"alpine.DEB.1.00.0807210310330.3305@eeepc-johanness","subject":"Re: [PATCH v2] Ensure that SSH runs in non-interactive mode","fromName":"Fredrik Tolf","fromEmail":"fredrik@dolda2000.com","sentAt":"2008-07-21T01:44:53Z","receivedAt":"2008-07-21T01:44:53Z","isPatch":true,"sender":{"key":"fredrik@dolda2000.com","avatar":null},"body":"On Mon, 2008-07-21 at 03:15 +0200, Johannes Schindelin wrote:\n> Hi,\n> \n> On Mon, 21 Jul 2008, Fredrik Tolf wrote:\n> \n> > I'm following my previous SSH patch up with this one, which should at\n> > least solve the problems discussed, and probably some more. If anything,\n> > it might be considered a bit overkill for the problem at hand.\n> \n> I am not assuming it is overkill, but since you do not reuse functions \n> such as strbuf_expand() and split_cmdline(), your patch ends up pretty \n> large.\n\nThanks! I didn't know that there was a split_cmdline. I will use it and\nresubmit.\n\n> And since you use very short and undescriptive variable names, with ugly \n> assignments inside arithmetic expressions, I will be less likely \n> reviewing it in detail.\n\nI actually use those assignments for clarity. IMO, it is more clear to\nhave one line which clearly initializes a buffer (and from which the\nexact details of the buffer initialization can still be read), than to\nlitter an algorithm with lots of auxiliary lines that obfuscate what the\ncode really does.\n\nSeeing how you disagree, though, I'll change that too when I'm at it.\n\n> > I assume it might have to be documented as well, if people approve of it.\n> \n> Catch 22.  Since you have not documented what %P should be useful for, \n> people might not approve of the patch, because they do not understand what\n> it is supposed to do.\n\nYes, that would be a problem. Here's some makeshift documentation:\n\nThe string specified in core.sshcommand is first checked if it matches\nany of the built-in templates, in which case it is expanded (I've added\nthe templates \"openssh\" and \"plink\" by default). When used, the string\nis split into words, each of which is processed as follows:\n\n* If a word is %p, it is replaced by the port number, if specified.\n  If the port number is not specified, the word is deleted.\n* If a word is %h, it is replaced by the remote host name.\n* If a word begins with %P, it is deleted if no port number is\n  specified. This is to allow for specifying different port number\n  flags for different SSH implementations. The syntax is a bit ugly,\n  but I cannot really think of anything that would look better.\n  If a port number has been specified, the leading %P is simply deleted.\n\nThe result is used, along with the command to run on the remote side, as\nthe SSH command line.\n\nFredrik Tolf\n"},{"id":"84159","messageId":"m3fxq3ws16.fsf@localhost.localdomain","threadId":"14553","inReplyTo":"1216604693.3673.20.camel@pc7.dolda2000.com","subject":"Re: [PATCH v2] Ensure that SSH runs in non-interactive mode","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-07-21T08:38:50Z","receivedAt":"2008-07-21T08:38:50Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Fredrik Tolf <fredrik@dolda2000.com> writes:\n\n> [...] Here's some makeshift documentation:\n> \n> The string specified in core.sshcommand is first checked if it matches\n> any of the built-in templates, in which case it is expanded (I've added\n> the templates \"openssh\" and \"plink\" by default). When used, the string\n> is split into words, each of which is processed as follows:\n> \n> * If a word is %p, it is replaced by the port number, if specified.\n>   If the port number is not specified, the word is deleted.\n> * If a word is %h, it is replaced by the remote host name.\n> * If a word begins with %P, it is deleted if no port number is\n>   specified. This is to allow for specifying different port number\n>   flags for different SSH implementations. The syntax is a bit ugly,\n>   but I cannot really think of anything that would look better.\n>   If a port number has been specified, the leading %P is simply deleted.\n\nThere is a syntax which would look better, but perhaps it is a bit\noverkill in this situation.  Namely use either shell conditional\nexpansion:\n\n  ${p:+-P $p}\n\nor syntax used in RPM spec macros\n\n  %{?p:-P %p}\n\n(and there is complementing %{!?<var>:<expansion>} in RPM spec macro\nlanguage).\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"84175","messageId":"alpine.DEB.1.00.0807211255010.3305@eeepc-johanness","threadId":"14553","inReplyTo":"m3fxq3ws16.fsf@localhost.localdomain","subject":"Re: [PATCH v2] Ensure that SSH runs in non-interactive mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-21T10:56:14Z","receivedAt":"2008-07-21T10:56:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 21 Jul 2008, Jakub Narebski wrote:\n\n> Fredrik Tolf <fredrik@dolda2000.com> writes:\n> \n> > [...] Here's some makeshift documentation:\n> > \n> > The string specified in core.sshcommand is first checked if it matches\n> > any of the built-in templates, in which case it is expanded (I've added\n> > the templates \"openssh\" and \"plink\" by default). When used, the string\n> > is split into words, each of which is processed as follows:\n> > \n> > * If a word is %p, it is replaced by the port number, if specified.\n> >   If the port number is not specified, the word is deleted.\n> > * If a word is %h, it is replaced by the remote host name.\n> > * If a word begins with %P, it is deleted if no port number is\n> >   specified. This is to allow for specifying different port number\n> >   flags for different SSH implementations. The syntax is a bit ugly,\n> >   but I cannot really think of anything that would look better.\n> >   If a port number has been specified, the leading %P is simply deleted.\n> \n> There is a syntax which would look better, but perhaps it is a bit\n> overkill in this situation.  Namely use either shell conditional\n> expansion:\n> \n>   ${p:+-P $p}\n> \n> or syntax used in RPM spec macros\n> \n>   %{?p:-P %p}\n> \n> (and there is complementing %{!?<var>:<expansion>} in RPM spec macro\n> language).\n\nYes, this is overkill.  I would even have passed the port argument \n_always_, since the port 22 for ssh is as likely to change as hell will \nnot freeze over.  Actually, I am not so sure about the latter.\n\nCiao,\nDscho\n"},{"id":"84179","messageId":"20080721110436.GA8395@sigill.intra.peff.net","threadId":"14553","inReplyTo":"alpine.DEB.1.00.0807211255010.3305@eeepc-johanness","subject":"Re: [PATCH v2] Ensure that SSH runs in non-interactive mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-07-21T11:04:36Z","receivedAt":"2008-07-21T11:04:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 21, 2008 at 12:56:14PM +0200, Johannes Schindelin wrote:\n\n> > or syntax used in RPM spec macros\n> > \n> >   %{?p:-P %p}\n> > \n> > (and there is complementing %{!?<var>:<expansion>} in RPM spec macro\n> > language).\n> \n> Yes, this is overkill.  I would even have passed the port argument \n> _always_, since the port 22 for ssh is as likely to change as hell will \n> not freeze over.  Actually, I am not so sure about the latter.\n\nBut keep in mind that \"-p 22\" on the command line _overrides_ what the\nuser has in their ssh config, so it is not a good idea to pass it all\nthe time.\n\n-Peff\n"},{"id":"84183","messageId":"alpine.DEB.1.00.0807211221310.8986@racer","threadId":"14553","inReplyTo":"20080721110436.GA8395@sigill.intra.peff.net","subject":"Re: [PATCH v2] Ensure that SSH runs in non-interactive mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-21T11:22:20Z","receivedAt":"2008-07-21T11:22:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 21 Jul 2008, Jeff King wrote:\n\n> On Mon, Jul 21, 2008 at 12:56:14PM +0200, Johannes Schindelin wrote:\n> \n> > > or syntax used in RPM spec macros\n> > > \n> > >   %{?p:-P %p}\n> > > \n> > > (and there is complementing %{!?<var>:<expansion>} in RPM spec macro\n> > > language).\n> > \n> > Yes, this is overkill.  I would even have passed the port argument \n> > _always_, since the port 22 for ssh is as likely to change as hell will \n> > not freeze over.  Actually, I am not so sure about the latter.\n> \n> But keep in mind that \"-p 22\" on the command line _overrides_ what the\n> user has in their ssh config, so it is not a good idea to pass it all\n> the time.\n\nOh, good point.  So we might need something distasteful as what Jakub \nproposed... Hrmpf.\n\nCiao,\nDscho\n"},{"id":"84190","messageId":"1216645452.3673.29.camel@pc7.dolda2000.com","threadId":"14553","inReplyTo":"alpine.DEB.1.00.0807211221310.8986@racer","subject":"Re: [PATCH v2] Ensure that SSH runs in non-interactive mode","fromName":"Fredrik Tolf","fromEmail":"fredrik@dolda2000.com","sentAt":"2008-07-21T13:04:09Z","receivedAt":"2008-07-21T13:04:09Z","isPatch":true,"sender":{"key":"fredrik@dolda2000.com","avatar":null},"body":"On Mon, 2008-07-21 at 12:22 +0100, Johannes Schindelin wrote:\n> Hi,\n> \n> On Mon, 21 Jul 2008, Jeff King wrote:\n> \n> > On Mon, Jul 21, 2008 at 12:56:14PM +0200, Johannes Schindelin wrote:\n> > \n> > > > or syntax used in RPM spec macros\n> > > > \n> > > >   %{?p:-P %p}\n> > > > \n> > > > (and there is complementing %{!?<var>:<expansion>} in RPM spec macro\n> > > > language).\n> > > \n> > > Yes, this is overkill.  I would even have passed the port argument \n> > > _always_, since the port 22 for ssh is as likely to change as hell will \n> > > not freeze over.  Actually, I am not so sure about the latter.\n> > \n> > But keep in mind that \"-p 22\" on the command line _overrides_ what the\n> > user has in their ssh config, so it is not a good idea to pass it all\n> > the time.\n> \n> Oh, good point.  So we might need something distasteful as what Jakub \n> proposed... Hrmpf.\n\nTwo suggestions:\n\n * Use %P as I suggested. I know it is kind of ugly, but I might still\n   consider it less ugly than adding full shell-style substitution to\n   the code. Especially considering that it would almost only have to be\n   used inside of Git. Users outside of the Git source would only very\n   seldomly have to touch it. (It would essentially only be those users\n   who both use alternative SSH ports *and* has an non-standard default\n   in their ssh_config *and* use a custom SSH command...)\n * Put the SSH port number and host name in the environment and call\n   \"/bin/sh -c\" with the textual concatenation of the SSH command and\n   the command that Git wants to call. While a bit more ugly than\n   handling word splitting internally, I don't really think that it has\n   any real ill effects, seeing how the Git command is word-split on the\n   remote side anyhow (I think that has to be SSH's most stupid\n   \"feature\").\n\nFredrik Tolf\n"}]}