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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1073 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]