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

[Junio C Hamano <gitster@pobox.com>]

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.

> 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?"


>> 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.