Re: reference-transaction regression in 2.36.0-rc1

[Bryan Turner <bturner@atlassian.com>]

On Tue, Apr 12, 2022 at 2:20 AM Bryan Turner <bturner@atlassian.com> wrote:
>
> 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