Re: [PATCH 4/5] upload-pack: implement protocol extension "symbolic-ref"

From: Jeff King <peff@peff.net>
Date: Nov 30, 2008, 18:02 UTC
Message-ID: <20081130180214.GA10375@coredump.intra.peff.net>
In-Reply-To: <1228039053-31099-2-git-send-email-gitster@pobox.com>

On Sun, Nov 30, 2008 at 01:57:29AM -0800, Junio C Hamano wrote:

Show 14 quoted lines

> Although the new capability is advertised on the first available ref in
> the same way as the other extensions, the way to trigger this extension
> from the receiving end is not by adding it in the first "want" line as
> usual.  Instead, the receiving end sends a "symbolic-ref" request packet
> before the usual sequence of "want" lines.
> 
> This is unfortunate because it forces an extra round trip (receiving end
> sends a "please tell me symbolic-ref" packet, and then upload side sends
> "here are the requested information" packet), but it has to be implemented
> this way because (1) ls-remote may need to ask for this information, in
> which case there is no "want" to be sent; and (2) the transport API
> insists that transport_get_remote_refs() returns the final list, and does
> not allow augmenting what was initially obtained from the call to it by
> later calls to transport_fetch_refs() easily.

Hrm. For (1), could we allow either interaction method? IOW, allow requesting a symref on the first want line, _or_ by separate "symbolic ref" packet? That would allow clients who are using "want" to piggy-back the symref request as an optimization, but not restrict those that just want to ask for it?

Not being too familiar with the transport code, I can't speak to (2). But it would be sad to see an internal API shortcoming that we have _now_ stick us with a crappy protocol _forever_. We can fix the API, but once the protocol is in the wild, it becomes much harder to change.

Show 6 quoted lines

> It also is unfortunate that with this change on the server side, older
> clients running "ls-remote" without actually downloading anything will
> trigger "The remote end hung up unexpectedly" error on the uploading side,
> which is annoying even though it is benign.  You can observe it by applying
> only this patch but not the patch to the receiving end and running t5601
> under "sh -x".

And obviously this wouldn't go away with the proposal above, since we'd still have to be looking for the "tell me this symbolic ref" packet. But the solution you outlined in 0/5 sounded sane to me (and I think it definitely needs to be addressed).

-Peff