| Message ID | pull.417.git.1571787022.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | commit: fix advice for empty commits during rebases | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> 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. 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.
Hi Junio, On Wed, 23 Oct 2019, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > 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. >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > As the bug was introduced in this cycle, I would like to see the bug fix > in v2.24.0, though;... Do you mean that before the change you blame for the "bug" things worked better than what we have at the tip of 'master', or do you mean that the change you blame for the "bug" changed the behaviour relative to the previous released version but the updated behavior is not what you consider correct?
Hi Junio, On Tue, 29 Oct 2019, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > As the bug was introduced in this cycle, I would like to see the bug fix > > in v2.24.0, though;... > > Do you mean that before the change you blame for the "bug" things > worked better than what we have at the tip of 'master', or do you > mean that the change you blame for the "bug" changed the behaviour > relative to the previous released version but the updated behavior > is not what you consider correct? What I mean is this: previously, when an interactive rebase interrupted due to a now-empty patch, the advice given was correct, of clunky. Now it is incorrect. (Previously, the advice was to `git reset`, which was not completely helpful, but now it suggests to `git cherry-pick --continue`, which is incorrect.) Ciao, Dscho