Bug: reference-transaction hook for branch deletions broken between Git v2.30 and Git v2.31

Hello,

The `reference-transaction` hook seems to have broken between Git v2.30 and v2.31, or at least violated my expectations as a user.

I didn't see any mention of the `reference-transaction` hook in the release notes, so I assume that this is a bug. Given that there's documentation at `man githooks` for the `reference-transaction` hook, I assume that the feature is no longer in a preliminary stage, and so a bug report is warranted. I couldn't find any mention of a `reference-transaction` hook bug already having been reported in the mailing list search online.

## Reproduction ##

To reproduce, run this script:

``` #!/bin/sh set -eu

# Change between Git v2.30.0 and v2.31.0 here. GIT=${GIT:-$(which git)} echo "Running version $("$GIT" version)"

# For determinism. export GIT_COMMITTER_DATE="Wed Mar 17 16:53:32 PDT 2021" export GIT_AUTHOR_DATE="Wed Mar 17 16:53:32 PDT 2021"

rm -rf repo mkdir repo cd repo "$GIT" init "$GIT" commit --allow-empty -m 'Initial commit'

mkdir .git/hooks cat >.git/hooks/reference-transaction <<'EOF' #!/bin/sh echo "reference-transaction ($1):" cat EOF chmod +x .git/hooks/reference-transaction

"$GIT" branch -v 'test-branch' echo "Created test-branch" "$GIT" branch -v -d 'test-branch' ```

## Expected behavior (git v2.30) ##

This is the output:

``` [master (root-commit) 3b61ecc] Initial commit reference-transaction (prepared): 0000000000000000000000000000000000000000 3b61ecc56006fcc283d42b302191e1385f19b551 refs/heads/test-branch reference-transaction (committed): 0000000000000000000000000000000000000000 3b61ecc56006fcc283d42b302191e1385f19b551 refs/heads/test-branch Created test-branch reference-transaction (aborted): 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/heads/test-branch reference-transaction (prepared): 3b61ecc56006fcc283d42b302191e1385f19b551 0000000000000000000000000000000000000000 refs/heads/test-branch reference-transaction (committed): 3b61ecc56006fcc283d42b302191e1385f19b551 0000000000000000000000000000000000000000 refs/heads/test-branch Deleted branch test-branch (was 3b61ecc). ```

It's pretty strange that there was an "aborted" reference-transaction from 0 to 0, especially with no previous "prepared" reference-transaction, but that's not the bug in question, and I can work around it by ignoring such transactions on my end.

Notice that as part of the branch deletion, there is a reference-transaction from a non-zero commit hash to a zero commit hash.

## Actual behavior (git v2.31) ##

``` [master (root-commit) 3b61ecc] Initial commit reference-transaction (prepared): 0000000000000000000000000000000000000000 3b61ecc56006fcc283d42b302191e1385f19b551 refs/heads/test-branch reference-transaction (committed): 0000000000000000000000000000000000000000 3b61ecc56006fcc283d42b302191e1385f19b551 refs/heads/test-branch Created test-branch reference-transaction (prepared): 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/heads/test-branch reference-transaction (committed): 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/heads/test-branch reference-transaction (aborted): 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/heads/test-branch reference-transaction (prepared): 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/heads/test-branch reference-transaction (committed): 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 refs/heads/test-branch Deleted branch test-branch (was 3b61ecc). ```

My issues are 1) the reference-transaction deleting the branch goes from a zero commit hash, instead of from the non-zero commit hash 3b61ec, and 2) there's two such "committed" transactions for some reason. Like the other example, there's also a mysterious unpaired aborted transaction, but I assume that's not new behavior in this release.

## `git bugreport` system info ##

`git` 2.30 bugreport (built from source):

