{"thread":{"id":"47773","subject":"[PATCH] files-backend: unlock packed store only if locked","startedAt":"2018-02-06T20:36:25Z","lastAt":"2018-02-07T20:53:54Z","messageCount":4,"participants":["Jonathan Tan","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"338538","messageId":"20180206203615.68504-1-jonathantanmy@google.com","threadId":"47773","inReplyTo":null,"subject":"[PATCH] files-backend: unlock packed store only if locked","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-02-06T20:36:15Z","receivedAt":"2018-02-06T20:36:25Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In commit 42c7f7ff9685 (\"commit_packed_refs(): remove call to\n`packed_refs_unlock()`\", 2017-06-23), a call to packed_refs_unlock() was\nadded to files_initial_transaction_commit() in order to compensate for\nremoving that call from commit_packed_refs(). However, that call was\nadded in the cleanup section, which is run even if the packed_ref_store\nwas never locked (which happens if an error occurs earlier in the\nfunction).\n\nCreate a new cleanup goto target which runs packed_refs_unlock(), and\nensure that only goto statements after a successful invocation of\npacked_refs_lock() jump there.\n\nSigned-off-by: Jonathan Tan \n---\nI noticed this when one of our servers sent duplicate refs in its ref\nadvertisement (noticed through GIT_TRACE_PACKET). With this change (and\nbefore the aforementioned commit 42c7f7ff9685), the error message is\n\"fatal: multiple updates for ref '' not allowed\", which gives a\nbigger clue to the problem. Currently, it is \"fatal: BUG:\npacked_refs_unlock() called when not locked\".\n\n(I couldn't replicate this problem in C Git.)\n---\n refs/files-backend.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f75d960e1..89bc5584a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2931,13 +2931,14 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n \n \tif (initial_ref_transaction_commit(packed_transaction, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n-\t\tgoto cleanup;\n+\t\tgoto locked_cleanup;\n \t}\n \n+locked_cleanup:\n+\tpacked_refs_unlock(refs->packed_ref_store);\n cleanup:\n \tif (packed_transaction)\n \t\tref_transaction_free(packed_transaction);\n-\tpacked_refs_unlock(refs->packed_ref_store);\n \ttransaction->state = REF_TRANSACTION_CLOSED;\n \tstring_list_clear(&affected_refnames, 0);\n \treturn ret;\n-- \n2.16.0.rc1.238.g530d649a79-goog\n\n"},{"id":"338627","messageId":"20180207144251.GB27420@sigill.intra.peff.net","threadId":"47773","inReplyTo":"20180206203615.68504-1-jonathantanmy@google.com","subject":"Re: [PATCH] files-backend: unlock packed store only if locked","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-07T14:42:51Z","receivedAt":"2018-02-07T14:42:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 06, 2018 at 12:36:15PM -0800, Jonathan Tan wrote:\n\n> In commit 42c7f7ff9685 (\"commit_packed_refs(): remove call to\n> `packed_refs_unlock()`\", 2017-06-23), a call to packed_refs_unlock() was\n> added to files_initial_transaction_commit() in order to compensate for\n> removing that call from commit_packed_refs(). However, that call was\n> added in the cleanup section, which is run even if the packed_ref_store\n> was never locked (which happens if an error occurs earlier in the\n> function).\n> \n> Create a new cleanup goto target which runs packed_refs_unlock(), and\n> ensure that only goto statements after a successful invocation of\n> packed_refs_lock() jump there.\n\nThe explanation and patch look good to me.\n\nBut this all seemed strangely familiar... I think this is the same bug\nas:\n\n https://public-inbox.org/git/20180118143841.1a4c674d@novascotia/\n\nwhich is queued as mr/packed-ref-store-fix. It's listed as \"will merge\nto next\" in the \"what's cooking\" from Jan 31st.\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index f75d960e1..89bc5584a 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2931,13 +2931,14 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,\n> \n> \tif (initial_ref_transaction_commit(packed_transaction, err)) {\n> \t\tret = TRANSACTION_GENERIC_ERROR;\n> -\t\tgoto cleanup;\n> +\t\tgoto locked_cleanup;\n> \t}\n> \n> +locked_cleanup:\n> +\tpacked_refs_unlock(refs->packed_ref_store);\n> cleanup:\n> \tif (packed_transaction)\n> \t\tref_transaction_free(packed_transaction);\n> -\tpacked_refs_unlock(refs->packed_ref_store);\n\nI actually like this double-label a bit more than what is queued on\nmr/packed-ref-store-fix, though I am OK with either solution.\n\n-Peff\n"},{"id":"338649","messageId":"20180207103245.d500efb67cc73cf31087f4be@google.com","threadId":"47773","inReplyTo":"20180207144251.GB27420@sigill.intra.peff.net","subject":"Re: [PATCH] files-backend: unlock packed store only if locked","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-02-07T18:32:45Z","receivedAt":"2018-02-07T18:32:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Wed, 7 Feb 2018 09:42:51 -0500\nJeff King wrote:\n\n> But this all seemed strangely familiar... I think this is the same bug\n> as:\n> \n> https://public-inbox.org/git/20180118143841.1a4c674d@novascotia/\n> \n> which is queued as mr/packed-ref-store-fix. It's listed as \"will merge\n> to next\" in the \"what's cooking\" from Jan 31st.\n\nAh...thanks, I didn't notice that.\n\n> I actually like this double-label a bit more than what is queued on\n> mr/packed-ref-store-fix, though I am OK with either solution.\n\nSame here.\n"},{"id":"338669","messageId":"xmqq372c7c8b.fsf@gitster-ct.c.googlers.com","threadId":"47773","inReplyTo":"20180207103245.d500efb67cc73cf31087f4be@google.com","subject":"Re: [PATCH] files-backend: unlock packed store only if locked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-07T20:53:40Z","receivedAt":"2018-02-07T20:53:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> On Wed, 7 Feb 2018 09:42:51 -0500\n> Jeff King wrote:\n>\n>> But this all seemed strangely familiar... I think this is the same bug\n>> as:\n>> \n>> https://public-inbox.org/git/20180118143841.1a4c674d@novascotia/\n>> \n>> which is queued as mr/packed-ref-store-fix. It's listed as \"will merge\n>> to next\" in the \"what's cooking\" from Jan 31st.\n>\n> Ah...thanks, I didn't notice that.\n>\n>> I actually like this double-label a bit more than what is queued on\n>> mr/packed-ref-store-fix, though I am OK with either solution.\n>\n> Same here.\n\nI do agree that the double-label approach is more future-proof way,\nespecially if we anticipate that there will be more code after the\n\"attempt initial ref transaction commit\" block before the\npacked-ref-store is unlocked. On the other hand, introduction of\nthe locked_cleanup label can be done as part of such a change that\nadds new code that needs to be skipped, so I am OK with what is\nqueued there.\n\nThanks, all.\n\n"}]}