git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>,
	Bryan Turner <bturner@atlassian.com>,
	Brandon Williams <bmwill@google.com>,
	Ben Humphreys <behumphreys@atlassian.com>,
	Git Users <git@vger.kernel.org>
Subject: Re: [ANNOUNCE] Git v2.16.0-rc0
Date: Thu, 14 Jun 2018 21:20:33 -0700	[thread overview]
Message-ID: <20180615042033.GB255581@aiede.svl.corp.google.com> (raw)
In-Reply-To: <20180614193943.GA2226@sigill.intra.peff.net>

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

  reply	other threads:[~2018-06-15  4:20 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-29  4:30 [ANNOUNCE] Git v2.16.0-rc0 Junio C Hamano
2017-12-29  7:17 ` Kaartic Sivaraam
2017-12-29 17:13 ` Paul Smith
2018-01-04 20:18   ` Thomas Gummerer
2018-01-04 21:35     ` Paul Smith
2018-01-03  3:34 ` Bryan Turner
2018-01-03  5:07   ` Jonathan Nieder
2018-01-03  5:41     ` Bryan Turner
2018-01-03  5:50       ` Jonathan Nieder
2018-01-03 21:15     ` Junio C Hamano
2018-01-03  5:35   ` Jonathan Nieder
2018-06-08  4:50     ` Jeff King
2018-06-12 16:29       ` Junio C Hamano
2018-06-14 18:30         ` Jeff King
2018-06-14 18:55           ` Jonathan Nieder
2018-06-14 19:39             ` Jeff King
2018-06-15  4:20               ` Jonathan Nieder [this message]
2018-06-15  5:13                 ` Jeff King
2018-06-15  7:26                   ` Jeff King
2018-01-04 20:33 ` Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180615042033.GB255581@aiede.svl.corp.google.com \
    --to=jrnieder@gmail.com \
    --cc=behumphreys@atlassian.com \
    --cc=bmwill@google.com \
    --cc=bturner@atlassian.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).