Re: [PATCH v2 2/2] connect: advertized capability is not a ref

From: Jeff King <peff@peff.net>
Date: Sep 2, 2016, 23:35 UTC
Message-ID: <20160902233547.mzgluioc7hhabalw@sigill.intra.peff.net>
In-Reply-To: <174c8ca6638f1cd3145a628925e65655b56af366.1472853827.git.jonathantanmy@google.com>

On Fri, Sep 02, 2016 at 03:06:12PM -0700, Jonathan Tan wrote:

Show 6 quoted lines

> The cause is that, since v3.1.0.201309270735-rc1~22 (Advertise capabilities
> with no refs in upload service., 2013-08-08), JGit's ref advertisement includes
> a ref named capabilities^{} to advertise its capabilities on, while git's ref
> advertisement is empty in this case. This allows the client to learn about the
> server's capabilities and is needed, for example, for fetch-by-sha1 to work
> when no refs are advertised.

So does JGit actually have a fetch-by-sha1 that works in such a case (obviously the repository has to have _some_ objects, so is this a feature where the server hides some of the refs?).

I was thinking we did not have one in git (i.e., we have nothing to allow fetching arbitrary sha1s). But combining hideRefs with allowTipSHA1InWant could trigger this case.

Show 9 quoted lines

> This also affects "ls-remote". For example, against an empty repository served
> by JGit:
> 
> 	$ git ls-remote git://localhost/tmp/empty
> 	0000000000000000000000000000000000000000        capabilities^{}
> 
> Git advertises the same capabilities^{} ref in its ref advertisement for push
> but since it never remembered to do so for fetch, the client forgot to handle
> this case. Handle it.

As you can probably guess from my previous emails in this thread, I don't think it is "never remembered to do so". It is more like "never intended to do so and was documented incorrectly".

That doesn't make things clear cut, of course. But I think the real story is more like (I dug a little in the history, as you'll see, but didn't look for conversations in the list archive, so take this with the appropriate grain of salt):

  0. Upload-pack existed without this capabilities^{} trick for some time.

  1. Receive-pack learned the capabilities^{} trick, and send-pack on
     the client side learned to accept it (this looks like it came along
     with the first capability in cfee10a (send-pack/receive-pack: allow
     errors to be reported back to pusher., 2005-12-25).

  2. Later, b31222c (Update packfile transfer protocol documentation,
     2009-11-03) tried to document the upload-pack and receive-pack
     protocols, but mistakenly documented both as having
     capabilities^{}.

  3. In ae1f469 (Advertise capabilities with no refs in upload service.,
     2013-08-08), JGit started sending these in its upload-pack
     equivalent, according to the documentation from (3).

So now we are in a state where JGit behavior does not match Git behavior. Since JGit versions have existed in the wild for a few years, it's a good idea for all clients to be liberal and accept both cases.

Show 10 quoted lines

> In this aspect, JGit is compliant with the specification in pack-protocol.txt.
> Because git client versions without this fix are expected to exist in the wild
> for a while, we should not change the server to always send the capabilities^{}
> line when there are no refs to advertise yet.  A transition will take multiple
> steps:
> 
>  1. This patch, which updates the client
> 
>  2. Update pack-protocol to clarify that both server behaviors must be
>     tolerated.

These two seem like obvious improvements.

Show 7 quoted lines

>  3. Add a configuration variable to allow git upload-pack to advertise
>     capabilities when there are no refs to advertise.  Leave it disabled
>     by default since git clients can't be counted on to have this patch (1)
>     yet.
> 
>  4. After a year or so, flip the default for that server configuration
>     variable to true.

I think "a year or so" is not nearly long enough, as this is not a backwards-compatible change. The only thing that mitigates it is that an older client doesn't barf totally, but just generates funny output.

I'd be more interested in the pain of this transition if there was a concrete use case for "hide literally all refs, but still allow fetches". Is that a thing that people do?

-Peff