Hi Junio,
On Wed, 23 Oct 2019, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> writes:
>
> > 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.
Thank you!
> The last one may have started its life as "let's fix advice for
> empty", but no longer is.
Indeed. Sorry, I should have said so in the commit message...
> 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.
That's a good idea! I'd like to postpone that until after the -rc
period, as it is not strictly necessary to fix the bug.
As the bug was introduced in this cycle, I would like to see the bug fix
in v2.24.0, though; I frequently rebase interactively, usually Git for
Windows' patch thicket on top of one of git.git's branches, which often
results in now-empty patches, and I'd like to see the correct advice ;-)
So as not to forget about introducing that `enum`, I opened a ticket at
https://github.com/gitgitgadget/git/issues/426.
Thanks,
Dscho
>
> Other than that, looked sensible. Will queue.
>
> Thanks.
>
