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

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

[Patrick Sharp]

The plink string detection in GIT_SSH for setting putty to true is very broad.

If plink is anywhere in the path to the shell file then putty gets set to true and ssh will fail trying to parse -batch as the hostname.

Wouldn’t searching for plink.exe be better?

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

[Johannes Schindelin]

Hi Patrick,

On 2015-04-22 16:36, Patrick Sharp wrote:
> The plink string detection in GIT_SSH for setting putty to true is very broad.

Wow. You probably wanted to state that you are using Windows, downloaded Git from [link here], that you are using [version] and that you use PLink [version] (from the Putty package downloaded [link here]) to do your ssh business.

Without that information, you leave readers who have no idea about Putty *quite* puzzled.

> If plink is anywhere in the path to the shell file then putty gets set
> to true and ssh will fail trying to parse -batch as the hostname.

This is cryptic even for me.

> Wouldn’t searching for plink.exe be better?--

I invite you to try your hand at improving anything you find flawed. For example, if you want to improve the PLink detection in Git for Windows 1.x, this would be the correct place to start: https://github.com/msysgit/msysgit/blob/70f24b4b0f5f86a5e85f7264a4ee2c0fec2d4391/share/WinGit/install.iss#L232-L253 (yes, you would have to download the development environment from https://msysgit.github.com/#download-msysgit and rebuild your own installer using `/share/msysGit/WinGit/release.sh 1.9.5-patrick` after editing the installer script).

Ciao,
Johannes

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

[Patrick Sharp]

Johannes,

You’re correct, looking back over it, I was pretty vague.

In truth, we are not using Windows OR putty at all.  Running git on an Ubuntu system, but we are setting the GIT_SSH environment variable to point to a shell script to use.

Upon attempting to run git ls-remote, the system was spitting out getaddrinfo errors for ‘atch’ .

Setting GIT_TRACE=2 showed that -batch was being added to the git command.

This was seen on several different servers with git versions 1.8.5.2, 1.9.1 and 2.3.5

After a bit we realized that it was the string ‘uplink’ in the GIT_SSH variable that was linked to the extra -batch flag.

Finally, after searching the git source, we narrowed it down to the ‘plink’ portion of the string.

https://github.com/git/git/blob/7c597ef345aed345576de616c51f27e6f4b342b3/connect.c#L747-L756


> On Apr 22, 2015, at 12:46 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> 
> Hi Patrick,
> 
> On 2015-04-22 16:36, Patrick Sharp wrote:
>> The plink string detection in GIT_SSH for setting putty to true is very broad.
> 
> Wow. You probably wanted to state that you are using Windows, downloaded Git from [link here], that you are using [version] and that you use PLink [version] (from the Putty package downloaded [link here]) to do your ssh business.
> 
> Without that information, you leave readers who have no idea about Putty *quite* puzzled.
> 
>> If plink is anywhere in the path to the shell file then putty gets set
>> to true and ssh will fail trying to parse -batch as the hostname.
> 
> This is cryptic even for me.
> 
>> Wouldn’t searching for plink.exe be better?--
> 
> I invite you to try your hand at improving anything you find flawed. For example, if you want to improve the PLink detection in Git for Windows 1.x, this would be the correct place to start: https://github.com/msysgit/msysgit/blob/70f24b4b0f5f86a5e85f7264a4ee2c0fec2d4391/share/WinGit/install.iss#L232-L253 (yes, you would have to download the development environment from https://msysgit.github.com/#download-msysgit and rebuild your own installer using `/share/msysGit/WinGit/release.sh 1.9.5-patrick` after editing the installer script).
> 
> Ciao,
> Johannes

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

[Jeff King]

On Wed, Apr 22, 2015 at 02:12:53PM -0500, Patrick Sharp wrote:

> Johannes,
> 
> You’re correct, looking back over it, I was pretty vague.
> 
> In truth, we are not using Windows OR putty at all.  Running git on an Ubuntu system, but we are setting the GIT_SSH environment variable to point to a shell script to use.
> 
> Upon attempting to run git ls-remote, the system was spitting out getaddrinfo errors for ‘atch’ .
> 
> Setting GIT_TRACE=2 showed that -batch was being added to the git command.
> 
> This was seen on several different servers with git versions 1.8.5.2, 1.9.1 and 2.3.5
> 
> After a bit we realized that it was the string ‘uplink’ in the GIT_SSH variable that was linked to the extra -batch flag.
> 
> Finally, after searching the git source, we narrowed it down to the ‘plink’ portion of the string.
> 
> https://github.com/git/git/blob/7c597ef345aed345576de616c51f27e6f4b342b3/connect.c#L747-L756

I think you want something like:

diff --git a/connect.c b/connect.c
index 9ae991a..58aad56 100644
--- a/connect.c
+++ b/connect.c
@@ -568,7 +568,8 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 	conn->argv = arg = xcalloc(7, sizeof(*arg));
 	if (protocol == PROTO_SSH) {
 		const char *ssh = getenv("GIT_SSH");
-		int putty = ssh && strcasestr(ssh, "plink");
+		int putty = ssh && (ends_with(ssh, "plink") ||
+				    ends_with("plink.exe"));
 		if (!ssh) ssh = "ssh";
 
 		*arg++ = ssh;

though that is not quite enough (we do not have a case-insensitive
version of "ends_with"). I'm also not sure if matching just "plink" and
"plink.exe" at the end of the string is enough (I'm just guessing that
was the original reason for using strstr in the first place).

Note that I don't think just switching the strcasestr to look for
"plink.exe" is right. For one thing, it just punts on the problem (it
can still happen, it's just less likely to trigger). But for another,
you can have plink (without ".exe") on Linux systems.

-Peff

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1675 bytes --]

On Wed, Apr 22, 2015 at 04:29:10PM -0400, Jeff King wrote:
> I think you want something like:
> 
> diff --git a/connect.c b/connect.c
> index 9ae991a..58aad56 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -568,7 +568,8 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
>  	conn->argv = arg = xcalloc(7, sizeof(*arg));
>  	if (protocol == PROTO_SSH) {
>  		const char *ssh = getenv("GIT_SSH");
> -		int putty = ssh && strcasestr(ssh, "plink");
> +		int putty = ssh && (ends_with(ssh, "plink") ||
> +				    ends_with("plink.exe"));
>  		if (!ssh) ssh = "ssh";
>  
>  		*arg++ = ssh;
> 
> though that is not quite enough (we do not have a case-insensitive
> version of "ends_with"). I'm also not sure if matching just "plink" and
> "plink.exe" at the end of the string is enough (I'm just guessing that
> was the original reason for using strstr in the first place).
> 
> Note that I don't think just switching the strcasestr to look for
> "plink.exe" is right. For one thing, it just punts on the problem (it
> can still happen, it's just less likely to trigger). But for another,
> you can have plink (without ".exe") on Linux systems.

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.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Jeff King]

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

> > Note that I don't think just switching the strcasestr to look for
> > "plink.exe" is right. For one thing, it just punts on the problem (it
> > can still happen, it's just less likely to trigger). But for another,
> > you can have plink (without ".exe") on Linux systems.
> 
> 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).

-Peff

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1466 bytes --]

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]));
 				}
 
 				argv_array_push(&conn->args, ssh);
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Jeff King]

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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2678 bytes --]

