[PATCH 0/3] refs: fix some bugs with batched-updates

[PATCH 0/3] refs: fix some bugs with batched-updates

[Karthik Nayak]

In 23fc8e4f61 (refs: implement batch reference update support,
2025-04-08) we introduced a mechanism to batch reference updates. The
idea being that we wanted to batch updates similar to a transaction, but
allow certain updates to fail. This would help reference backends
optimize such operations and also remove the overhead of processing
updates individually. Especially for the reftable backend, where
batching updates would ensure that the autocompaction would only occur
at the end of the batch instead of after every reference update.

As of 9d2962a7c4 (receive-pack: use batched reference updates,
2025-05-19) we also updated the 'git-fetch(1)' and 'git-receive-pack(1)'
command to use batched reference updates. This series fixes some bugs
that we found at GitLab by running our Gitaly service with the `next`
build of Git.

The first being in the files backend, which missed skipping over failed
updates in certain flows. When certain individual updates fail, we mark
them as such. However, we missed skipping over such failed updates,
which can cause a SEGFAULT.

The other is in the git-receive-pack(1) implementation when a user
pushes multiple branches such as:

  delete refs/heads/branch/conflict
  create refs/heads/branch

Before using batched updates, the references would be applied
sequentially and hence no conflicts would arise. With batched updates,
while the first update applies, the second fails due to F/D conflict. A
similar issue was present in 'git-fetch(1)' and was fixed by using
separating out reference pruning into a separate transaction. Apply a
similar mechanism for 'git-receive-pack(1)' and separate out reference
deletions into its own batch.

This is based off master 7014b55638 (A bit more topics for -rc1,
2025-05-30), with the changes from kn/fetch-push-bulk-ref-update merged
in.

---
 builtin/receive-pack.c           | 23 +++++++++++++++++++----
 refs/files-backend.c             |  7 +++++++
 t/t1400-update-ref.sh            | 14 ++++++++++++++
 t/t1416-ref-transaction-hooks.sh |  2 ++
 t/t5516-fetch-push.sh            | 23 ++++++++++++++++-------
 5 files changed, 58 insertions(+), 11 deletions(-)

Karthik Nayak (3):
      refs/files: skip updates with errors in batched updates
      t5516: use double quotes for tests with variables
      receive-pack: handle reference deletions separately



base-commit: 931c39f05e078e0df968a439379cb04b5c4666ef
change-id: 20250528-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-24ff53680144

Thanks
- Karthik

[PATCH 1/3] refs/files: skip updates with errors in batched updates

[Karthik Nayak]

The commit 23fc8e4f61 (refs: implement batch reference update support,
2025-04-08) introduced support for batched reference updates. This
allows users to batch updates together, while allowing some of the
updates to fail.

Under the hood, batched updates use the reference transaction mechanism.
Each update which fails is marked as such. Any failed updates must be
skipped over in the rest of the code, as they wouldn't apply any more.
In two of the loops within 'files_transaction_finish()' of the files
backend, the failed updates aren't skipped over. This can cause a
SEGFAULT otherwise. Add the missing skips and a test to validate the
same.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c  |  7 +++++++
 t/t1400-update-ref.sh | 14 ++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4d1f65a57a..c4a0f29072 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,
 	 */
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
+
+		if (update->rejection_err)
+			continue;
+
 		if (update->flags & REF_DELETING &&
 		    !(update->flags & REF_LOG_ONLY) &&
 		    !(update->flags & REF_IS_PRUNING)) {
@@ -3239,6 +3243,9 @@ static int files_transaction_finish(struct ref_store *ref_store,
 		struct ref_update *update = transaction->updates[i];
 		struct ref_lock *lock = update->backend_data;
 
+		if (update->rejection_err)
+			continue;
+
 		if (update->flags & REF_DELETING &&
 		    !(update->flags & REF_LOG_ONLY)) {
 			update->flags |= REF_DELETED_RMDIR;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index d29d23cb89..e9a605d0ba 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -2299,6 +2299,20 @@ do
 			test_grep -q "refname conflict" stdout
 		)
 	'
+
+	test_expect_success "stdin $type batch-updates delete non-existent ref" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit commit &&
+			head=$(git rev-parse HEAD) &&
+
+			format_command $type "delete refs/heads/non-existent" "$head" >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+			test_grep -q "reference does not exist" stdout
+		)
+	'
 done
 
 test_expect_success 'update-ref should also create reflog for HEAD' '

-- 
2.49.0

Re: [PATCH 1/3] refs/files: skip updates with errors in batched updates

[Patrick Steinhardt]

On Mon, Jun 02, 2025 at 11:57:24AM +0200, Karthik Nayak wrote:
> The commit 23fc8e4f61 (refs: implement batch reference update support,
> 2025-04-08) introduced support for batched reference updates. This
> allows users to batch updates together, while allowing some of the
> updates to fail.
> 
> Under the hood, batched updates use the reference transaction mechanism.
> Each update which fails is marked as such. Any failed updates must be
> skipped over in the rest of the code, as they wouldn't apply any more.
> In two of the loops within 'files_transaction_finish()' of the files
> backend, the failed updates aren't skipped over. This can cause a
> SEGFAULT otherwise. Add the missing skips and a test to validate the
> same.

Curious -- we do have tests, so why don't any of them hit this issue?

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs/files-backend.c  |  7 +++++++
>  t/t1400-update-ref.sh | 14 ++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 4d1f65a57a..c4a0f29072 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,
>  	 */
>  	for (i = 0; i < transaction->nr; i++) {
>  		struct ref_update *update = transaction->updates[i];
> +
> +		if (update->rejection_err)
> +			continue;
> +
>  		if (update->flags & REF_DELETING &&
>  		    !(update->flags & REF_LOG_ONLY) &&
>  		    !(update->flags & REF_IS_PRUNING)) {

Ok. And the reftable backend doesn't need the same treatment? Probably
doesn't because we queue all updates via `queue_transaction_update()`,
which knows to not add them to the list of updates in case any error has
happened. And in `_finish()` we iterate through that list of queued
updates instead of the global list of updates.

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index d29d23cb89..e9a605d0ba 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -2299,6 +2299,20 @@ do
>  			test_grep -q "refname conflict" stdout
>  		)
>  	'
> +
> +	test_expect_success "stdin $type batch-updates delete non-existent ref" '
> +		git init repo &&
> +		test_when_finished "rm -fr repo" &&
> +		(
> +			cd repo &&
> +			test_commit commit &&
> +			head=$(git rev-parse HEAD) &&
> +
> +			format_command $type "delete refs/heads/non-existent" "$head" >stdin &&
> +			git update-ref $type --stdin --batch-updates <stdin >stdout &&
> +			test_grep -q "reference does not exist" stdout

We typically don't silence the output of `test_grep`.

Patrick

Re: [PATCH 1/3] refs/files: skip updates with errors in batched updates

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3107 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Jun 02, 2025 at 11:57:24AM +0200, Karthik Nayak wrote:
>> The commit 23fc8e4f61 (refs: implement batch reference update support,
>> 2025-04-08) introduced support for batched reference updates. This
>> allows users to batch updates together, while allowing some of the
>> updates to fail.
>>
>> Under the hood, batched updates use the reference transaction mechanism.
>> Each update which fails is marked as such. Any failed updates must be
>> skipped over in the rest of the code, as they wouldn't apply any more.
>> In two of the loops within 'files_transaction_finish()' of the files
>> backend, the failed updates aren't skipped over. This can cause a
>> SEGFAULT otherwise. Add the missing skips and a test to validate the
>> same.
>
> Curious -- we do have tests, so why don't any of them hit this issue?
>

We don't hit this edge case in any of our tests I presume. This
basically requires a deletion request with an expected old OID, however
the reference should be non-existent.

>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  refs/files-backend.c  |  7 +++++++
>>  t/t1400-update-ref.sh | 14 ++++++++++++++
>>  2 files changed, 21 insertions(+)
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 4d1f65a57a..c4a0f29072 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,
>>  	 */
>>  	for (i = 0; i < transaction->nr; i++) {
>>  		struct ref_update *update = transaction->updates[i];
>> +
>> +		if (update->rejection_err)
>> +			continue;
>> +
>>  		if (update->flags & REF_DELETING &&
>>  		    !(update->flags & REF_LOG_ONLY) &&
>>  		    !(update->flags & REF_IS_PRUNING)) {
>
> Ok. And the reftable backend doesn't need the same treatment? Probably
> doesn't because we queue all updates via `queue_transaction_update()`,
> which knows to not add them to the list of updates in case any error has
> happened. And in `_finish()` we iterate through that list of queued
> updates instead of the global list of updates.
>

Exactly, all writes in the reftable backend are propagated finally via
'write_transaction_table()', which checks for rejections. The single
point of entry acts as a good gatekeeper.

>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
>> index d29d23cb89..e9a605d0ba 100755
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -2299,6 +2299,20 @@ do
>>  			test_grep -q "refname conflict" stdout
>>  		)
>>  	'
>> +
>> +	test_expect_success "stdin $type batch-updates delete non-existent ref" '
>> +		git init repo &&
>> +		test_when_finished "rm -fr repo" &&
>> +		(
>> +			cd repo &&
>> +			test_commit commit &&
>> +			head=$(git rev-parse HEAD) &&
>> +
>> +			format_command $type "delete refs/heads/non-existent" "$head" >stdin &&
>> +			git update-ref $type --stdin --batch-updates <stdin >stdout &&
>> +			test_grep -q "reference does not exist" stdout
>
> We typically don't silence the output of `test_grep`.
>
> Patrick

Will change, Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 1/3] refs/files: skip updates with errors in batched updates

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

>> Curious -- we do have tests, so why don't any of them hit this issue?
>>
>
> We don't hit this edge case in any of our tests I presume. This
> basically requires a deletion request with an expected old OID, however
> the reference should be non-existent.

I read Patrick's remart not as a question but as a rhetoric
suggestion to make sure we have test coverage ;-)

Re: [PATCH 1/3] refs/files: skip updates with errors in batched updates

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 548 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>> Curious -- we do have tests, so why don't any of them hit this issue?
>>>
>>
>> We don't hit this edge case in any of our tests I presume. This
>> basically requires a deletion request with an expected old OID, however
>> the reference should be non-existent.
>
> I read Patrick's remart not as a question but as a rhetoric
> suggestion to make sure we have test coverage ;-)

Ah! Okay, all is good then, since my patch does add a test for this now.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH 2/3] t5516: use double quotes for tests with variables

[Karthik Nayak]

Since expressions don't expand within single quotes, change test
descriptions containing variables to use double quotes.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 t/t5516-fetch-push.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index dabcc5f811..029ef92d58 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1421,7 +1421,7 @@ test_expect_success 'peeled advertisements are not considered ref tips' '
 	test_grep "Server does not allow request for unadvertised object" err
 '
 
-test_expect_success 'pushing a specific ref applies remote.$name.push as refmap' '
+test_expect_success "pushing a specific ref applies remote.$name.push as refmap" '
 	mk_test testrepo heads/main &&
 	test_when_finished "rm -rf src" &&
 	git init src &&
@@ -1446,7 +1446,7 @@ test_expect_success 'pushing a specific ref applies remote.$name.push as refmap'
 	test_cmp dst/expect dst/actual
 '
 
-test_expect_success 'with no remote.$name.push, it is not used as refmap' '
+test_expect_success "with no remote.$name.push, it is not used as refmap" '
 	mk_test testrepo heads/main &&
 	test_when_finished "rm -rf src" &&
 	git init src &&
@@ -1469,7 +1469,7 @@ test_expect_success 'with no remote.$name.push, it is not used as refmap' '
 	test_cmp dst/expect dst/actual
 '
 
-test_expect_success 'with no remote.$name.push, upstream mapping is used' '
+test_expect_success "with no remote.$name.push, upstream mapping is used" '
 	mk_test testrepo heads/main &&
 	test_when_finished "rm -rf src" &&
 	git init src &&

-- 
2.49.0

Re: [PATCH 2/3] t5516: use double quotes for tests with variables

[Eric Sunshine]

On Mon, Jun 2, 2025 at 5:58 AM Karthik Nayak <karthik.188@gmail.com> wrote:
> Since expressions don't expand within single quotes, change test
> descriptions containing variables to use double quotes.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> @@ -1421,7 +1421,7 @@ test_expect_success 'peeled advertisements are not considered ref tips' '
> -test_expect_success 'pushing a specific ref applies remote.$name.push as refmap' '
> +test_expect_success "pushing a specific ref applies remote.$name.push as refmap" '

If I'm understanding correctly, I think this and the other changes in
this patch are incorrect. There is no `name` variable in this script.
Rather, these "$name" instances are merely illustrative, acting as
placeholders for the person reading the test title.

Re: [PATCH 2/3] t5516: use double quotes for tests with variables

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Jun 2, 2025 at 5:58 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>> Since expressions don't expand within single quotes, change test
>> descriptions containing variables to use double quotes.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
>> @@ -1421,7 +1421,7 @@ test_expect_success 'peeled advertisements are not considered ref tips' '
>> -test_expect_success 'pushing a specific ref applies remote.$name.push as refmap' '
>> +test_expect_success "pushing a specific ref applies remote.$name.push as refmap" '
>
> If I'm understanding correctly, I think this and the other changes in
> this patch are incorrect. There is no `name` variable in this script.
> Rather, these "$name" instances are merely illustrative, acting as
> placeholders for the person reading the test title.

Huh, you're absolutely right. I think I did a 'grep -v' to ensure it
worked and failed to realize that my shell was replacing $name. Will
skip this patch in the follow up.

Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH 3/3] receive-pack: handle reference deletions separately

[Karthik Nayak]

In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
we updated the 'git-receive-pack(1)' command to use batched reference
updates. One edge case which was missed during this implementation was
when a user pushes multiple branches such as:

  delete refs/heads/branch/conflict
  create refs/heads/branch

Before using batched updates, the references would be applied
sequentially and hence no conflicts would arise. With batched updates,
while the first update applies, the second fails due to F/D conflict. A
similar issue was present in 'git-fetch(1)' and was fixed by using
separating out reference pruning into a separate transaction. Apply a
similar mechanism for 'git-receive-pack(1)' and separate out reference
deletions into its own batch.

