In 23fc8e4f61 (refs: implement batch reference update support, 2025-04-08) we introduced a mechanism to batch reference updates. The idea being that we wanted to batch updates similar to a transaction, but allow certain updates to fail. This would help reference backends optimize such operations and also remove the overhead of processing updates individually. Especially for the reftable backend, where batching updates would ensure that the autocompaction would only occur at the end of the batch instead of after every reference update. As of 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19) we also updated the 'git-fetch(1)' and 'git-receive-pack(1)' command to use batched reference updates. This series fixes some bugs that we found at GitLab by running our Gitaly service with the `next` build of Git. The first being in the files backend, which missed skipping over failed updates in certain flows. When certain individual updates fail, we mark them as such. However, we missed skipping over such failed updates, which can cause a SEGFAULT. The other is in the git-receive-pack(1) implementation 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 is based off master 7014b55638 (A bit more topics for -rc1, 2025-05-30), with the changes from kn/fetch-push-bulk-ref-update merged in. --- Changes in v2: - Modify the test in the first commit to no longer do a quiet grep, and some more tests. - Remove the second commit as it was unnecessary. - Modify the commit message in the last commit, to also talk about how we now use 2 transactions at minimum but this is still better than before. - Modify the logic in the last commit to no longer use an XOR and instead loop over the two different scenarios (deletion updates, other updates). - Link to v1: https://lore.kernel.org/r/20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-0-903d1db3f10e@xxxxxxxxx --- builtin/receive-pack.c | 90 +++++++++++++++++++++++++--------------- refs/files-backend.c | 7 ++++ t/t1400-update-ref.sh | 45 ++++++++++++++++++++ t/t1416-ref-transaction-hooks.sh | 2 + t/t5516-fetch-push.sh | 17 ++++++-- 5 files changed, 123 insertions(+), 38 deletions(-) Karthik Nayak (2): refs/files: skip updates with errors in batched updates receive-pack: handle reference deletions separately Range-diff versus v1: 1: 1c0ea8e209 ! 1: 65bcb5d2cf refs/files: skip updates with errors in batched updates @@ t/t1400-update-ref.sh: do ) ' + ++ test_expect_success "stdin $type batch-updates delete incorrect symbolic ref" ' ++ git init repo && ++ test_when_finished "rm -fr repo" && ++ ( ++ cd repo && ++ test_commit c1 && ++ head=$(git rev-parse HEAD) && ++ git symbolic-ref refs/heads/symbolic refs/heads/non-existent && ++ ++ format_command $type "delete refs/heads/symbolic" "$head" >stdin && ++ git update-ref $type --stdin --batch-updates <stdin >stdout && ++ test_grep "reference does not exist" stdout ++ ) ++ ' ++ ++ test_expect_success "stdin $type batch-updates delete with incorrect old_oid" ' ++ git init repo && ++ test_when_finished "rm -fr repo" && ++ ( ++ cd repo && ++ test_commit c1 && ++ git branch new-branch && ++ test_commit c2 && ++ head=$(git rev-parse HEAD) && ++ ++ format_command $type "delete refs/heads/new-branch" "$head" >stdin && ++ git update-ref $type --stdin --batch-updates <stdin >stdout && ++ test_grep "incorrect old value provided" stdout ++ ) ++ ' ++ + test_expect_success "stdin $type batch-updates delete non-existent ref" ' + git init repo && + test_when_finished "rm -fr repo" && @@ t/t1400-update-ref.sh: do + + format_command $type "delete refs/heads/non-existent" "$head" >stdin && + git update-ref $type --stdin --batch-updates <stdin >stdout && -+ test_grep -q "reference does not exist" stdout ++ test_grep "reference does not exist" stdout + ) + ' done 2: 5c21a3770e < -: ---------- t5516: use double quotes for tests with variables 3: 38eb79bd41 ! 2: e65b29b5f1 receive-pack: handle reference deletions separately @@ Commit message 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 ## -@@ builtin/receive-pack.c: static void ref_transaction_rejection_handler(const char *refname, +@@ builtin/receive-pack.c: 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_non_atomic(struct command *commands, -- struct shallow_info *si) -+ struct shallow_info *si, -+ int only_deletions) - { - struct command *cmd; - struct strbuf err = STRBUF_INIT; -@@ builtin/receive-pack.c: 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); - } + static void execute_commands_atomic(struct command *commands, @@ builtin/receive-pack.c: static void execute_commands(struct command *commands, /* * If there is no command ready to run, should return directly to destroy @@ builtin/receive-pack.c: static void execute_commands(struct command *commands, */ for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next) ; /* nothing */ -@@ builtin/receive-pack.c: 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); ## t/t1416-ref-transaction-hooks.sh ## @@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'interleaving hook calls succeed' ' base-commit: 931c39f05e078e0df968a439379cb04b5c4666ef change-id: 20250528-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-24ff53680144 Thanks - Karthik
