reference-transaction regression in 2.36.0-rc1

reference-transaction regression in 2.36.0-rc1

[Bryan Turner]

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.

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

If you _skip_ the pack-refs, you'll get output like this:
$ git branch -d to-delete
Callback: prepared
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
aab0f2e707acd1b84e81f4e4b833ff0d9ee7cc50
--
Callback: committed
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'
Deleted branch to-delete (was aab0f2e).

You can see that, during "prepared", rev-parse returns aab0f2e7 and
after "committed" the ref no longer exists.

With the pack-refs, you get this:
$ git branch -d to-delete
Callback: prepared
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'
Callback: committed
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'
Deleted branch to-delete (was aab0f2e).

In both cases, the reference-transaction isn't invoked with the hash
of the ref being deleted (even though Git clearly _knows_ it, since it
outputs it itself afterward), but if the ref exists loose we can at
least find out what it was.

Contrast this to Git 2.35.1:
$ git branch -d to-delete
Callback: prepared
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
aab0f2e707acd1b84e81f4e4b833ff0d9ee7cc50
--
Callback: committed
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'
Callback: aborted
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'
Callback: prepared
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'
Callback: committed
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'
Deleted branch to-delete (was aab0f2e).

With the reference-transactions for both packed and loose, when the
packed reference transaction runs we can still see the commit hash.

It seems like Git should either:
- Provide the PREPARED callback before _either_ packed or loose refs
are updated, or
- Provide the actual SHA as the old hash when invoking the hook (which
would be great because it would save us the overhead of resolving it
ourselves)

It's probably complicated to do either of those, but without one or
the other this change makes it very difficult to replicate updates
reliably via reference-transaction because we can't be 100% sure we're
applying the same deletion on replicas without knowing the old hash.

Best regards,
Bryan Turner

(Apologies for the double-send; forgot to enable plain text mode the
first time.)

Re: reference-transaction regression in 2.36.0-rc1

[Bryan Turner]

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

Re: reference-transaction regression in 2.36.0-rc1

[Æ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.

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

Re: reference-transaction regression in 2.36.0-rc1

[René Scharfe]

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

Re: reference-transaction regression in 2.36.0-rc1

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> This does look lik a series regression.

Yes.

> 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.

With the issue that /var/tmp/.git can cause trouble to those who
work in /var/tmp/$USER/playpen being taken reasonably good care of
already, it seems this is the issue with the highest priority at
this point.

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

Thanks.

Re: reference-transaction regression in 2.36.0-rc1

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> This does look lik a series regression.
>
> Yes.
>
>> 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.
>
> With the issue that /var/tmp/.git can cause trouble to those who
> work in /var/tmp/$USER/playpen being taken reasonably good care of
> already, it seems this is the issue with the highest priority at
> this point.
>
>> 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).

Reverting the merge 991b4d47f0a as a whole is an option.  It might
be the safest thing to do, if we do not to want to extend the cycle
and add a few more -rc releases before the final.

Thanks.

Re: reference-transaction regression in 2.36.0-rc1

[Bryan Turner]

On Wed, Apr 13, 2022 at 3:48 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> >
> >> This does look lik a series regression.
> >
> > Yes.
> >
> >> 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.
> >
> > With the issue that /var/tmp/.git can cause trouble to those who
> > work in /var/tmp/$USER/playpen being taken reasonably good care of
> > already, it seems this is the issue with the highest priority at
> > this point.
> >
> >> 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).
>
> Reverting the merge 991b4d47f0a as a whole is an option.  It might
> be the safest thing to do, if we do not to want to extend the cycle
> and add a few more -rc releases before the final.

I'm reviewing the changes in the patch series, but while I understand
the visible behavior fairly well the internals of it aren't something
I've spent a lot of time in. Compounding that, I'm down to my final 3
days with Atlassian before I start a new job, so I'm juggling trying
to finish up all my work and/or hand things over to others.

Based on that, I definitely would not want work on 2.36 to hinge on me
getting a patch up. I would _love_ to (finally) contribute something
back to Git, and the community that has helped me so much over the
last 10 years, but I can't say exactly when I'd be able to post
something. There's another developer internally who is also looking at
this, but it's been a number of years since he worked in C.

I guess all that's a long-winded way of saying a revert might be the
more timely option, for decoupling resolving this issue from the 2.36
timeline. That would also mean Patrick would be able to review any fix
that was posted, which seems valuable since he did the original work.

>
> Thanks.

Re: reference-transaction regression in 2.36.0-rc1

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Reverting the merge 991b4d47f0a as a whole is an option.  It might
> be the safest thing to do, if we do not to want to extend the cycle
> and add a few more -rc releases before the final.

