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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 15, 2018, 21:08 UTC
Message-ID: <xmqq602jzriy.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180613213925.10560-3-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 8 quoted lines

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

Show 8 quoted lines

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