Re: [ANNOUNCE] Git v2.16.0-rc0

[Jeff King]

On Fri, Jun 15, 2018 at 01:13:21AM -0400, Jeff King wrote:

> > The trouble is that GIT_SSH_VARIANT=simple is too... simple.  You
> > would like a variant that passes in [-p port] [-4] [-6] as well.  We
> > didn't implement that because we didn't have the attention of any
> > wrapper writer who wanted it; in absence of a potential user, we
> > decided to wait for a user to propose the interface they want.  Now we
> > can celebrate, since that day has come.
> 
> I'm not sure I'm celebrating. It seems like work for not much benefit.
> I'd just as soon use VARIANT=ssh and deal with any fallouts if my script
> does not behave exactly like openssh in all regards.

By the way, if this sounds like I'm crapping all over your ideas, I
really don't mean to. What I'm trying to say is that I think your
original patch may have hit a sweet spot between robustness and making
a simple-enough interface for users to configure.

-Peff

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jeff King]

On Thu, Jun 14, 2018 at 09:20:33PM -0700, Jonathan Nieder wrote:

> What we've switched to is a versioned interface.  By setting
> GIT_SSH_VARIANT=simple, you are asking Git to promise to pass exactly
>  options.  If Git has a new option it wants to pass (like the "-o
> SendEnv" thing) but can live without it, then it can avoid breaking
> your wrapper and continue to follow this new promise.
> 
> The trouble is that GIT_SSH_VARIANT=simple is too... simple.  You
> would like a variant that passes in [-p port] [-4] [-6] as well.  We
> didn't implement that because we didn't have the attention of any
> wrapper writer who wanted it; in absence of a potential user, we
> decided to wait for a user to propose the interface they want.  Now we
> can celebrate, since that day has come.

I'm not sure I'm celebrating. It seems like work for not much benefit.
I'd just as soon use VARIANT=ssh and deal with any fallouts if my script
does not behave exactly like openssh in all regards.

> How would you like your ssh variant to work?  Some possibilities to
> get the thought process going:
> 
>  A. Would you want to set a variable 'GIT_SSH_SUPPORTS_OPTIONS=p46'
> to inform Git about what options you support?

Not really. That just creates more work when I have to later use "op46"
to support "-o", even though my script already handles it fine (or
worse, since "-o" is open-ended).

>  B. Alternatively, what about a 'GIT_SSH_VARIANT=capabilities' variant
> that calls "your-ssh-variant --capabailities" to get a
> machine-readable list of capabilities it supports?

Not really. Now I have to implement this --capabilities thing. This is
literally a 10 line script.

>  C. Alternatively, would you like all parameters to come in on stdin,
> credential helper style?

That's even worse. ;)

>  D. Other ideas?

I really am happy just saying "look, my script is basically openssh, so
just assume that". If it breaks, it breaks, and I'll fix it.

The one thing I was left puzzled by is why "-G" didn't work in the first
place, since the script really does just pass through its options.
Poking around, I think the problem actually _is_ the old ssh version
thing. I have a new one on my workstation, but our CI boxes are jessie,
and have openssh 1:6.7p1-5+deb8u4. And that's where I was digging into
the failure.

So sorry for misleading you earlier. The wrapper script looks like it's
a red herring to some degree (I guess its name not being "ssh"
contributed, but then the -G detection failed).

> If you were using an old version of OpenSSH, this would be a reason to
> revive the old patch, but I'm tempted to stall longer just to get more
> use cases like this to come out of the woodwork.

So apparently I am using an old version. Just switching to use "-V"
seems like it might be another solution, then.

-Peff

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Thu, Jun 14, 2018 at 11:55:22AM -0700, Jonathan Nieder wrote:

>> Do you mean that it doesn't pass "-G" through, or that when using old
>> versions of openssh that doesn't support "-G" the probing fails?
>
> It just doesn't pass "-G" through.

Thanks.

>> If the former, then detecting the wrapper as something other than
>> "ssh" is intended behavior (though we might want to change what that
>> something is, as discussed in the previous thread).  If the latter,
>> then this is https://crbug.com/git/7 which I consider to be a bug.
>
> I certainly see the argument that "well, if it doesn't do '-G' then it's
> not _really_ openssh". My counter to that is that we don't actually
> _care_ about -G (and never did before recently). It's just a proxy for
> "do we understand -p", which my script does understand. My wrapper might
> eventually break if we depend on new options (like "-o SendEnv")

Exactly: what we want to detect is not whether the script happens to
support the OpenSSH options we use today, but whether it is a thin
wrapper around OpenSSH that supports *all* options.

This wrapper definitely does not qualify.  As we start to use more
OpenSSH options, we are likely to break it.  As you say,

> the worst case there is generally no different before or after your
> patch: the command barfs.

but as a caller, Git has no way to know that: it is easily possible
that the wrapper would ignore the new option we want to pass, for
example.

In other words, I think your response is based on the interface that
we previously, foolishly advertised: "We will pass exactly these
options to your wrapper".  Except those options changed over time,
multiple times.  It was a terrible interface.

What we've switched to is a versioned interface.  By setting
GIT_SSH_VARIANT=simple, you are asking Git to promise to pass exactly
 options.  If Git has a new option it wants to pass (like the "-o
SendEnv" thing) but can live without it, then it can avoid breaking
your wrapper and continue to follow this new promise.

The trouble is that GIT_SSH_VARIANT=simple is too... simple.  You
would like a variant that passes in [-p port] [-4] [-6] as well.  We
didn't implement that because we didn't have the attention of any
wrapper writer who wanted it; in absence of a potential user, we
decided to wait for a user to propose the interface they want.  Now we
can celebrate, since that day has come.

How would you like your ssh variant to work?  Some possibilities to
get the thought process going:

 A. Would you want to set a variable 'GIT_SSH_SUPPORTS_OPTIONS=p46'
to inform Git about what options you support?

 B. Alternatively, what about a 'GIT_SSH_VARIANT=capabilities' variant
that calls "your-ssh-variant --capabailities" to get a
machine-readable list of capabilities it supports?

 C. Alternatively, would you like all parameters to come in on stdin,
credential helper style?

 D. Other ideas?

[...]
> But again, I'm just describing what makes sense to me. If you feel
> strongly about requiring the variant to be explicitly specified, I can
> certainly live with that.

I think I do feel strongly.

If you were using an old version of OpenSSH, this would be a reason to
revive the old patch, but I'm tempted to stall longer just to get more
use cases like this to come out of the woodwork.

Of course that's a dangerous thought process.  Probably by tomorrow
I'll think better of it and revive the patch.  In the meantime I would
be happy to see your answers to the questions above.

Sincerely,
Jonathan

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jeff King]

On Thu, Jun 14, 2018 at 11:55:22AM -0700, Jonathan Nieder wrote:

> > No, my wrapper _isn't_ simple. It passes most options to openssh, but
> > just doesn't understand the "-G" probing.  So if the default was
> > openssh-like instead of "simple", then that would work fine without me
> > setting anything, just like it did before.
> >
> > Which I thought was where the discussion ended up, but perhaps I'm
> > misunderstanding.
> 
> Do you mean that it doesn't pass "-G" through, or that when using old
> versions of openssh that doesn't support "-G" the probing fails?

It just doesn't pass "-G" through.

> If the former, then detecting the wrapper as something other than
> "ssh" is intended behavior (though we might want to change what that
> something is, as discussed in the previous thread).  If the latter,
> then this is https://crbug.com/git/7 which I consider to be a bug.

I certainly see the argument that "well, if it doesn't do '-G' then it's
not _really_ openssh". My counter to that is that we don't actually
_care_ about -G (and never did before recently). It's just a proxy for
"do we understand -p", which my script does understand. My wrapper might
eventually break if we depend on new options (like "-o SendEnv"), but
the worst case there is generally no different before or after your
patch: the command barfs.

I say "generally" because of course you can come up with an example
where my script quietly interprets "-o" as something else, but it seems
like most uses there would cause an error.  And anyway, by making me set
GIT_SSH_VARIANT all we've bought is plausible deniability that it's _my_
fault for doing so when my script doesn't handle the new option
gracefully. ;)

