* [PATCH] files-backend: unlock packed store only if locked
@ 2018-02-06 20:36 Jonathan Tan
2018-02-07 14:42 ` Jeff King
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Tan @ 2018-02-06 20:36 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, mhagger
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 <jonathantanmy@google.com>
---
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 '<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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] files-backend: unlock packed store only if locked
2018-02-06 20:36 [PATCH] files-backend: unlock packed store only if locked Jonathan Tan
@ 2018-02-07 14:42 ` Jeff King
2018-02-07 18:32 ` Jonathan Tan
0 siblings, 1 reply; 4+ messages in thread
From: Jeff King @ 2018-02-07 14:42 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, mhagger, Junio C Hamano, Mathias Rav
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] files-backend: unlock packed store only if locked
2018-02-07 14:42 ` Jeff King
@ 2018-02-07 18:32 ` Jonathan Tan
2018-02-07 20:53 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Tan @ 2018-02-07 18:32 UTC (permalink / raw)
To: Jeff King; +Cc: git, mhagger, Junio C Hamano, Mathias Rav
On Wed, 7 Feb 2018 09:42:51 -0500
Jeff King <peff@peff.net> 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] files-backend: unlock packed store only if locked
2018-02-07 18:32 ` Jonathan Tan
@ 2018-02-07 20:53 ` Junio C Hamano
0 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2018-02-07 20:53 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Jeff King, git, mhagger, Mathias Rav
Jonathan Tan <jonathantanmy@google.com> writes:
> On Wed, 7 Feb 2018 09:42:51 -0500
> Jeff King <peff@peff.net> 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-02-07 20:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-06 20:36 [PATCH] files-backend: unlock packed store only if locked Jonathan Tan
2018-02-07 14:42 ` Jeff King
2018-02-07 18:32 ` Jonathan Tan
2018-02-07 20:53 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).