Add a test to validate this behavior.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/receive-pack.c           | 23 +++++++++++++++++++----
 t/t1416-ref-transaction-hooks.sh |  2 ++
 t/t5516-fetch-push.sh            | 17 +++++++++++++----
 3 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9e3cfb85cf..7157ced2a6 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1860,7 +1860,8 @@ static void ref_transaction_rejection_handler(const char *refname,
 }
 
 static void execute_commands_non_atomic(struct command *commands,
-					struct shallow_info *si)
+					struct shallow_info *si,
+					int only_deletions)
 {
 	struct command *cmd;
 	struct strbuf err = STRBUF_INIT;
@@ -1879,6 +1880,8 @@ static void execute_commands_non_atomic(struct command *commands,
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
 			continue;
+		if (only_deletions ^ is_null_oid(&cmd->new_oid))
+			continue;
 
 		cmd->error_string = update(cmd, si);
 	}
@@ -2024,6 +2027,9 @@ static void execute_commands(struct command *commands,
 	/*
 	 * If there is no command ready to run, should return directly to destroy
 	 * temporary data in the quarantine area.
+	 *
+	 * Check if any reference deletions exist, these are batched together in
+	 * a separate transaction to avoid F/D conflicts with other updates.
 	 */
 	for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)
 		; /* nothing */
@@ -2058,10 +2064,19 @@ static void execute_commands(struct command *commands,
 			    (cmd->run_proc_receive || use_atomic))
 				cmd->error_string = "fail to run proc-receive hook";
 
-	if (use_atomic)
+	if (use_atomic) {
 		execute_commands_atomic(commands, si);
-	else
-		execute_commands_non_atomic(commands, si);
+	} else {
+		/*
+		 * Reference updates, where F/D conflicts shouldn't arise due to
+		 * one reference being deleted, while the other being created
+		 * are treated as conflicts in batched updates. This is because
+		 * we don't do conflict resolution inside a transaction. To
+		 * mitigate this, delete references in a separate batch.
+		 */
+		execute_commands_non_atomic(commands, si, 1);
+		execute_commands_non_atomic(commands, si, 0);
+	}
 
 	if (shallow_update)
 		BUG_if_skipped_connectivity_check(commands, si);
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index d91dd3a3b5..b2aaa1908f 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -119,6 +119,8 @@ test_expect_success 'interleaving hook calls succeed' '
 	EOF
 
 	cat >expect <<-EOF &&
+		hooks/reference-transaction prepared
+		hooks/reference-transaction committed
 		hooks/update refs/tags/PRE $ZERO_OID $PRE_OID
 		hooks/update refs/tags/POST $ZERO_OID $POST_OID
 		hooks/reference-transaction prepared
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 029ef92d58..34eb3a5a07 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/next $orgnext $newnext
+		refs/heads/main $orgmain $newmain
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -808,8 +808,8 @@ test_expect_success 'deletion of a non-existent ref is not fed to post-receive a
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/nonexistent $ZERO_OID $ZERO_OID
+		refs/heads/main $orgmain $newmain
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -868,10 +868,10 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/next $orgnext $newnext
-		refs/heads/seen $orgseen $newseen
 		refs/heads/nonexistent $ZERO_OID $ZERO_OID
+		refs/heads/main $orgmain $newmain
+		refs/heads/seen $orgseen $newseen
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -1909,4 +1909,13 @@ test_expect_success 'push with config push.useBitmaps' '
 		--thin --delta-base-offset -q --no-use-bitmap-index <false
 '
 
+test_expect_success 'push with F/D conflict with deletion and creation' '
+	test_when_finished "git branch -D branch" &&
+	git branch branch/conflict &&
+	mk_test testrepo heads/branch/conflict &&
+	git branch -D branch/conflict &&
+	git branch branch &&
+	git push testrepo :refs/heads/branch/conflict refs/heads/branch
+'
+
 test_done

-- 
2.49.0

Re: [PATCH 3/3] receive-pack: handle reference deletions separately

[Patrick Steinhardt]

On Mon, Jun 02, 2025 at 11:57:26AM +0200, Karthik Nayak wrote:
> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
> we updated the 'git-receive-pack(1)' command to use batched reference
> updates. One edge case which was missed during this implementation was
> when a user pushes multiple branches such as:
> 
>   delete refs/heads/branch/conflict
>   create refs/heads/branch
> 
> Before using batched updates, the references would be applied
> sequentially and hence no conflicts would arise. With batched updates,
> while the first update applies, the second fails due to F/D conflict. A
> similar issue was present in 'git-fetch(1)' and was fixed by using
> separating out reference pruning into a separate transaction. Apply a
> similar mechanism for 'git-receive-pack(1)' and separate out reference
> deletions into its own batch.
> 
> Add a test to validate this behavior.

Okay. All of this is unfortunate as ideally the reference transaction
itself would know to resolve such conflicts. But we're no worse off than
before because we at most perform exactly two transactions now, whereas
before we would have performed _at least_ two transactions in this
conflicting case.

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  builtin/receive-pack.c           | 23 +++++++++++++++++++----
>  t/t1416-ref-transaction-hooks.sh |  2 ++
>  t/t5516-fetch-push.sh            | 17 +++++++++++++----
>  3 files changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 9e3cfb85cf..7157ced2a6 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1879,6 +1880,8 @@ static void execute_commands_non_atomic(struct command *commands,
>  	for (cmd = commands; cmd; cmd = cmd->next) {
>  		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
>  			continue;
> +		if (only_deletions ^ is_null_oid(&cmd->new_oid))
> +			continue;
>  
>  		cmd->error_string = update(cmd, si);
>  	}

Fancy.

> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 029ef92d58..34eb3a5a07 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
>  		EOF
>  
>  		cat >update.expect <<-EOF &&
> -		refs/heads/main $orgmain $newmain
>  		refs/heads/next $orgnext $newnext
> +		refs/heads/main $orgmain $newmain
>  		EOF
>  
>  		cat >post-receive.expect <<-EOF &&

Hm, so the ordering does change now as all deletes will now be listed
before the updates. We don't make any guarantees about how these are
sorted, but it makes me a bit uneasy to see this change. Can we avoid
this change in behaviour somehow?

Patrick

Re: [PATCH 3/3] receive-pack: handle reference deletions separately

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3382 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Jun 02, 2025 at 11:57:26AM +0200, Karthik Nayak wrote:
>> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
>> we updated the 'git-receive-pack(1)' command to use batched reference
>> updates. One edge case which was missed during this implementation was
>> when a user pushes multiple branches such as:
>>
>>   delete refs/heads/branch/conflict
>>   create refs/heads/branch
>>
>> Before using batched updates, the references would be applied
>> sequentially and hence no conflicts would arise. With batched updates,
>> while the first update applies, the second fails due to F/D conflict. A
>> similar issue was present in 'git-fetch(1)' and was fixed by using
>> separating out reference pruning into a separate transaction. Apply a
>> similar mechanism for 'git-receive-pack(1)' and separate out reference
>> deletions into its own batch.
>>
>> Add a test to validate this behavior.
>
> Okay. All of this is unfortunate as ideally the reference transaction
> itself would know to resolve such conflicts. But we're no worse off than
> before because we at most perform exactly two transactions now, whereas
> before we would have performed _at least_ two transactions in this
> conflicting case.
>

Exactly. It is not easy to build in conflict resolution but it would be
great to have it. Also to some extent F/D conflicts are due to
implementation details. With the reftable backend, we artificially
introduce this limitation, maybe someday if we get rid of the files
backend, we won't even have this issue.

Thanks for spelling this out, I think it would be worthwhile to add this
to the commit message too. I will do that.

>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  builtin/receive-pack.c           | 23 +++++++++++++++++++----
>>  t/t1416-ref-transaction-hooks.sh |  2 ++
>>  t/t5516-fetch-push.sh            | 17 +++++++++++++----
>>  3 files changed, 34 insertions(+), 8 deletions(-)
>>
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 9e3cfb85cf..7157ced2a6 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1879,6 +1880,8 @@ static void execute_commands_non_atomic(struct command *commands,
>>  	for (cmd = commands; cmd; cmd = cmd->next) {
>>  		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
>>  			continue;
>> +		if (only_deletions ^ is_null_oid(&cmd->new_oid))
>> +			continue;
>>
>>  		cmd->error_string = update(cmd, si);
>>  	}
>
> Fancy.
>
>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
>> index 029ef92d58..34eb3a5a07 100755
>> --- a/t/t5516-fetch-push.sh
>> +++ b/t/t5516-fetch-push.sh
>> @@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
>>  		EOF
>>
>>  		cat >update.expect <<-EOF &&
>> -		refs/heads/main $orgmain $newmain
>>  		refs/heads/next $orgnext $newnext
>> +		refs/heads/main $orgmain $newmain
>>  		EOF
>>
>>  		cat >post-receive.expect <<-EOF &&
>
> Hm, so the ordering does change now as all deletes will now be listed
> before the updates. We don't make any guarantees about how these are
> sorted, but it makes me a bit uneasy to see this change. Can we avoid
> this change in behaviour somehow?
>

Yeah it does. I couldn't think of a way thought to retain this behavior.
But I will spend some time on it.

> Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 3/3] receive-pack: handle reference deletions separately

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Okay. All of this is unfortunate as ideally the reference transaction
> itself would know to resolve such conflicts.

100% agreed.

> But we're no worse off than
> before because we at most perform exactly two transactions now, whereas
> before we would have performed _at least_ two transactions in this
> conflicting case.
>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  builtin/receive-pack.c           | 23 +++++++++++++++++++----
>>  t/t1416-ref-transaction-hooks.sh |  2 ++
>>  t/t5516-fetch-push.sh            | 17 +++++++++++++----
>>  3 files changed, 34 insertions(+), 8 deletions(-)
>> 
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 9e3cfb85cf..7157ced2a6 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1879,6 +1880,8 @@ static void execute_commands_non_atomic(struct command *commands,
>>  	for (cmd = commands; cmd; cmd = cmd->next) {
>>  		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
>>  			continue;
>> +		if (only_deletions ^ is_null_oid(&cmd->new_oid))
>> +			continue;
>>  
>>  		cmd->error_string = update(cmd, si);
>>  	}
>
> Fancy.

Is that a new synonym for "not worth being overly clever to
sacrifice readability"?

This may be a comment for [2/3], but a two-call sequence

	doit(only_deletions = yes);
	doit(only_deletions = no);

looked somewhat iffy for a first reader, as it hints that the second
call would do both non-deletions (i.e. creation and modification)
and deletions, which makes readers wonder "so we delete twice and
rely on that it is not an error to delete something that does not
exist?"


>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
>> index 029ef92d58..34eb3a5a07 100755
>> --- a/t/t5516-fetch-push.sh
>> +++ b/t/t5516-fetch-push.sh
>> @@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
>>  		EOF
>>  
>>  		cat >update.expect <<-EOF &&
>> -		refs/heads/main $orgmain $newmain
>>  		refs/heads/next $orgnext $newnext
>> +		refs/heads/main $orgmain $newmain
>>  		EOF
>>  
>>  		cat >post-receive.expect <<-EOF &&
>
> Hm, so the ordering does change now as all deletes will now be listed
> before the updates. We don't make any guarantees about how these are
> sorted, but it makes me a bit uneasy to see this change. Can we avoid
> this change in behaviour somehow?

Good eyes.

I was wondering about the "git push -v" reporting and was happy that
the order there follows the order the pushing side listed refs and
the reordering on the receiving end would not have any effect.  The
hooks on the receiving end can indeed observe this change.

They can observe, but can they notice?  If the pusher listed refspec
elements for deletions first before creations and modifications on
their command line, that would be what the hooks see.  They do not
know what the original "push" said so they have nothing to compare
and complain.

Ahhh, but humans that control the both ends may notice and complain.

OK, I think I agree with you that it is worth to at least spend some
brain cycles thinking about avoiding the behaviour change.

Thanks.

Re: [PATCH 3/3] receive-pack: handle reference deletions separately

[Patrick Steinhardt]

On Mon, Jun 02, 2025 at 08:20:09AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> >> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> >> index 9e3cfb85cf..7157ced2a6 100644
> >> --- a/builtin/receive-pack.c
> >> +++ b/builtin/receive-pack.c
> >> @@ -1879,6 +1880,8 @@ static void execute_commands_non_atomic(struct command *commands,
> >>  	for (cmd = commands; cmd; cmd = cmd->next) {
> >>  		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
> >>  			continue;
> >> +		if (only_deletions ^ is_null_oid(&cmd->new_oid))
> >> +			continue;
> >>  
> >>  		cmd->error_string = update(cmd, si);
> >>  	}
> >
> > Fancy.
> 
> Is that a new synonym for "not worth being overly clever to
> sacrifice readability"?

Yeah. I wasn't quite sure whether I like it or not as it felt a bit too
clever to me indeed. But I didn't feel strongly about it either, so it
turned into the above somewhat unhelpful remark.

> This may be a comment for [2/3], but a two-call sequence
> 
> 	doit(only_deletions = yes);
> 	doit(only_deletions = no);
> 
> looked somewhat iffy for a first reader, as it hints that the second
> call would do both non-deletions (i.e. creation and modification)
> and deletions, which makes readers wonder "so we delete twice and
> rely on that it is not an error to delete something that does not
> exist?"

I also wondered whether it wouldn't be nicer to have the loop in
`doit()` itself. It would require a bit of reindenting though, which is
why I didn't propose that variant.

> >> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> >> index 029ef92d58..34eb3a5a07 100755
> >> --- a/t/t5516-fetch-push.sh
> >> +++ b/t/t5516-fetch-push.sh
> >> @@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
> >>  		EOF
> >>  
> >>  		cat >update.expect <<-EOF &&
> >> -		refs/heads/main $orgmain $newmain
> >>  		refs/heads/next $orgnext $newnext
> >> +		refs/heads/main $orgmain $newmain
> >>  		EOF
> >>  
> >>  		cat >post-receive.expect <<-EOF &&
> >
> > Hm, so the ordering does change now as all deletes will now be listed
> > before the updates. We don't make any guarantees about how these are
> > sorted, but it makes me a bit uneasy to see this change. Can we avoid
> > this change in behaviour somehow?
> 
> Good eyes.
> 
> I was wondering about the "git push -v" reporting and was happy that
> the order there follows the order the pushing side listed refs and
> the reordering on the receiving end would not have any effect.  The
> hooks on the receiving end can indeed observe this change.
> 
> They can observe, but can they notice?  If the pusher listed refspec
> elements for deletions first before creations and modifications on
> their command line, that would be what the hooks see.  They do not
> know what the original "push" said so they have nothing to compare
> and complain.
> 
> Ahhh, but humans that control the both ends may notice and complain.
> 
> OK, I think I agree with you that it is worth to at least spend some
> brain cycles thinking about avoiding the behaviour change.

Hm. The thing I didn't realize is that the changed output order is for
the "update" hook. I thought it was for the "post-receive" hook, where I
do expect that ordering may matter as you see the whole picture of all
refs that have been updated. But for the "update" hook I think it is
sensible to change the ordering -- after all, the order of updates does
change a bit now. Furthermore, the "update" hook itself doesn't have the
complete picture anyway, so it's way less likely that anybody relies on
the ordering.

Patrick

Re: [PATCH 3/3] receive-pack: handle reference deletions separately

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 4205 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Jun 02, 2025 at 08:20:09AM -0700, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> >> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> >> index 9e3cfb85cf..7157ced2a6 100644
>> >> --- a/builtin/receive-pack.c
>> >> +++ b/builtin/receive-pack.c
>> >> @@ -1879,6 +1880,8 @@ static void execute_commands_non_atomic(struct command *commands,
>> >>  	for (cmd = commands; cmd; cmd = cmd->next) {
>> >>  		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
>> >>  			continue;
>> >> +		if (only_deletions ^ is_null_oid(&cmd->new_oid))
>> >> +			continue;
>> >>
>> >>  		cmd->error_string = update(cmd, si);
>> >>  	}
>> >
>> > Fancy.
>>
>> Is that a new synonym for "not worth being overly clever to
>> sacrifice readability"?
>
> Yeah. I wasn't quite sure whether I like it or not as it felt a bit too
> clever to me indeed. But I didn't feel strongly about it either, so it
> turned into the above somewhat unhelpful remark.
>

I will make it easier on the eyes in the next version.

>> This may be a comment for [2/3], but a two-call sequence
>>
>> 	doit(only_deletions = yes);
>> 	doit(only_deletions = no);
>>
>> looked somewhat iffy for a first reader, as it hints that the second
>> call would do both non-deletions (i.e. creation and modification)
>> and deletions, which makes readers wonder "so we delete twice and
>> rely on that it is not an error to delete something that does not
>> exist?"
>
> I also wondered whether it wouldn't be nicer to have the loop in
> `doit()` itself. It would require a bit of reindenting though, which is
> why I didn't propose that variant.
>

This is mostly what I was trying to avoid, but perhaps the way to go
anyways. So I'll do the needfull.

>> >> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
>> >> index 029ef92d58..34eb3a5a07 100755
>> >> --- a/t/t5516-fetch-push.sh
>> >> +++ b/t/t5516-fetch-push.sh
>> >> @@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
>> >>  		EOF
>> >>
>> >>  		cat >update.expect <<-EOF &&
>> >> -		refs/heads/main $orgmain $newmain
>> >>  		refs/heads/next $orgnext $newnext
>> >> +		refs/heads/main $orgmain $newmain
>> >>  		EOF
>> >>
>> >>  		cat >post-receive.expect <<-EOF &&
>> >
>> > Hm, so the ordering does change now as all deletes will now be listed
>> > before the updates. We don't make any guarantees about how these are
>> > sorted, but it makes me a bit uneasy to see this change. Can we avoid
>> > this change in behaviour somehow?
>>
>> Good eyes.
>>
>> I was wondering about the "git push -v" reporting and was happy that
>> the order there follows the order the pushing side listed refs and
>> the reordering on the receiving end would not have any effect.  The
>> hooks on the receiving end can indeed observe this change.
>>
>> They can observe, but can they notice?  If the pusher listed refspec
>> elements for deletions first before creations and modifications on
>> their command line, that would be what the hooks see.  They do not
>> know what the original "push" said so they have nothing to compare
>> and complain.
>>
>> Ahhh, but humans that control the both ends may notice and complain.
>>
>> OK, I think I agree with you that it is worth to at least spend some
>> brain cycles thinking about avoiding the behaviour change.
>
> Hm. The thing I didn't realize is that the changed output order is for
> the "update" hook. I thought it was for the "post-receive" hook, where I
> do expect that ordering may matter as you see the whole picture of all
> refs that have been updated. But for the "update" hook I think it is
> sensible to change the ordering -- after all, the order of updates does
> change a bit now. Furthermore, the "update" hook itself doesn't have the
> complete picture anyway, so it's way less likely that anybody relies on
> the ordering.
>
> Patrick

You're correct. I still think it'd be nice to have the 'update' hook
also be in the same order. But like you said, its okay. I do plan to
take up conflict resolution within transactions eventually, so that
would also fix this. So I'm going to leave this as is for now.

Karthik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v2 0/2] refs: fix some bugs with batched-updates

[Karthik Nayak]

In 23fc8e4f61 (refs: implement batch reference update support,
2025-04-08) we introduced a mechanism to batch reference updates. The
idea being that we wanted to batch updates similar to a transaction, but
allow certain updates to fail. This would help reference backends
optimize such operations and also remove the overhead of processing
updates individually. Especially for the reftable backend, where
batching updates would ensure that the autocompaction would only occur
at the end of the batch instead of after every reference update.

As of 9d2962a7c4 (receive-pack: use batched reference updates,
2025-05-19) we also updated the 'git-fetch(1)' and 'git-receive-pack(1)'
command to use batched reference updates. This series fixes some bugs
that we found at GitLab by running our Gitaly service with the `next`
build of Git.

The first being in the files backend, which missed skipping over failed
updates in certain flows. When certain individual updates fail, we mark
them as such. However, we missed skipping over such failed updates,
which can cause a SEGFAULT.

The other is in the git-receive-pack(1) implementation when a user
pushes multiple branches such as:

  delete refs/heads/branch/conflict
  create refs/heads/branch

Before using batched updates, the references would be applied
sequentially and hence no conflicts would arise. With batched updates,
while the first update applies, the second fails due to F/D conflict. A
similar issue was present in 'git-fetch(1)' and was fixed by using
separating out reference pruning into a separate transaction. Apply a
similar mechanism for 'git-receive-pack(1)' and separate out reference
deletions into its own batch.

This is based off master 7014b55638 (A bit more topics for -rc1,
2025-05-30), with the changes from kn/fetch-push-bulk-ref-update merged
in.

---
Changes in v2:
- Modify the test in the first commit to no longer do a quiet grep,
  and some more tests.
- Remove the second commit as it was unnecessary.
- Modify the commit message in the last commit, to also talk about how
  we now use 2 transactions at minimum but this is still better than
  before.
- Modify the logic in the last commit to no longer use an XOR and
  instead loop over the two different scenarios (deletion updates, other
  updates).
- Link to v1: https://lore.kernel.org/r/20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-0-903d1db3f10e@gmail.com

---
 builtin/receive-pack.c           | 90 +++++++++++++++++++++++++---------------
 refs/files-backend.c             |  7 ++++
 t/t1400-update-ref.sh            | 45 ++++++++++++++++++++
 t/t1416-ref-transaction-hooks.sh |  2 +
 t/t5516-fetch-push.sh            | 17 ++++++--
 5 files changed, 123 insertions(+), 38 deletions(-)

Karthik Nayak (2):
      refs/files: skip updates with errors in batched updates
      receive-pack: handle reference deletions separately

Range-diff versus v1:

1:  1c0ea8e209 ! 1:  65bcb5d2cf refs/files: skip updates with errors in batched updates
    @@ t/t1400-update-ref.sh: do
      		)
      	'
     +
    ++	test_expect_success "stdin $type batch-updates delete incorrect symbolic ref" '
    ++		git init repo &&
    ++		test_when_finished "rm -fr repo" &&
    ++		(
    ++			cd repo &&
    ++			test_commit c1 &&
    ++			head=$(git rev-parse HEAD) &&
    ++			git symbolic-ref refs/heads/symbolic refs/heads/non-existent &&
    ++
    ++			format_command $type "delete refs/heads/symbolic" "$head" >stdin &&
    ++			git update-ref $type --stdin --batch-updates <stdin >stdout &&
    ++			test_grep "reference does not exist" stdout
    ++		)
    ++	'
    ++
    ++	test_expect_success "stdin $type batch-updates delete with incorrect old_oid" '
    ++		git init repo &&
    ++		test_when_finished "rm -fr repo" &&
    ++		(
    ++			cd repo &&
    ++			test_commit c1 &&
    ++			git branch new-branch &&
    ++			test_commit c2 &&
    ++			head=$(git rev-parse HEAD) &&
    ++
    ++			format_command $type "delete refs/heads/new-branch" "$head" >stdin &&
    ++			git update-ref $type --stdin --batch-updates <stdin >stdout &&
    ++			test_grep "incorrect old value provided" stdout
    ++		)
    ++	'
    ++
     +	test_expect_success "stdin $type batch-updates delete non-existent ref" '
     +		git init repo &&
     +		test_when_finished "rm -fr repo" &&
    @@ t/t1400-update-ref.sh: do
     +
     +			format_command $type "delete refs/heads/non-existent" "$head" >stdin &&
     +			git update-ref $type --stdin --batch-updates <stdin >stdout &&
    -+			test_grep -q "reference does not exist" stdout
    ++			test_grep "reference does not exist" stdout
     +		)
     +	'
      done
2:  5c21a3770e < -:  ---------- t5516: use double quotes for tests with variables
3:  38eb79bd41 ! 2:  e65b29b5f1 receive-pack: handle reference deletions separately
    @@ Commit message
         similar mechanism for 'git-receive-pack(1)' and separate out reference
         deletions into its own batch.
     
    +    This means 'git-receive-pack(1)' will now use exactly two transactions,
    +    whereas before using batched updates it would use _at least_ two
    +    transactions. So using batched updates is the still the better option.
    +
         Add a test to validate this behavior.
     
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
      ## builtin/receive-pack.c ##
    -@@ builtin/receive-pack.c: static void ref_transaction_rejection_handler(const char *refname,
    +@@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *commands,
    + 	const char *reported_error = NULL;
    + 	struct strmap failed_refs = STRMAP_INIT;
    + 
    +-	transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
    +-						  REF_TRANSACTION_ALLOW_FAILURE, &err);
    +-	if (!transaction) {
    +-		rp_error("%s", err.buf);
    +-		strbuf_reset(&err);
    +-		reported_error = "transaction failed to start";
    +-		goto failure;
    +-	}
    ++	/*
    ++	 * Reference updates, where F/D conflicts shouldn't arise due to
    ++	 * one reference being deleted, while the other being created
    ++	 * are treated as conflicts in batched updates. This is because
    ++	 * we don't do conflict resolution inside a transaction. To
    ++	 * mitigate this, delete references in a separate batch.
    ++	 */
    ++	enum processing_phase {
    ++		PHASE_DELETIONS,
    ++		PHASE_OTHERS
    ++	};
    + 
    +-	for (cmd = commands; cmd; cmd = cmd->next) {
    +-		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
    +-			continue;
    ++	for (int phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {
    ++		transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
    ++							  REF_TRANSACTION_ALLOW_FAILURE, &err);
    ++		if (!transaction) {
    ++			rp_error("%s", err.buf);
    ++			strbuf_reset(&err);
    ++			reported_error = "transaction failed to s1tart";
    ++			goto failure;
    ++		}
    + 
    +-		cmd->error_string = update(cmd, si);
    +-	}
    ++		for (cmd = commands; cmd; cmd = cmd->next) {
    ++			if (!should_process_cmd(cmd) || cmd->run_proc_receive)
    ++				continue;
    + 
    +-	if (ref_transaction_commit(transaction, &err)) {
    +-		rp_error("%s", err.buf);
    +-		reported_error = "failed to update refs";
    +-		goto failure;
    +-	}
    ++			if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))
    ++				continue;
    ++			else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))
    ++				continue;
    + 
    +-	ref_transaction_for_each_rejected_update(transaction,
    +-						 ref_transaction_rejection_handler,
    +-						 &failed_refs);
    ++			cmd->error_string = update(cmd, si);
    ++		}
    + 
    +-	if (strmap_empty(&failed_refs))
    +-		goto cleanup;
    ++		if (ref_transaction_commit(transaction, &err)) {
    ++			rp_error("%s", err.buf);
    ++			reported_error = "failed to update refs";
    ++			goto failure;
    ++		}
    + 
    +-failure:
    +-	for (cmd = commands; cmd; cmd = cmd->next) {
    +-		if (reported_error)
    +-			cmd->error_string = reported_error;
    +-		else if (strmap_contains(&failed_refs, cmd->ref_name))
    +-			cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
    +-	}
    ++		ref_transaction_for_each_rejected_update(transaction,
    ++							 ref_transaction_rejection_handler,
    ++							 &failed_refs);
    + 
    +-cleanup:
    +-	ref_transaction_free(transaction);
    +-	strmap_clear(&failed_refs, 0);
    +-	strbuf_release(&err);
    ++		if (strmap_empty(&failed_refs))
    ++			goto cleanup;
    ++
    ++	failure:
    ++		for (cmd = commands; cmd; cmd = cmd->next) {
    ++			if (reported_error)
    ++				cmd->error_string = reported_error;
    ++			else if (strmap_contains(&failed_refs, cmd->ref_name))
    ++				cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
    ++		}
    ++
    ++	cleanup:
    ++		ref_transaction_free(transaction);
    ++		strmap_clear(&failed_refs, 0);
    ++		strbuf_release(&err);
    ++	}
      }
      
    - static void execute_commands_non_atomic(struct command *commands,
    --					struct shallow_info *si)
    -+					struct shallow_info *si,
    -+					int only_deletions)
    - {
    - 	struct command *cmd;
    - 	struct strbuf err = STRBUF_INIT;
    -@@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *commands,
    - 	for (cmd = commands; cmd; cmd = cmd->next) {
    - 		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
    - 			continue;
    -+		if (only_deletions ^ is_null_oid(&cmd->new_oid))
    -+			continue;
    - 
    - 		cmd->error_string = update(cmd, si);
    - 	}
    + static void execute_commands_atomic(struct command *commands,
     @@ builtin/receive-pack.c: static void execute_commands(struct command *commands,
      	/*
      	 * If there is no command ready to run, should return directly to destroy
    @@ builtin/receive-pack.c: static void execute_commands(struct command *commands,
      	 */
      	for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)
      		; /* nothing */
    -@@ builtin/receive-pack.c: static void execute_commands(struct command *commands,
    - 			    (cmd->run_proc_receive || use_atomic))
    - 				cmd->error_string = "fail to run proc-receive hook";
    - 
    --	if (use_atomic)
    -+	if (use_atomic) {
    - 		execute_commands_atomic(commands, si);
    --	else
    --		execute_commands_non_atomic(commands, si);
    -+	} else {
    -+		/*
    -+		 * Reference updates, where F/D conflicts shouldn't arise due to
    -+		 * one reference being deleted, while the other being created
    -+		 * are treated as conflicts in batched updates. This is because
    -+		 * we don't do conflict resolution inside a transaction. To
    -+		 * mitigate this, delete references in a separate batch.
    -+		 */
    -+		execute_commands_non_atomic(commands, si, 1);
    -+		execute_commands_non_atomic(commands, si, 0);
    -+	}
    - 
    - 	if (shallow_update)
    - 		BUG_if_skipped_connectivity_check(commands, si);
     
      ## t/t1416-ref-transaction-hooks.sh ##
     @@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'interleaving hook calls succeed' '


base-commit: 931c39f05e078e0df968a439379cb04b5c4666ef
change-id: 20250528-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-24ff53680144

Thanks
- Karthik

[PATCH v2 1/2] refs/files: skip updates with errors in batched updates

[Karthik Nayak]

The commit 23fc8e4f61 (refs: implement batch reference update support,
2025-04-08) introduced support for batched reference updates. This
allows users to batch updates together, while allowing some of the
updates to fail.

Under the hood, batched updates use the reference transaction mechanism.
Each update which fails is marked as such. Any failed updates must be
skipped over in the rest of the code, as they wouldn't apply any more.
In two of the loops within 'files_transaction_finish()' of the files
backend, the failed updates aren't skipped over. This can cause a
SEGFAULT otherwise. Add the missing skips and a test to validate the
same.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c  |  7 +++++++
 t/t1400-update-ref.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4d1f65a57a..c4a0f29072 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,
 	 */
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
+
+		if (update->rejection_err)
+			continue;
+
 		if (update->flags & REF_DELETING &&
 		    !(update->flags & REF_LOG_ONLY) &&
 		    !(update->flags & REF_IS_PRUNING)) {
@@ -3239,6 +3243,9 @@ static int files_transaction_finish(struct ref_store *ref_store,
 		struct ref_update *update = transaction->updates[i];
 		struct ref_lock *lock = update->backend_data;
 
+		if (update->rejection_err)
+			continue;
+
 		if (update->flags & REF_DELETING &&
 		    !(update->flags & REF_LOG_ONLY)) {
 			update->flags |= REF_DELETED_RMDIR;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index d29d23cb89..ca7eee7de2 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -2299,6 +2299,51 @@ do
 			test_grep -q "refname conflict" stdout
 		)
 	'
+
+	test_expect_success "stdin $type batch-updates delete incorrect symbolic ref" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit c1 &&
+			head=$(git rev-parse HEAD) &&
+			git symbolic-ref refs/heads/symbolic refs/heads/non-existent &&
+
+			format_command $type "delete refs/heads/symbolic" "$head" >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+			test_grep "reference does not exist" stdout
+		)
+	'
+
+	test_expect_success "stdin $type batch-updates delete with incorrect old_oid" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit c1 &&
+			git branch new-branch &&
+			test_commit c2 &&
+			head=$(git rev-parse HEAD) &&
+
+			format_command $type "delete refs/heads/new-branch" "$head" >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+			test_grep "incorrect old value provided" stdout
+		)
+	'
+
+	test_expect_success "stdin $type batch-updates delete non-existent ref" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit commit &&
+			head=$(git rev-parse HEAD) &&
+
+			format_command $type "delete refs/heads/non-existent" "$head" >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+			test_grep "reference does not exist" stdout
+		)
+	'
 done
 
 test_expect_success 'update-ref should also create reflog for HEAD' '

-- 
2.49.0

[PATCH v2 2/2] receive-pack: handle reference deletions separately

[Karthik Nayak]

In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
we updated the 'git-receive-pack(1)' command to use batched reference
updates. One edge case which was missed during this implementation was
when a user pushes multiple branches such as:

  delete refs/heads/branch/conflict
  create refs/heads/branch

Before using batched updates, the references would be applied
sequentially and hence no conflicts would arise. With batched updates,
while the first update applies, the second fails due to F/D conflict. A
similar issue was present in 'git-fetch(1)' and was fixed by using
separating out reference pruning into a separate transaction. Apply a
similar mechanism for 'git-receive-pack(1)' and separate out reference
deletions into its own batch.

This means 'git-receive-pack(1)' will now use exactly two transactions,
whereas before using batched updates it would use _at least_ two
transactions. So using batched updates is the still the better option.

Add a test to validate this behavior.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/receive-pack.c           | 90 +++++++++++++++++++++++++---------------
 t/t1416-ref-transaction-hooks.sh |  2 +
 t/t5516-fetch-push.sh            | 17 ++++++--
 3 files changed, 71 insertions(+), 38 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9e3cfb85cf..34db4377ca 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1867,47 +1867,66 @@ static void execute_commands_non_atomic(struct command *commands,
 	const char *reported_error = NULL;
 	struct strmap failed_refs = STRMAP_INIT;
 
-	transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
-						  REF_TRANSACTION_ALLOW_FAILURE, &err);
-	if (!transaction) {
-		rp_error("%s", err.buf);
-		strbuf_reset(&err);
-		reported_error = "transaction failed to start";
-		goto failure;
-	}
+	/*
+	 * Reference updates, where F/D conflicts shouldn't arise due to
+	 * one reference being deleted, while the other being created
+	 * are treated as conflicts in batched updates. This is because
+	 * we don't do conflict resolution inside a transaction. To
+	 * mitigate this, delete references in a separate batch.
+	 */
+	enum processing_phase {
+		PHASE_DELETIONS,
+		PHASE_OTHERS
+	};
 
-	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
-			continue;
+	for (int phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {
+		transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
+							  REF_TRANSACTION_ALLOW_FAILURE, &err);
+		if (!transaction) {
+			rp_error("%s", err.buf);
+			strbuf_reset(&err);
+			reported_error = "transaction failed to s1tart";
+			goto failure;
+		}
 
-		cmd->error_string = update(cmd, si);
-	}
+		for (cmd = commands; cmd; cmd = cmd->next) {
+			if (!should_process_cmd(cmd) || cmd->run_proc_receive)
+				continue;
 
-	if (ref_transaction_commit(transaction, &err)) {
-		rp_error("%s", err.buf);
-		reported_error = "failed to update refs";
-		goto failure;
-	}
+			if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))
+				continue;
+			else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))
+				continue;
 
