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

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > 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.
> 
> Having said all that, because I like the patch 3/4 so much, here is
> another way to fix this, and I think (of course I am biased, but...)
> the result is easier to grok.  Not only it makes it clear that the
> namespace for the actual command names on the command line and the
> namespace for the supported values of the configuration are distinct,
> it makes it clear that we do not do anything extra when the user
> explicitly tells us which variant to use.
> 
> This is designed to be squashed into [4/4] of this latest round (as
> opposed to the one in the message I am responding to, which is to be
> squashed into the previous round).
> 
>  connect.c | 42 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/connect.c b/connect.c
> index 7f1f802396..12ad8d4c8b 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -691,17 +691,39 @@ static const char *get_ssh_command(void)
>  	return NULL;
>  }
>  
> -static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
> -			      int *port_option, int *needs_batch)
> +static int override_ssh_variant(int *port_option, int *needs_batch)
>  {
> -	const char *variant = getenv("GIT_SSH_VARIANT");
> +	char *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")) {
> +		*port_option = 'P';
> +		*needs_batch = 0;
> +	} else if (!strcmp(variant, "tortoiseplink")) {
> +		*port_option = 'P';
> +		*needs_batch = 1;
> +	} else {
> +		*port_option = 'p';
> +		*needs_batch = 0;
> +	}
> +	free(variant);
> +	return 1;
> +}
> +
> +static void handle_ssh_variant(const char *ssh_command, int is_cmdline,
> +			       int *port_option, int *needs_batch)
> +{
> +	const char *variant;
>  	char *p = NULL;
>  
> -	if (variant)
> -		; /* okay, fall through */
> -	else if (!git_config_get_string("ssh.variant", &p))
> -		variant = p;
> -	else if (!is_cmdline) {
> +	if (override_ssh_variant(port_option, needs_batch))
> +		return;
> +
> +	if (!is_cmdline) {
>  		p = xstrdup(ssh_command);
>  		variant = basename(p);
>  	} else {
> @@ -717,7 +739,7 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
>  			 */
>  			free(ssh_argv);
>  		} else
> -			return 0;
> +			return;
>  	}
>  
>  	if (!strcasecmp(variant, "plink") ||
> @@ -730,8 +752,6 @@ static int handle_ssh_variant(const char *ssh_command, int is_cmdline,
>  		*needs_batch = 1;
>  	}
>  	free(p);
> -
> -	return 1;
>  }
>  
>  /*