* reference-transaction regression in 2.36.0-rc1 @ 2022-04-12 9:20 Bryan Turner 2022-04-12 20:53 ` Bryan Turner 0 siblings, 1 reply; 14+ messages in thread From: Bryan Turner @ 2022-04-12 9:20 UTC (permalink / raw) To: Git Users; +Cc: Patrick Steinhardt 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.) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: reference-transaction regression in 2.36.0-rc1 2022-04-12 9:20 reference-transaction regression in 2.36.0-rc1 Bryan Turner @ 2022-04-12 20:53 ` Bryan Turner 2022-04-13 14:34 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 14+ messages in thread From: Bryan Turner @ 2022-04-12 20:53 UTC (permalink / raw) To: Git Users; +Cc: Patrick Steinhardt 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: reference-transaction regression in 2.36.0-rc1 2022-04-12 20:53 ` Bryan Turner @ 2022-04-13 14:34 ` Ævar Arnfjörð Bjarmason 2022-04-13 16:21 ` René Scharfe 2022-04-13 19:52 ` Junio C Hamano 0 siblings, 2 replies; 14+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-04-13 14:34 UTC (permalink / raw) To: Bryan Turner; +Cc: Git Users, Patrick Steinhardt 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 :) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: reference-transaction regression in 2.36.0-rc1 2022-04-13 14:34 ` Ævar Arnfjörð Bjarmason @ 2022-04-13 16:21 ` René Scharfe 2022-04-13 19:52 ` Junio C Hamano 1 sibling, 0 replies; 14+ messages in thread From: René Scharfe @ 2022-04-13 16:21 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, Bryan Turner Cc: Git Users, Patrick Steinhardt 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 :) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: reference-transaction regression in 2.36.0-rc1 2022-04-13 14:34 ` Ævar Arnfjörð Bjarmason 2022-04-13 16:21 ` René Scharfe @ 2022-04-13 19:52 ` Junio C Hamano 2022-04-13 22:44 ` Junio C Hamano 2022-04-15 2:37 ` Bryan Turner 1 sibling, 2 replies; 14+ messages in thread From: Junio C Hamano @ 2022-04-13 19:52 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Bryan Turner, Git Users, Patrick Steinhardt Æ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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: reference-transaction regression in 2.36.0-rc1 2022-04-13 19:52 ` Junio C Hamano @ 2022-04-13 22:44 ` Junio C Hamano 2022-04-13 22:55 ` Bryan Turner 2022-04-13 22:56 ` Junio C Hamano 2022-04-15 2:37 ` Bryan Turner 1 sibling, 2 replies; 14+ messages in thread From: Junio C Hamano @ 2022-04-13 22:44 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Bryan Turner, Git Users, Patrick Steinhardt 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: reference-transaction regression in 2.36.0-rc1 2022-04-13 22:44 ` Junio C Hamano @ 2022-04-13 22:55 ` Bryan Turner 2022-04-13 22:56 ` Junio C Hamano 1 sibling, 0 replies; 14+ messages in thread From: Bryan Turner @ 2022-04-13 22:55 UTC (permalink / raw) To: Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, Git Users, Patrick Steinhardt 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: reference-transaction regression in 2.36.0-rc1 2022-04-13 22:44 ` Junio C Hamano 2022-04-13 22:55 ` Bryan Turner @ 2022-04-13 22:56 ` Junio C Hamano 2022-04-13 23:31 ` Junio C Hamano 1 sibling, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2022-04-13 22:56 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Bryan Turner, Git Users, Patrick Steinhardt 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: reference-transaction regression in 2.36.0-rc1 2022-04-13 22:56 ` Junio C Hamano @ 2022-04-13 23:31 ` Junio C Hamano 0 siblings, 0 replies; 14+ messages in thread From: Junio C Hamano @ 2022-04-13 23:31 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Bryan Turner, Git Users, Patrick Steinhardt 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: reference-transaction regression in 2.36.0-rc1 2022-04-13 19:52 ` Junio C Hamano 2022-04-13 22:44 ` Junio C Hamano @ 2022-04-15 2:37 ` Bryan Turner 2022-04-15 13:27 ` Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 14+ messages in thread From: Bryan Turner @ 2022-04-15 2:37 UTC (permalink / raw) To: Junio C Hamano Cc: Ævar Arnfjörð Bjarmason, Git Users, Patrick Steinhardt [-- 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; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: reference-transaction regression in 2.36.0-rc1 2022-04-15 2:37 ` Bryan Turner @ 2022-04-15 13:27 ` Ævar Arnfjörð Bjarmason 2022-04-15 16:53 ` Junio C Hamano 0 siblings, 1 reply; 14+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-04-15 13:27 UTC (permalink / raw) To: Bryan Turner; +Cc: Junio C Hamano, Git Users, Patrick Steinhardt 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... ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: reference-transaction regression in 2.36.0-rc1 2022-04-15 13:27 ` Ævar Arnfjörð Bjarmason @ 2022-04-15 16:53 ` Junio C Hamano [not found] ` <CAJDSCnOUQm-doY-xTobPk64oeiSsTd+vSFzsb_cz9BLORAxXGA@mail.gmail.com> 0 siblings, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2022-04-15 16:53 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Bryan Turner, Git Users, Patrick Steinhardt Æ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. ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <CAJDSCnOUQm-doY-xTobPk64oeiSsTd+vSFzsb_cz9BLORAxXGA@mail.gmail.com>]
* Re: reference-transaction regression in 2.36.0-rc1 [not found] ` <CAJDSCnOUQm-doY-xTobPk64oeiSsTd+vSFzsb_cz9BLORAxXGA@mail.gmail.com> @ 2022-04-27 11:05 ` Patrick Steinhardt [not found] ` <CAJDSCnM767fdo6qy085jc9sqezvbBqDD4ikXz1y5tHEjSYED2A@mail.gmail.com> 0 siblings, 1 reply; 14+ messages in thread From: Patrick Steinhardt @ 2022-04-27 11:05 UTC (permalink / raw) To: Michael Heemskerk Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, Bryan Turner, Git Users [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <CAJDSCnM767fdo6qy085jc9sqezvbBqDD4ikXz1y5tHEjSYED2A@mail.gmail.com>]
* Re: reference-transaction regression in 2.36.0-rc1 [not found] ` <CAJDSCnM767fdo6qy085jc9sqezvbBqDD4ikXz1y5tHEjSYED2A@mail.gmail.com> @ 2022-05-02 11:12 ` Patrick Steinhardt 0 siblings, 0 replies; 14+ messages in thread From: Patrick Steinhardt @ 2022-05-02 11:12 UTC (permalink / raw) To: Michael Heemskerk Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, Git Users [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-05-02 11:12 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-12 9:20 reference-transaction regression in 2.36.0-rc1 Bryan Turner 2022-04-12 20:53 ` Bryan Turner 2022-04-13 14:34 ` Ævar Arnfjörð Bjarmason 2022-04-13 16:21 ` René Scharfe 2022-04-13 19:52 ` Junio C Hamano 2022-04-13 22:44 ` Junio C Hamano 2022-04-13 22:55 ` Bryan Turner 2022-04-13 22:56 ` Junio C Hamano 2022-04-13 23:31 ` Junio C Hamano 2022-04-15 2:37 ` Bryan Turner 2022-04-15 13:27 ` Ævar Arnfjörð Bjarmason 2022-04-15 16:53 ` Junio C Hamano [not found] ` <CAJDSCnOUQm-doY-xTobPk64oeiSsTd+vSFzsb_cz9BLORAxXGA@mail.gmail.com> 2022-04-27 11:05 ` Patrick Steinhardt [not found] ` <CAJDSCnM767fdo6qy085jc9sqezvbBqDD4ikXz1y5tHEjSYED2A@mail.gmail.com> 2022-05-02 11:12 ` Patrick Steinhardt
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).