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

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

[Johannes Schindelin]

From: Segev Finer <segev208@gmail.com>

Git for Windows has special support for the popular SSH client PuTTY:
when using PuTTY's non-interactive version ("plink.exe"), we use the -P
option to specify the port rather than OpenSSH's -p option. TortoiseGit
ships with its own, forked version of plink.exe, that adds support for
the -batch option, and for good measure we special-case that, too.

However, this special-casing of PuTTY only covers the case where the
user overrides the SSH command via the environment variable GIT_SSH
(which allows specifying the name of the executable), not
GIT_SSH_COMMAND (which allows specifying a full command, including
additional command-line options).

When users want to pass any additional arguments to (Tortoise-)Plink,
such as setting a private key, they are required to either use a shell
script named plink or tortoiseplink or duplicate the logic that is
already in Git for passing the correct style of command line arguments,
which can be difficult, error prone and annoying to get right.

This patch simply reuses the existing logic and expands it to cover
GIT_SSH_COMMAND, too.

Note: it may look a little heavy-handed to duplicate the entire
command-line and then split it, only to extract the name of the
executable. However, this is not a performance-critical code path, and
the code is much more readable this way.

Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/putty-w-args-v1
Fetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v1


	Original Pull Request:
	https://github.com/git-for-windows/git/pull/1006

 connect.c        | 23 ++++++++++++++++-------
 t/t5601-clone.sh | 15 +++++++++++++++
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/connect.c b/connect.c
index 8cb93b0720..c81f77001b 100644
--- a/connect.c
+++ b/connect.c
@@ -772,6 +772,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 			int putty = 0, tortoiseplink = 0;
 			char *ssh_host = hostandport;
 			const char *port = NULL;
+			char *ssh_argv0 = NULL;
 			transport_check_allowed("ssh");
 			get_host_and_port(&ssh_host, &port);
 
@@ -792,10 +793,15 @@ struct child_process *git_connect(int fd[2], const char *url,
 			}
 
 			ssh = get_ssh_command();
