Re: reference-transaction regression in 2.36.0-rc1

[Bryan Turner <bturner@atlassian.com>]

[-- 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;