It turns out that this involves nontrivial amount of work to get
right, as the bottom commit of Patrick's "fetch --atomic" series
wants to count how many transaction we make, instead of ensuring
that the updates to the references are all-or-none, so at least
2a0cafd4 (fetch: increase test coverage of fetches, 2022-02-17)
needs to be reverted as well.  This in turn is made unnecessarily
more cumbersome as the history in the t/ directory is littered with
unrelated "clean-up" patches since these topics were merged.

Re: reference-transaction regression in 2.36.0-rc1

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Reverting the merge 991b4d47f0a as a whole is an option.  It might
>> be the safest thing to do, if we do not to want to extend the cycle
>> and add a few more -rc releases before the final.
>
> It turns out that this involves nontrivial amount of work to get
> right, as the bottom commit of Patrick's "fetch --atomic" series
> wants to count how many transaction we make, instead of ensuring
> that the updates to the references are all-or-none, so at least
> 2a0cafd4 (fetch: increase test coverage of fetches, 2022-02-17)
> needs to be reverted as well.  This in turn is made unnecessarily
> more cumbersome as the history in the t/ directory is littered with
> unrelated "clean-up" patches since these topics were merged.

I have a tentative revert of the merge and also the "fetch --atomic"
test that (unnecessarily) counted the number of transactions directly
on top of planned 'master', which merges the planned v2.35.3 into
v2.36-rc2 and pushed it as if it were the 'seen' branch.

The CI job https://github.com/git/git/actions/runs/2164208587 seems
to be doing well so far, so once the dust from releasing v2.30.4,
v2.31.3, v2.32.2, v2.33.3, v2.34.3, and v2.35.3 with Derrick's
hotfix for yesterday's CVE fix, I may merge it down to 'master'
before the final.

Thanks.

Re: reference-transaction regression in 2.36.0-rc1

[Bryan Turner]

[-- Attachment #1: Type: text/plain, Size: 7299 bytes --]

On Wed, Apr 13, 2022 at 12:52 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> > This does look lik a series regression.
>
> Yes.
>
> > 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.
>
> With the issue that /var/tmp/.git can cause trouble to those who
> work in /var/tmp/$USER/playpen being taken reasonably good care of
> already, it seems this is the issue with the highest priority at
> this point.
>
> > 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 :)
>
> Thanks.

One of my colleagues was able to spend some further time investigating
this. He also experimented with commands other than "git branch -d"
and found that "git update-ref -d", for example does not exhibit the
issue, but "git tag -d" does. Storing a replay of the various
reference-transaction callbacks shows that for every command _other
than_ "git branch -d" and "git tag -d", pre-2.36 reference
transactions follow the pattern "prepared", "prepared", "committed",
"committed". "git branch -d" and "git tag -d", on the other hand, go
"prepared", "committed", "prepared", "committed". This implies the
reference-transaction behavior is _already broken_ in 2.35, and the
changes in 2.36 to suppress packed callbacks just make it more
obvious.

For reference, here are the reference-transactions for _2.35_ using
"git branch -d" and "git update-ref -d". In both runs, the ref only
exists packed--there is no loose ref to delete. My
reference-transaction script is simple:
$ cat .git/hooks/reference-transaction
echo "-- $1"
cat
git rev-parse refs/heads/to-delete --
exit 0

 $ git-2.35.1 branch -d to-delete
-- prepared
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
1767344659fd3f7a6b788020203f74baeea0e0f7
--
-- committed
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'
-- aborted
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'
-- prepared
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'
-- committed
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'
Deleted branch to-delete (was 1767344).

$ git-2.35.1 update-ref -d refs/heads/to-delete
-- prepared
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
1767344659fd3f7a6b788020203f74baeea0e0f7
--
-- prepared
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
1767344659fd3f7a6b788020203f74baeea0e0f7
--
-- committed
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'
-- committed
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'

Now, the same two scenarios in 2.36.0-rc2:

$ git-2.36.0-rc2 branch -d to-delete
-- prepared
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'
-- committed
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'
Deleted branch to-delete (was 1767344).

$ git-2.36.0-rc2 update-ref -d refs/heads/to-delete
-- prepared
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
1767344659fd3f7a6b788020203f74baeea0e0f7
--
-- committed
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'

