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

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

[Waleed Khan]

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.

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

[Jeff King]

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

> 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.

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 --
> 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.

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2782 bytes --]

On Wed, Mar 17, 2021 at 08:42:10PM -0700, Waleed Khan wrote:
[snip]
> 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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]