{"thread":{"id":"44806","subject":"[PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND","startedAt":"2017-01-02T12:09:19Z","lastAt":"2017-02-01T23:08:01Z","messageCount":38,"participants":["Johannes Schindelin","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"308619","messageId":"2ff29a4d00e0e13d460122d8008e762361ca90aa.1483358673.git.johannes.schindelin@gmx.de","threadId":"44806","inReplyTo":null,"subject":"[PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-02T12:09:03Z","receivedAt":"2017-01-02T12:09:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Segev Finer \n\nGit for Windows has special support for the popular SSH client PuTTY:\nwhen using PuTTY's non-interactive version (\"plink.exe\"), we use the -P\noption to specify the port rather than OpenSSH's -p option. TortoiseGit\nships with its own, forked version of plink.exe, that adds support for\nthe -batch option, and for good measure we special-case that, too.\n\nHowever, this special-casing of PuTTY only covers the case where the\nuser overrides the SSH command via the environment variable GIT_SSH\n(which allows specifying the name of the executable), not\nGIT_SSH_COMMAND (which allows specifying a full command, including\nadditional command-line options).\n\nWhen users want to pass any additional arguments to (Tortoise-)Plink,\nsuch as setting a private key, they are required to either use a shell\nscript named plink or tortoiseplink or duplicate the logic that is\nalready in Git for passing the correct style of command line arguments,\nwhich can be difficult, error prone and annoying to get right.\n\nThis patch simply reuses the existing logic and expands it to cover\nGIT_SSH_COMMAND, too.\n\nNote: it may look a little heavy-handed to duplicate the entire\ncommand-line and then split it, only to extract the name of the\nexecutable. However, this is not a performance-critical code path, and\nthe code is much more readable this way.\n\nSigned-off-by: Segev Finer \nSigned-off-by: Johannes Schindelin \n---\nPublished-As: https://github.com/dscho/git/releases/tag/putty-w-args-v1\nFetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v1\n\n\n\tOriginal Pull Request:\n\thttps://github.com/git-for-windows/git/pull/1006\n\n connect.c | 23 ++++++++++++++++-------\n t/t5601-clone.sh | 15 +++++++++++++++\n 2 files changed, 31 insertions(+), 7 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 8cb93b0720..c81f77001b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -772,6 +772,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\tint putty = 0, tortoiseplink = 0;\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n+\t\t\tchar *ssh_argv0 = NULL;\n \t\t\ttransport_check_allowed(\"ssh\");\n \t\t\tget_host_and_port(&ssh_host, &port);\n \n@@ -792,10 +793,15 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t}\n \n \t\t\tssh = get_ssh_command();\n-\t\t\tif (!ssh) {\n-\t\t\t\tconst char *base;\n-\t\t\t\tchar *ssh_dup;\n-\n+\t\t\tif (ssh) {\n+\t\t\t\tchar *split_ssh = xstrdup(ssh);\n+\t\t\t\tconst char **ssh_argv;\n+\n+\t\t\t\tif (split_cmdline(split_ssh, &ssh_argv))\n+\t\t\t\t\tssh_argv0 = xstrdup(ssh_argv[0]);\n+\t\t\t\tfree(split_ssh);\n+\t\t\t\tfree((void *)ssh_argv);\n+\t\t\t} else {\n \t\t\t\t/*\n \t\t\t\t * GIT_SSH is the no-shell version of\n \t\t\t\t * GIT_SSH_COMMAND (and must remain so for\n@@ -807,8 +813,11 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tif (!ssh)\n \t\t\t\t\tssh = \"ssh\";\n \n-\t\t\t\tssh_dup = xstrdup(ssh);\n-\t\t\t\tbase = basename(ssh_dup);\n+\t\t\t\tssh_argv0 = xstrdup(ssh);\n+\t\t\t}\n+\n+\t\t\tif (ssh_argv0) {\n+\t\t\t\tconst char *base = basename(ssh_argv0);\n \n \t\t\t\ttortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n \t\t\t\t\t!strcasecmp(base, \"tortoiseplink.exe\");\n@@ -816,7 +825,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\t\t!strcasecmp(base, \"plink\") ||\n \t\t\t\t\t!strcasecmp(base, \"plink.exe\");\n \n-\t\t\t\tfree(ssh_dup);\n+\t\t\t\tfree(ssh_argv0);\n \t\t\t}\n \n \t\t\targv_array_push(&conn->args, ssh);\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex a433394200..5b228e2675 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -386,6 +386,21 @@ test_expect_success 'tortoiseplink is like putty, with extra arguments' '\n \texpect_ssh \"-batch -P 123\" myhost src\n '\n \n+test_expect_success 'double quoted plink.exe in GIT_SSH_COMMAND' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink.exe\" &&\n+\tGIT_SSH_COMMAND=\"\\\"$TRASH_DIRECTORY/plink.exe\\\" -v\" \\\n+\t\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-plink-3 &&\n+\texpect_ssh \"-v -P 123\" myhost src\n+'\n+\n+SQ=\"'\"\n+test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink.exe\" &&\n+\tGIT_SSH_COMMAND=\"$SQ$TRASH_DIRECTORY/plink.exe$SQ -v\" \\\n+\t\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-plink-4 &&\n+\texpect_ssh \"-v -P 123\" myhost src\n+'\n+\n # Reset the GIT_SSH environment variable for clone tests.\n setup_ssh_wrapper\n \n\nbase-commit: e05806da9ec4aff8adfed142ab2a2b3b02e33c8c\n-- \n2.11.0.rc3.windows.1\n"},{"id":"308944","messageId":"xmqqy3ym1dsc.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"2ff29a4d00e0e13d460122d8008e762361ca90aa.1483358673.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-08T02:33:23Z","receivedAt":"2017-01-08T02:33:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> +\t\t\tif (ssh) {\n> +\t\t\t\tchar *split_ssh = xstrdup(ssh);\n> +\t\t\t\tconst char **ssh_argv;\n> +\n> +\t\t\t\tif (split_cmdline(split_ssh, &ssh_argv))\n> +\t\t\t\t\tssh_argv0 = xstrdup(ssh_argv[0]);\n> +\t\t\t\tfree(split_ssh);\n> +\t\t\t\tfree((void *)ssh_argv);\n> +\t\t\t} else {\n> \t\t\t\t/*\n> \t\t\t\t * GIT_SSH is the no-shell version of\n> \t\t\t\t * GIT_SSH_COMMAND (and must remain so for\n> @@ -807,8 +813,11 @@ struct child_process *git_connect(int fd[2], const char *url,\n> \t\t\t\tif (!ssh)\n> \t\t\t\t\tssh = \"ssh\";\n> \n> -\t\t\t\tssh_dup = xstrdup(ssh);\n> -\t\t\t\tbase = basename(ssh_dup);\n> +\t\t\t\tssh_argv0 = xstrdup(ssh);\n> +\t\t\t}\n> +\n> +\t\t\tif (ssh_argv0) {\n> +\t\t\t\tconst char *base = basename(ssh_argv0);\n> \n> \t\t\t\ttortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n> \t\t\t\t\t!strcasecmp(base, \"tortoiseplink.exe\");\n\nI suspect that this will break when GIT_SSH_COMMAND, which is meant\nto be processed by the shell, hence the user can write anything,\nbegins with a one-shot environment variable assignment, e.g.\n\n\t[core] sshcommand = VAR1=VAL1 VAR2=VAL2 //path/to/tortoiseplink\n\nOne possible unintended side effect of this patch is when VAL1 ends\nwith /plink (or /tortoiseplink) and the command is not either of\nthese, in which case the \"tortoiseplink\" and \"putty\" variables will\ntweak the command line for an SSH implementation that does not want\nsuch a tweak to be made. There may be other unintended fallouts.\n\nSorry, no concrete suggestion to get this to work comes to my mind\noffhand. \n\nPerhaps give an explicit way to force \"tortoiseplink\" and \"putty\"\nvariables without looking at and guessing from the pathname, so that\nthe solution does not have to split and peek the command line?\n\n\n connect.c | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex 8cb93b0720..1768122456 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -691,6 +691,23 @@ static const char *get_ssh_command(void)\n \treturn NULL;\n }\n \n+static void get_ssh_variant(int *tortoiseplink, int *putty)\n+{\n+\tconst char *variant;\n+\tif (!git_config_get_string_const(\"ssh.variant\", &variant))\n+\t\treturn;\n+\tif (!strcmp(variant, \"tortoiseplink\")) {\n+\t\t*tortoiseplink = 1;\n+\t\t*putty = 1;\n+\t} else if (!strcmp(variant, \"putty\")) {\n+\t\t*tortoiseplink = 0;\n+\t\t*putty = 1;\n+\t} else {\n+\t\t*tortoiseplink = 0;\n+\t\t*putty = 0;\n+\t}\n+}\n+\n /*\n * This returns a dummy child_process if the transport protocol does not\n * need fork(2), or a struct child_process object if it does. Once done,\n@@ -824,6 +841,9 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\targv_array_push(&conn->args, \"-4\");\n \t\t\telse if (flags & CONNECT_IPV6)\n \t\t\t\targv_array_push(&conn->args, \"-6\");\n+\n+\t\t\tget_ssh_variant(&tortoiseplink, &putty);\n+\n \t\t\tif (tortoiseplink)\n \t\t\t\targv_array_push(&conn->args, \"-batch\");\n \t\t\tif (port) {\n"},{"id":"308971","messageId":"xmqq4m1911mf.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"xmqqy3ym1dsc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-09T01:08:24Z","receivedAt":"2017-01-09T01:08:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I suspect that this will break when GIT_SSH_COMMAND, which is meant\n> to be processed by the shell, hence the user can write anything,\n> begins with a one-shot environment variable assignment, e.g.\n>\n> \t[core] sshcommand = VAR1=VAL1 VAR2=VAL2 //path/to/tortoiseplink\n>\n> One possible unintended side effect of this patch is when VAL1 ends\n> with /plink (or /tortoiseplink) and the command is not either of\n> these, in which case the \"tortoiseplink\" and \"putty\" variables will\n> tweak the command line for an SSH implementation that does not want\n> such a tweak to be made. There may be other unintended fallouts.\n\nThinking about this further, as the sshcommand (or GIT_SSH_COMMAND)\ninterface takes any shell-interpretable commands, the first \"token\"\nyou see is not limited to a one-shot environment assignment. It\ncould even be \"cmd1 && cmd2 && ...\" or even:\n\n\tif test -f ${TPLINK:=//path/to/tortoiseplink}\n\tthen\n\t\texec \"$TPLINK\" \"$@\"\n\telif test -f ${PLINK:=//path/to/plink}\n\t\texec \"$PLINK\" \"$@\"\n\telse\n\t\techo >&2 no usable ssh on this host\n\tfi\n\nWorse, the above may be in \"myssh\" script found on user's PATH and\nthen core.sshcommand may be set to\n\n\tTPLINK=//my/path/to/tortoiseplink PLINK=//my/path/to/plink myssh\n\nin the configuration the user uses on multiple hosts, some have\ntortoiseplink installed and some do not. The idea the user who set\nit up may be to use whatever available depending on the host.\n\nThe posted patch would be confused that we are using tortoiseplink\nbut the \"myssh\" script would correctly notice that on a particular\nhost it is unavailable and choose to use plink instead.\n\n> Sorry, no concrete suggestion to get this to work comes to my mind\n> offhand. \n>\n> Perhaps give an explicit way to force \"tortoiseplink\" and \"putty\"\n> variables without looking at and guessing from the pathname, so that\n> the solution does not have to split and peek the command line?\n\nA user with such an elaborate set-up with multiple hosts with\ndifferent configurations would likely to want to say \"Just give me a\nregular SSH command line, and in my 'myssh' script I'll futz with\n-p/-P differences and such, as I'm writing a small script to cope\nwith Tortoiseplink and Puttyplink anyway\". With a separate,\nexplicit configuration variable to tell Git what variant of SSH you\nhave, even such a user can be served (she would just set ssh.variant\nto \"vanilla\" or whatever that is not \"tortoiseplink\" or \"plink\").\n\n\n\n.\n\n"},{"id":"309002","messageId":"alpine.DEB.2.20.1701090825510.3469@virtualbox","threadId":"44806","inReplyTo":"xmqq4m1911mf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-09T07:46:19Z","receivedAt":"2017-01-09T07:47:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sun, 8 Jan 2017, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > I suspect that this will break when GIT_SSH_COMMAND, which is meant\n> > to be processed by the shell, hence the user can write anything,\n> > begins with a one-shot environment variable assignment, e.g.\n> >\n> > \t[core] sshcommand = VAR1=VAL1 VAR2=VAL2 //path/to/tortoiseplink\n> >\n> > One possible unintended side effect of this patch is when VAL1 ends\n> > with /plink (or /tortoiseplink) and the command is not either of\n> > these, in which case the \"tortoiseplink\" and \"putty\" variables will\n> > tweak the command line for an SSH implementation that does not want\n> > such a tweak to be made. There may be other unintended fallouts.\n> \n> Thinking about this further, as the sshcommand (or GIT_SSH_COMMAND)\n> interface takes any shell-interpretable commands, the first \"token\"\n> you see is not limited to a one-shot environment assignment. It\n> could even be \"cmd1 && cmd2 && ...\" or even:\n> \n> \tif test -f ${TPLINK:=//path/to/tortoiseplink}\n> \tthen\n> \t\texec \"$TPLINK\" \"$@\"\n> \telif test -f ${PLINK:=//path/to/plink}\n> \t\texec \"$PLINK\" \"$@\"\n> \telse\n> \t\techo >&2 no usable ssh on this host\n> \tfi\n\nSure, it could be all of that. It could even blast through the environment\nlimits in some environments on some platforms, but not on others. It could\nautomatically transfer bitcoins whenever a connection to github.com is\nattempted. It could start the camera and build a time-lapse of \"every time\nI pushed or fetched a repository\", as an art project. It could shut down\nthe computer.\n\nIt could do all of that.\n\nIn practice, however, what users realistically do is to use\nGIT_SSH_COMMAND whenever they need to override the ssh command with\noptions.\n\nThis is the common case, and that is what we must make less cumbersome to\nuse.\n\nIf you feel strongly about your contrived examples possibly being affected\nby this patch, we could easily make this conditional on\n\n1) no '&&' or '||' being found on the command-line, and\n2) argv[0] not containing an '='\n\nAnother approach would be to verify that argv[i] starts with '-' for\nnon-zero i.\n\nBut do we really need to do that?\n\nLet's have a very brief look at the amount of work required to come up\nwith a false positive (I am not so much concerned about any power user\nwriting elaborate shell expressions that may call plink.exe: those power\nusers will simply have to continue to use their own -P => -p and -batch\nhacks):\n\nThe logic kicks in if the split command-line's first component has a\nbasename `plink` or `tortoiseplink` (possibly with `.exe` suffix). The\nonly way this logic can kick in by mistake is if the first component is\n*not* the command to call *and* if the first component *still* has a\nbasename of `plink` or `tortoiseplink`.\n\nThat means that the user has to specify something like\n\n\tHAHAHA_IT_IS_NOT=/plink.exe ssh\n\nas GIT_SSH_COMMAND.\n\nNow, let's take a *very* brief look at the question how likely it is to\nhave a basename of `plink` or `tortoiseplink`. Remember, there are two\nways that the basename can be a specific term: either the original string\nis already equal to that term, or it ends in a slash followed by the term.\nThat is, either the first component refers to plink.exe/tortoiseplink.exe, i.e.\nit would be a *true* positive. Or the first component would end in\n\"/plink\" or \"/tortoiseplink\" (possibly with the `.exe` suffix) *and not be\na command*. But how likely is it to specify a non-command (such as a\nper-process variable assignment) that specifically mentions plink or\ntortoiseplink?\n\nIs it even realistic to expect users to specify values for GIT_SSH_COMMAND\nthat contain \"plink\" as a substring when they do *not* want to call plink\nat all?\n\nAfter thinking a bit longer than I had originally planned about it, my\nanswer is: no. No, I do not think it is realistic. I fully expect *no*\nuser to have a GIT_SSH_COMMAND containing even a substring \"plink\"\n*anywhere*, unless they do, in fact, want to call plink or tortoiseplink.\n\nMy main aim with Git for Windows is to improve the user experience, and\nthe patch in question does do that. Of course, I also try to avoid\nbreaking existing setups while improving the user experience, and\nI believe that it is safe to assume that the patch in question in all\nlikelihood does not break any existing setup.\n\nCiao,\nDscho\n"},{"id":"309009","messageId":"xmqqh958y44c.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"alpine.DEB.2.20.1701090825510.3469@virtualbox","subject":"Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-09T09:28:03Z","receivedAt":"2017-01-09T09:28:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> If you feel strongly about your contrived examples possibly being\n> affected by this patch, we could easily make this conditional on\n>\n> 1) no '&&' or '||' being found on the command-line, and\n> 2) argv[0] not containing an '='\n>\n> Another approach would be to verify that argv[i] starts with '-' for\n> non-zero i.\n>\n> But do we really need to do that?\n\nNo. An explicit way to override an incorrect guess is sufficient\nand necessary. The above two-item list you gave will be just part\nof the machinery to make an incorrect guess. The auto-detection in\nthe posted patch should cover many users' use case and I do not\nthink it needs to be extended further to make it more brittle, as by\ndefinition its guess cannot be perfect. Just keep it simple and\ngive a separate escape hatch.\n\n> That means that the user has to specify something like\n>\n> \tHAHAHA_IT_IS_NOT=/plink.exe ssh\n>\n> as GIT_SSH_COMMAND.\n\nMy second message was to clarify that \"VAR1=VAL2 command\" is NOT a\ncontrived example, and this response indicates that I somehow failed\nto convey that to you. The \"if tortoiseplink exists (and the end\nuser can override the location with an environment), use that, and\nif PuTTY plink exists (ditto), use that instead\" in a \"myssh\"\nscript, and use it as core.sshcommand with the environment to\noverride my custom installation location to these two programs,\nwould be what I would do when I get two Windows machines, with these\nvariants of SSH on each. So take the second message as a bug report\nagainst the version of Git for Windows you ship with the patch in\nquestion.\n\nThe auto-detection may work for many people and that is a great\nthing. I failed to say that in my message, as I thought that was\nobvious. But it is important to plan to cope with the case where it\ndoes not work. The usual practice around here is to say \"the it may\nnot necessarily work for everybody, so lets be prepared to add an\nexplicit override if it turns out to be necessary\". \n\nThe second message, which you are responding to, was meant to be a\nbug report from the future, telling us that an override is needed,\nshowing that we do not have to wait for a bug report to act on.\n\n"},{"id":"309022","messageId":"alpine.DEB.2.20.1701091207480.3469@virtualbox","threadId":"44806","inReplyTo":"xmqqh958y44c.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-09T11:13:58Z","receivedAt":"2017-01-09T11:15:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 9 Jan 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > If you feel strongly about your contrived examples possibly being\n> > affected by this patch, we could easily make this conditional on\n> >\n> > 1) no '&&' or '||' being found on the command-line, and\n> > 2) argv[0] not containing an '='\n> >\n> > Another approach would be to verify that argv[i] starts with '-' for\n> > non-zero i.\n> >\n> > But do we really need to do that?\n> \n> No.\n\nExactly.\n\n> > That means that the user has to specify something like\n> >\n> > \tHAHAHA_IT_IS_NOT=/plink.exe ssh\n> >\n> > as GIT_SSH_COMMAND.\n> \n> My second message was to clarify that \"VAR1=VAL2 command\" is NOT a\n> contrived example, and this response indicates that I somehow failed\n> to convey that to you.\n\nIndeed. The quite contrived example was about a script that chooses\nbetween plink and tortoiseplink (and fails to call anything else). And it\nfailed to convince me.\n\nBut since you seem to convinced that a future bug report like this may\nhappen (I am not, and it contradicts my conviction that one should cross a\nbridge only when reaching it, but whatever), how about this, on top:\n\n-- snipsnap --\ndiff --git a/connect.c b/connect.c\nindex c81f77001b..b990dd6190 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -797,7 +797,8 @@ struct child_process *git_connect(int fd[2], const\nchar *url,\n \t\t\t\tchar *split_ssh = xstrdup(ssh);\n \t\t\t\tconst char **ssh_argv;\n \n-\t\t\t\tif (split_cmdline(split_ssh, &ssh_argv))\n+\t\t\t\tif (split_cmdline(split_ssh, &ssh_argv) &&\n+\t\t\t\t !strchr(ssh_argv[0], '='))\n \t\t\t\t\tssh_argv0 = xstrdup(ssh_argv[0]);\n \t\t\t\tfree(split_ssh);\n \t\t\t\tfree((void *)ssh_argv);\n\n"},{"id":"309031","messageId":"xmqqmvf0wc2h.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"alpine.DEB.2.20.1701091207480.3469@virtualbox","subject":"Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-09T14:19:18Z","receivedAt":"2017-01-09T14:19:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> > But do we really need to do that?\n>> \n>> No.\n>\n> Exactly.\n\n> But since you seem to convinced that a future bug report like this may\n> happen (I am not, and it contradicts my conviction that one should cross a\n> bridge only when reaching it, but whatever), how about this, on top:\n\nI do not understand why you keep arguing.\n\nWith No-Exactly exchange, I thought that we established that\n\n * The original auto-detection for GIT_SSH is battle tested and has\n worked for us very well. It may probably have covered 99.9% of\n the population, and we haven't heard complaints from the\n remaining 0.1%.\n\n * The added support for GIT_SSH_COMMAND, because the mechanism\n gives more lattitude to end-users to be creative, will have lower\n chance of correctly deciding between ssh, tortoiseplink and\n plink, but it would still be high enough, say 99%, correct\n detection rate.\n\n * It is foolish to add more code to refine the auto-detection to\n raise 99% to 99.5%. We know that the approach fundamentally\n cannot make the detection rate reach 100%.\n\nThe last one can be paraphrased as \"perfect is the enemy of good\".\n\nBut that does not mean that it is OK to leave the system unusable\nfor 1% of the users. If the auto-detection code does not work\ncorrectly for a tiny minority of users, it is still OK, as long as\nwe give them an escape hatch to allow them to say \"You may not be\nable to guess among ssh, tortoiseplink and plink, or you may even\nguess incorrectly, so I'll tell you. I am using plink\". 99% of\nusers do not have to know about that escape hatch, but 1% of users\nwill be helped.\n\nIOW, \"give an escape hatch to override auto-detected tortoiseplink\nand plink variables\" suggestion should be taken as an \"in addition\nto\" suggestion (as opposed to an \"instead of\" suggestion). I was\nnot clear in my very first message, and I thought in my second and\nmy third message I clarified it as such, but probably I wasn't\nexplicit enough.\n\nIn any case, \"do this only if the first one token on the command\nline does not have '='\" we see below is flawed for two reasons and I\nthink it would not be a workable counter-proposal (besides, it goes\nagainst the \"perfect is the enemy of good\" mantra).\n\nFor one thing, the command line may not be \"VAR=VAL cmd\", but just\n\"cmd\" that names the path to tortoiseplink, but the leading\ndirectory path that houses tortoiseplink may have a '=' in it, in\nwhich case we would detect it correctly if we didn't have such a\nhack, but with the hack we would punt. More importantly, even when\n\"VAR=VAL cmd\" form was caught with the change, it may punt and stop\nguessing, but punting alone does not help users by letting them say\n\"I know you cannot auto-detect, so let me help you---what I have is\nPuTTY plink\". If it always assumes that the user uses the plain\nvanilla ssh, then the change merely changed what incorrect guess the\nauto-detection makes from \"tortoiseplink\" to \"vanilla ssh\" for a\nuser who uses the PuTTY variant of plink.\n\n> -- snipsnap --\n> diff --git a/connect.c b/connect.c\n> index c81f77001b..b990dd6190 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -797,7 +797,8 @@ struct child_process *git_connect(int fd[2], const\n> char *url,\n> \t\t\t\tchar *split_ssh = xstrdup(ssh);\n> \t\t\t\tconst char **ssh_argv;\n> \n> -\t\t\t\tif (split_cmdline(split_ssh, &ssh_argv))\n> +\t\t\t\tif (split_cmdline(split_ssh, &ssh_argv) &&\n> +\t\t\t\t !strchr(ssh_argv[0], '='))\n> \t\t\t\t\tssh_argv0 = xstrdup(ssh_argv[0]);\n> \t\t\t\tfree(split_ssh);\n> \t\t\t\tfree((void *)ssh_argv);\n"},{"id":"310151","messageId":"alpine.DEB.2.20.1701251327090.3469@virtualbox","threadId":"44806","inReplyTo":"xmqqmvf0wc2h.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-25T12:34:21Z","receivedAt":"2017-01-25T12:34:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 9 Jan 2017, Junio C Hamano wrote:\n\n> IOW, \"give an escape hatch to override auto-detected tortoiseplink and\n> plink variables\" suggestion should be taken as an \"in addition to\"\n> suggestion (as opposed to an \"instead of\" suggestion). I was not clear\n> in my very first message, and I thought in my second and my third\n> message I clarified it as such, but probably I wasn't explicit enough.\n\nWhile you may not have been explicit enough, I was not smart enough.\n\nThat escape-hatch exists *already*. And it is exactly the thing that you\nmentioned earlier as a potential source of mis-identification.\n\nLet's assume that you want to use a script for the GIT_SSH_COMMAND that\neventually uses PuTTY, and you call this script \"junio-is-a-superstar.sh\".\nAll plausible so far ;-)\n\nNow, with the patch in question (without the follow-up, which I would like\nto ask you to ignore, just like you did so far), Git would not figure out\nthat your script calls PuTTY eventually. The work-around? Easy:\n\n\tDUMMY=/plink.exe /path/to/junio-is-a-superstar.sh\n\nIn other words: the thing that we thought may be a problem is actually a\nfeature.\n\nLikewise, if your GIT_SSH_COMMAND looks like this:\n\n\tTHIS_IS_NOT_ACTUALLY_PUTTY=/my/window/needs/putty my-ssh.sh\n\n... you can easily change Git's mind by prefixing\n\n\tDUMMY=blubber\n\nIf you want to use a script that decides itself whether to call OpenSSH or\nPuTTY, Git should \"stay dumb\" about it, as it will not be able to tell\nbeforehand whether a port argument should be passed via `-p` or `-P`\nanyway.\n\nOf course, given our discussion I think all of this should be documented\nin the commit message as well as in the man page.\n\nDo you agree this is a good direction to take?\n\nCiao,\nJohannes\n"},{"id":"310244","messageId":"xmqqinp2939j.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"alpine.DEB.2.20.1701251327090.3469@virtualbox","subject":"Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-25T22:35:36Z","receivedAt":"2017-01-25T22:35:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Now, with the patch in question (without the follow-up, which I would like\n> to ask you to ignore, just like you did so far), Git would not figure out\n> that your script calls PuTTY eventually. The work-around? Easy:\n>\n> \tDUMMY=/plink.exe /path/to/junio-is-a-superstar.sh\n\nThink about how you would explain that to an end-user in our\ndocument? You'll need to explain how exactly the auto-detection\nworks, so that the user can \"exploit\" the loophole to do that. And\nwhat maintenance burden does it add when auto-detection is updated?\n\nI think I know you well enough that you know well enough that it is\ntoo ugly to live, and I suspect that the above is a tongue-in-cheek\n\"arguing for the sake of argument\" and would not need a serious\nresponse, but just in case...\n\n> ...\n> Of course, given our discussion I think all of this should be documented\n> in the commit message as well as in the man page.\n\nYes. Here is what comes on an obvious clean-up patch (which will be\nsent as a follow-up to this message).\n\n-- >8 --\nSubject: [PATCH] connect: core.sshvariant to correct misidentification\n\nWhile the logic we have been using to guess which variant of SSH\nimplementation is in use by looking at the name of the program has\nbeen robust enough for GIT_SSH (which does not go through the shell,\nhence it can only spell the name the SSH program and nothing else),\nextending it to GIT_SSH_COMMAND and core.sshcommand opens it for\nlarger chance of misidentification, because these can specify\narbitrary shell command, and a simple \"the first word on the line\nmust be the command name\" heuristic may not even look at the command\nname at all.\n\nAs any effort to improve the heuristic will give us only diminishing\nreturns, and especially given that most of the users are expected to\nhave a command line for which the heuristic would work correctly,\ngive an explicit escape hatch to override a misidentification, just\nin case one happens.\n\nIt is arguably bad to add this new variable to the core.* set, in\nthe sense that you'll never see this variable if you do not interact\nwith other repositories over ssh, but core.sshcommand is already\nthere for some reason, so let's match it.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 13 +++++++++++++\n connect.c | 26 ++++++++++++++++++++++++++\n 2 files changed, 39 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex af2ae4cc02..8ea1db49c6 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -455,6 +455,19 @@ core.sshCommand::\n \tthe `GIT_SSH_COMMAND` environment variable and is overridden\n \twhen the environment variable is set.\n \n+core.sshVariant::\n+\tSSH implementations used by Git when running `git fetch`,\n+\t`git clone`, and `git push` use slightly different command\n+\tline options (e.g. PuTTY and TortoisePlink use `-P `\n+\twhile OpenSSH uses `-p ` to specify the port number.\n+\tTortoisePlink in addition requires `-batch` option to be\n+\tpassed). Git guesses which variant is in use correctly from\n+\tthe name of the ssh command used (see `core.sshCommand`\n+\tconfiguration variable and also `GIT_SSH_COMMAND`\n+\tenvironment variable) most of the time. You can set this\n+\tvariable to 'putty', 'tortoiseplink', or 'ssh' to correct it\n+\twhen Git makes an incorrect guess.\n+\n core.ignoreStat::\n \tIf true, Git will avoid using lstat() calls to detect if files have\n \tchanged by setting the \"assume-unchanged\" bit for those tracked files\ndiff --git a/connect.c b/connect.c\nindex aa51b33bfc..358e9c06f0 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -691,6 +691,29 @@ static const char *get_ssh_command(void)\n \treturn NULL;\n }\n \n+static void override_ssh_variant(int *port_option, int *needs_batch)\n+{\n+\tconst char *variant;\n+\n+\tif (git_config_get_string_const(\"core.sshvariant\", &variant))\n+\t\treturn;\n+\tif (!strcmp(variant, \"tortoiseplink\")) {\n+\t\t*port_option = 'P';\n+\t\t*needs_batch = 1;\n+\t} else if (!strcmp(variant, \"putty\")) {\n+\t\t*port_option = 'P';\n+\t\t*needs_batch = 0;\n+\t} else {\n+\t\t/* default */\n+\t\tif (strcmp(variant, \"ssh\")) {\n+\t\t\twarning(_(\"core.sshvariant: unknown value '%s'\"), variant);\n+\t\t\twarning(_(\"using OpenSSH compatible behaviour\"));\n+\t\t}\n+\t\t*port_option = 'p';\n+\t\t*needs_batch = 0;\n+\t}\n+}\n+\n /*\n * This returns a dummy child_process if the transport protocol does not\n * need fork(2), or a struct child_process object if it does. Once done,\n@@ -836,6 +859,9 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\targv_array_push(&conn->args, \"-4\");\n \t\t\telse if (flags & CONNECT_IPV6)\n \t\t\t\targv_array_push(&conn->args, \"-6\");\n+\n+\t\t\toverride_ssh_variant(&port_option, &needs_batch);\n+\n \t\t\tif (needs_batch)\n \t\t\t\targv_array_push(&conn->args, \"-batch\");\n \t\t\tif (port) {\n-- \n2.11.0-699-ga1f1475476\n\n\n\n"},{"id":"310245","messageId":"xmqqefzq936b.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"xmqqinp2939j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-25T22:37:32Z","receivedAt":"2017-01-25T22:37:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Yes. Here is what comes on an obvious clean-up patch (which will be\n> sent as a follow-up to this message).\n\n... and this is the \"obvious clean-up patch\" to apply directly on\ntop of Segev's GIT_SSH_COMMAND support, which comes before the\nprevious one.\n\n-- >8 --\nSubject: [PATCH] connect: rename tortoiseplink and putty variables\n\nOne of these two may have originally been named after \"what exact\nSSH implementation do we have\" so that we can tweak the command line\noptions, but these days \"putty=1\" no longer means \"We are using the\nplink SSH implementation that comes with PuTTY\". It is set when we\nguess that either PuTTY plink or Tortoiseplink is in use.\n\nRename them after what effect is desired. The current \"putty\"\noption is about using \"-P \" when OpenSSH would use \"-p \",\nso rename it to port_option whose value is either 'p' or 'P\". The\nother one is about passing an extra command line option \"-batch\",\nso rename it needs_batch.\n\nSigned-off-by: Junio C Hamano \n---\n connect.c | 22 ++++++++++++----------\n 1 file changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex c81f77001b..aa51b33bfc 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -769,7 +769,8 @@ 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 = 0, tortoiseplink = 0;\n+\t\t\tint needs_batch = 0;\n+\t\t\tint port_option = 'p';\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n \t\t\tchar *ssh_argv0 = NULL;\n@@ -819,12 +820,14 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\tif (ssh_argv0) {\n \t\t\t\tconst char *base = basename(ssh_argv0);\n \n-\t\t\t\ttortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n-\t\t\t\t\t!strcasecmp(base, \"tortoiseplink.exe\");\n-\t\t\t\tputty = tortoiseplink ||\n-\t\t\t\t\t!strcasecmp(base, \"plink\") ||\n-\t\t\t\t\t!strcasecmp(base, \"plink.exe\");\n-\n+\t\t\t\tif (!strcasecmp(base, \"tortoiseplink\") ||\n+\t\t\t\t !strcasecmp(base, \"tortoiseplink.exe\")) {\n+\t\t\t\t\tport_option = 'P';\n+\t\t\t\t\tneeds_batch = 1;\n+\t\t\t\t} else if (!strcasecmp(base, \"plink\") ||\n+\t\t\t\t\t !strcasecmp(base, \"plink.exe\")) {\n+\t\t\t\t\tport_option = 'P';\n+\t\t\t\t}\n \t\t\t\tfree(ssh_argv0);\n \t\t\t}\n \n@@ -833,11 +836,10 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\targv_array_push(&conn->args, \"-4\");\n \t\t\telse if (flags & CONNECT_IPV6)\n \t\t\t\targv_array_push(&conn->args, \"-6\");\n-\t\t\tif (tortoiseplink)\n+\t\t\tif (needs_batch)\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_pushf(&conn->args, \"-%c\", port_option);\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-- \n2.11.0-699-ga1f1475476\n\n\n\n"},{"id":"310247","messageId":"20170125224043.jxjc4avuy4ztnkm4@sigill.intra.peff.net","threadId":"44806","inReplyTo":"xmqqinp2939j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-25T22:40:43Z","receivedAt":"2017-01-25T22:41:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 25, 2017 at 02:35:36PM -0800, Junio C Hamano wrote:\n\n> -- >8 --\n> Subject: [PATCH] connect: core.sshvariant to correct misidentification\n\nI have been watching this discussion from the sidelines, and I agree\nthat this direction is a big improvement.\n\n> +static void override_ssh_variant(int *port_option, int *needs_batch)\n> +{\n> +\tconst char *variant;\n> +\n> +\tif (git_config_get_string_const(\"core.sshvariant\", &variant))\n> +\t\treturn;\n> +\tif (!strcmp(variant, \"tortoiseplink\")) {\n> +\t\t*port_option = 'P';\n> +\t\t*needs_batch = 1;\n> +\t} else if (!strcmp(variant, \"putty\")) {\n> +\t\t*port_option = 'P';\n> +\t\t*needs_batch = 0;\n> +\t} else {\n> +\t\t/* default */\n> +\t\tif (strcmp(variant, \"ssh\")) {\n> +\t\t\twarning(_(\"core.sshvariant: unknown value '%s'\"), variant);\n> +\t\t\twarning(_(\"using OpenSSH compatible behaviour\"));\n> +\t\t}\n> +\t\t*port_option = 'p';\n> +\t\t*needs_batch = 0;\n> +\t}\n> +}\n\nIIRC, the \"const\" in git_config_get_string_const is only about avoiding\nan annoying cast. The result is still allocated and needs freed. Since\nyou are not keeping the value after the function returns, I think you\ncould just use git_config_get_value().\n\n(Grepping around, I see a few other places that seem to make the same\nmistake. I think this is a confusing interface that should probably be\nfixed).\n\n-Peff\n"},{"id":"310260","messageId":"xmqqpoja7mej.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"20170125224043.jxjc4avuy4ztnkm4@sigill.intra.peff.net","subject":"Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-25T23:25:08Z","receivedAt":"2017-01-25T23:25:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jan 25, 2017 at 02:35:36PM -0800, Junio C Hamano wrote:\n>\n>> -- >8 --\n>> Subject: [PATCH] connect: core.sshvariant to correct misidentification\n>\n> I have been watching this discussion from the sidelines, and I agree\n> that this direction is a big improvement.\n> ...\n> IIRC, the \"const\" in git_config_get_string_const is only about avoiding\n> an annoying cast. The result is still allocated and needs freed. Since\n> you are not keeping the value after the function returns, I think you\n> could just use git_config_get_value().\n\nIt is too late for today's pushout (I have this topic near the tip\nof 'pu', so it is possible to tweak and redo only 'pu' branch, but I\ngenerally hate tweaking things after 15:00 my time), but I'll fix\nthat before the topic hits 'jch' (which is a bit more than 'next'\nand that is what I use for everyday work).\n\nThanks. \n"},{"id":"310306","messageId":"alpine.DEB.2.20.1701261259260.3469@virtualbox","threadId":"44806","inReplyTo":"xmqqinp2939j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-26T12:01:05Z","receivedAt":"2017-01-26T12:01:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 25 Jan 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Now, with the patch in question (without the follow-up, which I would\n> > like to ask you to ignore, just like you did so far), Git would not\n> > figure out that your script calls PuTTY eventually. The work-around?\n> > Easy:\n> >\n> > \tDUMMY=/plink.exe /path/to/junio-is-a-superstar.sh\n> \n> Think about how you would explain that to an end-user in our document?\n> You'll need to explain how exactly the auto-detection works, so that the\n> user can \"exploit\" the loophole to do that. And what maintenance burden\n> does it add when auto-detection is updated?\n\nFine, you do not like it. Saying so (instead of asking me questions) would\nhave been helpful.\n\n> I think I know you well enough that you know well enough that it is too\n> ugly to live, and I suspect that the above is a tongue-in-cheek \"arguing\n> for the sake of argument\" and would not need a serious response, but\n> just in case...\n\nIt was not tongue-in-cheek, I was being serious.\n\n> Yes. Here is what comes on an obvious clean-up patch (which will be\n> sent as a follow-up to this message).\n\nI'd much rather prefer\nhttps://github.com/git-for-windows/git/pull/1030 than your patch.\n\nCiao,\nJohannes\n"},{"id":"310322","messageId":"alpine.DEB.2.20.1701261535100.3469@virtualbox","threadId":"44806","inReplyTo":"xmqqefzq936b.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-26T14:45:28Z","receivedAt":"2017-01-26T14:46:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 25 Jan 2017, Junio C Hamano wrote:\n\n> Subject: [PATCH] connect: rename tortoiseplink and putty variables\n> \n> One of these two may have originally been named after \"what exact\n> SSH implementation do we have\" so that we can tweak the command line\n> options, but these days \"putty=1\" no longer means \"We are using the\n> plink SSH implementation that comes with PuTTY\". It is set when we\n> guess that either PuTTY plink or Tortoiseplink is in use.\n> \n> Rename them after what effect is desired. The current \"putty\"\n> option is about using \"-P \" when OpenSSH would use \"-p \",\n> so rename it to port_option whose value is either 'p' or 'P\". The\n> other one is about passing an extra command line option \"-batch\",\n> so rename it needs_batch.\n> \n> Signed-off-by: Junio C Hamano \n\nApart from an overly-long line, this patch looks good. It made my life a\nlittle harder, as I had to rebase Segev's ssh.variant (why this should be\na core.* variable is not clear to me) patch and it caused conflicts. I\nresolved those conflicts and made both patches part of this patch series.\n\nWill contribute v2 as soon as the test suite passes,\nJohannes\n"},{"id":"310325","messageId":"alpine.DEB.2.20.1701261550590.3469@virtualbox","threadId":"44806","inReplyTo":"2ff29a4d00e0e13d460122d8008e762361ca90aa.1483358673.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 0/3] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-26T14:51:46Z","receivedAt":"2017-01-26T14:51:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We already handle PuTTY's plink and TortoiseGit's tortoiseplink in\nGIT_SSH by automatically using the -P option to specify ports, and in\ntortoiseplink's case by passing the --batch option.\n\nFor users who need to pass additional command-line options to plink,\nthis poses a problem: the only way to do that is to use GIT_SSH_COMMAND,\nbut Git does not handle that specifically, so those users have to\nmanually parse the command-line options passed via GIT_SSH_COMMAND and\nreplace -p (if present) by -P, and add --batch in the case of\ntortoiseplink.\n\nThis is error-prone and a bad user experience.\n\nTo fix this, the changes proposed in this patch series introduce\nhandling this by splitting the GIT_SSH_COMMAND value and treating the\nfirst parameter with the same grace as GIT_SSH. To counter any possible\nmisdetection, the user can also specify explicitly via GIT_SSH_VARIANT\nor ssh.variant which SSH variant they are using.\n\nThis is v2 of the patch, now turned patch series. Relative to v1, it\nintegrates Junio's cleanup patch and Segev's follow-up Pull Request that\nintroduces the GIT_SSH_VARIANT and ssh.variant settings to override\nGit's autodetection manually.\n\n\nJunio C Hamano (1):\n connect: rename tortoiseplink and putty variables\n\nSegev Finer (2):\n connect: handle putty/plink also in GIT_SSH_COMMAND\n connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config\n\n Documentation/config.txt | 7 +++++\n Documentation/git.txt | 7 +++++\n connect.c | 66 +++++++++++++++++++++++++++++++++++-------------\n t/t5601-clone.sh | 41 ++++++++++++++++++++++++++++++\n 4 files changed, 104 insertions(+), 17 deletions(-)\n\n\nbase-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe\nPublished-As: https://github.com/dscho/git/releases/tag/putty-w-args-v2\nFetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v2\n\nInterdiff vs v1:\n\n diff --git a/Documentation/config.txt b/Documentation/config.txt\n index af2ae4cc02..f2c210f0a0 100644\n --- a/Documentation/config.txt\n +++ b/Documentation/config.txt\n @@ -1949,6 +1949,13 @@ Environment variable settings always override any matches. The URLs that are\n matched against are those given directly to Git commands. This means any URLs\n visited as a result of a redirection do not participate in matching.\n \n +ssh.variant::\n +\tOverride the autodetection of plink/tortoiseplink in the SSH\n +\tcommand that 'git fetch' and 'git push' use. It can be set to\n +\teither `ssh`, `plink`, `putty` or `tortoiseplink`. Any other\n +\tvalue will be treated as normal ssh. This is useful in case\n +\tthat Git gets this wrong.\n +\n i18n.commitEncoding::\n \tCharacter encoding the commit messages are stored in; Git itself\n \tdoes not care per se, but this information is necessary e.g. when\n diff --git a/Documentation/git.txt b/Documentation/git.txt\n index 4f208fab92..c322558aa7 100644\n --- a/Documentation/git.txt\n +++ b/Documentation/git.txt\n @@ -1020,6 +1020,13 @@ Usually it is easier to configure any desired options through your\n personal `.ssh/config` file. Please consult your ssh documentation\n for further details.\n \n +`GIT_SSH_VARIANT`::\n +\tIf this environment variable is set, it overrides the autodetection\n +\tof plink/tortoiseplink in the SSH command that 'git fetch' and 'git\n +\tpush' use. It can be set to either `ssh`, `plink`, `putty` or\n +\t`tortoiseplink`. Any other value will be treated as normal ssh. This\n +\tis useful in case that Git gets this wrong.\n +\n `GIT_ASKPASS`::\n \tIf this environment variable is set, then Git commands which need to\n \tacquire passwords or passphrases (e.g. for HTTP or IMAP authentication)\n diff --git a/connect.c b/connect.c\n index c81f77001b..7b4437578b 100644\n --- a/connect.c\n +++ b/connect.c\n @@ -691,6 +691,24 @@ static const char *get_ssh_command(void)\n \treturn NULL;\n }\n \n +static int handle_ssh_variant(int *port_option, int *needs_batch)\n +{\n +\tconst char *variant;\n +\n +\tif (!(variant = getenv(\"GIT_SSH_VARIANT\")) &&\n +\t\tgit_config_get_string_const(\"ssh.variant\", &variant))\n +\t\treturn 0;\n +\n +\tif (!strcmp(variant, \"plink\") || !strcmp(variant, \"putty\"))\n +\t\t*port_option = 'P';\n +\telse if (!strcmp(variant, \"tortoiseplink\")) {\n +\t\t*port_option = 'P';\n +\t\t*needs_batch = 1;\n +\t}\n +\n +\treturn 1;\n +}\n +\n /*\n * This returns a dummy child_process if the transport protocol does not\n * need fork(2), or a struct child_process object if it does. Once done,\n @@ -769,7 +787,8 @@ 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 = 0, tortoiseplink = 0;\n +\t\t\tint needs_batch = 0;\n +\t\t\tint port_option = 'p';\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n \t\t\tchar *ssh_argv0 = NULL;\n @@ -816,28 +835,32 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tssh_argv0 = xstrdup(ssh);\n \t\t\t}\n \n -\t\t\tif (ssh_argv0) {\n +\t\t\tif (!handle_ssh_variant(&port_option, &needs_batch) &&\n +\t\t\t ssh_argv0) {\n \t\t\t\tconst char *base = basename(ssh_argv0);\n \n -\t\t\t\ttortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n -\t\t\t\t\t!strcasecmp(base, \"tortoiseplink.exe\");\n -\t\t\t\tputty = tortoiseplink ||\n -\t\t\t\t\t!strcasecmp(base, \"plink\") ||\n -\t\t\t\t\t!strcasecmp(base, \"plink.exe\");\n -\n -\t\t\t\tfree(ssh_argv0);\n +\t\t\t\tif (!strcasecmp(base, \"tortoiseplink\") ||\n +\t\t\t\t !strcasecmp(base, \"tortoiseplink.exe\")) {\n +\t\t\t\t\tport_option = 'P';\n +\t\t\t\t\tneeds_batch = 1;\n +\t\t\t\t} else if (!strcasecmp(base, \"plink\") ||\n +\t\t\t\t\t !strcasecmp(base, \"plink.exe\")) {\n +\t\t\t\t\tport_option = 'P';\n +\t\t\t\t}\n \t\t\t}\n \n +\t\t\tfree(ssh_argv0);\n +\n \t\t\targv_array_push(&conn->args, ssh);\n \t\t\tif (flags & CONNECT_IPV4)\n \t\t\t\targv_array_push(&conn->args, \"-4\");\n \t\t\telse if (flags & CONNECT_IPV6)\n \t\t\t\targv_array_push(&conn->args, \"-6\");\n -\t\t\tif (tortoiseplink)\n +\t\t\tif (needs_batch)\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_pushf(&conn->args,\n +\t\t\t\t\t\t \"-%c\", port_option);\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 diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n index 9335e10c2a..b52b8acf98 100755\n --- a/t/t5601-clone.sh\n +++ b/t/t5601-clone.sh\n @@ -401,6 +401,32 @@ test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '\n \texpect_ssh \"-v -P 123\" myhost src\n '\n \n +test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '\n +\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n +\tGIT_SSH_VARIANT=ssh \\\n +\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-variant-1 &&\n +\texpect_ssh \"-p 123\" myhost src\n +'\n +\n +test_expect_success 'ssh.variant overrides plink detection' '\n +\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n +\tgit -c ssh.variant=ssh \\\n +\t\tclone \"[myhost:123]:src\" ssh-bracket-clone-variant-2 &&\n +\texpect_ssh \"-p 123\" myhost src\n +'\n +\n +test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '\n +\tGIT_SSH_VARIANT=plink \\\n +\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-variant-3 &&\n +\texpect_ssh \"-P 123\" myhost src\n +'\n +\n +test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '\n +\tGIT_SSH_VARIANT=tortoiseplink \\\n +\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-variant-4 &&\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\n-- \n2.11.1.windows.prerelease.2.9.g3014b57\n\n"},{"id":"310326","messageId":"199723aedcaa8464967a9faf33163c0cd196c941.1485442231.git.johannes.schindelin@gmx.de","threadId":"44806","inReplyTo":"cover.1485442231.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 1/3] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-26T14:51:51Z","receivedAt":"2017-01-26T14:52:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Segev Finer \n\nGit for Windows has special support for the popular SSH client PuTTY:\nwhen using PuTTY's non-interactive version (\"plink.exe\"), we use the -P\noption to specify the port rather than OpenSSH's -p option. TortoiseGit\nships with its own, forked version of plink.exe, that adds support for\nthe -batch option, and for good measure we special-case that, too.\n\nHowever, this special-casing of PuTTY only covers the case where the\nuser overrides the SSH command via the environment variable GIT_SSH\n(which allows specifying the name of the executable), not\nGIT_SSH_COMMAND (which allows specifying a full command, including\nadditional command-line options).\n\nWhen users want to pass any additional arguments to (Tortoise-)Plink,\nsuch as setting a private key, they are required to either use a shell\nscript named plink or tortoiseplink or duplicate the logic that is\nalready in Git for passing the correct style of command line arguments,\nwhich can be difficult, error prone and annoying to get right.\n\nThis patch simply reuses the existing logic and expands it to cover\nGIT_SSH_COMMAND, too.\n\nNote: it may look a little heavy-handed to duplicate the entire\ncommand-line and then split it, only to extract the name of the\nexecutable. However, this is not a performance-critical code path, and\nthe code is much more readable this way.\n\nSigned-off-by: Segev Finer \nSigned-off-by: Johannes Schindelin \n---\n connect.c | 23 ++++++++++++++++-------\n t/t5601-clone.sh | 15 +++++++++++++++\n 2 files changed, 31 insertions(+), 7 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 8cb93b0720..c81f77001b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -772,6 +772,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\tint putty = 0, tortoiseplink = 0;\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n+\t\t\tchar *ssh_argv0 = NULL;\n \t\t\ttransport_check_allowed(\"ssh\");\n \t\t\tget_host_and_port(&ssh_host, &port);\n \n@@ -792,10 +793,15 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t}\n \n \t\t\tssh = get_ssh_command();\n-\t\t\tif (!ssh) {\n-\t\t\t\tconst char *base;\n-\t\t\t\tchar *ssh_dup;\n-\n+\t\t\tif (ssh) {\n+\t\t\t\tchar *split_ssh = xstrdup(ssh);\n+\t\t\t\tconst char **ssh_argv;\n+\n+\t\t\t\tif (split_cmdline(split_ssh, &ssh_argv))\n+\t\t\t\t\tssh_argv0 = xstrdup(ssh_argv[0]);\n+\t\t\t\tfree(split_ssh);\n+\t\t\t\tfree((void *)ssh_argv);\n+\t\t\t} else {\n \t\t\t\t/*\n \t\t\t\t * GIT_SSH is the no-shell version of\n \t\t\t\t * GIT_SSH_COMMAND (and must remain so for\n@@ -807,8 +813,11 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tif (!ssh)\n \t\t\t\t\tssh = \"ssh\";\n \n-\t\t\t\tssh_dup = xstrdup(ssh);\n-\t\t\t\tbase = basename(ssh_dup);\n+\t\t\t\tssh_argv0 = xstrdup(ssh);\n+\t\t\t}\n+\n+\t\t\tif (ssh_argv0) {\n+\t\t\t\tconst char *base = basename(ssh_argv0);\n \n \t\t\t\ttortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n \t\t\t\t\t!strcasecmp(base, \"tortoiseplink.exe\");\n@@ -816,7 +825,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\t\t!strcasecmp(base, \"plink\") ||\n \t\t\t\t\t!strcasecmp(base, \"plink.exe\");\n \n-\t\t\t\tfree(ssh_dup);\n+\t\t\t\tfree(ssh_argv0);\n \t\t\t}\n \n \t\t\targv_array_push(&conn->args, ssh);\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 4241ea5b32..9335e10c2a 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -386,6 +386,21 @@ test_expect_success 'tortoiseplink is like putty, with extra arguments' '\n \texpect_ssh \"-batch -P 123\" myhost src\n '\n \n+test_expect_success 'double quoted plink.exe in GIT_SSH_COMMAND' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink.exe\" &&\n+\tGIT_SSH_COMMAND=\"\\\"$TRASH_DIRECTORY/plink.exe\\\" -v\" \\\n+\t\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-plink-3 &&\n+\texpect_ssh \"-v -P 123\" myhost src\n+'\n+\n+SQ=\"'\"\n+test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink.exe\" &&\n+\tGIT_SSH_COMMAND=\"$SQ$TRASH_DIRECTORY/plink.exe$SQ -v\" \\\n+\t\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-plink-4 &&\n+\texpect_ssh \"-v -P 123\" myhost src\n+'\n+\n # Reset the GIT_SSH environment variable for clone tests.\n setup_ssh_wrapper\n \n-- \n2.11.1.windows.prerelease.2.9.g3014b57\n\n\n"},{"id":"310327","messageId":"f414a890a98042f934f799ef89fe1f36e2c27d9f.1485442231.git.johannes.schindelin@gmx.de","threadId":"44806","inReplyTo":"cover.1485442231.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 2/3] connect: rename tortoiseplink and putty variables","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-26T14:51:58Z","receivedAt":"2017-01-26T14:52:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Junio C Hamano \n\nOne of these two may have originally been named after \"what exact\nSSH implementation do we have\" so that we can tweak the command line\noptions, but these days \"putty=1\" no longer means \"We are using the\nplink SSH implementation that comes with PuTTY\". It is set when we\nguess that either PuTTY plink or Tortoiseplink is in use.\n\nRename them after what effect is desired. The current \"putty\"\noption is about using \"-P \" when OpenSSH would use \"-p \",\nso rename it to port_option whose value is either 'p' or 'P\". The\nother one is about passing an extra command line option \"-batch\",\nso rename it needs_batch.\n\n[jes: wrapped overly-long line]\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Johannes Schindelin \n---\n connect.c | 23 +++++++++++++----------\n 1 file changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex c81f77001b..9f750eacb6 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -769,7 +769,8 @@ 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 = 0, tortoiseplink = 0;\n+\t\t\tint needs_batch = 0;\n+\t\t\tint port_option = 'p';\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n \t\t\tchar *ssh_argv0 = NULL;\n@@ -819,12 +820,14 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\tif (ssh_argv0) {\n \t\t\t\tconst char *base = basename(ssh_argv0);\n \n-\t\t\t\ttortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n-\t\t\t\t\t!strcasecmp(base, \"tortoiseplink.exe\");\n-\t\t\t\tputty = tortoiseplink ||\n-\t\t\t\t\t!strcasecmp(base, \"plink\") ||\n-\t\t\t\t\t!strcasecmp(base, \"plink.exe\");\n-\n+\t\t\t\tif (!strcasecmp(base, \"tortoiseplink\") ||\n+\t\t\t\t !strcasecmp(base, \"tortoiseplink.exe\")) {\n+\t\t\t\t\tport_option = 'P';\n+\t\t\t\t\tneeds_batch = 1;\n+\t\t\t\t} else if (!strcasecmp(base, \"plink\") ||\n+\t\t\t\t\t !strcasecmp(base, \"plink.exe\")) {\n+\t\t\t\t\tport_option = 'P';\n+\t\t\t\t}\n \t\t\t\tfree(ssh_argv0);\n \t\t\t}\n \n@@ -833,11 +836,11 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\targv_array_push(&conn->args, \"-4\");\n \t\t\telse if (flags & CONNECT_IPV6)\n \t\t\t\targv_array_push(&conn->args, \"-6\");\n-\t\t\tif (tortoiseplink)\n+\t\t\tif (needs_batch)\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_pushf(&conn->args,\n+\t\t\t\t\t\t \"-%c\", port_option);\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-- \n2.11.1.windows.prerelease.2.9.g3014b57\n\n\n"},{"id":"310328","messageId":"3d451f2c357a3fd7f0b0e4b427548553d7d05306.1485442231.git.johannes.schindelin@gmx.de","threadId":"44806","inReplyTo":"cover.1485442231.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-26T14:52:20Z","receivedAt":"2017-01-26T14:52:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Segev Finer \n\nThis environment variable and configuration value allow to\noverride the autodetection of plink/tortoiseplink in case that\nGit gets it wrong.\n\n[jes: wrapped overly-long lines, changed get_ssh_variant() to\nhandle_ssh_variant() to accomodate the change from the\nputty/tortoiseplink variables to port_option/needs_batch.]\n\nSigned-off-by: Segev Finer \nSigned-off-by: Johannes Schindelin \n---\n Documentation/config.txt | 7 +++++++\n Documentation/git.txt | 7 +++++++\n connect.c | 24 ++++++++++++++++++++++--\n t/t5601-clone.sh | 26 ++++++++++++++++++++++++++\n 4 files changed, 62 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex af2ae4cc02..f2c210f0a0 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1949,6 +1949,13 @@ Environment variable settings always override any matches. The URLs that are\n matched against are those given directly to Git commands. This means any URLs\n visited as a result of a redirection do not participate in matching.\n \n+ssh.variant::\n+\tOverride the autodetection of plink/tortoiseplink in the SSH\n+\tcommand that 'git fetch' and 'git push' use. It can be set to\n+\teither `ssh`, `plink`, `putty` or `tortoiseplink`. Any other\n+\tvalue will be treated as normal ssh. This is useful in case\n+\tthat Git gets this wrong.\n+\n i18n.commitEncoding::\n \tCharacter encoding the commit messages are stored in; Git itself\n \tdoes not care per se, but this information is necessary e.g. when\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 4f208fab92..c322558aa7 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -1020,6 +1020,13 @@ Usually it is easier to configure any desired options through your\n personal `.ssh/config` file. Please consult your ssh documentation\n for further details.\n \n+`GIT_SSH_VARIANT`::\n+\tIf this environment variable is set, it overrides the autodetection\n+\tof plink/tortoiseplink in the SSH command that 'git fetch' and 'git\n+\tpush' use. It can be set to either `ssh`, `plink`, `putty` or\n+\t`tortoiseplink`. Any other value will be treated as normal ssh. This\n+\tis useful in case that Git gets this wrong.\n+\n `GIT_ASKPASS`::\n \tIf this environment variable is set, then Git commands which need to\n \tacquire passwords or passphrases (e.g. for HTTP or IMAP authentication)\ndiff --git a/connect.c b/connect.c\nindex 9f750eacb6..7b4437578b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -691,6 +691,24 @@ static const char *get_ssh_command(void)\n \treturn NULL;\n }\n \n+static int handle_ssh_variant(int *port_option, int *needs_batch)\n+{\n+\tconst char *variant;\n+\n+\tif (!(variant = getenv(\"GIT_SSH_VARIANT\")) &&\n+\t\tgit_config_get_string_const(\"ssh.variant\", &variant))\n+\t\treturn 0;\n+\n+\tif (!strcmp(variant, \"plink\") || !strcmp(variant, \"putty\"))\n+\t\t*port_option = 'P';\n+\telse if (!strcmp(variant, \"tortoiseplink\")) {\n+\t\t*port_option = 'P';\n+\t\t*needs_batch = 1;\n+\t}\n+\n+\treturn 1;\n+}\n+\n /*\n * This returns a dummy child_process if the transport protocol does not\n * need fork(2), or a struct child_process object if it does. Once done,\n@@ -817,7 +835,8 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tssh_argv0 = xstrdup(ssh);\n \t\t\t}\n \n-\t\t\tif (ssh_argv0) {\n+\t\t\tif (!handle_ssh_variant(&port_option, &needs_batch) &&\n+\t\t\t ssh_argv0) {\n \t\t\t\tconst char *base = basename(ssh_argv0);\n \n \t\t\t\tif (!strcasecmp(base, \"tortoiseplink\") ||\n@@ -828,9 +847,10 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\t\t !strcasecmp(base, \"plink.exe\")) {\n \t\t\t\t\tport_option = 'P';\n \t\t\t\t}\n-\t\t\t\tfree(ssh_argv0);\n \t\t\t}\n \n+\t\t\tfree(ssh_argv0);\n+\n \t\t\targv_array_push(&conn->args, ssh);\n \t\t\tif (flags & CONNECT_IPV4)\n \t\t\t\targv_array_push(&conn->args, \"-4\");\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 9335e10c2a..b52b8acf98 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -401,6 +401,32 @@ test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '\n \texpect_ssh \"-v -P 123\" myhost src\n '\n \n+test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n+\tGIT_SSH_VARIANT=ssh \\\n+\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-variant-1 &&\n+\texpect_ssh \"-p 123\" myhost src\n+'\n+\n+test_expect_success 'ssh.variant overrides plink detection' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n+\tgit -c ssh.variant=ssh \\\n+\t\tclone \"[myhost:123]:src\" ssh-bracket-clone-variant-2 &&\n+\texpect_ssh \"-p 123\" myhost src\n+'\n+\n+test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '\n+\tGIT_SSH_VARIANT=plink \\\n+\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-variant-3 &&\n+\texpect_ssh \"-P 123\" myhost src\n+'\n+\n+test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '\n+\tGIT_SSH_VARIANT=tortoiseplink \\\n+\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-variant-4 &&\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-- \n2.11.1.windows.prerelease.2.9.g3014b57\n"},{"id":"310353","messageId":"xmqqo9yt4o5i.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"3d451f2c357a3fd7f0b0e4b427548553d7d05306.1485442231.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-26T19:27:52Z","receivedAt":"2017-01-26T19:28:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index af2ae4cc02..f2c210f0a0 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1949,6 +1949,13 @@ Environment variable settings always override any matches. The URLs that are\n> matched against are those given directly to Git commands. This means any URLs\n> visited as a result of a redirection do not participate in matching.\n> \n> +ssh.variant::\n> +\tOverride the autodetection of plink/tortoiseplink in the SSH\n> +\tcommand that 'git fetch' and 'git push' use. It can be set to\n> +\teither `ssh`, `plink`, `putty` or `tortoiseplink`. Any other\n> +\tvalue will be treated as normal ssh. This is useful in case\n> +\tthat Git gets this wrong.\n> +\n\nI do like the fact that this now sits under ssh.* hierarchy (not core.*).\n\nIt should mention core.sshCommand, GIT_SSH_COMMAND, etc., because\nreading this alone would mislead users to set only this one and expect\nthat their plink will be used without setting core.sshCommand etc.\n\nIt also should say that GIT_SSH_VARIANT environment variable will\noverride this variable.\n\n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index 4f208fab92..c322558aa7 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -1020,6 +1020,13 @@ Usually it is easier to configure any desired options through your\n> personal `.ssh/config` file. Please consult your ssh documentation\n> for further details.\n> \n> +`GIT_SSH_VARIANT`::\n> +\tIf this environment variable is set, it overrides the autodetection\n> +\tof plink/tortoiseplink in the SSH command that 'git fetch' and 'git\n> +\tpush' use. It can be set to either `ssh`, `plink`, `putty` or\n> +\t`tortoiseplink`. Any other value will be treated as normal ssh. This\n> +\tis useful in case that Git gets this wrong.\n\nSimilarly this should mention GIT_SSH_COMMAND at least. It is crazy\nto set something that will cause misdetection to core.sshCommand and\nuse this environment variable to fix it (instead of using ssh.variant),\nso I think it is OK not to mention core.sshCommand (but it would not\nhurt to do so).\n\n> diff --git a/connect.c b/connect.c\n> index 9f750eacb6..7b4437578b 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -691,6 +691,24 @@ static const char *get_ssh_command(void)\n> \treturn NULL;\n> }\n> \n> +static int handle_ssh_variant(int *port_option, int *needs_batch)\n> +{\n> +\tconst char *variant;\n> +\n> +\tif (!(variant = getenv(\"GIT_SSH_VARIANT\")) &&\n> +\t\tgit_config_get_string_const(\"ssh.variant\", &variant))\n> +\t\treturn 0;\n> +\n> +\tif (!strcmp(variant, \"plink\") || !strcmp(variant, \"putty\"))\n> +\t\t*port_option = 'P';\n> +\telse if (!strcmp(variant, \"tortoiseplink\")) {\n> +\t\t*port_option = 'P';\n> +\t\t*needs_batch = 1;\n> +\t}\n> +\n> +\treturn 1;\n> +}\n\nBetween handle and get I do not think there is strong reason to\nfavor one over the other. Unlike the one I sent yesterday, this is\nnot overriding but is getting, so we should keep the original name\nSegev gave it, which is shorter, I would think, rather than renaming\nit to \"handle\".\n\nThe string that came from the configuration must be freed, no? That\nis what I recall in Peff's comment yesterday.\n\n> @@ -817,7 +835,8 @@ struct child_process *git_connect(int fd[2], const char *url,\n> \t\t\t\tssh_argv0 = xstrdup(ssh);\n> \t\t\t}\n> \n> -\t\t\tif (ssh_argv0) {\n> +\t\t\tif (!handle_ssh_variant(&port_option, &needs_batch) &&\n> +\t\t\t ssh_argv0) {\n\nI like the placement of this \"if the user explicitly told us\" much\nbetter.\n\nMy patch yesterday was deliberately being lazy, because the next\nthought that will come to us after comparing the two is \"this way,\nwe avoid having to do the auto-detection altogether, which is way\nbetter than wasting the effort to auto-detect, only to override\".\n\nAnd that reasoning will lead to a realization \"there is no reason to\neven do the split_cmdline() if the user explicitly told us\". While\nthat reasoning is correct and we _should_ further refactor, I didn't\nwant to spend braincycles on that myself.\n"},{"id":"310400","messageId":"alpine.DEB.2.20.1701271132270.3469@virtualbox","threadId":"44806","inReplyTo":"xmqqo9yt4o5i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-27T10:35:38Z","receivedAt":"2017-01-27T10:46:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 26 Jan 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > diff --git a/connect.c b/connect.c\n> > index 9f750eacb6..7b4437578b 100644\n> > --- a/connect.c\n> > +++ b/connect.c\n> > @@ -691,6 +691,24 @@ static const char *get_ssh_command(void)\n> > \treturn NULL;\n> > }\n> > \n> > +static int handle_ssh_variant(int *port_option, int *needs_batch)\n> > +{\n> > +\tconst char *variant;\n> > +\n> > +\tif (!(variant = getenv(\"GIT_SSH_VARIANT\")) &&\n> > +\t\tgit_config_get_string_const(\"ssh.variant\", &variant))\n> > +\t\treturn 0;\n> > +\n> > +\tif (!strcmp(variant, \"plink\") || !strcmp(variant, \"putty\"))\n> > +\t\t*port_option = 'P';\n> > +\telse if (!strcmp(variant, \"tortoiseplink\")) {\n> > +\t\t*port_option = 'P';\n> > +\t\t*needs_batch = 1;\n> > +\t}\n> > +\n> > +\treturn 1;\n> > +}\n> \n> Between handle and get I do not think there is strong reason to\n> favor one over the other.\n\nThat is correct. \"handle\" and \"get\" are two very beautiful words, and none\nof them deserves to take a back seat behind the other.\n\nIn this case, \"get\" is inappropriate, though, as the function does not\nreturn the ssh variant, nor does it assign the ssh variant to any variable\nto which any of its argument points.\n\nWill try to find the time to address the other issues soon,\nJohannes\n"},{"id":"310426","messageId":"xmqqpoj8z7su.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"xmqqo9yt4o5i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-27T18:17:37Z","receivedAt":"2017-01-27T18:21:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Just to save us extra round-trip.\n\nJunio C Hamano writes:\n\n>> +`GIT_SSH_VARIANT`::\n>> +\tIf this environment variable is set, it overrides the autodetection\n>> +\tof plink/tortoiseplink in the SSH command that 'git fetch' and 'git\n>> +\tpush' use. It can be set to either `ssh`, `plink`, `putty` or\n>> +\t`tortoiseplink`. Any other value will be treated as normal ssh. This\n>> +\tis useful in case that Git gets this wrong.\n>\n> Similarly this should mention GIT_SSH_COMMAND at least. It is crazy\n> to set something that will cause misdetection to core.sshCommand and\n> use this environment variable to fix it (instead of using ssh.variant),\n> so I think it is OK not to mention core.sshCommand (but it would not\n> hurt to do so).\n>\n>> diff --git a/connect.c b/connect.c\n>> index 9f750eacb6..7b4437578b 100644\n>> --- a/connect.c\n>> +++ b/connect.c\n>> @@ -691,6 +691,24 @@ static const char *get_ssh_command(void)\n>> \treturn NULL;\n>> }\n>> \n>> +static int handle_ssh_variant(int *port_option, int *needs_batch)\n>> +{\n>> ...\n>> +}\n> ...\n>\n> The string that came from the configuration must be freed, no? That\n> is what I recall in Peff's comment yesterday.\n\nThe leak needs plugging in some way. \n\nBecause this thing is merely an escape hatch that somebody who needs\nit needs to use it always (as opposed to one-shot basis), we do not\nneed to support the environment variable and go with only the\nconfiguration, which may make it easier to plug the leak.\n\n>> @@ -817,7 +835,8 @@ struct child_process *git_connect(int fd[2], const char *url,\n>> \t\t\t\tssh_argv0 = xstrdup(ssh);\n>> \t\t\t}\n>> \n>> -\t\t\tif (ssh_argv0) {\n>> +\t\t\tif (!handle_ssh_variant(&port_option, &needs_batch) &&\n>> +\t\t\t ssh_argv0) {\n>\n> I like the placement of this \"if the user explicitly told us\" much\n> better.\n> ...\n> And that reasoning will lead to a realization \"there is no reason to\n> even do the split_cmdline() if the user explicitly told us\". While\n> that reasoning is correct and we _should_ further refactor, I didn't\n> want to spend braincycles on that myself.\n\nThis _should_ above is a lot weaker than _must_. \n\nIOW, I think it is acceptable to always split GIT_SSH_COMMAND into\ntokens before we realize that the user used the escape hatch and the\nsplitting was a wasted effort. This is exactly because this thing\nis an escape hatch that is expected to be rarely used. Of course,\nif the \"wasted effort\" can be eliminated without sacrificing the\nsimplicity of the code, that is fine as well.\n\n\n\n\n"},{"id":"310643","messageId":"cover.1485950225.git.johannes.schindelin@gmx.de","threadId":"44806","inReplyTo":"cover.1485442231.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-01T11:57:41Z","receivedAt":"2017-02-01T11:58:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We already handle PuTTY's plink and TortoiseGit's tortoiseplink in\nGIT_SSH by automatically using the -P option to specify ports, and in\ntortoiseplink's case by passing the --batch option.\n\nFor users who need to pass additional command-line options to plink,\nthis poses a problem: the only way to do that is to use GIT_SSH_COMMAND,\nbut Git does not handle that specifically, so those users have to\nmanually parse the command-line options passed via GIT_SSH_COMMAND and\nreplace -p (if present) by -P, and add --batch in the case of\ntortoiseplink.\n\nThis is error-prone and a bad user experience.\n\nTo fix this, the changes proposed in this patch series introduce\nhandling this by splitting the GIT_SSH_COMMAND value and treating the\nfirst parameter with the same grace as GIT_SSH. To counter any possible\nmisdetection, the user can also specify explicitly via GIT_SSH_VARIANT\nor ssh.variant which SSH variant they are using.\n\nChanges relative to v2:\n\n- touched up the documentation for ssh.variant to make it even easier to\n understand\n\n- free()d the config variable\n\n- completely refactored the code to fulfil Junio's burning desire to\n avoid split_cmdline() when unnecessary\n\nIt is quite preposterous to call this an \"iteration\" of the patch\nseries, because the code is so different now. I say this because I want\nto caution that this code has not been tested as thoroughly, by far, as\nthe first iteration.\n\nThe primary purpose of code review is correctness, everything else is\neither a consequence of it, or a means to make reviewing easier.\n\nThat means that I highly encourage those who pushed for these extensive\nchanges that make the patch series a lot less robust to balance things\nout by at least *rudimentary* testing.\n\n\nJohannes Schindelin (1):\n git_connect(): factor out SSH variant handling\n\nJunio C Hamano (1):\n connect: rename tortoiseplink and putty variables\n\nSegev Finer (2):\n connect: handle putty/plink also in GIT_SSH_COMMAND\n connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config\n\n Documentation/config.txt | 11 +++++++\n Documentation/git.txt | 6 ++++\n connect.c | 75 ++++++++++++++++++++++++++++++++++++------------\n t/t5601-clone.sh | 41 ++++++++++++++++++++++++++\n 4 files changed, 114 insertions(+), 19 deletions(-)\n\n\nbase-commit: 8f60064c1f538f06e1c579cbd9840b86b10bcd3d\nPublished-As: https://github.com/dscho/git/releases/tag/putty-w-args-v3\nFetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v3\n\nInterdiff vs v2:\n\n diff --git a/Documentation/config.txt b/Documentation/config.txt\n index f2c210f0a0..b88df57ab6 100644\n --- a/Documentation/config.txt\n +++ b/Documentation/config.txt\n @@ -1950,11 +1950,15 @@ matched against are those given directly to Git commands. This means any URLs\n visited as a result of a redirection do not participate in matching.\n \n ssh.variant::\n -\tOverride the autodetection of plink/tortoiseplink in the SSH\n -\tcommand that 'git fetch' and 'git push' use. It can be set to\n -\teither `ssh`, `plink`, `putty` or `tortoiseplink`. Any other\n -\tvalue will be treated as normal ssh. This is useful in case\n -\tthat Git gets this wrong.\n +\tDepending on the value of the environment variables `GIT_SSH` or\n +\t`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git\n +\tauto-detects whether to adjust its command-line parameters for use\n +\twith plink or tortoiseplink, as opposed to the default (OpenSSH).\n ++\n +The config variable `ssh.variant` can be set to override this auto-detection;\n +valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value\n +will be treated as normal ssh. This setting can be overridden via the\n +environment variable `GIT_SSH_VARIANT`.\n \n i18n.commitEncoding::\n \tCharacter encoding the commit messages are stored in; Git itself\n diff --git a/Documentation/git.txt b/Documentation/git.txt\n index c322558aa7..a0c6728d1a 100644\n --- a/Documentation/git.txt\n +++ b/Documentation/git.txt\n @@ -1021,11 +1021,10 @@ personal `.ssh/config` file. Please consult your ssh documentation\n for further details.\n \n `GIT_SSH_VARIANT`::\n -\tIf this environment variable is set, it overrides the autodetection\n -\tof plink/tortoiseplink in the SSH command that 'git fetch' and 'git\n -\tpush' use. It can be set to either `ssh`, `plink`, `putty` or\n -\t`tortoiseplink`. Any other value will be treated as normal ssh. This\n -\tis useful in case that Git gets this wrong.\n +\tIf this environment variable is set, it overrides Git's autodetection\n +\twhether `GIT_SSH`/`GIT_SSH_COMMAND`/`core.sshCommand` refer to OpenSSH,\n +\tplink or tortoiseplink. This variable overrides the config setting\n +\t`ssh.variant` that serves the same purpose.\n \n `GIT_ASKPASS`::\n \tIf this environment variable is set, then Git commands which need to\n diff --git a/connect.c b/connect.c\n index 7b4437578b..7f1f802396 100644\n --- a/connect.c\n +++ b/connect.c\n @@ -691,20 +691,45 @@ static const char *get_ssh_command(void)\n \treturn NULL;\n }\n \n -static int handle_ssh_variant(int *port_option, int *needs_batch)\n +static int handle_ssh_variant(const char *ssh_command, int is_cmdline,\n +\t\t\t int *port_option, int *needs_batch)\n {\n -\tconst char *variant;\n +\tconst char *variant = getenv(\"GIT_SSH_VARIANT\");\n +\tchar *p = NULL;\n +\n +\tif (variant)\n +\t\t; /* okay, fall through */\n +\telse if (!git_config_get_string(\"ssh.variant\", &p))\n +\t\tvariant = p;\n +\telse if (!is_cmdline) {\n +\t\tp = xstrdup(ssh_command);\n +\t\tvariant = basename(p);\n +\t} else {\n +\t\tconst char **ssh_argv;\n \n -\tif (!(variant = getenv(\"GIT_SSH_VARIANT\")) &&\n -\t\tgit_config_get_string_const(\"ssh.variant\", &variant))\n -\t\treturn 0;\n +\t\tp = xstrdup(ssh_command);\n +\t\tif (split_cmdline(p, &ssh_argv)) {\n +\t\t\tvariant = basename((char *)ssh_argv[0]);\n +\t\t\t/*\n +\t\t\t * At this point, variant points into the buffer\n +\t\t\t * referenced by p, hence we do not need ssh_argv\n +\t\t\t * any longer.\n +\t\t\t */\n +\t\t\tfree(ssh_argv);\n +\t\t} else\n +\t\t\treturn 0;\n +\t}\n \n -\tif (!strcmp(variant, \"plink\") || !strcmp(variant, \"putty\"))\n +\tif (!strcasecmp(variant, \"plink\") ||\n +\t !strcasecmp(variant, \"plink.exe\") ||\n +\t !strcasecmp(variant, \"putty\"))\n \t\t*port_option = 'P';\n -\telse if (!strcmp(variant, \"tortoiseplink\")) {\n +\telse if (!strcasecmp(variant, \"tortoiseplink\") ||\n +\t\t !strcasecmp(variant, \"tortoiseplink.exe\")) {\n \t\t*port_option = 'P';\n \t\t*needs_batch = 1;\n \t}\n +\tfree(p);\n \n \treturn 1;\n }\n @@ -791,7 +816,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\tint port_option = 'p';\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n -\t\t\tchar *ssh_argv0 = NULL;\n \t\t\ttransport_check_allowed(\"ssh\");\n \t\t\tget_host_and_port(&ssh_host, &port);\n \n @@ -812,15 +836,10 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t}\n \n \t\t\tssh = get_ssh_command();\n -\t\t\tif (ssh) {\n -\t\t\t\tchar *split_ssh = xstrdup(ssh);\n -\t\t\t\tconst char **ssh_argv;\n -\n -\t\t\t\tif (split_cmdline(split_ssh, &ssh_argv))\n -\t\t\t\t\tssh_argv0 = xstrdup(ssh_argv[0]);\n -\t\t\t\tfree(split_ssh);\n -\t\t\t\tfree((void *)ssh_argv);\n -\t\t\t} else {\n +\t\t\tif (ssh)\n +\t\t\t\thandle_ssh_variant(ssh, 1, &port_option,\n +\t\t\t\t\t\t &needs_batch);\n +\t\t\telse {\n \t\t\t\t/*\n \t\t\t\t * GIT_SSH is the no-shell version of\n \t\t\t\t * GIT_SSH_COMMAND (and must remain so for\n @@ -831,26 +850,12 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tssh = getenv(\"GIT_SSH\");\n \t\t\t\tif (!ssh)\n \t\t\t\t\tssh = \"ssh\";\n -\n -\t\t\t\tssh_argv0 = xstrdup(ssh);\n +\t\t\t\telse\n +\t\t\t\t\thandle_ssh_variant(ssh, 0,\n +\t\t\t\t\t\t\t &port_option,\n +\t\t\t\t\t\t\t &needs_batch);\n \t\t\t}\n \n -\t\t\tif (!handle_ssh_variant(&port_option, &needs_batch) &&\n -\t\t\t ssh_argv0) {\n -\t\t\t\tconst char *base = basename(ssh_argv0);\n -\n -\t\t\t\tif (!strcasecmp(base, \"tortoiseplink\") ||\n -\t\t\t\t !strcasecmp(base, \"tortoiseplink.exe\")) {\n -\t\t\t\t\tport_option = 'P';\n -\t\t\t\t\tneeds_batch = 1;\n -\t\t\t\t} else if (!strcasecmp(base, \"plink\") ||\n -\t\t\t\t\t !strcasecmp(base, \"plink.exe\")) {\n -\t\t\t\t\tport_option = 'P';\n -\t\t\t\t}\n -\t\t\t}\n -\n -\t\t\tfree(ssh_argv0);\n -\n \t\t\targv_array_push(&conn->args, ssh);\n \t\t\tif (flags & CONNECT_IPV4)\n \t\t\t\targv_array_push(&conn->args, \"-4\");\n\n-- \n2.11.1.windows.prerelease.2.9.g3014b57\n\n"},{"id":"310644","messageId":"fc511a35a8663f98e1da96d4be9991f7814b8874.1485950225.git.johannes.schindelin@gmx.de","threadId":"44806","inReplyTo":"cover.1485950225.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 1/4] connect: handle putty/plink also in GIT_SSH_COMMAND","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-01T12:01:00Z","receivedAt":"2017-02-01T12:01:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Segev Finer \n\nGit for Windows has special support for the popular SSH client PuTTY:\nwhen using PuTTY's non-interactive version (\"plink.exe\"), we use the -P\noption to specify the port rather than OpenSSH's -p option. TortoiseGit\nships with its own, forked version of plink.exe, that adds support for\nthe -batch option, and for good measure we special-case that, too.\n\nHowever, this special-casing of PuTTY only covers the case where the\nuser overrides the SSH command via the environment variable GIT_SSH\n(which allows specifying the name of the executable), not\nGIT_SSH_COMMAND (which allows specifying a full command, including\nadditional command-line options).\n\nWhen users want to pass any additional arguments to (Tortoise-)Plink,\nsuch as setting a private key, they are required to either use a shell\nscript named plink or tortoiseplink or duplicate the logic that is\nalready in Git for passing the correct style of command line arguments,\nwhich can be difficult, error prone and annoying to get right.\n\nThis patch simply reuses the existing logic and expands it to cover\nGIT_SSH_COMMAND, too.\n\nNote: it may look a little heavy-handed to duplicate the entire\ncommand-line and then split it, only to extract the name of the\nexecutable. However, this is not a performance-critical code path, and\nthe code is much more readable this way.\n\nSigned-off-by: Segev Finer \nSigned-off-by: Johannes Schindelin \n---\n connect.c | 23 ++++++++++++++++-------\n t/t5601-clone.sh | 15 +++++++++++++++\n 2 files changed, 31 insertions(+), 7 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 8cb93b0720..c81f77001b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -772,6 +772,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\tint putty = 0, tortoiseplink = 0;\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n+\t\t\tchar *ssh_argv0 = NULL;\n \t\t\ttransport_check_allowed(\"ssh\");\n \t\t\tget_host_and_port(&ssh_host, &port);\n \n@@ -792,10 +793,15 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t}\n \n \t\t\tssh = get_ssh_command();\n-\t\t\tif (!ssh) {\n-\t\t\t\tconst char *base;\n-\t\t\t\tchar *ssh_dup;\n-\n+\t\t\tif (ssh) {\n+\t\t\t\tchar *split_ssh = xstrdup(ssh);\n+\t\t\t\tconst char **ssh_argv;\n+\n+\t\t\t\tif (split_cmdline(split_ssh, &ssh_argv))\n+\t\t\t\t\tssh_argv0 = xstrdup(ssh_argv[0]);\n+\t\t\t\tfree(split_ssh);\n+\t\t\t\tfree((void *)ssh_argv);\n+\t\t\t} else {\n \t\t\t\t/*\n \t\t\t\t * GIT_SSH is the no-shell version of\n \t\t\t\t * GIT_SSH_COMMAND (and must remain so for\n@@ -807,8 +813,11 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tif (!ssh)\n \t\t\t\t\tssh = \"ssh\";\n \n-\t\t\t\tssh_dup = xstrdup(ssh);\n-\t\t\t\tbase = basename(ssh_dup);\n+\t\t\t\tssh_argv0 = xstrdup(ssh);\n+\t\t\t}\n+\n+\t\t\tif (ssh_argv0) {\n+\t\t\t\tconst char *base = basename(ssh_argv0);\n \n \t\t\t\ttortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n \t\t\t\t\t!strcasecmp(base, \"tortoiseplink.exe\");\n@@ -816,7 +825,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\t\t!strcasecmp(base, \"plink\") ||\n \t\t\t\t\t!strcasecmp(base, \"plink.exe\");\n \n-\t\t\t\tfree(ssh_dup);\n+\t\t\t\tfree(ssh_argv0);\n \t\t\t}\n \n \t\t\targv_array_push(&conn->args, ssh);\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 4241ea5b32..9335e10c2a 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -386,6 +386,21 @@ test_expect_success 'tortoiseplink is like putty, with extra arguments' '\n \texpect_ssh \"-batch -P 123\" myhost src\n '\n \n+test_expect_success 'double quoted plink.exe in GIT_SSH_COMMAND' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink.exe\" &&\n+\tGIT_SSH_COMMAND=\"\\\"$TRASH_DIRECTORY/plink.exe\\\" -v\" \\\n+\t\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-plink-3 &&\n+\texpect_ssh \"-v -P 123\" myhost src\n+'\n+\n+SQ=\"'\"\n+test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink.exe\" &&\n+\tGIT_SSH_COMMAND=\"$SQ$TRASH_DIRECTORY/plink.exe$SQ -v\" \\\n+\t\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-plink-4 &&\n+\texpect_ssh \"-v -P 123\" myhost src\n+'\n+\n # Reset the GIT_SSH environment variable for clone tests.\n setup_ssh_wrapper\n \n-- \n2.11.1.windows.prerelease.2.9.g3014b57\n\n\n"},{"id":"310645","messageId":"8d4852942119d378a4a625e97704cab88e769d8f.1485950225.git.johannes.schindelin@gmx.de","threadId":"44806","inReplyTo":"cover.1485950225.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 2/4] connect: rename tortoiseplink and putty variables","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-01T12:01:06Z","receivedAt":"2017-02-01T12:01:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Junio C Hamano \n\nOne of these two may have originally been named after \"what exact\nSSH implementation do we have\" so that we can tweak the command line\noptions, but these days \"putty=1\" no longer means \"We are using the\nplink SSH implementation that comes with PuTTY\". It is set when we\nguess that either PuTTY plink or Tortoiseplink is in use.\n\nRename them after what effect is desired. The current \"putty\"\noption is about using \"-P \" when OpenSSH would use \"-p \",\nso rename it to port_option whose value is either 'p' or 'P\". The\nother one is about passing an extra command line option \"-batch\",\nso rename it needs_batch.\n\n[jes: wrapped overly-long line]\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Johannes Schindelin \n---\n connect.c | 23 +++++++++++++----------\n 1 file changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex c81f77001b..9f750eacb6 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -769,7 +769,8 @@ 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 = 0, tortoiseplink = 0;\n+\t\t\tint needs_batch = 0;\n+\t\t\tint port_option = 'p';\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n \t\t\tchar *ssh_argv0 = NULL;\n@@ -819,12 +820,14 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\tif (ssh_argv0) {\n \t\t\t\tconst char *base = basename(ssh_argv0);\n \n-\t\t\t\ttortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n-\t\t\t\t\t!strcasecmp(base, \"tortoiseplink.exe\");\n-\t\t\t\tputty = tortoiseplink ||\n-\t\t\t\t\t!strcasecmp(base, \"plink\") ||\n-\t\t\t\t\t!strcasecmp(base, \"plink.exe\");\n-\n+\t\t\t\tif (!strcasecmp(base, \"tortoiseplink\") ||\n+\t\t\t\t !strcasecmp(base, \"tortoiseplink.exe\")) {\n+\t\t\t\t\tport_option = 'P';\n+\t\t\t\t\tneeds_batch = 1;\n+\t\t\t\t} else if (!strcasecmp(base, \"plink\") ||\n+\t\t\t\t\t !strcasecmp(base, \"plink.exe\")) {\n+\t\t\t\t\tport_option = 'P';\n+\t\t\t\t}\n \t\t\t\tfree(ssh_argv0);\n \t\t\t}\n \n@@ -833,11 +836,11 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\targv_array_push(&conn->args, \"-4\");\n \t\t\telse if (flags & CONNECT_IPV6)\n \t\t\t\targv_array_push(&conn->args, \"-6\");\n-\t\t\tif (tortoiseplink)\n+\t\t\tif (needs_batch)\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_pushf(&conn->args,\n+\t\t\t\t\t\t \"-%c\", port_option);\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-- \n2.11.1.windows.prerelease.2.9.g3014b57\n\n\n"},{"id":"310646","messageId":"838100594d8461d05da365d1b1c97c1f6c8c8ea3.1485950225.git.johannes.schindelin@gmx.de","threadId":"44806","inReplyTo":"cover.1485950225.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 3/4] git_connect(): factor out SSH variant handling","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-01T12:01:10Z","receivedAt":"2017-02-01T12:01:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We handle plink and tortoiseplink as OpenSSH replacements, by passing\nthe correct command-line options when detecting that they are used.\n\nTo let users override that auto-detection (in case Git gets it wrong),\nwe need to introduce new code to that end.\n\nIn preparation for this code, let's factor out the SSH variant handling\ninto its own function, handle_ssh_variant().\n\nSigned-off-by: Johannes Schindelin \n---\n connect.c | 72 ++++++++++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 46 insertions(+), 26 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 9f750eacb6..2734b9a1ca 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -691,6 +691,44 @@ static const char *get_ssh_command(void)\n \treturn NULL;\n }\n \n+static int handle_ssh_variant(const char *ssh_command, int is_cmdline,\n+\t\t\t int *port_option, int *needs_batch)\n+{\n+\tconst char *variant;\n+\tchar *p = NULL;\n+\n+\tif (!is_cmdline) {\n+\t\tp = xstrdup(ssh_command);\n+\t\tvariant = basename(p);\n+\t} else {\n+\t\tconst char **ssh_argv;\n+\n+\t\tp = xstrdup(ssh_command);\n+\t\tif (split_cmdline(p, &ssh_argv)) {\n+\t\t\tvariant = basename((char *)ssh_argv[0]);\n+\t\t\t/*\n+\t\t\t * At this point, variant points into the buffer\n+\t\t\t * referenced by p, hence we do not need ssh_argv\n+\t\t\t * any longer.\n+\t\t\t */\n+\t\t\tfree(ssh_argv);\n+\t\t} else\n+\t\t\treturn 0;\n+\t}\n+\n+\tif (!strcasecmp(variant, \"plink\") ||\n+\t !strcasecmp(variant, \"plink.exe\"))\n+\t\t*port_option = 'P';\n+\telse if (!strcasecmp(variant, \"tortoiseplink\") ||\n+\t\t !strcasecmp(variant, \"tortoiseplink.exe\")) {\n+\t\t*port_option = 'P';\n+\t\t*needs_batch = 1;\n+\t}\n+\tfree(p);\n+\n+\treturn 1;\n+}\n+\n /*\n * This returns a dummy child_process if the transport protocol does not\n * need fork(2), or a struct child_process object if it does. Once done,\n@@ -773,7 +811,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\tint port_option = 'p';\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n-\t\t\tchar *ssh_argv0 = NULL;\n \t\t\ttransport_check_allowed(\"ssh\");\n \t\t\tget_host_and_port(&ssh_host, &port);\n \n@@ -794,15 +831,10 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t}\n \n \t\t\tssh = get_ssh_command();\n-\t\t\tif (ssh) {\n-\t\t\t\tchar *split_ssh = xstrdup(ssh);\n-\t\t\t\tconst char **ssh_argv;\n-\n-\t\t\t\tif (split_cmdline(split_ssh, &ssh_argv))\n-\t\t\t\t\tssh_argv0 = xstrdup(ssh_argv[0]);\n-\t\t\t\tfree(split_ssh);\n-\t\t\t\tfree((void *)ssh_argv);\n-\t\t\t} else {\n+\t\t\tif (ssh)\n+\t\t\t\thandle_ssh_variant(ssh, 1, &port_option,\n+\t\t\t\t\t\t &needs_batch);\n+\t\t\telse {\n \t\t\t\t/*\n \t\t\t\t * GIT_SSH is the no-shell version of\n \t\t\t\t * GIT_SSH_COMMAND (and must remain so for\n@@ -813,22 +845,10 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\tssh = getenv(\"GIT_SSH\");\n \t\t\t\tif (!ssh)\n \t\t\t\t\tssh = \"ssh\";\n-\n-\t\t\t\tssh_argv0 = xstrdup(ssh);\n-\t\t\t}\n-\n-\t\t\tif (ssh_argv0) {\n-\t\t\t\tconst char *base = basename(ssh_argv0);\n-\n-\t\t\t\tif (!strcasecmp(base, \"tortoiseplink\") ||\n-\t\t\t\t !strcasecmp(base, \"tortoiseplink.exe\")) {\n-\t\t\t\t\tport_option = 'P';\n-\t\t\t\t\tneeds_batch = 1;\n-\t\t\t\t} else if (!strcasecmp(base, \"plink\") ||\n-\t\t\t\t\t !strcasecmp(base, \"plink.exe\")) {\n-\t\t\t\t\tport_option = 'P';\n-\t\t\t\t}\n-\t\t\t\tfree(ssh_argv0);\n+\t\t\t\telse\n+\t\t\t\t\thandle_ssh_variant(ssh, 0,\n+\t\t\t\t\t\t\t &port_option,\n+\t\t\t\t\t\t\t &needs_batch);\n \t\t\t}\n \n \t\t\targv_array_push(&conn->args, ssh);\n-- \n2.11.1.windows.prerelease.2.9.g3014b57\n\n\n"},{"id":"310647","messageId":"9780d67c9f11c056202987377c542d0313772ba2.1485950225.git.johannes.schindelin@gmx.de","threadId":"44806","inReplyTo":"cover.1485950225.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-01T12:01:16Z","receivedAt":"2017-02-01T12:01:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Segev Finer \n\nThis environment variable and configuration value allow to\noverride the autodetection of plink/tortoiseplink in case that\nGit gets it wrong.\n\n[jes: wrapped overly-long lines, factored out and changed\nget_ssh_variant() to handle_ssh_variant() to accomodate the\nchange from the putty/tortoiseplink variables to\nport_option/needs_batch, adjusted the documentation, free()d\nvalue obtained from the config.]\n\nSigned-off-by: Segev Finer \nSigned-off-by: Johannes Schindelin \n---\n Documentation/config.txt | 11 +++++++++++\n Documentation/git.txt | 6 ++++++\n connect.c | 11 ++++++++---\n t/t5601-clone.sh | 26 ++++++++++++++++++++++++++\n 4 files changed, 51 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex af2ae4cc02..b88df57ab6 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1949,6 +1949,17 @@ Environment variable settings always override any matches. The URLs that are\n matched against are those given directly to Git commands. This means any URLs\n visited as a result of a redirection do not participate in matching.\n \n+ssh.variant::\n+\tDepending on the value of the environment variables `GIT_SSH` or\n+\t`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git\n+\tauto-detects whether to adjust its command-line parameters for use\n+\twith plink or tortoiseplink, as opposed to the default (OpenSSH).\n++\n+The config variable `ssh.variant` can be set to override this auto-detection;\n+valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value\n+will be treated as normal ssh. This setting can be overridden via the\n+environment variable `GIT_SSH_VARIANT`.\n+\n i18n.commitEncoding::\n \tCharacter encoding the commit messages are stored in; Git itself\n \tdoes not care per se, but this information is necessary e.g. when\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 4f208fab92..a0c6728d1a 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -1020,6 +1020,12 @@ Usually it is easier to configure any desired options through your\n personal `.ssh/config` file. Please consult your ssh documentation\n for further details.\n \n+`GIT_SSH_VARIANT`::\n+\tIf this environment variable is set, it overrides Git's autodetection\n+\twhether `GIT_SSH`/`GIT_SSH_COMMAND`/`core.sshCommand` refer to OpenSSH,\n+\tplink or tortoiseplink. This variable overrides the config setting\n+\t`ssh.variant` that serves the same purpose.\n+\n `GIT_ASKPASS`::\n \tIf this environment variable is set, then Git commands which need to\n \tacquire passwords or passphrases (e.g. for HTTP or IMAP authentication)\ndiff --git a/connect.c b/connect.c\nindex 2734b9a1ca..7f1f802396 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -694,10 +694,14 @@ static const char *get_ssh_command(void)\n static int handle_ssh_variant(const char *ssh_command, int is_cmdline,\n \t\t\t int *port_option, int *needs_batch)\n {\n-\tconst char *variant;\n+\tconst char *variant = getenv(\"GIT_SSH_VARIANT\");\n \tchar *p = NULL;\n \n-\tif (!is_cmdline) {\n+\tif (variant)\n+\t\t; /* okay, fall through */\n+\telse if (!git_config_get_string(\"ssh.variant\", &p))\n+\t\tvariant = p;\n+\telse if (!is_cmdline) {\n \t\tp = xstrdup(ssh_command);\n \t\tvariant = basename(p);\n \t} else {\n@@ -717,7 +721,8 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,\n \t}\n \n \tif (!strcasecmp(variant, \"plink\") ||\n-\t !strcasecmp(variant, \"plink.exe\"))\n+\t !strcasecmp(variant, \"plink.exe\") ||\n+\t !strcasecmp(variant, \"putty\"))\n \t\t*port_option = 'P';\n \telse if (!strcasecmp(variant, \"tortoiseplink\") ||\n \t\t !strcasecmp(variant, \"tortoiseplink.exe\")) {\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 9335e10c2a..b52b8acf98 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -401,6 +401,32 @@ test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '\n \texpect_ssh \"-v -P 123\" myhost src\n '\n \n+test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n+\tGIT_SSH_VARIANT=ssh \\\n+\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-variant-1 &&\n+\texpect_ssh \"-p 123\" myhost src\n+'\n+\n+test_expect_success 'ssh.variant overrides plink detection' '\n+\tcopy_ssh_wrapper_as \"$TRASH_DIRECTORY/plink\" &&\n+\tgit -c ssh.variant=ssh \\\n+\t\tclone \"[myhost:123]:src\" ssh-bracket-clone-variant-2 &&\n+\texpect_ssh \"-p 123\" myhost src\n+'\n+\n+test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '\n+\tGIT_SSH_VARIANT=plink \\\n+\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-variant-3 &&\n+\texpect_ssh \"-P 123\" myhost src\n+'\n+\n+test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '\n+\tGIT_SSH_VARIANT=tortoiseplink \\\n+\tgit clone \"[myhost:123]:src\" ssh-bracket-clone-variant-4 &&\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-- \n2.11.1.windows.prerelease.2.9.g3014b57\n"},{"id":"310648","messageId":"alpine.DEB.2.20.1702011216130.3469@virtualbox","threadId":"44806","inReplyTo":"xmqqpoj8z7su.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-01T12:01:25Z","receivedAt":"2017-02-01T12:01:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 27 Jan 2017, Junio C Hamano wrote:\n\n> IOW, I think it is acceptable to always split GIT_SSH_COMMAND into\n> tokens before we realize that the user used the escape hatch and the\n> splitting was a wasted effort. This is exactly because this thing\n> is an escape hatch that is expected to be rarely used. Of course,\n> if the \"wasted effort\" can be eliminated without sacrificing the\n> simplicity of the code, that is fine as well.\n\nSimplicity is retained. Battle-readiness was sacrificed on the way: the\nnew code is not tested well enough, and `next` will not help one bit.\n\nCiao,\nJohannes\n"},{"id":"310651","messageId":"xmqqlgtpq2cz.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"alpine.DEB.2.20.1702011216130.3469@virtualbox","subject":"Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-01T16:53:32Z","receivedAt":"2017-02-01T16:53:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Fri, 27 Jan 2017, Junio C Hamano wrote:\n>\n>> IOW, I think it is acceptable to always split GIT_SSH_COMMAND into\n>> tokens before we realize that the user used the escape hatch and the\n>> splitting was a wasted effort. This is exactly because this thing\n>> is an escape hatch that is expected to be rarely used. Of course,\n>> if the \"wasted effort\" can be eliminated without sacrificing the\n>> simplicity of the code, that is fine as well.\n>\n> Simplicity is retained. Battle-readiness was sacrificed on the way: the\n> new code is not tested well enough, and `next` will not help one bit.\n\nLet me make it clear that there is no burning desire to sacrifice\nbattle-readiness in the above. If we expect that auto-detection\nwould be minority, then it makes sense to get the configured value\nfirst and then spend cycles to split and guess only when detection\nis needed. \n\nIn this case, because we expect that auto-detection will be used\nmost of the time, it is good enough to always split first, get the\nconfigured value, and spend cycles to guess, or for that matter it\nis perfectly fine to always split and guess first and then override\nwith the configured value.\n\nIf your attempt to optimize for a wrong case ended up causing new\nunnecessary bugs, don't blame me.\n"},{"id":"310656","messageId":"xmqq1svhpvm0.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"9780d67c9f11c056202987377c542d0313772ba2.1485950225.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-01T19:19:19Z","receivedAt":"2017-02-01T19:19:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> index 2734b9a1ca..7f1f802396 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -694,10 +694,14 @@ static const char *get_ssh_command(void)\n> static int handle_ssh_variant(const char *ssh_command, int is_cmdline,\n> \t\t\t int *port_option, int *needs_batch)\n> {\n> -\tconst char *variant;\n> +\tconst char *variant = getenv(\"GIT_SSH_VARIANT\");\n> \tchar *p = NULL;\n> \n> -\tif (!is_cmdline) {\n> +\tif (variant)\n> +\t\t; /* okay, fall through */\n> +\telse if (!git_config_get_string(\"ssh.variant\", &p))\n> +\t\tvariant = p;\n> +\telse if (!is_cmdline) {\n> \t\tp = xstrdup(ssh_command);\n> \t\tvariant = basename(p);\n> \t} else {\n> @@ -717,7 +721,8 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,\n> \t}\n> \n> \tif (!strcasecmp(variant, \"plink\") ||\n> -\t !strcasecmp(variant, \"plink.exe\"))\n> +\t !strcasecmp(variant, \"plink.exe\") ||\n> +\t !strcasecmp(variant, \"putty\"))\n\nThis means that \"putty\" that appear as the first word of the command\nline, not the value configured for ssh.variant or GIT_SSH_VARIANT,\nwill also trigger the option for \"plink\", no?\n\nWorse, \"plink.exe\" configured as the value of \"ssh.variant\", is\nanything other than 'ssh', 'plink', 'putty', or 'tortoiseplink', but\nit is not treated as normal ssh, contrary to the documentation.\n\n> +ssh.variant::\n> +\tDepending on the value of the environment variables `GIT_SSH` or\n> +\t`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git\n> +\tauto-detects whether to adjust its command-line parameters for use\n> +\twith plink or tortoiseplink, as opposed to the default (OpenSSH).\n> ++\n> +The config variable `ssh.variant` can be set to override this auto-detection;\n> +valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value\n> +will be treated as normal ssh. This setting can be overridden via the\n> +environment variable `GIT_SSH_VARIANT`.\n\nI was hoping that the \"rewrite that retains simplicity\" was also\ncorrect, but let's not go in this direction. The more lines you\ntouch in a hurry, the worse the code will get, and that is to be\nexpected.\n\nI'd suggest taking the documentation updates from this series, and\nthen minimally plug the leak introduced by the previous round,\nperhaps like the attached SQUASH. As I said, GIT_SSH_VARIANT and\nssh.variant are expected to be rare cases, and the case in which\nthey are set does not have to be optimized if it makes the necessary\nchange smaller and more likely to be correct.\n\nI think restructuring along the line of 3/4 of this round is very\nsensible in the longer term (if anything, handle_ssh_variant() now\nreally handles ssh variants in all cases, which makes it worthy of\nits name, as opposed to the one in the previous round that only\nreacts to the overrides). But it seems that it will take longer to\nget such a rewrite right, and my priority is seeing this topic to\nadd autodetection to GIT_SSH_COMMAND and other smaller topics in the\nupcoming -rc0 in a serviceable and correct shape.\n\nThe restructuring done by 3/4 can come later after the dust settles,\nif somebody cares deeply enough about performance in the rare cases.\n\n Documentation/config.txt | 14 +++++++++-----\n Documentation/git.txt | 9 ++++-----\n connect.c | 9 +++++----\n 3 files changed, 18 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex f2c210f0a0..b88df57ab6 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1950,11 +1950,15 @@ matched against are those given directly to Git commands. This means any URLs\n visited as a result of a redirection do not participate in matching.\n \n ssh.variant::\n-\tOverride the autodetection of plink/tortoiseplink in the SSH\n-\tcommand that 'git fetch' and 'git push' use. It can be set to\n-\teither `ssh`, `plink`, `putty` or `tortoiseplink`. Any other\n-\tvalue will be treated as normal ssh. This is useful in case\n-\tthat Git gets this wrong.\n+\tDepending on the value of the environment variables `GIT_SSH` or\n+\t`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git\n+\tauto-detects whether to adjust its command-line parameters for use\n+\twith plink or tortoiseplink, as opposed to the default (OpenSSH).\n++\n+The config variable `ssh.variant` can be set to override this auto-detection;\n+valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value\n+will be treated as normal ssh. This setting can be overridden via the\n+environment variable `GIT_SSH_VARIANT`.\n \n i18n.commitEncoding::\n \tCharacter encoding the commit messages are stored in; Git itself\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex c322558aa7..a0c6728d1a 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -1021,11 +1021,10 @@ personal `.ssh/config` file. Please consult your ssh documentation\n for further details.\n \n `GIT_SSH_VARIANT`::\n-\tIf this environment variable is set, it overrides the autodetection\n-\tof plink/tortoiseplink in the SSH command that 'git fetch' and 'git\n-\tpush' use. It can be set to either `ssh`, `plink`, `putty` or\n-\t`tortoiseplink`. Any other value will be treated as normal ssh. This\n-\tis useful in case that Git gets this wrong.\n+\tIf this environment variable is set, it overrides Git's autodetection\n+\twhether `GIT_SSH`/`GIT_SSH_COMMAND`/`core.sshCommand` refer to OpenSSH,\n+\tplink or tortoiseplink. This variable overrides the config setting\n+\t`ssh.variant` that serves the same purpose.\n \n `GIT_ASKPASS`::\n \tIf this environment variable is set, then Git commands which need to\ndiff --git a/connect.c b/connect.c\nindex 7b4437578b..da51fef9ee 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -693,10 +693,11 @@ static const char *get_ssh_command(void)\n \n static int handle_ssh_variant(int *port_option, int *needs_batch)\n {\n-\tconst char *variant;\n+\tchar *variant;\n \n-\tif (!(variant = getenv(\"GIT_SSH_VARIANT\")) &&\n-\t\tgit_config_get_string_const(\"ssh.variant\", &variant))\n+\tvariant = xstrdup_or_null(getenv(\"GIT_SSH_VARIANT\"));\n+\tif (!variant &&\n+\t git_config_get_string(\"ssh.variant\", &variant))\n \t\treturn 0;\n \n \tif (!strcmp(variant, \"plink\") || !strcmp(variant, \"putty\"))\n@@ -705,7 +706,7 @@ static int handle_ssh_variant(int *port_option, int *needs_batch)\n \t\t*port_option = 'P';\n \t\t*needs_batch = 1;\n \t}\n-\n+\tfree(variant);\n \treturn 1;\n }\n \n"},{"id":"310657","messageId":"xmqqwpd9ofry.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"xmqq1svhpvm0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-01T19:46:41Z","receivedAt":"2017-02-01T19:46:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I think restructuring along the line of 3/4 of this round is very\n> sensible in the longer term (if anything, handle_ssh_variant() now\n> really handles ssh variants in all cases, which makes it worthy of\n> its name, as opposed to the one in the previous round that only\n> reacts to the overrides). But it seems that it will take longer to\n> get such a rewrite right, and my priority is seeing this topic to\n> add autodetection to GIT_SSH_COMMAND and other smaller topics in the\n> upcoming -rc0 in a serviceable and correct shape.\n>\n> The restructuring done by 3/4 can come later after the dust settles,\n> if somebody cares deeply enough about performance in the rare cases.\n\nHaving said all that, because I like the patch 3/4 so much, here is\nanother way to fix this, and I think (of course I am biased, but...)\nthe result is easier to grok. Not only it makes it clear that the\nnamespace for the actual command names on the command line and the\nnamespace for the supported values of the configuration are distinct,\nit makes it clear that we do not do anything extra when the user\nexplicitly tells us which variant to use.\n\nThis is designed to be squashed into [4/4] of this latest round (as\nopposed to the one in the message I am responding to, which is to be\nsquashed into the previous round).\n\n connect.c | 42 +++++++++++++++++++++++++++++++-----------\n 1 file changed, 31 insertions(+), 11 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 7f1f802396..12ad8d4c8b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -691,17 +691,39 @@ static const char *get_ssh_command(void)\n \treturn NULL;\n }\n \n-static int handle_ssh_variant(const char *ssh_command, int is_cmdline,\n-\t\t\t int *port_option, int *needs_batch)\n+static int override_ssh_variant(int *port_option, int *needs_batch)\n {\n-\tconst char *variant = getenv(\"GIT_SSH_VARIANT\");\n+\tchar *variant;\n+\n+\tvariant = xstrdup_or_null(getenv(\"GIT_SSH_VARIANT\"));\n+\tif (!variant &&\n+\t git_config_get_string(\"ssh.variant\", &variant))\n+\t\treturn 0;\n+\n+\tif (!strcmp(variant, \"plink\") || !strcmp(variant, \"putty\")) {\n+\t\t*port_option = 'P';\n+\t\t*needs_batch = 0;\n+\t} else if (!strcmp(variant, \"tortoiseplink\")) {\n+\t\t*port_option = 'P';\n+\t\t*needs_batch = 1;\n+\t} else {\n+\t\t*port_option = 'p';\n+\t\t*needs_batch = 0;\n+\t}\n+\tfree(variant);\n+\treturn 1;\n+}\n+\n+static void handle_ssh_variant(const char *ssh_command, int is_cmdline,\n+\t\t\t int *port_option, int *needs_batch)\n+{\n+\tconst char *variant;\n \tchar *p = NULL;\n \n-\tif (variant)\n-\t\t; /* okay, fall through */\n-\telse if (!git_config_get_string(\"ssh.variant\", &p))\n-\t\tvariant = p;\n-\telse if (!is_cmdline) {\n+\tif (override_ssh_variant(port_option, needs_batch))\n+\t\treturn;\n+\n+\tif (!is_cmdline) {\n \t\tp = xstrdup(ssh_command);\n \t\tvariant = basename(p);\n \t} else {\n@@ -717,7 +739,7 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,\n \t\t\t */\n \t\t\tfree(ssh_argv);\n \t\t} else\n-\t\t\treturn 0;\n+\t\t\treturn;\n \t}\n \n \tif (!strcasecmp(variant, \"plink\") ||\n@@ -730,8 +752,6 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,\n \t\t*needs_batch = 1;\n \t}\n \tfree(p);\n-\n-\treturn 1;\n }\n \n /*\n"},{"id":"310659","messageId":"xmqqr33hoetx.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"cover.1485950225.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-01T20:07:06Z","receivedAt":"2017-02-01T20:07:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> It is quite preposterous to call this an \"iteration\" of the patch\n> series, because the code is so different now. I say this because I want\n> to caution that this code has not been tested as thoroughly, by far, as\n> the first iteration.\n>\n> The primary purpose of code review is correctness, everything else is\n> either a consequence of it, or a means to make reviewing easier.\n\nYou are utterly wrong here.\n\nThe primary purpose of code review is to spot and correct the\nproblems the submitter has missed. The problems can span from\nstupid bugs that affect correctness to maintainability, to design\nmistakes, to clarity of explanation for both end users and future\ndevelopers.\n\nAmong them, correctness problems are, as long as the problem to be\nsolved is specified clearly enough, the easiest to spot by the\nsubmitter before the patch is sent out. The submitter is focused on\nsolving one problem, and if the updated code does not even work as\nthe submitter advertises it would, that can be caught by the\nsubmitter before the patch is even sent out. \n\nOf course, humans are not perfect, and catching correctness problems\nis important, but that is not the only (let alone primary) thing.\n\nWhen a submitter has been focusing on solving one problem, it is\neasy to lose the larger picture and to end up adding something that\nmay be \"correct\" (in the sense of \"works as advertised by the\nsubmitter\") but does not fit well with the rest of the system, or\ncovers some use cases but misses other important and related use\ncases. If the \"does not fit well\" surfaces to the end user level,\nthat would become a design problem. If it affects the future\ndevelopers, that would become a maintainability problem.\n\nCompared to the correctness issue, these are much harder to spot by\nthe submitter alone, who focused so intensely to get his code\n\"correct\". The review process is of greater value to spot these\nissues.\n\nI've already read and commented on the series; as I said, I think\nthe rewrite in 3/4 makes the resulting code much easier to read, and\nwith the fix-up I just sent out, I think the end result is correct,\nworks as advertised, the way it is advertised is clear to end users,\nand is maintainable. \n\nBut I do share your uneasiness about the \"new-ness\" of the code.\n"},{"id":"310667","messageId":"alpine.DEB.2.20.1702012311440.3496@virtualbox","threadId":"44806","inReplyTo":"xmqqr33hoetx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-01T22:17:54Z","receivedAt":"2017-02-01T22:18:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 1 Feb 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > It is quite preposterous to call this an \"iteration\" of the patch\n> > series, because the code is so different now. I say this because I want\n> > to caution that this code has not been tested as thoroughly, by far, as\n> > the first iteration.\n> >\n> > The primary purpose of code review is correctness, everything else is\n> > either a consequence of it, or a means to make reviewing easier.\n> \n> You are utterly wrong here.\n> \n> The primary purpose of code review is to spot and correct the\n> problems the submitter has missed. The problems can span from\n> stupid bugs that affect correctness to maintainability, to design\n> mistakes, to clarity of explanation for both end users and future\n> developers.\n> \n> Among them, correctness problems are, as long as the problem to be\n> solved is specified clearly enough, the easiest to spot by the\n> submitter before the patch is sent out. The submitter is focused on\n> solving one problem, and if the updated code does not even work as\n> the submitter advertises it would, that can be caught by the\n> submitter before the patch is even sent out. \n> \n> Of course, humans are not perfect, and catching correctness problems\n> is important, but that is not the only (let alone primary) thing.\n> \n> When a submitter has been focusing on solving one problem, it is\n> easy to lose the larger picture and to end up adding something that\n> may be \"correct\" (in the sense of \"works as advertised by the\n> submitter\") but does not fit well with the rest of the system, or\n> covers some use cases but misses other important and related use\n> cases. If the \"does not fit well\" surfaces to the end user level,\n> that would become a design problem. If it affects the future\n> developers, that would become a maintainability problem.\n> \n> Compared to the correctness issue, these are much harder to spot by\n> the submitter alone, who focused so intensely to get his code\n> \"correct\". The review process is of greater value to spot these\n> issues.\n\nWe will never agree on this.\n\nFrom my perspective, design, explanation and maintainability are a\nconsequence of making it easy for reviewers to spot where the code is\nincorrect.\n\nAnd correctness is not covered by \"the submitter tested this\". Correctness\nincludes all the corner cases, where the \"many eyes make bugs shallow\"\nreally shines.\n\nI'd rather have reviewers find bugs than users.\n\nI will *never* be a fan of a review process that pushes correctness to a\nback seat (yes, it is much harder than spotting typos or lines longer than\n80 columns per row, but the ultimate goal is to deliver value to the end\nuser, not to make life easy for the maintainer).\n\nCiao,\nJohannes\n"},{"id":"310668","messageId":"alpine.DEB.2.20.1702012319460.3496@virtualbox","threadId":"44806","inReplyTo":"xmqqwpd9ofry.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-01T22:24:04Z","receivedAt":"2017-02-01T22:25:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 1 Feb 2017, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > I think restructuring along the line of 3/4 of this round is very\n> > sensible in the longer term (if anything, handle_ssh_variant() now\n> > really handles ssh variants in all cases, which makes it worthy of\n> > its name, as opposed to the one in the previous round that only\n> > reacts to the overrides). But it seems that it will take longer to\n> > get such a rewrite right, and my priority is seeing this topic to\n> > add autodetection to GIT_SSH_COMMAND and other smaller topics in the\n> > upcoming -rc0 in a serviceable and correct shape.\n> >\n> > The restructuring done by 3/4 can come later after the dust settles,\n> > if somebody cares deeply enough about performance in the rare cases.\n> \n> Having said all that, because I like the patch 3/4 so much, here is\n> another way to fix this, and I think (of course I am biased, but...)\n> the result is easier to grok. Not only it makes it clear that the\n> namespace for the actual command names on the command line and the\n> namespace for the supported values of the configuration are distinct,\n> it makes it clear that we do not do anything extra when the user\n> explicitly tells us which variant to use.\n> \n> This is designed to be squashed into [4/4] of this latest round (as\n> opposed to the one in the message I am responding to, which is to be\n> squashed into the previous round).\n> \n> connect.c | 42 +++++++++++++++++++++++++++++++-----------\n> 1 file changed, 31 insertions(+), 11 deletions(-)\n> \n> diff --git a/connect.c b/connect.c\n> index 7f1f802396..12ad8d4c8b 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -691,17 +691,39 @@ static const char *get_ssh_command(void)\n> \treturn NULL;\n> }\n> \n> -static int handle_ssh_variant(const char *ssh_command, int is_cmdline,\n> -\t\t\t int *port_option, int *needs_batch)\n> +static int override_ssh_variant(int *port_option, int *needs_batch)\n> {\n> -\tconst char *variant = getenv(\"GIT_SSH_VARIANT\");\n> +\tchar *variant;\n> +\n> +\tvariant = xstrdup_or_null(getenv(\"GIT_SSH_VARIANT\"));\n> +\tif (!variant &&\n> +\t git_config_get_string(\"ssh.variant\", &variant))\n> +\t\treturn 0;\n> +\n> +\tif (!strcmp(variant, \"plink\") || !strcmp(variant, \"putty\")) {\n> +\t\t*port_option = 'P';\n> +\t\t*needs_batch = 0;\n> +\t} else if (!strcmp(variant, \"tortoiseplink\")) {\n> +\t\t*port_option = 'P';\n> +\t\t*needs_batch = 1;\n> +\t} else {\n> +\t\t*port_option = 'p';\n> +\t\t*needs_batch = 0;\n> +\t}\n> +\tfree(variant);\n> +\treturn 1;\n> +}\n> +\n> +static void handle_ssh_variant(const char *ssh_command, int is_cmdline,\n> +\t\t\t int *port_option, int *needs_batch)\n> +{\n> +\tconst char *variant;\n> \tchar *p = NULL;\n> \n> -\tif (variant)\n> -\t\t; /* okay, fall through */\n> -\telse if (!git_config_get_string(\"ssh.variant\", &p))\n> -\t\tvariant = p;\n> -\telse if (!is_cmdline) {\n> +\tif (override_ssh_variant(port_option, needs_batch))\n> +\t\treturn;\n> +\n> +\tif (!is_cmdline) {\n> \t\tp = xstrdup(ssh_command);\n> \t\tvariant = basename(p);\n> \t} else {\n> @@ -717,7 +739,7 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,\n> \t\t\t */\n> \t\t\tfree(ssh_argv);\n> \t\t} else\n> -\t\t\treturn 0;\n> +\t\t\treturn;\n> \t}\n> \n> \tif (!strcasecmp(variant, \"plink\") ||\n> @@ -730,8 +752,6 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,\n> \t\t*needs_batch = 1;\n> \t}\n> \tfree(p);\n> -\n> -\treturn 1;\n> }\n> \n> /*\n\nThat leaves the \"putty\" case in handle_ssh_variant(), does it not? Was it\nnot your specific objection that that is the case?\n\nNo worries, I will let this simmer for a while. Your fixup has a lot of\nduplicated code (so much for maintainability as an important goal... ;-))\nand I will have to think about it. My immediate thinking is to *not*\nduplicate code, strip the .exe extension directly after calling basename()\nin the cases where we handle commands, and handle \"putty\" in the other\ncases by replacing it with the string \"plink\".\n\nBut as I said, this will simmer for a while.\n\nCiao,\nJohannes\n"},{"id":"310671","messageId":"xmqqzii5mthp.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"alpine.DEB.2.20.1702012319460.3496@virtualbox","subject":"Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-01T22:33:22Z","receivedAt":"2017-02-01T22:33:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> That leaves the \"putty\" case in handle_ssh_variant(), does it not? Was it\n> not your specific objection that that is the case?\n\nYup, you can remove that while you reroll.\n\n> No worries, I will let this simmer for a while. Your fixup has a lot of\n> duplicated code (so much for maintainability as an important goal... ;-))\n> and I will have to think about it. My immediate thinking is to *not*\n> duplicate code,...\n\nYou need to realize that the namespaces of the configuration and the\ncommand names are distinct. There is no code duplication.\n"},{"id":"310673","messageId":"alpine.DEB.2.20.1702012339080.3496@virtualbox","threadId":"44806","inReplyTo":"xmqqzii5mthp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-01T22:42:09Z","receivedAt":"2017-02-01T22:42:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 1 Feb 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > That leaves the \"putty\" case in handle_ssh_variant(), does it not? Was it\n> > not your specific objection that that is the case?\n> \n> Yup, you can remove that while you reroll.\n> \n> > No worries, I will let this simmer for a while. Your fixup has a lot of\n> > duplicated code (so much for maintainability as an important goal... ;-))\n> > and I will have to think about it. My immediate thinking is to *not*\n> > duplicate code,...\n> \n> You need to realize that the namespaces of the configuration and the\n> command names are distinct. There is no code duplication.\n\nSince your 2/4 did away with the \"plink\" and \"tortoiseplink\" values in\nfavor of \"port_option\" and \"batch_option\", there is a duplication of logic\nwhich I tried to undo in v3 and which you reintroduce in your fixup.\n\nMaybe that refactoring was not so smart to begin with, and we should have\ninstead modified the code to use an enum instead and keep the original\nconditionals instead of setting a port_option and a batch_option\nexplicitly.\n\nCiao,\nJohannes\n"},{"id":"310674","messageId":"xmqqvastmt09.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"xmqqzii5mthp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-01T22:43:50Z","receivedAt":"2017-02-01T22:43:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>> That leaves the \"putty\" case in handle_ssh_variant(), does it not? Was it\n>> not your specific objection that that is the case?\n>\n> Yup, you can remove that while you reroll.\n>\n>> No worries, I will let this simmer for a while. Your fixup has a lot of\n>> duplicated code (so much for maintainability as an important goal... ;-))\n>> and I will have to think about it. My immediate thinking is to *not*\n>> duplicate code,...\n>\n> You need to realize that the namespaces of the configuration and the\n> command names are distinct. There is no code duplication.\n\nTo explain this a bit, there is no reason why allowed values for\nSSH_VARIANT must be \"putty\" and \"tortoiseplink\". An alternative\ndesign could be \"port_option=-p,needs_batch=yes\" and it may be more\nlogical and futureproof if a variant of tortoiseplink decides to use\n\"-p\" instead of \"-P\" and still require \"-batch\".\n\nPrematurely attempting to share code, only because the current\nvocabularies for two distinct concepts happen to overlap, is not\nde-duplicating the code for maintainability. It is adding\nunnecessary work other people need to do in the future when they\nwant to extend the system.\n\n"},{"id":"310675","messageId":"xmqqr33hmshc.fsf@gitster.mtv.corp.google.com","threadId":"44806","inReplyTo":"alpine.DEB.2.20.1702012311440.3496@virtualbox","subject":"Re: [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-01T22:55:11Z","receivedAt":"2017-02-01T22:55:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Compared to the correctness issue, these are much harder to spot by\n>> the submitter alone, who focused so intensely to get his code\n>> \"correct\". The review process is of greater value to spot these\n>> issues.\n>\n> We will never agree on this.\n\nThat's too bad.\n\n> From my perspective, design, explanation and maintainability are a\n> consequence of making it easy for reviewers to spot where the code is\n> incorrect.\n>\n> And correctness is not covered by \"the submitter tested this\". Correctness\n> includes all the corner cases, where the \"many eyes make bugs shallow\"\n> really shines.\n>\n> I'd rather have reviewers find bugs than users.\n\nI'd rather have submitters find bugs than reviewers.\n\n> I will *never* be a fan of a review process that pushes correctness to a\n> back seat (yes, it is much harder than spotting typos or lines longer than\n> 80 columns per row, but the ultimate goal is to deliver value to the end\n> user, not to make life easy for the maintainer).\n\nDid I ever say correctness is pushed to a back seat?\n\nI said that it is easier to spot correctness issues for you as a\nsubmitter than other kinds of issues without outside help (and\nimplied that if you are a diligent contributor, you should aim for,\nand you should be able to achieve, a patch series where correctness\nissues do not need to be pointed out). But other higher level\nissues are harder for any submitter to spot (regardless of\nexperience and competence of the submitter), because one gets so\nmarried to one's own code, design and worldview. And that is why\n\"review is primarily to spot bugs\" can never be a correct viewpoint.\n\nA reviewee needs to be prepared to accept review comments on higher\nlevel issues, even more readily than comments on correctness issues,\nbecause it is too easy to be constrained by early decisions one has\nalready made while preparing a patch series and become blind to\nbigger picture after staring one's own new code for number of hours.\nHigher level issues can be more easily spotted by reviewers, whose\neyes are still fresh to the series.\n"},{"id":"310676","messageId":"alpine.DEB.2.20.1702020005570.3496@virtualbox","threadId":"44806","inReplyTo":"xmqqvastmt09.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-01T23:07:32Z","receivedAt":"2017-02-01T23:08:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 1 Feb 2017, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Johannes Schindelin writes:\n> >\n> >> That leaves the \"putty\" case in handle_ssh_variant(), does it not? Was it\n> >> not your specific objection that that is the case?\n> >\n> > Yup, you can remove that while you reroll.\n> >\n> >> No worries, I will let this simmer for a while. Your fixup has a lot of\n> >> duplicated code (so much for maintainability as an important goal... ;-))\n> >> and I will have to think about it. My immediate thinking is to *not*\n> >> duplicate code,...\n> >\n> > You need to realize that the namespaces of the configuration and the\n> > command names are distinct. There is no code duplication.\n> \n> To explain this a bit, there is no reason why allowed values for\n> SSH_VARIANT must be \"putty\" and \"tortoiseplink\". An alternative\n> design could be \"port_option=-p,needs_batch=yes\" and it may be more\n> logical and futureproof if a variant of tortoiseplink decides to use\n> \"-p\" instead of \"-P\" and still require \"-batch\".\n> \n> Prematurely attempting to share code, only because the current\n> vocabularies for two distinct concepts happen to overlap, is not\n> de-duplicating the code for maintainability. It is adding\n> unnecessary work other people need to do in the future when they\n> want to extend the system.\n\nExcept, of course, that your hypothetical port_option and needs_batch\nsettings would be handled at a different point altogether.\n\nI sense very strongly that this discussion has taken a very emotional\nturn, which is detrimental to the quality. So let's take a break here.\n\nCiao,\nJohannes\n"}]}