[System Info] git version: git version 2.30.2 cpu: x86_64 built from commit: 94f6e3e283f2adfc518b39cfc39291f1c2832ad0 sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh uname: Darwin 19.6.0 Darwin Kernel Version 19.6.0: Thu Oct 29 22:56:45 PDT 2020; root:xnu-6153.141.2.2~1/RELEASE_X86_64 x86_64 compiler info: clang: 12.0.0 (clang-1200.0.32.21) libc info: no libc information available $SHELL (typically, interactive shell): /bin/zsh

`git` 2.31 bugreport (built from source):

[System Info] git version: git version 2.31.0 cpu: x86_64 built from commit: a5828ae6b52137b913b978e16cd2334482eb4c1f sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh uname: Darwin 19.6.0 Darwin Kernel Version 19.6.0: Thu Oct 29 22:56:45 PDT 2020; root:xnu-6153.141.2.2~1/RELEASE_X86_64 x86_64 compiler info: clang: 12.0.0 (clang-1200.0.32.21) libc info: no libc information available $SHELL (typically, interactive shell): /bin/zsh

The issue also reproduces in CI builds of Git on Linux.

On Wed, Mar 17, 2021 at 08:42:10PM -0700, Waleed Khan wrote:

Thanks for a clear explanation and reproduction recipe. I can see the bug easily here. It bisects to 8198907795 (use delete_refs when deleting tags or branches, 2021-01-20). I've cc'd the author of that commit, as well as the author of the ref-transaction hook (I've also retained the reproduction info for them at the end of the message).

I _think_ this is probably another variant of what we were discussing in:

  https://lore.kernel.org/git/d255c7a5f95635c2e7ae36b9689c3efd07b4df5d.1604501894.git.ps@pks.im/

and that ended up with the documentation from:

  https://lore.kernel.org/git/55905b8693dd49637d0516ee123405cbfb58b6c6.1614591751.git.ps@pks.im/

Namely that the hook is not seeing the "old" ref value as it was on disk, but rather they see the value that the caller asked us to verify existed (or all zeroes if the caller didn't specify). So I think what happened is that git-branch went from calling "delete the ref X with value 1234abcd" to "delete the ref X, no matter what it's value is".

-Peff

-- >8 --

On Wed, Mar 17, 2021 at 08:42:10PM -0700, Waleed Khan wrote: [snip]

Peff has already answered the first part of your report, so I'm gonna concentrate on the "aborted" reference-transaction invocation.

What you're seeing here is the fact that git by default uses two reference backends: first the loose refs backend and second the packed-refs backend. All writes typically go into the loose refs backend first, and in most cases one doesn't need to bother with the packed-refs backend for these writes as loose refs override everything that's existing in the packed-refs file.

There is one exception though, which is deletion of references: if deleting a loose ref, it may happene that the same ref in the packed-refs file is now unshadowed. For ref deletions, we thus need to delete the ref from both the packed-refs file and also delete the loose ref. And that is in fact two reference transactions: git will lock both the loose ref it's about to delete and the packed-refs file, then delete the packed-refs file if it exists and finally delete the loose ref. So typically for deletions, you always get one transaction which does only force deletes of refs existing in the packed-refs backend and then the actual transaction which does those deletions for loose refs.

In your specific case, you try to delete a reference which only exists as loose ref. We still set up the reference transaction for the packed-refs backend though, but only to realize that it doesn't need any updates because it didn't contain any of the refs which are about to be deleted. So what happens is that we simply abort the transaction without either first locking the backend nor committing it.

This particular issue has been biting us at GitLab, too: at times, nodes were getting those weird force-delete-only transactions in "committed" state, while the other nodes didn't. This did cause errors from time to time when users tried to delete refs e.g. via a push because the nodes tried to vote on different outcomes. It took me some time to realize this was happening in case packed refs were about to be deleted [1] and that we now implicitly started to depend on whether a ref was packed or not. For us, we "fixed" it by ignoring transactions which only have force deletions.

Patrick

[1]: https://gitlab.com/gitlab-org/gitaly/-/merge_requests/3146