-	ref_transaction_for_each_rejected_update(transaction,
-						 ref_transaction_rejection_handler,
-						 &failed_refs);
+			cmd->error_string = update(cmd, si);
+		}
 
-	if (strmap_empty(&failed_refs))
-		goto cleanup;
+		if (ref_transaction_commit(transaction, &err)) {
+			rp_error("%s", err.buf);
+			reported_error = "failed to update refs";
+			goto failure;
+		}
 
-failure:
-	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (reported_error)
-			cmd->error_string = reported_error;
-		else if (strmap_contains(&failed_refs, cmd->ref_name))
-			cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
-	}
+		ref_transaction_for_each_rejected_update(transaction,
+							 ref_transaction_rejection_handler,
+							 &failed_refs);
 
-cleanup:
-	ref_transaction_free(transaction);
-	strmap_clear(&failed_refs, 0);
-	strbuf_release(&err);
+		if (strmap_empty(&failed_refs))
+			goto cleanup;
+
+	failure:
+		for (cmd = commands; cmd; cmd = cmd->next) {
+			if (reported_error)
+				cmd->error_string = reported_error;
+			else if (strmap_contains(&failed_refs, cmd->ref_name))
+				cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
+		}
+
+	cleanup:
+		ref_transaction_free(transaction);
+		strmap_clear(&failed_refs, 0);
+		strbuf_release(&err);
+	}
 }
 
 static void execute_commands_atomic(struct command *commands,
@@ -2024,6 +2043,9 @@ static void execute_commands(struct command *commands,
 	/*
 	 * If there is no command ready to run, should return directly to destroy
 	 * temporary data in the quarantine area.
+	 *
+	 * Check if any reference deletions exist, these are batched together in
+	 * a separate transaction to avoid F/D conflicts with other updates.
 	 */
 	for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)
 		; /* nothing */
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index d91dd3a3b5..b2aaa1908f 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -119,6 +119,8 @@ test_expect_success 'interleaving hook calls succeed' '
 	EOF
 
 	cat >expect <<-EOF &&
+		hooks/reference-transaction prepared
+		hooks/reference-transaction committed
 		hooks/update refs/tags/PRE $ZERO_OID $PRE_OID
 		hooks/update refs/tags/POST $ZERO_OID $POST_OID
 		hooks/reference-transaction prepared
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index dabcc5f811..1649667441 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/next $orgnext $newnext
+		refs/heads/main $orgmain $newmain
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -808,8 +808,8 @@ test_expect_success 'deletion of a non-existent ref is not fed to post-receive a
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/nonexistent $ZERO_OID $ZERO_OID
+		refs/heads/main $orgmain $newmain
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -868,10 +868,10 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/next $orgnext $newnext
-		refs/heads/seen $orgseen $newseen
 		refs/heads/nonexistent $ZERO_OID $ZERO_OID
+		refs/heads/main $orgmain $newmain
+		refs/heads/seen $orgseen $newseen
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -1909,4 +1909,13 @@ test_expect_success 'push with config push.useBitmaps' '
 		--thin --delta-base-offset -q --no-use-bitmap-index <false
 '
 
+test_expect_success 'push with F/D conflict with deletion and creation' '
+	test_when_finished "git branch -D branch" &&
+	git branch branch/conflict &&
+	mk_test testrepo heads/branch/conflict &&
+	git branch -D branch/conflict &&
+	git branch branch &&
+	git push testrepo :refs/heads/branch/conflict refs/heads/branch
+'
+
 test_done

-- 
2.49.0

Re: [PATCH v2 2/2] receive-pack: handle reference deletions separately

[Patrick Steinhardt]

On Thu, Jun 05, 2025 at 10:19:55AM +0200, Karthik Nayak wrote:
> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
> we updated the 'git-receive-pack(1)' command to use batched reference
> updates. One edge case which was missed during this implementation was
> when a user pushes multiple branches such as:
> 
>   delete refs/heads/branch/conflict
>   create refs/heads/branch
> 
> Before using batched updates, the references would be applied
> sequentially and hence no conflicts would arise. With batched updates,
> while the first update applies, the second fails due to F/D conflict. A
> similar issue was present in 'git-fetch(1)' and was fixed by using
> separating out reference pruning into a separate transaction. Apply a
> similar mechanism for 'git-receive-pack(1)' and separate out reference
> deletions into its own batch.
> 
> This means 'git-receive-pack(1)' will now use exactly two transactions,
> whereas before using batched updates it would use _at least_ two
> transactions. So using batched updates is the still the better option.

s/the still the/still the/

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 9e3cfb85cf..34db4377ca 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1867,47 +1867,66 @@ static void execute_commands_non_atomic(struct command *commands,
>  	const char *reported_error = NULL;
>  	struct strmap failed_refs = STRMAP_INIT;
>  
> -	transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> -						  REF_TRANSACTION_ALLOW_FAILURE, &err);
> -	if (!transaction) {
> -		rp_error("%s", err.buf);
> -		strbuf_reset(&err);
> -		reported_error = "transaction failed to start";
> -		goto failure;
> -	}
> +	/*
> +	 * Reference updates, where F/D conflicts shouldn't arise due to
> +	 * one reference being deleted, while the other being created
> +	 * are treated as conflicts in batched updates. This is because
> +	 * we don't do conflict resolution inside a transaction. To
> +	 * mitigate this, delete references in a separate batch.
> +	 */
> +	enum processing_phase {
> +		PHASE_DELETIONS,
> +		PHASE_OTHERS
> +	};
>  
> -	for (cmd = commands; cmd; cmd = cmd->next) {
> -		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
> -			continue;
> +	for (int phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {

s/int/enum processing_phase/

Doesn't make any difference, but it feels a bit cleaner.

> +		transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> +							  REF_TRANSACTION_ALLOW_FAILURE, &err);
> +		if (!transaction) {
> +			rp_error("%s", err.buf);
> +			strbuf_reset(&err);
> +			reported_error = "transaction failed to s1tart";
> +			goto failure;
> +		}

So if the transaction doesn't contain any deletions we'd now commit an
empty transaction. The same is true the other way round, in case there
are only deletions. Do we maybe want to skip phases when there is no
match? Ideally, we wouldn't even be starting a transaction.

We could for example skip forward to the first command that we would
have to queue. If there is no such command we continue the loop, if
there is we can remember that command, begin the transaction and start
queueing from there.

> @@ -2024,6 +2043,9 @@ static void execute_commands(struct command *commands,
>  	/*
>  	 * If there is no command ready to run, should return directly to destroy
>  	 * temporary data in the quarantine area.
> +	 *
> +	 * Check if any reference deletions exist, these are batched together in
> +	 * a separate transaction to avoid F/D conflicts with other updates.
>  	 */

Is this comment still accurate?

>  	for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)
>  		; /* nothing */
> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
> index d91dd3a3b5..b2aaa1908f 100755
> --- a/t/t1416-ref-transaction-hooks.sh
> +++ b/t/t1416-ref-transaction-hooks.sh
> @@ -119,6 +119,8 @@ test_expect_success 'interleaving hook calls succeed' '
>  	EOF
>  
>  	cat >expect <<-EOF &&
> +		hooks/reference-transaction prepared
> +		hooks/reference-transaction committed

Yeah, this shows the empty commits indeed.

Patrick

Re: [PATCH v2 2/2] receive-pack: handle reference deletions separately

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 4529 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Thu, Jun 05, 2025 at 10:19:55AM +0200, Karthik Nayak wrote:
>> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
>> we updated the 'git-receive-pack(1)' command to use batched reference
>> updates. One edge case which was missed during this implementation was
>> when a user pushes multiple branches such as:
>>
>>   delete refs/heads/branch/conflict
>>   create refs/heads/branch
>>
>> Before using batched updates, the references would be applied
>> sequentially and hence no conflicts would arise. With batched updates,
>> while the first update applies, the second fails due to F/D conflict. A
>> similar issue was present in 'git-fetch(1)' and was fixed by using
>> separating out reference pruning into a separate transaction. Apply a
>> similar mechanism for 'git-receive-pack(1)' and separate out reference
>> deletions into its own batch.
>>
>> This means 'git-receive-pack(1)' will now use exactly two transactions,
>> whereas before using batched updates it would use _at least_ two
>> transactions. So using batched updates is the still the better option.
>
> s/the still the/still the/
>

Will fix.

>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 9e3cfb85cf..34db4377ca 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1867,47 +1867,66 @@ static void execute_commands_non_atomic(struct command *commands,
>>  	const char *reported_error = NULL;
>>  	struct strmap failed_refs = STRMAP_INIT;
>>
>> -	transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
>> -						  REF_TRANSACTION_ALLOW_FAILURE, &err);
>> -	if (!transaction) {
>> -		rp_error("%s", err.buf);
>> -		strbuf_reset(&err);
>> -		reported_error = "transaction failed to start";
>> -		goto failure;
>> -	}
>> +	/*
>> +	 * Reference updates, where F/D conflicts shouldn't arise due to
>> +	 * one reference being deleted, while the other being created
>> +	 * are treated as conflicts in batched updates. This is because
>> +	 * we don't do conflict resolution inside a transaction. To
>> +	 * mitigate this, delete references in a separate batch.
>> +	 */
>> +	enum processing_phase {
>> +		PHASE_DELETIONS,
>> +		PHASE_OTHERS
>> +	};
>>
>> -	for (cmd = commands; cmd; cmd = cmd->next) {
>> -		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
>> -			continue;
>> +	for (int phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {
>
> s/int/enum processing_phase/
>
> Doesn't make any difference, but it feels a bit cleaner.
>

Yeah, agreed.

>> +		transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
>> +							  REF_TRANSACTION_ALLOW_FAILURE, &err);
>> +		if (!transaction) {
>> +			rp_error("%s", err.buf);
>> +			strbuf_reset(&err);
>> +			reported_error = "transaction failed to s1tart";
>> +			goto failure;
>> +		}
>
> So if the transaction doesn't contain any deletions we'd now commit an
> empty transaction. The same is true the other way round, in case there
> are only deletions. Do we maybe want to skip phases when there is no
> match? Ideally, we wouldn't even be starting a transaction.
>
> We could for example skip forward to the first command that we would
> have to queue. If there is no such command we continue the loop, if
> there is we can remember that command, begin the transaction and start
> queueing from there.
>

I think that's a fair point, and very simple to implement. I'll add that
in. Thanks.

>> @@ -2024,6 +2043,9 @@ static void execute_commands(struct command *commands,
>>  	/*
>>  	 * If there is no command ready to run, should return directly to destroy
>>  	 * temporary data in the quarantine area.
>> +	 *
>> +	 * Check if any reference deletions exist, these are batched together in
>> +	 * a separate transaction to avoid F/D conflicts with other updates.
>>  	 */
>
> Is this comment still accurate?
>

Nope, will remove this.

>>  	for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)
>>  		; /* nothing */
>> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
>> index d91dd3a3b5..b2aaa1908f 100755
>> --- a/t/t1416-ref-transaction-hooks.sh
>> +++ b/t/t1416-ref-transaction-hooks.sh
>> @@ -119,6 +119,8 @@ test_expect_success 'interleaving hook calls succeed' '
>>  	EOF
>>
>>  	cat >expect <<-EOF &&
>> +		hooks/reference-transaction prepared
>> +		hooks/reference-transaction committed
>
> Yeah, this shows the empty commits indeed.
>

Yup, thanks for the quick review.

> Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v3 0/2] refs: fix some bugs with batched-updates

[Karthik Nayak]

In 23fc8e4f61 (refs: implement batch reference update support,
2025-04-08) we introduced a mechanism to batch reference updates. The
idea being that we wanted to batch updates similar to a transaction, but
allow certain updates to fail. This would help reference backends
optimize such operations and also remove the overhead of processing
updates individually. Especially for the reftable backend, where
batching updates would ensure that the autocompaction would only occur
at the end of the batch instead of after every reference update.

As of 9d2962a7c4 (receive-pack: use batched reference updates,
2025-05-19) we also updated the 'git-fetch(1)' and 'git-receive-pack(1)'
command to use batched reference updates. This series fixes some bugs
that we found at GitLab by running our Gitaly service with the `next`
build of Git.

The first being in the files backend, which missed skipping over failed
updates in certain flows. When certain individual updates fail, we mark
them as such. However, we missed skipping over such failed updates,
which can cause a SEGFAULT.

The other is in the git-receive-pack(1) implementation when a user
pushes multiple branches such as:

  delete refs/heads/branch/conflict
  create refs/heads/branch

Before using batched updates, the references would be applied
sequentially and hence no conflicts would arise. With batched updates,
while the first update applies, the second fails due to F/D conflict. A
similar issue was present in 'git-fetch(1)' and was fixed by using
separating out reference pruning into a separate transaction. Apply a
similar mechanism for 'git-receive-pack(1)' and separate out reference
deletions into its own batch.

This is based off master 7014b55638 (A bit more topics for -rc1,
2025-05-30), with the changes from kn/fetch-push-bulk-ref-update merged
in.

---
Changes in v3:
- Cleanup the second commit message and remove stale comments.
- In the second commit, only create the transaction if needed.
- 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

Changes in v2:
- Modify the test in the first commit to no longer do a quiet grep,
  and some more tests.
- Remove the second commit as it was unnecessary.
- Modify the commit message in the last commit, to also talk about how
  we now use 2 transactions at minimum but this is still better than
  before.
- Modify the logic in the last commit to no longer use an XOR and
  instead loop over the two different scenarios (deletion updates, other
  updates).
- Link to v1: https://lore.kernel.org/r/20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-0-903d1db3f10e@gmail.com

---
 builtin/receive-pack.c | 100 ++++++++++++++++++++++++++++++++-----------------
 refs/files-backend.c   |   7 ++++
 t/t1400-update-ref.sh  |  45 ++++++++++++++++++++++
 t/t5516-fetch-push.sh  |  17 +++++++--
 4 files changed, 131 insertions(+), 38 deletions(-)

Karthik Nayak (2):
      refs/files: skip updates with errors in batched updates
      receive-pack: handle reference deletions separately

Range-diff versus v2:

1:  50f318714b = 1:  695cc3b9d4 refs/files: skip updates with errors in batched updates
2:  d7293b64b0 ! 2:  a20bb05dd6 receive-pack: handle reference deletions separately
    @@ Commit message
         similar mechanism for 'git-receive-pack(1)' and separate out reference
         deletions into its own batch.
     
    -    This means 'git-receive-pack(1)' will now use exactly two transactions,
    +    This means 'git-receive-pack(1)' will now use upto two transactions,
         whereas before using batched updates it would use _at least_ two
    -    transactions. So using batched updates is the still the better option.
    +    transactions. So using batched updates is still the better option.
     
         Add a test to validate this behavior.
     
    @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *
     -	for (cmd = commands; cmd; cmd = cmd->next) {
     -		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
     -			continue;
    -+	for (int phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {
    -+		transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
    -+							  REF_TRANSACTION_ALLOW_FAILURE, &err);
    -+		if (!transaction) {
    -+			rp_error("%s", err.buf);
    -+			strbuf_reset(&err);
    -+			reported_error = "transaction failed to s1tart";
    -+			goto failure;
    -+		}
    ++	for (enum processing_phase phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {
    ++		for (cmd = commands; cmd; cmd = cmd->next) {
    ++			if (!should_process_cmd(cmd) || cmd->run_proc_receive)
    ++				continue;
      
     -		cmd->error_string = update(cmd, si);
     -	}
    -+		for (cmd = commands; cmd; cmd = cmd->next) {
    -+			if (!should_process_cmd(cmd) || cmd->run_proc_receive)
    ++			if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))
    ++				continue;
    ++			else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))
     +				continue;
      
     -	if (ref_transaction_commit(transaction, &err)) {
    @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *
     -		reported_error = "failed to update refs";
     -		goto failure;
     -	}
    -+			if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))
    -+				continue;
    -+			else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))
    -+				continue;
    ++			/*
    ++			 * Lazily create a transaction only when we know there are
    ++			 * updates to be added.
    ++			 */
    ++			if (!transaction) {
    ++				transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
    ++									  REF_TRANSACTION_ALLOW_FAILURE, &err);
    ++				if (!transaction) {
    ++					rp_error("%s", err.buf);
    ++					strbuf_reset(&err);
    ++					reported_error = "transaction failed to s1tart";
    ++					goto failure;
    ++				}
    ++			}
      
     -	ref_transaction_for_each_rejected_update(transaction,
     -						 ref_transaction_rejection_handler,
    @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *
      
     -	if (strmap_empty(&failed_refs))
     -		goto cleanup;
    -+		if (ref_transaction_commit(transaction, &err)) {
    -+			rp_error("%s", err.buf);
    -+			reported_error = "failed to update refs";
    -+			goto failure;
    -+		}
    ++		/*
    ++		 * If no transaction was created, there is nothing to commit.
    ++		 */
    ++		if (!transaction)
    ++			goto cleanup;
      
     -failure:
     -	for (cmd = commands; cmd; cmd = cmd->next) {
    @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *
     -		else if (strmap_contains(&failed_refs, cmd->ref_name))
     -			cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
     -	}
    -+		ref_transaction_for_each_rejected_update(transaction,
    -+							 ref_transaction_rejection_handler,
    -+							 &failed_refs);
    ++		if (ref_transaction_commit(transaction, &err)) {
    ++			rp_error("%s", err.buf);
    ++			reported_error = "failed to update refs";
    ++			goto failure;
    ++		}
      
     -cleanup:
     -	ref_transaction_free(transaction);
     -	strmap_clear(&failed_refs, 0);
     -	strbuf_release(&err);
    ++		ref_transaction_for_each_rejected_update(transaction,
    ++							 ref_transaction_rejection_handler,
    ++							 &failed_refs);
    ++
     +		if (strmap_empty(&failed_refs))
     +			goto cleanup;
     +
    @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *
     +
     +	cleanup:
     +		ref_transaction_free(transaction);
    ++		transaction = NULL;
     +		strmap_clear(&failed_refs, 0);
     +		strbuf_release(&err);
     +	}
      }
      
      static void execute_commands_atomic(struct command *commands,
    -@@ builtin/receive-pack.c: static void execute_commands(struct command *commands,
    - 	/*
    - 	 * If there is no command ready to run, should return directly to destroy
    - 	 * temporary data in the quarantine area.
    -+	 *
    -+	 * Check if any reference deletions exist, these are batched together in
    -+	 * a separate transaction to avoid F/D conflicts with other updates.
    - 	 */
    - 	for (cmd = commands; cmd && cmd->error_string; cmd = cmd->next)
    - 		; /* nothing */
    -
    - ## t/t1416-ref-transaction-hooks.sh ##
    -@@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'interleaving hook calls succeed' '
    - 	EOF
    - 
    - 	cat >expect <<-EOF &&
    -+		hooks/reference-transaction prepared
    -+		hooks/reference-transaction committed
    - 		hooks/update refs/tags/PRE $ZERO_OID $PRE_OID
    - 		hooks/update refs/tags/POST $ZERO_OID $POST_OID
    - 		hooks/reference-transaction prepared
     
      ## t/t5516-fetch-push.sh ##
     @@ t/t5516-fetch-push.sh: test_expect_success 'pushing valid refs triggers post-receive and post-update ho


base-commit: 931c39f05e078e0df968a439379cb04b5c4666ef
change-id: 20250528-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-24ff53680144

Thanks
- Karthik

[PATCH v3 1/2] refs/files: skip updates with errors in batched updates

[Karthik Nayak]

The commit 23fc8e4f61 (refs: implement batch reference update support,
2025-04-08) introduced support for batched reference updates. This
allows users to batch updates together, while allowing some of the
updates to fail.

Under the hood, batched updates use the reference transaction mechanism.
Each update which fails is marked as such. Any failed updates must be
skipped over in the rest of the code, as they wouldn't apply any more.
In two of the loops within 'files_transaction_finish()' of the files
backend, the failed updates aren't skipped over. This can cause a
SEGFAULT otherwise. Add the missing skips and a test to validate the
same.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c  |  7 +++++++
 t/t1400-update-ref.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4d1f65a57a..c4a0f29072 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,
 	 */
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
+
+		if (update->rejection_err)
+			continue;
+
 		if (update->flags & REF_DELETING &&
 		    !(update->flags & REF_LOG_ONLY) &&
 		    !(update->flags & REF_IS_PRUNING)) {
@@ -3239,6 +3243,9 @@ static int files_transaction_finish(struct ref_store *ref_store,
 		struct ref_update *update = transaction->updates[i];
 		struct ref_lock *lock = update->backend_data;
 
+		if (update->rejection_err)
+			continue;
+
 		if (update->flags & REF_DELETING &&
 		    !(update->flags & REF_LOG_ONLY)) {
 			update->flags |= REF_DELETED_RMDIR;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index d29d23cb89..ca7eee7de2 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -2299,6 +2299,51 @@ do
 			test_grep -q "refname conflict" stdout
 		)
 	'
+
+	test_expect_success "stdin $type batch-updates delete incorrect symbolic ref" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit c1 &&
+			head=$(git rev-parse HEAD) &&
+			git symbolic-ref refs/heads/symbolic refs/heads/non-existent &&
+
+			format_command $type "delete refs/heads/symbolic" "$head" >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+			test_grep "reference does not exist" stdout
+		)
+	'
+
+	test_expect_success "stdin $type batch-updates delete with incorrect old_oid" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit c1 &&
+			git branch new-branch &&
+			test_commit c2 &&
+			head=$(git rev-parse HEAD) &&
+
+			format_command $type "delete refs/heads/new-branch" "$head" >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+			test_grep "incorrect old value provided" stdout
+		)
+	'
+
+	test_expect_success "stdin $type batch-updates delete non-existent ref" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit commit &&
+			head=$(git rev-parse HEAD) &&
+
+			format_command $type "delete refs/heads/non-existent" "$head" >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+			test_grep "reference does not exist" stdout
+		)
+	'
 done
 
 test_expect_success 'update-ref should also create reflog for HEAD' '

-- 
2.49.0

[PATCH v3 2/2] receive-pack: handle reference deletions separately

[Karthik Nayak]

In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
we updated the 'git-receive-pack(1)' command to use batched reference
updates. One edge case which was missed during this implementation was
when a user pushes multiple branches such as:

  delete refs/heads/branch/conflict
  create refs/heads/branch

Before using batched updates, the references would be applied
sequentially and hence no conflicts would arise. With batched updates,
while the first update applies, the second fails due to F/D conflict. A
similar issue was present in 'git-fetch(1)' and was fixed by using
separating out reference pruning into a separate transaction. Apply a
similar mechanism for 'git-receive-pack(1)' and separate out reference
deletions into its own batch.

This means 'git-receive-pack(1)' will now use upto two transactions,
whereas before using batched updates it would use _at least_ two
transactions. So using batched updates is still the better option.

Add a test to validate this behavior.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/receive-pack.c | 100 ++++++++++++++++++++++++++++++++-----------------
 t/t5516-fetch-push.sh  |  17 +++++++--
 2 files changed, 79 insertions(+), 38 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9e3cfb85cf..8ee792d2f8 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1867,47 +1867,79 @@ static void execute_commands_non_atomic(struct command *commands,
 	const char *reported_error = NULL;
 	struct strmap failed_refs = STRMAP_INIT;
 
-	transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
-						  REF_TRANSACTION_ALLOW_FAILURE, &err);
-	if (!transaction) {
-		rp_error("%s", err.buf);
-		strbuf_reset(&err);
-		reported_error = "transaction failed to start";
-		goto failure;
-	}
+	/*
+	 * Reference updates, where F/D conflicts shouldn't arise due to
+	 * one reference being deleted, while the other being created
+	 * are treated as conflicts in batched updates. This is because
+	 * we don't do conflict resolution inside a transaction. To
+	 * mitigate this, delete references in a separate batch.
+	 */
+	enum processing_phase {
+		PHASE_DELETIONS,
+		PHASE_OTHERS
+	};
 
-	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
-			continue;
+	for (enum processing_phase phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {
+		for (cmd = commands; cmd; cmd = cmd->next) {
+			if (!should_process_cmd(cmd) || cmd->run_proc_receive)
+				continue;
 
-		cmd->error_string = update(cmd, si);
-	}
+			if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))
+				continue;
+			else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))
+				continue;
 