But again, I'm just describing what makes sense to me. If you feel
strongly about requiring the variant to be explicitly specified, I can
certainly live with that.

-Peff

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jonathan Nieder]

Hi,

Sorry for the slow replies.  I was out of office earlier and am back
now.

Jeff King wrote:
> On Tue, Jun 12, 2018 at 09:29:13AM -0700, Junio C Hamano wrote:
>> Jeff King  writes:

>>> To be honest, I could easily see an argument that I _should_ be setting
>>> GIT_SSH_VARIANT to explain what my wrapper is expecting, even though it
>>> happened to work before.

Yes, I encourage anyone setting GIT_SSH to also set GIT_SSH_VARIANT.
This way, there can be no confusion about what the setter intends.

The autodetection is in the category of "necessary evil" that wouldn't
exist if we were starting over.

[...]
> No, my wrapper _isn't_ simple. It passes most options to openssh, but
> just doesn't understand the "-G" probing.  So if the default was
> openssh-like instead of "simple", then that would work fine without me
> setting anything, just like it did before.
>
> Which I thought was where the discussion ended up, but perhaps I'm
> misunderstanding.

Do you mean that it doesn't pass "-G" through, or that when using old
versions of openssh that doesn't support "-G" the probing fails?

If the former, then detecting the wrapper as something other than
"ssh" is intended behavior (though we might want to change what that
something is, as discussed in the previous thread).  If the latter,
then this is https://crbug.com/git/7 which I consider to be a bug.

