Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true

[Jeff King <peff@peff.net>]

On Wed, Apr 22, 2015 at 09:44:45PM +0000, brian m. carlson wrote:

> 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