Re: [PATCH 0/3] commit: fix advice for empty commits during rebases

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 23, 2019, 03:02 UTC
Message-ID: <xmqqr234i2q0.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <pull.417.git.1571787022.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 16 quoted lines

> In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02), we
> introduced a helpful message that suggests to run git cherry-pick --skip 
> (instead of the previous message that talked about git reset) when a
> cherry-pick failed due to an empty patch.
>
> However, the same message is displayed during a rebase, when the patch
> to-be-committed is empty. In this case, git reset would also have worked,
> but git cherry-pick --skip does not work. This is a regression introduced in
> this cycle.
>
> Let's be more careful here.
>
> Johannes Schindelin (3):
>   cherry-pick: add test for `--skip` advice in `git commit`
>   sequencer: export the function to get the path of `.git/rebase-merge/`
>   commit: give correct advice for empty commit during a rebase

Overall they looked nicely done. The last one may have started its life as "let's fix advice for empty", but no longer is.

The old code used the sequencer_in_use boolean to tell between two states ("are we doing a single pick, or a range pick?"), but now we have another boolean rebase_in_progress, and depending on the shape of the if statements existing codepaths happen to have, these two are combined in different ways to deal with new states. E.g. some places say

	if (rebase_in_progress && !sequencer_in_use)
		we are doing a rebase;
	else
		we are doing a cherry-pick;

and some others say

	if (sequencer_in_use)
		we are doing a multi pick;
	else if (rebase_in_progress)
		we are doing a rebase;
	else
		we are doing a single pick;

I wonder if it makes the resulting logic simpler to reason about if these combination of two variables are rewritten to use a single variable that enumerates three (or is it four, counting "we are doing neither a cherry-pick or a rebase"?) possible state.

Other than that, looked sensible.  Will queue.

Thanks.