Re: [PATCH v2 2/8] upload-pack: implement ref-in-want

From: Brandon Williams <bmwill@google.com>
Date: Jun 19, 2018, 18:50 UTC
Message-ID: <20180619185033.GC199585@google.com>
In-Reply-To: <xmqq602jzriy.fsf@gitster-ct.c.googlers.com>

On 06/15, Junio C Hamano wrote:

Show 28 quoted lines

> The story would be different if your request were 
> 
> 	git fetch refs/heads/*:refs/remotes/origin/*
> 
> in which case, you are not even saying "I want this and that ref";
> you are saying "all refs in refs/heads/* whoever ends up serving me
> happens to have".  You may initially contact one of my friends and
> learn that there are 'master' and 'bo' branches (and probably
> others), and after conversation end up talking with me who is stale
> and lack 'bo'.  In such a case, I agree that it is not sensible for
> me to fail the request as a whole and instead serve you whatever
> branches I happen to have.  I may lack 'bo' branch due to mirroring
> lag, but I may also have 'extra' branch that others no longer have
> due to mirroring lag of deletion of that branch!
> 
> But then I think your "git fetch refs/heads/*:refs/remotes/origin/*"
> should not fail not just because I do not have 'bo', but you also
> should grab other old branches I have, which you didn't hear about
> when you made the initial contact with my friend in the mirror pool.
> 
> So, given that, would it make sense for 'want-ref <ref>' request to
> name "a particular ref" as the above document says?  I have a
> feeling that it should allow a pattern to be matched at the server
> side (and it is not an error if the pattern did not match anything),
> in addition to asking for a particular ref (in which case, lack of
> that ref should be a hard failure, at least for the mirror that ends
> up serving the packfile and the final "here are the refs your
> request ended up fetching, with their values").

After some more in-office discussion about this I think I should revert back to making it a hard failure when a client requests a ref which doesn't exist on the server. This makes things more consistent with what happens today if I request a non-existent ref (Although that error is purely on the client). Its no worse than we have today and even with this solution not solving the issues of new/deleted refs (which are rare operations wrt updates) we still can get the benefit of not failing due to a ref updating. This is also very valuable for the servers which have to to ACL checks on individual ref names.

I also think that we should keep this first implementation of ref-in-want simple and *not* include patterns, even if that's what we may want someday down the road. Adding a new capability in the future for support of such patterns would be relatively simple and easy. The reason why I don't think we should add pattern support just yet is due to a request for "want-ref refs/tags/*" or a like request resulting in a larger than expected packfile every time "fetch --tags" is run. The issue being that in a fetch request "refs/tags/*" is too broad of a request and could be requesting 100s of tags when all we really wanted was to get the one or two new tags which are present on the remote (because we already have all the other tags present locally).

So I think the best way is to limit these patterns to the ls-refs request where we can then discover the few tags we're missing and then request just those tags explicitly, keeping the resulting packfile smaller.

Thoughts?

-- 
Brandon Williams