[...]
> So I'm OK if we just leave it as-is. It's mostly that I dug into the
> thread and was left with the impression that it was an unfinished
> leftover that we meant to do.
[...]
>> In any case, from where I sit, I am still waiting for this offer
>> by Jonathan
>>
>>> 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.
>>
>> to be followed up ;-)
>
> Yes, that was the part that left the impression. :)

Thanks for the poke.

The patch [1] didn't leave me super happy, since it means there is yet
another ssh variant for people to understand, in order to accomodate
an old version of OpenSSH that is going to go away eventually.  In
Debian, modern versions of git declare an incompatibility with old
versions of OpenSSH in their package metadata to avoid this issue.

In my defense, I said "If so" and no one seemed too enthusiastic about
the patch making sense. ;-)

I'll take another look and probably resend.

Sincerely,
Jonathan

[1] 
https://public-inbox.org/git/20180103050730.ga87...@aiede.mtv.corp.google.com/

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jeff King]

On Tue, Jun 12, 2018 at 09:29:13AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > To be honest, I could easily see an argument that I _should_ be setting
> > GIT_SSH_VARIANT to explain what my wrapper is expecting, even though it
> > happened to work before.
> 
> The way I read that message is that the patch proposed in
> 
>   
> https://public-inbox.org/git/20180103050730.ga87...@aiede.mtv.corp.google.com
> 
> is "lesser of two evils" in that it is still evil because somebody
> still has to be asked to explicitly set "variant" anyway but the
> changing what 'simple' means in the middle would mean those who did
> not have to set it now have to.  So, you should be setting it, even
> if we adopt the patch, right? ;-)

No, my wrapper _isn't_ simple. It passes most options to openssh, but
just doesn't understand the "-G" probing.  So if the default was
openssh-like instead of "simple", then that would work fine without me
setting anything, just like it did before.

Which I thought was where the discussion ended up, but perhaps I'm
misunderstanding.

> My impression is that regression "fix" does not exist---rather, it
> was a proposal (and it may make a better tradeoff than the status
> quo) to help users of older OpenSSH by inconveniencing users of
> different implementations that do -4/6/p differently from OpenSSH.

