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

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

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.