{"thread":{"id":"47632","subject":"[PATCH] files_initial_transaction_commit(): only unlock if locked","startedAt":"2018-01-18T13:44:05Z","lastAt":"2018-01-22T10:04:06Z","messageCount":5,"participants":["Mathias Rav","Jeff King","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"336774","messageId":"20180118143841.1a4c674d@novascotia","threadId":"47632","inReplyTo":null,"subject":"[PATCH] files_initial_transaction_commit(): only unlock if locked","fromName":"Mathias Rav","fromEmail":"m@git.strova.dk","sentAt":"2018-01-18T13:38:41Z","receivedAt":"2018-01-18T13:44:05Z","isPatch":true,"sender":{"key":"m@git.strova.dk","avatar":"https://avatars.githubusercontent.com/u/373639?v=4"},"body":"Running git clone --single-branch --mirror -b TAGNAME previously\ntriggered the following error message:\n\n\tfatal: multiple updates for ref 'refs/tags/TAGNAME' not allowed.\n\nThis error condition is handled in files_initial_transaction_commit().\n\n42c7f7ff9 (\"commit_packed_refs(): remove call to `packed_refs_unlock()`\", 2017-06-23)\nintroduced incorrect unlocking in the error path of this function,\nwhich changes the error message to\n\n\tfatal: BUG: packed_refs_unlock() called when not locked\n\nMove the call to packed_refs_unlock() above the \"cleanup:\" label\nsince the unlocking should only be done in the last error path.\n\nSigned-off-by: Mathias Rav <m@git.strova.dk>\n---\n refs/files-backend.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a80d60aa0..afe5c4e94 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2874,13 +2874,12 @@ 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}\n \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.15.1\n\n"},{"id":"336776","messageId":"20180118141914.GA32718@sigill.intra.peff.net","threadId":"47632","inReplyTo":"20180118143841.1a4c674d@novascotia","subject":"Re: [PATCH] files_initial_transaction_commit(): only unlock if locked","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-18T14:19:14Z","receivedAt":"2018-01-18T14:19:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 18, 2018 at 02:38:41PM +0100, Mathias Rav wrote:\n\n> Running git clone --single-branch --mirror -b TAGNAME previously\n> triggered the following error message:\n> \n> \tfatal: multiple updates for ref 'refs/tags/TAGNAME' not allowed.\n> \n> This error condition is handled in files_initial_transaction_commit().\n> \n> 42c7f7ff9 (\"commit_packed_refs(): remove call to `packed_refs_unlock()`\", 2017-06-23)\n> introduced incorrect unlocking in the error path of this function,\n> which changes the error message to\n> \n> \tfatal: BUG: packed_refs_unlock() called when not locked\n> \n> Move the call to packed_refs_unlock() above the \"cleanup:\" label\n> since the unlocking should only be done in the last error path.\n\nThanks, this solution looks correct to me. It's pretty low-impact since\nthe locking is the second-to-last thing in the function, so we don't\nhave to re-add the unlock to a bunch of error code paths. But one\nalternative would be to just do:\n\n  if (packed_refs_is_locked(refs))\n\tpacked_refs_unlock(refs->packed_ref_store);\n\nin the cleanup section.\n\n-Peff\n"},{"id":"336946","messageId":"xmqqwp0do5sg.fsf@gitster.mtv.corp.google.com","threadId":"47632","inReplyTo":"20180118141914.GA32718@sigill.intra.peff.net","subject":"Re: [PATCH] files_initial_transaction_commit(): only unlock if locked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-19T22:14:07Z","receivedAt":"2018-01-19T22:14:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Jan 18, 2018 at 02:38:41PM +0100, Mathias Rav wrote:\n>\n>> Running git clone --single-branch --mirror -b TAGNAME previously\n>> triggered the following error message:\n>> \n>> \tfatal: multiple updates for ref 'refs/tags/TAGNAME' not allowed.\n>> \n>> This error condition is handled in files_initial_transaction_commit().\n>> \n>> 42c7f7ff9 (\"commit_packed_refs(): remove call to `packed_refs_unlock()`\", 2017-06-23)\n>> introduced incorrect unlocking in the error path of this function,\n>> which changes the error message to\n>> \n>> \tfatal: BUG: packed_refs_unlock() called when not locked\n>> \n>> Move the call to packed_refs_unlock() above the \"cleanup:\" label\n>> since the unlocking should only be done in the last error path.\n>\n> Thanks, this solution looks correct to me. It's pretty low-impact since\n> the locking is the second-to-last thing in the function, so we don't\n> have to re-add the unlock to a bunch of error code paths. But one\n> alternative would be to just do:\n>\n>   if (packed_refs_is_locked(refs))\n> \tpacked_refs_unlock(refs->packed_ref_store);\n>\n> in the cleanup section.\n\nYeah, that may be a more future-proof alternative, and just as you\nsaid the patch as posted would be sufficient, too.\n\nThanks.\n"},{"id":"337053","messageId":"8328c28a-d93b-1076-20d3-823dbddf7e4c@alum.mit.edu","threadId":"47632","inReplyTo":"xmqqwp0do5sg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] files_initial_transaction_commit(): only unlock if locked","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2018-01-22T09:25:37Z","receivedAt":"2018-01-22T09:33:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/19/2018 11:14 PM, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>> On Thu, Jan 18, 2018 at 02:38:41PM +0100, Mathias Rav wrote:\n>>\n>>> Running git clone --single-branch --mirror -b TAGNAME previously\n>>> triggered the following error message:\n>>>\n>>> \tfatal: multiple updates for ref 'refs/tags/TAGNAME' not allowed.\n>>>\n>>> This error condition is handled in files_initial_transaction_commit().\n>>>\n>>> 42c7f7ff9 (\"commit_packed_refs(): remove call to `packed_refs_unlock()`\", 2017-06-23)\n>>> introduced incorrect unlocking in the error path of this function,\n>>> which changes the error message to\n>>>\n>>> \tfatal: BUG: packed_refs_unlock() called when not locked\n>>>\n>>> Move the call to packed_refs_unlock() above the \"cleanup:\" label\n>>> since the unlocking should only be done in the last error path.\n>>\n>> Thanks, this solution looks correct to me. It's pretty low-impact since\n>> the locking is the second-to-last thing in the function, so we don't\n>> have to re-add the unlock to a bunch of error code paths. But one\n>> alternative would be to just do:\n>>\n>>   if (packed_refs_is_locked(refs))\n>> \tpacked_refs_unlock(refs->packed_ref_store);\n>>\n>> in the cleanup section.\n> \n> Yeah, that may be a more future-proof alternative, and just as you\n> said the patch as posted would be sufficient, too.\n\nEither solution LGTM. Thanks for finding and fixing this bug.\n\nBut let's also take a step back. The invocation\n\n    git clone --single-branch --mirror -b TAGNAME\n\nseems curious. Does it even make sense to use `--mirror` and\n`--single-branch` at the same time? What should it do?\n\nNormally `--mirror` implies (aside from `--bare`) that the remote\nreferences should be converted 1:1 to local references and should be\noverwritten at every fetch; i.e., the refspec should be set to\n`+refs/*:refs/*`.\n\nTo me the most plausible interpretation of `--mirror --single-branch -b\nBRANCHNAME` would be that the single branch should be fetched and made\nthe HEAD, and the refspec should be set to\n`+refs/heads/BRANCHNAME:refs/heads/BRANCHNAME`. It also wouldn't be very\nsurprising if it were forbidden to use these options together.\n\nCurrently, we do neither of those things. Instead we fetch that one\nreference (as `refs/heads/BRANCHNAME`) but set the refspec to\n`+refs/*:refs/*`; i.e., the next fetch would fetch all of the history.\n\nIt's even more mind-bending if `-b` is passed a `TAGNAME` rather than a\n`BRANCHNAME`. The documentation says that `-b TAGNAME` \"detaches the\nHEAD at that commit in the resulting repository\". If `--single-branch -b\nTAGNAME` is used, then the refspec is set to\n`+refs/tags/TAGNAME:refs/tags/TAGNAME`. But what if `--mirror` is also used?\n\nCurrently, this fails, apparently because `--mirror` and `-b TAGNAME`\neach independently try to set `refs/tags/TAGNAME` (presumably to the\nsame value). *If* this is a useful use case, we could fix it so that it\ndoesn't fail. If not, maybe we should prohibit it explicitly and emit a\nclearer error message.\n\nMathias: if you encountered this problem in the real world, what were\nyou trying to accomplish? What behavior would you have expected?\n\nMaybe the behavior could be made more sane if there were a way to get\nthe 1:1 reference mapping that `--mirror` implies without also getting\n`--bare` [1]. Suppose there were a `--refspec` option. Then instead of\n\n    git clone --mirror --single-branch -b BRANCHNAME\n\nwith it's non-obvious semantics, you could prohibit that use and instead\nsupport\n\n    git clone --bare\n--refspec='+refs/heads/BRANCHNAME:refs/heads/BRANCHNAME'\n\nwhich seems clearer in its intent, if perhaps not super obvious. Or you\ncould give `clone` a `--no-fetch` option, which would give the user a\ntime to intervene between setting up the basic clone config and actually\nfetching objects.\n\nMichael\n\n\n[1] It seems like\n\n        git clone --config remote.origin.fetch='+refs/*:refs/*' clone ...\n\n    might do it, but that actually ends up setting up two refspecs and\nonly honoring `+refs/heads/*:refs/remotes/origin/*` for the initial\nfetch. Plus it is pretty obscure.\n"},{"id":"337054","messageId":"20180122110357.683b21a7@novascotia","threadId":"47632","inReplyTo":"8328c28a-d93b-1076-20d3-823dbddf7e4c@alum.mit.edu","subject":"Re: [PATCH] files_initial_transaction_commit(): only unlock if locked","fromName":"Mathias Rav","fromEmail":"m@git.strova.dk","sentAt":"2018-01-22T10:03:57Z","receivedAt":"2018-01-22T10:04:06Z","isPatch":true,"sender":{"key":"m@git.strova.dk","avatar":"https://avatars.githubusercontent.com/u/373639?v=4"},"body":"2018-01-22 10:25 +0100 Michael Haggerty <mhagger@alum.mit.edu>:\n> On 01/19/2018 11:14 PM, Junio C Hamano wrote:\n> > Jeff King <peff@peff.net> writes:\n> >   \n> >> On Thu, Jan 18, 2018 at 02:38:41PM +0100, Mathias Rav wrote:\n> >>  \n> >>> Running git clone --single-branch --mirror -b TAGNAME previously\n> >>> triggered the following error message:\n> >>>\n> >>> \tfatal: multiple updates for ref 'refs/tags/TAGNAME' not allowed.\n> >>>\n> >>> This error condition is handled in files_initial_transaction_commit().\n> >>>\n> >>> 42c7f7ff9 (\"commit_packed_refs(): remove call to `packed_refs_unlock()`\", 2017-06-23)\n> >>> introduced incorrect unlocking in the error path of this function,\n> >>> which changes the error message to\n> >>>\n> >>> \tfatal: BUG: packed_refs_unlock() called when not locked\n> >>>\n> >>> Move the call to packed_refs_unlock() above the \"cleanup:\" label\n> >>> since the unlocking should only be done in the last error path.  \n> >>\n> >> Thanks, this solution looks correct to me. It's pretty low-impact since\n> >> the locking is the second-to-last thing in the function, so we don't\n> >> have to re-add the unlock to a bunch of error code paths. But one\n> >> alternative would be to just do:\n> >>\n> >>   if (packed_refs_is_locked(refs))\n> >> \tpacked_refs_unlock(refs->packed_ref_store);\n> >>\n> >> in the cleanup section.  \n> > \n> > Yeah, that may be a more future-proof alternative, and just as you\n> > said the patch as posted would be sufficient, too.  \n> \n> Either solution LGTM. Thanks for finding and fixing this bug.\n> \n> But let's also take a step back. The invocation\n> \n>     git clone --single-branch --mirror -b TAGNAME\n> \n> seems curious. Does it even make sense to use `--mirror` and\n> `--single-branch` at the same time? What should it do?\n> \n> Normally `--mirror` implies (aside from `--bare`) that the remote\n> references should be converted 1:1 to local references and should be\n> overwritten at every fetch; i.e., the refspec should be set to\n> `+refs/*:refs/*`.\n> \n> To me the most plausible interpretation of `--mirror --single-branch -b\n> BRANCHNAME` would be that the single branch should be fetched and made\n> the HEAD, and the refspec should be set to\n> `+refs/heads/BRANCHNAME:refs/heads/BRANCHNAME`. It also wouldn't be very\n> surprising if it were forbidden to use these options together.\n> \n> Currently, we do neither of those things. Instead we fetch that one\n> reference (as `refs/heads/BRANCHNAME`) but set the refspec to\n> `+refs/*:refs/*`; i.e., the next fetch would fetch all of the history.\n> \n> It's even more mind-bending if `-b` is passed a `TAGNAME` rather than a\n> `BRANCHNAME`. The documentation says that `-b TAGNAME` \"detaches the\n> HEAD at that commit in the resulting repository\". If `--single-branch -b\n> TAGNAME` is used, then the refspec is set to\n> `+refs/tags/TAGNAME:refs/tags/TAGNAME`. But what if `--mirror` is also used?\n> \n> Currently, this fails, apparently because `--mirror` and `-b TAGNAME`\n> each independently try to set `refs/tags/TAGNAME` (presumably to the\n> same value). *If* this is a useful use case, we could fix it so that it\n> doesn't fail. If not, maybe we should prohibit it explicitly and emit a\n> clearer error message.\n> \n> Mathias: if you encountered this problem in the real world, what were\n> you trying to accomplish? What behavior would you have expected?\n\nI wanted a shallow, single-commit clone of a single tag into a bare\nrepo. Concretely, I wanted to change the Arch Linux build script for\nlinux-tools to make a shallow clone of the Linux kernel rather than an\nordinary clone, for a tagname corresponding to a released kernel\nversion. (Of course, it would have been better to change the build\nscript to download a tarball instead of using git, but without\nknowledge of the build script it seemed easier for me to just change\nthe git invocation.)\n\nCurrently, Arch Linux's build script uses\n`git clone --mirror \"$url\" \"$dir\"` to clone a remote repo;\na StackExchange post [1] suggested changing this to\n`git clone --mirror --depth 1 \"$url\" \"$dir\"`. (The post also adds\n`--single-branch`, but this is implied by `--depth`.)\n\n[1] https://unix.stackexchange.com/a/203335/220010\n\nNaively I added `-b TAGNAME` to fetch just a single tag, but this\nresulted in the error.\n\nIf I remove `--mirror`, i.e. invoke `git clone --depth 1 -b TAGNAME`,\nthen the refspec is `+refs/tags/TAGNAME:refs/tags/TAGNAME` as might be\nexpected, but this results in a non-bare repo.\n\nIf instead I change `--mirror` to `--bare`, i.e. invoke\n`git clone --bare --depth 1 -b TAGNAME`, this results in a cloned\nrepository with no `remote.origin.fetch` refspec at all.\nI would expect the refspec in this case to be set to\n`+refs/tags/TAGNAME:refs/tags/TAGNAME` (just as with `--mirror`).\n\n/Mathias\n"}]}