-	if (ref_transaction_commit(transaction, &err)) {
-		rp_error("%s", err.buf);
-		reported_error = "failed to update refs";
-		goto failure;
-	}
+			/*
+			 * Lazily create a transaction only when we know there are
+			 * updates to be added.
+			 */
+			if (!transaction) {
+				transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
+									  REF_TRANSACTION_ALLOW_FAILURE, &err);
+				if (!transaction) {
+					rp_error("%s", err.buf);
+					strbuf_reset(&err);
+					reported_error = "transaction failed to s1tart";
+					goto failure;
+				}
+			}
 
-	ref_transaction_for_each_rejected_update(transaction,
-						 ref_transaction_rejection_handler,
-						 &failed_refs);
+			cmd->error_string = update(cmd, si);
+		}
 
-	if (strmap_empty(&failed_refs))
-		goto cleanup;
+		/*
+		 * If no transaction was created, there is nothing to commit.
+		 */
+		if (!transaction)
+			goto cleanup;
 
-failure:
-	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (reported_error)
-			cmd->error_string = reported_error;
-		else if (strmap_contains(&failed_refs, cmd->ref_name))
-			cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
-	}
+		if (ref_transaction_commit(transaction, &err)) {
+			rp_error("%s", err.buf);
+			reported_error = "failed to update refs";
+			goto failure;
+		}
 
