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

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

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