Re: [PATCH v3] fetch-pack: always allow fetching of literal SHA1s

From: Jeff King <peff@peff.net>
Date: May 11, 2017, 10:05 UTC
Message-ID: <20170511100553.ptyazchujgdfxotl@sigill.intra.peff.net>
In-Reply-To: <20170510221157.8971-1-jonathantanmy@google.com>

On Wed, May 10, 2017 at 03:11:57PM -0700, Jonathan Tan wrote:

Show 11 quoted lines

> fetch-pack, when fetching a literal SHA-1 from a server that is not
> configured with uploadpack.allowtipsha1inwant (or similar), always
> returns an error message of the form "Server does not allow request for
> unadvertised object %s". However, it is sometimes the case that such
> object is advertised. This situation would occur, for example, if a user
> or a script was provided a SHA-1 instead of a branch or tag name for
> fetching, and wanted to invoke "git fetch" or "git fetch-pack" using
> that SHA-1.
> 
> Teach fetch-pack to also check the SHA-1s of the refs in the received
> ref advertisement if a literal SHA-1 was given by the user.

Stepping back a bit, what does this mean for a world where we implement protocol extensions to let the client specify a set of refspecs to limit the advertisement?

If we give the server our usual set of fetch refspecs, then we might fail to fulfill a request that would have been advertised outside of that set. And the behavior is confusing and non-transparent to the user. I don't think that really makes sense, though; the advertisement we ask for from the server should include only the bits we're interested in for _this_ fetch.

If we tell the server "we are interested in abcd1234", then it's not going to find any matching ref by name, obviously. So should servers then treat 40-hex names in the incoming refspecs as a request to show any names which have a matching sha1? That works against any server-side optimizations to avoid looking at the complete set of refs, but it would only have to kick in when the user actually specified a single SHA-1 (and even then only when allowAnySHA1 isn't on). So that's probably workable.

None of this is your problem now either way; the advertisement-limiting extension is still vaporware, albeit one we've discussed a lot. I just wanted to make sure we weren't painting ourselves into any corners. And I think this case could probably be handled.

-Peff