Patrick Steinhardt <ps@xxxxxx> 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@xxxxxxxxx>
>> ---
>> 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:
signature.asc
Description: PGP signature