On Wed, Apr 22, 2015 at 06:00:54PM -0400, Jeff King wrote:
> 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.

Sounds like a good idea.  I'll send a more formal patch a bit later
today.

> 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;

I can drop this in as a preparatory patch if I can have your sign-off.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Jeff King]

On Wed, Apr 22, 2015 at 10:24:55PM +0000, brian m. carlson wrote:

> On Wed, Apr 22, 2015 at 06:00:54PM -0400, Jeff King wrote:
> > 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.
> 
> Sounds like a good idea.  I'll send a more formal patch a bit later
> today.

Thanks.

> > 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:
> [...]
> 
> I can drop this in as a preparatory patch if I can have your sign-off.

Definitely, thanks.

Signed-off-by: Jeff King <peff@peff.net>

-Peff

[PATCH 1/2] connect: simplify SSH connection code path

[brian m. carlson]

The code path used in git_connect pushed the majority of the SSH
connection code into an else block, even though the if block returns.
Simplify the code by eliminating the else block, as it is unneeded.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 connect.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

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;
-- 
2.3.5

[PATCH 2/2] connect: improve check for plink to reduce false positives

[brian m. carlson]

The git_connect function has code to handle plink and tortoiseplink
specially, as they require different command line arguments from
OpenSSH.  However, the match was done by checking for "plink"
case-insensitively in the string, which led to false positives when
GIT_SSH contained "uplink".  Improve the check by looking for "plink" or
"tortoiseplink" only at the beginning of the string or immediately after
a directory separator.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
This is essentially just a deindentation.

 connect.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/connect.c b/connect.c
index 749a07b..bae76a2 100644
--- a/connect.c
+++ b/connect.c
@@ -724,7 +724,7 @@ 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;
+			int putty, tortoiseplink = 0;
 			char *ssh_host = hostandport;
 			const char *port = NULL;
 			get_host_and_port(&ssh_host, &port);
@@ -750,14 +750,23 @@ 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");
+
+				tortoiseplink = tplink == ssh ||
+					(tplink && is_dir_sep(tplink[-1]));
+				putty = tortoiseplink || plink == ssh ||
+					(plink && is_dir_sep(plink[-1]));
 			}
 
 			argv_array_push(&conn->args, ssh);
-			if (putty && !strcasestr(ssh, "tortoiseplink"))
+			if (tortoiseplink)
 				argv_array_push(&conn->args, "-batch");
 			if (port) {
 				/* P is for PuTTY, p is for OpenSSH */
-- 
2.3.5

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

[Torsten Bögershausen]

On 04/22/2015 09:12 PM, Patrick Sharp wrote:
> Johannes,
>
> You’re correct, looking back over it, I was pretty vague.
>
> In truth, we are not using Windows OR putty at all.  Running git on an Ubuntu system, but we are setting the GIT_SSH environment variable to point to a shell script to use.
>
> Upon attempting to run git ls-remote, the system was spitting out getaddrinfo errors for ‘atch’ .
>
> Setting GIT_TRACE=2 showed that -batch was being added to the git command.
>
> This was seen on several different servers with git versions 1.8.5.2, 1.9.1 and 2.3.5
>
> After a bit we realized that it was the string ‘uplink’ in the GIT_SSH variable that was linked to the extra -batch flag.
>
> Finally, after searching the git source, we narrowed it down to the ‘plink’ portion of the string.
>
> https://github.com/git/git/blob/7c597ef345aed345576de616c51f27e6f4b342b3/connect.c#L747-L7
Brian, I got your patch,
but can't see it in the list yet
  1/2 looks good, thanks.
(And add msygit)

My feeling is that  patch 2/2 may break things for an unknown
amount of users which e.g. use "myplink".

Patrick,
did you ever tell us, what you put into $GIT_SSH ?

Can your use case be covered by using $GIT_SSH_COMMAND ?

Re: [PATCH 2/2] connect: improve check for plink to reduce false positives

[Johannes Schindelin]

Hi Brian,

On 2015-04-23 02:06, brian m. carlson wrote:

> +				tortoiseplink = tplink == ssh ||
> +					(tplink && is_dir_sep(tplink[-1]));

Maybe have a helper function here? Something like `basename_matches(const char *path, const char *basename, int ignore_case)`? That would be easier to read (I have to admit that I had to wrap my head around the logic to ensure that tplink[-1] is valid; It is, but it requires more brain cycles to verify than I would like).

Also, I am really hesitant to just test the start of the basename; I would rather have an entire basename match so that something  like "PLinKoeln" would not match. (And of course, for Windows I would want that hypothetical `basename_matches()` function to allow basenames to end in `.exe` automatically).

In contrast to Torsten, I am not so concerned about `myplink` scripts: that only affects power users who can easily add the `-batch` into the script, where it actually belongs.

Ciao,
Dscho

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

[Patrick Sharp]

Torsten,

The relevant part of the path in GIT_SSH was ‘/uplink_deploy/‘.

I did begin to use GIT_SSH_COMMAND as a workaround, but regardless this still feels like an overly broad way to determine the value of the putty flag.

I was kind of surprised to find it being inferred from the value of GIT_SSH instead of being explicitly set by some additional variable.  GIT_USE_PUTTY or some such, though I can understand there may be some reluctance to put the onus of that on the end user.

Part of the issue stemmed from not being able to find any documentation on this.  After I discovered what was happening I found plenty of instructions that indicated to enable putty support set GIT_SSH to /path/to/plink.exe, but I didn’t find it stated anywhere that if ‘plink’ was found in GIT_SSH, then git will add the -batch option to the command args.  In other words, I was able to find instructions on what to do if we had been using putty, but not instructions on unexpected behavior if using GIT_SHH while NOT using putty.


> On Apr 23, 2015, at 12:08 AM, Torsten Bögershausen <tboegi@web.de> wrote:
> 
> On 04/22/2015 09:12 PM, Patrick Sharp wrote:
>> Johannes,
>> 
>> You’re correct, looking back over it, I was pretty vague.
>> 
>> In truth, we are not using Windows OR putty at all.  Running git on an Ubuntu system, but we are setting the GIT_SSH environment variable to point to a shell script to use.
>> 
>> Upon attempting to run git ls-remote, the system was spitting out getaddrinfo errors for ‘atch’ .
>> 
>> Setting GIT_TRACE=2 showed that -batch was being added to the git command.
>> 
>> This was seen on several different servers with git versions 1.8.5.2, 1.9.1 and 2.3.5
>> 
>> After a bit we realized that it was the string ‘uplink’ in the GIT_SSH variable that was linked to the extra -batch flag.
>> 
>> Finally, after searching the git source, we narrowed it down to the ‘plink’ portion of the string.
>> 
>> https://github.com/git/git/blob/7c597ef345aed345576de616c51f27e6f4b342b3/connect.c#L747-L7
> Brian, I got your patch,
> but can't see it in the list yet
> 1/2 looks good, thanks.
> (And add msygit)
> 
> My feeling is that  patch 2/2 may break things for an unknown
> amount of users which e.g. use "myplink".
> 
> Patrick,
> did you ever tell us, what you put into $GIT_SSH ?
> 
> Can your use case be covered by using $GIT_SSH_COMMAND ?
> 
> 
> 
> 
> 

Re: [PATCH 2/2] connect: improve check for plink to reduce false positives

[Jeff King]

On Thu, Apr 23, 2015 at 08:50:17AM +0200, Johannes Schindelin wrote:

> > +				tortoiseplink = tplink == ssh ||
> > +					(tplink && is_dir_sep(tplink[-1]));
> 
> Maybe have a helper function here? Something like
> `basename_matches(const char *path, const char *basename, int
> ignore_case)`? That would be easier to read (I have to admit that I
> had to wrap my head around the logic to ensure that tplink[-1] is
> valid; It is, but it requires more brain cycles to verify than I would
> like).

Yeah, I had a similar thought when reading the patch.

> Also, I am really hesitant to just test the start of the basename; I
> would rather have an entire basename match so that something  like
> "PLinKoeln" would not match. (And of course, for Windows I would want
> that hypothetical `basename_matches()` function to allow basenames to
> end in `.exe` automatically).

What about "plink-0.83" that was mentioned earlier in the thread? I
think that is the reason brian's patch stuck to matching the start and
not the end. But I have no idea if that is actually a real thing, or
just a hypothetical.

If I were writing from scratch, I would probably keep things as tight as
possible, like:

  const char *base = basename(ssh);
  plink = !strcasecmp(base, "plink") ||
          !strcasecmp(base, "plink.exe");
  tplink = !strcasecmp(base, "tortoiseplink") ||
           !strcasecmp(base, "tortoiseplink.exe"));

