Re: [PATCH v18 3/3] am: support --allow-empty to record specific empty patches

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 8, 2021, 11:32 UTC
Message-ID: <xmqq7dcf49ii.fsf@gitster.g>
In-Reply-To: <220a9954.252.17d98caf6ad.Coremail.pwxu@coremail.cn>

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

Show 11 quoted lines

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

Show 5 quoted lines

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

Show 17 quoted lines

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