Looking at the data this way makes it more obvious that the issue
isn't actually new in 2.36; even in 2.35, the branch is fully deleted
before the _loose_ transaction is prepared, with "git branch -d".
Since "git update-ref -d" prepares both transactions before committing
either, the branch still exists in both "prepared" callbacks. The real
difference here, between 2.35 and 2.36, is that Bitbucket Server (and,
ostensibly, other reference-transaction users), with enough checking,
can essentially pick-and-choose between "prepared" and "committed"
callbacks to cobble together a working pairing: For "git branch -d",
we replicate on the first "prepared" and wrap up replication on the
_second_ "committed", and for "git update-ref -d" we replicate on the
_second_ "prepared" and wrap up on the _second_ "committed". Since the
packed callbacks are gone in 2.36, that's not possible.

The attached patch on top of the v2.36.0-rc2 tag fixes the issue, and
all of the t14xx tests (including t1416) pass (aside from known
issues). (Apologies for attaching the patch rather than inlining it;
the Gmail editor butchers it if I try that.) With the patch applied,
the reference-transaction callbacks are identical to 2.36.0-rc2, in
terms of when they run, but the state of the ref is different:

$ git-patched branch -d to-delete
-- prepared
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
1767344659fd3f7a6b788020203f74baeea0e0f7
--
-- committed
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'
Deleted branch to-delete (was 1767344).

$ git-patched update-ref -d refs/heads/to-delete
-- prepared
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
1767344659fd3f7a6b788020203f74baeea0e0f7
--
-- committed
0000000000000000000000000000000000000000
0000000000000000000000000000000000000000 refs/heads/to-delete
fatal: bad revision 'refs/heads/to-delete'

Now "git branch -d" and "git update-ref -d" have the same callbacks,
and see the same ref state in each.

