Re: reference-transaction regression in 2.36.0-rc1

From: Bryan Turner <bturner@atlassian.com>
Date: Apr 12, 2022, 20:53 UTC
Message-ID: <CAGyf7-GaoBarXD2xKG3KUXwGZgbhKgv-4Mz45achbr6G9ihTBQ@mail.gmail.com>
In-Reply-To: <CAGyf7-GYYA2DOnAVYW--=QEc2WjSHzUhp2OQyuyOr3EOtFDm6g@mail.gmail.com>

On Tue, Apr 12, 2022 at 2:20 AM Bryan Turner <bturner@atlassian.com> wrote:

Show 14 quoted lines

>
> It looks like Git 2.36.0-rc1 includes the changes to eliminate/pare
> back reference transactions being raised independently for loose and
> packed refs. It's a great improvement, and one we're very grateful
> for.
>
> It looks like there's a regression, though. When deleting a ref that
> _only_ exists in packed-refs, by the time the "prepared" callback is
> raised the ref has already been deleted completely. Normally when
> "prepared" is raised the ref is still present. The ref still being
> present is important to us, since the reference-transaction hook is
> frequently not passed any previous hash; we resolve the ref during
> "prepared", if the previous hash is the null SHA1, so that we can
> correctly note what the tip commit was when the ref was deleted.

I've re-tested this with 2.36.0-rc2 and it has the same regression (as expected). However, in playing with it more, the regression is more serious than I had initially considered. It goes beyond just losing access to the SHA of the tip commit for deleted refs. If a ref only exists packed, this regression means vetoing the "prepared" callback _cannot prevent its deletion_, which violates the contract for the reference-transaction as I understand it.

Here's a slightly modified reproduction: git init ref-tx cd ref-tx git commit -m "Initial commit" --allow-empty git branch to-delete git pack-refs --all echo 'exit 1;' > .git/hooks/reference-transaction chmod +x .git/hooks/reference-transaction git branch -d to-delete

Running this reproduction ends with: $ git branch -d to-delete fatal: ref updates aborted by hook $ git rev-parse to-delete -- fatal: bad revision 'to-delete'

Even though the reference-transaction vetoed "prepared", the ref was still deleted.

In Bitbucket, we use the reference-transaction to perform replication. When we get the "prepared" callback on one machine, we dispatch the same change(s) to other replicas. Those replicas process the changes and arrive at their own "prepared" callbacks (or don't), at which point they vote to commit or rollback. The votes are tallied and the majority decision wins.

With this regression, that sort of setup no longer works reliably for ref deletions. If the ref only exists packed, it's deleted (and _visibly_ deleted) before we ever get the "prepared" callback. So if coordination fails (i.e. the majority votes to rollback), even if we try to abort the change it's already too late.

Best regards, Bryan Turner