Re: [PATCH 04/13] upload-pack: use 'struct upload_pack_data' in upload_pack()

[Derrick Stolee <stolee@gmail.com>]

On 5/15/2020 6:04 AM, Christian Couder wrote:
> As we cleanup 'upload-pack.c' by using 'struct upload_pack_data'
> more thoroughly, let's use 'struct upload_pack_data' in
> upload_pack().
> 
> This will make it possible in followup commits to remove a lot
> of static variables and local variables that have the same name
> and purpose as fields in 'struct upload_pack_data'. This will
> also make upload_pack() work in a more similar way as
> upload_pack_v2().
> 
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  upload-pack.c | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/upload-pack.c b/upload-pack.c
> index 9aeb3477c9..cb336c5713 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1144,18 +1144,17 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
>  void upload_pack(struct upload_pack_options *options)
>  {
>  	struct string_list symref = STRING_LIST_INIT_DUP;
> -	struct object_array want_obj = OBJECT_ARRAY_INIT;
>  	struct packet_reader reader;
> -	struct list_objects_filter_options filter_options;
> +	struct upload_pack_data data;
>  
>  	stateless_rpc = options->stateless_rpc;
>  	timeout = options->timeout;
>  	daemon_mode = options->daemon_mode;
>  
> -	memset(&filter_options, 0, sizeof(filter_options));
> -

I checked that upload_pack_data_init() runs memset(), which
initializes the memory for data.filter_options. Thanks.

>  	git_config(upload_pack_config, NULL);
>  
> +	upload_pack_data_init(&data);
> +
>  	head_ref_namespaced(find_symref, &symref);
>  
>  	if (options->advertise_refs || !stateless_rpc) {
> @@ -1169,21 +1168,24 @@ void upload_pack(struct upload_pack_options *options)


The control flow below was hard to parse from the diff because
a whitespace line split up the "-" lines and the "+" lines.
I reorder them here:

Old code:

> -	if (options->advertise_refs)
> -		return;
>  
> -	packet_reader_init(&reader, 0, NULL, 0,
> -			   PACKET_READ_CHOMP_NEWLINE |
> -			   PACKET_READ_DIE_ON_ERR_PACKET);
>  
> -	receive_needs(&reader, &want_obj, &filter_options);
> -	if (want_obj.nr) {
> -		struct object_array have_obj = OBJECT_ARRAY_INIT;
> -		get_common_commits(&reader, &have_obj, &want_obj);
> -		create_pack_file(&have_obj, &want_obj, &filter_options);
>	}
> -	list_objects_filter_release(&filter_options);
>  }

New code:

> +	if (!options->advertise_refs) {
> +		packet_reader_init(&reader, 0, NULL, 0,
> +				   PACKET_READ_CHOMP_NEWLINE |
> +				   PACKET_READ_DIE_ON_ERR_PACKET);
>  
> +		receive_needs(&reader, &data.want_obj, &data.filter_options);
> +		if (data.want_obj.nr) {
> +			get_common_commits(&reader,
> +					   &data.have_obj,
> +					   &data.want_obj);
> +			create_pack_file(&data.have_obj,
> +					 &data.want_obj,
> +					 &data.filter_options);
> +		}
>  	}
>  
> +	upload_pack_data_clear(&data);
>  }

The major change is that the "options->advertise_refs" case
now clears the data when before it did not. This seems like
a good change to make.

Also, the code that has now been surrounded by an if block
isn't so large as to justify a "goto cleanup" case.

Thanks,
-Stolee