Re: [PATCH] files_initial_transaction_commit(): only unlock if locked

From: Jeff King <peff@peff.net>
Date: Jan 18, 2018, 14:19 UTC
Message-ID: <20180118141914.GA32718@sigill.intra.peff.net>
In-Reply-To: <20180118143841.1a4c674d@novascotia>

On Thu, Jan 18, 2018 at 02:38:41PM +0100, Mathias Rav wrote:

Show 15 quoted lines

> Running git clone --single-branch --mirror -b TAGNAME previously
> triggered the following error message:
> 
> 	fatal: multiple updates for ref 'refs/tags/TAGNAME' not allowed.
> 
> This error condition is handled in files_initial_transaction_commit().
> 
> 42c7f7ff9 ("commit_packed_refs(): remove call to `packed_refs_unlock()`", 2017-06-23)
> introduced incorrect unlocking in the error path of this function,
> which changes the error message to
> 
> 	fatal: BUG: packed_refs_unlock() called when not locked
> 
> Move the call to packed_refs_unlock() above the "cleanup:" label
> since the unlocking should only be done in the last error path.

Thanks, this solution looks correct to me. It's pretty low-impact since the locking is the second-to-last thing in the function, so we don't have to re-add the unlock to a bunch of error code paths. But one alternative would be to just do:

  if (packed_refs_is_locked(refs))
	packed_refs_unlock(refs->packed_ref_store);

in the cleanup section.

-Peff