Re: [PATCH v2 6/7] fetch: merge fetching and consuming refs

[Derrick Stolee <stolee@gmail.com>]

On 8/24/2021 6:37 AM, Patrick Steinhardt wrote:
> -		if (ret) {
> -			transport_unlock_pack(transport);
> -			return ret;
> -		}
> +		if (ret)
> +			goto out;

You were just reorganizing this method in the previous patch.
This "goto out" trick could have applied there instead, which
wouldn't complicate that patch and would simplify this one.

But perhaps it would look strange to have the following ending
to the method, even if for only one patch:

	return 0;

out:
	transport_unlock_pack(transport);
	return res;
}

So, feel free to ignore me here. Decide based on your taste.

>  	}
>  
> -	/*
> -	 * Keep the new pack's ".keep" file around to allow the caller
> -	 * time to update refs to reference the new objects.
> -	 */
> -	return 0;
> -}
> -
> -/* Update local refs based on the ref values fetched from a remote */
> -static int consume_refs(struct transport *transport, struct ref *ref_map)
> -{
> -	int connectivity_checked = transport->smart_options
> +	connectivity_checked = transport->smart_options
>  		? transport->smart_options->connectivity_checked : 0;
> -	int ret;
> +
>  	trace2_region_enter("fetch", "consume_refs", the_repository);
>  	ret = store_updated_refs(transport->url,
>  				 transport->remote->name,
>  				 connectivity_checked,
>  				 ref_map);
> -	transport_unlock_pack(transport);
>  	trace2_region_leave("fetch", "consume_refs", the_repository);

This transport_unlock_pack() is leaving the trace2 region. I think
it is unlikely that the loop of unlink_or_warn() calls will take
significant time that affects this region, so it should be fine to
move it.

> +
> +out:
> +	transport_unlock_pack(transport);
>  	return ret;
>  }
...
> -	if (!fetch_refs(transport, ref_map))
> -		consume_refs(transport, ref_map);
> +	fetch_and_consume_refs(transport, ref_map);
>  
>  	if (gsecondary) {
>  		transport_disconnect(gsecondary);
> @@ -1610,7 +1600,7 @@ static int do_fetch(struct transport *transport,
>  				   transport->url);
>  		}
>  	}
> -	if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) {
> +	if (fetch_and_consume_refs(transport, ref_map)) {

The end goal of making these consumers simpler is worth it.

Thanks,
-Stolee