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

[Patrick Steinhardt <ps@pks.im>]

[-- 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 --]