Re: [PATCH 1/3] serve: pass "config context" through to individual commands

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 14, 2018, 09:28 UTC
Message-ID: <20181214092820.GB7121@google.com>
In-Reply-To: <20181214085507.GD11777@sigill.intra.peff.net>

Jeff King wrote:

> On Fri, Dec 14, 2018 at 12:36:21AM -0800, Jonathan Nieder wrote:
>> Jeff King wrote:

Show 13 quoted lines

>>> In protocol v2, instead of just running "upload-pack", we have a generic
>>> "serve" loop which runs command requests from the client. What used to
>>> be "upload-pack" is now generally split into two operations: "ls-refs"
>>> and "fetch". The latter knows it must respect uploadpack.* config, but
>>> the former is not actually specific to a fetch operation (we do not yet
>>> do v2 receive-pack, but eventually we may, and ls-refs would support
>>> both operations).
>>
>> I think I'm missing something.  Why wouldn't "ls-refs for push" not pass
>> the information that it's for push as part of the *body* of the ls-refs
>> request?
>
> I don't know. Why doesn't the current "ls-refs" say "ls-refs for fetch"?

Also YAGNI. ;-)

In other words, since the design for push isn't set in stone yet, we had nothing to be consistent with. And if there's an option to ls-ref to indicate whether it's for fetch or for push, then it can default to fetch.

As an aside, my experience from teaching people about Git protocol is that the concept of "ls-remote for push" producing a different result from "git ls-remote" is very confusing. Given what it is used for, I am motivated to replace it with something more tailored.

> Certainly if that information was carried from the client request it
> would work fine, and ls-refs would have enough to know which config to
> respect. But I could not find any documentation on this, nor discussion
> of plans for a v2 push.

Interesting. The last discussion of push v2 plans was in https://public-inbox.org/git/20180717210915.139521-1-bmwill@google.com/. Expect to hear more soon.

Show 7 quoted lines

>                         Since that information isn't passed now, we'd
> have to assume that "ls-refs" without "for-push" always means "for
> fetch".
>
> I'm conceptually OK with that, but if that is the plan for going
> forward, it was not at all obvious to me (and it does feel rather
> implicit).

Don't get me wrong: I haven't wrapped my head around config context and how it fits into the broader picture yet, but it may be a very good thing to have. So please consider this comment to be about the commit message only.

Based on the motivation you're describing here, I think treating it as uploadpack and adding a NEEDSWORK comment would be a good way forward. If we're moving toward a world with more protocol commands that don't fit in the upload-pack / receive-pack categories, then we need to figure out in more detail what that world looks like:

- do we keep on adding new endpoints, in the same spirit as
  upload-archive?  If so, what endpoint should a client use to get
  capabilities before it decides which endpoint to use?

- do we merge everything in "git serve" except where a specific
  endpoint is needed for protocol v0 compatibility?  That would lose
  the ability to distinguish fetches from pushes without looking at
  the body of requests (which is useful to some people for monitoring,
  blocking, etc) --- do we consider that to be an acceptable loss?

- once we've decided what the future should look like, how does the
  transition to that future look?

Show 7 quoted lines

>> Is there some other more immediate motivation for this patch?  In the
>> spirit of YAGNI, I would rather understand that motivation instead of
>> one that in many possible designs would never materialize.
>
> Without this information, in patch 3 ls-refs cannot know to look at
> uploadpack.hiderefs, unless it makes the implicit assumption that it is
> always serving a fetch.

I think that's a reasonable assumption to make, especially if made explicit using a simple comment. :)

Thanks for explaining, Jonathan