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.
Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
---
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
@@ -1860,7 +1860,8 @@ static void ref_transaction_rejection_handler(const char *refname,
}
static void execute_commands_non_atomic(struct command *commands,
- struct shallow_info *si)
+ struct shallow_info *si,
+ int only_deletions)
{
struct command *cmd;
struct strbuf err = STRBUF_INIT;
@@ -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);
}
@@ -2024,6 +2027,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 */
@@ -2058,10 +2064,19 @@ static void execute_commands(struct command *commands,
(cmd->run_proc_receive || use_atomic))
cmd->error_string = "fail to run proc-receive hook";
- if (use_atomic)
+ if (use_atomic) {
execute_commands_atomic(commands, si);
- else
- execute_commands_non_atomic(commands, si);
+ } else {
+ /*
+ * 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.
+ */
+ execute_commands_non_atomic(commands, si, 1);
+ execute_commands_non_atomic(commands, si, 0);
+ }
if (shallow_update)
BUG_if_skipped_connectivity_check(commands, si);
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 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 &&
@@ -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
