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

Brandon Williams [off-list ref] writes:

Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids.  This causes
a vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement.

In other words, your "git fetch refs/heads/master" initially
contacts a mirror A and learns that its refs/heads/master is at
commit X, starts negotiation, and end up with getting served by a
mirror B that is slightly beind mirror A, which wants to give you a
packfile that brings you up to say commit X~3.  You want to be able
to say "I want to update my refs/remotes/origin/master with whatever
is (close to) the latest at your refs/heads/master", not "I just
learned refs/heads/master is at X from one of you, I demand getting
updated to that exact commit."

For that, you'd say, after getting ref advertisement, "I want
the tip of refs/heads/master that one of you have, whoever ends up
serving me eventually".

But when you said "I want some version of refs/heads/bo", is it
sensible for me to say "Oh, among 7 mirrors, you unluckily ended up
with me who is most behind and do not even have 'bo' branch yet, so
I won't be giving you that branch (or history leading to the commit
my 6 friends may have at refs/heads/bo)"?  

What is the end-user visible effect of "ignoring"?  Does your "git
fetch bo" that were unluckily served by me who do not yet have the
branch error out?  If so, preferrably the conversation should faile
before packfile generation and transfer.

I am guessing that you do not want to fail the negotiation if your
"git fetch bo" happen to contact me (who lack 'bo') first, as long
as the conversation continues and switches later to one of my
friends who can fulfill the request, and that is why you are
forbidding me (who got initial contact and saw "want-ref bo") from
failing the whole thing.  But it is unclear who is responsible for
erroring out the whole "git fetch bo" *if* unlucky you ended up
getting served by the most stale mirror that does not even have
'bo'.

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

11 more lines

OK.  In my initial reading, I somehow failed to read this piece
before going on to the next hunk ...

... and wondered how the reader knows where wanted-ref list ends; we
will see a flush (or is it delim?  either is accepted?) where the
list ends, which is good.

On 06/15, Junio C Hamano wrote:

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