Right. That fixes the regression for openssh users, at the cost of not
help people on other implementations automatically. But those people
have to set the flag anyway, since the current behavior is "whoops, we
don't know how to support you, set the flag".

To be clear, I've already fixed my setup, and it wasn't that big a deal.
And I doubt all that many people would be affected either way. My use
case here was literally instrumenting the ssh calls of the git client as
part of a regression test suite. How many git test suites could there
possibly be? :)

So I'm OK if we just leave it as-is. It's mostly that I dug into the
thread and was left with the impression that it was an unfinished
leftover that we meant to do.

> In any case, from where I sit, I am still waiting for this offer
> by Jonathan
> 
> > 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.
> 
> to be followed up ;-)

Yes, that was the part that left the impression. :)

-Peff

Re: [ANNOUNCE] Git v2.16.0-rc0

[Junio C Hamano]

Jeff King  writes:

> To be honest, I could easily see an argument that I _should_ be setting
> GIT_SSH_VARIANT to explain what my wrapper is expecting, even though it
> happened to work before.

The way I read that message is that the patch proposed in

  https://public-inbox.org/git/20180103050730.ga87...@aiede.mtv.corp.google.com

is "lesser of two evils" in that it is still evil because somebody
still has to be asked to explicitly set "variant" anyway but the
changing what 'simple' means in the middle would mean those who did
not have to set it now have to.  So, you should be setting it, even
if we adopt the patch, right? ;-)

> But it seems like this discussion ended in
> favor of calling this a regression that should be fixed, and AFAICT
> nothing happened after. So I thought I'd ping and mention one more data
> point.

My impression is that regression "fix" does not exist---rather, it
was a proposal (and it may make a better tradeoff than the status
quo) to help users of older OpenSSH by inconveniencing users of
different implementations that do -4/6/p differently from OpenSSH.

In any case, from where I sit, I am still waiting for this offer
by Jonathan

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

to be followed up ;-)

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jeff King]

On Tue, Jan 02, 2018 at 09:35:16PM -0800, Jonathan Nieder wrote:

> > bturner@ubuntu:~$ ssh -V
> > OpenSSH_6.6.1p1 Ubuntu-2ubuntu2.8, OpenSSL 1.0.1f 6 Jan 2014
> >
> > bturner@ubuntu:~$ ssh -G -p 7999 localhost
> > unknown option -- G
> > usage: ssh [-1246AaCfgKkMNnqsTtVvXxYy] [-b bind_address] [-c cipher_spec]
> [...]
> > Is it possible to adjust the check, somehow, so it doesn't impact
> > older OpenSSH versions like this? As it stands, it seems likely a fair
> > number of users who have an SSH command that does support -4, -6 and
> > -p are going to end up getting "penalized" because it doesn't also
> > support -G, and have to manually set their SSH variant to "ssh" (or
> > something other than "auto") to avoid the automatic detection.
> >
> > I'd love to say I have a brilliant idea for how to work around this,
> > oh and here's a patch, but I don't. One option might be trying to
> > actually review the output, and another might be to run "ssh -V", but
> > both of those have their own flaws (and the extra process forks aren't
> > "free").
> 
> I have tomorrow off, so I've filed https://crbug.com/git/7 to make
> sure I remember to follow up the day after.  Of course I'll be happy
> if someone updates that bug saying they've fixed it in the meantime.

It doesn't look like we ever applied anything to deal with this
regression. Just FYI, this bit me today when upgrading my git on a
system that has an ssh wrapper that understands "-p" just fine, but not
"-G". So the behavior described in [1], namely to just fallback to
assuming some basic openssh-ish options, would have worked for me.

To be honest, I could easily see an argument that I _should_ be setting
GIT_SSH_VARIANT to explain what my wrapper is expecting, even though it
happened to work before. But it seems like this discussion ended in
favor of calling this a regression that should be fixed, and AFAICT
nothing happened after. So I thought I'd ping and mention one more data
point.

-Peff

[1] https://public-inbox.org/git/xmqqk1wyhcey@gitster.mtv.corp.google.com/