but maybe that is too tight at this point in time; we don't really know
what's out there and working (or maybe _we_ do, but _I_ do not :) ).

At any rate, brian's patch only looks for a dir-separator anywhere, not
the actual basename. So:

  /path/to/plink/ssh

would match, and I'm not sure if that's a good thing or not. So yet
another variant is to use basename(), and then just check that the
basename starts with "plink" (to catch "plink.exe", "plink-0.83", etc).
That avoids cruft in the intermediate path, and unless your actual
binary is named PlinKoeln, it will not false positive on the example you
gave.

-Peff

Re: [PATCH 2/2] connect: improve check for plink to reduce false positives

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1776 bytes --]

On Thu, Apr 23, 2015 at 11:53:04AM -0400, Jeff King wrote:
> On Thu, Apr 23, 2015 at 08:50:17AM +0200, Johannes Schindelin wrote:
> 
> > > +				tortoiseplink = tplink == ssh ||
> > > +					(tplink && is_dir_sep(tplink[-1]));
> > 
> > Maybe have a helper function here? Something like
> > `basename_matches(const char *path, const char *basename, int
> > ignore_case)`? That would be easier to read (I have to admit that I
> > had to wrap my head around the logic to ensure that tplink[-1] is
> > valid; It is, but it requires more brain cycles to verify than I would
> > like).
> 
> Yeah, I had a similar thought when reading the patch.

I was questioning whether a comment would have been helpful.  Apparently
the answer was yes.

> If I were writing from scratch, I would probably keep things as tight as
> possible, like:
> 
>   const char *base = basename(ssh);
>   plink = !strcasecmp(base, "plink") ||
>           !strcasecmp(base, "plink.exe");
>   tplink = !strcasecmp(base, "tortoiseplink") ||
>            !strcasecmp(base, "tortoiseplink.exe"));
> 
> but maybe that is too tight at this point in time; we don't really know
> what's out there and working (or maybe _we_ do, but _I_ do not :) ).
> 
> At any rate, brian's patch only looks for a dir-separator anywhere, not
> the actual basename. So:
> 
>   /path/to/plink/ssh
> 
> would match, and I'm not sure if that's a good thing or not.

This is true.  In hindsight, I think it's probably better to be a bit
stricter, so I'll reroll to use the stricter format.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] connect: improve check for plink to reduce false positives

[Johannes Schindelin]

Hi Peff,

On 2015-04-23 17:53, Jeff King wrote:

> What about "plink-0.83" that was mentioned earlier in the thread?

I was working a lot with Java projects where the base name is often considered to be the part before a version number that is encoded into the file name, so I think it would be a good idea here, too. Essentially, it would be the regex

    ^(.*[/\\])?plink(-.*|\.exe)$

... and maybe we should actually do exactly this, use a regex? I know that many developers try to stay away from them, but if you can read them, they are an efficient encoding of expectations.

Ciao,
Dscho

Re: [PATCH 2/2] connect: improve check for plink to reduce false positives

[Johannes Schindelin]

Hi Brian,

On 2015-04-24 01:14, brian m. carlson wrote:
> On Thu, Apr 23, 2015 at 11:53:04AM -0400, Jeff King wrote:
>
>> If I were writing from scratch, I would probably keep things as tight as
>> possible, like:
>>
>>   const char *base = basename(ssh);
>>   plink = !strcasecmp(base, "plink") ||
>>           !strcasecmp(base, "plink.exe");
>>   tplink = !strcasecmp(base, "tortoiseplink") ||
>>            !strcasecmp(base, "tortoiseplink.exe"));
>>
>> but maybe that is too tight at this point in time; we don't really know
>> what's out there and working (or maybe _we_ do, but _I_ do not :) ).
>>
>> At any rate, brian's patch only looks for a dir-separator anywhere, not
>> the actual basename. So:
>>
>>   /path/to/plink/ssh
>>
>> would match, and I'm not sure if that's a good thing or not.
> 
> This is true.  In hindsight, I think it's probably better to be a bit
> stricter, so I'll reroll to use the stricter format.