I'm happy to try and format this into a proper patch, with sign-offs
and new tests, if folks think this is the way to go. If the 2.36
release cycle is too far along, I can still try and get a patch to the
list for inclusion in 2.37 (though I'm less confident what commit I'd
base the patch on in that scenario). Any pointers would be
appreciated. (Or, if someone with more familiarity than me, not to
mention a patch submission setup that already works, wants to take
over, that's fine too.)

Best regards,
Bryan Turner

[-- Attachment #2: files-delete-ref.patch --]
[-- Type: application/octet-stream, Size: 1056 bytes --]

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 95acab78ee..c11abef186 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1273,29 +1273,12 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
 	if (!refnames->nr)
 		return 0;
 
-	if (packed_refs_lock(refs->packed_ref_store, 0, &err))
-		goto error;
-
-	transaction = ref_store_transaction_begin(refs->packed_ref_store,
-						  REF_TRANSACTION_SKIP_HOOK, &err);
-	if (!transaction)
-		goto error;
-
-	result = packed_refs_delete_refs(refs->packed_ref_store,
-					 transaction, msg, refnames, flags);
-	if (result)
-		goto error;
-
-	packed_refs_unlock(refs->packed_ref_store);
-
 	for (i = 0; i < refnames->nr; i++) {
 		const char *refname = refnames->items[i].string;
 
-		if (refs_delete_ref(&refs->base, msg, refname, NULL, flags))
+		if (refs_delete_ref(ref_store, msg, refname, NULL, flags))
 			result |= error(_("could not remove reference %s"), refname);
 	}
-
-	ref_transaction_free(transaction);
 	strbuf_release(&err);
 	return result;
 

Re: reference-transaction regression in 2.36.0-rc1

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 14 2022, Bryan Turner wrote:

> I'm happy to try and format this into a proper patch, with sign-offs
> and new tests, if folks think this is the way to go. If the 2.36
> release cycle is too far along, I can still try and get a patch to the
> list for inclusion in 2.37 (though I'm less confident what commit I'd
> base the patch on in that scenario). Any pointers would be
> appreciated. (Or, if someone with more familiarity than me, not to
> mention a patch submission setup that already works, wants to take
> over, that's fine too.)

I think that would be great if you can spend the time, but Junio's
already pushed out a revert of the topic for the final 2.36.0, can you
confirm that current "master" solves the issues you had?

So revert/retry & extra tests etc. can wait until the post-release, and
likely Patrick will be back to work on it at that time...

Re: reference-transaction regression in 2.36.0-rc1

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I think that would be great if you can spend the time, but Junio's
> already pushed out a revert of the topic for the final 2.36.0, can you
> confirm that current "master" solves the issues you had?

Thanks for asking this question, which is lot more urgent and I
should have asked last night before I went to bed.  Yes, Bryan
already said that the revert in 'seen' the previous day made their
system happier, but it certainly helps to know the reverts were OK
at the tip of 'master' without all the other random topics.

> So revert/retry & extra tests etc. can wait until the post-release, and
> likely Patrick will be back to work on it at that time...

Yup.

Re: reference-transaction regression in 2.36.0-rc1

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

Thanks a lot to all of you for handling this regression while I was out
of office!

On Tue, Apr 19, 2022 at 11:54:19AM +0200, Michael Heemskerk wrote:
> Apologies for the late reply. Bryan had his last day at Atlassian last
> week, so I'll take over from him on this topic.
> 
> Thanks for asking this question, which is lot more urgent and I
> > should have asked last night before I went to bed.  Yes, Bryan
> > already said that the revert in 'seen' the previous day made their
> > system happier, but it certainly helps to know the reverts were OK
> > at the tip of 'master' without all the other random topics.
> >
> 
> I have run all our tests on the tip of master prior to 2.36 being released,
>  and on the released 2.36. Our tests are happy on both, so thanks for
>  reverting those reference-transaction changes for 2.36.
> 
> I'd be happy to provide the fix to files_delete_refs in a patch (including
> some extra tests) on top of Patrick's
> avoid-unnecessary-hook-invocation-with-packed-refs series if and
> when that series is unreverted on 'next'.

That would be great. To be clear, do the fixes you have also fix the
pre-v2.36.0 issues Bryan has mentioned?

Please let me know whether you want to pursue this and whether you need
any further help with it.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: reference-transaction regression in 2.36.0-rc1

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3792 bytes --]

On Mon, May 02, 2022 at 11:41:26AM +0200, Michael Heemskerk wrote:
> > > I'd be happy to provide the fix to files_delete_refs in a patch
> > (including
> > > some extra tests) on top of Patrick's
> > > avoid-unnecessary-hook-invocation-with-packed-refs series if and
> > > when that series is unreverted on 'next'.
> >
> > That would be great. To be clear, do the fixes you have also fix the
> > pre-v2.36.0 issues Bryan has mentioned?
> >
> 
> That is correct. The pre-v2.36.0 issues are caused by files_delete_refs
> first
> preparing and committing a transaction for the packed_ref_store, and then
> iterating over each of the to-be-deleted refs and preparing and committing
> another transaction per ref. This latter transaction triggers the usual
> ABORTED (from packed_ref_store), PREPARED (ref_store),
> COMMITTED (ref_store) callbacks.
> 
> As far as I can tell, all we need to do is remove the separate transaction
> for
> the packed_ref_store.

Do you mean that we should just get rid of the transaction and instead
delete the refs in the packet-refs backend one by one? If so, then I
think that would be a performance regressions in a lot of places. If you
are deleting a bunch of references at the same time, then you do indeed
want to make this a single packed-refs transaction or otherwise we'd
repeatedly rewrite the whole file. And given that this file can easily
range in the hundreds of megabytes this can easily go into pathological
cases.

I think what we need to do instead is a bit more complicated:

    1. Lock the packed-refs backend. This is to ensure that it's not
       being updated while we update loose refs.

    2. Create a transaction for the loose-refs and queue all refs that
       we are about to delete. Now we're safe to prepate the loose refs,
       and at this point in time nothing has been pruned yet. As a
       result, it is still possible for the reference-transaction hook
       to intervene even if the refs only existed as packed-refs file.

    3. Now we have to prepare and commit the packed-refs file. This is
       to avoid a race where deleting loose refs would uncover anything
       that existed in the packed-refs file already.

    4. Unlock the packed-refs backend.

    5. Commit the loose-refs transaction we have prepared already.

This should be race-free and fix the usecase for aborting ref deletions
via the reference-transaction hook. It has the downside though that
packed-refs are locked for far longer than they had been before because
the lock also spans over preparation of the loose-refs transaction.

> Another thing we need to decide is whether or not to backport the fix to
> 2.36 and possibly older. My suggestion would be to only apply the fix to
> 'next' and NOT backport it to older git versions as changing the
> reference-transaction semantics in a patch release would be unexpected.

The way it sounds like this has been a longer-standing issue.
Furthermore, it's not a regression: it just hasn't ever been working. So
I don't feel like it's critical to backport this.

> > Please let me know whether you want to pursue this and whether you need
> > any further help with it.
> >
> 
> I'm happy to provide the patch, but assume that I need to wait for Patrick's
> series to be reapplied to 'next'? Alternatively, I can send the patch to
> Patrick
> to be included in a reroll of the series.
> 
> Michael

Given that you said that my patch series made the preexisting problem
worse I'd prefer to first land a proper fix for this issue, and only
then reapply my own patches on top. I'm also happy to fix the issue
myself -- at GitLab, we also care about this working correctly. Just let
me know your preference.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]