Re: [ANNOUNCE] Git v2.16.0-rc0

[Paul Smith]

On Thu, 2018-01-04 at 20:18 +, Thomas Gummerer wrote:
> On 12/29, Paul Smith wrote:
> > On Thu, 2017-12-28 at 20:30 -0800, Junio C Hamano wrote:
> > >   * The way "git worktree add" determines what branch to create
> > > from where and checkout in the new worktree has been updated a
> > > bit.
> > 
> > Does this include the enhancements published a few weeks ago to
> > allow worktrees to be created directly from remote branches without
> > first checking out the branch locally? I'm really looking forward
> > to that change...
> 
> Yes, this release will include that.  It would be awesome if you
> could test the rc, now is the best time to scream if something about
> it is not as you'd expect :)

OK, I pulled and built this locally, and indeed the straightforward
"git worktree add  " works just as I'd hoped!  Nice!

I'll play with it more and let you know if I hit issues.

Re: [ANNOUNCE] Git v2.16.0-rc0

[Johannes Schindelin]

Hi,

On Thu, 28 Dec 2017, Junio C Hamano wrote:

> An early preview release Git v2.16.0-rc0 is now available for
> testing at the usual places.

And a corresponding Git for Windows prerelease is also available:

https://github.com/git-for-windows/git/releases/tag/v2.16.0-rc0.windows.1

Ciao,
Johannes

Re: [ANNOUNCE] Git v2.16.0-rc0

[Thomas Gummerer]

On 12/29, Paul Smith wrote:
> On Thu, 2017-12-28 at 20:30 -0800, Junio C Hamano wrote:
> >  * The way "git worktree add" determines what branch to create from
> >where and checkout in the new worktree has been updated a bit.
> 
> Does this include the enhancements published a few weeks ago to allow
> worktrees to be created directly from remote branches without first
> checking out the branch locally? I'm really looking forward to that
> change...

Yes, this release will include that.  It would be awesome if you could
test the rc, now is the best time to scream if something about it is
not as you'd expect :)

> Thanks!

Re: [ANNOUNCE] Git v2.16.0-rc0

[Junio C Hamano]

Jonathan Nieder  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(, ssh_host);
>  
> - variant = run_command() ? VARIANT_SIMPLE : VARIANT_SSH;
> + variant = run_command() ? VARIANT_AUTO : VARIANT_SSH;
>   }
>  
>   argv_array_push(>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' '
>   

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jonathan Nieder]

Bryan Turner wrote:
> On Tue, Jan 2, 2018 at 9:07 PM, Jonathan Nieder  wrote:

>> So my first question is why the basename detection is not working for
>> you.  What value of GIT_SSH, GIT_SSH_COMMAND, or core.sshCommand are
>> you using?
>
> So I'd been digging further into this for the last hour because I
> wasn't seeing quite the behavior I was expecting when I ran Git from
> the command line on Ubuntu 12.04 or 14.04, and this nudged me to the
> right answer: We're setting GIT_SSH to a wrapper script. In our case,
> that wrapper script is just calling OpenSSH's ssh with all the
> provided arguments (plus a couple extra ones), but because we're
> setting GIT_SSH at all, that's why the auto variant code is running.
> That being the case, explicitly setting GIT_SSH_VARIANT=ssh may be the
> correct thing to do, to tell Git that we want to be treated like
> "normal" OpenSSH, as opposed to expecting Git to assume we behave like
> OpenSSH (when the Android repo use case clearly shows that assumption
> also doesn't hold).

Ah, that's a comfort.  Setting GIT_SSH_VARIANT would avoid this
autodetection code and is the recommended thing to do.

That said, we can't go back in time and update everyone's tools to do
that (e.g. there is not even a release of repo with [1] out yet), so
this is still considered a regression and I'm glad you found it.

Jonathan

[1] https://gerrit-review.googlesource.com/c/git-repo/+/134950

Re: [ANNOUNCE] Git v2.16.0-rc0

[Bryan Turner]

On Tue, Jan 2, 2018 at 9:07 PM, Jonathan Nieder  wrote:
> Hi Bryan,
>
> Bryan Turner wrote:
>
>> Our test environment is still on Ubuntu 12.04 LTS (it's a long story,
>> but one I doubt is unique to us), which means it's using OpenSSH 5.9.
>> ssh -G was added in OpenSSH 6.8 [1], circa March 2015, which means the
>> "auto" detection "fails" and chooses "simple" instead of "ssh". But
>> OpenSSH 5.9 _does_ support -4, -6 and -p. As a result, commands which
>> have been working without issue on all previous versions of Git start
>> to fail saying
>>
>> git -c gc.auto=0 -c credential.helper= fetch --force --prune --progress 
>> ssh://localhost:64281/repo.git +refs/*:refs/*' exited with code 128 saying: 
>> fatal: ssh variant 'simple' does not support setting port
>
> Hm, that's not expected.  git-config(1) says:
>
> 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 [...]
>
> So my first question is why the basename detection is not working for
> you.  What value of GIT_SSH, GIT_SSH_COMMAND, or core.sshCommand are
> you using?

So I'd been digging further into this for the last hour because I
wasn't seeing quite the behavior I was expecting when I ran Git from
the command line on Ubuntu 12.04 or 14.04, and this nudged me to the
right answer: We're setting GIT_SSH to a wrapper script. In our case,
that wrapper script is just calling OpenSSH's ssh with all the
provided arguments (plus a couple extra ones), but because we're
setting GIT_SSH at all, that's why the auto variant code is running.
That being the case, explicitly setting GIT_SSH_VARIANT=ssh may be the
correct thing to do, to tell Git that we want to be treated like
"normal" OpenSSH, as opposed to expecting Git to assume we behave like
OpenSSH (when the Android repo use case clearly shows that assumption
also doesn't hold).

Based on that, I'm not sure if you _actually_ need to change anything,
and I apologize for not turning up that we were setting GIT_SSH before
I bothered you.

Thanks immensely for looking into it, Jonathan, and my apologies again
for wasting your time!
Bryan Turner

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jonathan Nieder]

Hi,

A few more notes.

Bryan Turner wrote:

> bturner@ubuntu:~$ ssh -V
> OpenSSH_6.6.1p1 Ubuntu-2ubuntu2.8, OpenSSL 1.0.1f 6 Jan 2014
>
> bturner@ubuntu:~$ ssh -G -p 7999 localhost
> unknown option -- G
> usage: ssh [-1246AaCfgKkMNnqsTtVvXxYy] [-b bind_address] [-c cipher_spec]
[...]
> Is it possible to adjust the check, somehow, so it doesn't impact
> older OpenSSH versions like this? As it stands, it seems likely a fair
> number of users who have an SSH command that does support -4, -6 and
> -p are going to end up getting "penalized" because it doesn't also
> support -G, and have to manually set their SSH variant to "ssh" (or
> something other than "auto") to avoid the automatic detection.
>
> I'd love to say I have a brilliant idea for how to work around this,
> oh and here's a patch, but I don't. One option might be trying to
> actually review the output, and another might be to run "ssh -V", but
> both of those have their own flaws (and the extra process forks aren't
> "free").

I have tomorrow off, so I've filed https://crbug.com/git/7 to make
sure I remember to follow up the day after.  Of course I'll be happy
if someone updates that bug saying they've fixed it in the meantime.

One possibility would be to use -V as a fallback when -G fails, or
even as a replacement for this usage of -G.  To avoid misdetecting
PuTTY and other ssh variants that also implement -V as OpenSSH, we
would have to parse the output.  This would also misdetect a script
that does

host=$1; shift
ssh "$host" -- "$@"

as supporting OpenSSH options, when the use of -- ensures it doesn't.

Another possibility is to parse the output when -G fails.  That's
hacky, but I think it would work well!  We would not have to be too
clever, since we can look for the exact output produced by the
versions of OpenSSH that we care about.  This still has issues with
scripts that forward arguments to OpenSSH, but at least those issues
would go away once the user updates their copy of ssh. ;-)

