# [PATCH] files-backend: unlock packed store only if locked 4 messages from 2018-02-06 to 2018-02-07. Participants: Jonathan Tan, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/47773 ## Jonathan Tan, 2018-02-06 20:36 Subject: [PATCH] files-backend: unlock packed store only if locked Message-ID: <20180206203615.68504-1-jonathantanmy@google.com> URL: https://gitlist.dev/e/20180206203615.68504-1-jonathantanmy%40google.com ``` 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. Signed-off-by: Jonathan Tan --- I noticed this when one of our servers sent duplicate refs in its ref advertisement (noticed through GIT_TRACE_PACKET). With this change (and before the aforementioned commit 42c7f7ff9685), the error message is "fatal: multiple updates for ref '' not allowed", which gives a bigger clue to the problem. Currently, it is "fatal: BUG: packed_refs_unlock() called when not locked". (I couldn't replicate this problem in C Git.) --- refs/files-backend.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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); transaction->state = REF_TRANSACTION_CLOSED; string_list_clear(&affected_refnames, 0); return ret; -- 2.16.0.rc1.238.g530d649a79-goog ``` ## Jeff King, 2018-02-07 14:42 Subject: Re: [PATCH] files-backend: unlock packed store only if locked Message-ID: <20180207144251.GB27420@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180207144251.GB27420%40sigill.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: > 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. > 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 ``` ## Jonathan Tan, 2018-02-07 18:32 Subject: Re: [PATCH] files-backend: unlock packed store only if locked Message-ID: <20180207103245.d500efb67cc73cf31087f4be@google.com> URL: https://gitlist.dev/e/20180207103245.d500efb67cc73cf31087f4be%40google.com In-Reply-To: <20180207144251.GB27420@sigill.intra.peff.net> ``` On Wed, 7 Feb 2018 09:42:51 -0500 Jeff King wrote: > 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. Ah...thanks, I didn't notice that. > 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. Same here. ``` ## Junio C Hamano, 2018-02-07 20:53 Subject: Re: [PATCH] files-backend: unlock packed store only if locked Message-ID: URL: https://gitlist.dev/e/xmqq372c7c8b.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20180207103245.d500efb67cc73cf31087f4be@google.com> ``` Jonathan Tan writes: > On Wed, 7 Feb 2018 09:42:51 -0500 > Jeff King wrote: > >> 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. > > Ah...thanks, I didn't notice that. > >> 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. > > Same here. I do agree that the double-label approach is more future-proof way, especially if we anticipate that there will be more code after the "attempt initial ref transaction commit" block before the packed-ref-store is unlocked. On the other hand, introduction of the locked_cleanup label can be done as part of such a change that adds new code that needs to be skipped, so I am OK with what is queued there. Thanks, all. ```