{"thread":{"id":"47017","subject":"v2.15.0-rc2 ref deletion bug","startedAt":"2017-10-24T08:24:19Z","lastAt":"2017-10-24T19:04:19Z","messageCount":4,"participants":["Jeff King","Michael Haggerty"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"330902","messageId":"20171024082409.smwsd6pla64jjlua@sigill.intra.peff.net","threadId":"47017","inReplyTo":null,"subject":"v2.15.0-rc2 ref deletion bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-24T08:24:10Z","receivedAt":"2017-10-24T08:24:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I found a potentially serious bug in v2.15.0-rc2 (and earlier release\ncandidates, too) that we may want to deal with before the release.\n\nIf I do:\n\ngit init -q repo\ncd repo\nobj=$(git hash-object -w /dev/null)\ngit update-ref refs/tags/foo $obj\ngit update-ref --stdin <<-EOF\ndelete refs/tags/foo\nupdate refs/tags/foo/bar $obj\nEOF\ngit for-each-ref\n\nthen at the end we have no refs at all!\n\nI'd expect one of:\n\n  1. We delete \"foo\" before updating \"foo/bar\", and we end up with a\n     single ref.\n\n  2. We complain that we cannot update \"foo/bar\" because \"foo\" still\n     exists.\n\nI was hoping for (1). But in earlier releases we did (2). That makes\nsense because it's safer to do all updates in a transaction before doing\nany deletes (since if there's a simultaneous prune we'd rather see both\nrefs present for a moment rather than neither).\n\nBut the v2.15 behavior is just buggy, and may lead to data loss (we\nsilently lose the refs, and then a subsequent prune may lose the\nobjects). This bisects to Michael's dc39e09942 (files_ref_store: use a\ntransaction to update packed refs, 2017-09-08).\n\nCuriously, it doesn't happen if you reverse the order of the entries in\nthe transaction (which _shouldn't_ matter, since we try to process it\natomically, but obviously it just tickles this bug in a funny way).\n\nI haven't figured out if the deletion has to be a prefix of the update\nto trigger the bug, or if the problem is more widespread.\n\n-Peff\n"},{"id":"330906","messageId":"ea715dae-b410-d4fb-77c7-2e61b764a343@alum.mit.edu","threadId":"47017","inReplyTo":"20171024082409.smwsd6pla64jjlua@sigill.intra.peff.net","subject":"Re: v2.15.0-rc2 ref deletion bug","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-10-24T11:05:00Z","receivedAt":"2017-10-24T11:05:09Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/24/2017 10:24 AM, Jeff King wrote:\n> I found a potentially serious bug in v2.15.0-rc2 (and earlier release\n> candidates, too) that we may want to deal with before the release.\n> \n> If I do:\n> \n> git init -q repo\n> cd repo\n> obj=$(git hash-object -w /dev/null)\n> git update-ref refs/tags/foo $obj\n> git update-ref --stdin <<-EOF\n> delete refs/tags/foo\n> update refs/tags/foo/bar $obj\n> EOF\n> git for-each-ref\n> \n> then at the end we have no refs at all!\n\nThat's a serious bug. I'm looking into it right now.\n\n> I'd expect one of:\n> \n>   1. We delete \"foo\" before updating \"foo/bar\", and we end up with a\n>      single ref.\n\nI don't think that this is possible in the general case in a single\ntransaction. The problem is that the code would need to take locks\n\n    refs/tags/foo.lock\n    refs/tags/foo/bar.lock\n\nBut the latter couldn't coexist with the loose reference file\n\n    refs/tags/foo\n\n, which might already exist.\n\nIt is only imaginable to do this in a single transaction if you pack and\nprune `refs/tags/foo` first, to get the loose reference out of the way,\nbefore executing the transaction. Even then, you would have to beware of\na race where another process writes a loose version of `refs/tags/foo`\nbetween the time that `pack-refs` prunes it and the time that the\ntransaction obtains the lock again.\n\n> [...]\n\nMichael\n"},{"id":"330909","messageId":"44c489f0-4c72-46a1-61a0-48917d64958b@alum.mit.edu","threadId":"47017","inReplyTo":"ea715dae-b410-d4fb-77c7-2e61b764a343@alum.mit.edu","subject":"Re: v2.15.0-rc2 ref deletion bug","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-10-24T12:43:39Z","receivedAt":"2017-10-24T12:44:33Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/24/2017 01:05 PM, Michael Haggerty wrote:\n> On 10/24/2017 10:24 AM, Jeff King wrote:\n>> I found a potentially serious bug in v2.15.0-rc2 (and earlier release\n>> candidates, too) that we may want to deal with before the release.\n>>\n>> If I do:\n>> [...]\n>> then at the end we have no refs at all!\n> \n> That's a serious bug. I'm looking into it right now.\n\nThe fix is trivial (see below). But let me add some tests and make sure\nthat there are no similar breakages in the area, then submit a full patch.\n\nMichael\n\n----------------------------- refs/files-backend.c\n-----------------------------\nindex 29eb5e826f..fc3f2abcc6 100644\n@@ -2523,15 +2523,15 @@ static int files_transaction_prepare(struct\nref_store *ref_store,\n \t */\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n\n \t\tret = lock_ref_for_update(refs, update, transaction,\n \t\t\t\t\t  head_ref, &affected_refnames, err);\n \t\tif (ret)\n-\t\t\tbreak;\n+\t\t\tgoto cleanup;\n\n \t\tif (update->flags & REF_DELETING &&\n \t\t    !(update->flags & REF_LOG_ONLY) &&\n \t\t    !(update->flags & REF_ISPRUNING)) {\n \t\t\t/*\n \t\t\t * This reference has to be deleted from\n \t\t\t * packed-refs if it exists there.\n"},{"id":"330955","messageId":"20171024190411.dvv7zsq6ax4nndti@sigill.intra.peff.net","threadId":"47017","inReplyTo":"ea715dae-b410-d4fb-77c7-2e61b764a343@alum.mit.edu","subject":"Re: v2.15.0-rc2 ref deletion bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-24T19:04:11Z","receivedAt":"2017-10-24T19:04:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 24, 2017 at 01:05:00PM +0200, Michael Haggerty wrote:\n\n> > I'd expect one of:\n> > \n> >   1. We delete \"foo\" before updating \"foo/bar\", and we end up with a\n> >      single ref.\n> \n> I don't think that this is possible in the general case in a single\n> transaction. The problem is that the code would need to take locks\n> \n>     refs/tags/foo.lock\n>     refs/tags/foo/bar.lock\n> \n> But the latter couldn't coexist with the loose reference file\n> \n>     refs/tags/foo\n> \n> , which might already exist.\n\nYeah, you're right. I was thinking of the opposite case (where you\ncould create \"foo.lock\" even though \"foo/bar\" exists), but this one is\nimpossible with filesystem semantics.\n\n> It is only imaginable to do this in a single transaction if you pack and\n> prune `refs/tags/foo` first, to get the loose reference out of the way,\n> before executing the transaction. Even then, you would have to beware of\n> a race where another process writes a loose version of `refs/tags/foo`\n> between the time that `pack-refs` prunes it and the time that the\n> transaction obtains the lock again.\n\nYeah, it's probably better to avoid playing games here. Moving to a\nnon-filesystem storage backend would just make the problem go away.\n\n-Peff\n"}]}