Re: [PATCH GSoC v2 1/6] fetch-object-info: request all supported options dynamically

Pablo Sabater <[email protected]> writes:

> In send_object_info_request(), size is hardcoded to be the only option
> sent. In order to support type and future capabilities, replace the
> hardcoded size with a loop that requests everything on
> object_info_options list.
>
> This is safe because the list has already been trimmed previously in
> fetch_object_info() to only contain options that the server supports.

Thinking along and aloud to follow the code, my understanding of how
the relevant data flows in the code path to get here is as follows:

 * 'cat-file --batch-command' processes the remote-object-info
   command, and parse_cmd_remote_object_info() populates the
   object_info_options string list.

 * It puts 'size' and 'type' into the list if needed.

 * get_remote_info() is called, and the string list is attached to
   the '.object_info_options' member of the '.smart_options'
   structure of transport.

 * transport_fetch_object_info() calls fetch_object_info_via_pack(),
   where the local args structure receives in its
   '.object_info_options' member the value pointed to by the
   '.object_info_options' member of the '.smart_options' structure
   of 'gtransport'.

 * fetch_object_info_via_pack() finally calls fetch_object_info(),
   which uses server_supports_feature() to check and discard
   elements from this list that are not supported by the server.

This is how the 'args' structure seen by send_object_info_request()
gets prepared.  The "already been checked to only contain" comment
in the code refers to the loop in fetch_object_info() that uses
server_supports_feature().

So, after tracing the code flow up to this point, I agree with the
"This is safe" claim made in the proposed commit log message.

I always get confused while following code paths in the transport
layer; my ulterior motivation for this comment is that writing it
down once may help refresh my memory the next time I need it.

Thanks.


> Mentored-by: Karthik Nayak <[email protected]>
> Mentored-by: Chandra Pratap <[email protected]>
> Signed-off-by: Pablo Sabater <[email protected]>
> ---
>  fetch-object-info.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fetch-object-info.c b/fetch-object-info.c
> index ba7e179c44..ec8a80b3be 100644
> --- a/fetch-object-info.c
> +++ b/fetch-object-info.c
> @@ -12,13 +12,16 @@
>  static void send_object_info_request(const int fd_out, struct object_info_args *args)
>  {
>  	struct strbuf req_buf = STRBUF_INIT;
> +	struct string_list_item *item;
>  
>  	write_command_and_capabilities(&req_buf, "object-info", args->server_options);
>  
> -	if (unsorted_string_list_has_string(args->object_info_options, "size"))
> -		packet_buf_write(&req_buf, "size");
> -	else if (args->object_info_options->nr)
> -		BUG("only size should be in object_info_options");
> +	/*
> +	 * The list has already been checked to only contain valid and
> +	 * supported fields, so just request everything remaining on it.
> +	 */
> +	for_each_string_list_item(item, args->object_info_options)
> +		packet_buf_write(&req_buf, "%s", item->string);
>  
>  	if (args->oids)
>  		for (size_t i = 0; i < args->oids->nr; i++)