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

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 25, 2021, 14:26 UTC
Message-ID: <439cb382-a4d0-18ad-5475-0cb29fba98e7@gmail.com>
In-Reply-To: <31d9f72edf5c178b2e80c30bb7c0a9bc164ca5eb.1629800774.git.ps@pks.im>

On 8/24/2021 6:37 AM, Patrick Steinhardt wrote:

Show 6 quoted lines

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

Show 24 quoted lines

>  	}
>  
> -	/*
> -	 * 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.

Show 5 quoted lines

> +
> +out:
> +	transport_unlock_pack(transport);
>  	return ret;
>  }

...

Show 12 quoted lines

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