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 27, 2017, 18:17 UTC
Message-ID: <xmqqpoj8z7su.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqo9yt4o5i.fsf@gitster.mtv.corp.google.com>

Just to save us extra round-trip.

Junio C Hamano <gitster@pobox.com> writes:

Show 29 quoted lines

>> +`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)
>> +{
>> ...
>> +}
> ...
>
> The string that came from the configuration must be freed, no?  That
> is what I recall in Peff's comment yesterday.

The leak needs plugging in some way.

Because this thing is merely an escape hatch that somebody who needs it needs to use it always (as opposed to one-shot basis), we do not need to support the environment variable and go with only the configuration, which may make it easier to plug the leak.

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

This _should_ above is a lot weaker than _must_.

IOW, I think it is acceptable to always split GIT_SSH_COMMAND into tokens before we realize that the user used the escape hatch and the splitting was a wasted effort. This is exactly because this thing is an escape hatch that is expected to be rarely used. Of course, if the "wasted effort" can be eliminated without sacrificing the simplicity of the code, that is fine as well.