Re: [PATCH 3/5] ssh: 'auto' variant to select between 'ssh' and 'simple'

[Brandon Williams <bmwill@google.com>]

On 10/23, Jonathan Nieder wrote:
> Hi,
> 
> Jonathan Tan wrote:
> 
> > The new documentation seems to imply that setting ssh.variant (or
> > GIT_SSH_VARIANT) to "auto" is equivalent to not setting it at all, but
> > looking at the code, it doesn't seem to be the case (not setting it at
> > all invokes checking the first word of core.sshCommand, and only uses
> > VARIANT_AUTO if that check is inconclusive, whereas setting
> > ssh.variant=auto skips the core.sshCommand check entirely).
> >
> > Maybe document ssh.variant as follows:
> >
> >     If unset, Git will determine the command-line arguments to use based
> >     on the basename of the configured SSH command (through the
> >     environment variable `GIT_SSH` or `GIT_SSH_COMMAND`, or the config
> >     setting `core.sshCommand`). If the basename is unrecognized, Git
> >     will attempt to detect support of OpenSSH options by first invoking
> >     the configured SSH command with the `-G` (print configuration) flag,
> >     and will subsequently use OpenSSH options (upon success) or no
> >     options besides the host (upon failure).
> >
> >     If set, Git will not do any auto-detection based on the basename of
> >     the configured SSH command. This can be set to `ssh` (OpenSSH
> >     options), `plink`, `putty`, `tortoiseplink`, `simple` (no options
> >     besides the host), or `auto` (the detection with `-G` as described
> >     above). If set to any other value, Git behaves as if this is set to
> >     `ssh`.
> 
> Good point.  Brandon noticed something similar as well.
> 
> Separately from how to document it, what do you think a good behavior
> would be?  Should the "auto" configuration trigger command line based
> detection just like no configuration at all?  Should the "auto" value
> for configuration be removed and that behavior restricted to the
> no-configuration case?
> 
> I'm tempted to go with the former, which would look like the following.
> What do you think?

As a user having some variant as 'auto' doesn't make much sense, i mean
isn't that exactly what the default behavior is?  Check if my ssh
command matches existing variants and go with that.  What you are
proposing is the make the existing auto detection better (yay!) though I
don't know if it warrants adding a new variant all together.

Instead it may be better to stick this new improved detection at the end
of the existing variant discovery function 'determine_ssh_variant()' as
a last ditch effort to figure out the variant.  That way we don't have
an extra variant type that can be configured and eliminates some of the
additional code in the switch statements to handle that enum value
(though that isn't really that big of a deal).

> 
> If this looks good, I can reroll in a moment.
> 
> diff --git i/Documentation/config.txt w/Documentation/config.txt
> index 4a16b324f0..6dffa4aa3d 100644
> --- i/Documentation/config.txt
> +++ w/Documentation/config.txt
> @@ -2081,20 +2081,21 @@ 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::
> -	Depending on the value of the environment variables `GIT_SSH` or
> -	`GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
> -	auto-detects whether to pass command-line parameters for use
> -	with a simple wrapper script (simple), OpenSSH (ssh), plink, or
> -	tortoiseplink.
> -+
> -The default is `auto`, which means to auto-detect whether the ssh command
> -implements OpenSSH options using the `-G` (print configuration) option.
> -If the ssh command supports OpenSSH options, it then behaves like `ssh`;
> -otherwise, it behaves like `simple`.
> -+
> -The config variable `ssh.variant` can be set to override this auto-detection;
> -valid values are `ssh`, `simple`, `plink`, `putty`, `tortoiseplink`, and
> -`auto`.  Any other value will be treated as normal ssh.  This setting can be
> +	By default, Git determines the command line arguments to use
> +	based on the basename of the configured SSH command (configured
> +	using the environment variable `GIT_SSH` or `GIT_SSH_COMMAND` or
> +	the config setting `core.sshCommand`). If the basename is
> +	unrecognized, Git will attempt to detect support of OpenSSH
> +	options by first invoking the configured SSH command with the
> +	`-G` (print configuration) option and will subsequently use
> +	OpenSSH options (if that is successful) or no options besides
> +	the host and remote command (if it fails).
> ++
> +The config variable `ssh.variant` can be set to override this detection:
> +valid values are `ssh` (to use OpenSSH options), `plink`, `putty`,
> +`tortoiseplink`, `simple` (no options except the host and remote command).
> +The default auto-detection can be explicitly requested using the value
> +`auto`.  Any other value is treated as `ssh`.  This setting can also be
>  overridden via the environment variable `GIT_SSH_VARIANT`.
>  +
>  The current command-line parameters used for each variant are as
> diff --git i/connect.c w/connect.c
> index 98f2d9ce57..06bcd3981e 100644
> --- i/connect.c
> +++ w/connect.c
> @@ -785,12 +785,12 @@ enum ssh_variant {
>  	VARIANT_TORTOISEPLINK,
>  };
>  
> -static int override_ssh_variant(enum ssh_variant *ssh_variant)
> +static void override_ssh_variant(enum ssh_variant *ssh_variant)
>  {
>  	const char *variant = getenv("GIT_SSH_VARIANT");
>  
>  	if (!variant && git_config_get_string_const("ssh.variant", &variant))
> -		return 0;
> +		return;
>  
>  	if (!strcmp(variant, "auto"))
>  		*ssh_variant = VARIANT_AUTO;
> @@ -804,8 +804,6 @@ static int override_ssh_variant(enum ssh_variant *ssh_variant)
>  		*ssh_variant = VARIANT_SIMPLE;
>  	else
>  		*ssh_variant = VARIANT_SSH;
> -
> -	return 1;
>  }
>  
>  static enum ssh_variant determine_ssh_variant(const char *ssh_command,
> @@ -815,7 +813,9 @@ static enum ssh_variant determine_ssh_variant(const char *ssh_command,
>  	const char *variant;
>  	char *p = NULL;
>  
> -	if (override_ssh_variant(&ssh_variant))
> +	override_ssh_variant(&ssh_variant);
> +
> +	if (ssh_variant != VARIANT_AUTO)
>  		return ssh_variant;
>  
>  	if (!is_cmdline) {
> diff --git i/t/t5601-clone.sh w/t/t5601-clone.sh
> index 11fa516997..f9a2ae84c7 100755
> --- i/t/t5601-clone.sh
> +++ w/t/t5601-clone.sh
> @@ -373,6 +373,12 @@ test_expect_success 'variant can be overridden' '
>  	expect_ssh "-4 -P 123" myhost src
>  '
>  
> +test_expect_success 'variant=auto picks based on basename' '
> +	copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
> +	git -c ssh.variant=auto clone -4 "[myhost:123]:src" ssh-auto-clone &&
> +	expect_ssh "-4 -P 123" myhost src
> +'
> +
>  test_expect_success 'simple does not support -4/-6' '
>  	copy_ssh_wrapper_as "$TRASH_DIRECTORY/simple" &&
>  	test_must_fail git clone -4 "myhost:src" ssh-4-clone-simple

-- 
Brandon Williams