[PATCH 3/3] receive-pack: handle reference deletions separately
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jun 2, 2025, 09:57 UTC
- Message-ID
- <20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-3-903d1db3f10e@gmail.com>
- In-Reply-To
- <20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-0-903d1db3f10e@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.
Add a test to validate this behavior.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- 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