-cleanup:
-	ref_transaction_free(transaction);
-	strmap_clear(&failed_refs, 0);
-	strbuf_release(&err);
+		ref_transaction_for_each_rejected_update(transaction,
+							 ref_transaction_rejection_handler,
+							 &failed_refs);
+
+		if (strmap_empty(&failed_refs))
+			goto cleanup;
+
+	failure:
+		for (cmd = commands; cmd; cmd = cmd->next) {
+			if (reported_error)
+				cmd->error_string = reported_error;
+			else if (strmap_contains(&failed_refs, cmd->ref_name))
+				cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
+		}
+
+	cleanup:
+		ref_transaction_free(transaction);
+		transaction = NULL;
+		strmap_clear(&failed_refs, 0);
+		strbuf_release(&err);
+	}
 }
 
 static void execute_commands_atomic(struct command *commands,
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index dabcc5f811..1649667441 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/next $orgnext $newnext
+		refs/heads/main $orgmain $newmain
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -808,8 +808,8 @@ test_expect_success 'deletion of a non-existent ref is not fed to post-receive a
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/nonexistent $ZERO_OID $ZERO_OID
+		refs/heads/main $orgmain $newmain
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -868,10 +868,10 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/next $orgnext $newnext
-		refs/heads/seen $orgseen $newseen
 		refs/heads/nonexistent $ZERO_OID $ZERO_OID
+		refs/heads/main $orgmain $newmain
+		refs/heads/seen $orgseen $newseen
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -1909,4 +1909,13 @@ test_expect_success 'push with config push.useBitmaps' '
 		--thin --delta-base-offset -q --no-use-bitmap-index <false
 '
 
+test_expect_success 'push with F/D conflict with deletion and creation' '
+	test_when_finished "git branch -D branch" &&
+	git branch branch/conflict &&
+	mk_test testrepo heads/branch/conflict &&
+	git branch -D branch/conflict &&
+	git branch branch &&
+	git push testrepo :refs/heads/branch/conflict refs/heads/branch
+'
+
 test_done

-- 
2.49.0

Re: [PATCH v3 2/2] receive-pack: handle reference deletions separately

[Christian Couder]

On Fri, Jun 6, 2025 at 10:41 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>
> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
> we updated the 'git-receive-pack(1)' command to use batched reference
> updates. One edge case which was missed during this implementation was
> when a user pushes multiple branches such as:
>
>   delete refs/heads/branch/conflict
>   create refs/heads/branch
>
> Before using batched updates, the references would be applied
> sequentially and hence no conflicts would arise. With batched updates,
> while the first update applies, the second fails due to F/D conflict.

Nit: it looks like "D/F conflict" is more often used than "F/D
conflict" in the Git code base:

$ git grep -i 'd/f conflict' | wc -l
119
$ git grep -i 'f/d conflict' | wc -l
7

> A
> similar issue was present in 'git-fetch(1)' and was fixed by using
> separating out reference pruning into a separate transaction. Apply a

Maybe: s/using separating out/separating out/

> similar mechanism for 'git-receive-pack(1)' and separate out reference
> deletions into its own batch.
>
> This means 'git-receive-pack(1)' will now use upto two transactions,
> whereas before using batched updates it would use _at least_ two
> transactions. So using batched updates is still the better option.
>
> Add a test to validate this behavior.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  builtin/receive-pack.c | 100 ++++++++++++++++++++++++++++++++-----------------
>  t/t5516-fetch-push.sh  |  17 +++++++--
>  2 files changed, 79 insertions(+), 38 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 9e3cfb85cf..8ee792d2f8 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1867,47 +1867,79 @@ static void execute_commands_non_atomic(struct command *commands,
>         const char *reported_error = NULL;
>         struct strmap failed_refs = STRMAP_INIT;
>
> -       transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> -                                                 REF_TRANSACTION_ALLOW_FAILURE, &err);
> -       if (!transaction) {
> -               rp_error("%s", err.buf);
> -               strbuf_reset(&err);
> -               reported_error = "transaction failed to start";
> -               goto failure;
> -       }
> +       /*
> +        * Reference updates, where F/D conflicts shouldn't arise due to

Nit: here also maybe "D/F conflicts" is more standard.

> +        * one reference being deleted, while the other being created
> +        * are treated as conflicts in batched updates. This is because
> +        * we don't do conflict resolution inside a transaction. To
> +        * mitigate this, delete references in a separate batch.
> +        */
> +       enum processing_phase {
> +               PHASE_DELETIONS,
> +               PHASE_OTHERS
> +       };
>
> -       for (cmd = commands; cmd; cmd = cmd->next) {
> -               if (!should_process_cmd(cmd) || cmd->run_proc_receive)
> -                       continue;
> +       for (enum processing_phase phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {
> +               for (cmd = commands; cmd; cmd = cmd->next) {
> +                       if (!should_process_cmd(cmd) || cmd->run_proc_receive)
> +                               continue;
>
> -               cmd->error_string = update(cmd, si);
> -       }
> +                       if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))
> +                               continue;
> +                       else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))
> +                               continue;
>
> -       if (ref_transaction_commit(transaction, &err)) {
> -               rp_error("%s", err.buf);
> -               reported_error = "failed to update refs";
> -               goto failure;
> -       }
> +                       /*
> +                        * Lazily create a transaction only when we know there are
> +                        * updates to be added.
> +                        */
> +                       if (!transaction) {
> +                               transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> +                                                                         REF_TRANSACTION_ALLOW_FAILURE, &err);
> +                               if (!transaction) {
> +                                       rp_error("%s", err.buf);
> +                                       strbuf_reset(&err);
> +                                       reported_error = "transaction failed to s1tart";

s/s1tart/start/

> +                                       goto failure;
> +                               }
> +                       }
>
> -       ref_transaction_for_each_rejected_update(transaction,
> -                                                ref_transaction_rejection_handler,
> -                                                &failed_refs);
> +                       cmd->error_string = update(cmd, si);
> +               }
>
> -       if (strmap_empty(&failed_refs))
> -               goto cleanup;
> +               /*
> +                * If no transaction was created, there is nothing to commit.
> +                */

