On Tue, Apr 12 2022, Bryan Turner wrote: > On Tue, Apr 12, 2022 at 2:20 AM Bryan Turner <bturner@xxxxxxxxxxxxx> 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. This does look lik a series regression. I haven't had time to bisect this, but I suspect that it'll come down to something in the 991b4d47f0a (Merge branch 'ps/avoid-unnecessary-hook-invocation-with-packed-refs', 2022-02-18) series. I happen to know that Patrick is OoO until after the final v2.36.0 is scheduled (but I don't know for sure that we won't spot this thread & act on it before then). Is this something you think you'll be able to dig into further and possibly come up with a patch? It looks like you're way ahead of at least myself in knowing how this *should* work :)
