Re: [PATCH v2 2/2] connect: improve check for plink to reduce false positives

["Torsten Bögershausen" <tboegi@web.de>]

On 2015-04-25 00.28, brian m. carlson wrote:
> The git_connect function has code to handle plink and tortoiseplink
> specially, as they require different command line arguments from
> OpenSSH.  However, the match was done by checking for "plink"
> case-insensitively in the string, which led to false positives when
> GIT_SSH contained "uplink".  Improve the check by looking for "plink" or
> "tortoiseplink" (or those names suffixed with ".exe") in the final
> component of the path.
> 
(I'm not sute if the commit message describes the problem deep enough
for readers which are not familar with all the details of the original
report):
A feature implemented for Windows may break things for e.g. Linux users)
The following may read exaggerated, so please read it as a suggestion.

The git_connect function has code to handle plink and tortoiseplink
specially, as they require different command line arguments compared to
OpenSSH (-P instead of -p, tortoiseplink uses -batch), commit 36ad53ffee6ed.
The special handling is only needed for Windows, and a sloppy
case-insensitve search for "plink" will trigger that an the extra
parameter "-batch" is added to the command line.

This was observed on a Linux system where a command line including
"/uplink_deploy/" was used.

There are different ways to improve the situation:
(The following mentions only plink, but assumes that tortoiseplink is handled
 similar)
a) Disable the plink/tortoiseplink special handling on non-Windows systems
b) Tighten the search for plink:
   Allow basename() == plink || !!strcasestr(ssh, "plink.exe")
c) Tighten the search for plink:
   Allow basename() == plink || !!strcasestr(ssh, "plink.exe")
d) Tighten the check for tortoiseplink.
   Today we set "int putty" to true when plink is found, and -batch
   is set when tortoiseplink is not found.
   This fixes the reported bug, but still has the -P problem.
e) Unix users typically use shell scripts and could use GIT_SSH_COMMAND.
   Declare the GIT_SSH as not-well-documented (and to obsolete ?) for non-Windows systems,


This patch implements c):
Extract the basename and compare it to plink, plink.exe respective
tortoiseplink/tortoiseplink.exe

Note that there is a slight risk of breakage for Windows users:
Strings like "myplink" or "plink-0.83" are no longer accepted.

-------------
I would probably vote for a), as Unix/Linux/Mac OS users don't use plink/tortoiseplink
at all. 
-------------
What about adding test-cases in t5601,
this will ease the documentation later.
f:/util/plink
/c/util/plink.exe
f:/util/tortoiseplink
/c/util/tortoiseplink.exe
/usr/local/uplink/sshwrapper.sh


Other opinions, other thoughts ?