Nit: the comment needs a single line, so maybe:

              /* No transaction, so nothing to commit */

> +               if (!transaction)
> +                       goto cleanup;
>
> -failure:
> -       for (cmd = commands; cmd; cmd = cmd->next) {
> -               if (reported_error)
> -                       cmd->error_string = reported_error;
> -               else if (strmap_contains(&failed_refs, cmd->ref_name))
> -                       cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
> -       }
> +               if (ref_transaction_commit(transaction, &err)) {
> +                       rp_error("%s", err.buf);
> +                       reported_error = "failed to update refs";
> +                       goto failure;
> +               }
>
> -cleanup:
> -       ref_transaction_free(transaction);
> -       strmap_clear(&failed_refs, 0);
> -       strbuf_release(&err);
> +               ref_transaction_for_each_rejected_update(transaction,
> +                                                        ref_transaction_rejection_handler,
> +                                                        &failed_refs);
> +
> +               if (strmap_empty(&failed_refs))
> +                       goto cleanup;
> +
> +       failure:

This label looks indented while previously it was right at the start
of the line. Not sure if we have a standard for that, but a few quick
greps seems to show that goto labels are most often at the start of
the line.

> +               for (cmd = commands; cmd; cmd = cmd->next) {
> +                       if (reported_error)
> +                               cmd->error_string = reported_error;
> +                       else if (strmap_contains(&failed_refs, cmd->ref_name))
> +                               cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
> +               }
> +
> +       cleanup:

Idem for how this label is indented.

> +               ref_transaction_free(transaction);
> +               transaction = NULL;
> +               strmap_clear(&failed_refs, 0);
> +               strbuf_release(&err);
> +       }
>  }

Re: [PATCH v3 2/2] receive-pack: handle reference deletions separately

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

>> when a user pushes multiple branches such as:
>>
>>   delete refs/heads/branch/conflict
>>   create refs/heads/branch
>>
>> Before using batched updates, the references would be applied
>> sequentially and hence no conflicts would arise. With batched updates,
>> while the first update applies, the second fails due to F/D conflict.
>
> Nit: it looks like "D/F conflict" is more often used than "F/D
> conflict" in the Git code base:
>
> $ git grep -i 'd/f conflict' | wc -l
> 119
> $ git grep -i 'f/d conflict' | wc -l
> 7

I do not mind calling a situation F/D conflict if you have a file
and your attempt to create a directory at the same path fails (as
opposed to D/F where directory exists and you cannot overwrite it
with a file), but the above case does sound like a D/F conflict that
deletes directory r/h/b (by removing the last subpath in it), which
is OK, and creates file r/h/b, which the all-or-nothing machinery
does not allow well, so calling D/F may probably be more in line
with the existing practice, regardless of which situation we more
commonly talk about in the code base.

Re: [PATCH v3 2/2] receive-pack: handle reference deletions separately

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 8122 bytes --]

Christian Couder <christian.couder@gmail.com> writes:

> On Fri, Jun 6, 2025 at 10:41 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
>> we updated the 'git-receive-pack(1)' command to use batched reference
>> updates. One edge case which was missed during this implementation was
>> when a user pushes multiple branches such as:
>>
>>   delete refs/heads/branch/conflict
>>   create refs/heads/branch
>>
>> Before using batched updates, the references would be applied
>> sequentially and hence no conflicts would arise. With batched updates,
>> while the first update applies, the second fails due to F/D conflict.
>
> Nit: it looks like "D/F conflict" is more often used than "F/D
> conflict" in the Git code base:
>
> $ git grep -i 'd/f conflict' | wc -l
> 119
> $ git grep -i 'f/d conflict' | wc -l
> 7
>
>> A
>> similar issue was present in 'git-fetch(1)' and was fixed by using
>> separating out reference pruning into a separate transaction. Apply a
>
> Maybe: s/using separating out/separating out/
>

That's better, will change.

>> similar mechanism for 'git-receive-pack(1)' and separate out reference
>> deletions into its own batch.
>>
>> This means 'git-receive-pack(1)' will now use upto two transactions,
>> whereas before using batched updates it would use _at least_ two
>> transactions. So using batched updates is still the better option.
>>
>> Add a test to validate this behavior.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  builtin/receive-pack.c | 100 ++++++++++++++++++++++++++++++++-----------------
>>  t/t5516-fetch-push.sh  |  17 +++++++--
>>  2 files changed, 79 insertions(+), 38 deletions(-)
>>
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 9e3cfb85cf..8ee792d2f8 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1867,47 +1867,79 @@ static void execute_commands_non_atomic(struct command *commands,
>>         const char *reported_error = NULL;
>>         struct strmap failed_refs = STRMAP_INIT;
>>
>> -       transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
>> -                                                 REF_TRANSACTION_ALLOW_FAILURE, &err);
>> -       if (!transaction) {
>> -               rp_error("%s", err.buf);
>> -               strbuf_reset(&err);
>> -               reported_error = "transaction failed to start";
>> -               goto failure;
>> -       }
>> +       /*
>> +        * Reference updates, where F/D conflicts shouldn't arise due to
>
> Nit: here also maybe "D/F conflicts" is more standard.
>

Will change.

>> +        * one reference being deleted, while the other being created
>> +        * are treated as conflicts in batched updates. This is because
>> +        * we don't do conflict resolution inside a transaction. To
>> +        * mitigate this, delete references in a separate batch.
>> +        */
>> +       enum processing_phase {
>> +               PHASE_DELETIONS,
>> +               PHASE_OTHERS
>> +       };
>>
>> -       for (cmd = commands; cmd; cmd = cmd->next) {
>> -               if (!should_process_cmd(cmd) || cmd->run_proc_receive)
>> -                       continue;
>> +       for (enum processing_phase phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {
>> +               for (cmd = commands; cmd; cmd = cmd->next) {
>> +                       if (!should_process_cmd(cmd) || cmd->run_proc_receive)
>> +                               continue;
>>
>> -               cmd->error_string = update(cmd, si);
>> -       }
>> +                       if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))
>> +                               continue;
>> +                       else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))
>> +                               continue;
>>
>> -       if (ref_transaction_commit(transaction, &err)) {
>> -               rp_error("%s", err.buf);
>> -               reported_error = "failed to update refs";
>> -               goto failure;
>> -       }
>> +                       /*
>> +                        * Lazily create a transaction only when we know there are
>> +                        * updates to be added.
>> +                        */
>> +                       if (!transaction) {
>> +                               transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
>> +                                                                         REF_TRANSACTION_ALLOW_FAILURE, &err);
>> +                               if (!transaction) {
>> +                                       rp_error("%s", err.buf);
>> +                                       strbuf_reset(&err);
>> +                                       reported_error = "transaction failed to s1tart";
>
> s/s1tart/start/
>

Oops! Good catch.

>> +                                       goto failure;
>> +                               }
>> +                       }
>>
>> -       ref_transaction_for_each_rejected_update(transaction,
>> -                                                ref_transaction_rejection_handler,
>> -                                                &failed_refs);
>> +                       cmd->error_string = update(cmd, si);
>> +               }
>>
>> -       if (strmap_empty(&failed_refs))
>> -               goto cleanup;
>> +               /*
>> +                * If no transaction was created, there is nothing to commit.
>> +                */
>
> Nit: the comment needs a single line, so maybe:
>
>               /* No transaction, so nothing to commit */
>

Looks better.

>> +               if (!transaction)
>> +                       goto cleanup;
>>
>> -failure:
>> -       for (cmd = commands; cmd; cmd = cmd->next) {
>> -               if (reported_error)
>> -                       cmd->error_string = reported_error;
>> -               else if (strmap_contains(&failed_refs, cmd->ref_name))
>> -                       cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
>> -       }
>> +               if (ref_transaction_commit(transaction, &err)) {
>> +                       rp_error("%s", err.buf);
>> +                       reported_error = "failed to update refs";
>> +                       goto failure;
>> +               }
>>
>> -cleanup:
>> -       ref_transaction_free(transaction);
>> -       strmap_clear(&failed_refs, 0);
>> -       strbuf_release(&err);
>> +               ref_transaction_for_each_rejected_update(transaction,
>> +                                                        ref_transaction_rejection_handler,
>> +                                                        &failed_refs);
>> +
>> +               if (strmap_empty(&failed_refs))
>> +                       goto cleanup;
>> +
>> +       failure:
>
> This label looks indented while previously it was right at the start
> of the line. Not sure if we have a standard for that, but a few quick
> greps seems to show that goto labels are most often at the start of
> the line.
>

Yup, but now this label lies within a for loop, so the indentation is
aligned to the loop. So I think it is correct as is.

>> +               for (cmd = commands; cmd; cmd = cmd->next) {
>> +                       if (reported_error)
>> +                               cmd->error_string = reported_error;
>> +                       else if (strmap_contains(&failed_refs, cmd->ref_name))
>> +                               cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
>> +               }
>> +
>> +       cleanup:
>
> Idem for how this label is indented.
>

Same here, this too is within a loop.

>> +               ref_transaction_free(transaction);
>> +               transaction = NULL;
>> +               strmap_clear(&failed_refs, 0);
>> +               strbuf_release(&err);
>> +       }
>>  }

Thanks for the review!

- Karthik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v4 0/2] refs: fix some bugs with batched-updates

[Karthik Nayak]

In 23fc8e4f61 (refs: implement batch reference update support,
2025-04-08) we introduced a mechanism to batch reference updates. The
idea being that we wanted to batch updates similar to a transaction, but
allow certain updates to fail. This would help reference backends
optimize such operations and also remove the overhead of processing
updates individually. Especially for the reftable backend, where
batching updates would ensure that the autocompaction would only occur
at the end of the batch instead of after every reference update.

As of 9d2962a7c4 (receive-pack: use batched reference updates,
2025-05-19) we also updated the 'git-fetch(1)' and 'git-receive-pack(1)'
command to use batched reference updates. This series fixes some bugs
that we found at GitLab by running our Gitaly service with the `next`
build of Git.

The first being in the files backend, which missed skipping over failed
updates in certain flows. When certain individual updates fail, we mark
them as such. However, we missed skipping over such failed updates,
which can cause a SEGFAULT.

The other is in the git-receive-pack(1) implementation when a user
pushes multiple branches such as:

  delete refs/heads/branch/conflict
  create refs/heads/branch

Before using batched updates, the references would be applied
sequentially and hence no conflicts would arise. With batched updates,
while the first update applies, the second fails due to F/D conflict. A
similar issue was present in 'git-fetch(1)' and was fixed by using
separating out reference pruning into a separate transaction. Apply a
similar mechanism for 'git-receive-pack(1)' and separate out reference
deletions into its own batch.

This is based off master 7014b55638 (A bit more topics for -rc1,
2025-05-30), with the changes from kn/fetch-push-bulk-ref-update merged
in.

---
Changes in v4:
- Swapped out F/D for D/F in the second commit, since we are talking
  about conflicts between a directory and a file, also D/F is more
  consistent.
- Fixed some typos in the second commit.
- Changed comment to single line.
- 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

Changes in v3:
- Cleanup the second commit message and remove stale comments.
- In the second commit, only create the transaction if needed.
- 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

Changes in v2:
- Modify the test in the first commit to no longer do a quiet grep,
  and some more tests.
- Remove the second commit as it was unnecessary.
- Modify the commit message in the last commit, to also talk about how
  we now use 2 transactions at minimum but this is still better than
  before.
- Modify the logic in the last commit to no longer use an XOR and
  instead loop over the two different scenarios (deletion updates, other
  updates).
- Link to v1: https://lore.kernel.org/r/20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-0-903d1db3f10e@gmail.com

---
 builtin/receive-pack.c | 98 ++++++++++++++++++++++++++++++++------------------
 refs/files-backend.c   |  7 ++++
 t/t1400-update-ref.sh  | 45 +++++++++++++++++++++++
 t/t5516-fetch-push.sh  | 17 ++++++---
 4 files changed, 129 insertions(+), 38 deletions(-)

Karthik Nayak (2):
      refs/files: skip updates with errors in batched updates
      receive-pack: handle reference deletions separately

Range-diff versus v3:

1:  6a9a7fdcbf = 1:  44291af073 refs/files: skip updates with errors in batched updates
2:  3175c3430c ! 2:  c8026de30e receive-pack: handle reference deletions separately
    @@ Commit message
     
         Before using batched updates, the references would be applied
         sequentially and hence no conflicts would arise. With batched updates,
    -    while the first update applies, the second fails due to F/D conflict. A
    -    similar issue was present in 'git-fetch(1)' and was fixed by using
    -    separating out reference pruning into a separate transaction. Apply a
    -    similar mechanism for 'git-receive-pack(1)' and separate out reference
    -    deletions into its own batch.
    +    while the first update applies, the second fails due to D/F conflict. A
    +    similar issue was present in 'git-fetch(1)' and was fixed by separating
    +    out reference pruning into a separate transaction. Apply a similar
    +    mechanism for 'git-receive-pack(1)' and separate out reference deletions
    +    into its own batch.
     
    -    This means 'git-receive-pack(1)' will now use upto two transactions,
    +    This means 'git-receive-pack(1)' will now use up to two transactions,
         whereas before using batched updates it would use _at least_ two
         transactions. So using batched updates is still the better option.
     
    @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *
     -		goto failure;
     -	}
     +	/*
    -+	 * Reference updates, where F/D conflicts shouldn't arise due to
    ++	 * Reference updates, where D/F conflicts shouldn't arise due to
     +	 * one reference being deleted, while the other being created
     +	 * are treated as conflicts in batched updates. This is because
     +	 * we don't do conflict resolution inside a transaction. To
    @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *
     +				if (!transaction) {
     +					rp_error("%s", err.buf);
     +					strbuf_reset(&err);
    -+					reported_error = "transaction failed to s1tart";
    ++					reported_error = "transaction failed to start";
     +					goto failure;
     +				}
     +			}
    @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *
      
     -	if (strmap_empty(&failed_refs))
     -		goto cleanup;
    -+		/*
    -+		 * If no transaction was created, there is nothing to commit.
    -+		 */
    ++		/* No transaction, so nothing to commit */
     +		if (!transaction)
     +			goto cleanup;
      


