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

[Johannes Schindelin]

Hi Junio,

On Mon, 30 Oct 2017, Junio C Hamano wrote:

> Jonathan Nieder  writes:
> 
> > I have other changes to make when rerolling anyway (from Junio's
> > review), so no need for a followup patch.  Will fix this in the
> > reroll today.
> >
> > Thanks for catching and diagnosing this, Dscho!
> 
> In the meantime, I've queued this from Dscho; please take it into
> consideration when you reroll.
> 
> Thanks.

Thanks for sending it out as an email.

Ciao,
Dscho

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

[Junio C Hamano]

Jonathan Nieder  writes:

> I have other changes to make when rerolling anyway (from Junio's
> review), so no need for a followup patch.  Will fix this in the
> reroll today.
>
> Thanks for catching and diagnosing this, Dscho!

In the meantime, I've queued this from Dscho; please take it into
consideration when you reroll.

Thanks.

-- >8 --
From: Johannes Schindelin 
Date: Sun, 29 Oct 2017 16:12:46 +0100
Subject: [PATCH] fixup! ssh: 'auto' variant to select between 'ssh' and 'simple'

This is needed because on Windows, if `uplink.exe` exists, the MSYS2
Bash will overwrite that when redirecting via `>uplink`.

Signed-off-by: Johannes Schindelin 
---
 t/t5601-clone.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index f9a2ae84c7..534eb21915 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -391,6 +391,7 @@ test_expect_success 'simple does not support port' '
 
 test_expect_success 'uplink is treated as simple' '
copy_ssh_wrapper_as "$TRASH_DIRECTORY/uplink" &&
+   test_when_finished "rm \"$TRASH_DIRECTORY/uplink$X\"" &&
test_must_fail git clone "[myhost:123]:src" ssh-bracket-clone-uplink &&
git clone "myhost:src" ssh-clone-uplink &&
expect_ssh myhost src
-- 
2.15.0-rc2-267-g7d3ed0014a

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

[Jonathan Nieder]

Hi,

Stefan Beller wrote:
> On Wed, Oct 25, 2017 at 5:51 AM, Johannes Schindelin
>  wrote:

>> This breaks on Windows. And it is not immediately obvious how so, so let
>> me explain:
[nice explanation snipped]
>> As a consequence, the test fails. Could you please squash in this, to fix
>> the test on Windows?
>
> This explanation is in detail and would even make a good commit message
> for a follow up commit. (Squashing just that line would loose the explanation
> as I suspect the original commit will not dedicate so much text to
> this single line)

I have other changes to make when rerolling anyway (from Junio's
review), so no need for a followup patch.  Will fix this in the
reroll today.

Thanks for catching and diagnosing this, Dscho!

Jonathan

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

[Stefan Beller]

On Wed, Oct 25, 2017 at 5:51 AM, Johannes Schindelin
 wrote:
