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

[Patrick Steinhardt <ps@pks.im>]

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