Subject: Re: [PATCH v2 5/7] fetch: refactor fetch refs to be more extendable

From: Derrick Stolee <[email-masked]>
Date: Wed, 25 Aug 2021 10:19:27 -0400

On 8/24/2021 6:37 AM, Patrick Steinhardt wrote:
> Refactor `fetch_refs()` code to make it more extendable by explicitly
> handling error cases. The refactored code should behave the same.
...
> +	/*
> +	 * We don't need to perform a fetch in case we can already satisfy all
> +	 * refs.
> +	 */
> +	ret = check_exist_and_connected(ref_map);
>  	if (ret) {
>  		trace2_region_enter("fetch", "fetch_refs", the_repository);
>  		ret = transport_fetch_refs(transport, ref_map);
>  		trace2_region_leave("fetch", "fetch_refs", the_repository);
> +		if (ret) {
> +			transport_unlock_pack(transport);
> +			return ret;
> +		}>  	}

I see that this nested organization makes it more clear what cases
lead into this error state.

> -	if (!ret)
> -		/*
> -		 * Keep the new pack's ".keep" file around to allow the caller
> -		 * time to update refs to reference the new objects.
> -		 */
> -		return 0;
> -	transport_unlock_pack(transport);
> -	return ret;
> +
> +	/*
> +	 * Keep the new pack's ".keep" file around to allow the caller
> +	 * time to update refs to reference the new objects.
> +	 */
> +	return 0;

And it happens that 'ret' is zero here. Should we keep returning 'ret'
or perhaps add an "assert(!ret);" before the return? The assert()
doesn't do much, but at minimum would serve as an extra indicator to
anyone working in this method in the future.

Thanks,
-Stolee