Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
Hi Junio, On Wed, 25 Jan 2017, Junio C Hamano wrote: > Subject: [PATCH] connect: rename tortoiseplink and putty variables > > One of these two may have originally been named after "what exact > SSH implementation do we have" so that we can tweak the command line > options, but these days "putty=1" no longer means "We are using the > plink SSH implementation that comes with PuTTY". It is set when we > guess that either PuTTY plink or Tortoiseplink is in use. > > Rename them after what effect is desired. The current "putty" > option is about using "-P " when OpenSSH would use "-p ", > so rename it to port_option whose value is either 'p' or 'P". The > other one is about passing an extra command line option "-batch", > so rename it needs_batch. > > Signed-off-by: Junio C HamanoApart from an overly-long line, this patch looks good. It made my life a little harder, as I had to rebase Segev's ssh.variant (why this should be a core.* variable is not clear to me) patch and it caused conflicts. I resolved those conflicts and made both patches part of this patch series. Will contribute v2 as soon as the test suite passes, Johannes
Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
Hi Junio, On Wed, 25 Jan 2017, Junio C Hamano wrote: > Johannes Schindelinwrites: > > > Now, with the patch in question (without the follow-up, which I would > > like to ask you to ignore, just like you did so far), Git would not > > figure out that your script calls PuTTY eventually. The work-around? > > Easy: > > > > DUMMY=/plink.exe /path/to/junio-is-a-superstar.sh > > Think about how you would explain that to an end-user in our document? > You'll need to explain how exactly the auto-detection works, so that the > user can "exploit" the loophole to do that. And what maintenance burden > does it add when auto-detection is updated? Fine, you do not like it. Saying so (instead of asking me questions) would have been helpful. > I think I know you well enough that you know well enough that it is too > ugly to live, and I suspect that the above is a tongue-in-cheek "arguing > for the sake of argument" and would not need a serious response, but > just in case... It was not tongue-in-cheek, I was being serious. > Yes. Here is what comes on an obvious clean-up patch (which will be > sent as a follow-up to this message). I'd much rather prefer https://github.com/git-for-windows/git/pull/1030 than your patch. Ciao, Johannes
Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
Jeff Kingwrites: > On Wed, Jan 25, 2017 at 02:35:36PM -0800, Junio C Hamano wrote: > >> -- >8 -- >> Subject: [PATCH] connect: core.sshvariant to correct misidentification > > I have been watching this discussion from the sidelines, and I agree > that this direction is a big improvement. > ... > IIRC, the "const" in git_config_get_string_const is only about avoiding > an annoying cast. The result is still allocated and needs freed. Since > you are not keeping the value after the function returns, I think you > could just use git_config_get_value(). It is too late for today's pushout (I have this topic near the tip of 'pu', so it is possible to tweak and redo only 'pu' branch, but I generally hate tweaking things after 15:00 my time), but I'll fix that before the topic hits 'jch' (which is a bit more than 'next' and that is what I use for everyday work). Thanks.
Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
On Wed, Jan 25, 2017 at 02:35:36PM -0800, Junio C Hamano wrote: > -- >8 -- > Subject: [PATCH] connect: core.sshvariant to correct misidentification I have been watching this discussion from the sidelines, and I agree that this direction is a big improvement. > +static void override_ssh_variant(int *port_option, int *needs_batch) > +{ > + const char *variant; > + > + if (git_config_get_string_const("core.sshvariant", )) > + return; > + if (!strcmp(variant, "tortoiseplink")) { > + *port_option = 'P'; > + *needs_batch = 1; > + } else if (!strcmp(variant, "putty")) { > + *port_option = 'P'; > + *needs_batch = 0; > + } else { > + /* default */ > + if (strcmp(variant, "ssh")) { > + warning(_("core.sshvariant: unknown value '%s'"), > variant); > + warning(_("using OpenSSH compatible behaviour")); > + } > + *port_option = 'p'; > + *needs_batch = 0; > + } > +} IIRC, the "const" in git_config_get_string_const is only about avoiding an annoying cast. The result is still allocated and needs freed. Since you are not keeping the value after the function returns, I think you could just use git_config_get_value(). (Grepping around, I see a few other places that seem to make the same mistake. I think this is a confusing interface that should probably be fixed). -Peff
Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
Junio C Hamanowrites: > Yes. Here is what comes on an obvious clean-up patch (which will be > sent as a follow-up to this message). ... and this is the "obvious clean-up patch" to apply directly on top of Segev's GIT_SSH_COMMAND support, which comes before the previous one. -- >8 -- Subject: [PATCH] connect: rename tortoiseplink and putty variables One of these two may have originally been named after "what exact SSH implementation do we have" so that we can tweak the command line options, but these days "putty=1" no longer means "We are using the plink SSH implementation that comes with PuTTY". It is set when we guess that either PuTTY plink or Tortoiseplink is in use. Rename them after what effect is desired. The current "putty" option is about using "-P " when OpenSSH would use "-p ", so rename it to port_option whose value is either 'p' or 'P". The other one is about passing an extra command line option "-batch", so rename it needs_batch. Signed-off-by: Junio C Hamano --- connect.c | 22 -- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/connect.c b/connect.c index c81f77001b..aa51b33bfc 100644 --- a/connect.c +++ b/connect.c @@ -769,7 +769,8 @@ struct child_process *git_connect(int fd[2], const char *url, conn->in = conn->out = -1; if (protocol == PROTO_SSH) { const char *ssh; - int putty = 0, tortoiseplink = 0; + int needs_batch = 0; + int port_option = 'p'; char *ssh_host = hostandport; const char *port = NULL; char *ssh_argv0 = NULL; @@ -819,12 +820,14 @@ struct child_process *git_connect(int fd[2], const char *url, if (ssh_argv0) { const char *base = basename(ssh_argv0); - tortoiseplink = !strcasecmp(base, "tortoiseplink") || - !strcasecmp(base, "tortoiseplink.exe"); - putty = tortoiseplink || - !strcasecmp(base, "plink") || - !strcasecmp(base, "plink.exe"); - + if (!strcasecmp(base, "tortoiseplink") || + !strcasecmp(base, "tortoiseplink.exe")) { + port_option = 'P'; + needs_batch = 1; + } else if (!strcasecmp(base, "plink") || + !strcasecmp(base, "plink.exe")) { + port_option = 'P'; + } free(ssh_argv0); } @@ -833,11 +836,10 @@ struct child_process *git_connect(int fd[2], const char *url, argv_array_push(>args, "-4"); else if (flags & CONNECT_IPV6) argv_array_push(>args, "-6"); - if (tortoiseplink) + if (needs_batch) argv_array_push(>args, "-batch"); if (port) { - /* P is for PuTTY, p is for OpenSSH */ - argv_array_push(>args, putty ? "-P" : "-p"); + argv_array_pushf(>args, "-%c", port_option); argv_array_push(>args, port); } argv_array_push(>args, ssh_host); -- 2.11.0-699-ga1f1475476
Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
Johannes Schindelinwrites: > Now, with the patch in question (without the follow-up, which I would like > to ask you to ignore, just like you did so far), Git would not figure out > that your script calls PuTTY eventually. The work-around? Easy: > > DUMMY=/plink.exe /path/to/junio-is-a-superstar.sh Think about how you would explain that to an end-user in our document? You'll need to explain how exactly the auto-detection works, so that the user can "exploit" the loophole to do that. And what maintenance burden does it add when auto-detection is updated? I think I know you well enough that you know well enough that it is too ugly to live, and I suspect that the above is a tongue-in-cheek "arguing for the sake of argument" and would not need a serious response, but just in case... > ... > Of course, given our discussion I think all of this should be documented > in the commit message as well as in the man page. Yes. Here is what comes on an obvious clean-up patch (which will be sent as a follow-up to this message). -- >8 -- Subject: [PATCH] connect: core.sshvariant to correct misidentification While the logic we have been using to guess which variant of SSH implementation is in use by looking at the name of the program has been robust enough for GIT_SSH (which does not go through the shell, hence it can only spell the name the SSH program and nothing else), extending it to GIT_SSH_COMMAND and core.sshcommand opens it for larger chance of misidentification, because these can specify arbitrary shell command, and a simple "the first word on the line must be the command name" heuristic may not even look at the command name at all. As any effort to improve the heuristic will give us only diminishing returns, and especially given that most of the users are expected to have a command line for which the heuristic would work correctly, give an explicit escape hatch to override a misidentification, just in case one happens. It is arguably bad to add this new variable to the core.* set, in the sense that you'll never see this variable if you do not interact with other repositories over ssh, but core.sshcommand is already there for some reason, so let's match it. Signed-off-by: Junio C Hamano --- Documentation/config.txt | 13 + connect.c| 26 ++ 2 files changed, 39 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index af2ae4cc02..8ea1db49c6 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -455,6 +455,19 @@ core.sshCommand:: the `GIT_SSH_COMMAND` environment variable and is overridden when the environment variable is set. +core.sshVariant:: + SSH implementations used by Git when running `git fetch`, + `git clone`, and `git push` use slightly different command + line options (e.g. PuTTY and TortoisePlink use `-P ` + while OpenSSH uses `-p ` to specify the port number. + TortoisePlink in addition requires `-batch` option to be + passed). Git guesses which variant is in use correctly from + the name of the ssh command used (see `core.sshCommand` + configuration variable and also `GIT_SSH_COMMAND` + environment variable) most of the time. You can set this + variable to 'putty', 'tortoiseplink', or 'ssh' to correct it + when Git makes an incorrect guess. + core.ignoreStat:: If true, Git will avoid using lstat() calls to detect if files have changed by setting the "assume-unchanged" bit for those tracked files diff --git a/connect.c b/connect.c index aa51b33bfc..358e9c06f0 100644 --- a/connect.c +++ b/connect.c @@ -691,6 +691,29 @@ static const char *get_ssh_command(void) return NULL; } +static void override_ssh_variant(int *port_option, int *needs_batch) +{ + const char *variant; + + if (git_config_get_string_const("core.sshvariant", )) + return; + if (!strcmp(variant, "tortoiseplink")) { + *port_option = 'P'; + *needs_batch = 1; + } else if (!strcmp(variant, "putty")) { + *port_option = 'P'; + *needs_batch = 0; + } else { + /* default */ + if (strcmp(variant, "ssh")) { + warning(_("core.sshvariant: unknown value '%s'"), variant); + warning(_("using OpenSSH compatible behaviour")); + } + *port_option = 'p'; + *needs_batch = 0; + } +} + /* * This returns a dummy child_process if the transport protocol does not * need fork(2), or a struct child_process object if it does. Once done, @@ -836,6 +859,9 @@ struct child_process *git_connect(int fd[2], const char *url, argv_array_push(>args, "-4"); else if (flags & CONNECT_IPV6)
Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
Hi Junio, On Mon, 9 Jan 2017, Junio C Hamano wrote: > IOW, "give an escape hatch to override auto-detected tortoiseplink and > plink variables" suggestion should be taken as an "in addition to" > suggestion (as opposed to an "instead of" suggestion). I was not clear > in my very first message, and I thought in my second and my third > message I clarified it as such, but probably I wasn't explicit enough. While you may not have been explicit enough, I was not smart enough. That escape-hatch exists *already*. And it is exactly the thing that you mentioned earlier as a potential source of mis-identification. Let's assume that you want to use a script for the GIT_SSH_COMMAND that eventually uses PuTTY, and you call this script "junio-is-a-superstar.sh". All plausible so far ;-) Now, with the patch in question (without the follow-up, which I would like to ask you to ignore, just like you did so far), Git would not figure out that your script calls PuTTY eventually. The work-around? Easy: DUMMY=/plink.exe /path/to/junio-is-a-superstar.sh In other words: the thing that we thought may be a problem is actually a feature. Likewise, if your GIT_SSH_COMMAND looks like this: THIS_IS_NOT_ACTUALLY_PUTTY=/my/window/needs/putty my-ssh.sh ... you can easily change Git's mind by prefixing DUMMY=blubber If you want to use a script that decides itself whether to call OpenSSH or PuTTY, Git should "stay dumb" about it, as it will not be able to tell beforehand whether a port argument should be passed via `-p` or `-P` anyway. Of course, given our discussion I think all of this should be documented in the commit message as well as in the man page. Do you agree this is a good direction to take? Ciao, Johannes
Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
Johannes Schindelinwrites: >> > But do we really need to do that? >> >> No. > > Exactly. > But since you seem to convinced that a future bug report like this may > happen (I am not, and it contradicts my conviction that one should cross a > bridge only when reaching it, but whatever), how about this, on top: I do not understand why you keep arguing. With No-Exactly exchange, I thought that we established that * The original auto-detection for GIT_SSH is battle tested and has worked for us very well. It may probably have covered 99.9% of the population, and we haven't heard complaints from the remaining 0.1%. * The added support for GIT_SSH_COMMAND, because the mechanism gives more lattitude to end-users to be creative, will have lower chance of correctly deciding between ssh, tortoiseplink and plink, but it would still be high enough, say 99%, correct detection rate. * It is foolish to add more code to refine the auto-detection to raise 99% to 99.5%. We know that the approach fundamentally cannot make the detection rate reach 100%. The last one can be paraphrased as "perfect is the enemy of good". But that does not mean that it is OK to leave the system unusable for 1% of the users. If the auto-detection code does not work correctly for a tiny minority of users, it is still OK, as long as we give them an escape hatch to allow them to say "You may not be able to guess among ssh, tortoiseplink and plink, or you may even guess incorrectly, so I'll tell you. I am using plink". 99% of users do not have to know about that escape hatch, but 1% of users will be helped. IOW, "give an escape hatch to override auto-detected tortoiseplink and plink variables" suggestion should be taken as an "in addition to" suggestion (as opposed to an "instead of" suggestion). I was not clear in my very first message, and I thought in my second and my third message I clarified it as such, but probably I wasn't explicit enough. In any case, "do this only if the first one token on the command line does not have '='" we see below is flawed for two reasons and I think it would not be a workable counter-proposal (besides, it goes against the "perfect is the enemy of good" mantra). For one thing, the command line may not be "VAR=VAL cmd", but just "cmd" that names the path to tortoiseplink, but the leading directory path that houses tortoiseplink may have a '=' in it, in which case we would detect it correctly if we didn't have such a hack, but with the hack we would punt. More importantly, even when "VAR=VAL cmd" form was caught with the change, it may punt and stop guessing, but punting alone does not help users by letting them say "I know you cannot auto-detect, so let me help you---what I have is PuTTY plink". If it always assumes that the user uses the plain vanilla ssh, then the change merely changed what incorrect guess the auto-detection makes from "tortoiseplink" to "vanilla ssh" for a user who uses the PuTTY variant of plink. > -- snipsnap -- > diff --git a/connect.c b/connect.c > index c81f77001b..b990dd6190 100644 > --- a/connect.c > +++ b/connect.c > @@ -797,7 +797,8 @@ struct child_process *git_connect(int fd[2], const > char *url, > char *split_ssh = xstrdup(ssh); > const char **ssh_argv; > > - if (split_cmdline(split_ssh, _argv)) > + if (split_cmdline(split_ssh, _argv) && > + !strchr(ssh_argv[0], '=')) > ssh_argv0 = xstrdup(ssh_argv[0]); > free(split_ssh); > free((void *)ssh_argv);
Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
Hi Junio, On Mon, 9 Jan 2017, Junio C Hamano wrote: > Johannes Schindelinwrites: > > > If you feel strongly about your contrived examples possibly being > > affected by this patch, we could easily make this conditional on > > > > 1) no '&&' or '||' being found on the command-line, and > > 2) argv[0] not containing an '=' > > > > Another approach would be to verify that argv[i] starts with '-' for > > non-zero i. > > > > But do we really need to do that? > > No. Exactly. > > That means that the user has to specify something like > > > > HAHAHA_IT_IS_NOT=/plink.exe ssh > > > > as GIT_SSH_COMMAND. > > My second message was to clarify that "VAR1=VAL2 command" is NOT a > contrived example, and this response indicates that I somehow failed > to convey that to you. Indeed. The quite contrived example was about a script that chooses between plink and tortoiseplink (and fails to call anything else). And it failed to convince me. But since you seem to convinced that a future bug report like this may happen (I am not, and it contradicts my conviction that one should cross a bridge only when reaching it, but whatever), how about this, on top: -- snipsnap -- diff --git a/connect.c b/connect.c index c81f77001b..b990dd6190 100644 --- a/connect.c +++ b/connect.c @@ -797,7 +797,8 @@ struct child_process *git_connect(int fd[2], const char *url, char *split_ssh = xstrdup(ssh); const char **ssh_argv; - if (split_cmdline(split_ssh, _argv)) + if (split_cmdline(split_ssh, _argv) && + !strchr(ssh_argv[0], '=')) ssh_argv0 = xstrdup(ssh_argv[0]); free(split_ssh); free((void *)ssh_argv);
Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
Johannes Schindelinwrites: > If you feel strongly about your contrived examples possibly being > affected by this patch, we could easily make this conditional on > > 1) no '&&' or '||' being found on the command-line, and > 2) argv[0] not containing an '=' > > Another approach would be to verify that argv[i] starts with '-' for > non-zero i. > > But do we really need to do that? No. An explicit way to override an incorrect guess is sufficient and necessary. The above two-item list you gave will be just part of the machinery to make an incorrect guess. The auto-detection in the posted patch should cover many users' use case and I do not think it needs to be extended further to make it more brittle, as by definition its guess cannot be perfect. Just keep it simple and give a separate escape hatch. > That means that the user has to specify something like > > HAHAHA_IT_IS_NOT=/plink.exe ssh > > as GIT_SSH_COMMAND. My second message was to clarify that "VAR1=VAL2 command" is NOT a contrived example, and this response indicates that I somehow failed to convey that to you. The "if tortoiseplink exists (and the end user can override the location with an environment), use that, and if PuTTY plink exists (ditto), use that instead" in a "myssh" script, and use it as core.sshcommand with the environment to override my custom installation location to these two programs, would be what I would do when I get two Windows machines, with these variants of SSH on each. So take the second message as a bug report against the version of Git for Windows you ship with the patch in question. The auto-detection may work for many people and that is a great thing. I failed to say that in my message, as I thought that was obvious. But it is important to plan to cope with the case where it does not work. The usual practice around here is to say "the it may not necessarily work for everybody, so lets be prepared to add an explicit override if it turns out to be necessary". The second message, which you are responding to, was meant to be a bug report from the future, telling us that an override is needed, showing that we do not have to wait for a bug report to act on.
Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
Hi Junio, On Sun, 8 Jan 2017, Junio C Hamano wrote: > Junio C Hamanowrites: > > > I suspect that this will break when GIT_SSH_COMMAND, which is meant > > to be processed by the shell, hence the user can write anything, > > begins with a one-shot environment variable assignment, e.g. > > > > [core] sshcommand = VAR1=VAL1 VAR2=VAL2 //path/to/tortoiseplink > > > > One possible unintended side effect of this patch is when VAL1 ends > > with /plink (or /tortoiseplink) and the command is not either of > > these, in which case the "tortoiseplink" and "putty" variables will > > tweak the command line for an SSH implementation that does not want > > such a tweak to be made. There may be other unintended fallouts. > > Thinking about this further, as the sshcommand (or GIT_SSH_COMMAND) > interface takes any shell-interpretable commands, the first "token" > you see is not limited to a one-shot environment assignment. It > could even be "cmd1 && cmd2 && ..." or even: > > if test -f ${TPLINK:=//path/to/tortoiseplink} > then > exec "$TPLINK" "$@" > elif test -f ${PLINK:=//path/to/plink} > exec "$PLINK" "$@" > else > echo >&2 no usable ssh on this host > fi Sure, it could be all of that. It could even blast through the environment limits in some environments on some platforms, but not on others. It could automatically transfer bitcoins whenever a connection to github.com is attempted. It could start the camera and build a time-lapse of "every time I pushed or fetched a repository", as an art project. It could shut down the computer. It could do all of that. In practice, however, what users realistically do is to use GIT_SSH_COMMAND whenever they need to override the ssh command with options. This is the common case, and that is what we must make less cumbersome to use. If you feel strongly about your contrived examples possibly being affected by this patch, we could easily make this conditional on 1) no '&&' or '||' being found on the command-line, and 2) argv[0] not containing an '=' Another approach would be to verify that argv[i] starts with '-' for non-zero i. But do we really need to do that? Let's have a very brief look at the amount of work required to come up with a false positive (I am not so much concerned about any power user writing elaborate shell expressions that may call plink.exe: those power users will simply have to continue to use their own -P => -p and -batch hacks): The logic kicks in if the split command-line's first component has a basename `plink` or `tortoiseplink` (possibly with `.exe` suffix). The only way this logic can kick in by mistake is if the first component is *not* the command to call *and* if the first component *still* has a basename of `plink` or `tortoiseplink`. That means that the user has to specify something like HAHAHA_IT_IS_NOT=/plink.exe ssh as GIT_SSH_COMMAND. Now, let's take a *very* brief look at the question how likely it is to have a basename of `plink` or `tortoiseplink`. Remember, there are two ways that the basename can be a specific term: either the original string is already equal to that term, or it ends in a slash followed by the term. That is, either the first component refers to plink.exe/tortoiseplink.exe, i.e. it would be a *true* positive. Or the first component would end in "/plink" or "/tortoiseplink" (possibly with the `.exe` suffix) *and not be a command*. But how likely is it to specify a non-command (such as a per-process variable assignment) that specifically mentions plink or tortoiseplink? Is it even realistic to expect users to specify values for GIT_SSH_COMMAND that contain "plink" as a substring when they do *not* want to call plink at all? After thinking a bit longer than I had originally planned about it, my answer is: no. No, I do not think it is realistic. I fully expect *no* user to have a GIT_SSH_COMMAND containing even a substring "plink" *anywhere*, unless they do, in fact, want to call plink or tortoiseplink. My main aim with Git for Windows is to improve the user experience, and the patch in question does do that. Of course, I also try to avoid breaking existing setups while improving the user experience, and I believe that it is safe to assume that the patch in question in all likelihood does not break any existing setup. Ciao, Dscho
Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
Junio C Hamanowrites: > I suspect that this will break when GIT_SSH_COMMAND, which is meant > to be processed by the shell, hence the user can write anything, > begins with a one-shot environment variable assignment, e.g. > > [core] sshcommand = VAR1=VAL1 VAR2=VAL2 //path/to/tortoiseplink > > One possible unintended side effect of this patch is when VAL1 ends > with /plink (or /tortoiseplink) and the command is not either of > these, in which case the "tortoiseplink" and "putty" variables will > tweak the command line for an SSH implementation that does not want > such a tweak to be made. There may be other unintended fallouts. Thinking about this further, as the sshcommand (or GIT_SSH_COMMAND) interface takes any shell-interpretable commands, the first "token" you see is not limited to a one-shot environment assignment. It could even be "cmd1 && cmd2 && ..." or even: if test -f ${TPLINK:=//path/to/tortoiseplink} then exec "$TPLINK" "$@" elif test -f ${PLINK:=//path/to/plink} exec "$PLINK" "$@" else echo >&2 no usable ssh on this host fi Worse, the above may be in "myssh" script found on user's PATH and then core.sshcommand may be set to TPLINK=//my/path/to/tortoiseplink PLINK=//my/path/to/plink myssh in the configuration the user uses on multiple hosts, some have tortoiseplink installed and some do not. The idea the user who set it up may be to use whatever available depending on the host. The posted patch would be confused that we are using tortoiseplink but the "myssh" script would correctly notice that on a particular host it is unavailable and choose to use plink instead. > Sorry, no concrete suggestion to get this to work comes to my mind > offhand. > > Perhaps give an explicit way to force "tortoiseplink" and "putty" > variables without looking at and guessing from the pathname, so that > the solution does not have to split and peek the command line? A user with such an elaborate set-up with multiple hosts with different configurations would likely to want to say "Just give me a regular SSH command line, and in my 'myssh' script I'll futz with -p/-P differences and such, as I'm writing a small script to cope with Tortoiseplink and Puttyplink anyway". With a separate, explicit configuration variable to tell Git what variant of SSH you have, even such a user can be served (she would just set ssh.variant to "vanilla" or whatever that is not "tortoiseplink" or "plink"). .
Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND
Johannes Schindelinwrites: > + if (ssh) { > + char *split_ssh = xstrdup(ssh); > + const char **ssh_argv; > + > + if (split_cmdline(split_ssh, _argv)) > + ssh_argv0 = xstrdup(ssh_argv[0]); > + free(split_ssh); > + free((void *)ssh_argv); > + } else { > /* >* GIT_SSH is the no-shell version of >* GIT_SSH_COMMAND (and must remain so for > @@ -807,8 +813,11 @@ struct child_process *git_connect(int fd[2], const char > *url, > if (!ssh) > ssh = "ssh"; > > - ssh_dup = xstrdup(ssh); > - base = basename(ssh_dup); > + ssh_argv0 = xstrdup(ssh); > + } > + > + if (ssh_argv0) { > + const char *base = basename(ssh_argv0); > > tortoiseplink = !strcasecmp(base, > "tortoiseplink") || > !strcasecmp(base, "tortoiseplink.exe"); I suspect that this will break when GIT_SSH_COMMAND, which is meant to be processed by the shell, hence the user can write anything, begins with a one-shot environment variable assignment, e.g. [core] sshcommand = VAR1=VAL1 VAR2=VAL2 //path/to/tortoiseplink One possible unintended side effect of this patch is when VAL1 ends with /plink (or /tortoiseplink) and the command is not either of these, in which case the "tortoiseplink" and "putty" variables will tweak the command line for an SSH implementation that does not want such a tweak to be made. There may be other unintended fallouts. Sorry, no concrete suggestion to get this to work comes to my mind offhand. Perhaps give an explicit way to force "tortoiseplink" and "putty" variables without looking at and guessing from the pathname, so that the solution does not have to split and peek the command line? connect.c | 20 1 file changed, 20 insertions(+) diff --git a/connect.c b/connect.c index 8cb93b0720..1768122456 100644 --- a/connect.c +++ b/connect.c @@ -691,6 +691,23 @@ static const char *get_ssh_command(void) return NULL; } +static void get_ssh_variant(int *tortoiseplink, int *putty) +{ + const char *variant; + if (!git_config_get_string_const("ssh.variant", )) + return; + if (!strcmp(variant, "tortoiseplink")) { + *tortoiseplink = 1; + *putty = 1; + } else if (!strcmp(variant, "putty")) { + *tortoiseplink = 0; + *putty = 1; + } else { + *tortoiseplink = 0; + *putty = 0; + } +} + /* * This returns a dummy child_process if the transport protocol does not * need fork(2), or a struct child_process object if it does. Once done, @@ -824,6 +841,9 @@ struct child_process *git_connect(int fd[2], const char *url, argv_array_push(>args, "-4"); else if (flags & CONNECT_IPV6) argv_array_push(>args, "-6"); + + get_ssh_variant(, ); + if (tortoiseplink) argv_array_push(>args, "-batch"); if (port) {