Re: reference-transaction regression in 2.36.0-rc1
On Wed, Apr 13, 2022 at 12:52 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 26 quoted lines
>
> Æ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
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;
Bryan Turner
