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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 22, 2018, 23:01 UTC
Message-ID: <20180622230119.GL12013@aiede.svl.corp.google.com>
In-Reply-To: <20180620213235.10952-9-bmwill@google.com>

Hi,

Brandon Williams wrote:

Show 11 quoted lines

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

[...]

Show 9 quoted lines

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

[...]

Show 10 quoted lines

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

[...]

Show 20 quoted lines

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

[...]

Show 7 quoted lines

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

Show 5 quoted lines

>  		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