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

[Junio C Hamano <gitster@pobox.com>]

Brandon Williams <bmwill@google.com> 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".

> +    want-ref <ref>
> +	Indicates to the server that the client wants to retrieve a
> +	particular ref, where <ref> is the full name of a ref on the
> +	server.  A server should ignore any "want-ref <ref>" lines where
> +	<ref> doesn't exist on the server.

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
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").

>  The response of `fetch` is broken into a number of sections separated by
>  delimiter packets (0001), with each section beginning with its section
>  header.
>  
>      output = *section
> -    section = (acknowledgments | shallow-info | packfile)
> +    section = (acknowledgments | shallow-info | wanted-refs | packfile)
>  	      (flush-pkt | delim-pkt)

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

> @@ -319,6 +329,10 @@ header.
>      shallow = "shallow" SP obj-id
>      unshallow = "unshallow" SP obj-id
>  
> +    wanted-refs = PKT-LINE("wanted-refs" LF)
> +		  *PKT-LINE(wanted-ref LF)
> +    wanted-ref = obj-id SP refname
> +

... 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.