Re: [PATCH 3/3] receive-pack: handle reference deletions separately

Patrick Steinhardt [off-list ref] writes:

On Mon, Jun 02, 2025 at 11:57:26AM +0200, Karthik Nayak wrote:

quoted

In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
we updated the 'git-receive-pack(1)' command to use batched reference
updates. One edge case which was missed during this implementation was
when a user pushes multiple branches such as:

  delete refs/heads/branch/conflict
  create refs/heads/branch

Before using batched updates, the references would be applied
sequentially and hence no conflicts would arise. With batched updates,
while the first update applies, the second fails due to F/D conflict. A
similar issue was present in 'git-fetch(1)' and was fixed by using
separating out reference pruning into a separate transaction. Apply a
similar mechanism for 'git-receive-pack(1)' and separate out reference
deletions into its own batch.

Add a test to validate this behavior.

Okay. All of this is unfortunate as ideally the reference transaction
itself would know to resolve such conflicts. But we're no worse off than
before because we at most perform exactly two transactions now, whereas
before we would have performed _at least_ two transactions in this
conflicting case.

Exactly. It is not easy to build in conflict resolution but it would be
great to have it. Also to some extent F/D conflicts are due to
implementation details. With the reftable backend, we artificially
introduce this limitation, maybe someday if we get rid of the files
backend, we won't even have this issue.

Thanks for spelling this out, I think it would be worthwhile to add this
to the commit message too. I will do that.

quoted

Signed-off-by: Karthik Nayak <redacted>
---
 builtin/receive-pack.c           | 23 +++++++++++++++++++----
 t/t1416-ref-transaction-hooks.sh |  2 ++
 t/t5516-fetch-push.sh            | 17 +++++++++++++----
 3 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9e3cfb85cf..7157ced2a6 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c

@@ -1879,6 +1880,8 @@ static void execute_commands_non_atomic(struct command *commands,
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
 			continue;
+		if (only_deletions ^ is_null_oid(&cmd->new_oid))
+			continue;

 		cmd->error_string = update(cmd, si);
 	}

Fancy.

quoted

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 029ef92d58..34eb3a5a07 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh

@@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
 		EOF

 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/next $orgnext $newnext
+		refs/heads/main $orgmain $newmain
 		EOF

 		cat >post-receive.expect <<-EOF &&

Hm, so the ordering does change now as all deletes will now be listed
before the updates. We don't make any guarantees about how these are
sorted, but it makes me a bit uneasy to see this change. Can we avoid
this change in behaviour somehow?

Yeah it does. I couldn't think of a way thought to retain this behavior.
But I will spend some time on it.

Patrick