Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true
- From
Jeff King <peff@peff.net>
- Date
- Apr 22, 2015, 22:00 UTC
- Message-ID
- <20150422220053.GA32268@peff.net>
- In-Reply-To
- <20150422214445.GC827497@vauxhall.crustytoothpaste.net>
On Wed, Apr 22, 2015 at 09:44:45PM +0000, brian m. carlson wrote:
Show 30 quoted lines
> On Wed, Apr 22, 2015 at 05:29:04PM -0400, Jeff King wrote:
> > > Perhaps it would be worthwhile to check instead if the text "plink" is
> > > the beginning of string or is preceded by a path separator. That would
> > > give us a bit more confidence that the user is looking for plink, but
> > > would still allow people to use "plink-0.63" if they like.
> >
> > Yeah, I think that is a reasonable approach. Note that it needs to
> > handle the "tortoiseplink" case from below, too (you can still use your
> > strategy, you just need to look for either string).
>
> So maybe something like this?
>
> diff --git a/connect.c b/connect.c
> index 391d211..ba3ab34 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -749,10 +749,15 @@ struct child_process *git_connect(int fd[2], const char *url,
> conn->use_shell = 1;
> putty = 0;
> } else {
> + char *plink, *tplink;
> +
> ssh = getenv("GIT_SSH");
> if (!ssh)
> ssh = "ssh";
> - putty = !!strcasestr(ssh, "plink");
> + plink = strcasestr(ssh, "plink");
> + tplink = strcasestr(ssh, "tortoiseplink");
> + putty = plink == ssh || (plink && is_dir_sep(plink[-1])) ||
> + tplink == ssh || (tplink && is_dir_sep(tplink[-1]));Yeah, that looks right to me. You might want to represent the "are we tortoise" check as a separate flag, though, and reuse it a few lines later.
Also, not related to your patch, but I notice the "putty" declaration is in a different scope than I would have expected, which made me wonder if it gets initialized in all code paths. I think is from the recent addition of CONNECT_DIAG_URL, which pushes the bulk of the code into its own else clause, even though the first part of the "if" always returns early. I wonder if it would be simpler to read like:
diff --git a/connect.c b/connect.c index 391d211..749a07b 100644 --- a/connect.c +++ b/connect.c @@ -743,28 +743,28 @@ struct child_process *git_connect(int fd[2], const char *url, free(path); free(conn); return NULL; - } else { - 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"); - } - - argv_array_push(&conn->args, ssh); - if (putty && !strcasestr(ssh, "tortoiseplink")) - argv_array_push(&conn->args, "-batch"); - if (port) { - /* P is for PuTTY, p is for OpenSSH */ - argv_array_push(&conn->args, putty ? "-P" : "-p"); - argv_array_push(&conn->args, port); - } - argv_array_push(&conn->args, ssh_host); } + + 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"); + } + + argv_array_push(&conn->args, ssh); + if (putty && !strcasestr(ssh, "tortoiseplink")) + argv_array_push(&conn->args, "-batch"); + if (port) { + /* P is for PuTTY, p is for OpenSSH */ + argv_array_push(&conn->args, putty ? "-P" : "-p"); + argv_array_push(&conn->args, port); + } + argv_array_push(&conn->args, ssh_host); } else { /* remove repo-local variables from the environment */ conn->env = local_repo_env; -Peff