-			if (!ssh) {
-				const char *base;
-				char *ssh_dup;
-
+			if (ssh) {
+				char *split_ssh = xstrdup(ssh);
+				const char **ssh_argv;
+
+				if (split_cmdline(split_ssh, &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");
@@ -816,7 +825,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 					!strcasecmp(base, "plink") ||
 					!strcasecmp(base, "plink.exe");
 
-				free(ssh_dup);
+				free(ssh_argv0);
 			}
 
 			argv_array_push(&conn->args, ssh);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index a433394200..5b228e2675 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -386,6 +386,21 @@ test_expect_success 'tortoiseplink is like putty, with extra arguments' '
 	expect_ssh "-batch -P 123" myhost src
 '
 
+test_expect_success 'double quoted plink.exe in GIT_SSH_COMMAND' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+	GIT_SSH_COMMAND="\"$TRASH_DIRECTORY/plink.exe\" -v" \
+		git clone "[myhost:123]:src" ssh-bracket-clone-plink-3 &&
+	expect_ssh "-v -P 123" myhost src
+'
+
+SQ="'"
+test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+	GIT_SSH_COMMAND="$SQ$TRASH_DIRECTORY/plink.exe$SQ -v" \
+		git clone "[myhost:123]:src" ssh-bracket-clone-plink-4 &&
+	expect_ssh "-v -P 123" myhost src
+'
+
 # Reset the GIT_SSH environment variable for clone tests.
 setup_ssh_wrapper
 

base-commit: e05806da9ec4aff8adfed142ab2a2b3b02e33c8c
-- 
2.11.0.rc3.windows.1

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

[Junio C Hamano]

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

> +			if (ssh) {
> +				char *split_ssh = xstrdup(ssh);
> +				const char **ssh_argv;
> +
> +				if (split_cmdline(split_ssh, &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", &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(&conn->args, "-4");
 			else if (flags & CONNECT_IPV6)
 				argv_array_push(&conn->args, "-6");
+
+			get_ssh_variant(&tortoiseplink, &putty);
+
 			if (tortoiseplink)
 				argv_array_push(&conn->args, "-batch");
 			if (port) {

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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 Schindelin]

Hi Junio,

On Sun, 8 Jan 2017, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > 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 Hamano]

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

> 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

[Johannes Schindelin]

Hi Junio,

On Mon, 9 Jan 2017, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 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, &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);

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

[Junio C Hamano]

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);

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

[Johannes Schindelin]

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

[Junio C Hamano]

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

> 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 <gitster@pobox.com>
---
 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 <port>`
+	while OpenSSH uses `-p <port>` 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", &variant))
+		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(&conn->args, "-4");
 			else if (flags & CONNECT_IPV6)
 				argv_array_push(&conn->args, "-6");
+
+			override_ssh_variant(&port_option, &needs_batch);
+
 			if (needs_batch)
 				argv_array_push(&conn->args, "-batch");
 			if (port) {
-- 
2.11.0-699-ga1f1475476

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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 <port>" when OpenSSH would use "-p <port>",
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 <gitster@pobox.com>
---
 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(&conn->args, "-4");
 			else if (flags & CONNECT_IPV6)
 				argv_array_push(&conn->args, "-6");
-			if (tortoiseplink)
+			if (needs_batch)
 				argv_array_push(&conn->args, "-batch");
 			if (port) {
-				/* P is for PuTTY, p is for OpenSSH */
-				argv_array_push(&conn->args, putty ? "-P" : "-p");
+				argv_array_pushf(&conn->args, "-%c", port_option);
 				argv_array_push(&conn->args, port);
 			}
 			argv_array_push(&conn->args, ssh_host);
-- 
2.11.0-699-ga1f1475476

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

[Johannes Schindelin]

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 <port>" when OpenSSH would use "-p <port>",
> 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 <gitster@pobox.com>

Apart 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

[Jeff King]

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", &variant))
> +		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 Hamano]

Jeff King <peff@peff.net> writes:

> 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

[Johannes Schindelin]

Hi Junio,

On Wed, 25 Jan 2017, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 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

[PATCH v2 0/3] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND

[Johannes Schindelin]

We already handle PuTTY's plink and TortoiseGit's tortoiseplink in
GIT_SSH by automatically using the -P option to specify ports, and in
tortoiseplink's case by passing the --batch option.

For users who need to pass additional command-line options to plink,
this poses a problem: the only way to do that is to use GIT_SSH_COMMAND,
but Git does not handle that specifically, so those users have to
manually parse the command-line options passed via GIT_SSH_COMMAND and
replace -p (if present) by -P, and add --batch in the case of
tortoiseplink.

This is error-prone and a bad user experience.

To fix this, the changes proposed in this patch series introduce
handling this by splitting the GIT_SSH_COMMAND value and treating the
first parameter with the same grace as GIT_SSH. To counter any possible
misdetection, the user can also specify explicitly via GIT_SSH_VARIANT
or ssh.variant which SSH variant they are using.

This is v2 of the patch, now turned patch series. Relative to v1, it
integrates Junio's cleanup patch and Segev's follow-up Pull Request that
introduces the GIT_SSH_VARIANT and ssh.variant settings to override
Git's autodetection manually.


Junio C Hamano (1):
  connect: rename tortoiseplink and putty variables

Segev Finer (2):
  connect: handle putty/plink also in GIT_SSH_COMMAND
  connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

 Documentation/config.txt |  7 +++++
 Documentation/git.txt    |  7 +++++
 connect.c                | 66 +++++++++++++++++++++++++++++++++++-------------
 t/t5601-clone.sh         | 41 ++++++++++++++++++++++++++++++
 4 files changed, 104 insertions(+), 17 deletions(-)


base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
Published-As: https://github.com/dscho/git/releases/tag/putty-w-args-v2
Fetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v2

Interdiff vs v1:

 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index af2ae4cc02..f2c210f0a0 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -1949,6 +1949,13 @@ Environment variable settings always override any matches.  The URLs that are
  matched against are those given directly to Git commands.  This means any URLs
  visited as a result of a redirection do not participate in matching.
  
 +ssh.variant::
 +	Override the autodetection of plink/tortoiseplink in the SSH
 +	command that 'git fetch' and 'git push' use. It can be set to
 +	either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
 +	value will be treated as normal ssh. This is useful in case
 +	that Git gets this wrong.
 +
  i18n.commitEncoding::
  	Character encoding the commit messages are stored in; Git itself
  	does not care per se, but this information is necessary e.g. when
 diff --git a/Documentation/git.txt b/Documentation/git.txt
 index 4f208fab92..c322558aa7 100644
 --- a/Documentation/git.txt
 +++ b/Documentation/git.txt
 @@ -1020,6 +1020,13 @@ Usually it is easier to configure any desired options through your
  personal `.ssh/config` file.  Please consult your ssh documentation
  for further details.
  
 +`GIT_SSH_VARIANT`::
 +	If this environment variable is set, it overrides the autodetection
 +	of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
 +	push' use. It can be set to either `ssh`, `plink`, `putty` or
 +	`tortoiseplink`. Any other value will be treated as normal ssh. This
 +	is useful in case that Git gets this wrong.
 +
  `GIT_ASKPASS`::
  	If this environment variable is set, then Git commands which need to
  	acquire passwords or passphrases (e.g. for HTTP or IMAP authentication)
 diff --git a/connect.c b/connect.c
 index c81f77001b..7b4437578b 100644
 --- a/connect.c
 +++ b/connect.c
 @@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
  	return NULL;
  }
  
 +static int handle_ssh_variant(int *port_option, int *needs_batch)
 +{
 +	const char *variant;
 +
 +	if (!(variant = getenv("GIT_SSH_VARIANT")) &&
 +		git_config_get_string_const("ssh.variant", &variant))
 +		return 0;
 +
 +	if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
 +		*port_option = 'P';
 +	else if (!strcmp(variant, "tortoiseplink")) {
 +		*port_option = 'P';
 +		*needs_batch = 1;
 +	}
 +
 +	return 1;
 +}
 +
  /*
   * 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,
 @@ -769,7 +787,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;
 @@ -816,28 +835,32 @@ struct child_process *git_connect(int fd[2], const char *url,
  				ssh_argv0 = xstrdup(ssh);
  			}
  
 -			if (ssh_argv0) {
 +			if (!handle_ssh_variant(&port_option, &needs_batch) &&
 +			    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");
 -
 -				free(ssh_argv0);
 +				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);
 +
  			argv_array_push(&conn->args, ssh);
  			if (flags & CONNECT_IPV4)
  				argv_array_push(&conn->args, "-4");
  			else if (flags & CONNECT_IPV6)
  				argv_array_push(&conn->args, "-6");
 -			if (tortoiseplink)
 +			if (needs_batch)
  				argv_array_push(&conn->args, "-batch");
  			if (port) {
 -				/* P is for PuTTY, p is for OpenSSH */
 -				argv_array_push(&conn->args, putty ? "-P" : "-p");
 +				argv_array_pushf(&conn->args,
 +						 "-%c", port_option);
  				argv_array_push(&conn->args, port);
  			}
  			argv_array_push(&conn->args, ssh_host);
 diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
 index 9335e10c2a..b52b8acf98 100755
 --- a/t/t5601-clone.sh
 +++ b/t/t5601-clone.sh
 @@ -401,6 +401,32 @@ test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '
  	expect_ssh "-v -P 123" myhost src
  '
  
 +test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
 +	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
 +	GIT_SSH_VARIANT=ssh \
 +	git clone "[myhost:123]:src" ssh-bracket-clone-variant-1 &&
 +	expect_ssh "-p 123" myhost src
 +'
 +
 +test_expect_success 'ssh.variant overrides plink detection' '
 +	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
 +	git -c ssh.variant=ssh \
 +		clone "[myhost:123]:src" ssh-bracket-clone-variant-2 &&
 +	expect_ssh "-p 123" myhost src
 +'
 +
 +test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
 +	GIT_SSH_VARIANT=plink \
 +	git clone "[myhost:123]:src" ssh-bracket-clone-variant-3 &&
 +	expect_ssh "-P 123" myhost src
 +'
 +
 +test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '
 +	GIT_SSH_VARIANT=tortoiseplink \
 +	git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 &&
 +	expect_ssh "-batch -P 123" myhost src
 +'
 +
  # Reset the GIT_SSH environment variable for clone tests.
  setup_ssh_wrapper
  

-- 
2.11.1.windows.prerelease.2.9.g3014b57

[PATCH v2 1/3] connect: handle putty/plink also in GIT_SSH_COMMAND

[Johannes Schindelin]

From: Segev Finer <segev208@gmail.com>

Git for Windows has special support for the popular SSH client PuTTY:
when using PuTTY's non-interactive version ("plink.exe"), we use the -P
option to specify the port rather than OpenSSH's -p option. TortoiseGit
ships with its own, forked version of plink.exe, that adds support for
the -batch option, and for good measure we special-case that, too.

However, this special-casing of PuTTY only covers the case where the
user overrides the SSH command via the environment variable GIT_SSH
(which allows specifying the name of the executable), not
GIT_SSH_COMMAND (which allows specifying a full command, including
additional command-line options).

When users want to pass any additional arguments to (Tortoise-)Plink,
such as setting a private key, they are required to either use a shell
script named plink or tortoiseplink or duplicate the logic that is
already in Git for passing the correct style of command line arguments,
which can be difficult, error prone and annoying to get right.

This patch simply reuses the existing logic and expands it to cover
GIT_SSH_COMMAND, too.

Note: it may look a little heavy-handed to duplicate the entire
command-line and then split it, only to extract the name of the
executable. However, this is not a performance-critical code path, and
the code is much more readable this way.

Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 connect.c        | 23 ++++++++++++++++-------
 t/t5601-clone.sh | 15 +++++++++++++++
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/connect.c b/connect.c
index 8cb93b0720..c81f77001b 100644
--- a/connect.c
+++ b/connect.c
@@ -772,6 +772,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 			int putty = 0, tortoiseplink = 0;
 			char *ssh_host = hostandport;
 			const char *port = NULL;
+			char *ssh_argv0 = NULL;
 			transport_check_allowed("ssh");
 			get_host_and_port(&ssh_host, &port);
 
@@ -792,10 +793,15 @@ struct child_process *git_connect(int fd[2], const char *url,
 			}
 
 			ssh = get_ssh_command();
-			if (!ssh) {
-				const char *base;
-				char *ssh_dup;
-
+			if (ssh) {
+				char *split_ssh = xstrdup(ssh);
+				const char **ssh_argv;
+
+				if (split_cmdline(split_ssh, &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");
@@ -816,7 +825,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 					!strcasecmp(base, "plink") ||
 					!strcasecmp(base, "plink.exe");
 
-				free(ssh_dup);
+				free(ssh_argv0);
 			}
 
 			argv_array_push(&conn->args, ssh);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 4241ea5b32..9335e10c2a 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -386,6 +386,21 @@ test_expect_success 'tortoiseplink is like putty, with extra arguments' '
 	expect_ssh "-batch -P 123" myhost src
 '
 
+test_expect_success 'double quoted plink.exe in GIT_SSH_COMMAND' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+	GIT_SSH_COMMAND="\"$TRASH_DIRECTORY/plink.exe\" -v" \
+		git clone "[myhost:123]:src" ssh-bracket-clone-plink-3 &&
+	expect_ssh "-v -P 123" myhost src
+'
+
+SQ="'"
+test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+	GIT_SSH_COMMAND="$SQ$TRASH_DIRECTORY/plink.exe$SQ -v" \
+		git clone "[myhost:123]:src" ssh-bracket-clone-plink-4 &&
+	expect_ssh "-v -P 123" myhost src
+'
+
 # Reset the GIT_SSH environment variable for clone tests.
 setup_ssh_wrapper
 
-- 
2.11.1.windows.prerelease.2.9.g3014b57

[PATCH v2 2/3] connect: rename tortoiseplink and putty variables

[Johannes Schindelin]

From: Junio C Hamano <gitster@pobox.com>

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 <port>" when OpenSSH would use "-p <port>",
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.

[jes: wrapped overly-long line]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 connect.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/connect.c b/connect.c
index c81f77001b..9f750eacb6 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,11 @@ struct child_process *git_connect(int fd[2], const char *url,
 				argv_array_push(&conn->args, "-4");
 			else if (flags & CONNECT_IPV6)
 				argv_array_push(&conn->args, "-6");
-			if (tortoiseplink)
+			if (needs_batch)
 				argv_array_push(&conn->args, "-batch");
 			if (port) {
-				/* P is for PuTTY, p is for OpenSSH */
-				argv_array_push(&conn->args, putty ? "-P" : "-p");
+				argv_array_pushf(&conn->args,
+						 "-%c", port_option);
 				argv_array_push(&conn->args, port);
 			}
 			argv_array_push(&conn->args, ssh_host);
-- 
2.11.1.windows.prerelease.2.9.g3014b57

[PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Johannes Schindelin]

From: Segev Finer <segev208@gmail.com>

This environment variable and configuration value allow to
override the autodetection of plink/tortoiseplink in case that
Git gets it wrong.

[jes: wrapped overly-long lines, changed get_ssh_variant() to
handle_ssh_variant() to accomodate the change from the
putty/tortoiseplink variables to port_option/needs_batch.]

Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt |  7 +++++++
 Documentation/git.txt    |  7 +++++++
 connect.c                | 24 ++++++++++++++++++++++--
 t/t5601-clone.sh         | 26 ++++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4cc02..f2c210f0a0 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1949,6 +1949,13 @@ Environment variable settings always override any matches.  The URLs that are
 matched against are those given directly to Git commands.  This means any URLs
 visited as a result of a redirection do not participate in matching.
 
+ssh.variant::
+	Override the autodetection of plink/tortoiseplink in the SSH
+	command that 'git fetch' and 'git push' use. It can be set to
+	either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
+	value will be treated as normal ssh. This is useful in case
+	that Git gets this wrong.
+
 i18n.commitEncoding::
 	Character encoding the commit messages are stored in; Git itself
 	does not care per se, but this information is necessary e.g. when
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4f208fab92..c322558aa7 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1020,6 +1020,13 @@ Usually it is easier to configure any desired options through your
 personal `.ssh/config` file.  Please consult your ssh documentation
 for further details.
 
+`GIT_SSH_VARIANT`::
+	If this environment variable is set, it overrides the autodetection
+	of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
+	push' use. It can be set to either `ssh`, `plink`, `putty` or
+	`tortoiseplink`. Any other value will be treated as normal ssh. This
+	is useful in case that Git gets this wrong.
+
 `GIT_ASKPASS`::
 	If this environment variable is set, then Git commands which need to
 	acquire passwords or passphrases (e.g. for HTTP or IMAP authentication)
diff --git a/connect.c b/connect.c
index 9f750eacb6..7b4437578b 100644
--- a/connect.c
+++ b/connect.c
@@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
 	return NULL;
 }
 
+static int handle_ssh_variant(int *port_option, int *needs_batch)
+{
+	const char *variant;
+
+	if (!(variant = getenv("GIT_SSH_VARIANT")) &&
+		git_config_get_string_const("ssh.variant", &variant))
+		return 0;
+
+	if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
+		*port_option = 'P';
+	else if (!strcmp(variant, "tortoiseplink")) {
+		*port_option = 'P';
+		*needs_batch = 1;
+	}
+
+	return 1;
+}
+
 /*
  * 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,
@@ -817,7 +835,8 @@ struct child_process *git_connect(int fd[2], const char *url,
 				ssh_argv0 = xstrdup(ssh);
 			}
 
-			if (ssh_argv0) {
+			if (!handle_ssh_variant(&port_option, &needs_batch) &&
+			    ssh_argv0) {
 				const char *base = basename(ssh_argv0);
 
 				if (!strcasecmp(base, "tortoiseplink") ||
@@ -828,9 +847,10 @@ struct child_process *git_connect(int fd[2], const char *url,
 					   !strcasecmp(base, "plink.exe")) {
 					port_option = 'P';
 				}
-				free(ssh_argv0);
 			}
 
+			free(ssh_argv0);
+
 			argv_array_push(&conn->args, ssh);
 			if (flags & CONNECT_IPV4)
 				argv_array_push(&conn->args, "-4");
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9335e10c2a..b52b8acf98 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -401,6 +401,32 @@ test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '
 	expect_ssh "-v -P 123" myhost src
 '
 
+test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+	GIT_SSH_VARIANT=ssh \
+	git clone "[myhost:123]:src" ssh-bracket-clone-variant-1 &&
+	expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'ssh.variant overrides plink detection' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+	git -c ssh.variant=ssh \
+		clone "[myhost:123]:src" ssh-bracket-clone-variant-2 &&
+	expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
+	GIT_SSH_VARIANT=plink \
+	git clone "[myhost:123]:src" ssh-bracket-clone-variant-3 &&
+	expect_ssh "-P 123" myhost src
+'
+
+test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '
+	GIT_SSH_VARIANT=tortoiseplink \
+	git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 &&
+	expect_ssh "-batch -P 123" myhost src
+'
+
 # Reset the GIT_SSH environment variable for clone tests.
 setup_ssh_wrapper
 
-- 
2.11.1.windows.prerelease.2.9.g3014b57

Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Junio C Hamano]

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

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index af2ae4cc02..f2c210f0a0 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1949,6 +1949,13 @@ Environment variable settings always override any matches.  The URLs that are
>  matched against are those given directly to Git commands.  This means any URLs
>  visited as a result of a redirection do not participate in matching.
>  
> +ssh.variant::
> +	Override the autodetection of plink/tortoiseplink in the SSH
> +	command that 'git fetch' and 'git push' use. It can be set to
> +	either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
> +	value will be treated as normal ssh. This is useful in case
> +	that Git gets this wrong.
> +

I do like the fact that this now sits under ssh.* hierarchy (not core.*).

It should mention core.sshCommand, GIT_SSH_COMMAND, etc., because
reading this alone would mislead users to set only this one and expect
that their plink will be used without setting core.sshCommand etc.

It also should say that GIT_SSH_VARIANT environment variable will
override this variable.

> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 4f208fab92..c322558aa7 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -1020,6 +1020,13 @@ Usually it is easier to configure any desired options through your
>  personal `.ssh/config` file.  Please consult your ssh documentation
>  for further details.
>  
> +`GIT_SSH_VARIANT`::
> +	If this environment variable is set, it overrides the autodetection
> +	of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
> +	push' use. It can be set to either `ssh`, `plink`, `putty` or
> +	`tortoiseplink`. Any other value will be treated as normal ssh. This
> +	is useful in case that Git gets this wrong.

Similarly this should mention GIT_SSH_COMMAND at least.  It is crazy
to set something that will cause misdetection to core.sshCommand and
use this environment variable to fix it (instead of using ssh.variant),
so I think it is OK not to mention core.sshCommand (but it would not
hurt to do so).

> diff --git a/connect.c b/connect.c
> index 9f750eacb6..7b4437578b 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
>  	return NULL;
>  }
>  
> +static int handle_ssh_variant(int *port_option, int *needs_batch)
> +{
> +	const char *variant;
> +
> +	if (!(variant = getenv("GIT_SSH_VARIANT")) &&
> +		git_config_get_string_const("ssh.variant", &variant))
> +		return 0;
> +
> +	if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
> +		*port_option = 'P';
> +	else if (!strcmp(variant, "tortoiseplink")) {
> +		*port_option = 'P';
> +		*needs_batch = 1;
> +	}
> +
> +	return 1;
> +}

Between handle and get I do not think there is strong reason to
favor one over the other.  Unlike the one I sent yesterday, this is
not overriding but is getting, so we should keep the original name
Segev gave it, which is shorter, I would think, rather than renaming
it to "handle".

The string that came from the configuration must be freed, no?  That
is what I recall in Peff's comment yesterday.

> @@ -817,7 +835,8 @@ struct child_process *git_connect(int fd[2], const char *url,
>  				ssh_argv0 = xstrdup(ssh);
>  			}
>  
> -			if (ssh_argv0) {
> +			if (!handle_ssh_variant(&port_option, &needs_batch) &&
> +			    ssh_argv0) {

I like the placement of this "if the user explicitly told us" much
better.

My patch yesterday was deliberately being lazy, because the next
thought that will come to us after comparing the two is "this way,
we avoid having to do the auto-detection altogether, which is way
better than wasting the effort to auto-detect, only to override".

And that reasoning will lead to a realization "there is no reason to
even do the split_cmdline() if the user explicitly told us".  While
that reasoning is correct and we _should_ further refactor, I didn't
want to spend braincycles on that myself.

Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Johannes Schindelin]

Hi Junio,

On Thu, 26 Jan 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/connect.c b/connect.c
> > index 9f750eacb6..7b4437578b 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
> >  	return NULL;
> >  }
> >  
> > +static int handle_ssh_variant(int *port_option, int *needs_batch)
> > +{
> > +	const char *variant;
> > +
> > +	if (!(variant = getenv("GIT_SSH_VARIANT")) &&
> > +		git_config_get_string_const("ssh.variant", &variant))
> > +		return 0;
> > +
> > +	if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
> > +		*port_option = 'P';
> > +	else if (!strcmp(variant, "tortoiseplink")) {
> > +		*port_option = 'P';
> > +		*needs_batch = 1;
> > +	}
> > +
> > +	return 1;
> > +}
> 
> Between handle and get I do not think there is strong reason to
> favor one over the other.

That is correct. "handle" and "get" are two very beautiful words, and none
of them deserves to take a back seat behind the other.

In this case, "get" is inappropriate, though, as the function does not
return the ssh variant, nor does it assign the ssh variant to any variable
to which any of its argument points.

Will try to find the time to address the other issues soon,
Johannes

Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Junio C Hamano]

Just to save us extra round-trip.

Junio C Hamano <gitster@pobox.com> writes:

>> +`GIT_SSH_VARIANT`::
>> +	If this environment variable is set, it overrides the autodetection
>> +	of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
>> +	push' use. It can be set to either `ssh`, `plink`, `putty` or
>> +	`tortoiseplink`. Any other value will be treated as normal ssh. This
>> +	is useful in case that Git gets this wrong.
>
> Similarly this should mention GIT_SSH_COMMAND at least.  It is crazy
> to set something that will cause misdetection to core.sshCommand and
> use this environment variable to fix it (instead of using ssh.variant),
> so I think it is OK not to mention core.sshCommand (but it would not
> hurt to do so).
>
>> diff --git a/connect.c b/connect.c
>> index 9f750eacb6..7b4437578b 100644
>> --- a/connect.c
>> +++ b/connect.c
>> @@ -691,6 +691,24 @@ static const char *get_ssh_command(void)
>>  	return NULL;
>>  }
>>  
>> +static int handle_ssh_variant(int *port_option, int *needs_batch)
>> +{
>> ...
>> +}
> ...
>
> The string that came from the configuration must be freed, no?  That
> is what I recall in Peff's comment yesterday.

The leak needs plugging in some way.  

Because this thing is merely an escape hatch that somebody who needs
it needs to use it always (as opposed to one-shot basis), we do not
need to support the environment variable and go with only the
configuration, which may make it easier to plug the leak.

>> @@ -817,7 +835,8 @@ struct child_process *git_connect(int fd[2], const char *url,
>>  				ssh_argv0 = xstrdup(ssh);
>>  			}
>>  
>> -			if (ssh_argv0) {
>> +			if (!handle_ssh_variant(&port_option, &needs_batch) &&
>> +			    ssh_argv0) {
>
> I like the placement of this "if the user explicitly told us" much
> better.
> ...
> And that reasoning will lead to a realization "there is no reason to
> even do the split_cmdline() if the user explicitly told us".  While
> that reasoning is correct and we _should_ further refactor, I didn't
> want to spend braincycles on that myself.

This _should_ above is a lot weaker than _must_.  

IOW, I think it is acceptable to always split GIT_SSH_COMMAND into
tokens before we realize that the user used the escape hatch and the
splitting was a wasted effort.  This is exactly because this thing
is an escape hatch that is expected to be rarely used.  Of course,
if the "wasted effort" can be eliminated without sacrificing the
simplicity of the code, that is fine as well.

Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Johannes Schindelin]

Hi Junio,

On Fri, 27 Jan 2017, Junio C Hamano wrote:

> IOW, I think it is acceptable to always split GIT_SSH_COMMAND into
> tokens before we realize that the user used the escape hatch and the
> splitting was a wasted effort.  This is exactly because this thing
> is an escape hatch that is expected to be rarely used.  Of course,
> if the "wasted effort" can be eliminated without sacrificing the
> simplicity of the code, that is fine as well.

Simplicity is retained. Battle-readiness was sacrificed on the way: the
new code is not tested well enough, and `next` will not help one bit.

Ciao,
Johannes

Re: [PATCH v2 3/3] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Junio C Hamano]

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

> On Fri, 27 Jan 2017, Junio C Hamano wrote:
>
>> IOW, I think it is acceptable to always split GIT_SSH_COMMAND into
>> tokens before we realize that the user used the escape hatch and the
>> splitting was a wasted effort.  This is exactly because this thing
>> is an escape hatch that is expected to be rarely used.  Of course,
>> if the "wasted effort" can be eliminated without sacrificing the
>> simplicity of the code, that is fine as well.
>
> Simplicity is retained. Battle-readiness was sacrificed on the way: the
> new code is not tested well enough, and `next` will not help one bit.

Let me make it clear that there is no burning desire to sacrifice
battle-readiness in the above.  If we expect that auto-detection
would be minority, then it makes sense to get the configured value
first and then spend cycles to split and guess only when detection
is needed.  

In this case, because we expect that auto-detection will be used
most of the time, it is good enough to always split first, get the
configured value, and spend cycles to guess, or for that matter it
is perfectly fine to always split and guess first and then override
with the configured value.

If your attempt to optimize for a wrong case ended up causing new
unnecessary bugs, don't blame me.

[PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND

[Johannes Schindelin]

We already handle PuTTY's plink and TortoiseGit's tortoiseplink in
GIT_SSH by automatically using the -P option to specify ports, and in
tortoiseplink's case by passing the --batch option.

For users who need to pass additional command-line options to plink,
this poses a problem: the only way to do that is to use GIT_SSH_COMMAND,
but Git does not handle that specifically, so those users have to
manually parse the command-line options passed via GIT_SSH_COMMAND and
replace -p (if present) by -P, and add --batch in the case of
tortoiseplink.

This is error-prone and a bad user experience.

To fix this, the changes proposed in this patch series introduce
handling this by splitting the GIT_SSH_COMMAND value and treating the
first parameter with the same grace as GIT_SSH. To counter any possible
misdetection, the user can also specify explicitly via GIT_SSH_VARIANT
or ssh.variant which SSH variant they are using.

Changes relative to v2:

- touched up the documentation for ssh.variant to make it even easier to
  understand

- free()d the config variable

- completely refactored the code to fulfil Junio's burning desire to
  avoid split_cmdline() when unnecessary

It is quite preposterous to call this an "iteration" of the patch
series, because the code is so different now. I say this because I want
to caution that this code has not been tested as thoroughly, by far, as
the first iteration.

The primary purpose of code review is correctness, everything else is
either a consequence of it, or a means to make reviewing easier.

That means that I highly encourage those who pushed for these extensive
changes that make the patch series a lot less robust to balance things
out by at least *rudimentary* testing.


Johannes Schindelin (1):
  git_connect(): factor out SSH variant handling

Junio C Hamano (1):
  connect: rename tortoiseplink and putty variables

Segev Finer (2):
  connect: handle putty/plink also in GIT_SSH_COMMAND
  connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

 Documentation/config.txt | 11 +++++++
 Documentation/git.txt    |  6 ++++
 connect.c                | 75 ++++++++++++++++++++++++++++++++++++------------
 t/t5601-clone.sh         | 41 ++++++++++++++++++++++++++
 4 files changed, 114 insertions(+), 19 deletions(-)


base-commit: 8f60064c1f538f06e1c579cbd9840b86b10bcd3d
Published-As: https://github.com/dscho/git/releases/tag/putty-w-args-v3
Fetch-It-Via: git fetch https://github.com/dscho/git putty-w-args-v3

Interdiff vs v2:

 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index f2c210f0a0..b88df57ab6 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -1950,11 +1950,15 @@ matched against are those given directly to Git commands.  This means any URLs
  visited as a result of a redirection do not participate in matching.
  
  ssh.variant::
 -	Override the autodetection of plink/tortoiseplink in the SSH
 -	command that 'git fetch' and 'git push' use. It can be set to
 -	either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
 -	value will be treated as normal ssh. This is useful in case
 -	that Git gets this wrong.
 +	Depending on the value of the environment variables `GIT_SSH` or
 +	`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
 +	auto-detects whether to adjust its command-line parameters for use
 +	with plink or tortoiseplink, as opposed to the default (OpenSSH).
 ++
 +The config variable `ssh.variant` can be set to override this auto-detection;
 +valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
 +will be treated as normal ssh. This setting can be overridden via the
 +environment variable `GIT_SSH_VARIANT`.
  
  i18n.commitEncoding::
  	Character encoding the commit messages are stored in; Git itself
 diff --git a/Documentation/git.txt b/Documentation/git.txt
 index c322558aa7..a0c6728d1a 100644
 --- a/Documentation/git.txt
 +++ b/Documentation/git.txt
 @@ -1021,11 +1021,10 @@ personal `.ssh/config` file.  Please consult your ssh documentation
  for further details.
  
  `GIT_SSH_VARIANT`::
 -	If this environment variable is set, it overrides the autodetection
 -	of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
 -	push' use. It can be set to either `ssh`, `plink`, `putty` or
 -	`tortoiseplink`. Any other value will be treated as normal ssh. This
 -	is useful in case that Git gets this wrong.
 +	If this environment variable is set, it overrides Git's autodetection
 +	whether `GIT_SSH`/`GIT_SSH_COMMAND`/`core.sshCommand` refer to OpenSSH,
 +	plink or tortoiseplink. This variable overrides the config setting
 +	`ssh.variant` that serves the same purpose.
  
  `GIT_ASKPASS`::
  	If this environment variable is set, then Git commands which need to
 diff --git a/connect.c b/connect.c
 index 7b4437578b..7f1f802396 100644
 --- a/connect.c
 +++ b/connect.c
 @@ -691,20 +691,45 @@ static const char *get_ssh_command(void)
  	return NULL;
  }
  
 -static int handle_ssh_variant(int *port_option, int *needs_batch)
 +static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
 +			      int *port_option, int *needs_batch)
  {
 -	const char *variant;
 +	const char *variant = getenv("GIT_SSH_VARIANT");
 +	char *p = NULL;
 +
 +	if (variant)
 +		; /* okay, fall through */
 +	else if (!git_config_get_string("ssh.variant", &p))
 +		variant = p;
 +	else if (!is_cmdline) {
 +		p = xstrdup(ssh_command);
 +		variant = basename(p);
 +	} else {
 +		const char **ssh_argv;
  
 -	if (!(variant = getenv("GIT_SSH_VARIANT")) &&
 -		git_config_get_string_const("ssh.variant", &variant))
 -		return 0;
 +		p = xstrdup(ssh_command);
 +		if (split_cmdline(p, &ssh_argv)) {
 +			variant = basename((char *)ssh_argv[0]);
 +			/*
 +			 * At this point, variant points into the buffer
 +			 * referenced by p, hence we do not need ssh_argv
 +			 * any longer.
 +			 */
 +			free(ssh_argv);
 +		} else
 +			return 0;
 +	}
  
 -	if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
 +	if (!strcasecmp(variant, "plink") ||
 +	    !strcasecmp(variant, "plink.exe") ||
 +	    !strcasecmp(variant, "putty"))
  		*port_option = 'P';
 -	else if (!strcmp(variant, "tortoiseplink")) {
 +	else if (!strcasecmp(variant, "tortoiseplink") ||
 +		 !strcasecmp(variant, "tortoiseplink.exe")) {
  		*port_option = 'P';
  		*needs_batch = 1;
  	}
 +	free(p);
  
  	return 1;
  }
 @@ -791,7 +816,6 @@ struct child_process *git_connect(int fd[2], const char *url,
  			int port_option = 'p';
  			char *ssh_host = hostandport;
  			const char *port = NULL;
 -			char *ssh_argv0 = NULL;
  			transport_check_allowed("ssh");
  			get_host_and_port(&ssh_host, &port);
  
 @@ -812,15 +836,10 @@ struct child_process *git_connect(int fd[2], const char *url,
  			}
  
  			ssh = get_ssh_command();
 -			if (ssh) {
 -				char *split_ssh = xstrdup(ssh);
 -				const char **ssh_argv;
 -
 -				if (split_cmdline(split_ssh, &ssh_argv))
 -					ssh_argv0 = xstrdup(ssh_argv[0]);
 -				free(split_ssh);
 -				free((void *)ssh_argv);
 -			} else {
 +			if (ssh)
 +				handle_ssh_variant(ssh, 1, &port_option,
 +						   &needs_batch);
 +			else {
  				/*
  				 * GIT_SSH is the no-shell version of
  				 * GIT_SSH_COMMAND (and must remain so for
 @@ -831,26 +850,12 @@ struct child_process *git_connect(int fd[2], const char *url,
  				ssh = getenv("GIT_SSH");
  				if (!ssh)
  					ssh = "ssh";
 -
 -				ssh_argv0 = xstrdup(ssh);
 +				else
 +					handle_ssh_variant(ssh, 0,
 +							   &port_option,
 +							   &needs_batch);
  			}
  
 -			if (!handle_ssh_variant(&port_option, &needs_batch) &&
 -			    ssh_argv0) {
 -				const char *base = basename(ssh_argv0);
 -
 -				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);
 -
  			argv_array_push(&conn->args, ssh);
  			if (flags & CONNECT_IPV4)
  				argv_array_push(&conn->args, "-4");

-- 
2.11.1.windows.prerelease.2.9.g3014b57

[PATCH v3 1/4] connect: handle putty/plink also in GIT_SSH_COMMAND

[Johannes Schindelin]

From: Segev Finer <segev208@gmail.com>

Git for Windows has special support for the popular SSH client PuTTY:
when using PuTTY's non-interactive version ("plink.exe"), we use the -P
option to specify the port rather than OpenSSH's -p option. TortoiseGit
ships with its own, forked version of plink.exe, that adds support for
the -batch option, and for good measure we special-case that, too.

However, this special-casing of PuTTY only covers the case where the
user overrides the SSH command via the environment variable GIT_SSH
(which allows specifying the name of the executable), not
GIT_SSH_COMMAND (which allows specifying a full command, including
additional command-line options).

When users want to pass any additional arguments to (Tortoise-)Plink,
such as setting a private key, they are required to either use a shell
script named plink or tortoiseplink or duplicate the logic that is
already in Git for passing the correct style of command line arguments,
which can be difficult, error prone and annoying to get right.

This patch simply reuses the existing logic and expands it to cover
GIT_SSH_COMMAND, too.

Note: it may look a little heavy-handed to duplicate the entire
command-line and then split it, only to extract the name of the
executable. However, this is not a performance-critical code path, and
the code is much more readable this way.

Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 connect.c        | 23 ++++++++++++++++-------
 t/t5601-clone.sh | 15 +++++++++++++++
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/connect.c b/connect.c
index 8cb93b0720..c81f77001b 100644
--- a/connect.c
+++ b/connect.c
@@ -772,6 +772,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 			int putty = 0, tortoiseplink = 0;
 			char *ssh_host = hostandport;
 			const char *port = NULL;
+			char *ssh_argv0 = NULL;
 			transport_check_allowed("ssh");
 			get_host_and_port(&ssh_host, &port);
 
@@ -792,10 +793,15 @@ struct child_process *git_connect(int fd[2], const char *url,
 			}
 
 			ssh = get_ssh_command();
-			if (!ssh) {
-				const char *base;
-				char *ssh_dup;
-
+			if (ssh) {
+				char *split_ssh = xstrdup(ssh);
+				const char **ssh_argv;
+
+				if (split_cmdline(split_ssh, &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");
@@ -816,7 +825,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 					!strcasecmp(base, "plink") ||
 					!strcasecmp(base, "plink.exe");
 
-				free(ssh_dup);
+				free(ssh_argv0);
 			}
 
 			argv_array_push(&conn->args, ssh);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 4241ea5b32..9335e10c2a 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -386,6 +386,21 @@ test_expect_success 'tortoiseplink is like putty, with extra arguments' '
 	expect_ssh "-batch -P 123" myhost src
 '
 
+test_expect_success 'double quoted plink.exe in GIT_SSH_COMMAND' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+	GIT_SSH_COMMAND="\"$TRASH_DIRECTORY/plink.exe\" -v" \
+		git clone "[myhost:123]:src" ssh-bracket-clone-plink-3 &&
+	expect_ssh "-v -P 123" myhost src
+'
+
+SQ="'"
+test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" &&
+	GIT_SSH_COMMAND="$SQ$TRASH_DIRECTORY/plink.exe$SQ -v" \
+		git clone "[myhost:123]:src" ssh-bracket-clone-plink-4 &&
+	expect_ssh "-v -P 123" myhost src
+'
+
 # Reset the GIT_SSH environment variable for clone tests.
 setup_ssh_wrapper
 
-- 
2.11.1.windows.prerelease.2.9.g3014b57

[PATCH v3 2/4] connect: rename tortoiseplink and putty variables

[Johannes Schindelin]

From: Junio C Hamano <gitster@pobox.com>

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 <port>" when OpenSSH would use "-p <port>",
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.

[jes: wrapped overly-long line]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 connect.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/connect.c b/connect.c
index c81f77001b..9f750eacb6 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,11 @@ struct child_process *git_connect(int fd[2], const char *url,
 				argv_array_push(&conn->args, "-4");
 			else if (flags & CONNECT_IPV6)
 				argv_array_push(&conn->args, "-6");
-			if (tortoiseplink)
+			if (needs_batch)
 				argv_array_push(&conn->args, "-batch");
 			if (port) {
-				/* P is for PuTTY, p is for OpenSSH */
-				argv_array_push(&conn->args, putty ? "-P" : "-p");
+				argv_array_pushf(&conn->args,
+						 "-%c", port_option);
 				argv_array_push(&conn->args, port);
 			}
 			argv_array_push(&conn->args, ssh_host);
-- 
2.11.1.windows.prerelease.2.9.g3014b57

[PATCH v3 3/4] git_connect(): factor out SSH variant handling

[Johannes Schindelin]

We handle plink and tortoiseplink as OpenSSH replacements, by passing
the correct command-line options when detecting that they are used.

To let users override that auto-detection (in case Git gets it wrong),
we need to introduce new code to that end.

In preparation for this code, let's factor out the SSH variant handling
into its own function, handle_ssh_variant().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 connect.c | 72 ++++++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 46 insertions(+), 26 deletions(-)

diff --git a/connect.c b/connect.c
index 9f750eacb6..2734b9a1ca 100644
--- a/connect.c
+++ b/connect.c
@@ -691,6 +691,44 @@ static const char *get_ssh_command(void)
 	return NULL;
 }
 
+static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
+			      int *port_option, int *needs_batch)
+{
+	const char *variant;
+	char *p = NULL;
+
+	if (!is_cmdline) {
+		p = xstrdup(ssh_command);
+		variant = basename(p);
+	} else {
+		const char **ssh_argv;
+
+		p = xstrdup(ssh_command);
+		if (split_cmdline(p, &ssh_argv)) {
+			variant = basename((char *)ssh_argv[0]);
+			/*
+			 * At this point, variant points into the buffer
+			 * referenced by p, hence we do not need ssh_argv
+			 * any longer.
+			 */
+			free(ssh_argv);
+		} else
+			return 0;
+	}
+
+	if (!strcasecmp(variant, "plink") ||
+	    !strcasecmp(variant, "plink.exe"))
+		*port_option = 'P';
+	else if (!strcasecmp(variant, "tortoiseplink") ||
+		 !strcasecmp(variant, "tortoiseplink.exe")) {
+		*port_option = 'P';
+		*needs_batch = 1;
+	}
+	free(p);
+
+	return 1;
+}
+
 /*
  * 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,
@@ -773,7 +811,6 @@ struct child_process *git_connect(int fd[2], const char *url,
 			int port_option = 'p';
 			char *ssh_host = hostandport;
 			const char *port = NULL;
-			char *ssh_argv0 = NULL;
 			transport_check_allowed("ssh");
 			get_host_and_port(&ssh_host, &port);
 
@@ -794,15 +831,10 @@ struct child_process *git_connect(int fd[2], const char *url,
 			}
 
 			ssh = get_ssh_command();
-			if (ssh) {
-				char *split_ssh = xstrdup(ssh);
-				const char **ssh_argv;
-
-				if (split_cmdline(split_ssh, &ssh_argv))
-					ssh_argv0 = xstrdup(ssh_argv[0]);
-				free(split_ssh);
-				free((void *)ssh_argv);
-			} else {
+			if (ssh)
+				handle_ssh_variant(ssh, 1, &port_option,
+						   &needs_batch);
+			else {
 				/*
 				 * GIT_SSH is the no-shell version of
 				 * GIT_SSH_COMMAND (and must remain so for
@@ -813,22 +845,10 @@ struct child_process *git_connect(int fd[2], const char *url,
 				ssh = getenv("GIT_SSH");
 				if (!ssh)
 					ssh = "ssh";
-
-				ssh_argv0 = xstrdup(ssh);
-			}
-
-			if (ssh_argv0) {
-				const char *base = basename(ssh_argv0);
-
-				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);
+				else
+					handle_ssh_variant(ssh, 0,
+							   &port_option,
+							   &needs_batch);
 			}
 
 			argv_array_push(&conn->args, ssh);
-- 
2.11.1.windows.prerelease.2.9.g3014b57

[PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Johannes Schindelin]

From: Segev Finer <segev208@gmail.com>

This environment variable and configuration value allow to
override the autodetection of plink/tortoiseplink in case that
Git gets it wrong.

[jes: wrapped overly-long lines, factored out and changed
get_ssh_variant() to handle_ssh_variant() to accomodate the
change from the putty/tortoiseplink variables to
port_option/needs_batch, adjusted the documentation, free()d
value obtained from the config.]

Signed-off-by: Segev Finer <segev208@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config.txt | 11 +++++++++++
 Documentation/git.txt    |  6 ++++++
 connect.c                | 11 ++++++++---
 t/t5601-clone.sh         | 26 ++++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4cc02..b88df57ab6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1949,6 +1949,17 @@ Environment variable settings always override any matches.  The URLs that are
 matched against are those given directly to Git commands.  This means any URLs
 visited as a result of a redirection do not participate in matching.
 
+ssh.variant::
+	Depending on the value of the environment variables `GIT_SSH` or
+	`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
+	auto-detects whether to adjust its command-line parameters for use
+	with plink or tortoiseplink, as opposed to the default (OpenSSH).
++
+The config variable `ssh.variant` can be set to override this auto-detection;
+valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
+will be treated as normal ssh. This setting can be overridden via the
+environment variable `GIT_SSH_VARIANT`.
+
 i18n.commitEncoding::
 	Character encoding the commit messages are stored in; Git itself
 	does not care per se, but this information is necessary e.g. when
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4f208fab92..a0c6728d1a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1020,6 +1020,12 @@ Usually it is easier to configure any desired options through your
 personal `.ssh/config` file.  Please consult your ssh documentation
 for further details.
 
+`GIT_SSH_VARIANT`::
+	If this environment variable is set, it overrides Git's autodetection
+	whether `GIT_SSH`/`GIT_SSH_COMMAND`/`core.sshCommand` refer to OpenSSH,
+	plink or tortoiseplink. This variable overrides the config setting
+	`ssh.variant` that serves the same purpose.
+
 `GIT_ASKPASS`::
 	If this environment variable is set, then Git commands which need to
 	acquire passwords or passphrases (e.g. for HTTP or IMAP authentication)
diff --git a/connect.c b/connect.c
index 2734b9a1ca..7f1f802396 100644
--- a/connect.c
+++ b/connect.c
@@ -694,10 +694,14 @@ static const char *get_ssh_command(void)
 static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
 			      int *port_option, int *needs_batch)
 {
-	const char *variant;
+	const char *variant = getenv("GIT_SSH_VARIANT");
 	char *p = NULL;
 
-	if (!is_cmdline) {
+	if (variant)
+		; /* okay, fall through */
+	else if (!git_config_get_string("ssh.variant", &p))
+		variant = p;
+	else if (!is_cmdline) {
 		p = xstrdup(ssh_command);
 		variant = basename(p);
 	} else {
@@ -717,7 +721,8 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
 	}
 
 	if (!strcasecmp(variant, "plink") ||
-	    !strcasecmp(variant, "plink.exe"))
+	    !strcasecmp(variant, "plink.exe") ||
+	    !strcasecmp(variant, "putty"))
 		*port_option = 'P';
 	else if (!strcasecmp(variant, "tortoiseplink") ||
 		 !strcasecmp(variant, "tortoiseplink.exe")) {
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9335e10c2a..b52b8acf98 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -401,6 +401,32 @@ test_expect_success 'single quoted plink.exe in GIT_SSH_COMMAND' '
 	expect_ssh "-v -P 123" myhost src
 '
 
+test_expect_success 'GIT_SSH_VARIANT overrides plink detection' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+	GIT_SSH_VARIANT=ssh \
+	git clone "[myhost:123]:src" ssh-bracket-clone-variant-1 &&
+	expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'ssh.variant overrides plink detection' '
+	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
+	git -c ssh.variant=ssh \
+		clone "[myhost:123]:src" ssh-bracket-clone-variant-2 &&
+	expect_ssh "-p 123" myhost src
+'
+
+test_expect_success 'GIT_SSH_VARIANT overrides plink detection to plink' '
+	GIT_SSH_VARIANT=plink \
+	git clone "[myhost:123]:src" ssh-bracket-clone-variant-3 &&
+	expect_ssh "-P 123" myhost src
+'
+
+test_expect_success 'GIT_SSH_VARIANT overrides plink to tortoiseplink' '
+	GIT_SSH_VARIANT=tortoiseplink \
+	git clone "[myhost:123]:src" ssh-bracket-clone-variant-4 &&
+	expect_ssh "-batch -P 123" myhost src
+'
+
 # Reset the GIT_SSH environment variable for clone tests.
 setup_ssh_wrapper
 
-- 
2.11.1.windows.prerelease.2.9.g3014b57

Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Junio C Hamano]

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

> index 2734b9a1ca..7f1f802396 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -694,10 +694,14 @@ static const char *get_ssh_command(void)
>  static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
>  			      int *port_option, int *needs_batch)
>  {
> -	const char *variant;
> +	const char *variant = getenv("GIT_SSH_VARIANT");
>  	char *p = NULL;
>  
> -	if (!is_cmdline) {
> +	if (variant)
> +		; /* okay, fall through */
> +	else if (!git_config_get_string("ssh.variant", &p))
> +		variant = p;
> +	else if (!is_cmdline) {
>  		p = xstrdup(ssh_command);
>  		variant = basename(p);
>  	} else {
> @@ -717,7 +721,8 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
>  	}
>  
>  	if (!strcasecmp(variant, "plink") ||
> -	    !strcasecmp(variant, "plink.exe"))
> +	    !strcasecmp(variant, "plink.exe") ||
> +	    !strcasecmp(variant, "putty"))

This means that "putty" that appear as the first word of the command
line, not the value configured for ssh.variant or GIT_SSH_VARIANT,
will also trigger the option for "plink", no?

Worse, "plink.exe" configured as the value of "ssh.variant", is
anything other than 'ssh', 'plink', 'putty', or 'tortoiseplink', but
it is not treated as normal ssh, contrary to the documentation.

> +ssh.variant::
> +	Depending on the value of the environment variables `GIT_SSH` or
> +	`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
> +	auto-detects whether to adjust its command-line parameters for use
> +	with plink or tortoiseplink, as opposed to the default (OpenSSH).
> ++
> +The config variable `ssh.variant` can be set to override this auto-detection;
> +valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
> +will be treated as normal ssh. This setting can be overridden via the
> +environment variable `GIT_SSH_VARIANT`.

I was hoping that the "rewrite that retains simplicity" was also
correct, but let's not go in this direction.  The more lines you
touch in a hurry, the worse the code will get, and that is to be
expected.

I'd suggest taking the documentation updates from this series, and
then minimally plug the leak introduced by the previous round,
perhaps like the attached SQUASH.  As I said, GIT_SSH_VARIANT and
ssh.variant are expected to be rare cases, and the case in which
they are set does not have to be optimized if it makes the necessary
change smaller and more likely to be correct.

I think restructuring along the line of 3/4 of this round is very
sensible in the longer term (if anything, handle_ssh_variant() now
really handles ssh variants in all cases, which makes it worthy of
its name, as opposed to the one in the previous round that only
reacts to the overrides).  But it seems that it will take longer to
get such a rewrite right, and my priority is seeing this topic to
add autodetection to GIT_SSH_COMMAND and other smaller topics in the
upcoming -rc0 in a serviceable and correct shape.

The restructuring done by 3/4 can come later after the dust settles,
if somebody cares deeply enough about performance in the rare cases.

 Documentation/config.txt | 14 +++++++++-----
 Documentation/git.txt    |  9 ++++-----
 connect.c                |  9 +++++----
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f2c210f0a0..b88df57ab6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1950,11 +1950,15 @@ matched against are those given directly to Git commands.  This means any URLs
 visited as a result of a redirection do not participate in matching.
 
 ssh.variant::
-	Override the autodetection of plink/tortoiseplink in the SSH
-	command that 'git fetch' and 'git push' use. It can be set to
-	either `ssh`, `plink`, `putty` or `tortoiseplink`. Any other
-	value will be treated as normal ssh. This is useful in case
-	that Git gets this wrong.
+	Depending on the value of the environment variables `GIT_SSH` or
+	`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
+	auto-detects whether to adjust its command-line parameters for use
+	with plink or tortoiseplink, as opposed to the default (OpenSSH).
++
+The config variable `ssh.variant` can be set to override this auto-detection;
+valid values are `ssh`, `plink`, `putty` or `tortoiseplink`. Any other value
+will be treated as normal ssh. This setting can be overridden via the
+environment variable `GIT_SSH_VARIANT`.
 
 i18n.commitEncoding::
 	Character encoding the commit messages are stored in; Git itself
diff --git a/Documentation/git.txt b/Documentation/git.txt
index c322558aa7..a0c6728d1a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1021,11 +1021,10 @@ personal `.ssh/config` file.  Please consult your ssh documentation
 for further details.
 
 `GIT_SSH_VARIANT`::
-	If this environment variable is set, it overrides the autodetection
-	of plink/tortoiseplink in the SSH command that 'git fetch' and 'git
-	push' use. It can be set to either `ssh`, `plink`, `putty` or
-	`tortoiseplink`. Any other value will be treated as normal ssh. This
-	is useful in case that Git gets this wrong.
+	If this environment variable is set, it overrides Git's autodetection
+	whether `GIT_SSH`/`GIT_SSH_COMMAND`/`core.sshCommand` refer to OpenSSH,
+	plink or tortoiseplink. This variable overrides the config setting
+	`ssh.variant` that serves the same purpose.
 
 `GIT_ASKPASS`::
 	If this environment variable is set, then Git commands which need to
diff --git a/connect.c b/connect.c
index 7b4437578b..da51fef9ee 100644
--- a/connect.c
+++ b/connect.c
@@ -693,10 +693,11 @@ static const char *get_ssh_command(void)
 
 static int handle_ssh_variant(int *port_option, int *needs_batch)
 {
-	const char *variant;
+	char *variant;
 
-	if (!(variant = getenv("GIT_SSH_VARIANT")) &&
-		git_config_get_string_const("ssh.variant", &variant))
+	variant = xstrdup_or_null(getenv("GIT_SSH_VARIANT"));
+	if (!variant &&
+	    git_config_get_string("ssh.variant", &variant))
 		return 0;
 
 	if (!strcmp(variant, "plink") || !strcmp(variant, "putty"))
@@ -705,7 +706,7 @@ static int handle_ssh_variant(int *port_option, int *needs_batch)
 		*port_option = 'P';
 		*needs_batch = 1;
 	}
-
+	free(variant);
 	return 1;
 }
 

Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think restructuring along the line of 3/4 of this round is very
> sensible in the longer term (if anything, handle_ssh_variant() now
> really handles ssh variants in all cases, which makes it worthy of
> its name, as opposed to the one in the previous round that only
> reacts to the overrides).  But it seems that it will take longer to
> get such a rewrite right, and my priority is seeing this topic to
> add autodetection to GIT_SSH_COMMAND and other smaller topics in the
> upcoming -rc0 in a serviceable and correct shape.
>
> The restructuring done by 3/4 can come later after the dust settles,
> if somebody cares deeply enough about performance in the rare cases.

