Re: [PATCH 3/3] receive-pack: handle reference deletions separately
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 2, 2025, 15:20 UTC
- Message-ID
- <xmqq7c1uuq52.fsf@gitster.g>
- In-Reply-To
- <aD2SPsro694yr60Z@pks.im>
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.
Show 27 quoted lines
> 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?"
Show 19 quoted lines
>> 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.