{"thread":{"id":"56930","subject":"Bug report: Strange behavior with `git gc` and `reference-transaction` hook","startedAt":"2021-11-18T00:42:12Z","lastAt":"2021-12-07T08:24:45Z","messageCount":5,"participants":["Waleed Khan","Bryan Turner","Jeff King","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"441543","messageId":"CAKjfCeBcuYC3OXRVtxxDGWRGOxC38Fb7CNuSh_dMmxpGVip_9Q@mail.gmail.com","threadId":"56930","inReplyTo":null,"subject":"Bug report: Strange behavior with `git gc` and `reference-transaction` hook","fromName":"Waleed Khan","fromEmail":"me@waleedkhan.name","sentAt":"2021-11-18T00:41:34Z","receivedAt":"2021-11-18T00:42:12Z","isPatch":false,"sender":{"key":"me@waleedkhan.name","avatar":null},"body":"Hi all,\n\nI'm seeing unusual behavior on Git built from source at\ncd3e606211bb1cf8bc57f7d76bab98cc17a150bc, but which appears to extend\nback to Git v2.29.\n\nThis is a repro script:\n\n```\n#!/bin/sh\n\ntemp_dir=$(mktemp -d)\necho \"git dir is: $temp_dir\"\nmkdir -p \"$temp_dir\"\ntrap \"rm -rf $temp_dir\" EXIT\n\ncd \"$temp_dir\" || exit 1\ngit init -q\ndate='Thu, 07 Apr 2005 22:13:13 +0200'\nGIT_AUTHOR_DATE=\"$date\" GIT_COMMITTER_DATE=\"$date\" git commit -q\n--allow-empty -m 'Initial commit'\n\nmkdir -p '.git/hooks'\ncat >.git/hooks/reference-transaction <<'EOT'\n#!/bin/sh\n\n[[ \"$1\" != 'committed' ]] && exit 0\necho 'New reference-transaction invocation'\n\nwhile read old new ref; do\n  echo \"  old: $old, new: $new, ref: $ref\"\ndone\nEOT\nchmod +x .git/hooks/reference-transaction\n\ngit gc --prune=now -q\ngit show-ref refs/heads/master\n```\n\nAnd this is the output it produces:\n\n```\ngit dir is: /var/folders/gn/gdp9z_g968b9nx7c9lvgy8y00000gp/T/tmp.b3Jc6qnb\nNew reference-transaction invocation\n  old: 0000000000000000000000000000000000000000, new:\ne197d18c017d4038418be8b1cd38f4503e289165, ref: refs/heads/master\nNew reference-transaction invocation\n  old: e197d18c017d4038418be8b1cd38f4503e289165, new:\n0000000000000000000000000000000000000000, ref: refs/heads/master\ne197d18c017d4038418be8b1cd38f4503e289165 refs/heads/master\n```\n\nThese hooks are invoked a few milliseconds one after another.\n\nThe expected behavior would be that the latest reference transaction\nhook refers to the state of the references on disk. That is, either\n`master` should point to 0 (be deleted), or it should have said that\n`master` pointed to `e197d1`.\n\nBut if we actually examine `master`, it's set to `e197d1`, just as you\nwould expect. The GC should have been a no-op overall.\n\nBest,\nWaleed\n"},{"id":"441544","messageId":"CAGyf7-FoRyVtQHa2ETQtRA6fD7x0GDhKVPg+eAajhgPNrsw_OQ@mail.gmail.com","threadId":"56930","inReplyTo":"CAKjfCeBcuYC3OXRVtxxDGWRGOxC38Fb7CNuSh_dMmxpGVip_9Q@mail.gmail.com","subject":"Re: Bug report: Strange behavior with `git gc` and `reference-transaction` hook","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2021-11-18T00:52:46Z","receivedAt":"2021-11-18T00:52:59Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Wed, Nov 17, 2021 at 4:42 PM Waleed Khan <me@waleedkhan.name> wrote:\n>\n> Hi all,\n>\n> I'm seeing unusual behavior on Git built from source at\n> cd3e606211bb1cf8bc57f7d76bab98cc17a150bc, but which appears to extend\n> back to Git v2.29.\n>\n> This is a repro script:\n>\n> ```\n> #!/bin/sh\n>\n> temp_dir=$(mktemp -d)\n> echo \"git dir is: $temp_dir\"\n> mkdir -p \"$temp_dir\"\n> trap \"rm -rf $temp_dir\" EXIT\n>\n> cd \"$temp_dir\" || exit 1\n> git init -q\n> date='Thu, 07 Apr 2005 22:13:13 +0200'\n> GIT_AUTHOR_DATE=\"$date\" GIT_COMMITTER_DATE=\"$date\" git commit -q\n> --allow-empty -m 'Initial commit'\n>\n> mkdir -p '.git/hooks'\n> cat >.git/hooks/reference-transaction <<'EOT'\n> #!/bin/sh\n>\n> [[ \"$1\" != 'committed' ]] && exit 0\n> echo 'New reference-transaction invocation'\n>\n> while read old new ref; do\n>   echo \"  old: $old, new: $new, ref: $ref\"\n> done\n> EOT\n> chmod +x .git/hooks/reference-transaction\n>\n> git gc --prune=now -q\n> git show-ref refs/heads/master\n> ```\n>\n> And this is the output it produces:\n>\n> ```\n> git dir is: /var/folders/gn/gdp9z_g968b9nx7c9lvgy8y00000gp/T/tmp.b3Jc6qnb\n> New reference-transaction invocation\n>   old: 0000000000000000000000000000000000000000, new:\n> e197d18c017d4038418be8b1cd38f4503e289165, ref: refs/heads/master\n> New reference-transaction invocation\n>   old: e197d18c017d4038418be8b1cd38f4503e289165, new:\n> 0000000000000000000000000000000000000000, ref: refs/heads/master\n> e197d18c017d4038418be8b1cd38f4503e289165 refs/heads/master\n> ```\n>\n> These hooks are invoked a few milliseconds one after another.\n\nThere are two built-in \"ref backends\" that Git uses out of the box: A\npacked backend, which manages the \"packed-refs\" file, and a loose\nbackend, which manages other files under \"$GIT_DIR/refs\". What you're\nseeing is a reference transaction for the packed backend which is\nadding the new value for \"refs/heads/master\" to \"packed-refs\" (packed\nbackend reference-transactions rarely, if ever, include an actual old\nhash, as far as I can tell), and then a second reference transaction\nfor the loose backend to delete the loose \"refs/heads/master\" file,\nnow that it's packed.\n\n>\n> The expected behavior would be that the latest reference transaction\n> hook refers to the state of the references on disk. That is, either\n> `master` should point to 0 (be deleted), or it should have said that\n> `master` pointed to `e197d1`.\n>\n> But if we actually examine `master`, it's set to `e197d1`, just as you\n> would expect. The GC should have been a no-op overall.\n\nOne of the subtasks of \"git gc\" is \"git pack-refs\". If you inspect in\nmore detail, I suspect you'll find that \"refs/heads/master\" was loose\nbefore \"git gc\" ran (as in, there was a file\n\"$GIT_DIR/refs/heads/master\") and \"packed-refs\" either didn't have a\n\"refs/heads/master\" entry or had a different hash. (Loose refs always\n\"win\" over packed, since ref updates only write loose refs.)\n\nHope this helps,\nBryan Turner\n\n>\n> Best,\n> Waleed\n"},{"id":"441553","messageId":"CAGyf7-FgwMkjmyY1-ienUrPyzHAzcJFnZ+D7dcpUF=RGCZAFhw@mail.gmail.com","threadId":"56930","inReplyTo":"CAKjfCeD1C2DTnT0cLj4hC9Nq90TOFLiPnd_SLQi0HuMQHZcCaw@mail.gmail.com","subject":"Re: Bug report: Strange behavior with `git gc` and `reference-transaction` hook","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2021-11-18T01:28:06Z","receivedAt":"2021-11-18T01:28:19Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"(Please don't top-post on the list)\n\nOn Wed, Nov 17, 2021 at 5:01 PM Waleed Khan <me@waleedkhan.name> wrote:\n>\n> So what should users of the `reference-transaction` hook do in general? It sounds like we can never actually rely on the new value corresponding to the state of the reference on disk. Is that correct?\n\nI wish I had a good answer here, but unfortunately I don't. My\ncomments are coming from a similar place: The end result of my own\nexperiences and learnings trying to use \"reference-transaction\" for\nsomething. The dual backends, and the fact that\n\"reference-transaction\" receives _no hints_, as far as I'm aware, of\nwhat backend a callback is for, makes some use cases quite difficult\nin practice. It would be nice if there was an environment variable, or\nsomething, to indicate what backend a transaction was for.\n\nBut for cases like this, where a ref is being \"moved\" between\nbackends, the split nature of the backends, and the fact that each has\nits own separate transaction, makes working in a \"simple\" script,\nwhich has no more state than its current invocation, extremely\ndifficult. Of course, since Git doesn't really have an ACID mechanism\nfor atomically updating both \"packed-refs\" and loose ref files, trying\nto have a single reference transaction wouldn't really work either.\n\n>\n> I suppose if we want the real new value of the reference, we should always look it up freshly? If so, the githooks documentation should be updated. Currently, it has a remark on the validity when the old reference is zero, but not on the validity of the new reference.\n\nI'm not sure the \"validity\" of the new reference is the problem here.\nBoth reference transactions _are_ giving you the correct new value;\n\"packed-refs\" is inserting \"refs/heads/master\" at e197d18, and the\nloose ref for it is being deleted. To me the difficulty isn't that the\nnew value is _wrong_ (it's actually right), it's that, because there\nare two reference transactions and no shared context between them or\nhints as to what backend they're for, it's not really possible to put\n2+2 together and realize that 4 is a ref being moved from loose to\npacked--but otherwise unchanged.\n\nBest regards,\nBryan Turner\n"},{"id":"441649","messageId":"YZaWqTwPOyQz0/mu@coredump.intra.peff.net","threadId":"56930","inReplyTo":"CAGyf7-FoRyVtQHa2ETQtRA6fD7x0GDhKVPg+eAajhgPNrsw_OQ@mail.gmail.com","subject":"Re: Bug report: Strange behavior with `git gc` and `reference-transaction` hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-18T18:08:41Z","receivedAt":"2021-11-18T18:08:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc pks]\n\nOn Wed, Nov 17, 2021 at 04:52:46PM -0800, Bryan Turner wrote:\n\n> > The expected behavior would be that the latest reference transaction\n> > hook refers to the state of the references on disk. That is, either\n> > `master` should point to 0 (be deleted), or it should have said that\n> > `master` pointed to `e197d1`.\n> >\n> > But if we actually examine `master`, it's set to `e197d1`, just as you\n> > would expect. The GC should have been a no-op overall.\n> \n> One of the subtasks of \"git gc\" is \"git pack-refs\". If you inspect in\n> more detail, I suspect you'll find that \"refs/heads/master\" was loose\n> before \"git gc\" ran (as in, there was a file\n> \"$GIT_DIR/refs/heads/master\") and \"packed-refs\" either didn't have a\n> \"refs/heads/master\" entry or had a different hash. (Loose refs always\n> \"win\" over packed, since ref updates only write loose refs.)\n\nIt seems totally broken to me that we'd trigger the\nreference-transaction hook for ref packing. The point of the hook is to\ntrack logical updates to the refs. But during ref packing that does not\nchange at all; the value remains the same. So I don't think we should be\ntriggering the hook at all, let alone with confusing values.\n\nThis snippet shows a simple case that I think is wrong:\n\n-- >8 --\ngit init -q repo\ncd repo\n\ncat >.git/hooks/reference-transaction <<\\EOF\n#!/bin/sh\necho >&2 \"==> reference-transaction $*\"\nsed 's/^/  /'\nEOF\nchmod +x .git/hooks/reference-transaction\n\necho >&2 \"running commit...\"\ngit commit --allow-empty -qm foo\necho >&2 \"running pack-refs...\"\ngit pack-refs --all --prune\n-- >8 --\n\nIt produces:\n\n  running commit...\n  ==> reference-transaction prepared\n    0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b HEAD\n    0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b refs/heads/main\n  ==> reference-transaction committed\n    0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b HEAD\n    0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b refs/heads/main\n  running pack-refs...\n  ==> reference-transaction prepared\n    0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b refs/heads/main\n  ==> reference-transaction committed\n    0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b refs/heads/main\n  ==> reference-transaction prepared\n    77bcab0d950aee3021e8aa13a15d40e7a9a5f71b 0000000000000000000000000000000000000000 refs/heads/main\n  ==> reference-transaction committed\n    77bcab0d950aee3021e8aa13a15d40e7a9a5f71b 0000000000000000000000000000000000000000 refs/heads/main\n\nI think the final four invocations should be skipped entirely. They're\npointless at best (nothing actually changed), and extremely misleading\nat worst (they look like the ref ended up deleted!).\n\n-Peff\n"},{"id":"443243","messageId":"Ya8aIAAOlValUL2o@ncase","threadId":"56930","inReplyTo":"YZaWqTwPOyQz0/mu@coredump.intra.peff.net","subject":"Re: Bug report: Strange behavior with `git gc` and `reference-transaction` hook","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-12-07T08:24:00Z","receivedAt":"2021-12-07T08:24:45Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Nov 18, 2021 at 01:08:41PM -0500, Jeff King wrote:\n> [+cc pks]\n> \n> On Wed, Nov 17, 2021 at 04:52:46PM -0800, Bryan Turner wrote:\n> \n> > > The expected behavior would be that the latest reference transaction\n> > > hook refers to the state of the references on disk. That is, either\n> > > `master` should point to 0 (be deleted), or it should have said that\n> > > `master` pointed to `e197d1`.\n> > >\n> > > But if we actually examine `master`, it's set to `e197d1`, just as you\n> > > would expect. The GC should have been a no-op overall.\n> > \n> > One of the subtasks of \"git gc\" is \"git pack-refs\". If you inspect in\n> > more detail, I suspect you'll find that \"refs/heads/master\" was loose\n> > before \"git gc\" ran (as in, there was a file\n> > \"$GIT_DIR/refs/heads/master\") and \"packed-refs\" either didn't have a\n> > \"refs/heads/master\" entry or had a different hash. (Loose refs always\n> > \"win\" over packed, since ref updates only write loose refs.)\n> \n> It seems totally broken to me that we'd trigger the\n> reference-transaction hook for ref packing. The point of the hook is to\n> track logical updates to the refs. But during ref packing that does not\n> change at all; the value remains the same. So I don't think we should be\n> triggering the hook at all, let alone with confusing values.\n> \n> This snippet shows a simple case that I think is wrong:\n> \n> -- >8 --\n> git init -q repo\n> cd repo\n> \n> cat >.git/hooks/reference-transaction <<\\EOF\n> #!/bin/sh\n> echo >&2 \"==> reference-transaction $*\"\n> sed 's/^/  /'\n> EOF\n> chmod +x .git/hooks/reference-transaction\n> \n> echo >&2 \"running commit...\"\n> git commit --allow-empty -qm foo\n> echo >&2 \"running pack-refs...\"\n> git pack-refs --all --prune\n> -- >8 --\n> \n> It produces:\n> \n>   running commit...\n>   ==> reference-transaction prepared\n>     0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b HEAD\n>     0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b refs/heads/main\n>   ==> reference-transaction committed\n>     0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b HEAD\n>     0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b refs/heads/main\n>   running pack-refs...\n>   ==> reference-transaction prepared\n>     0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b refs/heads/main\n>   ==> reference-transaction committed\n>     0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b refs/heads/main\n>   ==> reference-transaction prepared\n>     77bcab0d950aee3021e8aa13a15d40e7a9a5f71b 0000000000000000000000000000000000000000 refs/heads/main\n>   ==> reference-transaction committed\n>     77bcab0d950aee3021e8aa13a15d40e7a9a5f71b 0000000000000000000000000000000000000000 refs/heads/main\n> \n> I think the final four invocations should be skipped entirely. They're\n> pointless at best (nothing actually changed), and extremely misleading\n> at worst (they look like the ref ended up deleted!).\n> \n> -Peff\n\nYeah, I agree that this is something that is totally misleading.\n\nFor what it's worth, we also hit a similar case in production at GitLab,\nwhere we use the hook to do voting on ref updates across different\nnodes. Sometimes we observed different votes on a subset of nodes, and\nit took me quite some time to figure out that this was dependent on\nwhether a ref was packed or not. We're now filtering out transactions\nwhich consist only of force-deletions [1], which are _likely_ to be\ncleanups of such packed refs. But this is very clearly a hack, and I\nagree that calling the hook for \n\nIn the end, these really are special cases of how the \"files\" backend\nworks and thus are implementation details which shouldn't be exposed to\nthe user at all. With these implementation details exposed, we'll start\nto see different behaviour of when the hook is executed depending on\nwhich ref backend you use, which is even worse compared to the current\nstate where it's at least consistently misleading.\n\nAs Peff said, the hook should really only track logical changes and not\nexpose any implementation details.\n\nPatrick\n\n[1]: https://gitlab.com/gitlab-org/gitaly/-/blob/3ef55853e9e161204464868390d97d1a1577042d/internal/gitaly/hook/referencetransaction.go#L58\n"}]}