> Hi Jonathan,
>
> [I only saw that you replied to 3/5 with v2 after writing this reply, but
> it would apply to v2's 3/5, too]
>
> On Mon, 23 Oct 2017, Jonathan Nieder wrote:
>
>> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
>> index 86811a0c35..fd94dd40d2 100755
>> --- a/t/t5601-clone.sh
>> +++ b/t/t5601-clone.sh
>> @@ -384,6 +384,20 @@ test_expect_success 'uplink is treated as simple' '
>>   expect_ssh myhost src
>>  '
>>
>> +test_expect_success 'OpenSSH-like uplink is treated as ssh' '
>> + write_script "$TRASH_DIRECTORY/uplink" <<-EOF &&
>> + if test "\$1" = "-G"
>> + then
>> + exit 0
>> + fi &&
>> + exec "\$TRASH_DIRECTORY/ssh$X" "\$@"
>> + EOF
>> + GIT_SSH="$TRASH_DIRECTORY/uplink" &&
>> + export GIT_SSH &&
>> + git clone "[myhost:123]:src" ssh-bracket-clone-sshlike-uplink &&
>> + expect_ssh "-p 123" myhost src
>> +'
>> +
>>  test_expect_success 'plink is treated specially (as putty)' '
>>   copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
>>   git clone "[myhost:123]:src" ssh-bracket-clone-plink-0 &&
>
> This breaks on Windows. And it is not immediately obvious how so, so let
> me explain:
>
> As you know, on Windows there is no executable flag. There is the .exe
> file extension to indicate an executable (and .com and .bat and .cmd are
> also handled as executable, at least as executable script).
>
> Now, what happens if you call "abc" in the MSYS2 Bash and there is no
> script called "abc" but an executable called "abc.exe" in the PATH? Why,
> of course we execute that executable. It has to, because if "abc.exe"
> would be renamed into "abc", it would not work any longer.
>
> That is also the reason why that "copy_ssh_wrapper" helper function
> automatically appends that .exe file suffix on Windows: it has to.
>
> Every workaround breaks down at some point, and this workaround is no
> exception. What should the MSYS2 Bash do if asked to overwrite "abc" and
> there is only an "abc.exe"? It actually overwrites "abc.exe" and moves on.
>
> And this is where we are here: the previous test case copied the ssh
> wrapper as "uplink". Except on Windows, it is "uplink.exe". And your newly
> introduced test case overwrites it. And then it tells Git specifically to
> look for "uplink", and Git does *not* append that .exe suffix
> automatically as the MSYS2 Bash would do, because git.exe is not intended
> to work MSYS2-like.
>
> As a consequence, the test fails. Could you please squash in this, to fix
> the test on Windows?

This explanation is in detail and would even make a good commit message
for a follow up commit. (Squashing just that line would loose the explanation
as I suspect the original commit will not dedicate so much text to
this single line)

>
> -- snipsnap --
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index ec4b17bca62..1afcbd00617 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -393,6 +393,7 @@ test_expect_success 'simple does not support port' '
>
>  test_expect_success 'uplink is treated as simple' '
> copy_ssh_wrapper_as "$TRASH_DIRECTORY/uplink" &&
> +   test_when_finished "rm \"$TRASH_DIRECTORY/uplink$X\"" &&
> test_must_fail git clone "[myhost:123]:src" ssh-bracket-clone-uplink 
> &&
> git clone "myhost:src" ssh-clone-uplink &&
> expect_ssh myhost src
>

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

[Johannes Schindelin]

Hi Jonathan,

[I only saw that you replied to 3/5 with v2 after writing this reply, but
it would apply to v2's 3/5, too]

On Mon, 23 Oct 2017, Jonathan Nieder wrote:

> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 86811a0c35..fd94dd40d2 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -384,6 +384,20 @@ test_expect_success 'uplink is treated as simple' '
>   expect_ssh myhost src
>  '
>  
> +test_expect_success 'OpenSSH-like uplink is treated as ssh' '
> + write_script "$TRASH_DIRECTORY/uplink" <<-EOF &&
> + if test "\$1" = "-G"
> + then
> + exit 0
> + fi &&
> + exec "\$TRASH_DIRECTORY/ssh$X" "\$@"
> + EOF
> + GIT_SSH="$TRASH_DIRECTORY/uplink" &&
> + export GIT_SSH &&
> + git clone "[myhost:123]:src" ssh-bracket-clone-sshlike-uplink &&
> + expect_ssh "-p 123" myhost src
> +'
> +
>  test_expect_success 'plink is treated specially (as putty)' '
>   copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
>   git clone "[myhost:123]:src" ssh-bracket-clone-plink-0 &&

This breaks on Windows. And it is not immediately obvious how so, so let
me explain:

As you know, on Windows there is no executable flag. There is the .exe
file extension to indicate an executable (and .com and .bat and .cmd are
also handled as executable, at least as executable script).

Now, what happens if you call "abc" in the MSYS2 Bash and there is no
script called "abc" but an executable called "abc.exe" in the PATH? Why,
of course we execute that executable. It has to, because if "abc.exe"
would be renamed into "abc", it would not work any longer.

That is also the reason why that "copy_ssh_wrapper" helper function
automatically appends that .exe file suffix on Windows: it has to.

Every workaround breaks down at some point, and this workaround is no
exception. What should the MSYS2 Bash do if asked to overwrite "abc" and
there is only an "abc.exe"? It actually overwrites "abc.exe" and moves on.

And this is where we are here: the previous test case copied the ssh
wrapper as "uplink". Except on Windows, it is "uplink.exe". And your newly
introduced test case overwrites it. And then it tells Git specifically to
look for "uplink", and Git does *not* append that .exe suffix
automatically as the MSYS2 Bash would do, because git.exe is not intended
to work MSYS2-like.

As a consequence, the test fails. Could you please squash in this, to fix
the test on Windows?

-- snipsnap --
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index ec4b17bca62..1afcbd00617 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -393,6 +393,7 @@ test_expect_success 'simple does not support port' '
 
 test_expect_success 'uplink is treated as simple' '
copy_ssh_wrapper_as "$TRASH_DIRECTORY/uplink" &&
+   test_when_finished "rm \"$TRASH_DIRECTORY/uplink$X\"" &&
test_must_fail git clone "[myhost:123]:src" ssh-bracket-clone-uplink &&
git clone "myhost:src" ssh-clone-uplink &&
expect_ssh myhost src

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

[Junio C Hamano]

Jonathan Nieder  writes:

>  1. First, check whether $GIT_SSH supports OpenSSH options by running
>
>   $GIT_SSH -G  
>
> This returns status 0 and prints configuration in OpenSSH if it
> recognizes all  and returns status 255 if it encounters
> an unrecognized option.  A wrapper script like
>
>   exec ssh -- "$@"
>
> would fail with
>
>   ssh: Could not resolve hostname -g: Name or service not known
>
> , correctly reflecting that it does not support OpenSSH options.

Two comments.

 * It would have been really nicer if the push_ssh_options() got
   split from its caller in a separate preparatory [PATCH 2.5/5].

 * Use of -G for auto-detection is clever and cute, but do we know
   how safe it would be in the real world?  What worries me the most
   is "myssh -G localhost" (no extra options) that does not fit our
   expectation on "-G" to either exit immediately with an error when
   it is not understood, or to exit like OpenSSH does, and instead
   successfully makes a connection and gets stuck.

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

[Stefan Beller]

>> This is the main one.  Simplified by making "auto" behave the same as
>> unset.
>
> I still don't see the benefit of allowing a user to explicitly set
> 'auto' then, if setting it to 'auto' is effectively a noop.  But maybe
> there's something I'm not seeing.
>

If /etc/gitconfig says SSH, and you have different configs in your
different repos,
the easiest way out is setting AUTO in ~/.gitconfig.

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

[Brandon Williams]

On 10/23, Jonathan Nieder wrote:
> Android's "repo" tool is a tool for managing a large codebase
> consisting of multiple smaller repositories, similar to Git's
> submodule feature.  Starting with Git 94b8ae5a (ssh: introduce a
> 'simple' ssh variant, 2017-10-16), users noticed that it stopped
> handling the port in ssh:// URLs.
> 
> The cause: when it encounters ssh:// URLs, repo pre-connects to the
> server and sets GIT_SSH to a helper ".repo/repo/git_ssh" that reuses
> that connection.  Before 94b8ae5a, the helper was assumed to support
> OpenSSH options for lack of a better guess and got passed a -p option
> to set the port.  After that patch, it uses the new default of a
> simple helper that does not accept an option to set the port.
> 
> The next release of "repo" will set GIT_SSH_VARIANT to "ssh" to avoid
> that.  But users of old versions and of other similar GIT_SSH
> implementations would not get the benefit of that fix.
> 
> So update the default to use OpenSSH options again, with a twist.  As
> observed in 94b8ae5a, we cannot assume that $GIT_SSH always handles
> OpenSSH options: common helpers such as travis-ci's dpl[*] are
> configured using GIT_SSH and do not accept OpenSSH options.  So make
> the default a new variant "auto", with the following behavior:
> 
>  1. First, check for a recognized basename, like today.
> 
>  2. If the basename is not recognized, check whether $GIT_SSH supports
> OpenSSH options by running
> 
>   $GIT_SSH -G  
> 
> This returns status 0 and prints configuration in OpenSSH if it
> recognizes all  and returns status 255 if it encounters
> an unrecognized option.  A wrapper script like
> 
>   exec ssh -- "$@"
> 
> would fail with
> 
>   ssh: Could not resolve hostname -g: Name or service not known
> 
> , correctly reflecting that it does not support OpenSSH options.
> 
>  3. Based on the result from step (2), behave like "ssh" (if it
> succeeded) or "simple" (if it failed).
> 
> This way, the default ssh variant for unrecognized commands can handle
> both the repo and dpl cases as intended.
> 
> [*] 
> https://github.com/travis-ci/dpl/blob/6c3fddfda1f2a85944c56b068bac0a77c049/lib/dpl/provider.rb#L215
> 
> Reported-by: William Yan 
> Improved-by: Jonathan Tan 
> Signed-off-by: Jonathan Nieder 
> ---
> This is the main one.  Simplified by making "auto" behave the same as
> unset.

I still don't see the benefit of allowing a user to explicitly set
'auto' then, if setting it to 'auto' is effectively a noop.  But maybe
there's something I'm not seeing.

> 
>  Documentation/config.txt | 24 --
>  connect.c| 82 
> +++-
>  t/t5601-clone.sh | 20 
>  3 files changed, 88 insertions(+), 38 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0460af37e2..6dffa4aa3d 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2081,16 +2081,22 @@ 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 adjust its command-line parameters for use
> - with ssh (OpenSSH), plink or tortoiseplink, as opposed to the default
> - (simple).
> + 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 auto-detection;
> -valid values are `ssh`, `simple`, `plink`, `putty` or `tortoiseplink`. Any
> -other value will be treated as normal ssh. This setting can be overridden via
> -the environment variable `GIT_SSH_VARIANT`.
> +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
>  follows:
> diff 

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

[Jonathan Nieder]

Brandon Williams wrote:
> On 10/23, Jonathan Nieder wrote:

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

Yes, if git config allowed e.g. ".git/config" to unset a setting from
e.g. "/etc/gitconfig", then I wouldn't want the 'auto' value to exist
in configuration at all.  But because git's config language doesn't
have a way to unset a variable, this series provided "auto" as a way
to explicitly request the same behavior as unset.

In that spirit, the patch I sent was broken ("auto" meant something
different from unset), so thanks for pointing the issue out.

Sensible?
Jonathan

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

[Jonathan Tan]

On Mon, 23 Oct 2017 15:51:06 -0700
Brandon Williams  wrote:

> On 10/23, Jonathan Nieder wrote:
> > 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?

So you're suggesting the second option ("that behavior restricted to the
no-configuration case")?

I'm leaning towards supporting "auto", actually. At the very least, it
gives the user a clear way to override an existing config.

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

This sounds like what is already being done in the code.

> > If this looks good, I can reroll in a moment.

Yes, this looks good.

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

[Jonathan Nieder]

Stefan Beller wrote:
> On Mon, Oct 23, 2017 at 2:31 PM, Jonathan Nieder  wrote:

>>  1. First, check whether $GIT_SSH supports OpenSSH options by running
>>
>> $GIT_SSH -G  
>>
>> This returns status 0 and prints configuration in OpenSSH if it
>> recognizes all  and returns status 255 if it encounters
>> an unrecognized option.  A wrapper script like
>>
>> exec ssh -- "$@"
>>
>> would fail with
>>
>> ssh: Could not resolve hostname -g: Name or service not known
>
> capital -G?

The actual error message uses lowercase (presumably they use tolower on
the hostname).

>> -   if (variant == VARIANT_TORTOISEPLINK)
>> -   argv_array_push(>args, "-batch");
>> +   detect.use_shell = conn->use_shell;
>
> Why do we have to use a shell for evaluation of this
> test balloon?

If the user set GIT_SSH_COMMAND instead of GIT_SSH then we need to run
it using a shell.  The above line inherits the use_shell setting so it
ends up doing whatever conn would do.

[...]
>> +   argv_array_push(, "-G");
> ...
>> +   variant = run_command() ? VARIANT_SIMPLE : 
>> VARIANT_SSH;
>
> What if we have a VARIANT_SIMPLE, that doesn't care about -G
> but just connects to the remote host (keeping the session open), do we need
> to kill it after some time to have run_command return eventually?
>
> Or can we give a command to be executed remotely (e.g. 'false') that
> we know returns quickly?

Since stdin is /dev/null, it would presumably return quickly.  But I
also don't expect this kind of behavior from GIT_SSH commands.  The
kinds I'd expect are

 A. The repo case, which forwards to 'ssh' and supports all 'ssh'
flags

 B. The travis-ci case, which expects a host and command and does
not accept any flags

 C. More sophisticated wrappers that parse flags but still do not
handle -G

For case (A), the detection run would figure out that this accepts
OpenSSH options. Good.

For case (B), the detection run would figure out that this does not
accept OpenSSH options. Good.

For case (C), the detection run would think that this does not accept
OpenSSH options, when it accepts some. But I think that's the best we
can do for now. Longer term, we need to work with the author of such a
script to find out what kind of interface they want.

Thanks and hope that helps,
Jonathan

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

[Brandon Williams]

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 

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

[Jonathan Nieder]

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?

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", ))
-   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 

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

[Stefan Beller]

On Mon, Oct 23, 2017 at 2:31 PM, Jonathan Nieder  wrote:

>  1. First, check whether $GIT_SSH supports OpenSSH options by running
>
> $GIT_SSH -G  
>
> This returns status 0 and prints configuration in OpenSSH if it
> recognizes all  and returns status 255 if it encounters
> an unrecognized option.  A wrapper script like
>
> exec ssh -- "$@"
>
> would fail with
>
> ssh: Could not resolve hostname -g: Name or service not known

capital -G?


> -   if (variant == VARIANT_TORTOISEPLINK)
> -   argv_array_push(>args, "-batch");
> +   detect.use_shell = conn->use_shell;

Why do we have to use a shell for evaluation of this
test balloon?

> +   detect.no_stdin = detect.no_stdout = detect.no_stderr = 1;

okay.

...
> +   argv_array_push(, "-G");
...
> +   variant = run_command() ? VARIANT_SIMPLE : VARIANT_SSH;

What if we have a VARIANT_SIMPLE, that doesn't care about -G
but just connects to the remote host (keeping the session open), do we need
to kill it after some time to have run_command return eventually?

Or can we give a command to be executed remotely (e.g. 'false') that
we know returns quickly?

>  '
>
> +test_expect_success 'OpenSSH-like uplink is treated as ssh' '
> +   write_script "$TRASH_DIRECTORY/uplink" <<-EOF &&
> +   if test "\$1" = "-G"

Reading this test (and the commit message) I realize, we do care
about the order of options, so this is fine.

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

[Jonathan Tan]

On Mon, 23 Oct 2017 14:31:59 -0700
Jonathan Nieder  wrote:

> @@ -2083,14 +2083,19 @@ 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 adjust its command-line parameters for use
> - with ssh (OpenSSH), plink or tortoiseplink, as opposed to the default
> - (simple).
> + 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` or `tortoiseplink`. Any
> -other value will be treated as normal ssh. This setting can be overridden via
> -the environment variable `GIT_SSH_VARIANT`.
> +valid values are `ssh`, `simple`, `plink`, `putty`, `tortoiseplink`, and
> +`auto`.  Any other value will be treated as normal ssh.  This setting can be
> +overridden via the environment variable `GIT_SSH_VARIANT`.

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

(Patches 1, 2, 4, and 5 seem fine to me.)