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

Aleen 徐沛文 [off-list ref] writes:

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.

Dears Hamano,

    I have doubted that since that the default behaviour is that stopping
    when meeting commit messages lacking a patch and giving control back
    to the user, is that necessary to provide duplicated '--empty=die'?
    Should we just provide '--empty=(drop|keep)'?

Aleen

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.