{"thread":{"id":"57714","subject":"reference-transaction regression in 2.36.0-rc1","startedAt":"2022-04-12T10:29:59Z","lastAt":"2022-05-02T11:12:18Z","messageCount":14,"participants":["Bryan Turner","Ævar Arnfjörð Bjarmason","René Scharfe","Junio C Hamano","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"453444","messageId":"CAGyf7-GYYA2DOnAVYW--=QEc2WjSHzUhp2OQyuyOr3EOtFDm6g@mail.gmail.com","threadId":"57714","inReplyTo":null,"subject":"reference-transaction regression in 2.36.0-rc1","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2022-04-12T09:20:21Z","receivedAt":"2022-04-12T10:29:59Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"It looks like Git 2.36.0-rc1 includes the changes to eliminate/pare\nback reference transactions being raised independently for loose and\npacked refs. It's a great improvement, and one we're very grateful\nfor.\n\nIt looks like there's a regression, though. When deleting a ref that\n_only_ exists in packed-refs, by the time the \"prepared\" callback is\nraised the ref has already been deleted completely. Normally when\n\"prepared\" is raised the ref is still present. The ref still being\npresent is important to us, since the reference-transaction hook is\nfrequently not passed any previous hash; we resolve the ref during\n\"prepared\", if the previous hash is the null SHA1, so that we can\ncorrectly note what the tip commit was when the ref was deleted.\n\nHere's a reproduction:\ngit init ref-tx\ncd ref-tx\ngit commit -m \"Initial commit\" --allow-empty\ngit branch to-delete\ngit pack-refs --all\necho 'echo \"Callback: $1\"; cat; git rev-parse refs/heads/to-delete --;\nexit 0' > .git/hooks/reference-transaction\nchmod +x .git/hooks/reference-transaction\ngit branch -d to-delete\n\nIf you _skip_ the pack-refs, you'll get output like this:\n$ git branch -d to-delete\nCallback: prepared\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\naab0f2e707acd1b84e81f4e4b833ff0d9ee7cc50\n--\nCallback: committed\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\nDeleted branch to-delete (was aab0f2e).\n\nYou can see that, during \"prepared\", rev-parse returns aab0f2e7 and\nafter \"committed\" the ref no longer exists.\n\nWith the pack-refs, you get this:\n$ git branch -d to-delete\nCallback: prepared\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\nCallback: committed\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\nDeleted branch to-delete (was aab0f2e).\n\nIn both cases, the reference-transaction isn't invoked with the hash\nof the ref being deleted (even though Git clearly _knows_ it, since it\noutputs it itself afterward), but if the ref exists loose we can at\nleast find out what it was.\n\nContrast this to Git 2.35.1:\n$ git branch -d to-delete\nCallback: prepared\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\naab0f2e707acd1b84e81f4e4b833ff0d9ee7cc50\n--\nCallback: committed\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\nCallback: aborted\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\nCallback: prepared\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\nCallback: committed\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\nDeleted branch to-delete (was aab0f2e).\n\nWith the reference-transactions for both packed and loose, when the\npacked reference transaction runs we can still see the commit hash.\n\nIt seems like Git should either:\n- Provide the PREPARED callback before _either_ packed or loose refs\nare updated, or\n- Provide the actual SHA as the old hash when invoking the hook (which\nwould be great because it would save us the overhead of resolving it\nourselves)\n\nIt's probably complicated to do either of those, but without one or\nthe other this change makes it very difficult to replicate updates\nreliably via reference-transaction because we can't be 100% sure we're\napplying the same deletion on replicas without knowing the old hash.\n\nBest regards,\nBryan Turner\n\n(Apologies for the double-send; forgot to enable plain text mode the\nfirst time.)\n"},{"id":"453466","messageId":"CAGyf7-GaoBarXD2xKG3KUXwGZgbhKgv-4Mz45achbr6G9ihTBQ@mail.gmail.com","threadId":"57714","inReplyTo":"CAGyf7-GYYA2DOnAVYW--=QEc2WjSHzUhp2OQyuyOr3EOtFDm6g@mail.gmail.com","subject":"Re: reference-transaction regression in 2.36.0-rc1","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2022-04-12T20:53:34Z","receivedAt":"2022-04-12T21:04:05Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Tue, Apr 12, 2022 at 2:20 AM Bryan Turner <bturner@atlassian.com> wrote:\n>\n> It looks like Git 2.36.0-rc1 includes the changes to eliminate/pare\n> back reference transactions being raised independently for loose and\n> packed refs. It's a great improvement, and one we're very grateful\n> for.\n>\n> It looks like there's a regression, though. When deleting a ref that\n> _only_ exists in packed-refs, by the time the \"prepared\" callback is\n> raised the ref has already been deleted completely. Normally when\n> \"prepared\" is raised the ref is still present. The ref still being\n> present is important to us, since the reference-transaction hook is\n> frequently not passed any previous hash; we resolve the ref during\n> \"prepared\", if the previous hash is the null SHA1, so that we can\n> correctly note what the tip commit was when the ref was deleted.\n\nI've re-tested this with 2.36.0-rc2 and it has the same regression (as\nexpected). However, in playing with it more, the regression is more\nserious than I had initially considered. It goes beyond just losing\naccess to the SHA of the tip commit for deleted refs. If a ref only\nexists packed, this regression means vetoing the \"prepared\" callback\n_cannot prevent its deletion_, which violates the contract for the\nreference-transaction as I understand it.\n\nHere's a slightly modified reproduction:\ngit init ref-tx\ncd ref-tx\ngit commit -m \"Initial commit\" --allow-empty\ngit branch to-delete\ngit pack-refs --all\necho 'exit 1;' > .git/hooks/reference-transaction\nchmod +x .git/hooks/reference-transaction\ngit branch -d to-delete\n\nRunning this reproduction ends with:\n$ git branch -d to-delete\nfatal: ref updates aborted by hook\n$ git rev-parse to-delete --\nfatal: bad revision 'to-delete'\n\nEven though the reference-transaction vetoed \"prepared\", the ref was\nstill deleted.\n\nIn Bitbucket, we use the reference-transaction to perform replication.\nWhen we get the \"prepared\" callback on one machine, we dispatch the\nsame change(s) to other replicas. Those replicas process the changes\nand arrive at their own \"prepared\" callbacks (or don't), at which\npoint they vote to commit or rollback. The votes are tallied and the\nmajority decision wins.\n\nWith this regression, that sort of setup no longer works reliably for\nref deletions. If the ref only exists packed, it's deleted (and\n_visibly_ deleted) before we ever get the \"prepared\" callback. So if\ncoordination fails (i.e. the majority votes to rollback), even if we\ntry to abort the change it's already too late.\n\nBest regards,\nBryan Turner\n"},{"id":"453482","messageId":"220413.86r161f3qp.gmgdl@evledraar.gmail.com","threadId":"57714","inReplyTo":"CAGyf7-GaoBarXD2xKG3KUXwGZgbhKgv-4Mz45achbr6G9ihTBQ@mail.gmail.com","subject":"Re: reference-transaction regression in 2.36.0-rc1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-13T14:34:46Z","receivedAt":"2022-04-13T14:38:14Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 12 2022, Bryan Turner wrote:\n\n> On Tue, Apr 12, 2022 at 2:20 AM Bryan Turner <bturner@atlassian.com> wrote:\n>>\n>> It looks like Git 2.36.0-rc1 includes the changes to eliminate/pare\n>> back reference transactions being raised independently for loose and\n>> packed refs. It's a great improvement, and one we're very grateful\n>> for.\n>>\n>> It looks like there's a regression, though. When deleting a ref that\n>> _only_ exists in packed-refs, by the time the \"prepared\" callback is\n>> raised the ref has already been deleted completely. Normally when\n>> \"prepared\" is raised the ref is still present. The ref still being\n>> present is important to us, since the reference-transaction hook is\n>> frequently not passed any previous hash; we resolve the ref during\n>> \"prepared\", if the previous hash is the null SHA1, so that we can\n>> correctly note what the tip commit was when the ref was deleted.\n>\n> I've re-tested this with 2.36.0-rc2 and it has the same regression (as\n> expected). However, in playing with it more, the regression is more\n> serious than I had initially considered. It goes beyond just losing\n> access to the SHA of the tip commit for deleted refs. If a ref only\n> exists packed, this regression means vetoing the \"prepared\" callback\n> _cannot prevent its deletion_, which violates the contract for the\n> reference-transaction as I understand it.\n>\n> Here's a slightly modified reproduction:\n> git init ref-tx\n> cd ref-tx\n> git commit -m \"Initial commit\" --allow-empty\n> git branch to-delete\n> git pack-refs --all\n> echo 'exit 1;' > .git/hooks/reference-transaction\n> chmod +x .git/hooks/reference-transaction\n> git branch -d to-delete\n>\n> Running this reproduction ends with:\n> $ git branch -d to-delete\n> fatal: ref updates aborted by hook\n> $ git rev-parse to-delete --\n> fatal: bad revision 'to-delete'\n>\n> Even though the reference-transaction vetoed \"prepared\", the ref was\n> still deleted.\n>\n> In Bitbucket, we use the reference-transaction to perform replication.\n> When we get the \"prepared\" callback on one machine, we dispatch the\n> same change(s) to other replicas. Those replicas process the changes\n> and arrive at their own \"prepared\" callbacks (or don't), at which\n> point they vote to commit or rollback. The votes are tallied and the\n> majority decision wins.\n>\n> With this regression, that sort of setup no longer works reliably for\n> ref deletions. If the ref only exists packed, it's deleted (and\n> _visibly_ deleted) before we ever get the \"prepared\" callback. So if\n> coordination fails (i.e. the majority votes to rollback), even if we\n> try to abort the change it's already too late.\n\nThis does look lik a series regression.\n\nI haven't had time to bisect this, but I suspect that it'll come down to\nsomething in the 991b4d47f0a (Merge branch\n'ps/avoid-unnecessary-hook-invocation-with-packed-refs', 2022-02-18)\nseries.\n\nI happen to know that Patrick is OoO until after the final v2.36.0 is\nscheduled (but I don't know for sure that we won't spot this thread &\nact on it before then).\n\nIs this something you think you'll be able to dig into further and\npossibly come up with a patch? It looks like you're way ahead of at\nleast myself in knowing how this *should* work :)\n"},{"id":"453495","messageId":"d804fd56-ce41-f7c0-23bc-9e9d14f6b200@web.de","threadId":"57714","inReplyTo":"220413.86r161f3qp.gmgdl@evledraar.gmail.com","subject":"Re: reference-transaction regression in 2.36.0-rc1","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-04-13T16:21:47Z","receivedAt":"2022-04-13T16:22:04Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.04.22 um 16:34 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Tue, Apr 12 2022, Bryan Turner wrote:\n>\n>> On Tue, Apr 12, 2022 at 2:20 AM Bryan Turner <bturner@atlassian.com> wrote:\n>>>\n>>> It looks like Git 2.36.0-rc1 includes the changes to eliminate/pare\n>>> back reference transactions being raised independently for loose and\n>>> packed refs. It's a great improvement, and one we're very grateful\n>>> for.\n>>>\n>>> It looks like there's a regression, though. When deleting a ref that\n>>> _only_ exists in packed-refs, by the time the \"prepared\" callback is\n>>> raised the ref has already been deleted completely. Normally when\n>>> \"prepared\" is raised the ref is still present. The ref still being\n>>> present is important to us, since the reference-transaction hook is\n>>> frequently not passed any previous hash; we resolve the ref during\n>>> \"prepared\", if the previous hash is the null SHA1, so that we can\n>>> correctly note what the tip commit was when the ref was deleted.\n>>\n>> I've re-tested this with 2.36.0-rc2 and it has the same regression (as\n>> expected). However, in playing with it more, the regression is more\n>> serious than I had initially considered. It goes beyond just losing\n>> access to the SHA of the tip commit for deleted refs. If a ref only\n>> exists packed, this regression means vetoing the \"prepared\" callback\n>> _cannot prevent its deletion_, which violates the contract for the\n>> reference-transaction as I understand it.\n>>\n>> Here's a slightly modified reproduction:\n>> git init ref-tx\n>> cd ref-tx\n>> git commit -m \"Initial commit\" --allow-empty\n>> git branch to-delete\n>> git pack-refs --all\n>> echo 'exit 1;' > .git/hooks/reference-transaction\n>> chmod +x .git/hooks/reference-transaction\n>> git branch -d to-delete\n>>\n>> Running this reproduction ends with:\n>> $ git branch -d to-delete\n>> fatal: ref updates aborted by hook\n>> $ git rev-parse to-delete --\n>> fatal: bad revision 'to-delete'\n>>\n>> Even though the reference-transaction vetoed \"prepared\", the ref was\n>> still deleted.\n>>\n>> In Bitbucket, we use the reference-transaction to perform replication.\n>> When we get the \"prepared\" callback on one machine, we dispatch the\n>> same change(s) to other replicas. Those replicas process the changes\n>> and arrive at their own \"prepared\" callbacks (or don't), at which\n>> point they vote to commit or rollback. The votes are tallied and the\n>> majority decision wins.\n>>\n>> With this regression, that sort of setup no longer works reliably for\n>> ref deletions. If the ref only exists packed, it's deleted (and\n>> _visibly_ deleted) before we ever get the \"prepared\" callback. So if\n>> coordination fails (i.e. the majority votes to rollback), even if we\n>> try to abort the change it's already too late.\n>\n> This does look lik a series regression.\n>\n> I haven't had time to bisect this, but I suspect that it'll come down to\n> something in the 991b4d47f0a (Merge branch\n> 'ps/avoid-unnecessary-hook-invocation-with-packed-refs', 2022-02-18)\n> series.\n\nIndeed, it bisects to 2ed1b64ebd (refs: skip hooks when deleting\nuncovered packed refs, 2022-01-17).\n\n> I happen to know that Patrick is OoO until after the final v2.36.0 is\n> scheduled (but I don't know for sure that we won't spot this thread &\n> act on it before then).\n>\n> Is this something you think you'll be able to dig into further and\n> possibly come up with a patch? It looks like you're way ahead of at\n> least myself in knowing how this *should* work :)\n"},{"id":"453537","messageId":"xmqq4k2w92xo.fsf@gitster.g","threadId":"57714","inReplyTo":"220413.86r161f3qp.gmgdl@evledraar.gmail.com","subject":"Re: reference-transaction regression in 2.36.0-rc1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-13T19:52:03Z","receivedAt":"2022-04-13T19:52:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> This does look lik a series regression.\n\nYes.\n\n> I haven't had time to bisect this, but I suspect that it'll come down to\n> something in the 991b4d47f0a (Merge branch\n> 'ps/avoid-unnecessary-hook-invocation-with-packed-refs', 2022-02-18)\n> series.\n\nWith the issue that /var/tmp/.git can cause trouble to those who\nwork in /var/tmp/$USER/playpen being taken reasonably good care of\nalready, it seems this is the issue with the highest priority at\nthis point.\n\n> I happen to know that Patrick is OoO until after the final v2.36.0 is\n> scheduled (but I don't know for sure that we won't spot this thread &\n> act on it before then).\n>\n> Is this something you think you'll be able to dig into further and\n> possibly come up with a patch? It looks like you're way ahead of at\n> least myself in knowing how this *should* work :)\n\nThanks.\n"},{"id":"453609","messageId":"xmqqilrc61te.fsf@gitster.g","threadId":"57714","inReplyTo":"xmqq4k2w92xo.fsf@gitster.g","subject":"Re: reference-transaction regression in 2.36.0-rc1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-13T22:44:29Z","receivedAt":"2022-04-13T22:44:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>> This does look lik a series regression.\n>\n> Yes.\n>\n>> I haven't had time to bisect this, but I suspect that it'll come down to\n>> something in the 991b4d47f0a (Merge branch\n>> 'ps/avoid-unnecessary-hook-invocation-with-packed-refs', 2022-02-18)\n>> series.\n>\n> With the issue that /var/tmp/.git can cause trouble to those who\n> work in /var/tmp/$USER/playpen being taken reasonably good care of\n> already, it seems this is the issue with the highest priority at\n> this point.\n>\n>> I happen to know that Patrick is OoO until after the final v2.36.0 is\n>> scheduled (but I don't know for sure that we won't spot this thread &\n>> act on it before then).\n\nReverting the merge 991b4d47f0a as a whole is an option.  It might\nbe the safest thing to do, if we do not to want to extend the cycle\nand add a few more -rc releases before the final.\n\nThanks.\n"},{"id":"453610","messageId":"CAGyf7-FGdciG+QF8NA_yi=o4B0RVPNiZM4LJZ610cDZzd3AjyQ@mail.gmail.com","threadId":"57714","inReplyTo":"xmqqilrc61te.fsf@gitster.g","subject":"Re: reference-transaction regression in 2.36.0-rc1","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2022-04-13T22:55:51Z","receivedAt":"2022-04-13T22:56:06Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Wed, Apr 13, 2022 at 3:48 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n> >\n> >> This does look lik a series regression.\n> >\n> > Yes.\n> >\n> >> I haven't had time to bisect this, but I suspect that it'll come down to\n> >> something in the 991b4d47f0a (Merge branch\n> >> 'ps/avoid-unnecessary-hook-invocation-with-packed-refs', 2022-02-18)\n> >> series.\n> >\n> > With the issue that /var/tmp/.git can cause trouble to those who\n> > work in /var/tmp/$USER/playpen being taken reasonably good care of\n> > already, it seems this is the issue with the highest priority at\n> > this point.\n> >\n> >> I happen to know that Patrick is OoO until after the final v2.36.0 is\n> >> scheduled (but I don't know for sure that we won't spot this thread &\n> >> act on it before then).\n>\n> Reverting the merge 991b4d47f0a as a whole is an option.  It might\n> be the safest thing to do, if we do not to want to extend the cycle\n> and add a few more -rc releases before the final.\n\nI'm reviewing the changes in the patch series, but while I understand\nthe visible behavior fairly well the internals of it aren't something\nI've spent a lot of time in. Compounding that, I'm down to my final 3\ndays with Atlassian before I start a new job, so I'm juggling trying\nto finish up all my work and/or hand things over to others.\n\nBased on that, I definitely would not want work on 2.36 to hinge on me\ngetting a patch up. I would _love_ to (finally) contribute something\nback to Git, and the community that has helped me so much over the\nlast 10 years, but I can't say exactly when I'd be able to post\nsomething. There's another developer internally who is also looking at\nthis, but it's been a number of years since he worked in C.\n\nI guess all that's a long-winded way of saying a revert might be the\nmore timely option, for decoupling resolving this issue from the 2.36\ntimeline. That would also mean Patrick would be able to review any fix\nthat was posted, which seems valuable since he did the original work.\n\n>\n> Thanks.\n"},{"id":"453611","messageId":"xmqqbkx4619t.fsf@gitster.g","threadId":"57714","inReplyTo":"xmqqilrc61te.fsf@gitster.g","subject":"Re: reference-transaction regression in 2.36.0-rc1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-13T22:56:14Z","receivedAt":"2022-04-13T22:56:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Reverting the merge 991b4d47f0a as a whole is an option.  It might\n> be the safest thing to do, if we do not to want to extend the cycle\n> and add a few more -rc releases before the final.\n\nIt turns out that this involves nontrivial amount of work to get\nright, as the bottom commit of Patrick's \"fetch --atomic\" series\nwants to count how many transaction we make, instead of ensuring\nthat the updates to the references are all-or-none, so at least\n2a0cafd4 (fetch: increase test coverage of fetches, 2022-02-17)\nneeds to be reverted as well.  This in turn is made unnecessarily\nmore cumbersome as the history in the t/ directory is littered with\nunrelated \"clean-up\" patches since these topics were merged.\n\n"},{"id":"453612","messageId":"xmqq7d7s5zno.fsf@gitster.g","threadId":"57714","inReplyTo":"xmqqbkx4619t.fsf@gitster.g","subject":"Re: reference-transaction regression in 2.36.0-rc1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-13T23:31:07Z","receivedAt":"2022-04-13T23:31:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Reverting the merge 991b4d47f0a as a whole is an option.  It might\n>> be the safest thing to do, if we do not to want to extend the cycle\n>> and add a few more -rc releases before the final.\n>\n> It turns out that this involves nontrivial amount of work to get\n> right, as the bottom commit of Patrick's \"fetch --atomic\" series\n> wants to count how many transaction we make, instead of ensuring\n> that the updates to the references are all-or-none, so at least\n> 2a0cafd4 (fetch: increase test coverage of fetches, 2022-02-17)\n> needs to be reverted as well.  This in turn is made unnecessarily\n> more cumbersome as the history in the t/ directory is littered with\n> unrelated \"clean-up\" patches since these topics were merged.\n\nI have a tentative revert of the merge and also the \"fetch --atomic\"\ntest that (unnecessarily) counted the number of transactions directly\non top of planned 'master', which merges the planned v2.35.3 into\nv2.36-rc2 and pushed it as if it were the 'seen' branch.\n\nThe CI job https://github.com/git/git/actions/runs/2164208587 seems\nto be doing well so far, so once the dust from releasing v2.30.4,\nv2.31.3, v2.32.2, v2.33.3, v2.34.3, and v2.35.3 with Derrick's\nhotfix for yesterday's CVE fix, I may merge it down to 'master'\nbefore the final.\n\nThanks.\n\n\n"},{"id":"453678","messageId":"CAGyf7-EU4aBO5JGfDAK+rkrjMwmDjZdCBeXBh_=z0Cqv=KnCkg@mail.gmail.com","threadId":"57714","inReplyTo":"xmqq4k2w92xo.fsf@gitster.g","subject":"Re: reference-transaction regression in 2.36.0-rc1","fromName":"Bryan Turner","fromEmail":"bturner@atlassian.com","sentAt":"2022-04-15T02:37:04Z","receivedAt":"2022-04-15T02:37:19Z","isPatch":false,"sender":{"key":"bturner@atlassian.com","avatar":"https://gravatar.com/avatar/16bcf3167981c1ef7c804e502642366d888a35b0d0b0a4ca01fdc442aa1acb1e?d=mp&s=160"},"body":"On Wed, Apr 13, 2022 at 12:52 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n> > This does look lik a series regression.\n>\n> Yes.\n>\n> > I haven't had time to bisect this, but I suspect that it'll come down to\n> > something in the 991b4d47f0a (Merge branch\n> > 'ps/avoid-unnecessary-hook-invocation-with-packed-refs', 2022-02-18)\n> > series.\n>\n> With the issue that /var/tmp/.git can cause trouble to those who\n> work in /var/tmp/$USER/playpen being taken reasonably good care of\n> already, it seems this is the issue with the highest priority at\n> this point.\n>\n> > I happen to know that Patrick is OoO until after the final v2.36.0 is\n> > scheduled (but I don't know for sure that we won't spot this thread &\n> > act on it before then).\n> >\n> > Is this something you think you'll be able to dig into further and\n> > possibly come up with a patch? It looks like you're way ahead of at\n> > least myself in knowing how this *should* work :)\n>\n> Thanks.\n\nOne of my colleagues was able to spend some further time investigating\nthis. He also experimented with commands other than \"git branch -d\"\nand found that \"git update-ref -d\", for example does not exhibit the\nissue, but \"git tag -d\" does. Storing a replay of the various\nreference-transaction callbacks shows that for every command _other\nthan_ \"git branch -d\" and \"git tag -d\", pre-2.36 reference\ntransactions follow the pattern \"prepared\", \"prepared\", \"committed\",\n\"committed\". \"git branch -d\" and \"git tag -d\", on the other hand, go\n\"prepared\", \"committed\", \"prepared\", \"committed\". This implies the\nreference-transaction behavior is _already broken_ in 2.35, and the\nchanges in 2.36 to suppress packed callbacks just make it more\nobvious.\n\nFor reference, here are the reference-transactions for _2.35_ using\n\"git branch -d\" and \"git update-ref -d\". In both runs, the ref only\nexists packed--there is no loose ref to delete. My\nreference-transaction script is simple:\n$ cat .git/hooks/reference-transaction\necho \"-- $1\"\ncat\ngit rev-parse refs/heads/to-delete --\nexit 0\n\n $ git-2.35.1 branch -d to-delete\n-- prepared\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\n1767344659fd3f7a6b788020203f74baeea0e0f7\n--\n-- committed\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\n-- aborted\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\n-- prepared\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\n-- committed\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\nDeleted branch to-delete (was 1767344).\n\n$ git-2.35.1 update-ref -d refs/heads/to-delete\n-- prepared\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\n1767344659fd3f7a6b788020203f74baeea0e0f7\n--\n-- prepared\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\n1767344659fd3f7a6b788020203f74baeea0e0f7\n--\n-- committed\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\n-- committed\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\n\nNow, the same two scenarios in 2.36.0-rc2:\n\n$ git-2.36.0-rc2 branch -d to-delete\n-- prepared\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\n-- committed\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\nDeleted branch to-delete (was 1767344).\n\n$ git-2.36.0-rc2 update-ref -d refs/heads/to-delete\n-- prepared\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\n1767344659fd3f7a6b788020203f74baeea0e0f7\n--\n-- committed\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\n\nLooking at the data this way makes it more obvious that the issue\nisn't actually new in 2.36; even in 2.35, the branch is fully deleted\nbefore the _loose_ transaction is prepared, with \"git branch -d\".\nSince \"git update-ref -d\" prepares both transactions before committing\neither, the branch still exists in both \"prepared\" callbacks. The real\ndifference here, between 2.35 and 2.36, is that Bitbucket Server (and,\nostensibly, other reference-transaction users), with enough checking,\ncan essentially pick-and-choose between \"prepared\" and \"committed\"\ncallbacks to cobble together a working pairing: For \"git branch -d\",\nwe replicate on the first \"prepared\" and wrap up replication on the\n_second_ \"committed\", and for \"git update-ref -d\" we replicate on the\n_second_ \"prepared\" and wrap up on the _second_ \"committed\". Since the\npacked callbacks are gone in 2.36, that's not possible.\n\nThe attached patch on top of the v2.36.0-rc2 tag fixes the issue, and\nall of the t14xx tests (including t1416) pass (aside from known\nissues). (Apologies for attaching the patch rather than inlining it;\nthe Gmail editor butchers it if I try that.) With the patch applied,\nthe reference-transaction callbacks are identical to 2.36.0-rc2, in\nterms of when they run, but the state of the ref is different:\n\n$ git-patched branch -d to-delete\n-- prepared\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\n1767344659fd3f7a6b788020203f74baeea0e0f7\n--\n-- committed\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\nDeleted branch to-delete (was 1767344).\n\n$ git-patched update-ref -d refs/heads/to-delete\n-- prepared\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\n1767344659fd3f7a6b788020203f74baeea0e0f7\n--\n-- committed\n0000000000000000000000000000000000000000\n0000000000000000000000000000000000000000 refs/heads/to-delete\nfatal: bad revision 'refs/heads/to-delete'\n\nNow \"git branch -d\" and \"git update-ref -d\" have the same callbacks,\nand see the same ref state in each.\n\nI'm happy to try and format this into a proper patch, with sign-offs\nand new tests, if folks think this is the way to go. If the 2.36\nrelease cycle is too far along, I can still try and get a patch to the\nlist for inclusion in 2.37 (though I'm less confident what commit I'd\nbase the patch on in that scenario). Any pointers would be\nappreciated. (Or, if someone with more familiarity than me, not to\nmention a patch submission setup that already works, wants to take\nover, that's fine too.)\n\nBest regards,\nBryan Turner\n\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 95acab78ee..c11abef186 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1273,29 +1273,12 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,\n \tif (!refnames->nr)\n \t\treturn 0;\n \n-\tif (packed_refs_lock(refs->packed_ref_store, 0, &err))\n-\t\tgoto error;\n-\n-\ttransaction = ref_store_transaction_begin(refs->packed_ref_store,\n-\t\t\t\t\t\t  REF_TRANSACTION_SKIP_HOOK, &err);\n-\tif (!transaction)\n-\t\tgoto error;\n-\n-\tresult = packed_refs_delete_refs(refs->packed_ref_store,\n-\t\t\t\t\t transaction, msg, refnames, flags);\n-\tif (result)\n-\t\tgoto error;\n-\n-\tpacked_refs_unlock(refs->packed_ref_store);\n-\n \tfor (i = 0; i < refnames->nr; i++) {\n \t\tconst char *refname = refnames->items[i].string;\n \n-\t\tif (refs_delete_ref(&refs->base, msg, refname, NULL, flags))\n+\t\tif (refs_delete_ref(ref_store, msg, refname, NULL, flags))\n \t\t\tresult |= error(_(\"could not remove reference %s\"), refname);\n \t}\n-\n-\tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n \treturn result;\n \n"},{"id":"453699","messageId":"220415.86sfqebhl6.gmgdl@evledraar.gmail.com","threadId":"57714","inReplyTo":"CAGyf7-EU4aBO5JGfDAK+rkrjMwmDjZdCBeXBh_=z0Cqv=KnCkg@mail.gmail.com","subject":"Re: reference-transaction regression in 2.36.0-rc1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-15T13:27:26Z","receivedAt":"2022-04-15T13:29:33Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Apr 14 2022, Bryan Turner wrote:\n\n> I'm happy to try and format this into a proper patch, with sign-offs\n> and new tests, if folks think this is the way to go. If the 2.36\n> release cycle is too far along, I can still try and get a patch to the\n> list for inclusion in 2.37 (though I'm less confident what commit I'd\n> base the patch on in that scenario). Any pointers would be\n> appreciated. (Or, if someone with more familiarity than me, not to\n> mention a patch submission setup that already works, wants to take\n> over, that's fine too.)\n\nI think that would be great if you can spend the time, but Junio's\nalready pushed out a revert of the topic for the final 2.36.0, can you\nconfirm that current \"master\" solves the issues you had?\n\nSo revert/retry & extra tests etc. can wait until the post-release, and\nlikely Patrick will be back to work on it at that time...\n"},{"id":"453714","messageId":"xmqqr15ye19j.fsf@gitster.g","threadId":"57714","inReplyTo":"220415.86sfqebhl6.gmgdl@evledraar.gmail.com","subject":"Re: reference-transaction regression in 2.36.0-rc1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-15T16:53:44Z","receivedAt":"2022-04-15T16:53:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> I think that would be great if you can spend the time, but Junio's\n> already pushed out a revert of the topic for the final 2.36.0, can you\n> confirm that current \"master\" solves the issues you had?\n\nThanks for asking this question, which is lot more urgent and I\nshould have asked last night before I went to bed.  Yes, Bryan\nalready said that the revert in 'seen' the previous day made their\nsystem happier, but it certainly helps to know the reverts were OK\nat the tip of 'master' without all the other random topics.\n\n> So revert/retry & extra tests etc. can wait until the post-release, and\n> likely Patrick will be back to work on it at that time...\n\nYup.\n"},{"id":"454492","messageId":"YmkjfyB3gIQ2t45V@ncase","threadId":"57714","inReplyTo":"CAJDSCnOUQm-doY-xTobPk64oeiSsTd+vSFzsb_cz9BLORAxXGA@mail.gmail.com","subject":"Re: reference-transaction regression in 2.36.0-rc1","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-04-27T11:05:35Z","receivedAt":"2022-04-27T11:06:54Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Thanks a lot to all of you for handling this regression while I was out\nof office!\n\nOn Tue, Apr 19, 2022 at 11:54:19AM +0200, Michael Heemskerk wrote:\n> Apologies for the late reply. Bryan had his last day at Atlassian last\n> week, so I'll take over from him on this topic.\n> \n> Thanks for asking this question, which is lot more urgent and I\n> > should have asked last night before I went to bed.  Yes, Bryan\n> > already said that the revert in 'seen' the previous day made their\n> > system happier, but it certainly helps to know the reverts were OK\n> > at the tip of 'master' without all the other random topics.\n> >\n> \n> I have run all our tests on the tip of master prior to 2.36 being released,\n>  and on the released 2.36. Our tests are happy on both, so thanks for\n>  reverting those reference-transaction changes for 2.36.\n> \n> I'd be happy to provide the fix to files_delete_refs in a patch (including\n> some extra tests) on top of Patrick's\n> avoid-unnecessary-hook-invocation-with-packed-refs series if and\n> when that series is unreverted on 'next'.\n\nThat would be great. To be clear, do the fixes you have also fix the\npre-v2.36.0 issues Bryan has mentioned?\n\nPlease let me know whether you want to pursue this and whether you need\nany further help with it.\n\nPatrick\n"},{"id":"454713","messageId":"Ym+8iCCy5TlfmObq@ncase","threadId":"57714","inReplyTo":"CAJDSCnM767fdo6qy085jc9sqezvbBqDD4ikXz1y5tHEjSYED2A@mail.gmail.com","subject":"Re: reference-transaction regression in 2.36.0-rc1","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-05-02T11:12:08Z","receivedAt":"2022-05-02T11:12:18Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 02, 2022 at 11:41:26AM +0200, Michael Heemskerk wrote:\n> > > I'd be happy to provide the fix to files_delete_refs in a patch\n> > (including\n> > > some extra tests) on top of Patrick's\n> > > avoid-unnecessary-hook-invocation-with-packed-refs series if and\n> > > when that series is unreverted on 'next'.\n> >\n> > That would be great. To be clear, do the fixes you have also fix the\n> > pre-v2.36.0 issues Bryan has mentioned?\n> >\n> \n> That is correct. The pre-v2.36.0 issues are caused by files_delete_refs\n> first\n> preparing and committing a transaction for the packed_ref_store, and then\n> iterating over each of the to-be-deleted refs and preparing and committing\n> another transaction per ref. This latter transaction triggers the usual\n> ABORTED (from packed_ref_store), PREPARED (ref_store),\n> COMMITTED (ref_store) callbacks.\n> \n> As far as I can tell, all we need to do is remove the separate transaction\n> for\n> the packed_ref_store.\n\nDo you mean that we should just get rid of the transaction and instead\ndelete the refs in the packet-refs backend one by one? If so, then I\nthink that would be a performance regressions in a lot of places. If you\nare deleting a bunch of references at the same time, then you do indeed\nwant to make this a single packed-refs transaction or otherwise we'd\nrepeatedly rewrite the whole file. And given that this file can easily\nrange in the hundreds of megabytes this can easily go into pathological\ncases.\n\nI think what we need to do instead is a bit more complicated:\n\n    1. Lock the packed-refs backend. This is to ensure that it's not\n       being updated while we update loose refs.\n\n    2. Create a transaction for the loose-refs and queue all refs that\n       we are about to delete. Now we're safe to prepate the loose refs,\n       and at this point in time nothing has been pruned yet. As a\n       result, it is still possible for the reference-transaction hook\n       to intervene even if the refs only existed as packed-refs file.\n\n    3. Now we have to prepare and commit the packed-refs file. This is\n       to avoid a race where deleting loose refs would uncover anything\n       that existed in the packed-refs file already.\n\n    4. Unlock the packed-refs backend.\n\n    5. Commit the loose-refs transaction we have prepared already.\n\nThis should be race-free and fix the usecase for aborting ref deletions\nvia the reference-transaction hook. It has the downside though that\npacked-refs are locked for far longer than they had been before because\nthe lock also spans over preparation of the loose-refs transaction.\n\n> Another thing we need to decide is whether or not to backport the fix to\n> 2.36 and possibly older. My suggestion would be to only apply the fix to\n> 'next' and NOT backport it to older git versions as changing the\n> reference-transaction semantics in a patch release would be unexpected.\n\nThe way it sounds like this has been a longer-standing issue.\nFurthermore, it's not a regression: it just hasn't ever been working. So\nI don't feel like it's critical to backport this.\n\n> > Please let me know whether you want to pursue this and whether you need\n> > any further help with it.\n> >\n> \n> I'm happy to provide the patch, but assume that I need to wait for Patrick's\n> series to be reapplied to 'next'? Alternatively, I can send the patch to\n> Patrick\n> to be included in a reroll of the series.\n> \n> Michael\n\nGiven that you said that my patch series made the preexisting problem\nworse I'd prefer to first land a proper fix for this issue, and only\nthen reapply my own patches on top. I'm also happy to fix the issue\nmyself -- at GitLab, we also care about this working correctly. Just let\nme know your preference.\n\nPatrick\n"}]}