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 D/F conflict. A
similar issue was present in 'git-fetch(1)' and was fixed by separating
out reference pruning into a separate transaction in the commit 'fetch:
use batched reference updates'. 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 up to 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@xxxxxxxxx>
---
builtin/receive-pack.c | 102 ++++++++++++++++++++++++++++++++-----------------
t/t5516-fetch-push.sh | 17 +++++++--
2 files changed, 81 insertions(+), 38 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9e3cfb85cf..1809539201 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1867,47 +1867,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 (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 start";
+ 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;
+ /* No transaction, so 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
