{"thread":{"id":"55344","subject":"Bug: reference-transaction hook for branch deletions broken between Git v2.30 and Git v2.31","startedAt":"2021-03-18T03:44:28Z","lastAt":"2021-03-18T07:34:05Z","messageCount":3,"participants":["Waleed Khan","Jeff King","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"419612","messageId":"CAKjfCeDdSRk5QwyYduvbQvz0zC9FCZ3+5bseeOmZBOULxZ0D7w@mail.gmail.com","threadId":"55344","inReplyTo":null,"subject":"Bug: reference-transaction hook for branch deletions broken between Git v2.30 and Git v2.31","fromName":"Waleed Khan","fromEmail":"me@waleedkhan.name","sentAt":"2021-03-18T03:42:10Z","receivedAt":"2021-03-18T03:44:28Z","isPatch":false,"sender":{"key":"me@waleedkhan.name","avatar":null},"body":"Hello,\n\nThe `reference-transaction` hook seems to have broken between Git\nv2.30 and v2.31, or at least violated my expectations as a user.\n\nI didn't see any mention of the `reference-transaction` hook in the\nrelease notes, so I assume that this is a bug. Given that there's\ndocumentation at `man githooks` for the `reference-transaction` hook,\nI assume that the feature is no longer in a preliminary stage, and so\na bug report is warranted. I couldn't find any mention of a\n`reference-transaction` hook bug already having been reported in the\nmailing list search online.\n\n## Reproduction ##\n\nTo reproduce, run this script:\n\n```\n#!/bin/sh\nset -eu\n\n# Change between Git v2.30.0 and v2.31.0 here.\nGIT=${GIT:-$(which git)}\necho \"Running version $(\"$GIT\" version)\"\n\n# For determinism.\nexport GIT_COMMITTER_DATE=\"Wed Mar 17 16:53:32 PDT 2021\"\nexport GIT_AUTHOR_DATE=\"Wed Mar 17 16:53:32 PDT 2021\"\n\nrm -rf repo\nmkdir repo\ncd repo\n\"$GIT\" init\n\"$GIT\" commit --allow-empty -m 'Initial commit'\n\nmkdir .git/hooks\ncat >.git/hooks/reference-transaction <<'EOF'\n#!/bin/sh\necho \"reference-transaction ($1):\"\ncat\nEOF\nchmod +x .git/hooks/reference-transaction\n\n\"$GIT\" branch -v 'test-branch'\necho \"Created test-branch\"\n\"$GIT\" branch -v -d 'test-branch'\n```\n\n## Expected behavior (git v2.30) ##\n\nThis is the output:\n\n```\n[master (root-commit) 3b61ecc] Initial commit\nreference-transaction (prepared):\n0000000000000000000000000000000000000000\n3b61ecc56006fcc283d42b302191e1385f19b551 refs/heads/test-branch\nreference-transaction (committed):\n0000000000000000000000000000000000000000\n3b61ecc56006fcc283d42b302191e1385f19b551 refs/heads/test-branch\nCreated test-branch\nreference-transaction (aborted):\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/test-branch\nreference-transaction (prepared):\n3b61ecc56006fcc283d42b302191e1385f19b551\n0000000000000000000000000000000000000000 refs/heads/test-branch\nreference-transaction (committed):\n3b61ecc56006fcc283d42b302191e1385f19b551\n0000000000000000000000000000000000000000 refs/heads/test-branch\nDeleted branch test-branch (was 3b61ecc).\n```\n\nIt's pretty strange that there was an \"aborted\" reference-transaction\nfrom 0 to 0, especially with no previous \"prepared\"\nreference-transaction, but that's not the bug in question, and I can\nwork around it by ignoring such transactions on my end.\n\nNotice that as part of the branch deletion, there is a\nreference-transaction from a non-zero commit hash to a zero commit\nhash.\n\n## Actual behavior (git v2.31) ##\n\n```\n[master (root-commit) 3b61ecc] Initial commit\nreference-transaction (prepared):\n0000000000000000000000000000000000000000\n3b61ecc56006fcc283d42b302191e1385f19b551 refs/heads/test-branch\nreference-transaction (committed):\n0000000000000000000000000000000000000000\n3b61ecc56006fcc283d42b302191e1385f19b551 refs/heads/test-branch\nCreated test-branch\nreference-transaction (prepared):\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/test-branch\nreference-transaction (committed):\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/test-branch\nreference-transaction (aborted):\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/test-branch\nreference-transaction (prepared):\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/test-branch\nreference-transaction (committed):\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/test-branch\nDeleted branch test-branch (was 3b61ecc).\n```\n\nMy issues are 1) the reference-transaction deleting the branch goes\nfrom a zero commit hash, instead of from the non-zero commit hash\n3b61ec, and 2) there's two such \"committed\" transactions for some\nreason. Like the other example, there's also a mysterious unpaired\naborted transaction, but I assume that's not new behavior in this\nrelease.\n\n## `git bugreport` system info ##\n\n`git` 2.30 bugreport (built from source):\n\n[System Info]\ngit version:\ngit version 2.30.2\ncpu: x86_64\nbuilt from commit: 94f6e3e283f2adfc518b39cfc39291f1c2832ad0\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Darwin 19.6.0 Darwin Kernel Version 19.6.0: Thu Oct 29 22:56:45\nPDT 2020; root:xnu-6153.141.2.2~1/RELEASE_X86_64 x86_64\ncompiler info: clang: 12.0.0 (clang-1200.0.32.21)\nlibc info: no libc information available\n$SHELL (typically, interactive shell): /bin/zsh\n\n`git` 2.31 bugreport (built from source):\n\n[System Info]\ngit version:\ngit version 2.31.0\ncpu: x86_64\nbuilt from commit: a5828ae6b52137b913b978e16cd2334482eb4c1f\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Darwin 19.6.0 Darwin Kernel Version 19.6.0: Thu Oct 29 22:56:45\nPDT 2020; root:xnu-6153.141.2.2~1/RELEASE_X86_64 x86_64\ncompiler info: clang: 12.0.0 (clang-1200.0.32.21)\nlibc info: no libc information available\n$SHELL (typically, interactive shell): /bin/zsh\n\nThe issue also reproduces in CI builds of Git on Linux.\n"},{"id":"419613","messageId":"YFLSPVr8k66c4CXs@coredump.intra.peff.net","threadId":"55344","inReplyTo":"CAKjfCeDdSRk5QwyYduvbQvz0zC9FCZ3+5bseeOmZBOULxZ0D7w@mail.gmail.com","subject":"Re: Bug: reference-transaction hook for branch deletions broken between Git v2.30 and Git v2.31","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-18T04:08:29Z","receivedAt":"2021-03-18T04:09:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 17, 2021 at 08:42:10PM -0700, Waleed Khan wrote:\n\n> The `reference-transaction` hook seems to have broken between Git\n> v2.30 and v2.31, or at least violated my expectations as a user.\n> \n> I didn't see any mention of the `reference-transaction` hook in the\n> release notes, so I assume that this is a bug. Given that there's\n> documentation at `man githooks` for the `reference-transaction` hook,\n> I assume that the feature is no longer in a preliminary stage, and so\n> a bug report is warranted. I couldn't find any mention of a\n> `reference-transaction` hook bug already having been reported in the\n> mailing list search online.\n\nThanks for a clear explanation and reproduction recipe. I can see the\nbug easily here. It bisects to 8198907795 (use delete_refs when deleting\ntags or branches, 2021-01-20). I've cc'd the author of that commit, as\nwell as the author of the ref-transaction hook (I've also retained the\nreproduction info for them at the end of the message).\n\nI _think_ this is probably another variant of what we were discussing\nin:\n\n  https://lore.kernel.org/git/d255c7a5f95635c2e7ae36b9689c3efd07b4df5d.1604501894.git.ps@pks.im/\n\nand that ended up with the documentation from:\n\n  https://lore.kernel.org/git/55905b8693dd49637d0516ee123405cbfb58b6c6.1614591751.git.ps@pks.im/\n\nNamely that the hook is not seeing the \"old\" ref value as it was on\ndisk, but rather they see the value that the caller asked us to verify\nexisted (or all zeroes if the caller didn't specify). So I think what\nhappened is that git-branch went from calling \"delete the ref X with\nvalue 1234abcd\" to \"delete the ref X, no matter what it's value is\".\n\n-Peff\n\n-- >8 --\n> To reproduce, run this script:\n> \n> ```\n> #!/bin/sh\n> set -eu\n> \n> # Change between Git v2.30.0 and v2.31.0 here.\n> GIT=${GIT:-$(which git)}\n> echo \"Running version $(\"$GIT\" version)\"\n> \n> # For determinism.\n> export GIT_COMMITTER_DATE=\"Wed Mar 17 16:53:32 PDT 2021\"\n> export GIT_AUTHOR_DATE=\"Wed Mar 17 16:53:32 PDT 2021\"\n> \n> rm -rf repo\n> mkdir repo\n> cd repo\n> \"$GIT\" init\n> \"$GIT\" commit --allow-empty -m 'Initial commit'\n> \n> mkdir .git/hooks\n> cat >.git/hooks/reference-transaction <<'EOF'\n> #!/bin/sh\n> echo \"reference-transaction ($1):\"\n> cat\n> EOF\n> chmod +x .git/hooks/reference-transaction\n> \n> \"$GIT\" branch -v 'test-branch'\n> echo \"Created test-branch\"\n> \"$GIT\" branch -v -d 'test-branch'\n> ```\n> \n> ## Expected behavior (git v2.30) ##\n> \n> This is the output:\n> \n> ```\n> [master (root-commit) 3b61ecc] Initial commit\n> reference-transaction (prepared):\n> 0000000000000000000000000000000000000000\n> 3b61ecc56006fcc283d42b302191e1385f19b551 refs/heads/test-branch\n> reference-transaction (committed):\n> 0000000000000000000000000000000000000000\n> 3b61ecc56006fcc283d42b302191e1385f19b551 refs/heads/test-branch\n> Created test-branch\n> reference-transaction (aborted):\n> 0000000000000000000000000000000000000000\n> 0000000000000000000000000000000000000000 refs/heads/test-branch\n> reference-transaction (prepared):\n> 3b61ecc56006fcc283d42b302191e1385f19b551\n> 0000000000000000000000000000000000000000 refs/heads/test-branch\n> reference-transaction (committed):\n> 3b61ecc56006fcc283d42b302191e1385f19b551\n> 0000000000000000000000000000000000000000 refs/heads/test-branch\n> Deleted branch test-branch (was 3b61ecc).\n> ```\n> \n> It's pretty strange that there was an \"aborted\" reference-transaction\n> from 0 to 0, especially with no previous \"prepared\"\n> reference-transaction, but that's not the bug in question, and I can\n> work around it by ignoring such transactions on my end.\n> \n> Notice that as part of the branch deletion, there is a\n> reference-transaction from a non-zero commit hash to a zero commit\n> hash.\n> \n> ## Actual behavior (git v2.31) ##\n> \n> ```\n> [master (root-commit) 3b61ecc] Initial commit\n> reference-transaction (prepared):\n> 0000000000000000000000000000000000000000\n> 3b61ecc56006fcc283d42b302191e1385f19b551 refs/heads/test-branch\n> reference-transaction (committed):\n> 0000000000000000000000000000000000000000\n> 3b61ecc56006fcc283d42b302191e1385f19b551 refs/heads/test-branch\n> Created test-branch\n> reference-transaction (prepared):\n> 0000000000000000000000000000000000000000\n> 0000000000000000000000000000000000000000 refs/heads/test-branch\n> reference-transaction (committed):\n> 0000000000000000000000000000000000000000\n> 0000000000000000000000000000000000000000 refs/heads/test-branch\n> reference-transaction (aborted):\n> 0000000000000000000000000000000000000000\n> 0000000000000000000000000000000000000000 refs/heads/test-branch\n> reference-transaction (prepared):\n> 0000000000000000000000000000000000000000\n> 0000000000000000000000000000000000000000 refs/heads/test-branch\n> reference-transaction (committed):\n> 0000000000000000000000000000000000000000\n> 0000000000000000000000000000000000000000 refs/heads/test-branch\n> Deleted branch test-branch (was 3b61ecc).\n> ```\n> \n> My issues are 1) the reference-transaction deleting the branch goes\n> from a zero commit hash, instead of from the non-zero commit hash\n> 3b61ec, and 2) there's two such \"committed\" transactions for some\n> reason. Like the other example, there's also a mysterious unpaired\n> aborted transaction, but I assume that's not new behavior in this\n> release.\n"},{"id":"419619","messageId":"YFMCLSdImkW3B1rM@ncase","threadId":"55344","inReplyTo":"CAKjfCeDdSRk5QwyYduvbQvz0zC9FCZ3+5bseeOmZBOULxZ0D7w@mail.gmail.com","subject":"Re: Bug: reference-transaction hook for branch deletions broken between Git v2.30 and Git v2.31","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-03-18T07:33:01Z","receivedAt":"2021-03-18T07:34:05Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 17, 2021 at 08:42:10PM -0700, Waleed Khan wrote:\n[snip]\n> It's pretty strange that there was an \"aborted\" reference-transaction\n> from 0 to 0, especially with no previous \"prepared\"\n> reference-transaction, but that's not the bug in question, and I can\n> work around it by ignoring such transactions on my end.\n> \n> Notice that as part of the branch deletion, there is a\n> reference-transaction from a non-zero commit hash to a zero commit\n> hash.\n\nPeff has already answered the first part of your report, so I'm gonna\nconcentrate on the \"aborted\" reference-transaction invocation.\n\nWhat you're seeing here is the fact that git by default uses two\nreference backends: first the loose refs backend and second the\npacked-refs backend. All writes typically go into the loose refs backend\nfirst, and in most cases one doesn't need to bother with the packed-refs\nbackend for these writes as loose refs override everything that's\nexisting in the packed-refs file.\n\nThere is one exception though, which is deletion of references: if\ndeleting a loose ref, it may happene that the same ref in the\npacked-refs file is now unshadowed. For ref deletions, we thus need to\ndelete the ref from both the packed-refs file and also delete the loose\nref. And that is in fact two reference transactions: git will lock both\nthe loose ref it's about to delete and the packed-refs file, then delete\nthe packed-refs file if it exists and finally delete the loose ref. So\ntypically for deletions, you always get one transaction which does only\nforce deletes of refs existing in the packed-refs backend and then the\nactual transaction which does those deletions for loose refs.\n\nIn your specific case, you try to delete a reference which only exists\nas loose ref. We still set up the reference transaction for the\npacked-refs backend though, but only to realize that it doesn't need any\nupdates because it didn't contain any of the refs which are about to be\ndeleted. So what happens is that we simply abort the transaction without\neither first locking the backend nor committing it.\n\nThis particular issue has been biting us at GitLab, too: at times, nodes\nwere getting those weird force-delete-only transactions in \"committed\"\nstate, while the other nodes didn't. This did cause errors from time to\ntime when users tried to delete refs e.g. via a push because the nodes\ntried to vote on different outcomes. It took me some time to realize\nthis was happening in case packed refs were about to be deleted [1] and\nthat we now implicitly started to depend on whether a ref was packed or\nnot. For us, we \"fixed\" it by ignoring transactions which only have\nforce deletions.\n\nPatrick\n\n[1]: https://gitlab.com/gitlab-org/gitaly/-/merge_requests/3146\n"}]}