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

> 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