Re: [PATCH v3 8/8] fetch-pack: implement ref-in-want

On 06/22, Jonathan Nieder wrote:

Hi,

Brandon Williams wrote:

quoted

Implement ref-in-want on the client side so that when a server supports
the "ref-in-want" feature, a client will send "want-ref" lines for each
reference the client wants to fetch.

Signed-off-by: Brandon Williams <redacted>
---
 fetch-pack.c                       | 35 +++++++++++++++++++++++++++---
 remote.c                           |  1 +
 remote.h                           |  1 +
 t/t5703-upload-pack-ref-in-want.sh |  4 ++--
 4 files changed, 36 insertions(+), 5 deletions(-)

This commit message doesn't tell me what ref-in-want is or is for.  Could
it include

 A. a pointer to Documentation/technical/protocol-v2.txt, or
 B. an example illustrating the effect e.g. using GIT_TRACE_PACKET

or both?

Yeah I can imporve the message here.

quoted

+
+		for (r = refs; r; r = r->next) {
+			if (!strcmp(end, r->name)) {
+				oidcpy(&r->old_oid, &oid);
+				break;
+			}

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?

Yeah I can update the docs in an earlier patch.

quoted

+		}
+	}
+
+	if (reader->status != PACKET_READ_DELIM)

The spec says

        * This section is only included if the client has requested a
          ref using a 'want-ref' line and if a packfile section is also
          included in the response.

What should happen if the client already has all the relevant objects
(or in other words if there is no packfile to send in the packfile
section)?  Is the idea that the client should already have known that
based on the ref advertisement?  What if ref values change to put us
in that state between the ls-refs and fetch steps?

I believe the current functionality is that if all wants are already
satisfied by all haves then an empty packfile is sent, so that would
fall under that case.


-- 
Brandon Williams