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 exactly two transactions,
whereas before using batched updates it would use _at least_ two
transactions. So using batched updates is the still the better option.
Add a test to validate this behavior.
Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
---
builtin/receive-pack.c | 90 +++++++++++++++++++++++++---------------
t/t1416-ref-transaction-hooks.sh | 2 +
t/t5516-fetch-push.sh | 17 ++++++--
3 files changed, 71 insertions(+), 38 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9e3cfb85cf..34db4377ca 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1867,47 +1867,66 @@ 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 (int phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {
+ 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;
+ }
- cmd->error_string = update(cmd, si);
- }
+ for (cmd = commands; cmd; cmd = cmd->next) {
+ if (!should_process_cmd(cmd) || cmd->run_proc_receive)
+ continue;
- if (ref_transaction_commit(transaction, &err)) {
- rp_error("%s", err.buf);
- reported_error = "failed to update refs";
- goto failure;
- }
+ if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))
+ continue;
+ else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))
+ continue;
- 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 (ref_transaction_commit(transaction, &err)) {
+ rp_error("%s", err.buf);
+ reported_error = "failed to update refs";
+ goto failure;
+ }
-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);
- }
+ ref_transaction_for_each_rejected_update(transaction,
+ ref_transaction_rejection_handler,
+ &failed_refs);
-cleanup:
- ref_transaction_free(transaction);
- strmap_clear(&failed_refs, 0);
- strbuf_release(&err);
+ 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);
+ strmap_clear(&failed_refs, 0);
+ strbuf_release(&err);
+ }
}
static void execute_commands_atomic(struct command *commands,
@@ -2024,6 +2043,9 @@ static void execute_commands(struct command *commands,
/*
* If there is no command ready to run, should return directly to destroy
* temporary data in the quarantine area.
+ *
+ * Check if any reference deletions exist, these are batched together in
+ * a separate transaction to avoid F/D conflicts with other updates.
*/
for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)
; /* nothing */
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index d91dd3a3b5..b2aaa1908f 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -119,6 +119,8 @@ test_expect_success 'interleaving hook calls succeed' '
EOF
cat >expect <<-EOF &&
+ hooks/reference-transaction prepared
+ hooks/reference-transaction committed
hooks/update refs/tags/PRE $ZERO_OID $PRE_OID
hooks/update refs/tags/POST $ZERO_OID $POST_OID
hooks/reference-transaction prepared
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
