git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jan 9, 2017, 14:19 UTC
Message-ID
<xmqqmvf0wc2h.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<alpine.DEB.2.20.1701091207480.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 5 quoted lines
>> > 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.

Show 16 quoted lines
> -- 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);
Previous: Johannes SchindelinNext: Johannes Schindelin
Message 7 of 38 in “connect: handle putty/plink also in GIT_SSH_COMMAND”
  1. connect: handle putty/plink also in GIT_SSH_COMMANDJohannes Schindelin, Jan 2, 2017
  2. Junio C HamanoJan 8, 2017
  3. Junio C HamanoJan 9, 2017
  4. Johannes SchindelinJan 9, 2017
  5. Junio C HamanoJan 9, 2017
  6. Johannes SchindelinJan 9, 2017
  7. Junio C HamanoJan 9, 2017
  8. Johannes SchindelinJan 25, 2017
  9. Junio C HamanoJan 25, 2017
  10. Junio C HamanoJan 25, 2017
  11. Johannes SchindelinJan 26, 2017
  12. Jeff KingJan 25, 2017
  13. Junio C HamanoJan 25, 2017
  14. Johannes SchindelinJan 26, 2017
  15. 0/3 Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMANDJohannes Schindelin, Jan 26, 2017
  16. 1/3 connect: handle putty/plink also in GIT_SSH_COMMANDJohannes Schindelin, Jan 26, 2017
  17. 2/3 connect: rename tortoiseplink and putty variablesJohannes Schindelin, Jan 26, 2017
  18. 3/3 connect: Add the envvar GIT_SSH_VARIANT and ssh.variant configJohannes Schindelin, Jan 26, 2017
  19. Junio C HamanoJan 26, 2017
  20. Johannes SchindelinJan 27, 2017
  21. Junio C HamanoJan 27, 2017
  22. Johannes SchindelinFeb 1, 2017
  23. Junio C HamanoFeb 1, 2017
  24. 0/4 Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMANDJohannes Schindelin, Feb 1, 2017
  25. 1/4 connect: handle putty/plink also in GIT_SSH_COMMANDJohannes Schindelin, Feb 1, 2017
  26. 2/4 connect: rename tortoiseplink and putty variablesJohannes Schindelin, Feb 1, 2017
  27. 3/4 git_connect(): factor out SSH variant handlingJohannes Schindelin, Feb 1, 2017
  28. 4/4 connect: Add the envvar GIT_SSH_VARIANT and ssh.variant configJohannes Schindelin, Feb 1, 2017
  29. Junio C HamanoFeb 1, 2017
  30. Junio C HamanoFeb 1, 2017
  31. Johannes SchindelinFeb 1, 2017
  32. Junio C HamanoFeb 1, 2017
  33. Johannes SchindelinFeb 1, 2017
  34. Junio C HamanoFeb 1, 2017
  35. Johannes SchindelinFeb 1, 2017
  36. Junio C HamanoFeb 1, 2017
  37. Johannes SchindelinFeb 1, 2017
  38. Junio C HamanoFeb 1, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.