git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3 2/2] receive-pack: handle reference deletions separately

From
Karthik Nayak <karthik.188@gmail.com>
Date
Jun 6, 2025, 08:41 UTC
Message-ID
<20250606-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v3-2-e1c41693bd35@gmail.com>
In-Reply-To
<20250606-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v3-0-e1c41693bd35@gmail.com>

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.

This means 'git-receive-pack(1)' will now use upto two transactions, whereas before using batched updates it would use _at least_ two transactions. So using batched updates is still the better option.

Add a test to validate this behavior.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/receive-pack.c | 100 ++++++++++++++++++++++++++++++++-----------------
 t/t5516-fetch-push.sh  |  17 +++++++--
 2 files changed, 79 insertions(+), 38 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9e3cfb85cf..8ee792d2f8 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1867,47 +1867,79 @@ static void execute_commands_non_atomic(struct command *commands,
 	const char *reported_error = NULL;
 	struct strmap failed_refs = STRMAP_INIT;
 
-	transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
-						  REF_TRANSACTION_ALLOW_FAILURE, &err);
-	if (!transaction) {
-		rp_error("%s", err.buf);
-		strbuf_reset(&err);
-		reported_error = "transaction failed to start";
-		goto failure;
-	}
+	/*
+	 * Reference updates, where F/D conflicts shouldn't arise due to
+	 * one reference being deleted, while the other being created
+	 * are treated as conflicts in batched updates. This is because
+	 * we don't do conflict resolution inside a transaction. To
+	 * mitigate this, delete references in a separate batch.
+	 */
+	enum processing_phase {
+		PHASE_DELETIONS,
+		PHASE_OTHERS
+	};
 
-	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
-			continue;
+	for (enum processing_phase phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {
+		for (cmd = commands; cmd; cmd = cmd->next) {
+			if (!should_process_cmd(cmd) || cmd->run_proc_receive)
+				continue;
 
-		cmd->error_string = update(cmd, si);
-	}
+			if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))
+				continue;
+			else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))
+				continue;
 
-	if (ref_transaction_commit(transaction, &err)) {
-		rp_error("%s", err.buf);
-		reported_error = "failed to update refs";
-		goto failure;
-	}
+			/*
+			 * Lazily create a transaction only when we know there are
+			 * updates to be added.
+			 */
+			if (!transaction) {
+				transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
+									  REF_TRANSACTION_ALLOW_FAILURE, &err);
+				if (!transaction) {
+					rp_error("%s", err.buf);
+					strbuf_reset(&err);
+					reported_error = "transaction failed to s1tart";
+					goto failure;
+				}
+			}
 
-	ref_transaction_for_each_rejected_update(transaction,
-						 ref_transaction_rejection_handler,
-						 &failed_refs);
+			cmd->error_string = update(cmd, si);
+		}
 
-	if (strmap_empty(&failed_refs))
-		goto cleanup;
+		/*
+		 * If no transaction was created, there is nothing to commit.
+		 */
+		if (!transaction)
+			goto cleanup;
 
-failure:
-	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (reported_error)
-			cmd->error_string = reported_error;
-		else if (strmap_contains(&failed_refs, cmd->ref_name))
-			cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
-	}
+		if (ref_transaction_commit(transaction, &err)) {
+			rp_error("%s", err.buf);
+			reported_error = "failed to update refs";
+			goto failure;
+		}
 