Thank you so much!
Dscho

[PATCH v2 1/2] connect: simplify SSH connection code path

[brian m. carlson]

The code path used in git_connect pushed the majority of the SSH
connection code into an else block, even though the if block returns.
Simplify the code by eliminating the else block, as it is unneeded.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 connect.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

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;
-- 
2.3.5

[PATCH v2 2/2] connect: improve check for plink to reduce false positives

[brian m. carlson]

The git_connect function has code to handle plink and tortoiseplink
specially, as they require different command line arguments from
OpenSSH.  However, the match was done by checking for "plink"
case-insensitively in the string, which led to false positives when
GIT_SSH contained "uplink".  Improve the check by looking for "plink" or
"tortoiseplink" (or those names suffixed with ".exe") in the final
component of the path.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 connect.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/connect.c b/connect.c
index 749a07b..c0144d8 100644
--- a/connect.c
+++ b/connect.c
@@ -724,7 +724,7 @@ 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;
+			int putty, tortoiseplink = 0;
 			char *ssh_host = hostandport;
 			const char *port = NULL;
 			get_host_and_port(&ssh_host, &port);
@@ -750,14 +750,26 @@ struct child_process *git_connect(int fd[2], const char *url,
 				conn->use_shell = 1;
 				putty = 0;
 			} else {
+				const char *base;
+				char *ssh_dup;
+
 				ssh = getenv("GIT_SSH");
 				if (!ssh)
 					ssh = "ssh";
-				putty = !!strcasestr(ssh, "plink");
+
+				ssh_dup = xstrdup(ssh);
+				base = basename(ssh_dup);
+
+				tortoiseplink = !strcasecmp(base, "tortoiseplink") ||
+					!strcasecmp(base, "tortoiseplink.exe");
+				putty = !strcasecmp(base, "plink") ||
+					!strcasecmp(base, "plink.exe") || tortoiseplink;
+
+				free(ssh_dup);
 			}
 
 			argv_array_push(&conn->args, ssh);
-			if (putty && !strcasestr(ssh, "tortoiseplink"))
+			if (tortoiseplink)
 				argv_array_push(&conn->args, "-batch");
 			if (port) {
 				/* P is for PuTTY, p is for OpenSSH */
-- 
2.3.5

Re: [PATCH v2 2/2] connect: improve check for plink to reduce false positives

[Pete Harlan]

On Fri, Apr 24, 2015 at 3:28 PM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> The git_connect function has code to handle plink and tortoiseplink
> specially, as they require different command line arguments from
> OpenSSH.  However, the match was done by checking for "plink"
> case-insensitively in the string, which led to false positives when

Perhaps s/case-insensitively/anywhere/?

It's not important that it ignored case (your change ignores it too),
it's that it was too lenient about its context.

--Pete

> GIT_SSH contained "uplink".  Improve the check by looking for "plink" or
> "tortoiseplink" (or those names suffixed with ".exe") in the final
> component of the path.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  connect.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/connect.c b/connect.c
> index 749a07b..c0144d8 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -724,7 +724,7 @@ 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;
> +                       int putty, tortoiseplink = 0;
>                         char *ssh_host = hostandport;
>                         const char *port = NULL;
>                         get_host_and_port(&ssh_host, &port);
> @@ -750,14 +750,26 @@ struct child_process *git_connect(int fd[2], const char *url,
>                                 conn->use_shell = 1;
>                                 putty = 0;
>                         } else {
> +                               const char *base;
> +                               char *ssh_dup;
> +
>                                 ssh = getenv("GIT_SSH");
>                                 if (!ssh)
>                                         ssh = "ssh";
> -                               putty = !!strcasestr(ssh, "plink");
> +
> +                               ssh_dup = xstrdup(ssh);
> +                               base = basename(ssh_dup);
> +
> +                               tortoiseplink = !strcasecmp(base, "tortoiseplink") ||
> +                                       !strcasecmp(base, "tortoiseplink.exe");
> +                               putty = !strcasecmp(base, "plink") ||
> +                                       !strcasecmp(base, "plink.exe") || tortoiseplink;
> +
> +                               free(ssh_dup);
>                         }
>
>                         argv_array_push(&conn->args, ssh);
> -                       if (putty && !strcasestr(ssh, "tortoiseplink"))
> +                       if (tortoiseplink)
>                                 argv_array_push(&conn->args, "-batch");
>                         if (port) {
>                                 /* P is for PuTTY, p is for OpenSSH */

Re: [PATCH v2 2/2] connect: improve check for plink to reduce false positives

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 940 bytes --]

On Fri, Apr 24, 2015 at 03:46:30PM -0700, Pete Harlan wrote:
> On Fri, Apr 24, 2015 at 3:28 PM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > The git_connect function has code to handle plink and tortoiseplink
> > specially, as they require different command line arguments from
> > OpenSSH.  However, the match was done by checking for "plink"
> > case-insensitively in the string, which led to false positives when
> 
> Perhaps s/case-insensitively/anywhere/?
>
> It's not important that it ignored case (your change ignores it too),
> it's that it was too lenient about its context.

Yes, I don't know what I was thinking.  I'll wait a bit to see if there
are any more comments and then reroll.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 2/2] connect: improve check for plink to reduce false positives

[Torsten Bögershausen]

On 2015-04-25 00.28, brian m. carlson wrote:
> The git_connect function has code to handle plink and tortoiseplink
> specially, as they require different command line arguments from
> OpenSSH.  However, the match was done by checking for "plink"
> case-insensitively in the string, which led to false positives when
> GIT_SSH contained "uplink".  Improve the check by looking for "plink" or
> "tortoiseplink" (or those names suffixed with ".exe") in the final
> component of the path.
> 
(I'm not sute if the commit message describes the problem deep enough
for readers which are not familar with all the details of the original
report):
A feature implemented for Windows may break things for e.g. Linux users)
The following may read exaggerated, so please read it as a suggestion.

The git_connect function has code to handle plink and tortoiseplink
specially, as they require different command line arguments compared to
OpenSSH (-P instead of -p, tortoiseplink uses -batch), commit 36ad53ffee6ed.
The special handling is only needed for Windows, and a sloppy
case-insensitve search for "plink" will trigger that an the extra
parameter "-batch" is added to the command line.

This was observed on a Linux system where a command line including
"/uplink_deploy/" was used.

There are different ways to improve the situation:
(The following mentions only plink, but assumes that tortoiseplink is handled
 similar)
a) Disable the plink/tortoiseplink special handling on non-Windows systems
b) Tighten the search for plink:
   Allow basename() == plink || !!strcasestr(ssh, "plink.exe")
