On Mon, Jun 02, 2025 at 08:20:09AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> 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
