Re: reference-transaction regression in 2.36.0-rc1

["René Scharfe" <l.s.r@web.de>]

Am 13.04.22 um 16:34 schrieb Ævar Arnfjörð Bjarmason:
>
> On Tue, Apr 12 2022, Bryan Turner wrote:
>
>> 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.
>
> 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.

Indeed, it bisects to 2ed1b64ebd (refs: skip hooks when deleting
uncovered packed refs, 2022-01-17).

> 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 :)