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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 26, 2017, 19:27 UTC
Message-ID: <xmqqo9yt4o5i.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <3d451f2c357a3fd7f0b0e4b427548553d7d05306.1485442231.git.johannes.schindelin@gmx.de>

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

Show 15 quoted lines

> 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.

Show 14 quoted lines

> 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).

Show 25 quoted lines

> 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.

Show 7 quoted lines

> @@ -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.