Re: [PATCH] files-backend: unlock packed store only if locked
- From
Jeff King <peff@peff.net>
- Date
- Feb 7, 2018, 14:42 UTC
- Message-ID
- <20180207144251.GB27420@sigill.intra.peff.net>
- In-Reply-To
- <20180206203615.68504-1-jonathantanmy@google.com>
On Tue, Feb 06, 2018 at 12:36:15PM -0800, Jonathan Tan wrote:
Show 11 quoted lines
> In commit 42c7f7ff9685 ("commit_packed_refs(): remove call to
> `packed_refs_unlock()`", 2017-06-23), a call to packed_refs_unlock() was
> added to files_initial_transaction_commit() in order to compensate for
> removing that call from commit_packed_refs(). However, that call was
> added in the cleanup section, which is run even if the packed_ref_store
> was never locked (which happens if an error occurs earlier in the
> function).
>
> Create a new cleanup goto target which runs packed_refs_unlock(), and
> ensure that only goto statements after a successful invocation of
> packed_refs_lock() jump there.The explanation and patch look good to me.
But this all seemed strangely familiar... I think this is the same bug as:
https://public-inbox.org/git/20180118143841.1a4c674d@novascotia/
which is queued as mr/packed-ref-store-fix. It's listed as "will merge to next" in the "what's cooking" from Jan 31st.
Show 18 quoted lines
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index f75d960e1..89bc5584a 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2931,13 +2931,14 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
>
> if (initial_ref_transaction_commit(packed_transaction, err)) {
> ret = TRANSACTION_GENERIC_ERROR;
> - goto cleanup;
> + goto locked_cleanup;
> }
>
> +locked_cleanup:
> + packed_refs_unlock(refs->packed_ref_store);
> cleanup:
> if (packed_transaction)
> ref_transaction_free(packed_transaction);
> - packed_refs_unlock(refs->packed_ref_store);I actually like this double-label a bit more than what is queued on mr/packed-ref-store-fix, though I am OK with either solution.
-Peff