{"thread":{"id":"59028","subject":"[BUG] `git gc` or `git pack-refs` wipes all notes for `git notes` command","startedAt":"2023-01-03T03:22:42Z","lastAt":"2023-01-06T13:05:50Z","messageCount":6,"participants":["Andrew Hlynskyi","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"469757","messageId":"CAAYtLELp4v=id-UUdGT+BoCxLuTV05Z0fFMQmPfd3Mt-yXJ9Tw@mail.gmail.com","threadId":"59028","inReplyTo":null,"subject":"[BUG] `git gc` or `git pack-refs` wipes all notes for `git notes` command","fromName":"Andrew Hlynskyi","fromEmail":"ahlincq@gmail.com","sentAt":"2023-01-03T03:22:01Z","receivedAt":"2023-01-03T03:22:42Z","isPatch":false,"sender":{"key":"ahlincq@gmail.com","avatar":null},"body":"Steps to reproduce:\n1. Create some notes with the `git notes add` command.\n2. Run `git notes` to list stored notes.\n3. Run any of `git gc  --prune=now` or `git pack-refs --all` commands.\n4. Run `git notes` again and see that there are no more seen notes.\n\nThe reason is that all commands like `git notes` or `git log --notes`\nexpect to find notes head as unpacked ref in .git/refs/notes/commits.\nBut the gc or the pack-refs command packs .git/refs/notes/commits ref\ninto .git/packed-refs file.\n\nIt's possible to restore the notes ref with a shell script like:\n```\nfix-notes-unpack-commits-ref(){\n    local hash path\n    grep refs/notes/commits .git/packed-refs |\\\n    while read hash path; do\n        path=\".git/$path\"\n        mkdir -p \"$(dirname \"$path\")\"\n        echo -n \"$hash\" > \"$path\"\n    done\n}\n```\nOr, fortunately, from another backup place: .git/logs/refs/notes/commits.\n\nThe expected behavior is that `git notes` and `git log` should work\nalso with notes ref stored in the .git/packed-refs file and also\nrespect that the `git notes` command can store notes with a custom ref\nby using the `--ref` option.\n\nYours sincerely,\nAndrew\n"},{"id":"469760","messageId":"Y7Pvqk00sj3R7cZv@coredump.intra.peff.net","threadId":"59028","inReplyTo":"CAAYtLELp4v=id-UUdGT+BoCxLuTV05Z0fFMQmPfd3Mt-yXJ9Tw@mail.gmail.com","subject":"Re: [BUG] `git gc` or `git pack-refs` wipes all notes for `git notes` command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-03T09:04:42Z","receivedAt":"2023-01-03T09:04:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 03, 2023 at 05:22:01AM +0200, Andrew Hlynskyi wrote:\n\n> Steps to reproduce:\n> 1. Create some notes with the `git notes add` command.\n> 2. Run `git notes` to list stored notes.\n> 3. Run any of `git gc  --prune=now` or `git pack-refs --all` commands.\n> 4. Run `git notes` again and see that there are no more seen notes.\n\nThis doesn't reproduce for me at all. Running:\n\n  git init repo\n  cd repo\n\n  git commit --allow-empty -m \"this is the commit message\"\n  git notes add -m \"this is the note\"\n\n  git notes\n  git pack-refs --all\n  git notes\n\nShows the same output before and after packing the refs.\n\n> The reason is that all commands like `git notes` or `git log --notes`\n> expect to find notes head as unpacked ref in .git/refs/notes/commits.\n> But the gc or the pack-refs command packs .git/refs/notes/commits ref\n> into .git/packed-refs file.\n\nThat would be very surprising. The lookup of the ref in the notes code\nuses the same generic ref code that the rest of Git uses, which\nunderstands packed-refs and so on.\n\nCan you share the .git directory of a repository that exhibits this\nbehavior? It's possible there's a bug or something in the packed-refs\ncode, though I find it pretty unlikely, as it's fairly well exercised in\nnormal use.\n\n-Peff\n"},{"id":"469810","messageId":"CAAYtLE+PWK_v0cc8uqaiKnTHKghrkxuCCgfWyo9bhD23+vxK1g@mail.gmail.com","threadId":"59028","inReplyTo":"Y7Pvqk00sj3R7cZv@coredump.intra.peff.net","subject":"Re: [BUG] `git gc` or `git pack-refs` wipes all notes for `git notes` command","fromName":"Andrew Hlynskyi","fromEmail":"ahlincq@gmail.com","sentAt":"2023-01-05T16:59:36Z","receivedAt":"2023-01-05T17:01:17Z","isPatch":false,"sender":{"key":"ahlincq@gmail.com","avatar":null},"body":"On Tue, Jan 3, 2023 at 11:04 AM Jeff King <peff@peff.net> wrote:\n\n> That would be very surprising. The lookup of the ref in the notes code\n> uses the same generic ref code that the rest of Git uses, which\n> understands packed-refs and so on.\n\nThank you Jeff for looking at this also.\n\nI tried to debug the issue by myself and found that the\n`find_reference_location` function can't find the record for\n`refs/notes/commits` in a repo.\nI recall that a year ago I had trouble with a repo and it was damaged,\nI restored it with the `git fsck` command and took some info from the\n`git reflog`.\nBut somehow there was an issue in the `.git/packed-refs` file that was\nthere unexplored all past year.\nThe core of the issue I can demonstrate with two `.git/packed-refs` excerpts.\n\nThe issue excerpt:\n```\ndca1abdb5b870d46deae3717e87d0085b62d7242 refs/heads/tmp\nd5ec01136b79a59a9e39d3ab02d92e1d91558579 refs/heads/tdf\nc60f96a3f76e99e1b42f07204ad830c1483647b0 refs/heads/trunk\n93119b80d4c7676a0ebe1bd6ae46574f96dfc7ed refs/notes/commits\n452a23af3cfac6eef3bb1045894521fac8e51e2a refs/heads/cargo-config-toml\nf5599d73873c451041fde857a09b5f12662357c1 refs/heads/custom\n64076392359c4d4989320bde5712a8b389056f49 refs/heads/main\n2119ff71ca66bec5117534907a127cee32680788 refs/heads/next\n7e0016059ee7a87e3d04f7e59154529a9616a238 refs/recovery/1\nde2876aa45022af815e717eb92becd61564efe32 refs/recovery/10\n```\n\nThe correct excerpt:\n```\ndca1abdb5b870d46deae3717e87d0085b62d7242 refs/heads/tmp\nd5ec01136b79a59a9e39d3ab02d92e1d91558579 refs/heads/tdf\nc60f96a3f76e99e1b42f07204ad830c1483647b0 refs/heads/trunk\n452a23af3cfac6eef3bb1045894521fac8e51e2a refs/heads/cargo-config-toml\nf5599d73873c451041fde857a09b5f12662357c1 refs/heads/custom\n64076392359c4d4989320bde5712a8b389056f49 refs/heads/main\n2119ff71ca66bec5117534907a127cee32680788 refs/heads/next\n93119b80d4c7676a0ebe1bd6ae46574f96dfc7ed refs/notes/commits\n7e0016059ee7a87e3d04f7e59154529a9616a238 refs/recovery/1\nde2876aa45022af815e717eb92becd61564efe32 refs/recovery/10\n```\n\nAs you may notice that on the first except there is an issue with\nsorting and after `ref/notes/*` there are several `ref/heads/*`.\nSo I suspect that the issue was introduced by myself a year ago when I\nwas recovering the repo.\n\n> Can you share the .git directory of a repository that exhibits this\n> behavior? It's possible there's a bug or something in the packed-refs\n> code, though I find it pretty unlikely, as it's fairly well exercised in\n> normal use.\n\nThe above excerpts completely describe the issue and there is no more\nspecial in the repo.\n\nI was surprised that such an issue can remain in the\n`.git/packed-refs` for more than a year.\nI thought that commands like `git fsck` would report such unordering\nproblems and `git gc` or `git pack-refs`\nmake ordering checks and make full resorting of the `.git/packed-refs`\nfile in case of issues with ordering.\nI understand that the `.git/packed-refs` file is for machines and not\nfor humans but sometimes\nit's the fastest way to make several simple corrections in it manually.\n\nP.S: What was most surprising is that `git pack-refs` was able to\ncorrectly update a hash even for an unordered record.\n\n-Andrew\n"},{"id":"469818","messageId":"Y7fikyZV1ky2modr@coredump.intra.peff.net","threadId":"59028","inReplyTo":"CAAYtLE+PWK_v0cc8uqaiKnTHKghrkxuCCgfWyo9bhD23+vxK1g@mail.gmail.com","subject":"Re: [BUG] `git gc` or `git pack-refs` wipes all notes for `git notes` command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-06T08:57:55Z","receivedAt":"2023-01-06T08:59:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 05, 2023 at 06:59:36PM +0200, Andrew Hlynskyi wrote:\n\n> > Can you share the .git directory of a repository that exhibits this\n> > behavior? It's possible there's a bug or something in the packed-refs\n> > code, though I find it pretty unlikely, as it's fairly well exercised in\n> > normal use.\n> \n> The above excerpts completely describe the issue and there is no more\n> special in the repo.\n\nThanks for digging. Your explanation makes sense.\n\n> I was surprised that such an issue can remain in the\n> `.git/packed-refs` for more than a year.\n> I thought that commands like `git fsck` would report such unordering\n> problems and `git gc` or `git pack-refs`\n> make ordering checks and make full resorting of the `.git/packed-refs`\n> file in case of issues with ordering.\n\nI don't think we have any fsck checks that the packed-refs file is in\nsorted order. It might be reasonable to have them. Likewise, when\npack-refs rewrites the file, it should be able to cheaply double-check\nthat the input is sorted by comparing each entry against its previous.\n\nI'm not _too_ surprised that the repo could persist for a while with an\nout-of-order packed-refs file, especially if you are not doing writes\nthat would trigger a gc. Lookups are done by binary-search (and thus\nrequire sorting), but many operations will just iterate over the whole\nlist and don't care.  For an infrequently used ref, you might get\nunlucky and fall on it during a binary search, but otherwise it wouldn't\naffect most results.\n\n> I understand that the `.git/packed-refs` file is for machines and not\n> for humans but sometimes\n> it's the fastest way to make several simple corrections in it manually.\n\nYes, I have certainly done this myself. There are is a header line at\nthe top of the file, though, which tells what is guaranteed:\n\n  $ head -1 .git/packed-refs\n  # pack-refs with: peeled fully-peeled sorted\n\nif you are going to muck with the file, deleting that line should be\nsufficient; the reading code will fall back to the older routines which\ndon't assume it's sorted.\n\n-Peff\n"},{"id":"469828","messageId":"xmqqilhjsuo9.fsf@gitster.g","threadId":"59028","inReplyTo":"Y7fikyZV1ky2modr@coredump.intra.peff.net","subject":"Re: [BUG] `git gc` or `git pack-refs` wipes all notes for `git notes` command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-06T12:40:22Z","receivedAt":"2023-01-06T12:40:29Z","isPatch":false,"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 05, 2023 at 06:59:36PM +0200, Andrew Hlynskyi wrote:\n>\n>> > Can you share the .git directory of a repository that exhibits this\n>> > behavior? It's possible there's a bug or something in the packed-refs\n>> > code, though I find it pretty unlikely, as it's fairly well exercised in\n>> > normal use.\n>> \n>> The above excerpts completely describe the issue and there is no more\n>> special in the repo.\n>\n> Thanks for digging. Your explanation makes sense.\n> ...\n> I don't think we have any fsck checks that the packed-refs file is in\n> sorted order. It might be reasonable to have them. Likewise, when\n> pack-refs rewrites the file, it should be able to cheaply double-check\n> that the input is sorted by comparing each entry against its previous.\n\nTrue.  I would not mind a patch to make us do so in the code path\nwhere we rewrite the file and add \"sorted\" trait to the file.\nrefs/packed-backend.c::sort_snapshot() seems to be already equipped\nto do this?\n\n>> I understand that the `.git/packed-refs` file is for machines and not\n>> for humans but sometimes\n>> it's the fastest way to make several simple corrections in it manually.\n>\n> Yes, I have certainly done this myself. There are is a header line at\n> the top of the file, though, which tells what is guaranteed:\n>\n>   $ head -1 .git/packed-refs\n>   # pack-refs with: peeled fully-peeled sorted\n>\n> if you are going to muck with the file, deleting that line should be\n> sufficient; the reading code will fall back to the older routines which\n> don't assume it's sorted.\n\nThanks, both.\n\nSo we can conclude that this discussion thread has an\nincorrect Subject: and the symptom was caused by human error?\n\n"},{"id":"469830","messageId":"Y7gcczFWyTePVjlk@coredump.intra.peff.net","threadId":"59028","inReplyTo":"xmqqilhjsuo9.fsf@gitster.g","subject":"Re: [BUG] `git gc` or `git pack-refs` wipes all notes for `git notes` command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-06T13:04:51Z","receivedAt":"2023-01-06T13:05:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 06, 2023 at 09:40:22PM +0900, Junio C Hamano wrote:\n\n> > I don't think we have any fsck checks that the packed-refs file is in\n> > sorted order. It might be reasonable to have them. Likewise, when\n> > pack-refs rewrites the file, it should be able to cheaply double-check\n> > that the input is sorted by comparing each entry against its previous.\n> \n> True.  I would not mind a patch to make us do so in the code path\n> where we rewrite the file and add \"sorted\" trait to the file.\n> refs/packed-backend.c::sort_snapshot() seems to be already equipped\n> to do this?\n\nI think it may be a little trickier than that. Yes, sort_snapshot()\nknows about sorting, but it only kicks in when the file isn't already\nmarked as sorted (and we sort on the fly so that the rest of the code\ncan use the same lookup routines).\n\nAnd it's possible that we can just sort_snapshot() before writing, even\nif the original claims to be sorted. But I'm not sure what performance\nimpact that might have on the normal case that everything is already in\ngood order. Maybe it's not a big deal; the write is already O(n), so\nadding an O(n log n) might not be the end of the world.\n\nBut I was thinking more that write_with_updates() would, while iterating\nthrough the existing entries, check that the values it gets from the\nref_iterator are indeed in sorted order. And if not, I think it needs to\nactually bail, since we might already have written a partially-confused\nresult. And there \"bail\" may mean \"write a warning to the user, abort\nthe current write, call sort_snapshot(), and then try again\".\n\nAll of which is to say I don't think it's conceptually _too_ hard, but\nit was not simple enough that I was comfortable dashing off a one-liner\nand saying \"probably something like this\". ;)\n\n> So we can conclude that this discussion thread has an\n> incorrect Subject: and the symptom was caused by human error?\n\nThat's my read on it.\n\n-Peff\n"}]}