diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 01e5601342..5bf5d1fcac 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1866,47 +1866,81 @@ 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 D/F 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. + * + * NEEDSWORK: Add conflict resolution between deletion and creation + * of reference updates within a transaction. With that, we can + * combine the two phases. + */ + enum processing_phase { + PHASE_DELETIONS, + PHASE_OTHERS + }; + + 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; + + if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid)) + continue; + else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid)) + continue; + + /* + * 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 start"; + goto failure; + } + } + + cmd->error_string = update(cmd, si); + } + + /* No transaction, so nothing to commit */ + if (!transaction) + goto cleanup; + + if (ref_transaction_commit(transaction, &err)) { + rp_error("%s", err.buf); + reported_error = "failed to update refs"; + goto failure; + } + + 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); } - - for (cmd = commands; cmd; cmd = cmd->next) { - if (!should_process_cmd(cmd) || cmd->run_proc_receive) - continue; - - cmd->error_string = update(cmd, si); - } - - if (ref_transaction_commit(transaction, &err)) { - rp_error("%s", err.buf); - reported_error = "failed to update refs"; - goto failure; - } - - 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); - 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