base-commit: 931c39f05e078e0df968a439379cb04b5c4666ef
change-id: 20250528-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-24ff53680144

Thanks
- Karthik

[PATCH v4 1/2] refs/files: skip updates with errors in batched updates

[Karthik Nayak]

The commit 23fc8e4f61 (refs: implement batch reference update support,
2025-04-08) introduced support for batched reference updates. This
allows users to batch updates together, while allowing some of the
updates to fail.

Under the hood, batched updates use the reference transaction mechanism.
Each update which fails is marked as such. Any failed updates must be
skipped over in the rest of the code, as they wouldn't apply any more.
In two of the loops within 'files_transaction_finish()' of the files
backend, the failed updates aren't skipped over. This can cause a
SEGFAULT otherwise. Add the missing skips and a test to validate the
same.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c  |  7 +++++++
 t/t1400-update-ref.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4d1f65a57a..c4a0f29072 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,
 	 */
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
+
+		if (update->rejection_err)
+			continue;
+
 		if (update->flags & REF_DELETING &&
 		    !(update->flags & REF_LOG_ONLY) &&
 		    !(update->flags & REF_IS_PRUNING)) {
@@ -3239,6 +3243,9 @@ static int files_transaction_finish(struct ref_store *ref_store,
 		struct ref_update *update = transaction->updates[i];
 		struct ref_lock *lock = update->backend_data;
 
+		if (update->rejection_err)
+			continue;
+
 		if (update->flags & REF_DELETING &&
 		    !(update->flags & REF_LOG_ONLY)) {
 			update->flags |= REF_DELETED_RMDIR;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index d29d23cb89..ca7eee7de2 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -2299,6 +2299,51 @@ do
 			test_grep -q "refname conflict" stdout
 		)
 	'
+
+	test_expect_success "stdin $type batch-updates delete incorrect symbolic ref" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit c1 &&
+			head=$(git rev-parse HEAD) &&
+			git symbolic-ref refs/heads/symbolic refs/heads/non-existent &&
+
+			format_command $type "delete refs/heads/symbolic" "$head" >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+			test_grep "reference does not exist" stdout
+		)
+	'
+
+	test_expect_success "stdin $type batch-updates delete with incorrect old_oid" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit c1 &&
+			git branch new-branch &&
+			test_commit c2 &&
+			head=$(git rev-parse HEAD) &&
+
+			format_command $type "delete refs/heads/new-branch" "$head" >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+			test_grep "incorrect old value provided" stdout
+		)
+	'
+
+	test_expect_success "stdin $type batch-updates delete non-existent ref" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit commit &&
+			head=$(git rev-parse HEAD) &&
+
+			format_command $type "delete refs/heads/non-existent" "$head" >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+			test_grep "reference does not exist" stdout
+		)
+	'
 done
 
 test_expect_success 'update-ref should also create reflog for HEAD' '

-- 
2.49.0

[PATCH v4 2/2] receive-pack: handle reference deletions separately

[Karthik Nayak]

In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
we updated the 'git-receive-pack(1)' command to use batched reference
updates. One edge case which was missed during this implementation was
when a user pushes multiple branches such as:

  delete refs/heads/branch/conflict
  create refs/heads/branch

Before using batched updates, the references would be applied
sequentially and hence no conflicts would arise. With batched updates,
while the first update applies, the second fails due to D/F conflict. A
similar issue was present in 'git-fetch(1)' and was fixed by separating
out reference pruning into a separate transaction. Apply a similar
mechanism for 'git-receive-pack(1)' and separate out reference deletions
into its own batch.

This means 'git-receive-pack(1)' will now use up to two transactions,
whereas before using batched updates it would use _at least_ two
transactions. So using batched updates is still the better option.

Add a test to validate this behavior.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/receive-pack.c | 98 ++++++++++++++++++++++++++++++++------------------
 t/t5516-fetch-push.sh  | 17 ++++++---
 2 files changed, 77 insertions(+), 38 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9e3cfb85cf..3fb2776e9a 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1867,47 +1867,77 @@ static void execute_commands_non_atomic(struct command *commands,
 	const char *reported_error = NULL;
 	struct strmap failed_refs = STRMAP_INIT;
 
-	transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
-						  REF_TRANSACTION_ALLOW_FAILURE, &err);
-	if (!transaction) {
-		rp_error("%s", err.buf);
-		strbuf_reset(&err);
-		reported_error = "transaction failed to start";
-		goto failure;
-	}
+	/*
+	 * Reference updates, where D/F conflicts shouldn't arise due to
+	 * one reference being deleted, while the other being created
+	 * are treated as conflicts in batched updates. This is because
+	 * we don't do conflict resolution inside a transaction. To
+	 * mitigate this, delete references in a separate batch.
+	 */
+	enum processing_phase {
+		PHASE_DELETIONS,
+		PHASE_OTHERS
+	};
 
-	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
-			continue;
+	for (enum processing_phase phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {
+		for (cmd = commands; cmd; cmd = cmd->next) {
+			if (!should_process_cmd(cmd) || cmd->run_proc_receive)
+				continue;
 
-		cmd->error_string = update(cmd, si);
-	}
+			if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))
+				continue;
+			else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))
+				continue;
 
-	if (ref_transaction_commit(transaction, &err)) {
-		rp_error("%s", err.buf);
-		reported_error = "failed to update refs";
-		goto failure;
-	}
+			/*
+			 * Lazily create a transaction only when we know there are
+			 * updates to be added.
+			 */
+			if (!transaction) {
+				transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
+									  REF_TRANSACTION_ALLOW_FAILURE, &err);
+				if (!transaction) {
+					rp_error("%s", err.buf);
+					strbuf_reset(&err);
+					reported_error = "transaction failed to start";
+					goto failure;
+				}
+			}
 
-	ref_transaction_for_each_rejected_update(transaction,
-						 ref_transaction_rejection_handler,
-						 &failed_refs);
+			cmd->error_string = update(cmd, si);
+		}
 
-	if (strmap_empty(&failed_refs))
-		goto cleanup;
+		/* No transaction, so nothing to commit */
+		if (!transaction)
+			goto cleanup;
 
-failure:
-	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (reported_error)
-			cmd->error_string = reported_error;
-		else if (strmap_contains(&failed_refs, cmd->ref_name))
-			cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
-	}
+		if (ref_transaction_commit(transaction, &err)) {
+			rp_error("%s", err.buf);
+			reported_error = "failed to update refs";
+			goto failure;
+		}
 
-cleanup:
-	ref_transaction_free(transaction);
-	strmap_clear(&failed_refs, 0);
-	strbuf_release(&err);
+		ref_transaction_for_each_rejected_update(transaction,
+							 ref_transaction_rejection_handler,
+							 &failed_refs);
+
+		if (strmap_empty(&failed_refs))
+			goto cleanup;
+
+	failure:
+		for (cmd = commands; cmd; cmd = cmd->next) {
+			if (reported_error)
+				cmd->error_string = reported_error;
+			else if (strmap_contains(&failed_refs, cmd->ref_name))
+				cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
+		}
+
+	cleanup:
+		ref_transaction_free(transaction);
+		transaction = NULL;
+		strmap_clear(&failed_refs, 0);
+		strbuf_release(&err);
+	}
 }
 
 static void execute_commands_atomic(struct command *commands,
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index dabcc5f811..1649667441 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/next $orgnext $newnext
+		refs/heads/main $orgmain $newmain
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -808,8 +808,8 @@ test_expect_success 'deletion of a non-existent ref is not fed to post-receive a
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/nonexistent $ZERO_OID $ZERO_OID
+		refs/heads/main $orgmain $newmain
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -868,10 +868,10 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/next $orgnext $newnext
-		refs/heads/seen $orgseen $newseen
 		refs/heads/nonexistent $ZERO_OID $ZERO_OID
+		refs/heads/main $orgmain $newmain
+		refs/heads/seen $orgseen $newseen
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -1909,4 +1909,13 @@ test_expect_success 'push with config push.useBitmaps' '
 		--thin --delta-base-offset -q --no-use-bitmap-index <false
 '
 
+test_expect_success 'push with F/D conflict with deletion and creation' '
+	test_when_finished "git branch -D branch" &&
+	git branch branch/conflict &&
+	mk_test testrepo heads/branch/conflict &&
+	git branch -D branch/conflict &&
+	git branch branch &&
+	git push testrepo :refs/heads/branch/conflict refs/heads/branch
+'
+
 test_done

-- 
2.49.0

Re: [PATCH v4 2/2] receive-pack: handle reference deletions separately

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
> we updated the 'git-receive-pack(1)' command to use batched reference
> updates. One edge case which was missed during this implementation was
> when a user pushes multiple branches such as:
>
>   delete refs/heads/branch/conflict
>   create refs/heads/branch
>
> Before using batched updates, the references would be applied
> sequentially and hence no conflicts would arise. With batched updates,
> while the first update applies, the second fails due to D/F conflict. A
> similar issue was present in 'git-fetch(1)' and was fixed by separating

Do you have a reference to such an earlier fix to "git fetch"?  If
so, let's add it here.

> out reference pruning into a separate transaction. Apply a similar
> mechanism for 'git-receive-pack(1)' and separate out reference deletions
> into its own batch.

The implication of this is that the earlier "delete" half of the
operation can succeed and be committed but the "create" half can
fail, leaving the resulting repository without the reference the
user wanted to have.

For now, this "two transactions" may suffice as a workaround but do
you think it is a viable solution for longer term?  As long as we
claim that the reference updates are transactional, my answer is no.
We'd need to fix it at a lower layer within a single transaction.

It is outside the topic of this patch series but we can at least
leave a NEEDSWORK comment that this is merely a workaround and we'll
have to fix the later?  I see a in-code comment that says "To
mitigate this" to hint the nature of the two phase solution, but we
may want an explicit note that says that "we know this is broken
even though it is less broken than it used to be".

> This means 'git-receive-pack(1)' will now use up to two transactions,
> whereas before using batched updates it would use _at least_ two
> transactions. So using batched updates is still the better option.
>
> Add a test to validate this behavior.

I wonder if we can write a test against a remote that accepts
deletions but fails the actions in the second phase as a
test_expect_failure documentation?

Other than that, very well described.  I know it is hard to describe
a patch that knowingly does a workaround instead of doing the right
thing for the sake of simplicity, and the proposed log message did a
very good job at it.

Will queue.  Thanks.

Re: [PATCH v4 2/2] receive-pack: handle reference deletions separately

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 4680 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
>> we updated the 'git-receive-pack(1)' command to use batched reference
>> updates. One edge case which was missed during this implementation was
>> when a user pushes multiple branches such as:
>>
>>   delete refs/heads/branch/conflict
>>   create refs/heads/branch
>>
>> Before using batched updates, the references would be applied
>> sequentially and hence no conflicts would arise. With batched updates,
>> while the first update applies, the second fails due to D/F conflict. A
>> similar issue was present in 'git-fetch(1)' and was fixed by separating
>
> Do you have a reference to such an earlier fix to "git fetch"?  If
> so, let's add it here.
>

Good point, will add it in.

>> out reference pruning into a separate transaction. Apply a similar
>> mechanism for 'git-receive-pack(1)' and separate out reference deletions
>> into its own batch.
>
> The implication of this is that the earlier "delete" half of the
> operation can succeed and be committed but the "create" half can
> fail, leaving the resulting repository without the reference the
> user wanted to have.
>

I'm not sure which earlier you're referring to, so let me describe them
all.

- Before we introduced batch updates. In this scenario, every reference
  update was independent. Meaning every update could pass/fail and we'd
  still iterate over all other references. Here, if a 'delete' reference
  passed and the 'create' failed, we'd end up in the scenario you
  mentioned.

- Post introduction of the batch updates. In this scenario, we use batch
  transactions, where all updates where batched together. Even here,
  individual updates can pass/fail and we'd still iterate over all the
  other references. The only issue was that, if there was a 'delete' &
  'create' with a D/F conflict, then the create would fail as they were
  within the same transaction. The ultimate fix for this is conflict
  resolution within a transaction.

- After this fix. In this scenario, we separate out deletions into their
  transaction, this ensures we don't have D/F conflicts within a single
  transaction. But we still iterate over all references.

You're right that now 'delete' half can pass and 'create' could fail,
leaving the repository in a state the user didn't want. But this was
also true before batched updates were introduced.

> For now, this "two transactions" may suffice as a workaround but do
> you think it is a viable solution for longer term?  As long as we
> claim that the reference updates are transactional, my answer is no.
> We'd need to fix it at a lower layer within a single transaction.
>

Yes definitely agreed here. Conflict resolution within a transaction is
something we should definitely look into after this, wherein if there is
a 'delete' and a 'create' within a single transaction they shouldn't
cause D/F conflicts. Since the conflict is going to be deleted out
anyways.

> It is outside the topic of this patch series but we can at least
> leave a NEEDSWORK comment that this is merely a workaround and we'll
> have to fix the later?  I see a in-code comment that says "To
> mitigate this" to hint the nature of the two phase solution, but we
> may want an explicit note that says that "we know this is broken
> even though it is less broken than it used to be".
>

Totally valid point. I will add in something like that.

>> This means 'git-receive-pack(1)' will now use up to two transactions,
>> whereas before using batched updates it would use _at least_ two
>> transactions. So using batched updates is still the better option.
>>
>> Add a test to validate this behavior.
>
> I wonder if we can write a test against a remote that accepts
> deletions but fails the actions in the second phase as a
> test_expect_failure documentation?
>

Well we possibly could, but could you explain the intent here? I'm not
able to see why we'd need that. Mostly since we allow individual
reference updates to fail when using batched updates. So the second
transaction here could pass with a few individual updates failing, this
would be similar to how things were before we introduced batched
reference updates to 'git-receive-pack(1)'.

> Other than that, very well described.  I know it is hard to describe
> a patch that knowingly does a workaround instead of doing the right
> thing for the sake of simplicity, and the proposed log message did a
> very good job at it.
>
> Will queue.  Thanks.
>

Thanks, and sorry for the late response, I was on vacation. Will send in
a new version soon.

- Karthik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v4 0/2] refs: fix some bugs with batched-updates

[Christian Couder]

On Fri, Jun 13, 2025 at 10:10 AM Karthik Nayak <karthik.188@gmail.com> wrote:

> Changes in v4:
> - Swapped out F/D for D/F in the second commit, since we are talking
>   about conflicts between a directory and a file, also D/F is more
>   consistent.
> - Fixed some typos in the second commit.
> - Changed comment to single line.
> - 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

This v4 looks good to me based on the range-diff and my previous look at the v3.

Thanks!

Re: [PATCH v4 0/2] refs: fix some bugs with batched-updates

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Fri, Jun 13, 2025 at 10:10 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>
>> Changes in v4:
>> - Swapped out F/D for D/F in the second commit, since we are talking
>>   about conflicts between a directory and a file, also D/F is more
>>   consistent.
>> - Fixed some typos in the second commit.
>> - Changed comment to single line.
>> - 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
>
> This v4 looks good to me based on the range-diff and my previous look at the v3.

Thanks, all.

[PATCH v5 0/2] refs: fix some bugs with batched-updates

[Karthik Nayak]

In 23fc8e4f61 (refs: implement batch reference update support,
2025-04-08) we introduced a mechanism to batch reference updates. The
idea being that we wanted to batch updates similar to a transaction, but
allow certain updates to fail. This would help reference backends
optimize such operations and also remove the overhead of processing
updates individually. Especially for the reftable backend, where
batching updates would ensure that the autocompaction would only occur
at the end of the batch instead of after every reference update.

As of 9d2962a7c4 (receive-pack: use batched reference updates,
2025-05-19) we also updated the 'git-fetch(1)' and 'git-receive-pack(1)'
command to use batched reference updates. This series fixes some bugs
that we found at GitLab by running our Gitaly service with the `next`
build of Git.