c) Tighten the search for plink:
   Allow basename() == plink || !!strcasestr(ssh, "plink.exe")
d) Tighten the check for tortoiseplink.
   Today we set "int putty" to true when plink is found, and -batch
   is set when tortoiseplink is not found.
   This fixes the reported bug, but still has the -P problem.
e) Unix users typically use shell scripts and could use GIT_SSH_COMMAND.
   Declare the GIT_SSH as not-well-documented (and to obsolete ?) for non-Windows systems,


This patch implements c):
Extract the basename and compare it to plink, plink.exe respective
tortoiseplink/tortoiseplink.exe

Note that there is a slight risk of breakage for Windows users:
Strings like "myplink" or "plink-0.83" are no longer accepted.

-------------
I would probably vote for a), as Unix/Linux/Mac OS users don't use plink/tortoiseplink
at all. 
-------------
What about adding test-cases in t5601,
this will ease the documentation later.
f:/util/plink
/c/util/plink.exe
f:/util/tortoiseplink
/c/util/tortoiseplink.exe
/usr/local/uplink/sshwrapper.sh


Other opinions, other thoughts ?

Re: [PATCH v2 2/2] connect: improve check for plink to reduce false positives

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3026 bytes --]

On Sat, Apr 25, 2015 at 06:03:22PM +0200, Torsten Bögershausen wrote:
> (I'm not sute if the commit message describes the problem deep enough
> for readers which are not familar with all the details of the original
> report):
> A feature implemented for Windows may break things for e.g. Linux users)
> The following may read exaggerated, so please read it as a suggestion.
> 
> The git_connect function has code to handle plink and tortoiseplink
> specially, as they require different command line arguments compared to
> OpenSSH (-P instead of -p, tortoiseplink uses -batch), commit 36ad53ffee6ed.
> The special handling is only needed for Windows, and a sloppy
> case-insensitve search for "plink" will trigger that an the extra
> parameter "-batch" is added to the command line.
> 
> This was observed on a Linux system where a command line including
> "/uplink_deploy/" was used.
> 
> There are different ways to improve the situation:
> (The following mentions only plink, but assumes that tortoiseplink is handled
>  similar)
> a) Disable the plink/tortoiseplink special handling on non-Windows systems
> b) Tighten the search for plink:
>    Allow basename() == plink || !!strcasestr(ssh, "plink.exe")
> c) Tighten the search for plink:
>    Allow basename() == plink || !!strcasestr(ssh, "plink.exe")
> d) Tighten the check for tortoiseplink.
>    Today we set "int putty" to true when plink is found, and -batch
>    is set when tortoiseplink is not found.
>    This fixes the reported bug, but still has the -P problem.
> e) Unix users typically use shell scripts and could use GIT_SSH_COMMAND.
>    Declare the GIT_SSH as not-well-documented (and to obsolete ?) for non-Windows systems,
> 
> This patch implements c):
> Extract the basename and compare it to plink, plink.exe respective
> tortoiseplink/tortoiseplink.exe
> 
> Note that there is a slight risk of breakage for Windows users:
> Strings like "myplink" or "plink-0.83" are no longer accepted.

I can certainly expand the commit message when I reroll.

> -------------
> I would probably vote for a), as Unix/Linux/Mac OS users don't use plink/tortoiseplink
> at all.

I have putty on my system:

vauxhall ok % uname -a && which plink
Linux vauxhall 3.16.0-4-amd64 #1 SMP Debian 3.16.7-ckt7-1 (2015-03-01) x86_64 GNU/Linux
/usr/bin/plink

While it's clearly not very common to use putty on Unix systems, it
certainly is possible and it would need to work the same way.

> -------------
> What about adding test-cases in t5601,
> this will ease the documentation later.
> f:/util/plink
> /c/util/plink.exe
> f:/util/tortoiseplink
> /c/util/tortoiseplink.exe
> /usr/local/uplink/sshwrapper.sh

It looks like there's already a framework for that, so sure, I can add
some tests.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v3 0/3] Improve robustness of putty detection

[brian m. carlson]

This series improves detection of plink (the putty utility).

While I was adding tests, I noticed that we had a broken test due to the
use of single quotes within a test, which resulted in the test always
being skipped.  Therefore, nobody noticed that the test was actually
broken.

brian m. carlson (3):
  connect: simplify SSH connection code path
  t5601: fix quotation error leading to skipped tests
  connect: improve check for plink to reduce false positives

 connect.c        | 56 ++++++++++++++++++++++++++++++++++----------------------
 t/t5601-clone.sh | 35 ++++++++++++++++++++++++++++++++++-
 2 files changed, 68 insertions(+), 23 deletions(-)

-- 
2.3.5

[PATCH v3 1/3] connect: simplify SSH connection code path

[brian m. carlson]

The code path used in git_connect pushed the majority of the SSH
connection code into an else block, even though the if block returns.
Simplify the code by eliminating the else block, as it is unneeded.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 connect.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

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;
-- 
2.3.5

[PATCH v3 2/3] t5601: fix quotation error leading to skipped tests

[brian m. carlson]

One of the tests in t5601 used single quotes to delimit an argument
containing spaces.  However, this caused test_expect_success to be
passed three arguments instead of two, which in turn caused the test
name to be treated as a prerequisite instead of a test name.  As there
was no prerequisite called "bracketed hostnames are still ssh", the test
was always skipped.

Because this test was always skipped, the fact that it passed the
arguments in the wrong order was obscured.  Use double quotes inside the
test and reorder the arguments so that the test runs and properly
reflects the arguments that are passed to ssh.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t5601-clone.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 1befc45..aae2324 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -332,7 +332,7 @@ test_expect_success !MINGW,!CYGWIN 'clone local path foo:bar' '
 
 test_expect_success 'bracketed hostnames are still ssh' '
 	git clone "[myhost:123]:src" ssh-bracket-clone &&
-	expect_ssh myhost '-p 123' src
+	expect_ssh "-p 123" myhost src
 '
 
 counter=0
-- 
2.3.5

[PATCH v3 3/3] connect: improve check for plink to reduce false positives

[brian m. carlson]

The git_connect function has code to handle plink and tortoiseplink
specially, as they require different command line arguments from
OpenSSH (-P instead of -p for ports; tortoiseplink additionally requires
-batch).  However, the match was done by checking for "plink" anywhere
in the string, which led to a GIT_SSH value containing "uplink" being
treated as an invocation of putty's plink.

Improve the check by looking for "plink" or "tortoiseplink" (or those
names suffixed with ".exe") only in the final component of the path.
This has the downside that a program such as "plink-0.63" would no
longer be recognized, but the increased robustness is likely worth it.
Add tests to cover these cases to avoid regressions.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 connect.c        | 18 +++++++++++++++---
 t/t5601-clone.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/connect.c b/connect.c
