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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Brandon Williams wrote:

> 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 <bmwill@google.com>
> ---
>  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?

[...]
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1102,9 +1102,10 @@ static void add_shallow_requests(struct strbuf *req_buf,
>  
>  static void add_wants(const struct ref *wants, struct strbuf *req_buf)
>  {
> +	int use_ref_in_want = server_supports_feature("fetch", "ref-in-want", 0);
> +
>  	for ( ; wants ; wants = wants->next) {

Not about this patch: it's kind of confusing that the iterator is called
'wants' even though it points into the middle of the list.  I would even
be tempted to do

	const struct ref *want;
	for (want = wants; want; want = want->next) {

It wouldn't make sense to do in this patch, though.

[...]
> @@ -1122,8 +1123,10 @@ static void add_wants(const struct ref *wants, struct strbuf *req_buf)
>  			continue;
>  		}
>  
> -		remote_hex = oid_to_hex(remote);
> -		packet_buf_write(req_buf, "want %s\n", remote_hex);
> +		if (!use_ref_in_want || wants->exact_oid)
> +			packet_buf_write(req_buf, "want %s\n", oid_to_hex(remote));
> +		else
> +			packet_buf_write(req_buf, "want-ref %s\n", wants->name);

Very neat.

[...]
> @@ -1334,6 +1337,29 @@ static void receive_shallow_info(struct fetch_pack_args *args,
>  	args->deepen = 1;
>  }
>  
> +static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs)
> +{
> +	process_section_header(reader, "wanted-refs", 0);
> +	while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
> +		struct object_id oid;
> +		const char *end;
> +		struct ref *r = NULL;
> +
> +		if (parse_oid_hex(reader->line, &oid, &end) || *end++ != ' ')
> +			die("expected wanted-ref, got '%s'", reader->line);
> +
> +		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?

> +		}
> +	}
> +
> +	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?

[...]
> --- a/remote.c
> +++ b/remote.c
> @@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs,
>  		if (refspec->exact_sha1) {
>  			ref_map = alloc_ref(name);
>  			get_oid_hex(name, &ref_map->old_oid);
> +			ref_map->exact_oid = 1;

Sensible.  The alternative would be that we check whether the
refname is oid-shaped at want-ref generation time, which would be
unnecessarily complicated.

[...]
> --- a/remote.h
> +++ b/remote.h
> @@ -73,6 +73,7 @@ struct ref {

Not about this patch: why is this in remote.h instead of ref.h?

>  		force:1,
>  		forced_update:1,
>  		expect_old_sha1:1,
> +		exact_oid:1,
>  		deletion:1;

Looks good, and we have room for plenty more bits there.

[...]
> +++ b/t/t5703-upload-pack-ref-in-want.sh

Nice.

Thanks for a pleasant read.

Sincerely,
Jonathan