Re: [PATCH v4 0/8] ref-in-want

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jun 25, 2018, 23:03 UTC
Message-ID: <20180625230310.210182-1-jonathantanmy@google.com>
In-Reply-To: <20180625185332.164142-1-bmwill@google.com>

> Changes in v4 are fairly minor.  There are a few documentation changes,
> commit message updates, as well as a few small style tweaks based on
> reviewer comments.

Patches 4 and 7, which I have commented on previously, look good.

As for patch 2, it still has a missing period in the documentation that I remarked upon in [1], but I'm not too worried about that. Having said that, Jonathan Nieder suggested [2]:

Show 7 quoted lines

> Stefan mentioned that the spec says
> 
>         * The server MUST NOT send any refs which were not requested
>           using 'want-ref' lines.
> 
> Can client enforce that?  If not, can the spec say SHOULD NOT for the
> server and add a MUST describing appropriate client behavior?

I noticed that you did use "SHOULD NOT" instead of "MUST NOT" - in this case, you should probably also follow the second part about appropriate client behavior - it's probably best to document and implement that we ignore all unwanted refs. But considering this situation, though, I think it's better to just put "MUST NOT" and have the client enforce this.

One more thing - I think that the fetch part needs to be tested more. In particular, test cases similar to that of the upload-pack tests (multiple ref names, ref name + exact SHA-1), and in addition, handling of wildcards (for example, a wildcard that expands to nothing and a wildcard that expands to 2 refs).

[1] https://public-inbox.org/git/20180625174056.53053-1-jonathantanmy@google.com/

[2] https://public-inbox.org/git/20180622230119.GL12013@aiede.svl.corp.google.com/