index 749a07b..c0144d8 100644
--- a/connect.c
+++ b/connect.c
@@ -724,7 +724,7 @@ 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;
+			int putty, tortoiseplink = 0;
 			char *ssh_host = hostandport;
 			const char *port = NULL;
 			get_host_and_port(&ssh_host, &port);
@@ -750,14 +750,26 @@ struct child_process *git_connect(int fd[2], const char *url,
 				conn->use_shell = 1;
 				putty = 0;
 			} else {
+				const char *base;
+				char *ssh_dup;
+
 				ssh = getenv("GIT_SSH");
 				if (!ssh)
 					ssh = "ssh";
-				putty = !!strcasestr(ssh, "plink");
+
+				ssh_dup = xstrdup(ssh);
+				base = basename(ssh_dup);
+
+				tortoiseplink = !strcasecmp(base, "tortoiseplink") ||
+					!strcasecmp(base, "tortoiseplink.exe");
+				putty = !strcasecmp(base, "plink") ||
+					!strcasecmp(base, "plink.exe") || tortoiseplink;
+
+				free(ssh_dup);
 			}
 
 			argv_array_push(&conn->args, ssh);
-			if (putty && !strcasestr(ssh, "tortoiseplink"))
+			if (tortoiseplink)
 				argv_array_push(&conn->args, "-batch");
 			if (port) {
 				/* P is for PuTTY, p is for OpenSSH */
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index aae2324..bfdaf75 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -296,6 +296,12 @@ setup_ssh_wrapper () {
 	'
 }
 
+copy_ssh_wrapper_as () {
+	cp "$TRASH_DIRECTORY/ssh-wrapper" "$1" &&
+	GIT_SSH="$1" &&
+	export GIT_SSH
+}
+
 expect_ssh () {
 	test_when_finished '
 		(cd "$TRASH_DIRECTORY" && rm -f ssh-expect && >ssh-output)
@@ -335,6 +341,33 @@ test_expect_success 'bracketed hostnames are still ssh' '
 	expect_ssh "-p 123" myhost src
 '
 
+test_expect_success 'uplink is not treated as putty' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/uplink" &&
+	git clone "[myhost:123]:src" ssh-bracket-clone-uplink &&
+	expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'plink is treated specially (as putty)' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+	git clone "[myhost:123]:src" ssh-bracket-clone-plink-0 &&
+	expect_ssh "-P 123" myhost src
+'
+
+test_expect_success 'plink.exe is treated specially (as putty)' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+	git clone "[myhost:123]:src" ssh-bracket-clone-plink-1 &&
+	expect_ssh "-P 123" myhost src
+'
+
+test_expect_success 'tortoiseplink is like putty, with extra arguments' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/tortoiseplink" &&
+	git clone "[myhost:123]:src" ssh-bracket-clone-plink-2 &&
+	expect_ssh "-batch -P 123" myhost src
+'
+
+# Reset the GIT_SSH environment variable for clone tests.
+setup_ssh_wrapper
+
 counter=0
 # $1 url
 # $2 none|host
-- 
2.3.5

Re: [PATCH v3 0/3] Improve robustness of putty detection

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> While I was adding tests, I noticed that we had a broken test due to the
> use of single quotes within a test, which resulted in the test always
> being skipped.

Good eyes.  While fixing the test is necessary, we should also be
able to improve the test framework to prevent such mistakes at the
same time.

ok 38 # skip
        git clone "[myhost:123]:src" ssh-bracket-clone &&
        expect_ssh myhost -p (missing bracketed hostnames are still
        ssh)

The test scripts are expected to take either 3 or 4 parameters, and
the extra parameter when it takes 4 is the comma separated list of
prerequisites.  "bracketed hostnames are still ssh" does not look
like prerequisites at all to us humans, and the framework should
also be able to notice that and barf, I would think.

Perhaps something like this?

 t/test-lib-functions.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0698ce7..0e4f2a6 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -348,11 +348,18 @@ test_declared_prereq () {
 	return 1
 }
 
+test_verify_prereq () {
+	test -z "$test_prereq" ||
+	expr >/dev/null "$test_prereq" : '^[A-Z0-9_,!]*$' ||
+	error "bug in the test script: '$test_prereq' does not look like a prereq"
+}
+
 test_expect_failure () {
 	test_start_
 	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 or 3 parameters to test-expect-failure"
+	test_verify_prereq
 	export test_prereq
 	if ! test_skip "$@"
 	then
@@ -372,6 +379,7 @@ test_expect_success () {
 	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 or 3 parameters to test-expect-success"
+	test_verify_prereq
 	export test_prereq
 	if ! test_skip "$@"
 	then
@@ -400,6 +408,7 @@ test_external () {
 	error >&5 "bug in the test script: not 3 or 4 parameters to test_external"
 	descr="$1"
 	shift
+	test_verify_prereq
 	export test_prereq
 	if ! test_skip "$descr" "$@"
 	then

Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives

[Johannes Schindelin]

Hi,

On 2015-04-26 22:30, brian m. carlson wrote:
> The git_connect function has code to handle plink and tortoiseplink
> specially, as they require different command line arguments from
> OpenSSH (-P instead of -p for ports; tortoiseplink additionally requires
> -batch).  However, the match was done by checking for "plink" anywhere
> in the string, which led to a GIT_SSH value containing "uplink" being
> treated as an invocation of putty's plink.
> 
> Improve the check by looking for "plink" or "tortoiseplink" (or those
> names suffixed with ".exe") only in the final component of the path.
> This has the downside that a program such as "plink-0.63" would no
> longer be recognized, but the increased robustness is likely worth it.
> Add tests to cover these cases to avoid regressions.
> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>

I like it!

Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Ciao,
Dscho

Re: [PATCH v3 0/3] Improve robustness of putty detection

[Torsten Bögershausen]

On 04/27/2015 12:04 AM, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
>> While I was adding tests, I noticed that we had a broken test due to the
>> use of single quotes within a test, which resulted in the test always
>> being skipped.
> 
> Good eyes.  While fixing the test is necessary, we should also be
> able to improve the test framework to prevent such mistakes at the
> same time.
> 
> ok 38 # skip
>         git clone "[myhost:123]:src" ssh-bracket-clone &&
>         expect_ssh myhost -p (missing bracketed hostnames are still
>         ssh)
> 
> The test scripts are expected to take either 3 or 4 parameters, and
> the extra parameter when it takes 4 is the comma separated list of
> prerequisites.  "bracketed hostnames are still ssh" does not look
> like prerequisites at all to us humans, and the framework should
> also be able to notice that and barf, I would think.
> 
> Perhaps something like this?
> 
>  t/test-lib-functions.sh | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 0698ce7..0e4f2a6 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -348,11 +348,18 @@ test_declared_prereq () {
>  	return 1
>  }
>  
> +test_verify_prereq () {
> +	test -z "$test_prereq" ||
> +	expr >/dev/null "$test_prereq" : '^[A-Z0-9_,!]*$' ||
> +	error "bug in the test script: '$test_prereq' does not look like a prereq"
> +}
> +
>  test_expect_failure () {
>  	test_start_
>  	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
>  	test "$#" = 2 ||
>  	error "bug in the test script: not 2 or 3 parameters to test-expect-failure"
> +	test_verify_prereq
>  	export test_prereq
>  	if ! test_skip "$@"
>  	then
> @@ -372,6 +379,7 @@ test_expect_success () {
>  	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
>  	test "$#" = 2 ||
>  	error "bug in the test script: not 2 or 3 parameters to test-expect-success"
> +	test_verify_prereq
>  	export test_prereq
>  	if ! test_skip "$@"
>  	then
> @@ -400,6 +408,7 @@ test_external () {
>  	error >&5 "bug in the test script: not 3 or 4 parameters to test_external"
>  	descr="$1"
>  	shift
> +	test_verify_prereq
>  	export test_prereq
>  	if ! test_skip "$descr" "$@"
>  	then
> 
Thanks for fixing my bugs :-)

Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives

[Jeff King]

On Sun, Apr 26, 2015 at 08:30:12PM +0000, brian m. carlson wrote:

> The git_connect function has code to handle plink and tortoiseplink
> specially, as they require different command line arguments from
> OpenSSH (-P instead of -p for ports; tortoiseplink additionally requires
> -batch).  However, the match was done by checking for "plink" anywhere
> in the string, which led to a GIT_SSH value containing "uplink" being
> treated as an invocation of putty's plink.
> 
> Improve the check by looking for "plink" or "tortoiseplink" (or those
> names suffixed with ".exe") only in the final component of the path.
> This has the downside that a program such as "plink-0.63" would no
> longer be recognized, but the increased robustness is likely worth it.
> Add tests to cover these cases to avoid regressions.

Thanks, I think this version looks good. It's possible that it's too
tight, but I think it represents our best guess at what is reasonable,
and we can only know more by getting it into the hands of users
(hopefully while it is cooking during this release cycle, but I am not
incredibly optimistic about such things).

-Peff

Re: [PATCH v3 0/3] Improve robustness of putty detection

[Jeff King]

On Sun, Apr 26, 2015 at 03:04:56PM -0700, Junio C Hamano wrote:

> The test scripts are expected to take either 3 or 4 parameters, and
> the extra parameter when it takes 4 is the comma separated list of
> prerequisites.  "bracketed hostnames are still ssh" does not look
> like prerequisites at all to us humans, and the framework should
> also be able to notice that and barf, I would think.
> 
> Perhaps something like this?

I like it. I wondered if we could even recognize a known set of prereqs
(e.g., say "I don't know about the FOO prereq; did you forget to
test_lazy_prereq it?"), but I don't think we can. Some of the prereqs
are set by arbitrary code, and when they are not set, we don't call a
"test_do_not_set_prereq FOO".  Enforcing a sane syntax is almost as
good, and seems pretty easy to implement.

> +test_verify_prereq () {
> +	test -z "$test_prereq" ||
> +	expr >/dev/null "$test_prereq" : '^[A-Z0-9_,!]*$' ||
> +	error "bug in the test script: '$test_prereq' does not look like a prereq"
> +}

The leading "^" is unnecessary in an expr regexp, as such expressions
are always left-anchored. And according to POSIX, even undesirable
due to hysterical raisins. You do still need the '$' to anchor the end.

I was surprised that the regexp does not match the empty string itself
(making the initial "test -z" redundant), but it does not seem to with
my version of expr. Weird. I think it is because the exit value is not
"did it match" but "is the return value of the expression non-zero", and
of course we matched zero characters.

At any rate, we are probably much better off having the initial "test
-z" there as an optimization, anyway. "expr" is not usually a built-in,
and otherwise we add an extra fork to each test invocation (not
something I expect matters under Linux, but I know Windows folks are
very sensitive to it).

-Peff

Re: [PATCH v3 0/3] Improve robustness of putty detection

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

On Sun, Apr 26, 2015 at 03:04:56PM -0700, Junio C Hamano wrote:
> Good eyes.  While fixing the test is necessary, we should also be
> able to improve the test framework to prevent such mistakes at the
> same time.
> 
> ok 38 # skip
>         git clone "[myhost:123]:src" ssh-bracket-clone &&
>         expect_ssh myhost -p (missing bracketed hostnames are still
>         ssh)
> 
> The test scripts are expected to take either 3 or 4 parameters, and
> the extra parameter when it takes 4 is the comma separated list of
> prerequisites.  "bracketed hostnames are still ssh" does not look
> like prerequisites at all to us humans, and the framework should
> also be able to notice that and barf, I would think.
> 
> Perhaps something like this?

I think this is a good change.  I haven't tested to see if we have any
other issues in the testsuite that this would expose, but if so, they
should be easy enough to fix up.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives

[Jeff King]

On Sun, Apr 26, 2015 at 08:30:12PM +0000, brian m. carlson wrote:

> The git_connect function has code to handle plink and tortoiseplink
> specially, as they require different command line arguments from
> OpenSSH (-P instead of -p for ports; tortoiseplink additionally requires
> -batch).  However, the match was done by checking for "plink" anywhere
> in the string, which led to a GIT_SSH value containing "uplink" being
> treated as an invocation of putty's plink.
> 
> Improve the check by looking for "plink" or "tortoiseplink" (or those
> names suffixed with ".exe") only in the final component of the path.
> This has the downside that a program such as "plink-0.63" would no
> longer be recognized, but the increased robustness is likely worth it.
> Add tests to cover these cases to avoid regressions.

FYI, this ended up biting me today. We have some integration tests that
make sure we can clone over putty, and we wrap plink in a
"plink-wrapper.sh" script that tweaks a few extra options. That used to
match under the old scheme, but not the new. It would also match if we
looked for "plink" anywhere in the basename (but not in leading
directories).

I was able to work around it pretty easily by changing our test setup,
but I thought I would include it here as a data point. It's probably not
that representative of real-world users.

-Peff

Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Apr 26, 2015 at 08:30:12PM +0000, brian m. carlson wrote:
>
>> Improve the check by looking for "plink" or "tortoiseplink" (or those
>> names suffixed with ".exe") only in the final component of the path.
>> This has the downside that a program such as "plink-0.63" would no
>> longer be recognized, but the increased robustness is likely worth it.
>> Add tests to cover these cases to avoid regressions.
>
> FYI, this ended up biting me today. We have some integration tests that
> make sure we can clone over putty, and we wrap plink in a
> "plink-wrapper.sh" script that tweaks a few extra options. That used to
> match under the old scheme, but not the new. It would also match if we
> looked for "plink" anywhere in the basename (but not in leading
> directories).

So this was a minor regression? ;-)

> I was able to work around it pretty easily by changing our test setup,
> but I thought I would include it here as a data point. It's probably not
> that representative of real-world users.

I'd imagine that "/usr/local/github/wrapped/bin/plink" may be a more
appropriate name to install that wrapper as than "plink-wrapper.sh",
but then people would need to think how to help that wrapper find
the real plink, so...

Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives

[Jeff King]

On Fri, Jun 26, 2015 at 09:16:20AM -0700, Junio C Hamano wrote:

> > FYI, this ended up biting me today. We have some integration tests that
> > make sure we can clone over putty, and we wrap plink in a
> > "plink-wrapper.sh" script that tweaks a few extra options. That used to
> > match under the old scheme, but not the new. It would also match if we
> > looked for "plink" anywhere in the basename (but not in leading
> > directories).
> 
> So this was a minor regression? ;-)

Yes. :)

> > I was able to work around it pretty easily by changing our test setup,
> > but I thought I would include it here as a data point. It's probably not
> > that representative of real-world users.
> 
> I'd imagine that "/usr/local/github/wrapped/bin/plink" may be a more
> appropriate name to install that wrapper as than "plink-wrapper.sh",
> but then people would need to think how to help that wrapper find
> the real plink, so...

It's the test suite for the server side of our git infrastructure, so
nothing gets installed. It's more like:

  export GIT_SSH=$PROJECT_ROOT/test/plink-wrapper.sh
  export REAL_PLINK=$PROJECT_ROOT/vendor/putty/plink
  git clone localhost:foo.git

and the wrapper knows to chain to $REAL_PLINK. So it was actually pretty
easy to swap, without any hacks to avoid recursing to ourselves in the
$PATH.

I doubt it is a problem for most people, because I don't imagine they
are writing test suites for git-related software.

-Peff

Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives

[Johannes Schindelin]

Hi Peff,

On 2015-06-26 18:27, Jeff King wrote:
> On Fri, Jun 26, 2015 at 09:16:20AM -0700, Junio C Hamano wrote:
> 
>> > FYI, this ended up biting me today. We have some integration tests that
>> > make sure we can clone over putty, and we wrap plink in a
>> > "plink-wrapper.sh" script that tweaks a few extra options. That used to
>> > match under the old scheme, but not the new. It would also match if we
>> > looked for "plink" anywhere in the basename (but not in leading
>> > directories).
>>
>> So this was a minor regression? ;-)
> 
> Yes. :)
> 
>> > I was able to work around it pretty easily by changing our test setup,
>> > but I thought I would include it here as a data point. It's probably not
>> > that representative of real-world users.
>>
>> I'd imagine that "/usr/local/github/wrapped/bin/plink" may be a more
>> appropriate name to install that wrapper as than "plink-wrapper.sh",
>> but then people would need to think how to help that wrapper find
>> the real plink, so...
> 
> It's the test suite for the server side of our git infrastructure, so
> nothing gets installed. It's more like:
> 
>   export GIT_SSH=$PROJECT_ROOT/test/plink-wrapper.sh
>   export REAL_PLINK=$PROJECT_ROOT/vendor/putty/plink
>   git clone localhost:foo.git
> 
> and the wrapper knows to chain to $REAL_PLINK. So it was actually pretty
> easy to swap, without any hacks to avoid recursing to ourselves in the
> $PATH.
> 
> I doubt it is a problem for most people, because I don't imagine they
> are writing test suites for git-related software.

Sorry to be so unavailable... day-job and Git for Windows[*1*], what can I say.

Would it help you if we detected ^plink[^a-zA-Z]?

Ciao,
Dscho

Footnote *1*: took me friggin' 9 1/2 hours to figure this one out: https://github.com/Alexpux/MSYS2-packages/pull/275

Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives

[Jeff King]

On Fri, Jun 26, 2015 at 07:13:15PM +0200, Johannes Schindelin wrote:

> > It's the test suite for the server side of our git infrastructure, so
> > nothing gets installed. It's more like:
> > 
> >   export GIT_SSH=$PROJECT_ROOT/test/plink-wrapper.sh
> >   export REAL_PLINK=$PROJECT_ROOT/vendor/putty/plink
> >   git clone localhost:foo.git
> > 
> > and the wrapper knows to chain to $REAL_PLINK. So it was actually pretty
> > easy to swap, without any hacks to avoid recursing to ourselves in the
> > $PATH.
> > 
> > I doubt it is a problem for most people, because I don't imagine they
> > are writing test suites for git-related software.
> 
> Sorry to be so unavailable... day-job and Git for Windows[*1*], what can I say.

No problem. I don't envy you. :)

> Would it help you if we detected ^plink[^a-zA-Z]?

In our we would have needed "^plink[^a-zA-Z-.]". I think there's no real
"right" answer here, as you can come up with hypotheticals that work and
don't work with just about every pattern. I was less trying to advocate
for loosening, and more just providing a data point to the list.

If we were to do any loosening, I'd probably suggest "^plink.*" (in the
basename).

-Peff

Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1545 bytes --]

On Fri, Jun 26, 2015 at 09:15:24AM -0400, Jeff King wrote:
> On Sun, Apr 26, 2015 at 08:30:12PM +0000, brian m. carlson wrote:
> > Improve the check by looking for "plink" or "tortoiseplink" (or those
> > names suffixed with ".exe") only in the final component of the path.
> > This has the downside that a program such as "plink-0.63" would no
> > longer be recognized, but the increased robustness is likely worth it.
> > Add tests to cover these cases to avoid regressions.
> 
> FYI, this ended up biting me today. We have some integration tests that
> make sure we can clone over putty, and we wrap plink in a
> "plink-wrapper.sh" script that tweaks a few extra options. That used to
> match under the old scheme, but not the new. It would also match if we
> looked for "plink" anywhere in the basename (but not in leading
> directories).
> 
> I was able to work around it pretty easily by changing our test setup,
> but I thought I would include it here as a data point. It's probably not
> that representative of real-world users.

Thanks for the data point.  While we don't use plink at $DAYJOB, this is
the kind of test we might well perform.  I expect it's most likely to
hit people in test setups like this, but if it turns out to be a
problem, we can certainly loosen it or if necessary, revert it.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 835 bytes --]