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

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

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