From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 08 Dec 2021 11:32:37 GMT
Subject: Re: [PATCH v18 3/3] am: support --allow-empty to record specific empty patches
Message-ID: <xmqq7dcf49ii.fsf@gitster.g>
In-Reply-To: <220a9954.252.17d98caf6ad.Coremail.pwxu@coremail.cn>

Aleen 徐沛文 <pwxu@coremail.cn> writes:

>> This series has been trying to add code to punish users who give
>> "--allow-empty" when the command would have worked the same way
>> without it at least since the last round, and I am not sure where
>> that wish to stop users comes from.  Please explain.  I am starting
>> to think I may be missing something and this extra rigidity may be
>> helping a user (but again, I do not see how).
>
> If there is no such code to check, the user can use
> "--allow-empty" to do the same thing as "--continue" or
> "--resolved" after resolving conflicts because they all go into
> `am_resolve()`.  

Yes, exactly.  That is the point of "allow"; we continue normally,
and even in a narrow situation (i.e. input is empty, and no change
is made to the index) where we normally stop, we allow it to
continue by creating an empty commit.

> In my opinion, "--allow-empty" only makes sense
> to allow the user to create empty commits when:
>
>     1. the result is an empty change (`!index_changed`)
>     2. the input is an empty change (`is_empty_or_missing_file(am_path(state, "patch"))`)

Then you are forcing the user to use it ONLY whe nhe input is empty
and no other time.  Whatever that is, that is not "allow" empty.

>> Here is where we can use is-empty-or-missing on "patch" and give a
>> different message (i.e. "even though the patch file is not empty,
>> the result of your patch application did not change anything"), if
>> we wanted to.  Or you could choose to reject such an attempt, which
>> might be simpler.  I.e.
>> 
>> 		if (allow_empty &&
>> 		    is_empty_or_missing_file(am_path(state, "patch"))
>> 			"No changes - recorded an empty commit."
>> 		else
>> 			... the original error for no changes ...
>> 
>> 
>> Hmm?
>
> Based on the previous checking, there is no need to check
> `is_empty_or_missing_file(am_path(state, "patch"))` here because it is permanently true.

I think you misunderstood.

If we correct the overly strict check that makes "--allow-empty" to
mean "only create empty, anything else is an error", the "is patch
an empty file?" must be checked here.