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

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

Just to save us extra round-trip.

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

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

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