Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > 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, &ssh_argv))
> +				if (split_cmdline(split_ssh, &ssh_argv) &&
> +				    !strchr(ssh_argv[0], '='))
>  					ssh_argv0 = xstrdup(ssh_argv[0]);
>  				free(split_ssh);
>  				free((void *)ssh_argv);