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

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 1, 2021, 12:48 UTC
Message-ID: <YS92mY5WMIEuXvG/@ncase>
In-Reply-To: <0481bb7d-2799-740c-a3de-e16ae05b8f45@gmail.com>

On Wed, Aug 25, 2021 at 10:19:27AM -0400, Derrick Stolee wrote:

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

[snip]

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

The assert isn't really needed: in the subsequent patch, we always unlock the packfile on exit.

Patrick