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

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 25, 2021, 14:19 UTC
Message-ID: <0481bb7d-2799-740c-a3de-e16ae05b8f45@gmail.com>
In-Reply-To: <56a9158ac331f9911a4347d7d4afc2bbd2cf4d33.1629800774.git.ps@pks.im>

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.

...

Show 13 quoted lines

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

Show 14 quoted lines

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