Another possibility is to pass options *before* -V:

ssh -p 7999 -V

Since OpenSSH parses its arguments left-to-right, this gives similar
information to what we did with -G, and scripts like

host=$1; shift
ssh "$host" -- "$@"

would even be correctly detected as not supporting OpenSSH options.
We still would need to parse the output to distinguish OpenSSH from
other ssh implementations like putty (unlike OpenSSH, putty saves up
argument errors in an 'error' variable and forgets about them once it
sees -V).

Trying -G and falling back to -V seems like the simplest detection
mechanism to me at the moment.  I'm hoping I'm missing something
simple (another ssh option?) that allows avoiding this mess.

Regardless, I think we should do something like [1] first to get rid
of the regression.  Thanks again for reporting it.

Sincerely,
Jonathan

[1] 
https://public-inbox.org/git/20180103050730.ga87...@aiede.mtv.corp.google.com/

Re: [ANNOUNCE] Git v2.16.0-rc0

[Jonathan Nieder]

Hi Bryan,

Bryan Turner wrote:

> Our test environment is still on Ubuntu 12.04 LTS (it's a long story,
> but one I doubt is unique to us), which means it's using OpenSSH 5.9.
> ssh -G was added in OpenSSH 6.8 [1], circa March 2015, which means the
> "auto" detection "fails" and chooses "simple" instead of "ssh". But
> OpenSSH 5.9 _does_ support -4, -6 and -p. As a result, commands which
> have been working without issue on all previous versions of Git start
> to fail saying
>
> git -c gc.auto=0 -c credential.helper= fetch --force --prune --progress 
> ssh://localhost:64281/repo.git +refs/*:refs/*' exited with code 128 saying: 
> fatal: ssh variant 'simple' does not support setting port

Hm, that's not expected.  git-config(1) says:

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 [...]

So my first question is why the basename detection is not working for
you.  What value of GIT_SSH, GIT_SSH_COMMAND, or core.sshCommand are
you using?

