{"thread":{"id":"39136","subject":"[BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true","startedAt":"2015-04-22T14:36:45Z","lastAt":"2015-06-26T20:43:56Z","messageCount":41,"participants":["Patrick Sharp","Johannes Schindelin","Jeff King","brian m. carlson","Torsten Bögershausen","Pete Harlan","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"259799","messageId":"2A6FFC1D-5479-4DCA-A5E5-FF92DE0AA552@gmail.com","threadId":"39136","inReplyTo":null,"subject":"[BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true","fromName":"Patrick Sharp","fromEmail":"jakanapes@gmail.com","sentAt":"2015-04-22T14:36:45Z","receivedAt":"2015-04-22T14:36:45Z","isPatch":false,"sender":{"key":"jakanapes@gmail.com","avatar":"https://gravatar.com/avatar/dbeb9829a4c54a18cadbab4ef52cb3e63dd9f680941913dbd6075f4b561c8ea6?d=mp&s=160"},"body":"The plink string detection in GIT_SSH for setting putty to true is very broad.\n\nIf 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.\n\nWouldn’t searching for plink.exe be better?"},{"id":"259807","messageId":"6c2a6b53e8e6e019b145dddc67a9d2ca@www.dscho.org","threadId":"39136","inReplyTo":"2A6FFC1D-5479-4DCA-A5E5-FF92DE0AA552@gmail.com","subject":"Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-04-22T17:46:09Z","receivedAt":"2015-04-22T17:46:09Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn 2015-04-22 16:36, Patrick Sharp wrote:\n> The plink string detection in GIT_SSH for setting putty to true is very broad.\n\nWow. 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.\n\nWithout that information, you leave readers who have no idea about Putty *quite* puzzled.\n\n> If plink is anywhere in the path to the shell file then putty gets set\n> to true and ssh will fail trying to parse -batch as the hostname.\n\nThis is cryptic even for me.\n\n> Wouldn’t searching for plink.exe be better?--\n\nI 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).\n\nCiao,\nJohannes\n"},{"id":"259819","messageId":"F1AB6016-DA88-446C-84E9-1BD42436843D@gmail.com","threadId":"39136","inReplyTo":"6c2a6b53e8e6e019b145dddc67a9d2ca@www.dscho.org","subject":"Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true","fromName":"Patrick Sharp","fromEmail":"jakanapes@gmail.com","sentAt":"2015-04-22T19:12:53Z","receivedAt":"2015-04-22T19:12:53Z","isPatch":false,"sender":{"key":"jakanapes@gmail.com","avatar":"https://gravatar.com/avatar/dbeb9829a4c54a18cadbab4ef52cb3e63dd9f680941913dbd6075f4b561c8ea6?d=mp&s=160"},"body":"Johannes,\n\nYou’re correct, looking back over it, I was pretty vague.\n\nIn 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.\n\nUpon attempting to run git ls-remote, the system was spitting out getaddrinfo errors for ‘atch’ .\n\nSetting GIT_TRACE=2 showed that -batch was being added to the git command.\n\nThis was seen on several different servers with git versions 1.8.5.2, 1.9.1 and 2.3.5\n\nAfter a bit we realized that it was the string ‘uplink’ in the GIT_SSH variable that was linked to the extra -batch flag.\n\nFinally, after searching the git source, we narrowed it down to the ‘plink’ portion of the string.\n\nhttps://github.com/git/git/blob/7c597ef345aed345576de616c51f27e6f4b342b3/connect.c#L747-L756\n\n\n> On Apr 22, 2015, at 12:46 PM, Johannes Schindelin wrote:\n> \n> Hi Patrick,\n> \n> On 2015-04-22 16:36, Patrick Sharp wrote:\n>> The plink string detection in GIT_SSH for setting putty to true is very broad.\n> \n> 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.\n> \n> Without that information, you leave readers who have no idea about Putty *quite* puzzled.\n> \n>> If plink is anywhere in the path to the shell file then putty gets set\n>> to true and ssh will fail trying to parse -batch as the hostname.\n> \n> This is cryptic even for me.\n> \n>> Wouldn’t searching for plink.exe be better?--\n> \n> 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).\n> \n> Ciao,\n> Johannes\n"},{"id":"259845","messageId":"20150422202910.GB26917@peff.net","threadId":"39136","inReplyTo":"F1AB6016-DA88-446C-84E9-1BD42436843D@gmail.com","subject":"Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-22T20:29:10Z","receivedAt":"2015-04-22T20:29:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2015 at 02:12:53PM -0500, Patrick Sharp wrote:\n\n> Johannes,\n> \n> You’re correct, looking back over it, I was pretty vague.\n> \n> 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.\n> \n> Upon attempting to run git ls-remote, the system was spitting out getaddrinfo errors for ‘atch’ .\n> \n> Setting GIT_TRACE=2 showed that -batch was being added to the git command.\n> \n> This was seen on several different servers with git versions 1.8.5.2, 1.9.1 and 2.3.5\n> \n> After a bit we realized that it was the string ‘uplink’ in the GIT_SSH variable that was linked to the extra -batch flag.\n> \n> Finally, after searching the git source, we narrowed it down to the ‘plink’ portion of the string.\n> \n> https://github.com/git/git/blob/7c597ef345aed345576de616c51f27e6f4b342b3/connect.c#L747-L756\n\nI think you want something like:\n\ndiff --git a/connect.c b/connect.c\nindex 9ae991a..58aad56 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -568,7 +568,8 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \tconn->argv = arg = xcalloc(7, sizeof(*arg));\n \tif (protocol == PROTO_SSH) {\n \t\tconst char *ssh = getenv(\"GIT_SSH\");\n-\t\tint putty = ssh && strcasestr(ssh, \"plink\");\n+\t\tint putty = ssh && (ends_with(ssh, \"plink\") ||\n+\t\t\t\t ends_with(\"plink.exe\"));\n \t\tif (!ssh) ssh = \"ssh\";\n \n \t\t*arg++ = ssh;\n\nthough that is not quite enough (we do not have a case-insensitive\nversion of \"ends_with\"). I'm also not sure if matching just \"plink\" and\n\"plink.exe\" at the end of the string is enough (I'm just guessing that\nwas the original reason for using strstr in the first place).\n\nNote that I don't think just switching the strcasestr to look for\n\"plink.exe\" is right. For one thing, it just punts on the problem (it\ncan still happen, it's just less likely to trigger). But for another,\nyou can have plink (without \".exe\") on Linux systems.\n\n-Peff\n"},{"id":"259848","messageId":"20150422211915.GB827497@vauxhall.crustytoothpaste.net","threadId":"39136","inReplyTo":"20150422202910.GB26917@peff.net","subject":"Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-22T21:19:15Z","receivedAt":"2015-04-22T21:19:15Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Apr 22, 2015 at 04:29:10PM -0400, Jeff King wrote:\n> I think you want something like:\n> \n> diff --git a/connect.c b/connect.c\n> index 9ae991a..58aad56 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -568,7 +568,8 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n> \tconn->argv = arg = xcalloc(7, sizeof(*arg));\n> \tif (protocol == PROTO_SSH) {\n> \t\tconst char *ssh = getenv(\"GIT_SSH\");\n> -\t\tint putty = ssh && strcasestr(ssh, \"plink\");\n> +\t\tint putty = ssh && (ends_with(ssh, \"plink\") ||\n> +\t\t\t\t ends_with(\"plink.exe\"));\n> \t\tif (!ssh) ssh = \"ssh\";\n> \n> \t\t*arg++ = ssh;\n> \n> though that is not quite enough (we do not have a case-insensitive\n> version of \"ends_with\"). I'm also not sure if matching just \"plink\" and\n> \"plink.exe\" at the end of the string is enough (I'm just guessing that\n> was the original reason for using strstr in the first place).\n> \n> Note that I don't think just switching the strcasestr to look for\n> \"plink.exe\" is right. For one thing, it just punts on the problem (it\n> can still happen, it's just less likely to trigger). But for another,\n> you can have plink (without \".exe\") on Linux systems.\n\nPerhaps it would be worthwhile to check instead if the text \"plink\" is\nthe beginning of string or is preceded by a path separator. That would\ngive us a bit more confidence that the user is looking for plink, but\nwould still allow people to use \"plink-0.63\" if they like.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"259849","messageId":"20150422212904.GA32043@peff.net","threadId":"39136","inReplyTo":"20150422211915.GB827497@vauxhall.crustytoothpaste.net","subject":"Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-22T21:29:04Z","receivedAt":"2015-04-22T21:29:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2015 at 09:19:15PM +0000, brian m. carlson wrote:\n\n> > Note that I don't think just switching the strcasestr to look for\n> > \"plink.exe\" is right. For one thing, it just punts on the problem (it\n> > can still happen, it's just less likely to trigger). But for another,\n> > you can have plink (without \".exe\") on Linux systems.\n> \n> Perhaps it would be worthwhile to check instead if the text \"plink\" is\n> the beginning of string or is preceded by a path separator. That would\n> give us a bit more confidence that the user is looking for plink, but\n> would still allow people to use \"plink-0.63\" if they like.\n\nYeah, I think that is a reasonable approach. Note that it needs to\nhandle the \"tortoiseplink\" case from below, too (you can still use your\nstrategy, you just need to look for either string).\n\n-Peff\n"},{"id":"259852","messageId":"20150422214445.GC827497@vauxhall.crustytoothpaste.net","threadId":"39136","inReplyTo":"20150422212904.GA32043@peff.net","subject":"Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-22T21:44:45Z","receivedAt":"2015-04-22T21:44:45Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Apr 22, 2015 at 05:29:04PM -0400, Jeff King wrote:\n> > Perhaps it would be worthwhile to check instead if the text \"plink\" is\n> > the beginning of string or is preceded by a path separator. That would\n> > give us a bit more confidence that the user is looking for plink, but\n> > would still allow people to use \"plink-0.63\" if they like.\n> \n> Yeah, I think that is a reasonable approach. Note that it needs to\n> handle the \"tortoiseplink\" case from below, too (you can still use your\n> strategy, you just need to look for either string).\n\nSo maybe something like this?\n\ndiff --git a/connect.c b/connect.c\nindex 391d211..ba3ab34 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -749,10 +749,15 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\t\tconn->use_shell = 1;\n \t\t\t\t\tputty = 0;\n \t\t\t\t} else {\n+\t\t\t\t\tchar *plink, *tplink;\n+\n \t\t\t\t\tssh = getenv(\"GIT_SSH\");\n \t\t\t\t\tif (!ssh)\n \t\t\t\t\t\tssh = \"ssh\";\n-\t\t\t\t\tputty = !!strcasestr(ssh, \"plink\");\n+\t\t\t\t\tplink = strcasestr(ssh, \"plink\");\n+\t\t\t\t\ttplink = strcasestr(ssh, \"tortoiseplink\");\n+\t\t\t\t\tputty = plink == ssh || (plink && is_dir_sep(plink[-1])) ||\n+\t\t\t\t\t\ttplink == ssh || (tplink && is_dir_sep(tplink[-1]));\n \t\t\t\t}\n \n \t\t\t\targv_array_push(&conn->args, ssh);\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"259854","messageId":"20150422220053.GA32268@peff.net","threadId":"39136","inReplyTo":"20150422214445.GC827497@vauxhall.crustytoothpaste.net","subject":"Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-22T22:00:54Z","receivedAt":"2015-04-22T22:00:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2015 at 09:44:45PM +0000, brian m. carlson wrote:\n\n> On Wed, Apr 22, 2015 at 05:29:04PM -0400, Jeff King wrote:\n> > > Perhaps it would be worthwhile to check instead if the text \"plink\" is\n> > > the beginning of string or is preceded by a path separator. That would\n> > > give us a bit more confidence that the user is looking for plink, but\n> > > would still allow people to use \"plink-0.63\" if they like.\n> > \n> > Yeah, I think that is a reasonable approach. Note that it needs to\n> > handle the \"tortoiseplink\" case from below, too (you can still use your\n> > strategy, you just need to look for either string).\n> \n> So maybe something like this?\n> \n> diff --git a/connect.c b/connect.c\n> index 391d211..ba3ab34 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -749,10 +749,15 @@ struct child_process *git_connect(int fd[2], const char *url,\n> \t\t\t\t\tconn->use_shell = 1;\n> \t\t\t\t\tputty = 0;\n> \t\t\t\t} else {\n> +\t\t\t\t\tchar *plink, *tplink;\n> +\n> \t\t\t\t\tssh = getenv(\"GIT_SSH\");\n> \t\t\t\t\tif (!ssh)\n> \t\t\t\t\t\tssh = \"ssh\";\n> -\t\t\t\t\tputty = !!strcasestr(ssh, \"plink\");\n> +\t\t\t\t\tplink = strcasestr(ssh, \"plink\");\n> +\t\t\t\t\ttplink = strcasestr(ssh, \"tortoiseplink\");\n> +\t\t\t\t\tputty = plink == ssh || (plink && is_dir_sep(plink[-1])) ||\n> +\t\t\t\t\t\ttplink == ssh || (tplink && is_dir_sep(tplink[-1]));\n\nYeah, that looks right to me. You might want to represent the \"are we\ntortoise\" check as a separate flag, though, and reuse it a few lines\nlater.\n\nAlso, not related to your patch, but I notice the \"putty\" declaration is\nin a different scope than I would have expected, which made me wonder if\nit gets initialized in all code paths. I think is from the recent\naddition of CONNECT_DIAG_URL, which pushes the bulk of the code into its\nown else clause, even though the first part of the \"if\" always returns\nearly. I wonder if it would be simpler to read like:\n\ndiff --git a/connect.c b/connect.c\nindex 391d211..749a07b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -743,28 +743,28 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tfree(path);\n \t\t\t\tfree(conn);\n \t\t\t\treturn NULL;\n-\t\t\t} else {\n-\t\t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n-\t\t\t\tif (ssh) {\n-\t\t\t\t\tconn->use_shell = 1;\n-\t\t\t\t\tputty = 0;\n-\t\t\t\t} else {\n-\t\t\t\t\tssh = getenv(\"GIT_SSH\");\n-\t\t\t\t\tif (!ssh)\n-\t\t\t\t\t\tssh = \"ssh\";\n-\t\t\t\t\tputty = !!strcasestr(ssh, \"plink\");\n-\t\t\t\t}\n-\n-\t\t\t\targv_array_push(&conn->args, ssh);\n-\t\t\t\tif (putty && !strcasestr(ssh, \"tortoiseplink\"))\n-\t\t\t\t\targv_array_push(&conn->args, \"-batch\");\n-\t\t\t\tif (port) {\n-\t\t\t\t\t/* P is for PuTTY, p is for OpenSSH */\n-\t\t\t\t\targv_array_push(&conn->args, putty ? \"-P\" : \"-p\");\n-\t\t\t\t\targv_array_push(&conn->args, port);\n-\t\t\t\t}\n-\t\t\t\targv_array_push(&conn->args, ssh_host);\n \t\t\t}\n+\n+\t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n+\t\t\tif (ssh) {\n+\t\t\t\tconn->use_shell = 1;\n+\t\t\t\tputty = 0;\n+\t\t\t} else {\n+\t\t\t\tssh = getenv(\"GIT_SSH\");\n+\t\t\t\tif (!ssh)\n+\t\t\t\t\tssh = \"ssh\";\n+\t\t\t\tputty = !!strcasestr(ssh, \"plink\");\n+\t\t\t}\n+\n+\t\t\targv_array_push(&conn->args, ssh);\n+\t\t\tif (putty && !strcasestr(ssh, \"tortoiseplink\"))\n+\t\t\t\targv_array_push(&conn->args, \"-batch\");\n+\t\t\tif (port) {\n+\t\t\t\t/* P is for PuTTY, p is for OpenSSH */\n+\t\t\t\targv_array_push(&conn->args, putty ? \"-P\" : \"-p\");\n+\t\t\t\targv_array_push(&conn->args, port);\n+\t\t\t}\n+\t\t\targv_array_push(&conn->args, ssh_host);\n \t\t} else {\n \t\t\t/* remove repo-local variables from the environment */\n \t\t\tconn->env = local_repo_env;\n\n-Peff\n"},{"id":"259856","messageId":"20150422222455.GA274681@vauxhall.crustytoothpaste.net","threadId":"39136","inReplyTo":"20150422220053.GA32268@peff.net","subject":"Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-22T22:24:55Z","receivedAt":"2015-04-22T22:24:55Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Apr 22, 2015 at 06:00:54PM -0400, Jeff King wrote:\n> Yeah, that looks right to me. You might want to represent the \"are we\n> tortoise\" check as a separate flag, though, and reuse it a few lines\n> later.\n\nSounds like a good idea. I'll send a more formal patch a bit later\ntoday.\n\n> Also, not related to your patch, but I notice the \"putty\" declaration is\n> in a different scope than I would have expected, which made me wonder if\n> it gets initialized in all code paths. I think is from the recent\n> addition of CONNECT_DIAG_URL, which pushes the bulk of the code into its\n> own else clause, even though the first part of the \"if\" always returns\n> early. I wonder if it would be simpler to read like:\n> \n> diff --git a/connect.c b/connect.c\n> index 391d211..749a07b 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -743,28 +743,28 @@ struct child_process *git_connect(int fd[2], const char *url,\n> \t\t\t\tfree(path);\n> \t\t\t\tfree(conn);\n> \t\t\t\treturn NULL;\n> -\t\t\t} else {\n> -\t\t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n> -\t\t\t\tif (ssh) {\n> -\t\t\t\t\tconn->use_shell = 1;\n> -\t\t\t\t\tputty = 0;\n> -\t\t\t\t} else {\n> -\t\t\t\t\tssh = getenv(\"GIT_SSH\");\n> -\t\t\t\t\tif (!ssh)\n> -\t\t\t\t\t\tssh = \"ssh\";\n> -\t\t\t\t\tputty = !!strcasestr(ssh, \"plink\");\n> -\t\t\t\t}\n> -\n> -\t\t\t\targv_array_push(&conn->args, ssh);\n> -\t\t\t\tif (putty && !strcasestr(ssh, \"tortoiseplink\"))\n> -\t\t\t\t\targv_array_push(&conn->args, \"-batch\");\n> -\t\t\t\tif (port) {\n> -\t\t\t\t\t/* P is for PuTTY, p is for OpenSSH */\n> -\t\t\t\t\targv_array_push(&conn->args, putty ? \"-P\" : \"-p\");\n> -\t\t\t\t\targv_array_push(&conn->args, port);\n> -\t\t\t\t}\n> -\t\t\t\targv_array_push(&conn->args, ssh_host);\n> \t\t\t}\n> +\n> +\t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n> +\t\t\tif (ssh) {\n> +\t\t\t\tconn->use_shell = 1;\n> +\t\t\t\tputty = 0;\n> +\t\t\t} else {\n> +\t\t\t\tssh = getenv(\"GIT_SSH\");\n> +\t\t\t\tif (!ssh)\n> +\t\t\t\t\tssh = \"ssh\";\n> +\t\t\t\tputty = !!strcasestr(ssh, \"plink\");\n> +\t\t\t}\n> +\n> +\t\t\targv_array_push(&conn->args, ssh);\n> +\t\t\tif (putty && !strcasestr(ssh, \"tortoiseplink\"))\n> +\t\t\t\targv_array_push(&conn->args, \"-batch\");\n> +\t\t\tif (port) {\n> +\t\t\t\t/* P is for PuTTY, p is for OpenSSH */\n> +\t\t\t\targv_array_push(&conn->args, putty ? \"-P\" : \"-p\");\n> +\t\t\t\targv_array_push(&conn->args, port);\n> +\t\t\t}\n> +\t\t\targv_array_push(&conn->args, ssh_host);\n> \t\t} else {\n> \t\t\t/* remove repo-local variables from the environment */\n> \t\t\tconn->env = local_repo_env;\n\nI can drop this in as a preparatory patch if I can have your sign-off.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"259858","messageId":"20150422232306.GA32705@peff.net","threadId":"39136","inReplyTo":"20150422222455.GA274681@vauxhall.crustytoothpaste.net","subject":"Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-22T23:23:06Z","receivedAt":"2015-04-22T23:23:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2015 at 10:24:55PM +0000, brian m. carlson wrote:\n\n> On Wed, Apr 22, 2015 at 06:00:54PM -0400, Jeff King wrote:\n> > Yeah, that looks right to me. You might want to represent the \"are we\n> > tortoise\" check as a separate flag, though, and reuse it a few lines\n> > later.\n> \n> Sounds like a good idea. I'll send a more formal patch a bit later\n> today.\n\nThanks.\n\n> > Also, not related to your patch, but I notice the \"putty\" declaration is\n> > in a different scope than I would have expected, which made me wonder if\n> > it gets initialized in all code paths. I think is from the recent\n> > addition of CONNECT_DIAG_URL, which pushes the bulk of the code into its\n> > own else clause, even though the first part of the \"if\" always returns\n> > early. I wonder if it would be simpler to read like:\n> [...]\n> \n> I can drop this in as a preparatory patch if I can have your sign-off.\n\nDefinitely, thanks.\n\nSigned-off-by: Jeff King \n\n-Peff\n"},{"id":"259877","messageId":"1429747595-298095-1-git-send-email-sandals@crustytoothpaste.net","threadId":"39136","inReplyTo":"20150422232306.GA32705@peff.net","subject":"[PATCH 1/2] connect: simplify SSH connection code path","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-23T00:06:34Z","receivedAt":"2015-04-23T00:06:34Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The code path used in git_connect pushed the majority of the SSH\nconnection code into an else block, even though the if block returns.\nSimplify the code by eliminating the else block, as it is unneeded.\n\nSigned-off-by: Jeff King \nSigned-off-by: brian m. carlson \n---\n connect.c | 42 +++++++++++++++++++++---------------------\n 1 file changed, 21 insertions(+), 21 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 391d211..749a07b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -743,28 +743,28 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tfree(path);\n \t\t\t\tfree(conn);\n \t\t\t\treturn NULL;\n-\t\t\t} else {\n-\t\t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n-\t\t\t\tif (ssh) {\n-\t\t\t\t\tconn->use_shell = 1;\n-\t\t\t\t\tputty = 0;\n-\t\t\t\t} else {\n-\t\t\t\t\tssh = getenv(\"GIT_SSH\");\n-\t\t\t\t\tif (!ssh)\n-\t\t\t\t\t\tssh = \"ssh\";\n-\t\t\t\t\tputty = !!strcasestr(ssh, \"plink\");\n-\t\t\t\t}\n-\n-\t\t\t\targv_array_push(&conn->args, ssh);\n-\t\t\t\tif (putty && !strcasestr(ssh, \"tortoiseplink\"))\n-\t\t\t\t\targv_array_push(&conn->args, \"-batch\");\n-\t\t\t\tif (port) {\n-\t\t\t\t\t/* P is for PuTTY, p is for OpenSSH */\n-\t\t\t\t\targv_array_push(&conn->args, putty ? \"-P\" : \"-p\");\n-\t\t\t\t\targv_array_push(&conn->args, port);\n-\t\t\t\t}\n-\t\t\t\targv_array_push(&conn->args, ssh_host);\n \t\t\t}\n+\n+\t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n+\t\t\tif (ssh) {\n+\t\t\t\tconn->use_shell = 1;\n+\t\t\t\tputty = 0;\n+\t\t\t} else {\n+\t\t\t\tssh = getenv(\"GIT_SSH\");\n+\t\t\t\tif (!ssh)\n+\t\t\t\t\tssh = \"ssh\";\n+\t\t\t\tputty = !!strcasestr(ssh, \"plink\");\n+\t\t\t}\n+\n+\t\t\targv_array_push(&conn->args, ssh);\n+\t\t\tif (putty && !strcasestr(ssh, \"tortoiseplink\"))\n+\t\t\t\targv_array_push(&conn->args, \"-batch\");\n+\t\t\tif (port) {\n+\t\t\t\t/* P is for PuTTY, p is for OpenSSH */\n+\t\t\t\targv_array_push(&conn->args, putty ? \"-P\" : \"-p\");\n+\t\t\t\targv_array_push(&conn->args, port);\n+\t\t\t}\n+\t\t\targv_array_push(&conn->args, ssh_host);\n \t\t} else {\n \t\t\t/* remove repo-local variables from the environment */\n \t\t\tconn->env = local_repo_env;\n-- \n2.3.5\n"},{"id":"259878","messageId":"1429747595-298095-2-git-send-email-sandals@crustytoothpaste.net","threadId":"39136","inReplyTo":"1429747595-298095-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH 2/2] connect: improve check for plink to reduce false positives","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-23T00:06:35Z","receivedAt":"2015-04-23T00:06:35Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The git_connect function has code to handle plink and tortoiseplink\nspecially, as they require different command line arguments from\nOpenSSH. However, the match was done by checking for \"plink\"\ncase-insensitively in the string, which led to false positives when\nGIT_SSH contained \"uplink\". Improve the check by looking for \"plink\" or\n\"tortoiseplink\" only at the beginning of the string or immediately after\na directory separator.\n\nSigned-off-by: brian m. carlson \n---\nThis is essentially just a deindentation.\n\n connect.c | 15 ++++++++++++---\n 1 file changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 749a07b..bae76a2 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -724,7 +724,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tconn->in = conn->out = -1;\n \t\tif (protocol == PROTO_SSH) {\n \t\t\tconst char *ssh;\n-\t\t\tint putty;\n+\t\t\tint putty, tortoiseplink = 0;\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n \t\t\tget_host_and_port(&ssh_host, &port);\n@@ -750,14 +750,23 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tconn->use_shell = 1;\n \t\t\t\tputty = 0;\n \t\t\t} else {\n+\t\t\t\tchar *plink, *tplink;\n+\n \t\t\t\tssh = getenv(\"GIT_SSH\");\n \t\t\t\tif (!ssh)\n \t\t\t\t\tssh = \"ssh\";\n-\t\t\t\tputty = !!strcasestr(ssh, \"plink\");\n+\n+\t\t\t\tplink = strcasestr(ssh, \"plink\");\n+\t\t\t\ttplink = strcasestr(ssh, \"tortoiseplink\");\n+\n+\t\t\t\ttortoiseplink = tplink == ssh ||\n+\t\t\t\t\t(tplink && is_dir_sep(tplink[-1]));\n+\t\t\t\tputty = tortoiseplink || plink == ssh ||\n+\t\t\t\t\t(plink && is_dir_sep(plink[-1]));\n \t\t\t}\n \n \t\t\targv_array_push(&conn->args, ssh);\n-\t\t\tif (putty && !strcasestr(ssh, \"tortoiseplink\"))\n+\t\t\tif (tortoiseplink)\n \t\t\t\targv_array_push(&conn->args, \"-batch\");\n \t\t\tif (port) {\n \t\t\t\t/* P is for PuTTY, p is for OpenSSH */\n-- \n2.3.5\n"},{"id":"259882","messageId":"55387E31.5010100@web.de","threadId":"39136","inReplyTo":"F1AB6016-DA88-446C-84E9-1BD42436843D@gmail.com","subject":"Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-04-23T05:08:01Z","receivedAt":"2015-04-23T05:08:01Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 04/22/2015 09:12 PM, Patrick Sharp wrote:\n> Johannes,\n>\n> You’re correct, looking back over it, I was pretty vague.\n>\n> 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.\n>\n> Upon attempting to run git ls-remote, the system was spitting out getaddrinfo errors for ‘atch’ .\n>\n> Setting GIT_TRACE=2 showed that -batch was being added to the git command.\n>\n> This was seen on several different servers with git versions 1.8.5.2, 1.9.1 and 2.3.5\n>\n> After a bit we realized that it was the string ‘uplink’ in the GIT_SSH variable that was linked to the extra -batch flag.\n>\n> Finally, after searching the git source, we narrowed it down to the ‘plink’ portion of the string.\n>\n> https://github.com/git/git/blob/7c597ef345aed345576de616c51f27e6f4b342b3/connect.c#L747-L7\nBrian, I got your patch,\nbut can't see it in the list yet\n 1/2 looks good, thanks.\n(And add msygit)\n\nMy feeling is that patch 2/2 may break things for an unknown\namount of users which e.g. use \"myplink\".\n\nPatrick,\ndid you ever tell us, what you put into $GIT_SSH ?\n\nCan your use case be covered by using $GIT_SSH_COMMAND ?\n"},{"id":"259884","messageId":"b6b4da1f7735b834043375e3d8eaa331@www.dscho.org","threadId":"39136","inReplyTo":"1429747595-298095-2-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH 2/2] connect: improve check for plink to reduce false positives","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-04-23T06:50:17Z","receivedAt":"2015-04-23T06:50:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brian,\n\nOn 2015-04-23 02:06, brian m. carlson wrote:\n\n> +\t\t\t\ttortoiseplink = tplink == ssh ||\n> +\t\t\t\t\t(tplink && is_dir_sep(tplink[-1]));\n\nMaybe 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).\n\nAlso, 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).\n\nIn 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.\n\nCiao,\nDscho\n"},{"id":"259897","messageId":"317B332E-CA9A-4B6B-A0C4-7202CBF5C1AA@gmail.com","threadId":"39136","inReplyTo":"55387E31.5010100@web.de","subject":"Re: [BUG] having 'plink' anywhere in the GIT_SSH environment variables sets putty = true","fromName":"Patrick Sharp","fromEmail":"jakanapes@gmail.com","sentAt":"2015-04-23T13:15:39Z","receivedAt":"2015-04-23T13:15:39Z","isPatch":false,"sender":{"key":"jakanapes@gmail.com","avatar":"https://gravatar.com/avatar/dbeb9829a4c54a18cadbab4ef52cb3e63dd9f680941913dbd6075f4b561c8ea6?d=mp&s=160"},"body":"Torsten,\n\nThe relevant part of the path in GIT_SSH was ‘/uplink_deploy/‘.\n\nI 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.\n\nI 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.\n\nPart 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.\n\n\n> On Apr 23, 2015, at 12:08 AM, Torsten Bögershausen wrote:\n> \n> On 04/22/2015 09:12 PM, Patrick Sharp wrote:\n>> Johannes,\n>> \n>> You’re correct, looking back over it, I was pretty vague.\n>> \n>> 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.\n>> \n>> Upon attempting to run git ls-remote, the system was spitting out getaddrinfo errors for ‘atch’ .\n>> \n>> Setting GIT_TRACE=2 showed that -batch was being added to the git command.\n>> \n>> This was seen on several different servers with git versions 1.8.5.2, 1.9.1 and 2.3.5\n>> \n>> After a bit we realized that it was the string ‘uplink’ in the GIT_SSH variable that was linked to the extra -batch flag.\n>> \n>> Finally, after searching the git source, we narrowed it down to the ‘plink’ portion of the string.\n>> \n>> https://github.com/git/git/blob/7c597ef345aed345576de616c51f27e6f4b342b3/connect.c#L747-L7\n> Brian, I got your patch,\n> but can't see it in the list yet\n> 1/2 looks good, thanks.\n> (And add msygit)\n> \n> My feeling is that patch 2/2 may break things for an unknown\n> amount of users which e.g. use \"myplink\".\n> \n> Patrick,\n> did you ever tell us, what you put into $GIT_SSH ?\n> \n> Can your use case be covered by using $GIT_SSH_COMMAND ?\n> \n> \n> \n> \n> \n"},{"id":"259901","messageId":"20150423155304.GA26018@peff.net","threadId":"39136","inReplyTo":"b6b4da1f7735b834043375e3d8eaa331@www.dscho.org","subject":"Re: [PATCH 2/2] connect: improve check for plink to reduce false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-23T15:53:04Z","receivedAt":"2015-04-23T15:53:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 23, 2015 at 08:50:17AM +0200, Johannes Schindelin wrote:\n\n> > +\t\t\t\ttortoiseplink = tplink == ssh ||\n> > +\t\t\t\t\t(tplink && is_dir_sep(tplink[-1]));\n> \n> Maybe have a helper function here? Something like\n> `basename_matches(const char *path, const char *basename, int\n> ignore_case)`? That would be easier to read (I have to admit that I\n> had to wrap my head around the logic to ensure that tplink[-1] is\n> valid; It is, but it requires more brain cycles to verify than I would\n> like).\n\nYeah, I had a similar thought when reading the patch.\n\n> Also, I am really hesitant to just test the start of the basename; I\n> would rather have an entire basename match so that something like\n> \"PLinKoeln\" would not match. (And of course, for Windows I would want\n> that hypothetical `basename_matches()` function to allow basenames to\n> end in `.exe` automatically).\n\nWhat about \"plink-0.83\" that was mentioned earlier in the thread? I\nthink that is the reason brian's patch stuck to matching the start and\nnot the end. But I have no idea if that is actually a real thing, or\njust a hypothetical.\n\nIf I were writing from scratch, I would probably keep things as tight as\npossible, like:\n\n const char *base = basename(ssh);\n plink = !strcasecmp(base, \"plink\") ||\n !strcasecmp(base, \"plink.exe\");\n tplink = !strcasecmp(base, \"tortoiseplink\") ||\n !strcasecmp(base, \"tortoiseplink.exe\"));\n\nbut maybe that is too tight at this point in time; we don't really know\nwhat's out there and working (or maybe _we_ do, but _I_ do not :) ).\n\nAt any rate, brian's patch only looks for a dir-separator anywhere, not\nthe actual basename. So:\n\n /path/to/plink/ssh\n\nwould match, and I'm not sure if that's a good thing or not. So yet\nanother variant is to use basename(), and then just check that the\nbasename starts with \"plink\" (to catch \"plink.exe\", \"plink-0.83\", etc).\nThat avoids cruft in the intermediate path, and unless your actual\nbinary is named PlinKoeln, it will not false positive on the example you\ngave.\n\n-Peff\n"},{"id":"259919","messageId":"20150423231403.GC274681@vauxhall.crustytoothpaste.net","threadId":"39136","inReplyTo":"20150423155304.GA26018@peff.net","subject":"Re: [PATCH 2/2] connect: improve check for plink to reduce false positives","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-23T23:14:04Z","receivedAt":"2015-04-23T23:14:04Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, Apr 23, 2015 at 11:53:04AM -0400, Jeff King wrote:\n> On Thu, Apr 23, 2015 at 08:50:17AM +0200, Johannes Schindelin wrote:\n> \n> > > +\t\t\t\ttortoiseplink = tplink == ssh ||\n> > > +\t\t\t\t\t(tplink && is_dir_sep(tplink[-1]));\n> > \n> > Maybe have a helper function here? Something like\n> > `basename_matches(const char *path, const char *basename, int\n> > ignore_case)`? That would be easier to read (I have to admit that I\n> > had to wrap my head around the logic to ensure that tplink[-1] is\n> > valid; It is, but it requires more brain cycles to verify than I would\n> > like).\n> \n> Yeah, I had a similar thought when reading the patch.\n\nI was questioning whether a comment would have been helpful. Apparently\nthe answer was yes.\n\n> If I were writing from scratch, I would probably keep things as tight as\n> possible, like:\n> \n> const char *base = basename(ssh);\n> plink = !strcasecmp(base, \"plink\") ||\n> !strcasecmp(base, \"plink.exe\");\n> tplink = !strcasecmp(base, \"tortoiseplink\") ||\n> !strcasecmp(base, \"tortoiseplink.exe\"));\n> \n> but maybe that is too tight at this point in time; we don't really know\n> what's out there and working (or maybe _we_ do, but _I_ do not :) ).\n> \n> At any rate, brian's patch only looks for a dir-separator anywhere, not\n> the actual basename. So:\n> \n> /path/to/plink/ssh\n> \n> would match, and I'm not sure if that's a good thing or not.\n\nThis is true. In hindsight, I think it's probably better to be a bit\nstricter, so I'll reroll to use the stricter format.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"259931","messageId":"af8ebead06428a182fcb628301f34dd9@www.dscho.org","threadId":"39136","inReplyTo":"20150423155304.GA26018@peff.net","subject":"Re: [PATCH 2/2] connect: improve check for plink to reduce false positives","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-04-24T06:37:21Z","receivedAt":"2015-04-24T06:37:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn 2015-04-23 17:53, Jeff King wrote:\n\n> What about \"plink-0.83\" that was mentioned earlier in the thread?\n\nI 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\n\n ^(.*[/\\\\])?plink(-.*|\\.exe)$\n\n... 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.\n\nCiao,\nDscho\n"},{"id":"259932","messageId":"92bcca820a7ade286142458148d27bf3@www.dscho.org","threadId":"39136","inReplyTo":"20150423231403.GC274681@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH 2/2] connect: improve check for plink to reduce false positives","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-04-24T06:41:13Z","receivedAt":"2015-04-24T06:41:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brian,\n\nOn 2015-04-24 01:14, brian m. carlson wrote:\n> On Thu, Apr 23, 2015 at 11:53:04AM -0400, Jeff King wrote:\n>\n>> If I were writing from scratch, I would probably keep things as tight as\n>> possible, like:\n>>\n>> const char *base = basename(ssh);\n>> plink = !strcasecmp(base, \"plink\") ||\n>> !strcasecmp(base, \"plink.exe\");\n>> tplink = !strcasecmp(base, \"tortoiseplink\") ||\n>> !strcasecmp(base, \"tortoiseplink.exe\"));\n>>\n>> but maybe that is too tight at this point in time; we don't really know\n>> what's out there and working (or maybe _we_ do, but _I_ do not :) ).\n>>\n>> At any rate, brian's patch only looks for a dir-separator anywhere, not\n>> the actual basename. So:\n>>\n>> /path/to/plink/ssh\n>>\n>> would match, and I'm not sure if that's a good thing or not.\n> \n> This is true. In hindsight, I think it's probably better to be a bit\n> stricter, so I'll reroll to use the stricter format.\n\nThank you so much!\nDscho\n"},{"id":"259968","messageId":"1429914505-325708-1-git-send-email-sandals@crustytoothpaste.net","threadId":"39136","inReplyTo":"20150423231403.GC274681@vauxhall.crustytoothpaste.net","subject":"[PATCH v2 1/2] connect: simplify SSH connection code path","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-24T22:28:24Z","receivedAt":"2015-04-24T22:28:24Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The code path used in git_connect pushed the majority of the SSH\nconnection code into an else block, even though the if block returns.\nSimplify the code by eliminating the else block, as it is unneeded.\n\nSigned-off-by: Jeff King \nSigned-off-by: brian m. carlson \n---\n connect.c | 42 +++++++++++++++++++++---------------------\n 1 file changed, 21 insertions(+), 21 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 391d211..749a07b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -743,28 +743,28 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tfree(path);\n \t\t\t\tfree(conn);\n \t\t\t\treturn NULL;\n-\t\t\t} else {\n-\t\t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n-\t\t\t\tif (ssh) {\n-\t\t\t\t\tconn->use_shell = 1;\n-\t\t\t\t\tputty = 0;\n-\t\t\t\t} else {\n-\t\t\t\t\tssh = getenv(\"GIT_SSH\");\n-\t\t\t\t\tif (!ssh)\n-\t\t\t\t\t\tssh = \"ssh\";\n-\t\t\t\t\tputty = !!strcasestr(ssh, \"plink\");\n-\t\t\t\t}\n-\n-\t\t\t\targv_array_push(&conn->args, ssh);\n-\t\t\t\tif (putty && !strcasestr(ssh, \"tortoiseplink\"))\n-\t\t\t\t\targv_array_push(&conn->args, \"-batch\");\n-\t\t\t\tif (port) {\n-\t\t\t\t\t/* P is for PuTTY, p is for OpenSSH */\n-\t\t\t\t\targv_array_push(&conn->args, putty ? \"-P\" : \"-p\");\n-\t\t\t\t\targv_array_push(&conn->args, port);\n-\t\t\t\t}\n-\t\t\t\targv_array_push(&conn->args, ssh_host);\n \t\t\t}\n+\n+\t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n+\t\t\tif (ssh) {\n+\t\t\t\tconn->use_shell = 1;\n+\t\t\t\tputty = 0;\n+\t\t\t} else {\n+\t\t\t\tssh = getenv(\"GIT_SSH\");\n+\t\t\t\tif (!ssh)\n+\t\t\t\t\tssh = \"ssh\";\n+\t\t\t\tputty = !!strcasestr(ssh, \"plink\");\n+\t\t\t}\n+\n+\t\t\targv_array_push(&conn->args, ssh);\n+\t\t\tif (putty && !strcasestr(ssh, \"tortoiseplink\"))\n+\t\t\t\targv_array_push(&conn->args, \"-batch\");\n+\t\t\tif (port) {\n+\t\t\t\t/* P is for PuTTY, p is for OpenSSH */\n+\t\t\t\targv_array_push(&conn->args, putty ? \"-P\" : \"-p\");\n+\t\t\t\targv_array_push(&conn->args, port);\n+\t\t\t}\n+\t\t\targv_array_push(&conn->args, ssh_host);\n \t\t} else {\n \t\t\t/* remove repo-local variables from the environment */\n \t\t\tconn->env = local_repo_env;\n-- \n2.3.5\n"},{"id":"259967","messageId":"1429914505-325708-2-git-send-email-sandals@crustytoothpaste.net","threadId":"39136","inReplyTo":"1429914505-325708-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v2 2/2] connect: improve check for plink to reduce false positives","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-24T22:28:25Z","receivedAt":"2015-04-24T22:28:25Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The git_connect function has code to handle plink and tortoiseplink\nspecially, as they require different command line arguments from\nOpenSSH. However, the match was done by checking for \"plink\"\ncase-insensitively in the string, which led to false positives when\nGIT_SSH contained \"uplink\". Improve the check by looking for \"plink\" or\n\"tortoiseplink\" (or those names suffixed with \".exe\") in the final\ncomponent of the path.\n\nSigned-off-by: brian m. carlson \n---\n connect.c | 18 +++++++++++++++---\n 1 file changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 749a07b..c0144d8 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -724,7 +724,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tconn->in = conn->out = -1;\n \t\tif (protocol == PROTO_SSH) {\n \t\t\tconst char *ssh;\n-\t\t\tint putty;\n+\t\t\tint putty, tortoiseplink = 0;\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n \t\t\tget_host_and_port(&ssh_host, &port);\n@@ -750,14 +750,26 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tconn->use_shell = 1;\n \t\t\t\tputty = 0;\n \t\t\t} else {\n+\t\t\t\tconst char *base;\n+\t\t\t\tchar *ssh_dup;\n+\n \t\t\t\tssh = getenv(\"GIT_SSH\");\n \t\t\t\tif (!ssh)\n \t\t\t\t\tssh = \"ssh\";\n-\t\t\t\tputty = !!strcasestr(ssh, \"plink\");\n+\n+\t\t\t\tssh_dup = xstrdup(ssh);\n+\t\t\t\tbase = basename(ssh_dup);\n+\n+\t\t\t\ttortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n+\t\t\t\t\t!strcasecmp(base, \"tortoiseplink.exe\");\n+\t\t\t\tputty = !strcasecmp(base, \"plink\") ||\n+\t\t\t\t\t!strcasecmp(base, \"plink.exe\") || tortoiseplink;\n+\n+\t\t\t\tfree(ssh_dup);\n \t\t\t}\n \n \t\t\targv_array_push(&conn->args, ssh);\n-\t\t\tif (putty && !strcasestr(ssh, \"tortoiseplink\"))\n+\t\t\tif (tortoiseplink)\n \t\t\t\targv_array_push(&conn->args, \"-batch\");\n \t\t\tif (port) {\n \t\t\t\t/* P is for PuTTY, p is for OpenSSH */\n-- \n2.3.5\n"},{"id":"259971","messageId":"CAM=ud8zKE-Huq_ZGVq9ycFFw+R7TBkQ5R4tcwfha9OBm_geASg@mail.gmail.com","threadId":"39136","inReplyTo":"1429914505-325708-2-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 2/2] connect: improve check for plink to reduce false positives","fromName":"Pete Harlan","fromEmail":"pchpublic88@gmail.com","sentAt":"2015-04-24T22:46:30Z","receivedAt":"2015-04-24T22:46:30Z","isPatch":true,"sender":{"key":"pchpublic88@gmail.com","avatar":null},"body":"On Fri, Apr 24, 2015 at 3:28 PM, brian m. carlson\n wrote:\n> The git_connect function has code to handle plink and tortoiseplink\n> specially, as they require different command line arguments from\n> OpenSSH. However, the match was done by checking for \"plink\"\n> case-insensitively in the string, which led to false positives when\n\nPerhaps s/case-insensitively/anywhere/?\n\nIt's not important that it ignored case (your change ignores it too),\nit's that it was too lenient about its context.\n\n--Pete\n\n> GIT_SSH contained \"uplink\". Improve the check by looking for \"plink\" or\n> \"tortoiseplink\" (or those names suffixed with \".exe\") in the final\n> component of the path.\n>\n> Signed-off-by: brian m. carlson \n> ---\n> connect.c | 18 +++++++++++++++---\n> 1 file changed, 15 insertions(+), 3 deletions(-)\n>\n> diff --git a/connect.c b/connect.c\n> index 749a07b..c0144d8 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -724,7 +724,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n> conn->in = conn->out = -1;\n> if (protocol == PROTO_SSH) {\n> const char *ssh;\n> - int putty;\n> + int putty, tortoiseplink = 0;\n> char *ssh_host = hostandport;\n> const char *port = NULL;\n> get_host_and_port(&ssh_host, &port);\n> @@ -750,14 +750,26 @@ struct child_process *git_connect(int fd[2], const char *url,\n> conn->use_shell = 1;\n> putty = 0;\n> } else {\n> + const char *base;\n> + char *ssh_dup;\n> +\n> ssh = getenv(\"GIT_SSH\");\n> if (!ssh)\n> ssh = \"ssh\";\n> - putty = !!strcasestr(ssh, \"plink\");\n> +\n> + ssh_dup = xstrdup(ssh);\n> + base = basename(ssh_dup);\n> +\n> + tortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n> + !strcasecmp(base, \"tortoiseplink.exe\");\n> + putty = !strcasecmp(base, \"plink\") ||\n> + !strcasecmp(base, \"plink.exe\") || tortoiseplink;\n> +\n> + free(ssh_dup);\n> }\n>\n> argv_array_push(&conn->args, ssh);\n> - if (putty && !strcasestr(ssh, \"tortoiseplink\"))\n> + if (tortoiseplink)\n> argv_array_push(&conn->args, \"-batch\");\n> if (port) {\n> /* P is for PuTTY, p is for OpenSSH */\n"},{"id":"259972","messageId":"20150424224833.GC322887@vauxhall.crustytoothpaste.net","threadId":"39136","inReplyTo":"CAM=ud8zKE-Huq_ZGVq9ycFFw+R7TBkQ5R4tcwfha9OBm_geASg@mail.gmail.com","subject":"Re: [PATCH v2 2/2] connect: improve check for plink to reduce false positives","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-24T22:48:33Z","receivedAt":"2015-04-24T22:48:33Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Apr 24, 2015 at 03:46:30PM -0700, Pete Harlan wrote:\n> On Fri, Apr 24, 2015 at 3:28 PM, brian m. carlson\n> wrote:\n> > The git_connect function has code to handle plink and tortoiseplink\n> > specially, as they require different command line arguments from\n> > OpenSSH. However, the match was done by checking for \"plink\"\n> > case-insensitively in the string, which led to false positives when\n> \n> Perhaps s/case-insensitively/anywhere/?\n>\n> It's not important that it ignored case (your change ignores it too),\n> it's that it was too lenient about its context.\n\nYes, I don't know what I was thinking. I'll wait a bit to see if there\nare any more comments and then reroll.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"259995","messageId":"553BBACA.70704@web.de","threadId":"39136","inReplyTo":"1429914505-325708-2-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 2/2] connect: improve check for plink to reduce false positives","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-04-25T16:03:22Z","receivedAt":"2015-04-25T16:03:22Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-04-25 00.28, brian m. carlson wrote:\n> The git_connect function has code to handle plink and tortoiseplink\n> specially, as they require different command line arguments from\n> OpenSSH. However, the match was done by checking for \"plink\"\n> case-insensitively in the string, which led to false positives when\n> GIT_SSH contained \"uplink\". Improve the check by looking for \"plink\" or\n> \"tortoiseplink\" (or those names suffixed with \".exe\") in the final\n> component of the path.\n> \n(I'm not sute if the commit message describes the problem deep enough\nfor readers which are not familar with all the details of the original\nreport):\nA feature implemented for Windows may break things for e.g. Linux users)\nThe following may read exaggerated, so please read it as a suggestion.\n\nThe git_connect function has code to handle plink and tortoiseplink\nspecially, as they require different command line arguments compared to\nOpenSSH (-P instead of -p, tortoiseplink uses -batch), commit 36ad53ffee6ed.\nThe special handling is only needed for Windows, and a sloppy\ncase-insensitve search for \"plink\" will trigger that an the extra\nparameter \"-batch\" is added to the command line.\n\nThis was observed on a Linux system where a command line including\n\"/uplink_deploy/\" was used.\n\nThere are different ways to improve the situation:\n(The following mentions only plink, but assumes that tortoiseplink is handled\n similar)\na) Disable the plink/tortoiseplink special handling on non-Windows systems\nb) Tighten the search for plink:\n Allow basename() == plink || !!strcasestr(ssh, \"plink.exe\")\nc) Tighten the search for plink:\n Allow basename() == plink || !!strcasestr(ssh, \"plink.exe\")\nd) Tighten the check for tortoiseplink.\n Today we set \"int putty\" to true when plink is found, and -batch\n is set when tortoiseplink is not found.\n This fixes the reported bug, but still has the -P problem.\ne) Unix users typically use shell scripts and could use GIT_SSH_COMMAND.\n Declare the GIT_SSH as not-well-documented (and to obsolete ?) for non-Windows systems,\n\n\nThis patch implements c):\nExtract the basename and compare it to plink, plink.exe respective\ntortoiseplink/tortoiseplink.exe\n\nNote that there is a slight risk of breakage for Windows users:\nStrings like \"myplink\" or \"plink-0.83\" are no longer accepted.\n\n-------------\nI would probably vote for a), as Unix/Linux/Mac OS users don't use plink/tortoiseplink\nat all. \n-------------\nWhat about adding test-cases in t5601,\nthis will ease the documentation later.\nf:/util/plink\n/c/util/plink.exe\nf:/util/tortoiseplink\n/c/util/tortoiseplink.exe\n/usr/local/uplink/sshwrapper.sh\n\n\nOther opinions, other thoughts ?\n"},{"id":"260038","messageId":"20150426185216.GE322887@vauxhall.crustytoothpaste.net","threadId":"39136","inReplyTo":"553BBACA.70704@web.de","subject":"Re: [PATCH v2 2/2] connect: improve check for plink to reduce false positives","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-26T18:52:16Z","receivedAt":"2015-04-26T18:52:16Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Apr 25, 2015 at 06:03:22PM +0200, Torsten Bögershausen wrote:\n> (I'm not sute if the commit message describes the problem deep enough\n> for readers which are not familar with all the details of the original\n> report):\n> A feature implemented for Windows may break things for e.g. Linux users)\n> The following may read exaggerated, so please read it as a suggestion.\n> \n> The git_connect function has code to handle plink and tortoiseplink\n> specially, as they require different command line arguments compared to\n> OpenSSH (-P instead of -p, tortoiseplink uses -batch), commit 36ad53ffee6ed.\n> The special handling is only needed for Windows, and a sloppy\n> case-insensitve search for \"plink\" will trigger that an the extra\n> parameter \"-batch\" is added to the command line.\n> \n> This was observed on a Linux system where a command line including\n> \"/uplink_deploy/\" was used.\n> \n> There are different ways to improve the situation:\n> (The following mentions only plink, but assumes that tortoiseplink is handled\n> similar)\n> a) Disable the plink/tortoiseplink special handling on non-Windows systems\n> b) Tighten the search for plink:\n> Allow basename() == plink || !!strcasestr(ssh, \"plink.exe\")\n> c) Tighten the search for plink:\n> Allow basename() == plink || !!strcasestr(ssh, \"plink.exe\")\n> d) Tighten the check for tortoiseplink.\n> Today we set \"int putty\" to true when plink is found, and -batch\n> is set when tortoiseplink is not found.\n> This fixes the reported bug, but still has the -P problem.\n> e) Unix users typically use shell scripts and could use GIT_SSH_COMMAND.\n> Declare the GIT_SSH as not-well-documented (and to obsolete ?) for non-Windows systems,\n> \n> This patch implements c):\n> Extract the basename and compare it to plink, plink.exe respective\n> tortoiseplink/tortoiseplink.exe\n> \n> Note that there is a slight risk of breakage for Windows users:\n> Strings like \"myplink\" or \"plink-0.83\" are no longer accepted.\n\nI can certainly expand the commit message when I reroll.\n\n> -------------\n> I would probably vote for a), as Unix/Linux/Mac OS users don't use plink/tortoiseplink\n> at all.\n\nI have putty on my system:\n\nvauxhall ok % uname -a && which plink\nLinux vauxhall 3.16.0-4-amd64 #1 SMP Debian 3.16.7-ckt7-1 (2015-03-01) x86_64 GNU/Linux\n/usr/bin/plink\n\nWhile it's clearly not very common to use putty on Unix systems, it\ncertainly is possible and it would need to work the same way.\n\n> -------------\n> What about adding test-cases in t5601,\n> this will ease the documentation later.\n> f:/util/plink\n> /c/util/plink.exe\n> f:/util/tortoiseplink\n> /c/util/tortoiseplink.exe\n> /usr/local/uplink/sshwrapper.sh\n\nIt looks like there's already a framework for that, so sure, I can add\nsome tests.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"260041","messageId":"1430080212-396370-1-git-send-email-sandals@crustytoothpaste.net","threadId":"39136","inReplyTo":"1429914505-325708-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v3 0/3] Improve robustness of putty detection","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-26T20:30:09Z","receivedAt":"2015-04-26T20:30:09Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"This series improves detection of plink (the putty utility).\n\nWhile I was adding tests, I noticed that we had a broken test due to the\nuse of single quotes within a test, which resulted in the test always\nbeing skipped. Therefore, nobody noticed that the test was actually\nbroken.\n\nbrian m. carlson (3):\n connect: simplify SSH connection code path\n t5601: fix quotation error leading to skipped tests\n connect: improve check for plink to reduce false positives\n\n connect.c | 56 ++++++++++++++++++++++++++++++++++----------------------\n t/t5601-clone.sh | 35 ++++++++++++++++++++++++++++++++++-\n 2 files changed, 68 insertions(+), 23 deletions(-)\n\n-- \n2.3.5\n"},{"id":"260043","messageId":"1430080212-396370-2-git-send-email-sandals@crustytoothpaste.net","threadId":"39136","inReplyTo":"1430080212-396370-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v3 1/3] connect: simplify SSH connection code path","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-26T20:30:10Z","receivedAt":"2015-04-26T20:30:10Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The code path used in git_connect pushed the majority of the SSH\nconnection code into an else block, even though the if block returns.\nSimplify the code by eliminating the else block, as it is unneeded.\n\nSigned-off-by: Jeff King \nSigned-off-by: brian m. carlson \n---\n connect.c | 42 +++++++++++++++++++++---------------------\n 1 file changed, 21 insertions(+), 21 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 391d211..749a07b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -743,28 +743,28 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tfree(path);\n \t\t\t\tfree(conn);\n \t\t\t\treturn NULL;\n-\t\t\t} else {\n-\t\t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n-\t\t\t\tif (ssh) {\n-\t\t\t\t\tconn->use_shell = 1;\n-\t\t\t\t\tputty = 0;\n-\t\t\t\t} else {\n-\t\t\t\t\tssh = getenv(\"GIT_SSH\");\n-\t\t\t\t\tif (!ssh)\n-\t\t\t\t\t\tssh = \"ssh\";\n-\t\t\t\t\tputty = !!strcasestr(ssh, \"plink\");\n-\t\t\t\t}\n-\n-\t\t\t\targv_array_push(&conn->args, ssh);\n-\t\t\t\tif (putty && !strcasestr(ssh, \"tortoiseplink\"))\n-\t\t\t\t\targv_array_push(&conn->args, \"-batch\");\n-\t\t\t\tif (port) {\n-\t\t\t\t\t/* P is for PuTTY, p is for OpenSSH */\n-\t\t\t\t\targv_array_push(&conn->args, putty ? \"-P\" : \"-p\");\n-\t\t\t\t\targv_array_push(&conn->args, port);\n-\t\t\t\t}\n-\t\t\t\targv_array_push(&conn->args, ssh_host);\n \t\t\t}\n+\n+\t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n+\t\t\tif (ssh) {\n+\t\t\t\tconn->use_shell = 1;\n+\t\t\t\tputty = 0;\n+\t\t\t} else {\n+\t\t\t\tssh = getenv(\"GIT_SSH\");\n+\t\t\t\tif (!ssh)\n+\t\t\t\t\tssh = \"ssh\";\n+\t\t\t\tputty = !!strcasestr(ssh, \"plink\");\n+\t\t\t}\n+\n+\t\t\targv_array_push(&conn->args, ssh);\n+\t\t\tif (putty && !strcasestr(ssh, \"tortoiseplink\"))\n+\t\t\t\targv_array_push(&conn->args, \"-batch\");\n+\t\t\tif (port) {\n+\t\t\t\t/* P is for PuTTY, p is for OpenSSH */\n+\t\t\t\targv_array_push(&conn->args, putty ? \"-P\" : \"-p\");\n+\t\t\t\targv_array_push(&conn->args, port);\n+\t\t\t}\n+\t\t\targv_array_push(&conn->args, ssh_host);\n \t\t} else {\n \t\t\t/* remove repo-local variables from the environment */\n \t\t\tconn->env = local_repo_env;\n-- \n2.3.5\n"},{"id":"260040","messageId":"1430080212-396370-3-git-send-email-sandals@crustytoothpaste.net","threadId":"39136","inReplyTo":"1430080212-396370-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v3 2/3] t5601: fix quotation error leading to skipped tests","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-26T20:30:11Z","receivedAt":"2015-04-26T20:30:11Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"One of the tests in t5601 used single quotes to delimit an argument\ncontaining spaces. However, this caused test_expect_success to be\npassed three arguments instead of two, which in turn caused the test\nname to be treated as a prerequisite instead of a test name. As there\nwas no prerequisite called \"bracketed hostnames are still ssh\", the test\nwas always skipped.\n\nBecause this test was always skipped, the fact that it passed the\narguments in the wrong order was obscured. Use double quotes inside the\ntest and reorder the arguments so that the test runs and properly\nreflects the arguments that are passed to ssh.\n\nSigned-off-by: brian m. carlson \n---\n t/t5601-clone.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 1befc45..aae2324 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -332,7 +332,7 @@ test_expect_success !MINGW,!CYGWIN 'clone local path foo:bar' '\n \n test_expect_success 'bracketed hostnames are still ssh' '\n \tgit clone \"[myhost:123]:src\" ssh-bracket-clone &&\n-\texpect_ssh myhost '-p 123' src\n+\texpect_ssh \"-p 123\" myhost src\n '\n \n counter=0\n-- \n2.3.5\n"},{"id":"260042","messageId":"1430080212-396370-4-git-send-email-sandals@crustytoothpaste.net","threadId":"39136","inReplyTo":"1430080212-396370-1-git-send-email-sandals@crustytoothpaste.net","subject":"[PATCH v3 3/3] connect: improve check for plink to reduce false positives","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-26T20:30:12Z","receivedAt":"2015-04-26T20:30:12Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The git_connect function has code to handle plink and tortoiseplink\nspecially, as they require different command line arguments from\nOpenSSH (-P instead of -p for ports; tortoiseplink additionally requires\n-batch). However, the match was done by checking for \"plink\" anywhere\nin the string, which led to a GIT_SSH value containing \"uplink\" being\ntreated as an invocation of putty's plink.\n\nImprove the check by looking for \"plink\" or \"tortoiseplink\" (or those\nnames suffixed with \".exe\") only in the final component of the path.\nThis has the downside that a program such as \"plink-0.63\" would no\nlonger be recognized, but the increased robustness is likely worth it.\nAdd tests to cover these cases to avoid regressions.\n\nSigned-off-by: brian m. carlson \n---\n connect.c | 18 +++++++++++++++---\n t/t5601-clone.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 3 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 749a07b..c0144d8 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -724,7 +724,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tconn->in = conn->out = -1;\n \t\tif (protocol == PROTO_SSH) {\n \t\t\tconst char *ssh;\n-\t\t\tint putty;\n+\t\t\tint putty, tortoiseplink = 0;\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n \t\t\tget_host_and_port(&ssh_host, &port);\n@@ -750,14 +750,26 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tconn->use_shell = 1;\n \t\t\t\tputty = 0;\n \t\t\t} else {\n+\t\t\t\tconst char *base;\n+\t\t\t\tchar *ssh_dup;\n+\n \t\t\t\tssh = getenv(\"GIT_SSH\");\n \t\t\t\tif (!ssh)\n \t\t\t\t\tssh = \"ssh\";\n-\t\t\t\tputty = !!strcasestr(ssh, \"plink\");\n+\n+\t\t\t\tssh_dup = xstrdup(ssh);\n+\t\t\t\tbase = basename(ssh_dup);\n+\n+\t\t\t\ttortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n+\t\t\t\t\t!strcasecmp(base, \"tortoiseplink.exe\");\n+\t\t\t\tputty = !strcasecmp(base, \"plink\") ||\n+\t\t\t\t\t!strcasecmp(base, \"plink.exe\") || tortoiseplink;\n+\n+\t\t\t\tfree(ssh_dup);\n \t\t\t}\n \n \t\t\targv_array_push(&conn->args, ssh);\n-\t\t\tif (putty && !strcasestr(ssh, \"tortoiseplink\"))\n+\t\t\tif (tortoiseplink)\n \t\t\t\targv_array_push(&conn->args, \"-batch\");\n \t\t\tif (port) {\n \t\t\t\t/* P is for PuTTY, p is for OpenSSH */\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex aae2324..bfdaf75 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -296,6 +296,12 @@ setup_ssh_wrapper () {\n \t'\n }\n \n+copy_ssh_wrapper_as () {\n+\tcp \"$TRASH_DIRECTORY/ssh-wrapper\" \"$1\" &&\n+\tGIT_SSH=\"$1\" &&\n+\texport GIT_SSH\n+}\n+\n expect_ssh () {\n \ttest_when_finished '\n \t\t(cd \"$TRASH_DIRECTORY\" && rm -f ssh-expect && >ssh-output)\n@@ -335,6 +341,33 @@ test_expect_success 'bracketed hostnames are still ssh' '\n \texpect_ssh \"-p 123\" myhost src\n '\n \n+test_expect_success 'uplink is not treated as putty' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/uplink\" &&\n+\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-uplink &&\n+\texpect_ssh \"-p 123\" myhost src\n+'\n+\n+test_expect_success 'plink is treated specially (as putty)' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n+\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-plink-0 &&\n+\texpect_ssh \"-P 123\" myhost src\n+'\n+\n+test_expect_success 'plink.exe is treated specially (as putty)' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink.exe\" &&\n+\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-plink-1 &&\n+\texpect_ssh \"-P 123\" myhost src\n+'\n+\n+test_expect_success 'tortoiseplink is like putty, with extra arguments' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/tortoiseplink\" &&\n+\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-plink-2 &&\n+\texpect_ssh \"-batch -P 123\" myhost src\n+'\n+\n+# Reset the GIT_SSH environment variable for clone tests.\n+setup_ssh_wrapper\n+\n counter=0\n # $1 url\n # $2 none|host\n-- \n2.3.5\n"},{"id":"260044","messageId":"xmqq4mo2zgtz.fsf@gitster.dls.corp.google.com","threadId":"39136","inReplyTo":"1430080212-396370-1-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v3 0/3] Improve robustness of putty detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-26T22:04:56Z","receivedAt":"2015-04-26T22:04:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> While I was adding tests, I noticed that we had a broken test due to the\n> use of single quotes within a test, which resulted in the test always\n> being skipped.\n\nGood eyes. While fixing the test is necessary, we should also be\nable to improve the test framework to prevent such mistakes at the\nsame time.\n\nok 38 # skip\n git clone \"[myhost:123]:src\" ssh-bracket-clone &&\n expect_ssh myhost -p (missing bracketed hostnames are still\n ssh)\n\nThe test scripts are expected to take either 3 or 4 parameters, and\nthe extra parameter when it takes 4 is the comma separated list of\nprerequisites. \"bracketed hostnames are still ssh\" does not look\nlike prerequisites at all to us humans, and the framework should\nalso be able to notice that and barf, I would think.\n\nPerhaps something like this?\n\n t/test-lib-functions.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 0698ce7..0e4f2a6 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -348,11 +348,18 @@ test_declared_prereq () {\n \treturn 1\n }\n \n+test_verify_prereq () {\n+\ttest -z \"$test_prereq\" ||\n+\texpr >/dev/null \"$test_prereq\" : '^[A-Z0-9_,!]*$' ||\n+\terror \"bug in the test script: '$test_prereq' does not look like a prereq\"\n+}\n+\n test_expect_failure () {\n \ttest_start_\n \ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n \ttest \"$#\" = 2 ||\n \terror \"bug in the test script: not 2 or 3 parameters to test-expect-failure\"\n+\ttest_verify_prereq\n \texport test_prereq\n \tif ! test_skip \"$@\"\n \tthen\n@@ -372,6 +379,7 @@ test_expect_success () {\n \ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n \ttest \"$#\" = 2 ||\n \terror \"bug in the test script: not 2 or 3 parameters to test-expect-success\"\n+\ttest_verify_prereq\n \texport test_prereq\n \tif ! test_skip \"$@\"\n \tthen\n@@ -400,6 +408,7 @@ test_external () {\n \terror >&5 \"bug in the test script: not 3 or 4 parameters to test_external\"\n \tdescr=\"$1\"\n \tshift\n+\ttest_verify_prereq\n \texport test_prereq\n \tif ! test_skip \"$descr\" \"$@\"\n \tthen\n"},{"id":"260051","messageId":"302f37d0837581378cc0db33499d8297@www.dscho.org","threadId":"39136","inReplyTo":"1430080212-396370-4-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-04-27T07:57:12Z","receivedAt":"2015-04-27T07:57:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn 2015-04-26 22:30, brian m. carlson wrote:\n> The git_connect function has code to handle plink and tortoiseplink\n> specially, as they require different command line arguments from\n> OpenSSH (-P instead of -p for ports; tortoiseplink additionally requires\n> -batch). However, the match was done by checking for \"plink\" anywhere\n> in the string, which led to a GIT_SSH value containing \"uplink\" being\n> treated as an invocation of putty's plink.\n> \n> Improve the check by looking for \"plink\" or \"tortoiseplink\" (or those\n> names suffixed with \".exe\") only in the final component of the path.\n> This has the downside that a program such as \"plink-0.63\" would no\n> longer be recognized, but the increased robustness is likely worth it.\n> Add tests to cover these cases to avoid regressions.\n> \n> Signed-off-by: brian m. carlson \n\nI like it!\n\nAcked-by: Johannes Schindelin \n\nCiao,\nDscho\n"},{"id":"260058","messageId":"553E59C5.1000706@web.de","threadId":"39136","inReplyTo":"xmqq4mo2zgtz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 0/3] Improve robustness of putty detection","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-04-27T15:46:13Z","receivedAt":"2015-04-27T15:46:13Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 04/27/2015 12:04 AM, Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n> \n>> While I was adding tests, I noticed that we had a broken test due to the\n>> use of single quotes within a test, which resulted in the test always\n>> being skipped.\n> \n> Good eyes. While fixing the test is necessary, we should also be\n> able to improve the test framework to prevent such mistakes at the\n> same time.\n> \n> ok 38 # skip\n> git clone \"[myhost:123]:src\" ssh-bracket-clone &&\n> expect_ssh myhost -p (missing bracketed hostnames are still\n> ssh)\n> \n> The test scripts are expected to take either 3 or 4 parameters, and\n> the extra parameter when it takes 4 is the comma separated list of\n> prerequisites. \"bracketed hostnames are still ssh\" does not look\n> like prerequisites at all to us humans, and the framework should\n> also be able to notice that and barf, I would think.\n> \n> Perhaps something like this?\n> \n> t/test-lib-functions.sh | 9 +++++++++\n> 1 file changed, 9 insertions(+)\n> \n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 0698ce7..0e4f2a6 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -348,11 +348,18 @@ test_declared_prereq () {\n> \treturn 1\n> }\n> \n> +test_verify_prereq () {\n> +\ttest -z \"$test_prereq\" ||\n> +\texpr >/dev/null \"$test_prereq\" : '^[A-Z0-9_,!]*$' ||\n> +\terror \"bug in the test script: '$test_prereq' does not look like a prereq\"\n> +}\n> +\n> test_expect_failure () {\n> \ttest_start_\n> \ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n> \ttest \"$#\" = 2 ||\n> \terror \"bug in the test script: not 2 or 3 parameters to test-expect-failure\"\n> +\ttest_verify_prereq\n> \texport test_prereq\n> \tif ! test_skip \"$@\"\n> \tthen\n> @@ -372,6 +379,7 @@ test_expect_success () {\n> \ttest \"$#\" = 3 && { test_prereq=$1; shift; } || test_prereq=\n> \ttest \"$#\" = 2 ||\n> \terror \"bug in the test script: not 2 or 3 parameters to test-expect-success\"\n> +\ttest_verify_prereq\n> \texport test_prereq\n> \tif ! test_skip \"$@\"\n> \tthen\n> @@ -400,6 +408,7 @@ test_external () {\n> \terror >&5 \"bug in the test script: not 3 or 4 parameters to test_external\"\n> \tdescr=\"$1\"\n> \tshift\n> +\ttest_verify_prereq\n> \texport test_prereq\n> \tif ! test_skip \"$descr\" \"$@\"\n> \tthen\n> \nThanks for fixing my bugs :-)\n"},{"id":"260092","messageId":"20150428035344.GA24580@peff.net","threadId":"39136","inReplyTo":"1430080212-396370-4-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-28T03:53:44Z","receivedAt":"2015-04-28T03:53:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 26, 2015 at 08:30:12PM +0000, brian m. carlson wrote:\n\n> The git_connect function has code to handle plink and tortoiseplink\n> specially, as they require different command line arguments from\n> OpenSSH (-P instead of -p for ports; tortoiseplink additionally requires\n> -batch). However, the match was done by checking for \"plink\" anywhere\n> in the string, which led to a GIT_SSH value containing \"uplink\" being\n> treated as an invocation of putty's plink.\n> \n> Improve the check by looking for \"plink\" or \"tortoiseplink\" (or those\n> names suffixed with \".exe\") only in the final component of the path.\n> This has the downside that a program such as \"plink-0.63\" would no\n> longer be recognized, but the increased robustness is likely worth it.\n> Add tests to cover these cases to avoid regressions.\n\nThanks, I think this version looks good. It's possible that it's too\ntight, but I think it represents our best guess at what is reasonable,\nand we can only know more by getting it into the hands of users\n(hopefully while it is cooking during this release cycle, but I am not\nincredibly optimistic about such things).\n\n-Peff\n"},{"id":"260093","messageId":"20150428041521.GB24580@peff.net","threadId":"39136","inReplyTo":"xmqq4mo2zgtz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 0/3] Improve robustness of putty detection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-28T04:15:21Z","receivedAt":"2015-04-28T04:15:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 26, 2015 at 03:04:56PM -0700, Junio C Hamano wrote:\n\n> The test scripts are expected to take either 3 or 4 parameters, and\n> the extra parameter when it takes 4 is the comma separated list of\n> prerequisites. \"bracketed hostnames are still ssh\" does not look\n> like prerequisites at all to us humans, and the framework should\n> also be able to notice that and barf, I would think.\n> \n> Perhaps something like this?\n\nI like it. I wondered if we could even recognize a known set of prereqs\n(e.g., say \"I don't know about the FOO prereq; did you forget to\ntest_lazy_prereq it?\"), but I don't think we can. Some of the prereqs\nare set by arbitrary code, and when they are not set, we don't call a\n\"test_do_not_set_prereq FOO\". Enforcing a sane syntax is almost as\ngood, and seems pretty easy to implement.\n\n> +test_verify_prereq () {\n> +\ttest -z \"$test_prereq\" ||\n> +\texpr >/dev/null \"$test_prereq\" : '^[A-Z0-9_,!]*$' ||\n> +\terror \"bug in the test script: '$test_prereq' does not look like a prereq\"\n> +}\n\nThe leading \"^\" is unnecessary in an expr regexp, as such expressions\nare always left-anchored. And according to POSIX, even undesirable\ndue to hysterical raisins. You do still need the '$' to anchor the end.\n\nI was surprised that the regexp does not match the empty string itself\n(making the initial \"test -z\" redundant), but it does not seem to with\nmy version of expr. Weird. I think it is because the exit value is not\n\"did it match\" but \"is the return value of the expression non-zero\", and\nof course we matched zero characters.\n\nAt any rate, we are probably much better off having the initial \"test\n-z\" there as an optimization, anyway. \"expr\" is not usually a built-in,\nand otherwise we add an extra fork to each test invocation (not\nsomething I expect matters under Linux, but I know Windows folks are\nvery sensitive to it).\n\n-Peff\n"},{"id":"260153","messageId":"20150429013831.GB5015@vauxhall.crustytoothpaste.net","threadId":"39136","inReplyTo":"xmqq4mo2zgtz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 0/3] Improve robustness of putty detection","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-04-29T01:38:31Z","receivedAt":"2015-04-29T01:38:31Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Apr 26, 2015 at 03:04:56PM -0700, Junio C Hamano wrote:\n> Good eyes. While fixing the test is necessary, we should also be\n> able to improve the test framework to prevent such mistakes at the\n> same time.\n> \n> ok 38 # skip\n> git clone \"[myhost:123]:src\" ssh-bracket-clone &&\n> expect_ssh myhost -p (missing bracketed hostnames are still\n> ssh)\n> \n> The test scripts are expected to take either 3 or 4 parameters, and\n> the extra parameter when it takes 4 is the comma separated list of\n> prerequisites. \"bracketed hostnames are still ssh\" does not look\n> like prerequisites at all to us humans, and the framework should\n> also be able to notice that and barf, I would think.\n> \n> Perhaps something like this?\n\nI think this is a good change. I haven't tested to see if we have any\nother issues in the testsuite that this would expose, but if so, they\nshould be easy enough to fix up.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"264950","messageId":"20150626131524.GA2626@peff.net","threadId":"39136","inReplyTo":"1430080212-396370-4-git-send-email-sandals@crustytoothpaste.net","subject":"Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-26T13:15:24Z","receivedAt":"2015-06-26T13:15:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 26, 2015 at 08:30:12PM +0000, brian m. carlson wrote:\n\n> The git_connect function has code to handle plink and tortoiseplink\n> specially, as they require different command line arguments from\n> OpenSSH (-P instead of -p for ports; tortoiseplink additionally requires\n> -batch). However, the match was done by checking for \"plink\" anywhere\n> in the string, which led to a GIT_SSH value containing \"uplink\" being\n> treated as an invocation of putty's plink.\n> \n> Improve the check by looking for \"plink\" or \"tortoiseplink\" (or those\n> names suffixed with \".exe\") only in the final component of the path.\n> This has the downside that a program such as \"plink-0.63\" would no\n> longer be recognized, but the increased robustness is likely worth it.\n> Add tests to cover these cases to avoid regressions.\n\nFYI, this ended up biting me today. We have some integration tests that\nmake sure we can clone over putty, and we wrap plink in a\n\"plink-wrapper.sh\" script that tweaks a few extra options. That used to\nmatch under the old scheme, but not the new. It would also match if we\nlooked for \"plink\" anywhere in the basename (but not in leading\ndirectories).\n\nI was able to work around it pretty easily by changing our test setup,\nbut I thought I would include it here as a data point. It's probably not\nthat representative of real-world users.\n\n-Peff\n"},{"id":"264957","messageId":"xmqq381ewiln.fsf@gitster.dls.corp.google.com","threadId":"39136","inReplyTo":"20150626131524.GA2626@peff.net","subject":"Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-26T16:16:20Z","receivedAt":"2015-06-26T16:16:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Apr 26, 2015 at 08:30:12PM +0000, brian m. carlson wrote:\n>\n>> Improve the check by looking for \"plink\" or \"tortoiseplink\" (or those\n>> names suffixed with \".exe\") only in the final component of the path.\n>> This has the downside that a program such as \"plink-0.63\" would no\n>> longer be recognized, but the increased robustness is likely worth it.\n>> Add tests to cover these cases to avoid regressions.\n>\n> FYI, this ended up biting me today. We have some integration tests that\n> make sure we can clone over putty, and we wrap plink in a\n> \"plink-wrapper.sh\" script that tweaks a few extra options. That used to\n> match under the old scheme, but not the new. It would also match if we\n> looked for \"plink\" anywhere in the basename (but not in leading\n> directories).\n\nSo this was a minor regression? ;-)\n\n> I was able to work around it pretty easily by changing our test setup,\n> but I thought I would include it here as a data point. It's probably not\n> that representative of real-world users.\n\nI'd imagine that \"/usr/local/github/wrapped/bin/plink\" may be a more\nappropriate name to install that wrapper as than \"plink-wrapper.sh\",\nbut then people would need to think how to help that wrapper find\nthe real plink, so...\n"},{"id":"264959","messageId":"20150626162701.GA32123@peff.net","threadId":"39136","inReplyTo":"xmqq381ewiln.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-26T16:27:01Z","receivedAt":"2015-06-26T16:27:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 26, 2015 at 09:16:20AM -0700, Junio C Hamano wrote:\n\n> > FYI, this ended up biting me today. We have some integration tests that\n> > make sure we can clone over putty, and we wrap plink in a\n> > \"plink-wrapper.sh\" script that tweaks a few extra options. That used to\n> > match under the old scheme, but not the new. It would also match if we\n> > looked for \"plink\" anywhere in the basename (but not in leading\n> > directories).\n> \n> So this was a minor regression? ;-)\n\nYes. :)\n\n> > I was able to work around it pretty easily by changing our test setup,\n> > but I thought I would include it here as a data point. It's probably not\n> > that representative of real-world users.\n> \n> I'd imagine that \"/usr/local/github/wrapped/bin/plink\" may be a more\n> appropriate name to install that wrapper as than \"plink-wrapper.sh\",\n> but then people would need to think how to help that wrapper find\n> the real plink, so...\n\nIt's the test suite for the server side of our git infrastructure, so\nnothing gets installed. It's more like:\n\n export GIT_SSH=$PROJECT_ROOT/test/plink-wrapper.sh\n export REAL_PLINK=$PROJECT_ROOT/vendor/putty/plink\n git clone localhost:foo.git\n\nand the wrapper knows to chain to $REAL_PLINK. So it was actually pretty\neasy to swap, without any hacks to avoid recursing to ourselves in the\n$PATH.\n\nI doubt it is a problem for most people, because I don't imagine they\nare writing test suites for git-related software.\n\n-Peff\n"},{"id":"264972","messageId":"0838aaf747ce0db5a83de107495270cf@www.dscho.org","threadId":"39136","inReplyTo":"20150626162701.GA32123@peff.net","subject":"Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-06-26T17:13:15Z","receivedAt":"2015-06-26T17:13:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn 2015-06-26 18:27, Jeff King wrote:\n> On Fri, Jun 26, 2015 at 09:16:20AM -0700, Junio C Hamano wrote:\n> \n>> > FYI, this ended up biting me today. We have some integration tests that\n>> > make sure we can clone over putty, and we wrap plink in a\n>> > \"plink-wrapper.sh\" script that tweaks a few extra options. That used to\n>> > match under the old scheme, but not the new. It would also match if we\n>> > looked for \"plink\" anywhere in the basename (but not in leading\n>> > directories).\n>>\n>> So this was a minor regression? ;-)\n> \n> Yes. :)\n> \n>> > I was able to work around it pretty easily by changing our test setup,\n>> > but I thought I would include it here as a data point. It's probably not\n>> > that representative of real-world users.\n>>\n>> I'd imagine that \"/usr/local/github/wrapped/bin/plink\" may be a more\n>> appropriate name to install that wrapper as than \"plink-wrapper.sh\",\n>> but then people would need to think how to help that wrapper find\n>> the real plink, so...\n> \n> It's the test suite for the server side of our git infrastructure, so\n> nothing gets installed. It's more like:\n> \n> export GIT_SSH=$PROJECT_ROOT/test/plink-wrapper.sh\n> export REAL_PLINK=$PROJECT_ROOT/vendor/putty/plink\n> git clone localhost:foo.git\n> \n> and the wrapper knows to chain to $REAL_PLINK. So it was actually pretty\n> easy to swap, without any hacks to avoid recursing to ourselves in the\n> $PATH.\n> \n> I doubt it is a problem for most people, because I don't imagine they\n> are writing test suites for git-related software.\n\nSorry to be so unavailable... day-job and Git for Windows[*1*], what can I say.\n\nWould it help you if we detected ^plink[^a-zA-Z]?\n\nCiao,\nDscho\n\nFootnote *1*: took me friggin' 9 1/2 hours to figure this one out: https://github.com/Alexpux/MSYS2-packages/pull/275\n"},{"id":"264974","messageId":"20150626172339.GA1945@peff.net","threadId":"39136","inReplyTo":"0838aaf747ce0db5a83de107495270cf@www.dscho.org","subject":"Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-26T17:23:39Z","receivedAt":"2015-06-26T17:23:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 26, 2015 at 07:13:15PM +0200, Johannes Schindelin wrote:\n\n> > It's the test suite for the server side of our git infrastructure, so\n> > nothing gets installed. It's more like:\n> > \n> > export GIT_SSH=$PROJECT_ROOT/test/plink-wrapper.sh\n> > export REAL_PLINK=$PROJECT_ROOT/vendor/putty/plink\n> > git clone localhost:foo.git\n> > \n> > and the wrapper knows to chain to $REAL_PLINK. So it was actually pretty\n> > easy to swap, without any hacks to avoid recursing to ourselves in the\n> > $PATH.\n> > \n> > I doubt it is a problem for most people, because I don't imagine they\n> > are writing test suites for git-related software.\n> \n> Sorry to be so unavailable... day-job and Git for Windows[*1*], what can I say.\n\nNo problem. I don't envy you. :)\n\n> Would it help you if we detected ^plink[^a-zA-Z]?\n\nIn our we would have needed \"^plink[^a-zA-Z-.]\". I think there's no real\n\"right\" answer here, as you can come up with hypotheticals that work and\ndon't work with just about every pattern. I was less trying to advocate\nfor loosening, and more just providing a data point to the list.\n\nIf we were to do any loosening, I'd probably suggest \"^plink.*\" (in the\nbasename).\n\n-Peff\n"},{"id":"264996","messageId":"20150626204356.GB263667@vauxhall.crustytoothpaste.net","threadId":"39136","inReplyTo":"20150626131524.GA2626@peff.net","subject":"Re: [PATCH v3 3/3] connect: improve check for plink to reduce false positives","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2015-06-26T20:43:56Z","receivedAt":"2015-06-26T20:43:56Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Jun 26, 2015 at 09:15:24AM -0400, Jeff King wrote:\n> On Sun, Apr 26, 2015 at 08:30:12PM +0000, brian m. carlson wrote:\n> > Improve the check by looking for \"plink\" or \"tortoiseplink\" (or those\n> > names suffixed with \".exe\") only in the final component of the path.\n> > This has the downside that a program such as \"plink-0.63\" would no\n> > longer be recognized, but the increased robustness is likely worth it.\n> > Add tests to cover these cases to avoid regressions.\n> \n> FYI, this ended up biting me today. We have some integration tests that\n> make sure we can clone over putty, and we wrap plink in a\n> \"plink-wrapper.sh\" script that tweaks a few extra options. That used to\n> match under the old scheme, but not the new. It would also match if we\n> looked for \"plink\" anywhere in the basename (but not in leading\n> directories).\n> \n> I was able to work around it pretty easily by changing our test setup,\n> but I thought I would include it here as a data point. It's probably not\n> that representative of real-world users.\n\nThanks for the data point. While we don't use plink at $DAYJOB, this is\nthe kind of test we might well perform. I expect it's most likely to\nhit people in test setups like this, but if it turns out to be a\nproblem, we can certainly loosen it or if necessary, revert it.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"}]}