Re: What's cooking in git.git (Feb 2017, #02; Mon, 6)

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

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

> (And that would have to be handled at a different point, as I
> had pointed out, so that suggested preparation would most likely not help
> at all.)

I did not think "that would have to be handled at a different point"
is correct at all, if by "a point" you meant "a location in the
code" [*1*].  If we want to make it configurable in a more detailed
way by directly allowing to override port_option and needs_batch
separately, you would do that in override_ssh_variant(), without
touching handle_ssh_variant() in the refactored code.  That way, you
do not have to worry about breaking the auto detection based on the
command name.


[Footnote]

*1* Or did you mean "point in time", aka "let's do it outside the
    scope of this patch series"?

Let's not keep it as a SQUASH??? proposal, but as a separate hot-fix
follow-up patch.

-- >8 --
Subject: connect.c: stop conflating ssh command names and overrides

dd33e07766 ("connect: Add the envvar GIT_SSH_VARIANT and ssh.variant
config", 2017-02-01) attempted to add support for configuration and
environment variable to override the different handling of
port_option and needs_batch settings suitable for variants of the
ssh implementation that was autodetected by looking at the ssh
command name.  Because it piggybacked on the code that turns command
name to specific override (e.g. "plink.exe" and "plink" means
port_option needs to be set to 'P' instead of the default 'p'), yet
it defined a separate namespace for these overrides (e.g. "putty"
can be usable to signal that port_option needs to be 'P'), however,
it made the auto-detection based on the command name less robust
(e.g. the code now accepts "putty" as a SSH command name and applies
the same override).

Separate the code that interprets the override that was read from
the configuration & environment from the original code that handles
the command names, as they are in separate namespaces, to fix this
confusion.

This incidentally also makes it easier for future enhancement of the
override syntax (e.g. "port_option=p,needs_batch=1" may want to be
accepted as a more explicit syntax) without affecting the code for
auto-detection based on the command name.

While at it, update the return type of the handle_ssh_variant()
helper function to void; the caller does not use it, and the
function does not return any meaningful value.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 connect.c | 45 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/connect.c b/connect.c
index 7f1f802396..7d65c1c736 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,12 +739,11 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
 			 */
 			free(ssh_argv);
 		} else
-			return 0;
+			return;
 	}
 
 	if (!strcasecmp(variant, "plink") ||
-	    !strcasecmp(variant, "plink.exe") ||
-	    !strcasecmp(variant, "putty"))
+	    !strcasecmp(variant, "plink.exe"))
 		*port_option = 'P';
 	else if (!strcasecmp(variant, "tortoiseplink") ||
 		 !strcasecmp(variant, "tortoiseplink.exe")) {
@@ -730,8 +751,6 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
 		*needs_batch = 1;
 	}
 	free(p);
-
-	return 1;
 }
 
 /*
-- 
2.12.0-rc0-221-g3e954cf1aa