From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 01 Aug 2022 13:30:00 GMT
Subject: Re: [PATCH v5 5/6] transport: add client support for object-info
Message-ID: <b33ec478-8858-faa7-2678-f02559426539@gmail.com>
In-Reply-To: <20220728230210.2952731-6-calvinwan@google.com>

Hi Calvin

On 29/07/2022 00:02, Calvin Wan wrote:
> Sometimes it is useful to get information about an object without having
> to download it completely. The server logic has already been implemented
> as “a2ba162cda (object-info: support for retrieving object info,
> 2021-04-20)”. This patch adds client functions to communicate with the
> server.
> 
> The client currently supports requesting a list of object ids with
> features 'size' and 'type' from a v2 server. If a server does not
> advertise either of the requested features, then the client falls back
> to making the request through 'fetch'.

I'm confused by the 'type' support, I might have missed something as I'm 
not familiar with this code but I couldn't see where we parse the type 
returned by the server.

> +	for (i = 0; i < args.object_info_options->nr; i++) {
> +		if (packet_reader_read(&reader) != PACKET_READ_NORMAL) {
> +			check_stateless_delimiter(transport->stateless_rpc, &reader, "stateless delimiter expected");

This is one of a number of lines in this series that are way over the 80 
column limit.

> +			return -1;
> +		}
> +		if (unsorted_string_list_has_string(args.object_info_options, reader.line)) {
> +			if (!strcmp(reader.line, "size"))
> +				size_index = i;

Should we be checking for "type" as well? Also does protocol-v2.txt need 
updating as it only mentions "size" as an attribute.

> +			continue;
> +		}
> +		return -1;
> +	}
> +
> +	i = 0;
> +	while (packet_reader_read(&reader) == PACKET_READ_NORMAL && i < args.oids->nr) {
> +		struct string_list object_info_values = STRING_LIST_INIT_DUP;
> +
> +		string_list_split(&object_info_values, reader.line, ' ', -1);
> +		if (0 <= size_index) {

To avoid a possible out-of-bounds access we need to check that 
size_index + 1 < object_info_value.nr in case the server response is 
malformed

> +			if (!strcmp(object_info_values.items[1 + size_index].string, ""))
> +				die("object-info: not our ref %s",

I'm a bit confused by this message is it trying to say "object %s is 
missing on the server"?

> +					object_info_values.items[0].string);
> +			*(*object_info_data)[i].sizep = strtoul(object_info_values.items[1 + size_index].string, NULL, 10);

As Junio pointed out in his comments in v4 there is no error checking 
here - we should check the server has actually returned a number. Note 
that strtoul() will happily parse negative numbers so we probably want 
to do something like

const char *endp
errno = 0
if (!isdigit(*object_info_values.items[1 + size_index].string))
   die("...")
*(*object_info_data)[i].sizep = strtoul(object_info_values.items[1 + 
size_index].string, &endp, 10);
if (errno || *endp)
   die("...")

Should be we checking the object id matches what we asked for? (I'm not 
sure if protocol-v2.txt mentions the order in which the objects are 
returned)

Should we be parsing the object type here as well?

> @@ -392,8 +468,25 @@ static int fetch_refs_via_pack(struct transport *transport,
>   	args.server_options = transport->server_options;
>   	args.negotiation_tips = data->options.negotiation_tips;
>   	args.reject_shallow_remote = transport->smart_options->reject_shallow;
> -
> -	if (!data->got_remote_heads) {
> +	args.object_info = transport->smart_options->object_info;
> +
> +	if (transport->smart_options && transport->smart_options->object_info) {
> +		struct ref *ref = object_info_refs;
> +
> +		if (!fetch_object_info(transport, data->options.object_info_data))
> +			goto cleanup;
> +		args.object_info_data = data->options.object_info_data;
> +		args.quiet = 1;
> +		args.no_progress = 1;
> +		for (i = 0; i < transport->smart_options->object_info_oids->nr; i++) {
> +			struct ref *temp_ref = xcalloc(1, sizeof (struct ref));

Using CALLOC_ARRAY() or p = xcalloc(1, sizeof(*p)) would be safer here 
(and everywhere else where you have used xcalloc()) as it ensures we 
allocate the correct size.

> diff --git a/transport.h b/transport.h
> index b5bf7b3e70..5512fdb140 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -31,6 +32,12 @@ struct git_transport_options {
>   	 */
>   	unsigned connectivity_checked:1;
>   
> +	/*
> +	 * Transport will attempt to pull only object-info. Fallbacks
> +	 * to pulling entire object if object-info is not supported
> +	 */

Is it definitely true that we fallback to pulling the entire object? - 
there is at least one place above where we do

 > +	if (transport->smart_options->object_info) {
 > +		die(_("--object-info requires protocol v2"));
 > +	}
 >

Best Wishes

Phillip