The first being in the files backend, which missed skipping over failed
updates in certain flows. When certain individual updates fail, we mark
them as such. However, we missed skipping over such failed updates,
which can cause a SEGFAULT.

The other is in the git-receive-pack(1) implementation when a user
pushes multiple branches such as:

  delete refs/heads/branch/conflict
  create refs/heads/branch

Before using batched updates, the references would be applied
sequentially and hence no conflicts would arise. With batched updates,
while the first update applies, the second fails due to F/D conflict. A
similar issue was present in 'git-fetch(1)' and was fixed by using
separating out reference pruning into a separate transaction. Apply a
similar mechanism for 'git-receive-pack(1)' and separate out reference
deletions into its own batch.

This is based off master 7014b55638 (A bit more topics for -rc1,
2025-05-30), with the changes from kn/fetch-push-bulk-ref-update merged
in.

---
Changes in v5:
- Modify the commit message in patch 2 to also mention the commit
  which adds multiple transactions to 'git-fetch(1)'.
- Also add a NEEDSWORK to the existing comment to mention how conflict
  resolution within transactions can remove the need for multiple 
  transactions.
- 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

Changes in v4:
- Swapped out F/D for D/F in the second commit, since we are talking
  about conflicts between a directory and a file, also D/F is more
  consistent.
- Fixed some typos in the second commit.
- Changed comment to single line.
- 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

Changes in v3:
- Cleanup the second commit message and remove stale comments.
- In the second commit, only create the transaction if needed.
- 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

Changes in v2:
- Modify the test in the first commit to no longer do a quiet grep,
  and some more tests.
- Remove the second commit as it was unnecessary.
- Modify the commit message in the last commit, to also talk about how
  we now use 2 transactions at minimum but this is still better than
  before.
- Modify the logic in the last commit to no longer use an XOR and
  instead loop over the two different scenarios (deletion updates, other
  updates).
- Link to v1: https://lore.kernel.org/r/20250602-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v1-0-903d1db3f10e@gmail.com

---
 builtin/receive-pack.c | 102 ++++++++++++++++++++++++++++++++-----------------
 refs/files-backend.c   |   7 ++++
 t/t1400-update-ref.sh  |  45 ++++++++++++++++++++++
 t/t5516-fetch-push.sh  |  17 +++++++--
 4 files changed, 133 insertions(+), 38 deletions(-)

Karthik Nayak (2):
      refs/files: skip updates with errors in batched updates
      receive-pack: handle reference deletions separately

Range-diff versus v4:

1:  0ab0890bf1 = 1:  9248bfd474 refs/files: skip updates with errors in batched updates
2:  e45ec3139b ! 2:  00bf816b2d receive-pack: handle reference deletions separately
    @@ Commit message
         sequentially and hence no conflicts would arise. With batched updates,
         while the first update applies, the second fails due to D/F conflict. A
         similar issue was present in 'git-fetch(1)' and was fixed by separating
    -    out reference pruning into a separate transaction. Apply a similar
    -    mechanism for 'git-receive-pack(1)' and separate out reference deletions
    -    into its own batch.
    +    out reference pruning into a separate transaction in the commit 'fetch:
    +    use batched reference updates'. Apply a similar mechanism for
    +    'git-receive-pack(1)' and separate out reference deletions into its own
    +    batch.
     
         This means 'git-receive-pack(1)' will now use up to two transactions,
         whereas before using batched updates it would use _at least_ two
    @@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *
     +	 * are treated as conflicts in batched updates. This is because
     +	 * we don't do conflict resolution inside a transaction. To
     +	 * mitigate this, delete references in a separate batch.
    ++	 *
    ++	 * NEEDSWORK: Add conflict resolution between deletion and creation
    ++	 * of reference updates within a transaction. With that, we can
    ++	 * combine the two phases.
     +	 */
     +	enum processing_phase {
     +		PHASE_DELETIONS,


base-commit: 931c39f05e078e0df968a439379cb04b5c4666ef
change-id: 20250528-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-24ff53680144

Thanks
- Karthik

[PATCH v5 1/2] refs/files: skip updates with errors in batched updates

[Karthik Nayak]

The commit 23fc8e4f61 (refs: implement batch reference update support,
2025-04-08) introduced support for batched reference updates. This
allows users to batch updates together, while allowing some of the
updates to fail.

Under the hood, batched updates use the reference transaction mechanism.
Each update which fails is marked as such. Any failed updates must be
skipped over in the rest of the code, as they wouldn't apply any more.
In two of the loops within 'files_transaction_finish()' of the files
backend, the failed updates aren't skipped over. This can cause a
SEGFAULT otherwise. Add the missing skips and a test to validate the
same.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/files-backend.c  |  7 +++++++
 t/t1400-update-ref.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4d1f65a57a..c4a0f29072 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,
 	 */
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
+
+		if (update->rejection_err)
+			continue;
+
 		if (update->flags & REF_DELETING &&
 		    !(update->flags & REF_LOG_ONLY) &&
 		    !(update->flags & REF_IS_PRUNING)) {
@@ -3239,6 +3243,9 @@ static int files_transaction_finish(struct ref_store *ref_store,
 		struct ref_update *update = transaction->updates[i];
 		struct ref_lock *lock = update->backend_data;
 
+		if (update->rejection_err)
+			continue;
+
 		if (update->flags & REF_DELETING &&
 		    !(update->flags & REF_LOG_ONLY)) {
 			update->flags |= REF_DELETED_RMDIR;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index d29d23cb89..ca7eee7de2 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -2299,6 +2299,51 @@ do
 			test_grep -q "refname conflict" stdout
 		)
 	'
+
+	test_expect_success "stdin $type batch-updates delete incorrect symbolic ref" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit c1 &&
+			head=$(git rev-parse HEAD) &&
+			git symbolic-ref refs/heads/symbolic refs/heads/non-existent &&
+
+			format_command $type "delete refs/heads/symbolic" "$head" >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+			test_grep "reference does not exist" stdout
+		)
+	'
+
+	test_expect_success "stdin $type batch-updates delete with incorrect old_oid" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit c1 &&
+			git branch new-branch &&
+			test_commit c2 &&
+			head=$(git rev-parse HEAD) &&
+
+			format_command $type "delete refs/heads/new-branch" "$head" >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+			test_grep "incorrect old value provided" stdout
+		)
+	'
+
+	test_expect_success "stdin $type batch-updates delete non-existent ref" '
+		git init repo &&
+		test_when_finished "rm -fr repo" &&
+		(
+			cd repo &&
+			test_commit commit &&
+			head=$(git rev-parse HEAD) &&
+
+			format_command $type "delete refs/heads/non-existent" "$head" >stdin &&
+			git update-ref $type --stdin --batch-updates <stdin >stdout &&
+			test_grep "reference does not exist" stdout
+		)
+	'
 done
 
 test_expect_success 'update-ref should also create reflog for HEAD' '

-- 
2.49.0

[PATCH v5 2/2] receive-pack: handle reference deletions separately

[Karthik Nayak]

In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
we updated the 'git-receive-pack(1)' command to use batched reference
updates. One edge case which was missed during this implementation was
when a user pushes multiple branches such as:

  delete refs/heads/branch/conflict
  create refs/heads/branch

Before using batched updates, the references would be applied
sequentially and hence no conflicts would arise. With batched updates,
while the first update applies, the second fails due to D/F conflict. A
similar issue was present in 'git-fetch(1)' and was fixed by separating
out reference pruning into a separate transaction in the commit 'fetch:
use batched reference updates'. Apply a similar mechanism for
'git-receive-pack(1)' and separate out reference deletions into its own
batch.

This means 'git-receive-pack(1)' will now use up to two transactions,
whereas before using batched updates it would use _at least_ two
transactions. So using batched updates is still the better option.

Add a test to validate this behavior.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/receive-pack.c | 102 ++++++++++++++++++++++++++++++++-----------------
 t/t5516-fetch-push.sh  |  17 +++++++--
 2 files changed, 81 insertions(+), 38 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9e3cfb85cf..1809539201 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1867,47 +1867,81 @@ static void execute_commands_non_atomic(struct command *commands,
 	const char *reported_error = NULL;
 	struct strmap failed_refs = STRMAP_INIT;
 
-	transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
-						  REF_TRANSACTION_ALLOW_FAILURE, &err);
-	if (!transaction) {
-		rp_error("%s", err.buf);
-		strbuf_reset(&err);
-		reported_error = "transaction failed to start";
-		goto failure;
-	}
+	/*
+	 * Reference updates, where D/F conflicts shouldn't arise due to
+	 * one reference being deleted, while the other being created
+	 * are treated as conflicts in batched updates. This is because
+	 * we don't do conflict resolution inside a transaction. To
+	 * mitigate this, delete references in a separate batch.
+	 *
+	 * NEEDSWORK: Add conflict resolution between deletion and creation
+	 * of reference updates within a transaction. With that, we can
+	 * combine the two phases.
+	 */
+	enum processing_phase {
+		PHASE_DELETIONS,
+		PHASE_OTHERS
+	};
 
-	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (!should_process_cmd(cmd) || cmd->run_proc_receive)
-			continue;
+	for (enum processing_phase phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {
+		for (cmd = commands; cmd; cmd = cmd->next) {
+			if (!should_process_cmd(cmd) || cmd->run_proc_receive)
+				continue;
 
-		cmd->error_string = update(cmd, si);
-	}
+			if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))
+				continue;
+			else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))
+				continue;
 
-	if (ref_transaction_commit(transaction, &err)) {
-		rp_error("%s", err.buf);
-		reported_error = "failed to update refs";
-		goto failure;
-	}
+			/*
+			 * Lazily create a transaction only when we know there are
+			 * updates to be added.
+			 */
+			if (!transaction) {
+				transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
+									  REF_TRANSACTION_ALLOW_FAILURE, &err);
+				if (!transaction) {
+					rp_error("%s", err.buf);
+					strbuf_reset(&err);
+					reported_error = "transaction failed to start";
+					goto failure;
+				}
+			}
 
-	ref_transaction_for_each_rejected_update(transaction,
-						 ref_transaction_rejection_handler,
-						 &failed_refs);
+			cmd->error_string = update(cmd, si);
+		}
 
-	if (strmap_empty(&failed_refs))
-		goto cleanup;
+		/* No transaction, so nothing to commit */
+		if (!transaction)
+			goto cleanup;
 
-failure:
-	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (reported_error)
-			cmd->error_string = reported_error;
-		else if (strmap_contains(&failed_refs, cmd->ref_name))
-			cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
-	}
+		if (ref_transaction_commit(transaction, &err)) {
+			rp_error("%s", err.buf);
+			reported_error = "failed to update refs";
+			goto failure;
+		}
 
-cleanup:
-	ref_transaction_free(transaction);
-	strmap_clear(&failed_refs, 0);
-	strbuf_release(&err);
+		ref_transaction_for_each_rejected_update(transaction,
+							 ref_transaction_rejection_handler,
+							 &failed_refs);
+
+		if (strmap_empty(&failed_refs))
+			goto cleanup;
+
+	failure:
+		for (cmd = commands; cmd; cmd = cmd->next) {
+			if (reported_error)
+				cmd->error_string = reported_error;
+			else if (strmap_contains(&failed_refs, cmd->ref_name))
+				cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
+		}
+
+	cleanup:
+		ref_transaction_free(transaction);
+		transaction = NULL;
+		strmap_clear(&failed_refs, 0);
+		strbuf_release(&err);
+	}
 }
 
 static void execute_commands_atomic(struct command *commands,
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index dabcc5f811..1649667441 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/next $orgnext $newnext
+		refs/heads/main $orgmain $newmain
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -808,8 +808,8 @@ test_expect_success 'deletion of a non-existent ref is not fed to post-receive a
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/nonexistent $ZERO_OID $ZERO_OID
+		refs/heads/main $orgmain $newmain
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -868,10 +868,10 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w
 		EOF
 
 		cat >update.expect <<-EOF &&
-		refs/heads/main $orgmain $newmain
 		refs/heads/next $orgnext $newnext
-		refs/heads/seen $orgseen $newseen
 		refs/heads/nonexistent $ZERO_OID $ZERO_OID
+		refs/heads/main $orgmain $newmain
+		refs/heads/seen $orgseen $newseen
 		EOF
 
 		cat >post-receive.expect <<-EOF &&
@@ -1909,4 +1909,13 @@ test_expect_success 'push with config push.useBitmaps' '
 		--thin --delta-base-offset -q --no-use-bitmap-index <false
 '
 
+test_expect_success 'push with F/D conflict with deletion and creation' '
+	test_when_finished "git branch -D branch" &&
+	git branch branch/conflict &&
+	mk_test testrepo heads/branch/conflict &&
+	git branch -D branch/conflict &&
+	git branch branch &&
+	git push testrepo :refs/heads/branch/conflict refs/heads/branch
+'
+
 test_done

-- 
2.49.0

Re: [PATCH v5 0/2] refs: fix some bugs with batched-updates

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> In 23fc8e4f61 (refs: implement batch reference update support,
> 2025-04-08) we introduced a mechanism to batch reference updates.

Just to let you know, as these two are fixups to the topic that are
no longer in 'next' as we rewound the tip of 'next' after release,
if you want, you can redo the base topic instead of piling small
fixes on top.

Will replace.

Thanks.

Re: [PATCH v5 0/2] refs: fix some bugs with batched-updates

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> In 23fc8e4f61 (refs: implement batch reference update support,
>> 2025-04-08) we introduced a mechanism to batch reference updates.
>
> Just to let you know, as these two are fixups to the topic that are
> no longer in 'next' as we rewound the tip of 'next' after release,
> if you want, you can redo the base topic instead of piling small
> fixes on top.
>

Well the first patch in this series, is a bug fix for master since we
already have batched updates exposed via git-update-ref(1). So only the
second patch can be squashed in.

That said, while it is easier for me to not re-roll, I'd happy to do so,
what do you think?

> Will replace.
>
> Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v5 0/2] refs: fix some bugs with batched-updates

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

>> Just to let you know, as these two are fixups to the topic that are
>> no longer in 'next' as we rewound the tip of 'next' after release,
>> if you want, you can redo the base topic instead of piling small
>> fixes on top.
>>
>
> Well the first patch in this series, is a bug fix for master since we
> already have batched updates exposed via git-update-ref(1). So only the
> second patch can be squashed in.
>
> That said, while it is easier for me to not re-roll, I'd happy to do so,
> what do you think?

I'll let you decide; please choose whichever way you consider would
give us the better result.  The second one seems to be a band-aid
that trades one bug with another bug, so it may be prudent to leave
it separate.  It would make it easier for a future change that fixes
the lower-layer transaction processing to refer to it, with "earlier
we took thw two step approach, which had these downsides. now we fix
the issue for real".

Thanks.

Re: [PATCH v5 0/2] refs: fix some bugs with batched-updates

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>> Just to let you know, as these two are fixups to the topic that are
>>> no longer in 'next' as we rewound the tip of 'next' after release,
>>> if you want, you can redo the base topic instead of piling small
>>> fixes on top.
>>>
>>
>> Well the first patch in this series, is a bug fix for master since we
>> already have batched updates exposed via git-update-ref(1). So only the
>> second patch can be squashed in.
>>
>> That said, while it is easier for me to not re-roll, I'd happy to do so,
>> what do you think?
>
> I'll let you decide; please choose whichever way you consider would
> give us the better result.  The second one seems to be a band-aid
> that trades one bug with another bug, so it may be prudent to leave
> it separate.  It would make it easier for a future change that fixes
> the lower-layer transaction processing to refer to it, with "earlier
> we took thw two step approach, which had these downsides. now we fix
> the issue for real".
>
> Thanks.

I was thinking the same. I do think leaving it separate has the benefit
of extra context being present which in the future would be useful like
you mentioned. The only downside being that the base patches are anyway
out of 'next', so we could squash the patch in. But, In the end I do
think that keeping them separate is better also because only one patch
will be squashed in. So let's keep them separate.

- Karthik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]