Re: [ANNOUNCE] Git v2.16.0-rc0

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

Jonathan Nieder <jrnieder@gmail.com> writes:

> It's good you caught this flaw in the detection.  Would something like
> the following make sense?  If so, I can resend with a commit message
> and tests tomorrow or the day after.

So the idea is to keep the 'simple' for implementations that do not
support OpenSSH options, but declare that the auto-detection was
overly conservative and assume that -4/6/p are supported by
everybody?

This change means that those who were meant to be helped by the
original change, that introduced 'simple' and made the (overly
conservative) auto-detection, would now have to explicitly set
ssh.variant to simple, because otherwise they will be passed one of
these three options.  Am I reading the intention of the change
correctly?  If so, I tend to agree that it is lesser of the two
evils to make sure things continue to work for older openssh users
with their current setting like this patch does, even with the cost
of telling users with implementations that do not honor -4/6/p to
set things up manually, I guess.

Thanks, both, for digging this issue through.

>
> diff --git i/Documentation/config.txt w/Documentation/config.txt
> index 64c1dbba94..75eafd8db6 100644
> --- i/Documentation/config.txt
> +++ w/Documentation/config.txt
> @@ -2118,8 +2118,8 @@ ssh.variant::
>  	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).
> +	OpenSSH options if that is successful or a conservative set of
> +	OpenSSH-style options 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`,
> diff --git i/connect.c w/connect.c
> index c3a014c5ba..3784c2be53 100644
> --- i/connect.c
> +++ w/connect.c
> @@ -941,10 +941,9 @@ static void push_ssh_options(struct argv_array *args, struct argv_array *env,
>  
>  	if (flags & CONNECT_IPV4) {
>  		switch (variant) {
> -		case VARIANT_AUTO:
> -			BUG("VARIANT_AUTO passed to push_ssh_options");
>  		case VARIANT_SIMPLE:
>  			die("ssh variant 'simple' does not support -4");
> +		case VARIANT_AUTO:
>  		case VARIANT_SSH:
>  		case VARIANT_PLINK:
>  		case VARIANT_PUTTY:
> @@ -953,10 +952,9 @@ static void push_ssh_options(struct argv_array *args, struct argv_array *env,
>  		}
>  	} else if (flags & CONNECT_IPV6) {
>  		switch (variant) {
> -		case VARIANT_AUTO:
> -			BUG("VARIANT_AUTO passed to push_ssh_options");
>  		case VARIANT_SIMPLE:
>  			die("ssh variant 'simple' does not support -6");
> +		case VARIANT_AUTO:
>  		case VARIANT_SSH:
>  		case VARIANT_PLINK:
>  		case VARIANT_PUTTY:
> @@ -970,10 +968,9 @@ static void push_ssh_options(struct argv_array *args, struct argv_array *env,
>  
>  	if (port) {
>  		switch (variant) {
> -		case VARIANT_AUTO:
> -			BUG("VARIANT_AUTO passed to push_ssh_options");
>  		case VARIANT_SIMPLE:
>  			die("ssh variant 'simple' does not support setting port");
> +		case VARIANT_AUTO:
>  		case VARIANT_SSH:
>  			argv_array_push(args, "-p");
>  			break;
> @@ -1026,7 +1023,7 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
>  				 VARIANT_SSH, port, flags);
>  		argv_array_push(&detect.args, ssh_host);
>  
> -		variant = run_command(&detect) ? VARIANT_SIMPLE : VARIANT_SSH;
> +		variant = run_command(&detect) ? VARIANT_AUTO : VARIANT_SSH;
>  	}
>  
>  	argv_array_push(&conn->args, ssh);
> diff --git i/t/t5601-clone.sh w/t/t5601-clone.sh
> index 0f895478f0..0224edc85b 100755
> --- i/t/t5601-clone.sh
> +++ w/t/t5601-clone.sh
> @@ -365,6 +365,11 @@ test_expect_success 'OpenSSH variant passes -4' '
>  	expect_ssh "-4 -p 123" myhost src
>  '
>  
> +test_expect_success 'OpenSSH passes GIT_PROTOCOL envvar' '
> +	git -c protocol.version=1 clone [myhost:123]:src ssh-v1-clone &&
> +	expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
> +'
> +
>  test_expect_success 'variant can be overridden' '
>  	copy_ssh_wrapper_as "$TRASH_DIRECTORY/putty" &&
>  	git -c ssh.variant=putty clone -4 "[myhost:123]:src" ssh-putty-clone &&
> @@ -377,19 +382,32 @@ test_expect_success 'variant=auto picks based on basename' '
>  	expect_ssh "-4 -P 123" myhost src
>  '
>  
> -test_expect_success 'simple does not support -4/-6' '
> +test_expect_success 'variant=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
> +	test_must_fail git -c ssh.variant=simple clone -4 "myhost:src" ssh-4-clone-simple
>  '
>  
> -test_expect_success 'simple does not support port' '
> +test_expect_success 'variant=simple does not support port' '
>  	copy_ssh_wrapper_as "$TRASH_DIRECTORY/simple" &&
> -	test_must_fail git clone "[myhost:123]:src" ssh-bracket-clone-simple
> +	test_must_fail git -c ssh.variant=simple clone "[myhost:123]:src" ssh-bracket-clone-simple
> +'
> +
> +test_expect_success 'old OpenSSH passes -4 and -p' '
> +	copy_ssh_wrapper_as "$TRASH_DIRECTORY/old" &&
> +	git -c ssh.variant=auto clone -4 "[myhost:123]:src" old-ssh-clone &&
> +	expect_ssh "-4 -p 123" myhost src
>  '
>  
> -test_expect_success 'uplink is treated as simple' '
> +test_expect_success 'old OpenSSH does not pass GIT_PROTOCOL envvar' '
> +	copy_ssh_wrapper_as "$TRASH_DIRECTORY/old" &&
> +	git -c protocol.version=1 -c ssh.variant=auto clone "[myhost:123]:src" old-ssh-protocol-v1 &&
> +	expect_ssh "-p 123" myhost src
> +'
> +
> +test_expect_success 'uplink is treated as old OpenSSH' '
>  	copy_ssh_wrapper_as "$TRASH_DIRECTORY/uplink" &&
> -	test_must_fail git clone "[myhost:123]:src" ssh-bracket-clone-uplink &&
> +	git -c protocol.version=1 clone "[myhost:123]:src" ssh-bracket-clone-uplink &&
> +	expect_ssh "-p 123" myhost src &&
>  	git clone "myhost:src" ssh-clone-uplink &&
>  	expect_ssh myhost src
>  '
> @@ -405,8 +423,8 @@ test_expect_success 'OpenSSH-like uplink is treated as ssh' '
>  	test_when_finished "rm -f \"\$TRASH_DIRECTORY/uplink\"" &&
>  	GIT_SSH="$TRASH_DIRECTORY/uplink" &&
>  	test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\"" &&
> -	git clone "[myhost:123]:src" ssh-bracket-clone-sshlike-uplink &&
> -	expect_ssh "-p 123" myhost src
> +	git -c protocol.version=1 clone "[myhost:123]:src" ssh-bracket-clone-sshlike-uplink &&
> +	expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
>  '
>  
>  test_expect_success 'plink is treated specially (as putty)' '