Thread (41 messages) 41 messages, 6 authors, 2016-09-10

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

From: Jeff King <hidden>
Date: 2016-09-02 23:35:55

On Fri, Sep 02, 2016 at 03:06:12PM -0700, Jonathan Tan wrote:
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.
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.
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.
 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help