Having said all that, because I like the patch 3/4 so much, here is
another way to fix this, and I think (of course I am biased, but...)
the result is easier to grok.  Not only it makes it clear that the
namespace for the actual command names on the command line and the
namespace for the supported values of the configuration are distinct,
it makes it clear that we do not do anything extra when the user
explicitly tells us which variant to use.

This is designed to be squashed into [4/4] of this latest round (as
opposed to the one in the message I am responding to, which is to be
squashed into the previous round).

 connect.c | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/connect.c b/connect.c
index 7f1f802396..12ad8d4c8b 100644
--- a/connect.c
+++ b/connect.c
@@ -691,17 +691,39 @@ static const char *get_ssh_command(void)
 	return NULL;
 }
 
-static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
-			      int *port_option, int *needs_batch)
+static int override_ssh_variant(int *port_option, int *needs_batch)
 {
-	const char *variant = getenv("GIT_SSH_VARIANT");
+	char *variant;
+
+	variant = xstrdup_or_null(getenv("GIT_SSH_VARIANT"));
+	if (!variant &&
+	    git_config_get_string("ssh.variant", &variant))
+		return 0;
+
+	if (!strcmp(variant, "plink") || !strcmp(variant, "putty")) {
+		*port_option = 'P';
+		*needs_batch = 0;
+	} else if (!strcmp(variant, "tortoiseplink")) {
+		*port_option = 'P';
+		*needs_batch = 1;
+	} else {
+		*port_option = 'p';
+		*needs_batch = 0;
+	}
+	free(variant);
+	return 1;
+}
+
+static void handle_ssh_variant(const char *ssh_command, int is_cmdline,
+			       int *port_option, int *needs_batch)
+{
+	const char *variant;
 	char *p = NULL;
 
-	if (variant)
-		; /* okay, fall through */
-	else if (!git_config_get_string("ssh.variant", &p))
-		variant = p;
-	else if (!is_cmdline) {
+	if (override_ssh_variant(port_option, needs_batch))
+		return;
+
+	if (!is_cmdline) {
 		p = xstrdup(ssh_command);
 		variant = basename(p);
 	} else {
@@ -717,7 +739,7 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
 			 */
 			free(ssh_argv);
 		} else
-			return 0;
+			return;
 	}
 
 	if (!strcasecmp(variant, "plink") ||
@@ -730,8 +752,6 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
 		*needs_batch = 1;
 	}
 	free(p);
-
-	return 1;
 }
 
 /*

Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Johannes Schindelin]

Hi Junio,

On Wed, 1 Feb 2017, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I think restructuring along the line of 3/4 of this round is very
> > sensible in the longer term (if anything, handle_ssh_variant() now
> > really handles ssh variants in all cases, which makes it worthy of
> > its name, as opposed to the one in the previous round that only
> > reacts to the overrides).  But it seems that it will take longer to
> > get such a rewrite right, and my priority is seeing this topic to
> > add autodetection to GIT_SSH_COMMAND and other smaller topics in the
> > upcoming -rc0 in a serviceable and correct shape.
> >
> > The restructuring done by 3/4 can come later after the dust settles,
> > if somebody cares deeply enough about performance in the rare cases.
> 
> Having said all that, because I like the patch 3/4 so much, here is
> another way to fix this, and I think (of course I am biased, but...)
> the result is easier to grok.  Not only it makes it clear that the
> namespace for the actual command names on the command line and the
> namespace for the supported values of the configuration are distinct,
> it makes it clear that we do not do anything extra when the user
> explicitly tells us which variant to use.
> 
> This is designed to be squashed into [4/4] of this latest round (as
> opposed to the one in the message I am responding to, which is to be
> squashed into the previous round).
> 
>  connect.c | 42 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/connect.c b/connect.c
> index 7f1f802396..12ad8d4c8b 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -691,17 +691,39 @@ static const char *get_ssh_command(void)
>  	return NULL;
>  }
>  
> -static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
> -			      int *port_option, int *needs_batch)
> +static int override_ssh_variant(int *port_option, int *needs_batch)
>  {
> -	const char *variant = getenv("GIT_SSH_VARIANT");
> +	char *variant;
> +
> +	variant = xstrdup_or_null(getenv("GIT_SSH_VARIANT"));
> +	if (!variant &&
> +	    git_config_get_string("ssh.variant", &variant))
> +		return 0;
> +
> +	if (!strcmp(variant, "plink") || !strcmp(variant, "putty")) {
> +		*port_option = 'P';
> +		*needs_batch = 0;
> +	} else if (!strcmp(variant, "tortoiseplink")) {
> +		*port_option = 'P';
> +		*needs_batch = 1;
> +	} else {
> +		*port_option = 'p';
> +		*needs_batch = 0;
> +	}
> +	free(variant);
> +	return 1;
> +}
> +
> +static void handle_ssh_variant(const char *ssh_command, int is_cmdline,
> +			       int *port_option, int *needs_batch)
> +{
> +	const char *variant;
>  	char *p = NULL;
>  
> -	if (variant)
> -		; /* okay, fall through */
> -	else if (!git_config_get_string("ssh.variant", &p))
> -		variant = p;
> -	else if (!is_cmdline) {
> +	if (override_ssh_variant(port_option, needs_batch))
> +		return;
> +
> +	if (!is_cmdline) {
>  		p = xstrdup(ssh_command);
>  		variant = basename(p);
>  	} else {
> @@ -717,7 +739,7 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
>  			 */
>  			free(ssh_argv);
>  		} else
> -			return 0;
> +			return;
>  	}
>  
>  	if (!strcasecmp(variant, "plink") ||
> @@ -730,8 +752,6 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
>  		*needs_batch = 1;
>  	}
>  	free(p);
> -
> -	return 1;
>  }
>  
>  /*

That leaves the "putty" case in handle_ssh_variant(), does it not? Was it
not your specific objection that that is the case?

No worries, I will let this simmer for a while. Your fixup has a lot of
duplicated code (so much for maintainability as an important goal... ;-))
and I will have to think about it. My immediate thinking is to *not*
duplicate code, strip the .exe extension directly after calling basename()
in the cases where we handle commands, and handle "putty" in the other
cases by replacing it with the string "plink".

But as I said, this will simmer for a while.

Ciao,
Johannes

Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Junio C Hamano]

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

> That leaves the "putty" case in handle_ssh_variant(), does it not? Was it
> not your specific objection that that is the case?

Yup, you can remove that while you reroll.

> No worries, I will let this simmer for a while. Your fixup has a lot of
> duplicated code (so much for maintainability as an important goal... ;-))
> and I will have to think about it. My immediate thinking is to *not*
> duplicate code,...

You need to realize that the namespaces of the configuration and the
command names are distinct.  There is no code duplication.

Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Johannes Schindelin]

Hi Junio,

On Wed, 1 Feb 2017, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > That leaves the "putty" case in handle_ssh_variant(), does it not? Was it
> > not your specific objection that that is the case?
> 
> Yup, you can remove that while you reroll.
> 
> > No worries, I will let this simmer for a while. Your fixup has a lot of
> > duplicated code (so much for maintainability as an important goal... ;-))
> > and I will have to think about it. My immediate thinking is to *not*
> > duplicate code,...
> 
> You need to realize that the namespaces of the configuration and the
> command names are distinct.  There is no code duplication.

Since your 2/4 did away with the "plink" and "tortoiseplink" values in
favor of "port_option" and "batch_option", there is a duplication of logic
which I tried to undo in v3 and which you reintroduce in your fixup.

Maybe that refactoring was not so smart to begin with, and we should have
instead modified the code to use an enum instead and keep the original
conditionals instead of setting a port_option and a batch_option
explicitly.

Ciao,
Johannes

Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> That leaves the "putty" case in handle_ssh_variant(), does it not? Was it
>> not your specific objection that that is the case?
>
> Yup, you can remove that while you reroll.
>
>> No worries, I will let this simmer for a while. Your fixup has a lot of
>> duplicated code (so much for maintainability as an important goal... ;-))
>> and I will have to think about it. My immediate thinking is to *not*
>> duplicate code,...
>
> You need to realize that the namespaces of the configuration and the
> command names are distinct.  There is no code duplication.

To explain this a bit, there is no reason why allowed values for
SSH_VARIANT must be "putty" and "tortoiseplink".  An alternative
design could be "port_option=-p,needs_batch=yes" and it may be more
logical and futureproof if a variant of tortoiseplink decides to use
"-p" instead of "-P" and still require "-batch".

Prematurely attempting to share code, only because the current
vocabularies for two distinct concepts happen to overlap, is not
de-duplicating the code for maintainability.  It is adding
unnecessary work other people need to do in the future when they
want to extend the system.

Re: [PATCH v3 4/4] connect: Add the envvar GIT_SSH_VARIANT and ssh.variant config

[Johannes Schindelin]

Hi Junio,

On Wed, 1 Feb 2017, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> >> That leaves the "putty" case in handle_ssh_variant(), does it not? Was it
> >> not your specific objection that that is the case?
> >
> > Yup, you can remove that while you reroll.
> >
> >> No worries, I will let this simmer for a while. Your fixup has a lot of
> >> duplicated code (so much for maintainability as an important goal... ;-))
> >> and I will have to think about it. My immediate thinking is to *not*
> >> duplicate code,...
> >
> > You need to realize that the namespaces of the configuration and the
> > command names are distinct.  There is no code duplication.
> 
> To explain this a bit, there is no reason why allowed values for
> SSH_VARIANT must be "putty" and "tortoiseplink".  An alternative
> design could be "port_option=-p,needs_batch=yes" and it may be more
> logical and futureproof if a variant of tortoiseplink decides to use
> "-p" instead of "-P" and still require "-batch".
> 
> Prematurely attempting to share code, only because the current
> vocabularies for two distinct concepts happen to overlap, is not
> de-duplicating the code for maintainability.  It is adding
> unnecessary work other people need to do in the future when they
> want to extend the system.

Except, of course, that your hypothetical port_option and needs_batch
settings would be handled at a different point altogether.

I sense very strongly that this discussion has taken a very emotional
turn, which is detrimental to the quality. So let's take a break here.

Ciao,
Johannes

Re: [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND

[Junio C Hamano]

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

> It is quite preposterous to call this an "iteration" of the patch
> series, because the code is so different now. I say this because I want
> to caution that this code has not been tested as thoroughly, by far, as
> the first iteration.
>
> The primary purpose of code review is correctness, everything else is
> either a consequence of it, or a means to make reviewing easier.

You are utterly wrong here.

The primary purpose of code review is to spot and correct the
problems the submitter has missed.  The problems can span from
stupid bugs that affect correctness to maintainability, to design
mistakes, to clarity of explanation for both end users and future
developers.

Among them, correctness problems are, as long as the problem to be
solved is specified clearly enough, the easiest to spot by the
submitter before the patch is sent out.  The submitter is focused on
solving one problem, and if the updated code does not even work as
the submitter advertises it would, that can be caught by the
submitter before the patch is even sent out.  

Of course, humans are not perfect, and catching correctness problems
is important, but that is not the only (let alone primary) thing.

When a submitter has been focusing on solving one problem, it is
easy to lose the larger picture and to end up adding something that
may be "correct" (in the sense of "works as advertised by the
submitter") but does not fit well with the rest of the system, or
covers some use cases but misses other important and related use
cases.  If the "does not fit well" surfaces to the end user level,
that would become a design problem.  If it affects the future
developers, that would become a maintainability problem.

Compared to the correctness issue, these are much harder to spot by
the submitter alone, who focused so intensely to get his code
"correct".  The review process is of greater value to spot these
issues.

I've already read and commented on the series; as I said, I think
the rewrite in 3/4 makes the resulting code much easier to read, and
with the fix-up I just sent out, I think the end result is correct,
works as advertised, the way it is advertised is clear to end users,
and is maintainable.  

But I do share your uneasiness about the "new-ness" of the code.

Re: [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND

[Johannes Schindelin]

Hi Junio,

On Wed, 1 Feb 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > It is quite preposterous to call this an "iteration" of the patch
> > series, because the code is so different now. I say this because I want
> > to caution that this code has not been tested as thoroughly, by far, as
> > the first iteration.
> >
> > The primary purpose of code review is correctness, everything else is
> > either a consequence of it, or a means to make reviewing easier.
> 
> You are utterly wrong here.
> 
> The primary purpose of code review is to spot and correct the
> problems the submitter has missed.  The problems can span from
> stupid bugs that affect correctness to maintainability, to design
> mistakes, to clarity of explanation for both end users and future
> developers.
> 
> Among them, correctness problems are, as long as the problem to be
> solved is specified clearly enough, the easiest to spot by the
> submitter before the patch is sent out.  The submitter is focused on
> solving one problem, and if the updated code does not even work as
> the submitter advertises it would, that can be caught by the
> submitter before the patch is even sent out.  
> 
> Of course, humans are not perfect, and catching correctness problems
> is important, but that is not the only (let alone primary) thing.
> 
> When a submitter has been focusing on solving one problem, it is
> easy to lose the larger picture and to end up adding something that
> may be "correct" (in the sense of "works as advertised by the
> submitter") but does not fit well with the rest of the system, or
> covers some use cases but misses other important and related use
> cases.  If the "does not fit well" surfaces to the end user level,
> that would become a design problem.  If it affects the future
> developers, that would become a maintainability problem.
> 
> Compared to the correctness issue, these are much harder to spot by
> the submitter alone, who focused so intensely to get his code
> "correct".  The review process is of greater value to spot these
> issues.

We will never agree on this.

From my perspective, design, explanation and maintainability are a
consequence of making it easy for reviewers to spot where the code is
incorrect.

And correctness is not covered by "the submitter tested this". Correctness
includes all the corner cases, where the "many eyes make bugs shallow"
really shines.

I'd rather have reviewers find bugs than users.

I will *never* be a fan of a review process that pushes correctness to a
back seat (yes, it is much harder than spotting typos or lines longer than
80 columns per row, but the ultimate goal is to deliver value to the end
user, not to make life easy for the maintainer).

Ciao,
Johannes

Re: [PATCH v3 0/4] Handle PuTTY (plink/tortoiseplink) even in GIT_SSH_COMMAND

[Junio C Hamano]

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

>> Compared to the correctness issue, these are much harder to spot by
>> the submitter alone, who focused so intensely to get his code
>> "correct".  The review process is of greater value to spot these
>> issues.
>
> We will never agree on this.

That's too bad.

> From my perspective, design, explanation and maintainability are a
> consequence of making it easy for reviewers to spot where the code is
> incorrect.
>
> And correctness is not covered by "the submitter tested this". Correctness
> includes all the corner cases, where the "many eyes make bugs shallow"
> really shines.
>
> I'd rather have reviewers find bugs than users.

I'd rather have submitters find bugs than reviewers.

> I will *never* be a fan of a review process that pushes correctness to a
> back seat (yes, it is much harder than spotting typos or lines longer than
> 80 columns per row, but the ultimate goal is to deliver value to the end
> user, not to make life easy for the maintainer).

Did I ever say correctness is pushed to a back seat?

I said that it is easier to spot correctness issues for you as a
submitter than other kinds of issues without outside help (and
implied that if you are a diligent contributor, you should aim for,
and you should be able to achieve, a patch series where correctness
issues do not need to be pointed out).  But other higher level
issues are harder for any submitter to spot (regardless of
experience and competence of the submitter), because one gets so
married to one's own code, design and worldview.  And that is why
"review is primarily to spot bugs" can never be a correct viewpoint.

A reviewee needs to be prepared to accept review comments on higher
level issues, even more readily than comments on correctness issues,
because it is too easy to be constrained by early decisions one has
already made while preparing a patch series and become blind to
bigger picture after staring one's own new code for number of hours.
Higher level issues can be more easily spotted by reviewers, whose
eyes are still fresh to the series.