-cleanup:
-	ref_transaction_free(transaction);
-	strmap_clear(&failed_refs, 0);
-	strbuf_release(&err);
+		ref_transaction_for_each_rejected_update(transaction,
+							 ref_transaction_rejection_handler,
+							 &failed_refs);
+
+		if (strmap_empty(&failed_refs))
+			goto cleanup;
+
+	failure:
+		for (cmd = commands; cmd; cmd = cmd->next) {
+			if (reported_error)
+				cmd->error_string = reported_error;
+			else if (strmap_contains(&failed_refs, cmd->ref_name))
+				cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
+		}
+
+	cleanup:
+		ref_transaction_free(transaction);
+		transaction = NULL;
+		strmap_clear(&failed_refs, 0);
+		strbuf_release(&err);
+	}
 }
 
 static void execute_commands_atomic(struct command *commands,
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index dabcc5f811..1649667441 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 &&
@@ -808,8 +808,8 @@ test_expect_success 'deletion of a non-existent ref is not fed to post-receive a
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/nonexistent $ZERO_OID $ZERO_OID
+		refs/heads/main $orgmain $newmain
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -868,10 +868,10 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/next $orgnext $newnext
-		refs/heads/seen $orgseen $newseen
 		refs/heads/nonexistent $ZERO_OID $ZERO_OID
+		refs/heads/main $orgmain $newmain
+		refs/heads/seen $orgseen $newseen
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -1909,4 +1909,13 @@ test_expect_success 'push with config push.useBitmaps' '
 		--thin --delta-base-offset -q --no-use-bitmap-index <false
 '
 
+test_expect_success 'push with F/D conflict with deletion and creation' '
+	test_when_finished "git branch -D branch" &&
+	git branch branch/conflict &&
+	mk_test testrepo heads/branch/conflict &&
+	git branch -D branch/conflict &&
+	git branch branch &&
+	git push testrepo :refs/heads/branch/conflict refs/heads/branch
+'
+
 test_done

-- 
2.49.0
Previous: Karthik NayakNext: Christian Couder
Message 23 of 40 in “refs: fix some bugs with batched-updates”
  1. 0/3 refs: fix some bugs with batched-updatesKarthik Nayak, Jun 2, 2025
  2. 1/3 refs/files: skip updates with errors in batched updatesKarthik Nayak, Jun 2, 2025
  3. Patrick SteinhardtJun 2, 2025
  4. Karthik NayakJun 2, 2025
  5. Junio C HamanoJun 2, 2025
  6. Karthik NayakJun 2, 2025
  7. 2/3 t5516: use double quotes for tests with variablesKarthik Nayak, Jun 2, 2025
  8. Eric SunshineJun 2, 2025
  9. Karthik NayakJun 2, 2025
  10. 3/3 receive-pack: handle reference deletions separatelyKarthik Nayak, Jun 2, 2025
  11. Patrick SteinhardtJun 2, 2025
  12. Karthik NayakJun 2, 2025
  13. Junio C HamanoJun 2, 2025
  14. Patrick SteinhardtJun 2, 2025
  15. Karthik NayakJun 4, 2025
  16. 0/2 refs: fix some bugs with batched-updatesKarthik Nayak, Jun 5, 2025
  17. 1/2 refs/files: skip updates with errors in batched updatesKarthik Nayak, Jun 5, 2025
  18. 2/2 receive-pack: handle reference deletions separatelyKarthik Nayak, Jun 5, 2025
  19. Patrick SteinhardtJun 5, 2025
  20. Karthik NayakJun 5, 2025
  21. 0/2 refs: fix some bugs with batched-updatesKarthik Nayak, Jun 6, 2025
  22. 1/2 refs/files: skip updates with errors in batched updatesKarthik Nayak, Jun 6, 2025
  23. 2/2 receive-pack: handle reference deletions separatelyKarthik Nayak, Jun 6, 2025
  24. Christian CouderJun 12, 2025
  25. Junio C HamanoJun 12, 2025
  26. Karthik NayakJun 13, 2025
  27. 0/2 refs: fix some bugs with batched-updatesKarthik Nayak, Jun 13, 2025
  28. 1/2 refs/files: skip updates with errors in batched updatesKarthik Nayak, Jun 13, 2025
  29. 2/2 receive-pack: handle reference deletions separatelyKarthik Nayak, Jun 13, 2025
  30. Junio C HamanoJun 13, 2025
  31. Karthik NayakJun 19, 2025
  32. Christian CouderJun 13, 2025
  33. Junio C HamanoJun 13, 2025
  34. 0/2 refs: fix some bugs with batched-updatesKarthik Nayak, Jun 20, 2025
  35. 1/2 refs/files: skip updates with errors in batched updatesKarthik Nayak, Jun 20, 2025
  36. 2/2 receive-pack: handle reference deletions separatelyKarthik Nayak, Jun 20, 2025
  37. Junio C HamanoJun 20, 2025
  38. Karthik NayakJun 21, 2025
  39. Junio C HamanoJun 22, 2025
  40. Karthik NayakJun 22, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.