> I know Ubuntu 12.04 LTS is end-of-life, but 14.04 LTS, which is
> running OpenSSH 6.6 [2], has the same issue. The following is from a
> fully patched 14.04.5:
>
> bturner@ubuntu:~$ cat /etc/*ease | head -4
> DISTRIB_ID=Ubuntu
> DISTRIB_RELEASE=14.04
> DISTRIB_CODENAME=trusty
> DISTRIB_DESCRIPTION="Ubuntu 14.04.5 LTS"
>
> bturner@ubuntu:~$ ssh -V
> OpenSSH_6.6.1p1 Ubuntu-2ubuntu2.8, OpenSSL 1.0.1f 6 Jan 2014
>
> bturner@ubuntu:~$ ssh -G -p 7999 localhost
> unknown option -- G

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.

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(, ssh_host);
 
-   variant = run_command() ? VARIANT_SIMPLE : VARIANT_SSH;
+   variant = run_command() ? VARIANT_AUTO : VARIANT_SSH;
}
 
argv_array_push(>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 @@ 

Re: [ANNOUNCE] Git v2.16.0-rc0

[Bryan Turner]

On Thu, Dec 28, 2017 at 8:30 PM, Junio C Hamano  wrote:
> An early preview release Git v2.16.0-rc0 is now available for
> testing at the usual places.  It is comprised of 435 non-merge
> commits since v2.15.0, contributed by 76 people, 22 of which are
> new faces.

> Brandon Williams (24):
>   ssh: introduce a 'simple' ssh variant

> Jonathan Nieder (10):
>   ssh test: make copy_ssh_wrapper_as clean up after itself
>   connect: move no_fork fallback to git_tcp_connect
>   connect: split git:// setup into a separate function
>   connect: split ssh command line options into separate function
>   connect: split ssh option computation to its own function
>   ssh: 'auto' variant to select between 'ssh' and 'simple'
>   ssh: 'simple' variant does not support -4/-6
>   ssh: 'simple' variant does not support --port
>   connect: correct style of C-style comment
>   generate-cmdlist: avoid non-deterministic output

Sorry for being late to the party on the "simple" variant for SSH, but
we've been doing some testing with 2.16.0-rc0 and noticed an
unexpected issue.

Our test environment is still on Ubuntu 12.04 LTS (it's a long story,
but one I doubt is unique to us), which means it's using OpenSSH 5.9.
ssh -G was added in OpenSSH 6.8 [1], circa March 2015, which means the
"auto" detection "fails" and chooses "simple" instead of "ssh". But
OpenSSH 5.9 _does_ support -4, -6 and -p. As a result, commands which
have been working without issue on all previous versions of Git start
to fail saying:

git -c gc.auto=0 -c credential.helper= fetch --force --prune
--progress ssh://localhost:64281/repo.git +refs/*:refs/*' exited with
code 128 saying: fatal: ssh variant 'simple' does not support setting
port

I know Ubuntu 12.04 LTS is end-of-life, but 14.04 LTS, which is
running OpenSSH 6.6 [2], has the same issue. The following is from a
fully patched 14.04.5:

bturner@ubuntu:~$ cat /etc/*ease | head -4
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=14.04
DISTRIB_CODENAME=trusty
DISTRIB_DESCRIPTION="Ubuntu 14.04.5 LTS"

bturner@ubuntu:~$ ssh -V
OpenSSH_6.6.1p1 Ubuntu-2ubuntu2.8, OpenSSL 1.0.1f 6 Jan 2014

bturner@ubuntu:~$ ssh -G -p 7999 localhost
unknown option -- G
usage: ssh [-1246AaCfgKkMNnqsTtVvXxYy] [-b bind_address] [-c cipher_spec]
   [-D [bind_address:]port] [-E log_file] [-e escape_char]
   [-F configfile] [-I pkcs11] [-i identity_file]
   [-L [bind_address:]port:host:hostport] [-l login_name] [-m mac_spec]
   [-O ctl_cmd] [-o option] [-p port]
   [-Q cipher | cipher-auth | mac | kex | key]
   [-R [bind_address:]port:host:hostport] [-S ctl_path] [-W host:port]
   [-w local_tun[:remote_tun]] [user@]hostname [command]

Is it possible to adjust the check, somehow, so it doesn't impact
older OpenSSH versions like this? As it stands, it seems likely a fair
number of users who have an SSH command that does support -4, -6 and
-p are going to end up getting "penalized" because it doesn't also
support -G, and have to manually set their SSH variant to "ssh" (or
something other than "auto") to avoid the automatic detection.

I'd love to say I have a brilliant idea for how to work around this,
oh and here's a patch, but I don't. One option might be trying to
actually review the output, and another might be to run "ssh -V", but
both of those have their own flaws (and the extra process forks aren't
"free").

[1] https://www.openssh.com/txt/release-6.8
[2] https://launchpad.net/ubuntu/+source/openssh

Best regards,
Bryan Turner

Re: [ANNOUNCE] Git v2.16.0-rc0

[Paul Smith]

On Thu, 2017-12-28 at 20:30 -0800, Junio C Hamano wrote:
>  * The way "git worktree add" determines what branch to create from
>where and checkout in the new worktree has been updated a bit.

Does this include the enhancements published a few weeks ago to allow
worktrees to be created directly from remote branches without first
checking out the branch locally? I'm really looking forward to that
change...

Thanks!

Re: [ANNOUNCE] Git v2.16.0-rc0

[Kaartic Sivaraam]

On Friday 29 December 2017 10:00 AM, Junio C Hamano wrote:

  * "git branch" and "git checkout -b" are now forbidden from creating
a branch whose name is "HEAD".


"git branch" already forbid a branch named "HEAD", didn't it? I thought 
we just made "git checkout -b" to reject "HEAD" as a valid branch name, 
recently.


--
Kaartic