{"thread":{"id":"63560","subject":"[PATCH 0/3] refs: fix some bugs with batched-updates","startedAt":"2025-06-02T09:57:39Z","lastAt":"2025-06-22T14:20:34Z","messageCount":40,"participants":["Karthik Nayak","Patrick Steinhardt","Junio C Hamano","Eric Sunshine","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"519402","messageId":"20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-0-903d1db3f10e@gmail.com","threadId":"63560","inReplyTo":null,"subject":"[PATCH 0/3] refs: fix some bugs with batched-updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-02T09:57:23Z","receivedAt":"2025-06-02T09:57:39Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08) we introduced a mechanism to batch reference updates. The\nidea being that we wanted to batch updates similar to a transaction, but\nallow certain updates to fail. This would help reference backends\noptimize such operations and also remove the overhead of processing\nupdates individually. Especially for the reftable backend, where\nbatching updates would ensure that the autocompaction would only occur\nat the end of the batch instead of after every reference update.\n\nAs of 9d2962a7c4 (receive-pack: use batched reference updates,\n2025-05-19) we also updated the 'git-fetch(1)' and 'git-receive-pack(1)'\ncommand to use batched reference updates. This series fixes some bugs\nthat we found at GitLab by running our Gitaly service with the `next`\nbuild of Git.\n\nThe first being in the files backend, which missed skipping over failed\nupdates in certain flows. When certain individual updates fail, we mark\nthem as such. However, we missed skipping over such failed updates,\nwhich can cause a SEGFAULT.\n\nThe other is in the git-receive-pack(1) implementation when a user\npushes multiple branches such as:\n\n delete refs/heads/branch/conflict\n create refs/heads/branch\n\nBefore using batched updates, the references would be applied\nsequentially and hence no conflicts would arise. With batched updates,\nwhile the first update applies, the second fails due to F/D conflict. A\nsimilar issue was present in 'git-fetch(1)' and was fixed by using\nseparating out reference pruning into a separate transaction. Apply a\nsimilar mechanism for 'git-receive-pack(1)' and separate out reference\ndeletions into its own batch.\n\nThis is based off master 7014b55638 (A bit more topics for -rc1,\n2025-05-30), with the changes from kn/fetch-push-bulk-ref-update merged\nin.\n\n---\n builtin/receive-pack.c | 23 +++++++++++++++++++----\n refs/files-backend.c | 7 +++++++\n t/t1400-update-ref.sh | 14 ++++++++++++++\n t/t1416-ref-transaction-hooks.sh | 2 ++\n t/t5516-fetch-push.sh | 23 ++++++++++++++++-------\n 5 files changed, 58 insertions(+), 11 deletions(-)\n\nKarthik Nayak (3):\n refs/files: skip updates with errors in batched updates\n t5516: use double quotes for tests with variables\n receive-pack: handle reference deletions separately\n\n\n\nbase-commit: 931c39f05e078e0df968a439379cb04b5c4666ef\nchange-id: 20250528-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-24ff53680144\n\nThanks\n- Karthik\n\n"},{"id":"519403","messageId":"20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-1-903d1db3f10e@gmail.com","threadId":"63560","inReplyTo":"20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-0-903d1db3f10e@gmail.com","subject":"[PATCH 1/3] refs/files: skip updates with errors in batched updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-02T09:57:24Z","receivedAt":"2025-06-02T09:57:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08) introduced support for batched reference updates. This\nallows users to batch updates together, while allowing some of the\nupdates to fail.\n\nUnder the hood, batched updates use the reference transaction mechanism.\nEach update which fails is marked as such. Any failed updates must be\nskipped over in the rest of the code, as they wouldn't apply any more.\nIn two of the loops within 'files_transaction_finish()' of the files\nbackend, the failed updates aren't skipped over. This can cause a\nSEGFAULT otherwise. Add the missing skips and a test to validate the\nsame.\n\nSigned-off-by: Karthik Nayak \n---\n refs/files-backend.c | 7 +++++++\n t/t1400-update-ref.sh | 14 ++++++++++++++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4d1f65a57a..c4a0f29072 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t */\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n+\n+\t\tif (update->rejection_err)\n+\t\t\tcontinue;\n+\n \t\tif (update->flags & REF_DELETING &&\n \t\t !(update->flags & REF_LOG_ONLY) &&\n \t\t !(update->flags & REF_IS_PRUNING)) {\n@@ -3239,6 +3243,9 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t\tstruct ref_update *update = transaction->updates[i];\n \t\tstruct ref_lock *lock = update->backend_data;\n \n+\t\tif (update->rejection_err)\n+\t\t\tcontinue;\n+\n \t\tif (update->flags & REF_DELETING &&\n \t\t !(update->flags & REF_LOG_ONLY)) {\n \t\t\tupdate->flags |= REF_DELETED_RMDIR;\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex d29d23cb89..e9a605d0ba 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -2299,6 +2299,20 @@ do\n \t\t\ttest_grep -q \"refname conflict\" stdout\n \t\t)\n \t'\n+\n+\ttest_expect_success \"stdin $type batch-updates delete non-existent ref\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit commit &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\n+\t\t\tformat_command $type \"delete refs/heads/non-existent\" \"$head\" >stdin &&\n+\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n+\t\t\ttest_grep -q \"reference does not exist\" stdout\n+\t\t)\n+\t'\n done\n \n test_expect_success 'update-ref should also create reflog for HEAD' '\n\n-- \n2.49.0\n\n"},{"id":"519404","messageId":"20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-2-903d1db3f10e@gmail.com","threadId":"63560","inReplyTo":"20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-0-903d1db3f10e@gmail.com","subject":"[PATCH 2/3] t5516: use double quotes for tests with variables","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-02T09:57:25Z","receivedAt":"2025-06-02T09:57:41Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Since expressions don't expand within single quotes, change test\ndescriptions containing variables to use double quotes.\n\nSigned-off-by: Karthik Nayak \n---\n t/t5516-fetch-push.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex dabcc5f811..029ef92d58 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1421,7 +1421,7 @@ test_expect_success 'peeled advertisements are not considered ref tips' '\n \ttest_grep \"Server does not allow request for unadvertised object\" err\n '\n \n-test_expect_success 'pushing a specific ref applies remote.$name.push as refmap' '\n+test_expect_success \"pushing a specific ref applies remote.$name.push as refmap\" '\n \tmk_test testrepo heads/main &&\n \ttest_when_finished \"rm -rf src\" &&\n \tgit init src &&\n@@ -1446,7 +1446,7 @@ test_expect_success 'pushing a specific ref applies remote.$name.push as refmap'\n \ttest_cmp dst/expect dst/actual\n '\n \n-test_expect_success 'with no remote.$name.push, it is not used as refmap' '\n+test_expect_success \"with no remote.$name.push, it is not used as refmap\" '\n \tmk_test testrepo heads/main &&\n \ttest_when_finished \"rm -rf src\" &&\n \tgit init src &&\n@@ -1469,7 +1469,7 @@ test_expect_success 'with no remote.$name.push, it is not used as refmap' '\n \ttest_cmp dst/expect dst/actual\n '\n \n-test_expect_success 'with no remote.$name.push, upstream mapping is used' '\n+test_expect_success \"with no remote.$name.push, upstream mapping is used\" '\n \tmk_test testrepo heads/main &&\n \ttest_when_finished \"rm -rf src\" &&\n \tgit init src &&\n\n-- \n2.49.0\n\n"},{"id":"519405","messageId":"20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-3-903d1db3f10e@gmail.com","threadId":"63560","inReplyTo":"20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-0-903d1db3f10e@gmail.com","subject":"[PATCH 3/3] receive-pack: handle reference deletions separately","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-02T09:57:26Z","receivedAt":"2025-06-02T09:57:42Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)\nwe updated the 'git-receive-pack(1)' command to use batched reference\nupdates. One edge case which was missed during this implementation was\nwhen a user pushes multiple branches such as:\n\n delete refs/heads/branch/conflict\n create refs/heads/branch\n\nBefore using batched updates, the references would be applied\nsequentially and hence no conflicts would arise. With batched updates,\nwhile the first update applies, the second fails due to F/D conflict. A\nsimilar issue was present in 'git-fetch(1)' and was fixed by using\nseparating out reference pruning into a separate transaction. Apply a\nsimilar mechanism for 'git-receive-pack(1)' and separate out reference\ndeletions into its own batch.\n\nAdd a test to validate this behavior.\n\nSigned-off-by: Karthik Nayak \n---\n builtin/receive-pack.c | 23 +++++++++++++++++++----\n t/t1416-ref-transaction-hooks.sh | 2 ++\n t/t5516-fetch-push.sh | 17 +++++++++++++----\n 3 files changed, 34 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9e3cfb85cf..7157ced2a6 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1860,7 +1860,8 @@ static void ref_transaction_rejection_handler(const char *refname,\n }\n \n static void execute_commands_non_atomic(struct command *commands,\n-\t\t\t\t\tstruct shallow_info *si)\n+\t\t\t\t\tstruct shallow_info *si,\n+\t\t\t\t\tint only_deletions)\n {\n \tstruct command *cmd;\n \tstruct strbuf err = STRBUF_INIT;\n@@ -1879,6 +1880,8 @@ static void execute_commands_non_atomic(struct command *commands,\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n \t\t\tcontinue;\n+\t\tif (only_deletions ^ is_null_oid(&cmd->new_oid))\n+\t\t\tcontinue;\n \n \t\tcmd->error_string = update(cmd, si);\n \t}\n@@ -2024,6 +2027,9 @@ static void execute_commands(struct command *commands,\n \t/*\n \t * If there is no command ready to run, should return directly to destroy\n \t * temporary data in the quarantine area.\n+\t *\n+\t * Check if any reference deletions exist, these are batched together in\n+\t * a separate transaction to avoid F/D conflicts with other updates.\n \t */\n \tfor (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)\n \t\t; /* nothing */\n@@ -2058,10 +2064,19 @@ static void execute_commands(struct command *commands,\n \t\t\t (cmd->run_proc_receive || use_atomic))\n \t\t\t\tcmd->error_string = \"fail to run proc-receive hook\";\n \n-\tif (use_atomic)\n+\tif (use_atomic) {\n \t\texecute_commands_atomic(commands, si);\n-\telse\n-\t\texecute_commands_non_atomic(commands, si);\n+\t} else {\n+\t\t/*\n+\t\t * Reference updates, where F/D conflicts shouldn't arise due to\n+\t\t * one reference being deleted, while the other being created\n+\t\t * are treated as conflicts in batched updates. This is because\n+\t\t * we don't do conflict resolution inside a transaction. To\n+\t\t * mitigate this, delete references in a separate batch.\n+\t\t */\n+\t\texecute_commands_non_atomic(commands, si, 1);\n+\t\texecute_commands_non_atomic(commands, si, 0);\n+\t}\n \n \tif (shallow_update)\n \t\tBUG_if_skipped_connectivity_check(commands, si);\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex d91dd3a3b5..b2aaa1908f 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -119,6 +119,8 @@ test_expect_success 'interleaving hook calls succeed' '\n \tEOF\n \n \tcat >expect <<-EOF &&\n+\t\thooks/reference-transaction prepared\n+\t\thooks/reference-transaction committed\n \t\thooks/update refs/tags/PRE $ZERO_OID $PRE_OID\n \t\thooks/update refs/tags/POST $ZERO_OID $POST_OID\n \t\thooks/reference-transaction prepared\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 029ef92d58..34eb3a5a07 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $orgmain $newmain\n \t\trefs/heads/next $orgnext $newnext\n+\t\trefs/heads/main $orgmain $newmain\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n@@ -808,8 +808,8 @@ test_expect_success 'deletion of a non-existent ref is not fed to post-receive a\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $orgmain $newmain\n \t\trefs/heads/nonexistent $ZERO_OID $ZERO_OID\n+\t\trefs/heads/main $orgmain $newmain\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n@@ -868,10 +868,10 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $orgmain $newmain\n \t\trefs/heads/next $orgnext $newnext\n-\t\trefs/heads/seen $orgseen $newseen\n \t\trefs/heads/nonexistent $ZERO_OID $ZERO_OID\n+\t\trefs/heads/main $orgmain $newmain\n+\t\trefs/heads/seen $orgseen $newseen\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n@@ -1909,4 +1909,13 @@ test_expect_success 'push with config push.useBitmaps' '\n \t\t--thin --delta-base-offset -q --no-use-bitmap-index In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)\n> we updated the 'git-receive-pack(1)' command to use batched reference\n> updates. One edge case which was missed during this implementation was\n> when a user pushes multiple branches such as:\n> \n> delete refs/heads/branch/conflict\n> create refs/heads/branch\n> \n> Before using batched updates, the references would be applied\n> sequentially and hence no conflicts would arise. With batched updates,\n> while the first update applies, the second fails due to F/D conflict. A\n> similar issue was present in 'git-fetch(1)' and was fixed by using\n> separating out reference pruning into a separate transaction. Apply a\n> similar mechanism for 'git-receive-pack(1)' and separate out reference\n> deletions into its own batch.\n> \n> Add a test to validate this behavior.\n\nOkay. All of this is unfortunate as ideally the reference transaction\nitself would know to resolve such conflicts. But we're no worse off than\nbefore because we at most perform exactly two transactions now, whereas\nbefore we would have performed _at least_ two transactions in this\nconflicting case.\n\n> Signed-off-by: Karthik Nayak \n> ---\n> builtin/receive-pack.c | 23 +++++++++++++++++++----\n> t/t1416-ref-transaction-hooks.sh | 2 ++\n> t/t5516-fetch-push.sh | 17 +++++++++++++----\n> 3 files changed, 34 insertions(+), 8 deletions(-)\n> \n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 9e3cfb85cf..7157ced2a6 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1879,6 +1880,8 @@ static void execute_commands_non_atomic(struct command *commands,\n> \tfor (cmd = commands; cmd; cmd = cmd->next) {\n> \t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n> \t\t\tcontinue;\n> +\t\tif (only_deletions ^ is_null_oid(&cmd->new_oid))\n> +\t\t\tcontinue;\n> \n> \t\tcmd->error_string = update(cmd, si);\n> \t}\n\nFancy.\n\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 029ef92d58..34eb3a5a07 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho\n> \t\tEOF\n> \n> \t\tcat >update.expect <<-EOF &&\n> -\t\trefs/heads/main $orgmain $newmain\n> \t\trefs/heads/next $orgnext $newnext\n> +\t\trefs/heads/main $orgmain $newmain\n> \t\tEOF\n> \n> \t\tcat >post-receive.expect <<-EOF &&\n\nHm, so the ordering does change now as all deletes will now be listed\nbefore the updates. We don't make any guarantees about how these are\nsorted, but it makes me a bit uneasy to see this change. Can we avoid\nthis change in behaviour somehow?\n\nPatrick\n"},{"id":"519441","messageId":"aD2SRmlSKZm8g8kn@pks.im","threadId":"63560","inReplyTo":"20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-1-903d1db3f10e@gmail.com","subject":"Re: [PATCH 1/3] refs/files: skip updates with errors in batched updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-06-02T12:00:06Z","receivedAt":"2025-06-02T12:00:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jun 02, 2025 at 11:57:24AM +0200, Karthik Nayak wrote:\n> The commit 23fc8e4f61 (refs: implement batch reference update support,\n> 2025-04-08) introduced support for batched reference updates. This\n> allows users to batch updates together, while allowing some of the\n> updates to fail.\n> \n> Under the hood, batched updates use the reference transaction mechanism.\n> Each update which fails is marked as such. Any failed updates must be\n> skipped over in the rest of the code, as they wouldn't apply any more.\n> In two of the loops within 'files_transaction_finish()' of the files\n> backend, the failed updates aren't skipped over. This can cause a\n> SEGFAULT otherwise. Add the missing skips and a test to validate the\n> same.\n\nCurious -- we do have tests, so why don't any of them hit this issue?\n\n> Signed-off-by: Karthik Nayak \n> ---\n> refs/files-backend.c | 7 +++++++\n> t/t1400-update-ref.sh | 14 ++++++++++++++\n> 2 files changed, 21 insertions(+)\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 4d1f65a57a..c4a0f29072 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,\n> \t */\n> \tfor (i = 0; i < transaction->nr; i++) {\n> \t\tstruct ref_update *update = transaction->updates[i];\n> +\n> +\t\tif (update->rejection_err)\n> +\t\t\tcontinue;\n> +\n> \t\tif (update->flags & REF_DELETING &&\n> \t\t !(update->flags & REF_LOG_ONLY) &&\n> \t\t !(update->flags & REF_IS_PRUNING)) {\n\nOk. And the reftable backend doesn't need the same treatment? Probably\ndoesn't because we queue all updates via `queue_transaction_update()`,\nwhich knows to not add them to the list of updates in case any error has\nhappened. And in `_finish()` we iterate through that list of queued\nupdates instead of the global list of updates.\n\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index d29d23cb89..e9a605d0ba 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -2299,6 +2299,20 @@ do\n> \t\t\ttest_grep -q \"refname conflict\" stdout\n> \t\t)\n> \t'\n> +\n> +\ttest_expect_success \"stdin $type batch-updates delete non-existent ref\" '\n> +\t\tgit init repo &&\n> +\t\ttest_when_finished \"rm -fr repo\" &&\n> +\t\t(\n> +\t\t\tcd repo &&\n> +\t\t\ttest_commit commit &&\n> +\t\t\thead=$(git rev-parse HEAD) &&\n> +\n> +\t\t\tformat_command $type \"delete refs/heads/non-existent\" \"$head\" >stdin &&\n> +\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n> +\t\t\ttest_grep -q \"reference does not exist\" stdout\n\nWe typically don't silence the output of `test_grep`.\n\nPatrick\n"},{"id":"519455","messageId":"CAOLa=ZQQCFA=jdOGDONGcsO_VDUAxfHjn_Wk5r+8weHMFXDQ3w@mail.gmail.com","threadId":"63560","inReplyTo":"aD2SRmlSKZm8g8kn@pks.im","subject":"Re: [PATCH 1/3] refs/files: skip updates with errors in batched updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-02T12:46:12Z","receivedAt":"2025-06-02T12:46:15Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Mon, Jun 02, 2025 at 11:57:24AM +0200, Karthik Nayak wrote:\n>> The commit 23fc8e4f61 (refs: implement batch reference update support,\n>> 2025-04-08) introduced support for batched reference updates. This\n>> allows users to batch updates together, while allowing some of the\n>> updates to fail.\n>>\n>> Under the hood, batched updates use the reference transaction mechanism.\n>> Each update which fails is marked as such. Any failed updates must be\n>> skipped over in the rest of the code, as they wouldn't apply any more.\n>> In two of the loops within 'files_transaction_finish()' of the files\n>> backend, the failed updates aren't skipped over. This can cause a\n>> SEGFAULT otherwise. Add the missing skips and a test to validate the\n>> same.\n>\n> Curious -- we do have tests, so why don't any of them hit this issue?\n>\n\nWe don't hit this edge case in any of our tests I presume. This\nbasically requires a deletion request with an expected old OID, however\nthe reference should be non-existent.\n\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> refs/files-backend.c | 7 +++++++\n>> t/t1400-update-ref.sh | 14 ++++++++++++++\n>> 2 files changed, 21 insertions(+)\n>>\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index 4d1f65a57a..c4a0f29072 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,\n>> \t */\n>> \tfor (i = 0; i < transaction->nr; i++) {\n>> \t\tstruct ref_update *update = transaction->updates[i];\n>> +\n>> +\t\tif (update->rejection_err)\n>> +\t\t\tcontinue;\n>> +\n>> \t\tif (update->flags & REF_DELETING &&\n>> \t\t !(update->flags & REF_LOG_ONLY) &&\n>> \t\t !(update->flags & REF_IS_PRUNING)) {\n>\n> Ok. And the reftable backend doesn't need the same treatment? Probably\n> doesn't because we queue all updates via `queue_transaction_update()`,\n> which knows to not add them to the list of updates in case any error has\n> happened. And in `_finish()` we iterate through that list of queued\n> updates instead of the global list of updates.\n>\n\nExactly, all writes in the reftable backend are propagated finally via\n'write_transaction_table()', which checks for rejections. The single\npoint of entry acts as a good gatekeeper.\n\n>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n>> index d29d23cb89..e9a605d0ba 100755\n>> --- a/t/t1400-update-ref.sh\n>> +++ b/t/t1400-update-ref.sh\n>> @@ -2299,6 +2299,20 @@ do\n>> \t\t\ttest_grep -q \"refname conflict\" stdout\n>> \t\t)\n>> \t'\n>> +\n>> +\ttest_expect_success \"stdin $type batch-updates delete non-existent ref\" '\n>> +\t\tgit init repo &&\n>> +\t\ttest_when_finished \"rm -fr repo\" &&\n>> +\t\t(\n>> +\t\t\tcd repo &&\n>> +\t\t\ttest_commit commit &&\n>> +\t\t\thead=$(git rev-parse HEAD) &&\n>> +\n>> +\t\t\tformat_command $type \"delete refs/heads/non-existent\" \"$head\" >stdin &&\n>> +\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n>> +\t\t\ttest_grep -q \"reference does not exist\" stdout\n>\n> We typically don't silence the output of `test_grep`.\n>\n> Patrick\n\nWill change, Thanks!\n"},{"id":"519456","messageId":"CAOLa=ZQ5To=ksfv7pfCoeJLsORWxaAvAR4Qs8kj8O_e8pNjsJg@mail.gmail.com","threadId":"63560","inReplyTo":"aD2SPsro694yr60Z@pks.im","subject":"Re: [PATCH 3/3] receive-pack: handle reference deletions separately","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-02T12:54:55Z","receivedAt":"2025-06-02T12:54:58Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Mon, Jun 02, 2025 at 11:57:26AM +0200, Karthik Nayak wrote:\n>> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)\n>> we updated the 'git-receive-pack(1)' command to use batched reference\n>> updates. One edge case which was missed during this implementation was\n>> when a user pushes multiple branches such as:\n>>\n>> delete refs/heads/branch/conflict\n>> create refs/heads/branch\n>>\n>> Before using batched updates, the references would be applied\n>> sequentially and hence no conflicts would arise. With batched updates,\n>> while the first update applies, the second fails due to F/D conflict. A\n>> similar issue was present in 'git-fetch(1)' and was fixed by using\n>> separating out reference pruning into a separate transaction. Apply a\n>> similar mechanism for 'git-receive-pack(1)' and separate out reference\n>> deletions into its own batch.\n>>\n>> Add a test to validate this behavior.\n>\n> Okay. All of this is unfortunate as ideally the reference transaction\n> itself would know to resolve such conflicts. But we're no worse off than\n> before because we at most perform exactly two transactions now, whereas\n> before we would have performed _at least_ two transactions in this\n> conflicting case.\n>\n\nExactly. It is not easy to build in conflict resolution but it would be\ngreat to have it. Also to some extent F/D conflicts are due to\nimplementation details. With the reftable backend, we artificially\nintroduce this limitation, maybe someday if we get rid of the files\nbackend, we won't even have this issue.\n\nThanks for spelling this out, I think it would be worthwhile to add this\nto the commit message too. I will do that.\n\n>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> builtin/receive-pack.c | 23 +++++++++++++++++++----\n>> t/t1416-ref-transaction-hooks.sh | 2 ++\n>> t/t5516-fetch-push.sh | 17 +++++++++++++----\n>> 3 files changed, 34 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n>> index 9e3cfb85cf..7157ced2a6 100644\n>> --- a/builtin/receive-pack.c\n>> +++ b/builtin/receive-pack.c\n>> @@ -1879,6 +1880,8 @@ static void execute_commands_non_atomic(struct command *commands,\n>> \tfor (cmd = commands; cmd; cmd = cmd->next) {\n>> \t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n>> \t\t\tcontinue;\n>> +\t\tif (only_deletions ^ is_null_oid(&cmd->new_oid))\n>> +\t\t\tcontinue;\n>>\n>> \t\tcmd->error_string = update(cmd, si);\n>> \t}\n>\n> Fancy.\n>\n>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n>> index 029ef92d58..34eb3a5a07 100755\n>> --- a/t/t5516-fetch-push.sh\n>> +++ b/t/t5516-fetch-push.sh\n>> @@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho\n>> \t\tEOF\n>>\n>> \t\tcat >update.expect <<-EOF &&\n>> -\t\trefs/heads/main $orgmain $newmain\n>> \t\trefs/heads/next $orgnext $newnext\n>> +\t\trefs/heads/main $orgmain $newmain\n>> \t\tEOF\n>>\n>> \t\tcat >post-receive.expect <<-EOF &&\n>\n> Hm, so the ordering does change now as all deletes will now be listed\n> before the updates. We don't make any guarantees about how these are\n> sorted, but it makes me a bit uneasy to see this change. Can we avoid\n> this change in behaviour somehow?\n>\n\nYeah it does. I couldn't think of a way thought to retain this behavior.\nBut I will spend some time on it.\n\n> Patrick\n"},{"id":"519472","messageId":"xmqqcybmuqrj.fsf@gitster.g","threadId":"63560","inReplyTo":"CAOLa=ZQQCFA=jdOGDONGcsO_VDUAxfHjn_Wk5r+8weHMFXDQ3w@mail.gmail.com","subject":"Re: [PATCH 1/3] refs/files: skip updates with errors in batched updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-02T15:06:40Z","receivedAt":"2025-06-02T15:06:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n>> Curious -- we do have tests, so why don't any of them hit this issue?\n>>\n>\n> We don't hit this edge case in any of our tests I presume. This\n> basically requires a deletion request with an expected old OID, however\n> the reference should be non-existent.\n\nI read Patrick's remart not as a question but as a rhetoric\nsuggestion to make sure we have test coverage ;-)\n"},{"id":"519473","messageId":"xmqq7c1uuq52.fsf@gitster.g","threadId":"63560","inReplyTo":"aD2SPsro694yr60Z@pks.im","subject":"Re: [PATCH 3/3] receive-pack: handle reference deletions separately","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-02T15:20:09Z","receivedAt":"2025-06-02T15:20:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Okay. All of this is unfortunate as ideally the reference transaction\n> itself would know to resolve such conflicts.\n\n100% agreed.\n\n> But we're no worse off than\n> before because we at most perform exactly two transactions now, whereas\n> before we would have performed _at least_ two transactions in this\n> conflicting case.\n>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> builtin/receive-pack.c | 23 +++++++++++++++++++----\n>> t/t1416-ref-transaction-hooks.sh | 2 ++\n>> t/t5516-fetch-push.sh | 17 +++++++++++++----\n>> 3 files changed, 34 insertions(+), 8 deletions(-)\n>> \n>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n>> index 9e3cfb85cf..7157ced2a6 100644\n>> --- a/builtin/receive-pack.c\n>> +++ b/builtin/receive-pack.c\n>> @@ -1879,6 +1880,8 @@ static void execute_commands_non_atomic(struct command *commands,\n>> \tfor (cmd = commands; cmd; cmd = cmd->next) {\n>> \t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n>> \t\t\tcontinue;\n>> +\t\tif (only_deletions ^ is_null_oid(&cmd->new_oid))\n>> +\t\t\tcontinue;\n>> \n>> \t\tcmd->error_string = update(cmd, si);\n>> \t}\n>\n> Fancy.\n\nIs that a new synonym for \"not worth being overly clever to\nsacrifice readability\"?\n\nThis may be a comment for [2/3], but a two-call sequence\n\n\tdoit(only_deletions = yes);\n\tdoit(only_deletions = no);\n\nlooked somewhat iffy for a first reader, as it hints that the second\ncall would do both non-deletions (i.e. creation and modification)\nand deletions, which makes readers wonder \"so we delete twice and\nrely on that it is not an error to delete something that does not\nexist?\"\n\n\n>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n>> index 029ef92d58..34eb3a5a07 100755\n>> --- a/t/t5516-fetch-push.sh\n>> +++ b/t/t5516-fetch-push.sh\n>> @@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho\n>> \t\tEOF\n>> \n>> \t\tcat >update.expect <<-EOF &&\n>> -\t\trefs/heads/main $orgmain $newmain\n>> \t\trefs/heads/next $orgnext $newnext\n>> +\t\trefs/heads/main $orgmain $newmain\n>> \t\tEOF\n>> \n>> \t\tcat >post-receive.expect <<-EOF &&\n>\n> Hm, so the ordering does change now as all deletes will now be listed\n> before the updates. We don't make any guarantees about how these are\n> sorted, but it makes me a bit uneasy to see this change. Can we avoid\n> this change in behaviour somehow?\n\nGood eyes.\n\nI was wondering about the \"git push -v\" reporting and was happy that\nthe order there follows the order the pushing side listed refs and\nthe reordering on the receiving end would not have any effect. The\nhooks on the receiving end can indeed observe this change.\n\nThey can observe, but can they notice? If the pusher listed refspec\nelements for deletions first before creations and modifications on\ntheir command line, that would be what the hooks see. They do not\nknow what the original \"push\" said so they have nothing to compare\nand complain.\n\nAhhh, but humans that control the both ends may notice and complain.\n\nOK, I think I agree with you that it is worth to at least spend some\nbrain cycles thinking about avoiding the behaviour change.\n\nThanks.\n"},{"id":"519478","messageId":"aD3JsdlRPVi4wjuz@pks.im","threadId":"63560","inReplyTo":"xmqq7c1uuq52.fsf@gitster.g","subject":"Re: [PATCH 3/3] receive-pack: handle reference deletions separately","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-06-02T15:56:33Z","receivedAt":"2025-06-02T15:56:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jun 02, 2025 at 08:20:09AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> >> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> >> index 9e3cfb85cf..7157ced2a6 100644\n> >> --- a/builtin/receive-pack.c\n> >> +++ b/builtin/receive-pack.c\n> >> @@ -1879,6 +1880,8 @@ static void execute_commands_non_atomic(struct command *commands,\n> >> \tfor (cmd = commands; cmd; cmd = cmd->next) {\n> >> \t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n> >> \t\t\tcontinue;\n> >> +\t\tif (only_deletions ^ is_null_oid(&cmd->new_oid))\n> >> +\t\t\tcontinue;\n> >> \n> >> \t\tcmd->error_string = update(cmd, si);\n> >> \t}\n> >\n> > Fancy.\n> \n> Is that a new synonym for \"not worth being overly clever to\n> sacrifice readability\"?\n\nYeah. I wasn't quite sure whether I like it or not as it felt a bit too\nclever to me indeed. But I didn't feel strongly about it either, so it\nturned into the above somewhat unhelpful remark.\n\n> This may be a comment for [2/3], but a two-call sequence\n> \n> \tdoit(only_deletions = yes);\n> \tdoit(only_deletions = no);\n> \n> looked somewhat iffy for a first reader, as it hints that the second\n> call would do both non-deletions (i.e. creation and modification)\n> and deletions, which makes readers wonder \"so we delete twice and\n> rely on that it is not an error to delete something that does not\n> exist?\"\n\nI also wondered whether it wouldn't be nicer to have the loop in\n`doit()` itself. It would require a bit of reindenting though, which is\nwhy I didn't propose that variant.\n\n> >> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> >> index 029ef92d58..34eb3a5a07 100755\n> >> --- a/t/t5516-fetch-push.sh\n> >> +++ b/t/t5516-fetch-push.sh\n> >> @@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho\n> >> \t\tEOF\n> >> \n> >> \t\tcat >update.expect <<-EOF &&\n> >> -\t\trefs/heads/main $orgmain $newmain\n> >> \t\trefs/heads/next $orgnext $newnext\n> >> +\t\trefs/heads/main $orgmain $newmain\n> >> \t\tEOF\n> >> \n> >> \t\tcat >post-receive.expect <<-EOF &&\n> >\n> > Hm, so the ordering does change now as all deletes will now be listed\n> > before the updates. We don't make any guarantees about how these are\n> > sorted, but it makes me a bit uneasy to see this change. Can we avoid\n> > this change in behaviour somehow?\n> \n> Good eyes.\n> \n> I was wondering about the \"git push -v\" reporting and was happy that\n> the order there follows the order the pushing side listed refs and\n> the reordering on the receiving end would not have any effect. The\n> hooks on the receiving end can indeed observe this change.\n> \n> They can observe, but can they notice? If the pusher listed refspec\n> elements for deletions first before creations and modifications on\n> their command line, that would be what the hooks see. They do not\n> know what the original \"push\" said so they have nothing to compare\n> and complain.\n> \n> Ahhh, but humans that control the both ends may notice and complain.\n> \n> OK, I think I agree with you that it is worth to at least spend some\n> brain cycles thinking about avoiding the behaviour change.\n\nHm. The thing I didn't realize is that the changed output order is for\nthe \"update\" hook. I thought it was for the \"post-receive\" hook, where I\ndo expect that ordering may matter as you see the whole picture of all\nrefs that have been updated. But for the \"update\" hook I think it is\nsensible to change the ordering -- after all, the order of updates does\nchange a bit now. Furthermore, the \"update\" hook itself doesn't have the\ncomplete picture anyway, so it's way less likely that anybody relies on\nthe ordering.\n\nPatrick\n"},{"id":"519486","messageId":"CAPig+cTMDcvwfzq7Pqn+MF7r1x7qZq2k=8pAV3bio0pcnuh4eA@mail.gmail.com","threadId":"63560","inReplyTo":"20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-2-903d1db3f10e@gmail.com","subject":"Re: [PATCH 2/3] t5516: use double quotes for tests with variables","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-06-02T16:59:38Z","receivedAt":"2025-06-02T16:59:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 2, 2025 at 5:58 AM Karthik Nayak wrote:\n> Since expressions don't expand within single quotes, change test\n> descriptions containing variables to use double quotes.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> @@ -1421,7 +1421,7 @@ test_expect_success 'peeled advertisements are not considered ref tips' '\n> -test_expect_success 'pushing a specific ref applies remote.$name.push as refmap' '\n> +test_expect_success \"pushing a specific ref applies remote.$name.push as refmap\" '\n\nIf I'm understanding correctly, I think this and the other changes in\nthis patch are incorrect. There is no `name` variable in this script.\nRather, these \"$name\" instances are merely illustrative, acting as\nplaceholders for the person reading the test title.\n"},{"id":"519487","messageId":"CAOLa=ZTp2F8QNg10O3SvGuje9MTZ5=RChcJ0t91MSNjLJF+m+A@mail.gmail.com","threadId":"63560","inReplyTo":"CAPig+cTMDcvwfzq7Pqn+MF7r1x7qZq2k=8pAV3bio0pcnuh4eA@mail.gmail.com","subject":"Re: [PATCH 2/3] t5516: use double quotes for tests with variables","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-02T17:13:10Z","receivedAt":"2025-06-02T17:13:11Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Eric Sunshine writes:\n\n> On Mon, Jun 2, 2025 at 5:58 AM Karthik Nayak wrote:\n>> Since expressions don't expand within single quotes, change test\n>> descriptions containing variables to use double quotes.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n>> @@ -1421,7 +1421,7 @@ test_expect_success 'peeled advertisements are not considered ref tips' '\n>> -test_expect_success 'pushing a specific ref applies remote.$name.push as refmap' '\n>> +test_expect_success \"pushing a specific ref applies remote.$name.push as refmap\" '\n>\n> If I'm understanding correctly, I think this and the other changes in\n> this patch are incorrect. There is no `name` variable in this script.\n> Rather, these \"$name\" instances are merely illustrative, acting as\n> placeholders for the person reading the test title.\n\nHuh, you're absolutely right. I think I did a 'grep -v' to ensure it\nworked and failed to realize that my shell was replacing $name. Will\nskip this patch in the follow up.\n\nThanks!\n"},{"id":"519488","messageId":"CAOLa=ZTP0i5q9Qw4g_ei+kSLyaRAMEoHf=PnPFbr7pRqgnyXAw@mail.gmail.com","threadId":"63560","inReplyTo":"xmqqcybmuqrj.fsf@gitster.g","subject":"Re: [PATCH 1/3] refs/files: skip updates with errors in batched updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-02T17:13:49Z","receivedAt":"2025-06-02T17:13:54Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>>> Curious -- we do have tests, so why don't any of them hit this issue?\n>>>\n>>\n>> We don't hit this edge case in any of our tests I presume. This\n>> basically requires a deletion request with an expected old OID, however\n>> the reference should be non-existent.\n>\n> I read Patrick's remart not as a question but as a rhetoric\n> suggestion to make sure we have test coverage ;-)\n\nAh! Okay, all is good then, since my patch does add a test for this now.\n"},{"id":"519653","messageId":"CAOLa=ZQmNNtTCeisdgk3gnqC_YDbWk4+To36bzp1A2-zXompJg@mail.gmail.com","threadId":"63560","inReplyTo":"aD3JsdlRPVi4wjuz@pks.im","subject":"Re: [PATCH 3/3] receive-pack: handle reference deletions separately","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-04T11:08:38Z","receivedAt":"2025-06-04T11:08:51Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Mon, Jun 02, 2025 at 08:20:09AM -0700, Junio C Hamano wrote:\n>> Patrick Steinhardt writes:\n>> >> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n>> >> index 9e3cfb85cf..7157ced2a6 100644\n>> >> --- a/builtin/receive-pack.c\n>> >> +++ b/builtin/receive-pack.c\n>> >> @@ -1879,6 +1880,8 @@ static void execute_commands_non_atomic(struct command *commands,\n>> >> \tfor (cmd = commands; cmd; cmd = cmd->next) {\n>> >> \t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n>> >> \t\t\tcontinue;\n>> >> +\t\tif (only_deletions ^ is_null_oid(&cmd->new_oid))\n>> >> +\t\t\tcontinue;\n>> >>\n>> >> \t\tcmd->error_string = update(cmd, si);\n>> >> \t}\n>> >\n>> > Fancy.\n>>\n>> Is that a new synonym for \"not worth being overly clever to\n>> sacrifice readability\"?\n>\n> Yeah. I wasn't quite sure whether I like it or not as it felt a bit too\n> clever to me indeed. But I didn't feel strongly about it either, so it\n> turned into the above somewhat unhelpful remark.\n>\n\nI will make it easier on the eyes in the next version.\n\n>> This may be a comment for [2/3], but a two-call sequence\n>>\n>> \tdoit(only_deletions = yes);\n>> \tdoit(only_deletions = no);\n>>\n>> looked somewhat iffy for a first reader, as it hints that the second\n>> call would do both non-deletions (i.e. creation and modification)\n>> and deletions, which makes readers wonder \"so we delete twice and\n>> rely on that it is not an error to delete something that does not\n>> exist?\"\n>\n> I also wondered whether it wouldn't be nicer to have the loop in\n> `doit()` itself. It would require a bit of reindenting though, which is\n> why I didn't propose that variant.\n>\n\nThis is mostly what I was trying to avoid, but perhaps the way to go\nanyways. So I'll do the needfull.\n\n>> >> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n>> >> index 029ef92d58..34eb3a5a07 100755\n>> >> --- a/t/t5516-fetch-push.sh\n>> >> +++ b/t/t5516-fetch-push.sh\n>> >> @@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho\n>> >> \t\tEOF\n>> >>\n>> >> \t\tcat >update.expect <<-EOF &&\n>> >> -\t\trefs/heads/main $orgmain $newmain\n>> >> \t\trefs/heads/next $orgnext $newnext\n>> >> +\t\trefs/heads/main $orgmain $newmain\n>> >> \t\tEOF\n>> >>\n>> >> \t\tcat >post-receive.expect <<-EOF &&\n>> >\n>> > Hm, so the ordering does change now as all deletes will now be listed\n>> > before the updates. We don't make any guarantees about how these are\n>> > sorted, but it makes me a bit uneasy to see this change. Can we avoid\n>> > this change in behaviour somehow?\n>>\n>> Good eyes.\n>>\n>> I was wondering about the \"git push -v\" reporting and was happy that\n>> the order there follows the order the pushing side listed refs and\n>> the reordering on the receiving end would not have any effect. The\n>> hooks on the receiving end can indeed observe this change.\n>>\n>> They can observe, but can they notice? If the pusher listed refspec\n>> elements for deletions first before creations and modifications on\n>> their command line, that would be what the hooks see. They do not\n>> know what the original \"push\" said so they have nothing to compare\n>> and complain.\n>>\n>> Ahhh, but humans that control the both ends may notice and complain.\n>>\n>> OK, I think I agree with you that it is worth to at least spend some\n>> brain cycles thinking about avoiding the behaviour change.\n>\n> Hm. The thing I didn't realize is that the changed output order is for\n> the \"update\" hook. I thought it was for the \"post-receive\" hook, where I\n> do expect that ordering may matter as you see the whole picture of all\n> refs that have been updated. But for the \"update\" hook I think it is\n> sensible to change the ordering -- after all, the order of updates does\n> change a bit now. Furthermore, the \"update\" hook itself doesn't have the\n> complete picture anyway, so it's way less likely that anybody relies on\n> the ordering.\n>\n> Patrick\n\nYou're correct. I still think it'd be nice to have the 'update' hook\nalso be in the same order. But like you said, its okay. I do plan to\ntake up conflict resolution within transactions eventually, so that\nwould also fix this. So I'm going to leave this as is for now.\n\nKarthik\n"},{"id":"519737","messageId":"20250605-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v2-0-26cd05b8a79e@gmail.com","threadId":"63560","inReplyTo":"20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-0-903d1db3f10e@gmail.com","subject":"[PATCH v2 0/2] refs: fix some bugs with batched-updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-05T08:19:53Z","receivedAt":"2025-06-05T08:20:01Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08) we introduced a mechanism to batch reference updates. The\nidea being that we wanted to batch updates similar to a transaction, but\nallow certain updates to fail. This would help reference backends\noptimize such operations and also remove the overhead of processing\nupdates individually. Especially for the reftable backend, where\nbatching updates would ensure that the autocompaction would only occur\nat the end of the batch instead of after every reference update.\n\nAs of 9d2962a7c4 (receive-pack: use batched reference updates,\n2025-05-19) we also updated the 'git-fetch(1)' and 'git-receive-pack(1)'\ncommand to use batched reference updates. This series fixes some bugs\nthat we found at GitLab by running our Gitaly service with the `next`\nbuild of Git.\n\nThe first being in the files backend, which missed skipping over failed\nupdates in certain flows. When certain individual updates fail, we mark\nthem as such. However, we missed skipping over such failed updates,\nwhich can cause a SEGFAULT.\n\nThe other is in the git-receive-pack(1) implementation when a user\npushes multiple branches such as:\n\n delete refs/heads/branch/conflict\n create refs/heads/branch\n\nBefore using batched updates, the references would be applied\nsequentially and hence no conflicts would arise. With batched updates,\nwhile the first update applies, the second fails due to F/D conflict. A\nsimilar issue was present in 'git-fetch(1)' and was fixed by using\nseparating out reference pruning into a separate transaction. Apply a\nsimilar mechanism for 'git-receive-pack(1)' and separate out reference\ndeletions into its own batch.\n\nThis is based off master 7014b55638 (A bit more topics for -rc1,\n2025-05-30), with the changes from kn/fetch-push-bulk-ref-update merged\nin.\n\n---\nChanges in v2:\n- Modify the test in the first commit to no longer do a quiet grep,\n and some more tests.\n- Remove the second commit as it was unnecessary.\n- Modify the commit message in the last commit, to also talk about how\n we now use 2 transactions at minimum but this is still better than\n before.\n- Modify the logic in the last commit to no longer use an XOR and\n instead loop over the two different scenarios (deletion updates, other\n updates).\n- 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@gmail.com\n\n---\n builtin/receive-pack.c | 90 +++++++++++++++++++++++++---------------\n refs/files-backend.c | 7 ++++\n t/t1400-update-ref.sh | 45 ++++++++++++++++++++\n t/t1416-ref-transaction-hooks.sh | 2 +\n t/t5516-fetch-push.sh | 17 ++++++--\n 5 files changed, 123 insertions(+), 38 deletions(-)\n\nKarthik Nayak (2):\n refs/files: skip updates with errors in batched updates\n receive-pack: handle reference deletions separately\n\nRange-diff versus v1:\n\n1: 1c0ea8e209 ! 1: 65bcb5d2cf refs/files: skip updates with errors in batched updates\n @@ t/t1400-update-ref.sh: do\n \t\t)\n \t'\n +\n ++\ttest_expect_success \"stdin $type batch-updates delete incorrect symbolic ref\" '\n ++\t\tgit init repo &&\n ++\t\ttest_when_finished \"rm -fr repo\" &&\n ++\t\t(\n ++\t\t\tcd repo &&\n ++\t\t\ttest_commit c1 &&\n ++\t\t\thead=$(git rev-parse HEAD) &&\n ++\t\t\tgit symbolic-ref refs/heads/symbolic refs/heads/non-existent &&\n ++\n ++\t\t\tformat_command $type \"delete refs/heads/symbolic\" \"$head\" >stdin &&\n ++\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n ++\t\t\ttest_grep \"reference does not exist\" stdout\n ++\t\t)\n ++\t'\n ++\n ++\ttest_expect_success \"stdin $type batch-updates delete with incorrect old_oid\" '\n ++\t\tgit init repo &&\n ++\t\ttest_when_finished \"rm -fr repo\" &&\n ++\t\t(\n ++\t\t\tcd repo &&\n ++\t\t\ttest_commit c1 &&\n ++\t\t\tgit branch new-branch &&\n ++\t\t\ttest_commit c2 &&\n ++\t\t\thead=$(git rev-parse HEAD) &&\n ++\n ++\t\t\tformat_command $type \"delete refs/heads/new-branch\" \"$head\" >stdin &&\n ++\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n ++\t\t\ttest_grep \"incorrect old value provided\" stdout\n ++\t\t)\n ++\t'\n ++\n +\ttest_expect_success \"stdin $type batch-updates delete non-existent ref\" '\n +\t\tgit init repo &&\n +\t\ttest_when_finished \"rm -fr repo\" &&\n @@ t/t1400-update-ref.sh: do\n +\n +\t\t\tformat_command $type \"delete refs/heads/non-existent\" \"$head\" >stdin &&\n +\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n -+\t\t\ttest_grep -q \"reference does not exist\" stdout\n ++\t\t\ttest_grep \"reference does not exist\" stdout\n +\t\t)\n +\t'\n done\n2: 5c21a3770e < -: ---------- t5516: use double quotes for tests with variables\n3: 38eb79bd41 ! 2: e65b29b5f1 receive-pack: handle reference deletions separately\n @@ Commit message\n similar mechanism for 'git-receive-pack(1)' and separate out reference\n deletions into its own batch.\n \n + This means 'git-receive-pack(1)' will now use exactly two transactions,\n + whereas before using batched updates it would use _at least_ two\n + transactions. So using batched updates is the still the better option.\n +\n Add a test to validate this behavior.\n \n Signed-off-by: Karthik Nayak \n \n ## builtin/receive-pack.c ##\n -@@ builtin/receive-pack.c: static void ref_transaction_rejection_handler(const char *refname,\n +@@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *commands,\n + \tconst char *reported_error = NULL;\n + \tstruct strmap failed_refs = STRMAP_INIT;\n + \n +-\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n +-\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n +-\tif (!transaction) {\n +-\t\trp_error(\"%s\", err.buf);\n +-\t\tstrbuf_reset(&err);\n +-\t\treported_error = \"transaction failed to start\";\n +-\t\tgoto failure;\n +-\t}\n ++\t/*\n ++\t * Reference updates, where F/D conflicts shouldn't arise due to\n ++\t * one reference being deleted, while the other being created\n ++\t * are treated as conflicts in batched updates. This is because\n ++\t * we don't do conflict resolution inside a transaction. To\n ++\t * mitigate this, delete references in a separate batch.\n ++\t */\n ++\tenum processing_phase {\n ++\t\tPHASE_DELETIONS,\n ++\t\tPHASE_OTHERS\n ++\t};\n + \n +-\tfor (cmd = commands; cmd; cmd = cmd->next) {\n +-\t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n +-\t\t\tcontinue;\n ++\tfor (int phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {\n ++\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n ++\t\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n ++\t\tif (!transaction) {\n ++\t\t\trp_error(\"%s\", err.buf);\n ++\t\t\tstrbuf_reset(&err);\n ++\t\t\treported_error = \"transaction failed to s1tart\";\n ++\t\t\tgoto failure;\n ++\t\t}\n + \n +-\t\tcmd->error_string = update(cmd, si);\n +-\t}\n ++\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n ++\t\t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n ++\t\t\t\tcontinue;\n + \n +-\tif (ref_transaction_commit(transaction, &err)) {\n +-\t\trp_error(\"%s\", err.buf);\n +-\t\treported_error = \"failed to update refs\";\n +-\t\tgoto failure;\n +-\t}\n ++\t\t\tif (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))\n ++\t\t\t\tcontinue;\n ++\t\t\telse if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))\n ++\t\t\t\tcontinue;\n + \n +-\tref_transaction_for_each_rejected_update(transaction,\n +-\t\t\t\t\t\t ref_transaction_rejection_handler,\n +-\t\t\t\t\t\t &failed_refs);\n ++\t\t\tcmd->error_string = update(cmd, si);\n ++\t\t}\n + \n +-\tif (strmap_empty(&failed_refs))\n +-\t\tgoto cleanup;\n ++\t\tif (ref_transaction_commit(transaction, &err)) {\n ++\t\t\trp_error(\"%s\", err.buf);\n ++\t\t\treported_error = \"failed to update refs\";\n ++\t\t\tgoto failure;\n ++\t\t}\n + \n +-failure:\n +-\tfor (cmd = commands; cmd; cmd = cmd->next) {\n +-\t\tif (reported_error)\n +-\t\t\tcmd->error_string = reported_error;\n +-\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n +-\t\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n +-\t}\n ++\t\tref_transaction_for_each_rejected_update(transaction,\n ++\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n ++\t\t\t\t\t\t\t &failed_refs);\n + \n +-cleanup:\n +-\tref_transaction_free(transaction);\n +-\tstrmap_clear(&failed_refs, 0);\n +-\tstrbuf_release(&err);\n ++\t\tif (strmap_empty(&failed_refs))\n ++\t\t\tgoto cleanup;\n ++\n ++\tfailure:\n ++\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n ++\t\t\tif (reported_error)\n ++\t\t\t\tcmd->error_string = reported_error;\n ++\t\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n ++\t\t\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n ++\t\t}\n ++\n ++\tcleanup:\n ++\t\tref_transaction_free(transaction);\n ++\t\tstrmap_clear(&failed_refs, 0);\n ++\t\tstrbuf_release(&err);\n ++\t}\n }\n \n - static void execute_commands_non_atomic(struct command *commands,\n --\t\t\t\t\tstruct shallow_info *si)\n -+\t\t\t\t\tstruct shallow_info *si,\n -+\t\t\t\t\tint only_deletions)\n - {\n - \tstruct command *cmd;\n - \tstruct strbuf err = STRBUF_INIT;\n -@@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *commands,\n - \tfor (cmd = commands; cmd; cmd = cmd->next) {\n - \t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n - \t\t\tcontinue;\n -+\t\tif (only_deletions ^ is_null_oid(&cmd->new_oid))\n -+\t\t\tcontinue;\n - \n - \t\tcmd->error_string = update(cmd, si);\n - \t}\n + static void execute_commands_atomic(struct command *commands,\n @@ builtin/receive-pack.c: static void execute_commands(struct command *commands,\n \t/*\n \t * If there is no command ready to run, should return directly to destroy\n @@ builtin/receive-pack.c: static void execute_commands(struct command *commands,\n \t */\n \tfor (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)\n \t\t; /* nothing */\n -@@ builtin/receive-pack.c: static void execute_commands(struct command *commands,\n - \t\t\t (cmd->run_proc_receive || use_atomic))\n - \t\t\t\tcmd->error_string = \"fail to run proc-receive hook\";\n - \n --\tif (use_atomic)\n -+\tif (use_atomic) {\n - \t\texecute_commands_atomic(commands, si);\n --\telse\n --\t\texecute_commands_non_atomic(commands, si);\n -+\t} else {\n -+\t\t/*\n -+\t\t * Reference updates, where F/D conflicts shouldn't arise due to\n -+\t\t * one reference being deleted, while the other being created\n -+\t\t * are treated as conflicts in batched updates. This is because\n -+\t\t * we don't do conflict resolution inside a transaction. To\n -+\t\t * mitigate this, delete references in a separate batch.\n -+\t\t */\n -+\t\texecute_commands_non_atomic(commands, si, 1);\n -+\t\texecute_commands_non_atomic(commands, si, 0);\n -+\t}\n - \n - \tif (shallow_update)\n - \t\tBUG_if_skipped_connectivity_check(commands, si);\n \n ## t/t1416-ref-transaction-hooks.sh ##\n @@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'interleaving hook calls succeed' '\n\n\nbase-commit: 931c39f05e078e0df968a439379cb04b5c4666ef\nchange-id: 20250528-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-24ff53680144\n\nThanks\n- Karthik\n\n"},{"id":"519738","messageId":"20250605-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v2-1-26cd05b8a79e@gmail.com","threadId":"63560","inReplyTo":"20250605-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v2-0-26cd05b8a79e@gmail.com","subject":"[PATCH v2 1/2] refs/files: skip updates with errors in batched updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-05T08:19:54Z","receivedAt":"2025-06-05T08:20:02Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08) introduced support for batched reference updates. This\nallows users to batch updates together, while allowing some of the\nupdates to fail.\n\nUnder the hood, batched updates use the reference transaction mechanism.\nEach update which fails is marked as such. Any failed updates must be\nskipped over in the rest of the code, as they wouldn't apply any more.\nIn two of the loops within 'files_transaction_finish()' of the files\nbackend, the failed updates aren't skipped over. This can cause a\nSEGFAULT otherwise. Add the missing skips and a test to validate the\nsame.\n\nSigned-off-by: Karthik Nayak \n---\n refs/files-backend.c | 7 +++++++\n t/t1400-update-ref.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4d1f65a57a..c4a0f29072 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t */\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n+\n+\t\tif (update->rejection_err)\n+\t\t\tcontinue;\n+\n \t\tif (update->flags & REF_DELETING &&\n \t\t !(update->flags & REF_LOG_ONLY) &&\n \t\t !(update->flags & REF_IS_PRUNING)) {\n@@ -3239,6 +3243,9 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t\tstruct ref_update *update = transaction->updates[i];\n \t\tstruct ref_lock *lock = update->backend_data;\n \n+\t\tif (update->rejection_err)\n+\t\t\tcontinue;\n+\n \t\tif (update->flags & REF_DELETING &&\n \t\t !(update->flags & REF_LOG_ONLY)) {\n \t\t\tupdate->flags |= REF_DELETED_RMDIR;\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex d29d23cb89..ca7eee7de2 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -2299,6 +2299,51 @@ do\n \t\t\ttest_grep -q \"refname conflict\" stdout\n \t\t)\n \t'\n+\n+\ttest_expect_success \"stdin $type batch-updates delete incorrect symbolic ref\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit c1 &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\t\t\tgit symbolic-ref refs/heads/symbolic refs/heads/non-existent &&\n+\n+\t\t\tformat_command $type \"delete refs/heads/symbolic\" \"$head\" >stdin &&\n+\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n+\t\t\ttest_grep \"reference does not exist\" stdout\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"stdin $type batch-updates delete with incorrect old_oid\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit c1 &&\n+\t\t\tgit branch new-branch &&\n+\t\t\ttest_commit c2 &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\n+\t\t\tformat_command $type \"delete refs/heads/new-branch\" \"$head\" >stdin &&\n+\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n+\t\t\ttest_grep \"incorrect old value provided\" stdout\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"stdin $type batch-updates delete non-existent ref\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit commit &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\n+\t\t\tformat_command $type \"delete refs/heads/non-existent\" \"$head\" >stdin &&\n+\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n+\t\t\ttest_grep \"reference does not exist\" stdout\n+\t\t)\n+\t'\n done\n \n test_expect_success 'update-ref should also create reflog for HEAD' '\n\n-- \n2.49.0\n\n"},{"id":"519739","messageId":"20250605-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v2-2-26cd05b8a79e@gmail.com","threadId":"63560","inReplyTo":"20250605-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v2-0-26cd05b8a79e@gmail.com","subject":"[PATCH v2 2/2] receive-pack: handle reference deletions separately","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-05T08:19:55Z","receivedAt":"2025-06-05T08:20:02Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)\nwe updated the 'git-receive-pack(1)' command to use batched reference\nupdates. One edge case which was missed during this implementation was\nwhen a user pushes multiple branches such as:\n\n delete refs/heads/branch/conflict\n create refs/heads/branch\n\nBefore using batched updates, the references would be applied\nsequentially and hence no conflicts would arise. With batched updates,\nwhile the first update applies, the second fails due to F/D conflict. A\nsimilar issue was present in 'git-fetch(1)' and was fixed by using\nseparating out reference pruning into a separate transaction. Apply a\nsimilar mechanism for 'git-receive-pack(1)' and separate out reference\ndeletions into its own batch.\n\nThis means 'git-receive-pack(1)' will now use exactly two transactions,\nwhereas before using batched updates it would use _at least_ two\ntransactions. So using batched updates is the still the better option.\n\nAdd a test to validate this behavior.\n\nSigned-off-by: Karthik Nayak \n---\n builtin/receive-pack.c | 90 +++++++++++++++++++++++++---------------\n t/t1416-ref-transaction-hooks.sh | 2 +\n t/t5516-fetch-push.sh | 17 ++++++--\n 3 files changed, 71 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9e3cfb85cf..34db4377ca 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1867,47 +1867,66 @@ static void execute_commands_non_atomic(struct command *commands,\n \tconst char *reported_error = NULL;\n \tstruct strmap failed_refs = STRMAP_INIT;\n \n-\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n-\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n-\tif (!transaction) {\n-\t\trp_error(\"%s\", err.buf);\n-\t\tstrbuf_reset(&err);\n-\t\treported_error = \"transaction failed to start\";\n-\t\tgoto failure;\n-\t}\n+\t/*\n+\t * Reference updates, where F/D conflicts shouldn't arise due to\n+\t * one reference being deleted, while the other being created\n+\t * are treated as conflicts in batched updates. This is because\n+\t * we don't do conflict resolution inside a transaction. To\n+\t * mitigate this, delete references in a separate batch.\n+\t */\n+\tenum processing_phase {\n+\t\tPHASE_DELETIONS,\n+\t\tPHASE_OTHERS\n+\t};\n \n-\tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n-\t\t\tcontinue;\n+\tfor (int phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {\n+\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n+\t\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n+\t\tif (!transaction) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\tstrbuf_reset(&err);\n+\t\t\treported_error = \"transaction failed to s1tart\";\n+\t\t\tgoto failure;\n+\t\t}\n \n-\t\tcmd->error_string = update(cmd, si);\n-\t}\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n+\t\t\t\tcontinue;\n \n-\tif (ref_transaction_commit(transaction, &err)) {\n-\t\trp_error(\"%s\", err.buf);\n-\t\treported_error = \"failed to update refs\";\n-\t\tgoto failure;\n-\t}\n+\t\t\tif (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))\n+\t\t\t\tcontinue;\n+\t\t\telse if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))\n+\t\t\t\tcontinue;\n \n-\tref_transaction_for_each_rejected_update(transaction,\n-\t\t\t\t\t\t ref_transaction_rejection_handler,\n-\t\t\t\t\t\t &failed_refs);\n+\t\t\tcmd->error_string = update(cmd, si);\n+\t\t}\n \n-\tif (strmap_empty(&failed_refs))\n-\t\tgoto cleanup;\n+\t\tif (ref_transaction_commit(transaction, &err)) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\treported_error = \"failed to update refs\";\n+\t\t\tgoto failure;\n+\t\t}\n \n-failure:\n-\tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (reported_error)\n-\t\t\tcmd->error_string = reported_error;\n-\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n-\t\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n-\t}\n+\t\tref_transaction_for_each_rejected_update(transaction,\n+\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t\t &failed_refs);\n \n-cleanup:\n-\tref_transaction_free(transaction);\n-\tstrmap_clear(&failed_refs, 0);\n-\tstrbuf_release(&err);\n+\t\tif (strmap_empty(&failed_refs))\n+\t\t\tgoto cleanup;\n+\n+\tfailure:\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\t\tif (reported_error)\n+\t\t\t\tcmd->error_string = reported_error;\n+\t\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n+\t\t\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n+\t\t}\n+\n+\tcleanup:\n+\t\tref_transaction_free(transaction);\n+\t\tstrmap_clear(&failed_refs, 0);\n+\t\tstrbuf_release(&err);\n+\t}\n }\n \n static void execute_commands_atomic(struct command *commands,\n@@ -2024,6 +2043,9 @@ static void execute_commands(struct command *commands,\n \t/*\n \t * If there is no command ready to run, should return directly to destroy\n \t * temporary data in the quarantine area.\n+\t *\n+\t * Check if any reference deletions exist, these are batched together in\n+\t * a separate transaction to avoid F/D conflicts with other updates.\n \t */\n \tfor (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)\n \t\t; /* nothing */\ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex d91dd3a3b5..b2aaa1908f 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -119,6 +119,8 @@ test_expect_success 'interleaving hook calls succeed' '\n \tEOF\n \n \tcat >expect <<-EOF &&\n+\t\thooks/reference-transaction prepared\n+\t\thooks/reference-transaction committed\n \t\thooks/update refs/tags/PRE $ZERO_OID $PRE_OID\n \t\thooks/update refs/tags/POST $ZERO_OID $POST_OID\n \t\thooks/reference-transaction prepared\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex dabcc5f811..1649667441 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $orgmain $newmain\n \t\trefs/heads/next $orgnext $newnext\n+\t\trefs/heads/main $orgmain $newmain\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n@@ -808,8 +808,8 @@ test_expect_success 'deletion of a non-existent ref is not fed to post-receive a\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $orgmain $newmain\n \t\trefs/heads/nonexistent $ZERO_OID $ZERO_OID\n+\t\trefs/heads/main $orgmain $newmain\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n@@ -868,10 +868,10 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $orgmain $newmain\n \t\trefs/heads/next $orgnext $newnext\n-\t\trefs/heads/seen $orgseen $newseen\n \t\trefs/heads/nonexistent $ZERO_OID $ZERO_OID\n+\t\trefs/heads/main $orgmain $newmain\n+\t\trefs/heads/seen $orgseen $newseen\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n@@ -1909,4 +1909,13 @@ test_expect_success 'push with config push.useBitmaps' '\n \t\t--thin --delta-base-offset -q --no-use-bitmap-index In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)\n> we updated the 'git-receive-pack(1)' command to use batched reference\n> updates. One edge case which was missed during this implementation was\n> when a user pushes multiple branches such as:\n> \n> delete refs/heads/branch/conflict\n> create refs/heads/branch\n> \n> Before using batched updates, the references would be applied\n> sequentially and hence no conflicts would arise. With batched updates,\n> while the first update applies, the second fails due to F/D conflict. A\n> similar issue was present in 'git-fetch(1)' and was fixed by using\n> separating out reference pruning into a separate transaction. Apply a\n> similar mechanism for 'git-receive-pack(1)' and separate out reference\n> deletions into its own batch.\n> \n> This means 'git-receive-pack(1)' will now use exactly two transactions,\n> whereas before using batched updates it would use _at least_ two\n> transactions. So using batched updates is the still the better option.\n\ns/the still the/still the/\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 9e3cfb85cf..34db4377ca 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1867,47 +1867,66 @@ static void execute_commands_non_atomic(struct command *commands,\n> \tconst char *reported_error = NULL;\n> \tstruct strmap failed_refs = STRMAP_INIT;\n> \n> -\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n> -\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n> -\tif (!transaction) {\n> -\t\trp_error(\"%s\", err.buf);\n> -\t\tstrbuf_reset(&err);\n> -\t\treported_error = \"transaction failed to start\";\n> -\t\tgoto failure;\n> -\t}\n> +\t/*\n> +\t * Reference updates, where F/D conflicts shouldn't arise due to\n> +\t * one reference being deleted, while the other being created\n> +\t * are treated as conflicts in batched updates. This is because\n> +\t * we don't do conflict resolution inside a transaction. To\n> +\t * mitigate this, delete references in a separate batch.\n> +\t */\n> +\tenum processing_phase {\n> +\t\tPHASE_DELETIONS,\n> +\t\tPHASE_OTHERS\n> +\t};\n> \n> -\tfor (cmd = commands; cmd; cmd = cmd->next) {\n> -\t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n> -\t\t\tcontinue;\n> +\tfor (int phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {\n\ns/int/enum processing_phase/\n\nDoesn't make any difference, but it feels a bit cleaner.\n\n> +\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n> +\t\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n> +\t\tif (!transaction) {\n> +\t\t\trp_error(\"%s\", err.buf);\n> +\t\t\tstrbuf_reset(&err);\n> +\t\t\treported_error = \"transaction failed to s1tart\";\n> +\t\t\tgoto failure;\n> +\t\t}\n\nSo if the transaction doesn't contain any deletions we'd now commit an\nempty transaction. The same is true the other way round, in case there\nare only deletions. Do we maybe want to skip phases when there is no\nmatch? Ideally, we wouldn't even be starting a transaction.\n\nWe could for example skip forward to the first command that we would\nhave to queue. If there is no such command we continue the loop, if\nthere is we can remember that command, begin the transaction and start\nqueueing from there.\n\n> @@ -2024,6 +2043,9 @@ static void execute_commands(struct command *commands,\n> \t/*\n> \t * If there is no command ready to run, should return directly to destroy\n> \t * temporary data in the quarantine area.\n> +\t *\n> +\t * Check if any reference deletions exist, these are batched together in\n> +\t * a separate transaction to avoid F/D conflicts with other updates.\n> \t */\n\nIs this comment still accurate?\n\n> \tfor (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)\n> \t\t; /* nothing */\n> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\n> index d91dd3a3b5..b2aaa1908f 100755\n> --- a/t/t1416-ref-transaction-hooks.sh\n> +++ b/t/t1416-ref-transaction-hooks.sh\n> @@ -119,6 +119,8 @@ test_expect_success 'interleaving hook calls succeed' '\n> \tEOF\n> \n> \tcat >expect <<-EOF &&\n> +\t\thooks/reference-transaction prepared\n> +\t\thooks/reference-transaction committed\n\nYeah, this shows the empty commits indeed.\n\nPatrick\n"},{"id":"519756","messageId":"CAOLa=ZQVyU63pWgaHONj2bCdCw3XmXLDVqANWkw9TkVzPQshoQ@mail.gmail.com","threadId":"63560","inReplyTo":"aEFZnmjoxjopv2xF@pks.im","subject":"Re: [PATCH v2 2/2] receive-pack: handle reference deletions separately","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-05T09:08:30Z","receivedAt":"2025-06-05T09:08:46Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Thu, Jun 05, 2025 at 10:19:55AM +0200, Karthik Nayak wrote:\n>> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)\n>> we updated the 'git-receive-pack(1)' command to use batched reference\n>> updates. One edge case which was missed during this implementation was\n>> when a user pushes multiple branches such as:\n>>\n>> delete refs/heads/branch/conflict\n>> create refs/heads/branch\n>>\n>> Before using batched updates, the references would be applied\n>> sequentially and hence no conflicts would arise. With batched updates,\n>> while the first update applies, the second fails due to F/D conflict. A\n>> similar issue was present in 'git-fetch(1)' and was fixed by using\n>> separating out reference pruning into a separate transaction. Apply a\n>> similar mechanism for 'git-receive-pack(1)' and separate out reference\n>> deletions into its own batch.\n>>\n>> This means 'git-receive-pack(1)' will now use exactly two transactions,\n>> whereas before using batched updates it would use _at least_ two\n>> transactions. So using batched updates is the still the better option.\n>\n> s/the still the/still the/\n>\n\nWill fix.\n\n>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n>> index 9e3cfb85cf..34db4377ca 100644\n>> --- a/builtin/receive-pack.c\n>> +++ b/builtin/receive-pack.c\n>> @@ -1867,47 +1867,66 @@ static void execute_commands_non_atomic(struct command *commands,\n>> \tconst char *reported_error = NULL;\n>> \tstruct strmap failed_refs = STRMAP_INIT;\n>>\n>> -\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n>> -\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n>> -\tif (!transaction) {\n>> -\t\trp_error(\"%s\", err.buf);\n>> -\t\tstrbuf_reset(&err);\n>> -\t\treported_error = \"transaction failed to start\";\n>> -\t\tgoto failure;\n>> -\t}\n>> +\t/*\n>> +\t * Reference updates, where F/D conflicts shouldn't arise due to\n>> +\t * one reference being deleted, while the other being created\n>> +\t * are treated as conflicts in batched updates. This is because\n>> +\t * we don't do conflict resolution inside a transaction. To\n>> +\t * mitigate this, delete references in a separate batch.\n>> +\t */\n>> +\tenum processing_phase {\n>> +\t\tPHASE_DELETIONS,\n>> +\t\tPHASE_OTHERS\n>> +\t};\n>>\n>> -\tfor (cmd = commands; cmd; cmd = cmd->next) {\n>> -\t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n>> -\t\t\tcontinue;\n>> +\tfor (int phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {\n>\n> s/int/enum processing_phase/\n>\n> Doesn't make any difference, but it feels a bit cleaner.\n>\n\nYeah, agreed.\n\n>> +\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n>> +\t\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n>> +\t\tif (!transaction) {\n>> +\t\t\trp_error(\"%s\", err.buf);\n>> +\t\t\tstrbuf_reset(&err);\n>> +\t\t\treported_error = \"transaction failed to s1tart\";\n>> +\t\t\tgoto failure;\n>> +\t\t}\n>\n> So if the transaction doesn't contain any deletions we'd now commit an\n> empty transaction. The same is true the other way round, in case there\n> are only deletions. Do we maybe want to skip phases when there is no\n> match? Ideally, we wouldn't even be starting a transaction.\n>\n> We could for example skip forward to the first command that we would\n> have to queue. If there is no such command we continue the loop, if\n> there is we can remember that command, begin the transaction and start\n> queueing from there.\n>\n\nI think that's a fair point, and very simple to implement. I'll add that\nin. Thanks.\n\n>> @@ -2024,6 +2043,9 @@ static void execute_commands(struct command *commands,\n>> \t/*\n>> \t * If there is no command ready to run, should return directly to destroy\n>> \t * temporary data in the quarantine area.\n>> +\t *\n>> +\t * Check if any reference deletions exist, these are batched together in\n>> +\t * a separate transaction to avoid F/D conflicts with other updates.\n>> \t */\n>\n> Is this comment still accurate?\n>\n\nNope, will remove this.\n\n>> \tfor (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)\n>> \t\t; /* nothing */\n>> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\n>> index d91dd3a3b5..b2aaa1908f 100755\n>> --- a/t/t1416-ref-transaction-hooks.sh\n>> +++ b/t/t1416-ref-transaction-hooks.sh\n>> @@ -119,6 +119,8 @@ test_expect_success 'interleaving hook calls succeed' '\n>> \tEOF\n>>\n>> \tcat >expect <<-EOF &&\n>> +\t\thooks/reference-transaction prepared\n>> +\t\thooks/reference-transaction committed\n>\n> Yeah, this shows the empty commits indeed.\n>\n\nYup, thanks for the quick review.\n\n> Patrick\n"},{"id":"519830","messageId":"20250606-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v3-0-e1c41693bd35@gmail.com","threadId":"63560","inReplyTo":"20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-0-903d1db3f10e@gmail.com","subject":"[PATCH v3 0/2] refs: fix some bugs with batched-updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-06T08:41:09Z","receivedAt":"2025-06-06T08:41:24Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08) we introduced a mechanism to batch reference updates. The\nidea being that we wanted to batch updates similar to a transaction, but\nallow certain updates to fail. This would help reference backends\noptimize such operations and also remove the overhead of processing\nupdates individually. Especially for the reftable backend, where\nbatching updates would ensure that the autocompaction would only occur\nat the end of the batch instead of after every reference update.\n\nAs of 9d2962a7c4 (receive-pack: use batched reference updates,\n2025-05-19) we also updated the 'git-fetch(1)' and 'git-receive-pack(1)'\ncommand to use batched reference updates. This series fixes some bugs\nthat we found at GitLab by running our Gitaly service with the `next`\nbuild of Git.\n\nThe first being in the files backend, which missed skipping over failed\nupdates in certain flows. When certain individual updates fail, we mark\nthem as such. However, we missed skipping over such failed updates,\nwhich can cause a SEGFAULT.\n\nThe other is in the git-receive-pack(1) implementation when a user\npushes multiple branches such as:\n\n delete refs/heads/branch/conflict\n create refs/heads/branch\n\nBefore using batched updates, the references would be applied\nsequentially and hence no conflicts would arise. With batched updates,\nwhile the first update applies, the second fails due to F/D conflict. A\nsimilar issue was present in 'git-fetch(1)' and was fixed by using\nseparating out reference pruning into a separate transaction. Apply a\nsimilar mechanism for 'git-receive-pack(1)' and separate out reference\ndeletions into its own batch.\n\nThis is based off master 7014b55638 (A bit more topics for -rc1,\n2025-05-30), with the changes from kn/fetch-push-bulk-ref-update merged\nin.\n\n---\nChanges in v3:\n- Cleanup the second commit message and remove stale comments.\n- In the second commit, only create the transaction if needed.\n- Link to v2: https://lore.kernel.org/r/20250605-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v2-0-26cd05b8a79e@gmail.com\n\nChanges in v2:\n- Modify the test in the first commit to no longer do a quiet grep,\n and some more tests.\n- Remove the second commit as it was unnecessary.\n- Modify the commit message in the last commit, to also talk about how\n we now use 2 transactions at minimum but this is still better than\n before.\n- Modify the logic in the last commit to no longer use an XOR and\n instead loop over the two different scenarios (deletion updates, other\n updates).\n- 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@gmail.com\n\n---\n builtin/receive-pack.c | 100 ++++++++++++++++++++++++++++++++-----------------\n refs/files-backend.c | 7 ++++\n t/t1400-update-ref.sh | 45 ++++++++++++++++++++++\n t/t5516-fetch-push.sh | 17 +++++++--\n 4 files changed, 131 insertions(+), 38 deletions(-)\n\nKarthik Nayak (2):\n refs/files: skip updates with errors in batched updates\n receive-pack: handle reference deletions separately\n\nRange-diff versus v2:\n\n1: 50f318714b = 1: 695cc3b9d4 refs/files: skip updates with errors in batched updates\n2: d7293b64b0 ! 2: a20bb05dd6 receive-pack: handle reference deletions separately\n @@ Commit message\n similar mechanism for 'git-receive-pack(1)' and separate out reference\n deletions into its own batch.\n \n - This means 'git-receive-pack(1)' will now use exactly two transactions,\n + This means 'git-receive-pack(1)' will now use upto two transactions,\n whereas before using batched updates it would use _at least_ two\n - transactions. So using batched updates is the still the better option.\n + transactions. So using batched updates is still the better option.\n \n Add a test to validate this behavior.\n \n @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *\n -\tfor (cmd = commands; cmd; cmd = cmd->next) {\n -\t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n -\t\t\tcontinue;\n -+\tfor (int phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {\n -+\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n -+\t\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n -+\t\tif (!transaction) {\n -+\t\t\trp_error(\"%s\", err.buf);\n -+\t\t\tstrbuf_reset(&err);\n -+\t\t\treported_error = \"transaction failed to s1tart\";\n -+\t\t\tgoto failure;\n -+\t\t}\n ++\tfor (enum processing_phase phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {\n ++\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n ++\t\t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n ++\t\t\t\tcontinue;\n \n -\t\tcmd->error_string = update(cmd, si);\n -\t}\n -+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n -+\t\t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n ++\t\t\tif (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))\n ++\t\t\t\tcontinue;\n ++\t\t\telse if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))\n +\t\t\t\tcontinue;\n \n -\tif (ref_transaction_commit(transaction, &err)) {\n @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *\n -\t\treported_error = \"failed to update refs\";\n -\t\tgoto failure;\n -\t}\n -+\t\t\tif (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))\n -+\t\t\t\tcontinue;\n -+\t\t\telse if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))\n -+\t\t\t\tcontinue;\n ++\t\t\t/*\n ++\t\t\t * Lazily create a transaction only when we know there are\n ++\t\t\t * updates to be added.\n ++\t\t\t */\n ++\t\t\tif (!transaction) {\n ++\t\t\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n ++\t\t\t\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n ++\t\t\t\tif (!transaction) {\n ++\t\t\t\t\trp_error(\"%s\", err.buf);\n ++\t\t\t\t\tstrbuf_reset(&err);\n ++\t\t\t\t\treported_error = \"transaction failed to s1tart\";\n ++\t\t\t\t\tgoto failure;\n ++\t\t\t\t}\n ++\t\t\t}\n \n -\tref_transaction_for_each_rejected_update(transaction,\n -\t\t\t\t\t\t ref_transaction_rejection_handler,\n @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *\n \n -\tif (strmap_empty(&failed_refs))\n -\t\tgoto cleanup;\n -+\t\tif (ref_transaction_commit(transaction, &err)) {\n -+\t\t\trp_error(\"%s\", err.buf);\n -+\t\t\treported_error = \"failed to update refs\";\n -+\t\t\tgoto failure;\n -+\t\t}\n ++\t\t/*\n ++\t\t * If no transaction was created, there is nothing to commit.\n ++\t\t */\n ++\t\tif (!transaction)\n ++\t\t\tgoto cleanup;\n \n -failure:\n -\tfor (cmd = commands; cmd; cmd = cmd->next) {\n @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *\n -\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n -\t\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n -\t}\n -+\t\tref_transaction_for_each_rejected_update(transaction,\n -+\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n -+\t\t\t\t\t\t\t &failed_refs);\n ++\t\tif (ref_transaction_commit(transaction, &err)) {\n ++\t\t\trp_error(\"%s\", err.buf);\n ++\t\t\treported_error = \"failed to update refs\";\n ++\t\t\tgoto failure;\n ++\t\t}\n \n -cleanup:\n -\tref_transaction_free(transaction);\n -\tstrmap_clear(&failed_refs, 0);\n -\tstrbuf_release(&err);\n ++\t\tref_transaction_for_each_rejected_update(transaction,\n ++\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n ++\t\t\t\t\t\t\t &failed_refs);\n ++\n +\t\tif (strmap_empty(&failed_refs))\n +\t\t\tgoto cleanup;\n +\n @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *\n +\n +\tcleanup:\n +\t\tref_transaction_free(transaction);\n ++\t\ttransaction = NULL;\n +\t\tstrmap_clear(&failed_refs, 0);\n +\t\tstrbuf_release(&err);\n +\t}\n }\n \n static void execute_commands_atomic(struct command *commands,\n -@@ builtin/receive-pack.c: static void execute_commands(struct command *commands,\n - \t/*\n - \t * If there is no command ready to run, should return directly to destroy\n - \t * temporary data in the quarantine area.\n -+\t *\n -+\t * Check if any reference deletions exist, these are batched together in\n -+\t * a separate transaction to avoid F/D conflicts with other updates.\n - \t */\n - \tfor (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)\n - \t\t; /* nothing */\n -\n - ## t/t1416-ref-transaction-hooks.sh ##\n -@@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'interleaving hook calls succeed' '\n - \tEOF\n - \n - \tcat >expect <<-EOF &&\n -+\t\thooks/reference-transaction prepared\n -+\t\thooks/reference-transaction committed\n - \t\thooks/update refs/tags/PRE $ZERO_OID $PRE_OID\n - \t\thooks/update refs/tags/POST $ZERO_OID $POST_OID\n - \t\thooks/reference-transaction prepared\n \n ## t/t5516-fetch-push.sh ##\n @@ t/t5516-fetch-push.sh: test_expect_success 'pushing valid refs triggers post-receive and post-update ho\n\n\nbase-commit: 931c39f05e078e0df968a439379cb04b5c4666ef\nchange-id: 20250528-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-24ff53680144\n\nThanks\n- Karthik\n\n"},{"id":"519831","messageId":"20250606-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v3-1-e1c41693bd35@gmail.com","threadId":"63560","inReplyTo":"20250606-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v3-0-e1c41693bd35@gmail.com","subject":"[PATCH v3 1/2] refs/files: skip updates with errors in batched updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-06T08:41:10Z","receivedAt":"2025-06-06T08:41:29Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08) introduced support for batched reference updates. This\nallows users to batch updates together, while allowing some of the\nupdates to fail.\n\nUnder the hood, batched updates use the reference transaction mechanism.\nEach update which fails is marked as such. Any failed updates must be\nskipped over in the rest of the code, as they wouldn't apply any more.\nIn two of the loops within 'files_transaction_finish()' of the files\nbackend, the failed updates aren't skipped over. This can cause a\nSEGFAULT otherwise. Add the missing skips and a test to validate the\nsame.\n\nSigned-off-by: Karthik Nayak \n---\n refs/files-backend.c | 7 +++++++\n t/t1400-update-ref.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4d1f65a57a..c4a0f29072 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t */\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n+\n+\t\tif (update->rejection_err)\n+\t\t\tcontinue;\n+\n \t\tif (update->flags & REF_DELETING &&\n \t\t !(update->flags & REF_LOG_ONLY) &&\n \t\t !(update->flags & REF_IS_PRUNING)) {\n@@ -3239,6 +3243,9 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t\tstruct ref_update *update = transaction->updates[i];\n \t\tstruct ref_lock *lock = update->backend_data;\n \n+\t\tif (update->rejection_err)\n+\t\t\tcontinue;\n+\n \t\tif (update->flags & REF_DELETING &&\n \t\t !(update->flags & REF_LOG_ONLY)) {\n \t\t\tupdate->flags |= REF_DELETED_RMDIR;\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex d29d23cb89..ca7eee7de2 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -2299,6 +2299,51 @@ do\n \t\t\ttest_grep -q \"refname conflict\" stdout\n \t\t)\n \t'\n+\n+\ttest_expect_success \"stdin $type batch-updates delete incorrect symbolic ref\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit c1 &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\t\t\tgit symbolic-ref refs/heads/symbolic refs/heads/non-existent &&\n+\n+\t\t\tformat_command $type \"delete refs/heads/symbolic\" \"$head\" >stdin &&\n+\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n+\t\t\ttest_grep \"reference does not exist\" stdout\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"stdin $type batch-updates delete with incorrect old_oid\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit c1 &&\n+\t\t\tgit branch new-branch &&\n+\t\t\ttest_commit c2 &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\n+\t\t\tformat_command $type \"delete refs/heads/new-branch\" \"$head\" >stdin &&\n+\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n+\t\t\ttest_grep \"incorrect old value provided\" stdout\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"stdin $type batch-updates delete non-existent ref\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit commit &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\n+\t\t\tformat_command $type \"delete refs/heads/non-existent\" \"$head\" >stdin &&\n+\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n+\t\t\ttest_grep \"reference does not exist\" stdout\n+\t\t)\n+\t'\n done\n \n test_expect_success 'update-ref should also create reflog for HEAD' '\n\n-- \n2.49.0\n\n"},{"id":"519832","messageId":"20250606-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v3-2-e1c41693bd35@gmail.com","threadId":"63560","inReplyTo":"20250606-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v3-0-e1c41693bd35@gmail.com","subject":"[PATCH v3 2/2] receive-pack: handle reference deletions separately","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-06T08:41:11Z","receivedAt":"2025-06-06T08:41:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)\nwe updated the 'git-receive-pack(1)' command to use batched reference\nupdates. One edge case which was missed during this implementation was\nwhen a user pushes multiple branches such as:\n\n delete refs/heads/branch/conflict\n create refs/heads/branch\n\nBefore using batched updates, the references would be applied\nsequentially and hence no conflicts would arise. With batched updates,\nwhile the first update applies, the second fails due to F/D conflict. A\nsimilar issue was present in 'git-fetch(1)' and was fixed by using\nseparating out reference pruning into a separate transaction. Apply a\nsimilar mechanism for 'git-receive-pack(1)' and separate out reference\ndeletions into its own batch.\n\nThis means 'git-receive-pack(1)' will now use upto two transactions,\nwhereas before using batched updates it would use _at least_ two\ntransactions. So using batched updates is still the better option.\n\nAdd a test to validate this behavior.\n\nSigned-off-by: Karthik Nayak \n---\n builtin/receive-pack.c | 100 ++++++++++++++++++++++++++++++++-----------------\n t/t5516-fetch-push.sh | 17 +++++++--\n 2 files changed, 79 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9e3cfb85cf..8ee792d2f8 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1867,47 +1867,79 @@ static void execute_commands_non_atomic(struct command *commands,\n \tconst char *reported_error = NULL;\n \tstruct strmap failed_refs = STRMAP_INIT;\n \n-\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n-\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n-\tif (!transaction) {\n-\t\trp_error(\"%s\", err.buf);\n-\t\tstrbuf_reset(&err);\n-\t\treported_error = \"transaction failed to start\";\n-\t\tgoto failure;\n-\t}\n+\t/*\n+\t * Reference updates, where F/D conflicts shouldn't arise due to\n+\t * one reference being deleted, while the other being created\n+\t * are treated as conflicts in batched updates. This is because\n+\t * we don't do conflict resolution inside a transaction. To\n+\t * mitigate this, delete references in a separate batch.\n+\t */\n+\tenum processing_phase {\n+\t\tPHASE_DELETIONS,\n+\t\tPHASE_OTHERS\n+\t};\n \n-\tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n-\t\t\tcontinue;\n+\tfor (enum processing_phase phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n+\t\t\t\tcontinue;\n \n-\t\tcmd->error_string = update(cmd, si);\n-\t}\n+\t\t\tif (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))\n+\t\t\t\tcontinue;\n+\t\t\telse if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))\n+\t\t\t\tcontinue;\n \n-\tif (ref_transaction_commit(transaction, &err)) {\n-\t\trp_error(\"%s\", err.buf);\n-\t\treported_error = \"failed to update refs\";\n-\t\tgoto failure;\n-\t}\n+\t\t\t/*\n+\t\t\t * Lazily create a transaction only when we know there are\n+\t\t\t * updates to be added.\n+\t\t\t */\n+\t\t\tif (!transaction) {\n+\t\t\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n+\t\t\t\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n+\t\t\t\tif (!transaction) {\n+\t\t\t\t\trp_error(\"%s\", err.buf);\n+\t\t\t\t\tstrbuf_reset(&err);\n+\t\t\t\t\treported_error = \"transaction failed to s1tart\";\n+\t\t\t\t\tgoto failure;\n+\t\t\t\t}\n+\t\t\t}\n \n-\tref_transaction_for_each_rejected_update(transaction,\n-\t\t\t\t\t\t ref_transaction_rejection_handler,\n-\t\t\t\t\t\t &failed_refs);\n+\t\t\tcmd->error_string = update(cmd, si);\n+\t\t}\n \n-\tif (strmap_empty(&failed_refs))\n-\t\tgoto cleanup;\n+\t\t/*\n+\t\t * If no transaction was created, there is nothing to commit.\n+\t\t */\n+\t\tif (!transaction)\n+\t\t\tgoto cleanup;\n \n-failure:\n-\tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (reported_error)\n-\t\t\tcmd->error_string = reported_error;\n-\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n-\t\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n-\t}\n+\t\tif (ref_transaction_commit(transaction, &err)) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\treported_error = \"failed to update refs\";\n+\t\t\tgoto failure;\n+\t\t}\n \n-cleanup:\n-\tref_transaction_free(transaction);\n-\tstrmap_clear(&failed_refs, 0);\n-\tstrbuf_release(&err);\n+\t\tref_transaction_for_each_rejected_update(transaction,\n+\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t\t &failed_refs);\n+\n+\t\tif (strmap_empty(&failed_refs))\n+\t\t\tgoto cleanup;\n+\n+\tfailure:\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\t\tif (reported_error)\n+\t\t\t\tcmd->error_string = reported_error;\n+\t\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n+\t\t\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n+\t\t}\n+\n+\tcleanup:\n+\t\tref_transaction_free(transaction);\n+\t\ttransaction = NULL;\n+\t\tstrmap_clear(&failed_refs, 0);\n+\t\tstrbuf_release(&err);\n+\t}\n }\n \n static void execute_commands_atomic(struct command *commands,\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex dabcc5f811..1649667441 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $orgmain $newmain\n \t\trefs/heads/next $orgnext $newnext\n+\t\trefs/heads/main $orgmain $newmain\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n@@ -808,8 +808,8 @@ test_expect_success 'deletion of a non-existent ref is not fed to post-receive a\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $orgmain $newmain\n \t\trefs/heads/nonexistent $ZERO_OID $ZERO_OID\n+\t\trefs/heads/main $orgmain $newmain\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n@@ -868,10 +868,10 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $orgmain $newmain\n \t\trefs/heads/next $orgnext $newnext\n-\t\trefs/heads/seen $orgseen $newseen\n \t\trefs/heads/nonexistent $ZERO_OID $ZERO_OID\n+\t\trefs/heads/main $orgmain $newmain\n+\t\trefs/heads/seen $orgseen $newseen\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n@@ -1909,4 +1909,13 @@ test_expect_success 'push with config push.useBitmaps' '\n \t\t--thin --delta-base-offset -q --no-use-bitmap-index wrote:\n>\n> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)\n> we updated the 'git-receive-pack(1)' command to use batched reference\n> updates. One edge case which was missed during this implementation was\n> when a user pushes multiple branches such as:\n>\n> delete refs/heads/branch/conflict\n> create refs/heads/branch\n>\n> Before using batched updates, the references would be applied\n> sequentially and hence no conflicts would arise. With batched updates,\n> while the first update applies, the second fails due to F/D conflict.\n\nNit: it looks like \"D/F conflict\" is more often used than \"F/D\nconflict\" in the Git code base:\n\n$ git grep -i 'd/f conflict' | wc -l\n119\n$ git grep -i 'f/d conflict' | wc -l\n7\n\n> A\n> similar issue was present in 'git-fetch(1)' and was fixed by using\n> separating out reference pruning into a separate transaction. Apply a\n\nMaybe: s/using separating out/separating out/\n\n> similar mechanism for 'git-receive-pack(1)' and separate out reference\n> deletions into its own batch.\n>\n> This means 'git-receive-pack(1)' will now use upto two transactions,\n> whereas before using batched updates it would use _at least_ two\n> transactions. So using batched updates is still the better option.\n>\n> Add a test to validate this behavior.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> builtin/receive-pack.c | 100 ++++++++++++++++++++++++++++++++-----------------\n> t/t5516-fetch-push.sh | 17 +++++++--\n> 2 files changed, 79 insertions(+), 38 deletions(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 9e3cfb85cf..8ee792d2f8 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1867,47 +1867,79 @@ static void execute_commands_non_atomic(struct command *commands,\n> const char *reported_error = NULL;\n> struct strmap failed_refs = STRMAP_INIT;\n>\n> - transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n> - REF_TRANSACTION_ALLOW_FAILURE, &err);\n> - if (!transaction) {\n> - rp_error(\"%s\", err.buf);\n> - strbuf_reset(&err);\n> - reported_error = \"transaction failed to start\";\n> - goto failure;\n> - }\n> + /*\n> + * Reference updates, where F/D conflicts shouldn't arise due to\n\nNit: here also maybe \"D/F conflicts\" is more standard.\n\n> + * one reference being deleted, while the other being created\n> + * are treated as conflicts in batched updates. This is because\n> + * we don't do conflict resolution inside a transaction. To\n> + * mitigate this, delete references in a separate batch.\n> + */\n> + enum processing_phase {\n> + PHASE_DELETIONS,\n> + PHASE_OTHERS\n> + };\n>\n> - for (cmd = commands; cmd; cmd = cmd->next) {\n> - if (!should_process_cmd(cmd) || cmd->run_proc_receive)\n> - continue;\n> + for (enum processing_phase phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {\n> + for (cmd = commands; cmd; cmd = cmd->next) {\n> + if (!should_process_cmd(cmd) || cmd->run_proc_receive)\n> + continue;\n>\n> - cmd->error_string = update(cmd, si);\n> - }\n> + if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))\n> + continue;\n> + else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))\n> + continue;\n>\n> - if (ref_transaction_commit(transaction, &err)) {\n> - rp_error(\"%s\", err.buf);\n> - reported_error = \"failed to update refs\";\n> - goto failure;\n> - }\n> + /*\n> + * Lazily create a transaction only when we know there are\n> + * updates to be added.\n> + */\n> + if (!transaction) {\n> + transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n> + REF_TRANSACTION_ALLOW_FAILURE, &err);\n> + if (!transaction) {\n> + rp_error(\"%s\", err.buf);\n> + strbuf_reset(&err);\n> + reported_error = \"transaction failed to s1tart\";\n\ns/s1tart/start/\n\n> + goto failure;\n> + }\n> + }\n>\n> - ref_transaction_for_each_rejected_update(transaction,\n> - ref_transaction_rejection_handler,\n> - &failed_refs);\n> + cmd->error_string = update(cmd, si);\n> + }\n>\n> - if (strmap_empty(&failed_refs))\n> - goto cleanup;\n> + /*\n> + * If no transaction was created, there is nothing to commit.\n> + */\n\nNit: the comment needs a single line, so maybe:\n\n /* No transaction, so nothing to commit */\n\n> + if (!transaction)\n> + goto cleanup;\n>\n> -failure:\n> - for (cmd = commands; cmd; cmd = cmd->next) {\n> - if (reported_error)\n> - cmd->error_string = reported_error;\n> - else if (strmap_contains(&failed_refs, cmd->ref_name))\n> - cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n> - }\n> + if (ref_transaction_commit(transaction, &err)) {\n> + rp_error(\"%s\", err.buf);\n> + reported_error = \"failed to update refs\";\n> + goto failure;\n> + }\n>\n> -cleanup:\n> - ref_transaction_free(transaction);\n> - strmap_clear(&failed_refs, 0);\n> - strbuf_release(&err);\n> + ref_transaction_for_each_rejected_update(transaction,\n> + ref_transaction_rejection_handler,\n> + &failed_refs);\n> +\n> + if (strmap_empty(&failed_refs))\n> + goto cleanup;\n> +\n> + failure:\n\nThis label looks indented while previously it was right at the start\nof the line. Not sure if we have a standard for that, but a few quick\ngreps seems to show that goto labels are most often at the start of\nthe line.\n\n> + for (cmd = commands; cmd; cmd = cmd->next) {\n> + if (reported_error)\n> + cmd->error_string = reported_error;\n> + else if (strmap_contains(&failed_refs, cmd->ref_name))\n> + cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n> + }\n> +\n> + cleanup:\n\nIdem for how this label is indented.\n\n> + ref_transaction_free(transaction);\n> + transaction = NULL;\n> + strmap_clear(&failed_refs, 0);\n> + strbuf_release(&err);\n> + }\n> }\n"},{"id":"520190","messageId":"xmqq8qlwd775.fsf@gitster.g","threadId":"63560","inReplyTo":"CAP8UFD0Az9YUd7tHbCWjrZ5bTv1V_0RZ2azasPmOrpf+ARMjug@mail.gmail.com","subject":"Re: [PATCH v3 2/2] receive-pack: handle reference deletions separately","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-12T20:40:46Z","receivedAt":"2025-06-12T20:40:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>> when a user pushes multiple branches such as:\n>>\n>> delete refs/heads/branch/conflict\n>> create refs/heads/branch\n>>\n>> Before using batched updates, the references would be applied\n>> sequentially and hence no conflicts would arise. With batched updates,\n>> while the first update applies, the second fails due to F/D conflict.\n>\n> Nit: it looks like \"D/F conflict\" is more often used than \"F/D\n> conflict\" in the Git code base:\n>\n> $ git grep -i 'd/f conflict' | wc -l\n> 119\n> $ git grep -i 'f/d conflict' | wc -l\n> 7\n\nI do not mind calling a situation F/D conflict if you have a file\nand your attempt to create a directory at the same path fails (as\nopposed to D/F where directory exists and you cannot overwrite it\nwith a file), but the above case does sound like a D/F conflict that\ndeletes directory r/h/b (by removing the last subpath in it), which\nis OK, and creates file r/h/b, which the all-or-nothing machinery\ndoes not allow well, so calling D/F may probably be more in line\nwith the existing practice, regardless of which situation we more\ncommonly talk about in the code base.\n"},{"id":"520208","messageId":"CAOLa=ZSU-r44oTiOiaqPrShj6p=4VUQTU-pXk5FJ4fenxB_FaA@mail.gmail.com","threadId":"63560","inReplyTo":"CAP8UFD0Az9YUd7tHbCWjrZ5bTv1V_0RZ2azasPmOrpf+ARMjug@mail.gmail.com","subject":"Re: [PATCH v3 2/2] receive-pack: handle reference deletions separately","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-13T07:23:47Z","receivedAt":"2025-06-13T07:23:49Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Christian Couder writes:\n\n> On Fri, Jun 6, 2025 at 10:41 AM Karthik Nayak wrote:\n>>\n>> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)\n>> we updated the 'git-receive-pack(1)' command to use batched reference\n>> updates. One edge case which was missed during this implementation was\n>> when a user pushes multiple branches such as:\n>>\n>> delete refs/heads/branch/conflict\n>> create refs/heads/branch\n>>\n>> Before using batched updates, the references would be applied\n>> sequentially and hence no conflicts would arise. With batched updates,\n>> while the first update applies, the second fails due to F/D conflict.\n>\n> Nit: it looks like \"D/F conflict\" is more often used than \"F/D\n> conflict\" in the Git code base:\n>\n> $ git grep -i 'd/f conflict' | wc -l\n> 119\n> $ git grep -i 'f/d conflict' | wc -l\n> 7\n>\n>> A\n>> similar issue was present in 'git-fetch(1)' and was fixed by using\n>> separating out reference pruning into a separate transaction. Apply a\n>\n> Maybe: s/using separating out/separating out/\n>\n\nThat's better, will change.\n\n>> similar mechanism for 'git-receive-pack(1)' and separate out reference\n>> deletions into its own batch.\n>>\n>> This means 'git-receive-pack(1)' will now use upto two transactions,\n>> whereas before using batched updates it would use _at least_ two\n>> transactions. So using batched updates is still the better option.\n>>\n>> Add a test to validate this behavior.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> builtin/receive-pack.c | 100 ++++++++++++++++++++++++++++++++-----------------\n>> t/t5516-fetch-push.sh | 17 +++++++--\n>> 2 files changed, 79 insertions(+), 38 deletions(-)\n>>\n>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n>> index 9e3cfb85cf..8ee792d2f8 100644\n>> --- a/builtin/receive-pack.c\n>> +++ b/builtin/receive-pack.c\n>> @@ -1867,47 +1867,79 @@ static void execute_commands_non_atomic(struct command *commands,\n>> const char *reported_error = NULL;\n>> struct strmap failed_refs = STRMAP_INIT;\n>>\n>> - transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n>> - REF_TRANSACTION_ALLOW_FAILURE, &err);\n>> - if (!transaction) {\n>> - rp_error(\"%s\", err.buf);\n>> - strbuf_reset(&err);\n>> - reported_error = \"transaction failed to start\";\n>> - goto failure;\n>> - }\n>> + /*\n>> + * Reference updates, where F/D conflicts shouldn't arise due to\n>\n> Nit: here also maybe \"D/F conflicts\" is more standard.\n>\n\nWill change.\n\n>> + * one reference being deleted, while the other being created\n>> + * are treated as conflicts in batched updates. This is because\n>> + * we don't do conflict resolution inside a transaction. To\n>> + * mitigate this, delete references in a separate batch.\n>> + */\n>> + enum processing_phase {\n>> + PHASE_DELETIONS,\n>> + PHASE_OTHERS\n>> + };\n>>\n>> - for (cmd = commands; cmd; cmd = cmd->next) {\n>> - if (!should_process_cmd(cmd) || cmd->run_proc_receive)\n>> - continue;\n>> + for (enum processing_phase phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {\n>> + for (cmd = commands; cmd; cmd = cmd->next) {\n>> + if (!should_process_cmd(cmd) || cmd->run_proc_receive)\n>> + continue;\n>>\n>> - cmd->error_string = update(cmd, si);\n>> - }\n>> + if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))\n>> + continue;\n>> + else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))\n>> + continue;\n>>\n>> - if (ref_transaction_commit(transaction, &err)) {\n>> - rp_error(\"%s\", err.buf);\n>> - reported_error = \"failed to update refs\";\n>> - goto failure;\n>> - }\n>> + /*\n>> + * Lazily create a transaction only when we know there are\n>> + * updates to be added.\n>> + */\n>> + if (!transaction) {\n>> + transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n>> + REF_TRANSACTION_ALLOW_FAILURE, &err);\n>> + if (!transaction) {\n>> + rp_error(\"%s\", err.buf);\n>> + strbuf_reset(&err);\n>> + reported_error = \"transaction failed to s1tart\";\n>\n> s/s1tart/start/\n>\n\nOops! Good catch.\n\n>> + goto failure;\n>> + }\n>> + }\n>>\n>> - ref_transaction_for_each_rejected_update(transaction,\n>> - ref_transaction_rejection_handler,\n>> - &failed_refs);\n>> + cmd->error_string = update(cmd, si);\n>> + }\n>>\n>> - if (strmap_empty(&failed_refs))\n>> - goto cleanup;\n>> + /*\n>> + * If no transaction was created, there is nothing to commit.\n>> + */\n>\n> Nit: the comment needs a single line, so maybe:\n>\n> /* No transaction, so nothing to commit */\n>\n\nLooks better.\n\n>> + if (!transaction)\n>> + goto cleanup;\n>>\n>> -failure:\n>> - for (cmd = commands; cmd; cmd = cmd->next) {\n>> - if (reported_error)\n>> - cmd->error_string = reported_error;\n>> - else if (strmap_contains(&failed_refs, cmd->ref_name))\n>> - cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n>> - }\n>> + if (ref_transaction_commit(transaction, &err)) {\n>> + rp_error(\"%s\", err.buf);\n>> + reported_error = \"failed to update refs\";\n>> + goto failure;\n>> + }\n>>\n>> -cleanup:\n>> - ref_transaction_free(transaction);\n>> - strmap_clear(&failed_refs, 0);\n>> - strbuf_release(&err);\n>> + ref_transaction_for_each_rejected_update(transaction,\n>> + ref_transaction_rejection_handler,\n>> + &failed_refs);\n>> +\n>> + if (strmap_empty(&failed_refs))\n>> + goto cleanup;\n>> +\n>> + failure:\n>\n> This label looks indented while previously it was right at the start\n> of the line. Not sure if we have a standard for that, but a few quick\n> greps seems to show that goto labels are most often at the start of\n> the line.\n>\n\nYup, but now this label lies within a for loop, so the indentation is\naligned to the loop. So I think it is correct as is.\n\n>> + for (cmd = commands; cmd; cmd = cmd->next) {\n>> + if (reported_error)\n>> + cmd->error_string = reported_error;\n>> + else if (strmap_contains(&failed_refs, cmd->ref_name))\n>> + cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n>> + }\n>> +\n>> + cleanup:\n>\n> Idem for how this label is indented.\n>\n\nSame here, this too is within a loop.\n\n>> + ref_transaction_free(transaction);\n>> + transaction = NULL;\n>> + strmap_clear(&failed_refs, 0);\n>> + strbuf_release(&err);\n>> + }\n>> }\n\nThanks for the review!\n\n- Karthik\n"},{"id":"520210","messageId":"20250613-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v4-0-ebf53edb9795@gmail.com","threadId":"63560","inReplyTo":"20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-0-903d1db3f10e@gmail.com","subject":"[PATCH v4 0/2] refs: fix some bugs with batched-updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-13T08:10:16Z","receivedAt":"2025-06-13T08:10:25Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08) we introduced a mechanism to batch reference updates. The\nidea being that we wanted to batch updates similar to a transaction, but\nallow certain updates to fail. This would help reference backends\noptimize such operations and also remove the overhead of processing\nupdates individually. Especially for the reftable backend, where\nbatching updates would ensure that the autocompaction would only occur\nat the end of the batch instead of after every reference update.\n\nAs of 9d2962a7c4 (receive-pack: use batched reference updates,\n2025-05-19) we also updated the 'git-fetch(1)' and 'git-receive-pack(1)'\ncommand to use batched reference updates. This series fixes some bugs\nthat we found at GitLab by running our Gitaly service with the `next`\nbuild of Git.\n\nThe first being in the files backend, which missed skipping over failed\nupdates in certain flows. When certain individual updates fail, we mark\nthem as such. However, we missed skipping over such failed updates,\nwhich can cause a SEGFAULT.\n\nThe other is in the git-receive-pack(1) implementation when a user\npushes multiple branches such as:\n\n delete refs/heads/branch/conflict\n create refs/heads/branch\n\nBefore using batched updates, the references would be applied\nsequentially and hence no conflicts would arise. With batched updates,\nwhile the first update applies, the second fails due to F/D conflict. A\nsimilar issue was present in 'git-fetch(1)' and was fixed by using\nseparating out reference pruning into a separate transaction. Apply a\nsimilar mechanism for 'git-receive-pack(1)' and separate out reference\ndeletions into its own batch.\n\nThis is based off master 7014b55638 (A bit more topics for -rc1,\n2025-05-30), with the changes from kn/fetch-push-bulk-ref-update merged\nin.\n\n---\nChanges in v4:\n- Swapped out F/D for D/F in the second commit, since we are talking\n about conflicts between a directory and a file, also D/F is more\n consistent.\n- Fixed some typos in the second commit.\n- Changed comment to single line.\n- Link to v3: https://lore.kernel.org/r/20250606-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v3-0-e1c41693bd35@gmail.com\n\nChanges in v3:\n- Cleanup the second commit message and remove stale comments.\n- In the second commit, only create the transaction if needed.\n- Link to v2: https://lore.kernel.org/r/20250605-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v2-0-26cd05b8a79e@gmail.com\n\nChanges in v2:\n- Modify the test in the first commit to no longer do a quiet grep,\n and some more tests.\n- Remove the second commit as it was unnecessary.\n- Modify the commit message in the last commit, to also talk about how\n we now use 2 transactions at minimum but this is still better than\n before.\n- Modify the logic in the last commit to no longer use an XOR and\n instead loop over the two different scenarios (deletion updates, other\n updates).\n- 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@gmail.com\n\n---\n builtin/receive-pack.c | 98 ++++++++++++++++++++++++++++++++------------------\n refs/files-backend.c | 7 ++++\n t/t1400-update-ref.sh | 45 +++++++++++++++++++++++\n t/t5516-fetch-push.sh | 17 ++++++---\n 4 files changed, 129 insertions(+), 38 deletions(-)\n\nKarthik Nayak (2):\n refs/files: skip updates with errors in batched updates\n receive-pack: handle reference deletions separately\n\nRange-diff versus v3:\n\n1: 6a9a7fdcbf = 1: 44291af073 refs/files: skip updates with errors in batched updates\n2: 3175c3430c ! 2: c8026de30e receive-pack: handle reference deletions separately\n @@ Commit message\n \n Before using batched updates, the references would be applied\n sequentially and hence no conflicts would arise. With batched updates,\n - while the first update applies, the second fails due to F/D conflict. A\n - similar issue was present in 'git-fetch(1)' and was fixed by using\n - separating out reference pruning into a separate transaction. Apply a\n - similar mechanism for 'git-receive-pack(1)' and separate out reference\n - deletions into its own batch.\n + while the first update applies, the second fails due to D/F conflict. A\n + similar issue was present in 'git-fetch(1)' and was fixed by separating\n + out reference pruning into a separate transaction. Apply a similar\n + mechanism for 'git-receive-pack(1)' and separate out reference deletions\n + into its own batch.\n \n - This means 'git-receive-pack(1)' will now use upto two transactions,\n + This means 'git-receive-pack(1)' will now use up to two transactions,\n whereas before using batched updates it would use _at least_ two\n transactions. So using batched updates is still the better option.\n \n @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *\n -\t\tgoto failure;\n -\t}\n +\t/*\n -+\t * Reference updates, where F/D conflicts shouldn't arise due to\n ++\t * Reference updates, where D/F conflicts shouldn't arise due to\n +\t * one reference being deleted, while the other being created\n +\t * are treated as conflicts in batched updates. This is because\n +\t * we don't do conflict resolution inside a transaction. To\n @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *\n +\t\t\t\tif (!transaction) {\n +\t\t\t\t\trp_error(\"%s\", err.buf);\n +\t\t\t\t\tstrbuf_reset(&err);\n -+\t\t\t\t\treported_error = \"transaction failed to s1tart\";\n ++\t\t\t\t\treported_error = \"transaction failed to start\";\n +\t\t\t\t\tgoto failure;\n +\t\t\t\t}\n +\t\t\t}\n @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *\n \n -\tif (strmap_empty(&failed_refs))\n -\t\tgoto cleanup;\n -+\t\t/*\n -+\t\t * If no transaction was created, there is nothing to commit.\n -+\t\t */\n ++\t\t/* No transaction, so nothing to commit */\n +\t\tif (!transaction)\n +\t\t\tgoto cleanup;\n \n\n\nbase-commit: 931c39f05e078e0df968a439379cb04b5c4666ef\nchange-id: 20250528-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-24ff53680144\n\nThanks\n- Karthik\n\n"},{"id":"520211","messageId":"20250613-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v4-1-ebf53edb9795@gmail.com","threadId":"63560","inReplyTo":"20250613-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v4-0-ebf53edb9795@gmail.com","subject":"[PATCH v4 1/2] refs/files: skip updates with errors in batched updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-13T08:10:17Z","receivedAt":"2025-06-13T08:10:25Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08) introduced support for batched reference updates. This\nallows users to batch updates together, while allowing some of the\nupdates to fail.\n\nUnder the hood, batched updates use the reference transaction mechanism.\nEach update which fails is marked as such. Any failed updates must be\nskipped over in the rest of the code, as they wouldn't apply any more.\nIn two of the loops within 'files_transaction_finish()' of the files\nbackend, the failed updates aren't skipped over. This can cause a\nSEGFAULT otherwise. Add the missing skips and a test to validate the\nsame.\n\nSigned-off-by: Karthik Nayak \n---\n refs/files-backend.c | 7 +++++++\n t/t1400-update-ref.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4d1f65a57a..c4a0f29072 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t */\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n+\n+\t\tif (update->rejection_err)\n+\t\t\tcontinue;\n+\n \t\tif (update->flags & REF_DELETING &&\n \t\t !(update->flags & REF_LOG_ONLY) &&\n \t\t !(update->flags & REF_IS_PRUNING)) {\n@@ -3239,6 +3243,9 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t\tstruct ref_update *update = transaction->updates[i];\n \t\tstruct ref_lock *lock = update->backend_data;\n \n+\t\tif (update->rejection_err)\n+\t\t\tcontinue;\n+\n \t\tif (update->flags & REF_DELETING &&\n \t\t !(update->flags & REF_LOG_ONLY)) {\n \t\t\tupdate->flags |= REF_DELETED_RMDIR;\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex d29d23cb89..ca7eee7de2 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -2299,6 +2299,51 @@ do\n \t\t\ttest_grep -q \"refname conflict\" stdout\n \t\t)\n \t'\n+\n+\ttest_expect_success \"stdin $type batch-updates delete incorrect symbolic ref\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit c1 &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\t\t\tgit symbolic-ref refs/heads/symbolic refs/heads/non-existent &&\n+\n+\t\t\tformat_command $type \"delete refs/heads/symbolic\" \"$head\" >stdin &&\n+\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n+\t\t\ttest_grep \"reference does not exist\" stdout\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"stdin $type batch-updates delete with incorrect old_oid\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit c1 &&\n+\t\t\tgit branch new-branch &&\n+\t\t\ttest_commit c2 &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\n+\t\t\tformat_command $type \"delete refs/heads/new-branch\" \"$head\" >stdin &&\n+\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n+\t\t\ttest_grep \"incorrect old value provided\" stdout\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"stdin $type batch-updates delete non-existent ref\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit commit &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\n+\t\t\tformat_command $type \"delete refs/heads/non-existent\" \"$head\" >stdin &&\n+\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n+\t\t\ttest_grep \"reference does not exist\" stdout\n+\t\t)\n+\t'\n done\n \n test_expect_success 'update-ref should also create reflog for HEAD' '\n\n-- \n2.49.0\n\n"},{"id":"520212","messageId":"20250613-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v4-2-ebf53edb9795@gmail.com","threadId":"63560","inReplyTo":"20250613-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v4-0-ebf53edb9795@gmail.com","subject":"[PATCH v4 2/2] receive-pack: handle reference deletions separately","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-13T08:10:18Z","receivedAt":"2025-06-13T08:10:26Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)\nwe updated the 'git-receive-pack(1)' command to use batched reference\nupdates. One edge case which was missed during this implementation was\nwhen a user pushes multiple branches such as:\n\n delete refs/heads/branch/conflict\n create refs/heads/branch\n\nBefore using batched updates, the references would be applied\nsequentially and hence no conflicts would arise. With batched updates,\nwhile the first update applies, the second fails due to D/F conflict. A\nsimilar issue was present in 'git-fetch(1)' and was fixed by separating\nout reference pruning into a separate transaction. Apply a similar\nmechanism for 'git-receive-pack(1)' and separate out reference deletions\ninto its own batch.\n\nThis means 'git-receive-pack(1)' will now use up to two transactions,\nwhereas before using batched updates it would use _at least_ two\ntransactions. So using batched updates is still the better option.\n\nAdd a test to validate this behavior.\n\nSigned-off-by: Karthik Nayak \n---\n builtin/receive-pack.c | 98 ++++++++++++++++++++++++++++++++------------------\n t/t5516-fetch-push.sh | 17 ++++++---\n 2 files changed, 77 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9e3cfb85cf..3fb2776e9a 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1867,47 +1867,77 @@ static void execute_commands_non_atomic(struct command *commands,\n \tconst char *reported_error = NULL;\n \tstruct strmap failed_refs = STRMAP_INIT;\n \n-\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n-\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n-\tif (!transaction) {\n-\t\trp_error(\"%s\", err.buf);\n-\t\tstrbuf_reset(&err);\n-\t\treported_error = \"transaction failed to start\";\n-\t\tgoto failure;\n-\t}\n+\t/*\n+\t * Reference updates, where D/F conflicts shouldn't arise due to\n+\t * one reference being deleted, while the other being created\n+\t * are treated as conflicts in batched updates. This is because\n+\t * we don't do conflict resolution inside a transaction. To\n+\t * mitigate this, delete references in a separate batch.\n+\t */\n+\tenum processing_phase {\n+\t\tPHASE_DELETIONS,\n+\t\tPHASE_OTHERS\n+\t};\n \n-\tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n-\t\t\tcontinue;\n+\tfor (enum processing_phase phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n+\t\t\t\tcontinue;\n \n-\t\tcmd->error_string = update(cmd, si);\n-\t}\n+\t\t\tif (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))\n+\t\t\t\tcontinue;\n+\t\t\telse if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))\n+\t\t\t\tcontinue;\n \n-\tif (ref_transaction_commit(transaction, &err)) {\n-\t\trp_error(\"%s\", err.buf);\n-\t\treported_error = \"failed to update refs\";\n-\t\tgoto failure;\n-\t}\n+\t\t\t/*\n+\t\t\t * Lazily create a transaction only when we know there are\n+\t\t\t * updates to be added.\n+\t\t\t */\n+\t\t\tif (!transaction) {\n+\t\t\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n+\t\t\t\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n+\t\t\t\tif (!transaction) {\n+\t\t\t\t\trp_error(\"%s\", err.buf);\n+\t\t\t\t\tstrbuf_reset(&err);\n+\t\t\t\t\treported_error = \"transaction failed to start\";\n+\t\t\t\t\tgoto failure;\n+\t\t\t\t}\n+\t\t\t}\n \n-\tref_transaction_for_each_rejected_update(transaction,\n-\t\t\t\t\t\t ref_transaction_rejection_handler,\n-\t\t\t\t\t\t &failed_refs);\n+\t\t\tcmd->error_string = update(cmd, si);\n+\t\t}\n \n-\tif (strmap_empty(&failed_refs))\n-\t\tgoto cleanup;\n+\t\t/* No transaction, so nothing to commit */\n+\t\tif (!transaction)\n+\t\t\tgoto cleanup;\n \n-failure:\n-\tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (reported_error)\n-\t\t\tcmd->error_string = reported_error;\n-\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n-\t\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n-\t}\n+\t\tif (ref_transaction_commit(transaction, &err)) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\treported_error = \"failed to update refs\";\n+\t\t\tgoto failure;\n+\t\t}\n \n-cleanup:\n-\tref_transaction_free(transaction);\n-\tstrmap_clear(&failed_refs, 0);\n-\tstrbuf_release(&err);\n+\t\tref_transaction_for_each_rejected_update(transaction,\n+\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t\t &failed_refs);\n+\n+\t\tif (strmap_empty(&failed_refs))\n+\t\t\tgoto cleanup;\n+\n+\tfailure:\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\t\tif (reported_error)\n+\t\t\t\tcmd->error_string = reported_error;\n+\t\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n+\t\t\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n+\t\t}\n+\n+\tcleanup:\n+\t\tref_transaction_free(transaction);\n+\t\ttransaction = NULL;\n+\t\tstrmap_clear(&failed_refs, 0);\n+\t\tstrbuf_release(&err);\n+\t}\n }\n \n static void execute_commands_atomic(struct command *commands,\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex dabcc5f811..1649667441 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $orgmain $newmain\n \t\trefs/heads/next $orgnext $newnext\n+\t\trefs/heads/main $orgmain $newmain\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n@@ -808,8 +808,8 @@ test_expect_success 'deletion of a non-existent ref is not fed to post-receive a\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $orgmain $newmain\n \t\trefs/heads/nonexistent $ZERO_OID $ZERO_OID\n+\t\trefs/heads/main $orgmain $newmain\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n@@ -868,10 +868,10 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $orgmain $newmain\n \t\trefs/heads/next $orgnext $newnext\n-\t\trefs/heads/seen $orgseen $newseen\n \t\trefs/heads/nonexistent $ZERO_OID $ZERO_OID\n+\t\trefs/heads/main $orgmain $newmain\n+\t\trefs/heads/seen $orgseen $newseen\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n@@ -1909,4 +1909,13 @@ test_expect_success 'push with config push.useBitmaps' '\n \t\t--thin --delta-base-offset -q --no-use-bitmap-index wrote:\n\n> Changes in v4:\n> - Swapped out F/D for D/F in the second commit, since we are talking\n> about conflicts between a directory and a file, also D/F is more\n> consistent.\n> - Fixed some typos in the second commit.\n> - Changed comment to single line.\n> - Link to v3: https://lore.kernel.org/r/20250606-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v3-0-e1c41693bd35@gmail.com\n\nThis v4 looks good to me based on the range-diff and my previous look at the v3.\n\nThanks!\n"},{"id":"520224","messageId":"xmqqldpvably.fsf@gitster.g","threadId":"63560","inReplyTo":"20250613-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v4-2-ebf53edb9795@gmail.com","subject":"Re: [PATCH v4 2/2] receive-pack: handle reference deletions separately","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-13T15:46:01Z","receivedAt":"2025-06-13T15:46:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)\n> we updated the 'git-receive-pack(1)' command to use batched reference\n> updates. One edge case which was missed during this implementation was\n> when a user pushes multiple branches such as:\n>\n> delete refs/heads/branch/conflict\n> create refs/heads/branch\n>\n> Before using batched updates, the references would be applied\n> sequentially and hence no conflicts would arise. With batched updates,\n> while the first update applies, the second fails due to D/F conflict. A\n> similar issue was present in 'git-fetch(1)' and was fixed by separating\n\nDo you have a reference to such an earlier fix to \"git fetch\"? If\nso, let's add it here.\n\n> out reference pruning into a separate transaction. Apply a similar\n> mechanism for 'git-receive-pack(1)' and separate out reference deletions\n> into its own batch.\n\nThe implication of this is that the earlier \"delete\" half of the\noperation can succeed and be committed but the \"create\" half can\nfail, leaving the resulting repository without the reference the\nuser wanted to have.\n\nFor now, this \"two transactions\" may suffice as a workaround but do\nyou think it is a viable solution for longer term? As long as we\nclaim that the reference updates are transactional, my answer is no.\nWe'd need to fix it at a lower layer within a single transaction.\n\nIt is outside the topic of this patch series but we can at least\nleave a NEEDSWORK comment that this is merely a workaround and we'll\nhave to fix the later? I see a in-code comment that says \"To\nmitigate this\" to hint the nature of the two phase solution, but we\nmay want an explicit note that says that \"we know this is broken\neven though it is less broken than it used to be\".\n\n> This means 'git-receive-pack(1)' will now use up to two transactions,\n> whereas before using batched updates it would use _at least_ two\n> transactions. So using batched updates is still the better option.\n>\n> Add a test to validate this behavior.\n\nI wonder if we can write a test against a remote that accepts\ndeletions but fails the actions in the second phase as a\ntest_expect_failure documentation?\n\nOther than that, very well described. I know it is hard to describe\na patch that knowingly does a workaround instead of doing the right\nthing for the sake of simplicity, and the proposed log message did a\nvery good job at it.\n\nWill queue. Thanks.\n"},{"id":"520233","messageId":"xmqqmsab8o6u.fsf@gitster.g","threadId":"63560","inReplyTo":"CAP8UFD2ZhXSE85EtQbA42UR0ds+nsQba4EcY589raMBAoOrfLA@mail.gmail.com","subject":"Re: [PATCH v4 0/2] refs: fix some bugs with batched-updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-13T18:57:13Z","receivedAt":"2025-06-13T18:57:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Fri, Jun 13, 2025 at 10:10 AM Karthik Nayak wrote:\n>\n>> Changes in v4:\n>> - Swapped out F/D for D/F in the second commit, since we are talking\n>> about conflicts between a directory and a file, also D/F is more\n>> consistent.\n>> - Fixed some typos in the second commit.\n>> - Changed comment to single line.\n>> - Link to v3: https://lore.kernel.org/r/20250606-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v3-0-e1c41693bd35@gmail.com\n>\n> This v4 looks good to me based on the range-diff and my previous look at the v3.\n\nThanks, all.\n"},{"id":"520399","messageId":"CAOLa=ZQUJ_RDLn9W64=0_E+ybeWjJ9oNtK036uTX1DGjQshxcw@mail.gmail.com","threadId":"63560","inReplyTo":"xmqqldpvably.fsf@gitster.g","subject":"Re: [PATCH v4 2/2] receive-pack: handle reference deletions separately","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-19T09:39:17Z","receivedAt":"2025-06-19T09:39:20Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)\n>> we updated the 'git-receive-pack(1)' command to use batched reference\n>> updates. One edge case which was missed during this implementation was\n>> when a user pushes multiple branches such as:\n>>\n>> delete refs/heads/branch/conflict\n>> create refs/heads/branch\n>>\n>> Before using batched updates, the references would be applied\n>> sequentially and hence no conflicts would arise. With batched updates,\n>> while the first update applies, the second fails due to D/F conflict. A\n>> similar issue was present in 'git-fetch(1)' and was fixed by separating\n>\n> Do you have a reference to such an earlier fix to \"git fetch\"? If\n> so, let's add it here.\n>\n\nGood point, will add it in.\n\n>> out reference pruning into a separate transaction. Apply a similar\n>> mechanism for 'git-receive-pack(1)' and separate out reference deletions\n>> into its own batch.\n>\n> The implication of this is that the earlier \"delete\" half of the\n> operation can succeed and be committed but the \"create\" half can\n> fail, leaving the resulting repository without the reference the\n> user wanted to have.\n>\n\nI'm not sure which earlier you're referring to, so let me describe them\nall.\n\n- Before we introduced batch updates. In this scenario, every reference\n update was independent. Meaning every update could pass/fail and we'd\n still iterate over all other references. Here, if a 'delete' reference\n passed and the 'create' failed, we'd end up in the scenario you\n mentioned.\n\n- Post introduction of the batch updates. In this scenario, we use batch\n transactions, where all updates where batched together. Even here,\n individual updates can pass/fail and we'd still iterate over all the\n other references. The only issue was that, if there was a 'delete' &\n 'create' with a D/F conflict, then the create would fail as they were\n within the same transaction. The ultimate fix for this is conflict\n resolution within a transaction.\n\n- After this fix. In this scenario, we separate out deletions into their\n transaction, this ensures we don't have D/F conflicts within a single\n transaction. But we still iterate over all references.\n\nYou're right that now 'delete' half can pass and 'create' could fail,\nleaving the repository in a state the user didn't want. But this was\nalso true before batched updates were introduced.\n\n> For now, this \"two transactions\" may suffice as a workaround but do\n> you think it is a viable solution for longer term? As long as we\n> claim that the reference updates are transactional, my answer is no.\n> We'd need to fix it at a lower layer within a single transaction.\n>\n\nYes definitely agreed here. Conflict resolution within a transaction is\nsomething we should definitely look into after this, wherein if there is\na 'delete' and a 'create' within a single transaction they shouldn't\ncause D/F conflicts. Since the conflict is going to be deleted out\nanyways.\n\n> It is outside the topic of this patch series but we can at least\n> leave a NEEDSWORK comment that this is merely a workaround and we'll\n> have to fix the later? I see a in-code comment that says \"To\n> mitigate this\" to hint the nature of the two phase solution, but we\n> may want an explicit note that says that \"we know this is broken\n> even though it is less broken than it used to be\".\n>\n\nTotally valid point. I will add in something like that.\n\n>> This means 'git-receive-pack(1)' will now use up to two transactions,\n>> whereas before using batched updates it would use _at least_ two\n>> transactions. So using batched updates is still the better option.\n>>\n>> Add a test to validate this behavior.\n>\n> I wonder if we can write a test against a remote that accepts\n> deletions but fails the actions in the second phase as a\n> test_expect_failure documentation?\n>\n\nWell we possibly could, but could you explain the intent here? I'm not\nable to see why we'd need that. Mostly since we allow individual\nreference updates to fail when using batched updates. So the second\ntransaction here could pass with a few individual updates failing, this\nwould be similar to how things were before we introduced batched\nreference updates to 'git-receive-pack(1)'.\n\n> Other than that, very well described. I know it is hard to describe\n> a patch that knowingly does a workaround instead of doing the right\n> thing for the sake of simplicity, and the proposed log message did a\n> very good job at it.\n>\n> Will queue. Thanks.\n>\n\nThanks, and sorry for the late response, I was on vacation. Will send in\na new version soon.\n\n- Karthik\n"},{"id":"520481","messageId":"20250620-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v5-0-f35ee6b59a82@gmail.com","threadId":"63560","inReplyTo":"20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-0-903d1db3f10e@gmail.com","subject":"[PATCH v5 0/2] refs: fix some bugs with batched-updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-20T07:15:43Z","receivedAt":"2025-06-20T07:15:52Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08) we introduced a mechanism to batch reference updates. The\nidea being that we wanted to batch updates similar to a transaction, but\nallow certain updates to fail. This would help reference backends\noptimize such operations and also remove the overhead of processing\nupdates individually. Especially for the reftable backend, where\nbatching updates would ensure that the autocompaction would only occur\nat the end of the batch instead of after every reference update.\n\nAs of 9d2962a7c4 (receive-pack: use batched reference updates,\n2025-05-19) we also updated the 'git-fetch(1)' and 'git-receive-pack(1)'\ncommand to use batched reference updates. This series fixes some bugs\nthat we found at GitLab by running our Gitaly service with the `next`\nbuild of Git.\n\nThe first being in the files backend, which missed skipping over failed\nupdates in certain flows. When certain individual updates fail, we mark\nthem as such. However, we missed skipping over such failed updates,\nwhich can cause a SEGFAULT.\n\nThe other is in the git-receive-pack(1) implementation when a user\npushes multiple branches such as:\n\n delete refs/heads/branch/conflict\n create refs/heads/branch\n\nBefore using batched updates, the references would be applied\nsequentially and hence no conflicts would arise. With batched updates,\nwhile the first update applies, the second fails due to F/D conflict. A\nsimilar issue was present in 'git-fetch(1)' and was fixed by using\nseparating out reference pruning into a separate transaction. Apply a\nsimilar mechanism for 'git-receive-pack(1)' and separate out reference\ndeletions into its own batch.\n\nThis is based off master 7014b55638 (A bit more topics for -rc1,\n2025-05-30), with the changes from kn/fetch-push-bulk-ref-update merged\nin.\n\n---\nChanges in v5:\n- Modify the commit message in patch 2 to also mention the commit\n which adds multiple transactions to 'git-fetch(1)'.\n- Also add a NEEDSWORK to the existing comment to mention how conflict\n resolution within transactions can remove the need for multiple \n transactions.\n- Link to v4: https://lore.kernel.org/r/20250613-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v4-0-ebf53edb9795@gmail.com\n\nChanges in v4:\n- Swapped out F/D for D/F in the second commit, since we are talking\n about conflicts between a directory and a file, also D/F is more\n consistent.\n- Fixed some typos in the second commit.\n- Changed comment to single line.\n- Link to v3: https://lore.kernel.org/r/20250606-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v3-0-e1c41693bd35@gmail.com\n\nChanges in v3:\n- Cleanup the second commit message and remove stale comments.\n- In the second commit, only create the transaction if needed.\n- Link to v2: https://lore.kernel.org/r/20250605-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v2-0-26cd05b8a79e@gmail.com\n\nChanges in v2:\n- Modify the test in the first commit to no longer do a quiet grep,\n and some more tests.\n- Remove the second commit as it was unnecessary.\n- Modify the commit message in the last commit, to also talk about how\n we now use 2 transactions at minimum but this is still better than\n before.\n- Modify the logic in the last commit to no longer use an XOR and\n instead loop over the two different scenarios (deletion updates, other\n updates).\n- 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@gmail.com\n\n---\n builtin/receive-pack.c | 102 ++++++++++++++++++++++++++++++++-----------------\n refs/files-backend.c | 7 ++++\n t/t1400-update-ref.sh | 45 ++++++++++++++++++++++\n t/t5516-fetch-push.sh | 17 +++++++--\n 4 files changed, 133 insertions(+), 38 deletions(-)\n\nKarthik Nayak (2):\n refs/files: skip updates with errors in batched updates\n receive-pack: handle reference deletions separately\n\nRange-diff versus v4:\n\n1: 0ab0890bf1 = 1: 9248bfd474 refs/files: skip updates with errors in batched updates\n2: e45ec3139b ! 2: 00bf816b2d receive-pack: handle reference deletions separately\n @@ Commit message\n sequentially and hence no conflicts would arise. With batched updates,\n while the first update applies, the second fails due to D/F conflict. A\n similar issue was present in 'git-fetch(1)' and was fixed by separating\n - out reference pruning into a separate transaction. Apply a similar\n - mechanism for 'git-receive-pack(1)' and separate out reference deletions\n - into its own batch.\n + out reference pruning into a separate transaction in the commit 'fetch:\n + use batched reference updates'. Apply a similar mechanism for\n + 'git-receive-pack(1)' and separate out reference deletions into its own\n + batch.\n \n This means 'git-receive-pack(1)' will now use up to two transactions,\n whereas before using batched updates it would use _at least_ two\n @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *\n +\t * are treated as conflicts in batched updates. This is because\n +\t * we don't do conflict resolution inside a transaction. To\n +\t * mitigate this, delete references in a separate batch.\n ++\t *\n ++\t * NEEDSWORK: Add conflict resolution between deletion and creation\n ++\t * of reference updates within a transaction. With that, we can\n ++\t * combine the two phases.\n +\t */\n +\tenum processing_phase {\n +\t\tPHASE_DELETIONS,\n\n\nbase-commit: 931c39f05e078e0df968a439379cb04b5c4666ef\nchange-id: 20250528-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-24ff53680144\n\nThanks\n- Karthik\n\n"},{"id":"520482","messageId":"20250620-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v5-1-f35ee6b59a82@gmail.com","threadId":"63560","inReplyTo":"20250620-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v5-0-f35ee6b59a82@gmail.com","subject":"[PATCH v5 1/2] refs/files: skip updates with errors in batched updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-20T07:15:44Z","receivedAt":"2025-06-20T07:15:53Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08) introduced support for batched reference updates. This\nallows users to batch updates together, while allowing some of the\nupdates to fail.\n\nUnder the hood, batched updates use the reference transaction mechanism.\nEach update which fails is marked as such. Any failed updates must be\nskipped over in the rest of the code, as they wouldn't apply any more.\nIn two of the loops within 'files_transaction_finish()' of the files\nbackend, the failed updates aren't skipped over. This can cause a\nSEGFAULT otherwise. Add the missing skips and a test to validate the\nsame.\n\nSigned-off-by: Karthik Nayak \n---\n refs/files-backend.c | 7 +++++++\n t/t1400-update-ref.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4d1f65a57a..c4a0f29072 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t */\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n+\n+\t\tif (update->rejection_err)\n+\t\t\tcontinue;\n+\n \t\tif (update->flags & REF_DELETING &&\n \t\t !(update->flags & REF_LOG_ONLY) &&\n \t\t !(update->flags & REF_IS_PRUNING)) {\n@@ -3239,6 +3243,9 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t\tstruct ref_update *update = transaction->updates[i];\n \t\tstruct ref_lock *lock = update->backend_data;\n \n+\t\tif (update->rejection_err)\n+\t\t\tcontinue;\n+\n \t\tif (update->flags & REF_DELETING &&\n \t\t !(update->flags & REF_LOG_ONLY)) {\n \t\t\tupdate->flags |= REF_DELETED_RMDIR;\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex d29d23cb89..ca7eee7de2 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -2299,6 +2299,51 @@ do\n \t\t\ttest_grep -q \"refname conflict\" stdout\n \t\t)\n \t'\n+\n+\ttest_expect_success \"stdin $type batch-updates delete incorrect symbolic ref\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit c1 &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\t\t\tgit symbolic-ref refs/heads/symbolic refs/heads/non-existent &&\n+\n+\t\t\tformat_command $type \"delete refs/heads/symbolic\" \"$head\" >stdin &&\n+\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n+\t\t\ttest_grep \"reference does not exist\" stdout\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"stdin $type batch-updates delete with incorrect old_oid\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit c1 &&\n+\t\t\tgit branch new-branch &&\n+\t\t\ttest_commit c2 &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\n+\t\t\tformat_command $type \"delete refs/heads/new-branch\" \"$head\" >stdin &&\n+\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n+\t\t\ttest_grep \"incorrect old value provided\" stdout\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"stdin $type batch-updates delete non-existent ref\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit commit &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\n+\t\t\tformat_command $type \"delete refs/heads/non-existent\" \"$head\" >stdin &&\n+\t\t\tgit update-ref $type --stdin --batch-updates stdout &&\n+\t\t\ttest_grep \"reference does not exist\" stdout\n+\t\t)\n+\t'\n done\n \n test_expect_success 'update-ref should also create reflog for HEAD' '\n\n-- \n2.49.0\n\n"},{"id":"520483","messageId":"20250620-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v5-2-f35ee6b59a82@gmail.com","threadId":"63560","inReplyTo":"20250620-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v5-0-f35ee6b59a82@gmail.com","subject":"[PATCH v5 2/2] receive-pack: handle reference deletions separately","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-20T07:15:45Z","receivedAt":"2025-06-20T07:15:54Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)\nwe updated the 'git-receive-pack(1)' command to use batched reference\nupdates. One edge case which was missed during this implementation was\nwhen a user pushes multiple branches such as:\n\n delete refs/heads/branch/conflict\n create refs/heads/branch\n\nBefore using batched updates, the references would be applied\nsequentially and hence no conflicts would arise. With batched updates,\nwhile the first update applies, the second fails due to D/F conflict. A\nsimilar issue was present in 'git-fetch(1)' and was fixed by separating\nout reference pruning into a separate transaction in the commit 'fetch:\nuse batched reference updates'. Apply a similar mechanism for\n'git-receive-pack(1)' and separate out reference deletions into its own\nbatch.\n\nThis means 'git-receive-pack(1)' will now use up to two transactions,\nwhereas before using batched updates it would use _at least_ two\ntransactions. So using batched updates is still the better option.\n\nAdd a test to validate this behavior.\n\nSigned-off-by: Karthik Nayak \n---\n builtin/receive-pack.c | 102 ++++++++++++++++++++++++++++++++-----------------\n t/t5516-fetch-push.sh | 17 +++++++--\n 2 files changed, 81 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9e3cfb85cf..1809539201 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1867,47 +1867,81 @@ static void execute_commands_non_atomic(struct command *commands,\n \tconst char *reported_error = NULL;\n \tstruct strmap failed_refs = STRMAP_INIT;\n \n-\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n-\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n-\tif (!transaction) {\n-\t\trp_error(\"%s\", err.buf);\n-\t\tstrbuf_reset(&err);\n-\t\treported_error = \"transaction failed to start\";\n-\t\tgoto failure;\n-\t}\n+\t/*\n+\t * Reference updates, where D/F conflicts shouldn't arise due to\n+\t * one reference being deleted, while the other being created\n+\t * are treated as conflicts in batched updates. This is because\n+\t * we don't do conflict resolution inside a transaction. To\n+\t * mitigate this, delete references in a separate batch.\n+\t *\n+\t * NEEDSWORK: Add conflict resolution between deletion and creation\n+\t * of reference updates within a transaction. With that, we can\n+\t * combine the two phases.\n+\t */\n+\tenum processing_phase {\n+\t\tPHASE_DELETIONS,\n+\t\tPHASE_OTHERS\n+\t};\n \n-\tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n-\t\t\tcontinue;\n+\tfor (enum processing_phase phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n+\t\t\t\tcontinue;\n \n-\t\tcmd->error_string = update(cmd, si);\n-\t}\n+\t\t\tif (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))\n+\t\t\t\tcontinue;\n+\t\t\telse if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))\n+\t\t\t\tcontinue;\n \n-\tif (ref_transaction_commit(transaction, &err)) {\n-\t\trp_error(\"%s\", err.buf);\n-\t\treported_error = \"failed to update refs\";\n-\t\tgoto failure;\n-\t}\n+\t\t\t/*\n+\t\t\t * Lazily create a transaction only when we know there are\n+\t\t\t * updates to be added.\n+\t\t\t */\n+\t\t\tif (!transaction) {\n+\t\t\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n+\t\t\t\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n+\t\t\t\tif (!transaction) {\n+\t\t\t\t\trp_error(\"%s\", err.buf);\n+\t\t\t\t\tstrbuf_reset(&err);\n+\t\t\t\t\treported_error = \"transaction failed to start\";\n+\t\t\t\t\tgoto failure;\n+\t\t\t\t}\n+\t\t\t}\n \n-\tref_transaction_for_each_rejected_update(transaction,\n-\t\t\t\t\t\t ref_transaction_rejection_handler,\n-\t\t\t\t\t\t &failed_refs);\n+\t\t\tcmd->error_string = update(cmd, si);\n+\t\t}\n \n-\tif (strmap_empty(&failed_refs))\n-\t\tgoto cleanup;\n+\t\t/* No transaction, so nothing to commit */\n+\t\tif (!transaction)\n+\t\t\tgoto cleanup;\n \n-failure:\n-\tfor (cmd = commands; cmd; cmd = cmd->next) {\n-\t\tif (reported_error)\n-\t\t\tcmd->error_string = reported_error;\n-\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n-\t\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n-\t}\n+\t\tif (ref_transaction_commit(transaction, &err)) {\n+\t\t\trp_error(\"%s\", err.buf);\n+\t\t\treported_error = \"failed to update refs\";\n+\t\t\tgoto failure;\n+\t\t}\n \n-cleanup:\n-\tref_transaction_free(transaction);\n-\tstrmap_clear(&failed_refs, 0);\n-\tstrbuf_release(&err);\n+\t\tref_transaction_for_each_rejected_update(transaction,\n+\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t\t &failed_refs);\n+\n+\t\tif (strmap_empty(&failed_refs))\n+\t\t\tgoto cleanup;\n+\n+\tfailure:\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\t\tif (reported_error)\n+\t\t\t\tcmd->error_string = reported_error;\n+\t\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n+\t\t\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n+\t\t}\n+\n+\tcleanup:\n+\t\tref_transaction_free(transaction);\n+\t\ttransaction = NULL;\n+\t\tstrmap_clear(&failed_refs, 0);\n+\t\tstrbuf_release(&err);\n+\t}\n }\n \n static void execute_commands_atomic(struct command *commands,\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex dabcc5f811..1649667441 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $orgmain $newmain\n \t\trefs/heads/next $orgnext $newnext\n+\t\trefs/heads/main $orgmain $newmain\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n@@ -808,8 +808,8 @@ test_expect_success 'deletion of a non-existent ref is not fed to post-receive a\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $orgmain $newmain\n \t\trefs/heads/nonexistent $ZERO_OID $ZERO_OID\n+\t\trefs/heads/main $orgmain $newmain\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n@@ -868,10 +868,10 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w\n \t\tEOF\n \n \t\tcat >update.expect <<-EOF &&\n-\t\trefs/heads/main $orgmain $newmain\n \t\trefs/heads/next $orgnext $newnext\n-\t\trefs/heads/seen $orgseen $newseen\n \t\trefs/heads/nonexistent $ZERO_OID $ZERO_OID\n+\t\trefs/heads/main $orgmain $newmain\n+\t\trefs/heads/seen $orgseen $newseen\n \t\tEOF\n \n \t\tcat >post-receive.expect <<-EOF &&\n@@ -1909,4 +1909,13 @@ test_expect_success 'push with config push.useBitmaps' '\n \t\t--thin --delta-base-offset -q --no-use-bitmap-index writes:\n\n> In 23fc8e4f61 (refs: implement batch reference update support,\n> 2025-04-08) we introduced a mechanism to batch reference updates.\n\nJust to let you know, as these two are fixups to the topic that are\nno longer in 'next' as we rewound the tip of 'next' after release,\nif you want, you can redo the base topic instead of piling small\nfixes on top.\n\nWill replace.\n\nThanks.\n"},{"id":"520532","messageId":"CAOLa=ZRuyr_DALnjDJfT4F3SJwcExPKZZnf4w6g3+JshBM4iJQ@mail.gmail.com","threadId":"63560","inReplyTo":"xmqqcyay4c4o.fsf@gitster.g","subject":"Re: [PATCH v5 0/2] refs: fix some bugs with batched-updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-21T11:08:22Z","receivedAt":"2025-06-21T11:08:24Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> In 23fc8e4f61 (refs: implement batch reference update support,\n>> 2025-04-08) we introduced a mechanism to batch reference updates.\n>\n> Just to let you know, as these two are fixups to the topic that are\n> no longer in 'next' as we rewound the tip of 'next' after release,\n> if you want, you can redo the base topic instead of piling small\n> fixes on top.\n>\n\nWell the first patch in this series, is a bug fix for master since we\nalready have batched updates exposed via git-update-ref(1). So only the\nsecond patch can be squashed in.\n\nThat said, while it is easier for me to not re-roll, I'd happy to do so,\nwhat do you think?\n\n> Will replace.\n>\n> Thanks.\n"},{"id":"520540","messageId":"xmqq5xgo8kwk.fsf@gitster.g","threadId":"63560","inReplyTo":"CAOLa=ZRuyr_DALnjDJfT4F3SJwcExPKZZnf4w6g3+JshBM4iJQ@mail.gmail.com","subject":"Re: [PATCH v5 0/2] refs: fix some bugs with batched-updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-22T04:23:07Z","receivedAt":"2025-06-22T04:23:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n>> Just to let you know, as these two are fixups to the topic that are\n>> no longer in 'next' as we rewound the tip of 'next' after release,\n>> if you want, you can redo the base topic instead of piling small\n>> fixes on top.\n>>\n>\n> Well the first patch in this series, is a bug fix for master since we\n> already have batched updates exposed via git-update-ref(1). So only the\n> second patch can be squashed in.\n>\n> That said, while it is easier for me to not re-roll, I'd happy to do so,\n> what do you think?\n\nI'll let you decide; please choose whichever way you consider would\ngive us the better result. The second one seems to be a band-aid\nthat trades one bug with another bug, so it may be prudent to leave\nit separate. It would make it easier for a future change that fixes\nthe lower-layer transaction processing to refer to it, with \"earlier\nwe took thw two step approach, which had these downsides. now we fix\nthe issue for real\".\n\nThanks.\n\n"},{"id":"520542","messageId":"CAOLa=ZQaLN5ijGJfGZkBxTd6JCKizUCne=yWs+bBgrW7mv3PVA@mail.gmail.com","threadId":"63560","inReplyTo":"xmqq5xgo8kwk.fsf@gitster.g","subject":"Re: [PATCH v5 0/2] refs: fix some bugs with batched-updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-06-22T14:20:32Z","receivedAt":"2025-06-22T14:20:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>>> Just to let you know, as these two are fixups to the topic that are\n>>> no longer in 'next' as we rewound the tip of 'next' after release,\n>>> if you want, you can redo the base topic instead of piling small\n>>> fixes on top.\n>>>\n>>\n>> Well the first patch in this series, is a bug fix for master since we\n>> already have batched updates exposed via git-update-ref(1). So only the\n>> second patch can be squashed in.\n>>\n>> That said, while it is easier for me to not re-roll, I'd happy to do so,\n>> what do you think?\n>\n> I'll let you decide; please choose whichever way you consider would\n> give us the better result. The second one seems to be a band-aid\n> that trades one bug with another bug, so it may be prudent to leave\n> it separate. It would make it easier for a future change that fixes\n> the lower-layer transaction processing to refer to it, with \"earlier\n> we took thw two step approach, which had these downsides. now we fix\n> the issue for real\".\n>\n> Thanks.\n\nI was thinking the same. I do think leaving it separate has the benefit\nof extra context being present which in the future would be useful like\nyou mentioned. The only downside being that the base patches are anyway\nout of 'next', so we could squash the patch in. But, In the end I do\nthink that keeping them separate is better also because only one patch\nwill be squashed in. So let's keep them separate.\n\n- Karthik\n"}]}