Re: What's cooking in git.git (Nov 2021, #07; Mon, 29)

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 6, 2021, 17:23 UTC
Message-ID: <xmqqwnkhhck4.fsf@gitster.g>
In-Reply-To: <6fa17536.18b.17d8e7c7a09.Coremail.pwxu@coremail.cn>

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

Show 14 quoted lines

> The confusing point seems why "git am" stops into the middle and gives
> control back to the user when passing "die". In common cases, "die"
> should stop the whole process, and is it better to distinguish the case
> when passing "die" from the default behaviour? Like what the following
> snippet is implemented:
>
>     case ERR_EMPTY_COMMIT:
>         printf_ln(_("Patch is empty.\n"
>                     "If you want to record it as an empty commit, run \"git am --allow-empty\"."));
> 	die_user_resolve(state);
>     case DIE_EMPTY_COMMIT:
>         am_destroy(state);
>         die(_("Patch is empty."));
>         break;

Sorry, but I am afraid that I still don't get it.

As we can see, the ERR_EMPTY_COMMIT case already exists and that is the behaviour we want when we say "create commits from the messages with patches, stop and give me control back when you see an empty commit, so that I can decide what to do". And one of the things you can do at that point is "am --abort" that causes the am_destroy() to be called.

That is very much in line with the behaviour the users are used to see from "git am" when it detects conditions other than "there is no patch" that needs to stop and give control back to the user, e.g. when the patch does not apply cleanly or the log message did not pass msg hook. am_destroy() is destructive, and limiting such a destructive operation to "am --abort" would avoid mistakes and surprises. I do not know where you got "In common cases, die should stop the whole" from, but it is not a friendly thing to do to our users.

Why should the "in addition, stop when there is no patch", which is what we already handle just fine, needs to become different? More importantly, is it worth forcing the users to be aware of the difference and be extra careful to avoid it?

It is my understanding that the ONLY reason the patch proposes to add an option other than "skip the step" and "create an empty commit" is to allow an earlier "--empty=skip" on the command line to be overridden by a later "--empty=die". If that option does not make the command behave identically to "--empty=<anything>" option on the command line, i.e. ERR_EMPTY_COMMIT case, it does not serve the intended purpose of overriding the earlier option to revert the behaviour back to the default.

By the way, I agree with an earlier comment (I think it was from Dscho) that these names should say "${DO_THIS}_ON_EMPTY_COMMIT"; the above without "ON" feels somewhat strange.

Thanks.