Patrick Steinhardt <ps@xxxxxx> writes:
> 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.
>
I will make it easier on the eyes in the next version.
>> 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.
>
This is mostly what I was trying to avoid, but perhaps the way to go
anyways. So I'll do the needfull.
>> >> 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
You're correct. I still think it'd be nice to have the 'update' hook
also be in the same order. But like you said, its okay. I do plan to
take up conflict resolution within transactions eventually, so that
would also fix this. So I'm going to leave this as is for now.
Karthik
Attachment:
signature.asc
Description: PGP signature
