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

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

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;
 }
 
 /*