From: Phillip Wood Date: Fri, 25 Oct 2019 14:01:19 GMT Subject: Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase Message-ID: In-Reply-To: Hi Dscho On 25/10/2019 12:48, Johannes Schindelin wrote: > Hi Phillip, > > On Thu, 24 Oct 2019, Phillip Wood wrote: > >> On 23/10/2019 00:30, Johannes Schindelin via GitGitGadget wrote: >>> From: Johannes Schindelin >>> >>> In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02), >>> `git commit` learned to suggest to run `git cherry-pick --skip` when >>> trying to cherry-pick an empty patch. >>> >>> However, it was overlooked that there are more conditions than just a >>> `git cherry-pick` when this advice is printed (which originally >>> suggested the neutral `git reset`): the same can happen during a rebase. >>> >>> Let's suggest the correct command, even during a rebase. >>> >>> While at it, we adjust more places in `builtin/commit.c` that >>> incorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that >>> surely this must be a `cherry-pick` in progress. >>> >>> Note: we take pains to handle the situation when a user runs a `git >>> cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec` >>> line in an interactive rebase). On the other hand, it is not possible to >>> run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and >>> `sequencer/` exist, we still want to advise to use `git cherry-pick >>> --skip`. >> >> Thanks for working on this. It's unfortunate that rebase does not remove >> CHERRY_PICK_HEAD for empty commits as it does if the commit is not >> empty. > > Oh, so that's what it is that bites me all the time? I frequently run > interactive rebases and sometimes mess up authorship (taking authorship > of patches that I did not, in fact, author). I guess what happens is > that I `git commit` in the middle of a rebase that was interrupted by > merge conflicts. I used to do that a lot, I eventually trained myself not to commit after conflicts during a rebase and leave it to `rebase --continue` but it is annoying that it does not work > So I would actually like to see the _exact opposite_ of what you want to > see: I want to keep `CHERRY_PICK_HEAD` even in the non-empty case. I don't necessarily object - having it consistent one way or the other would be a distinct improvement, it has been removed when there are conflicts since it's inception [1] (That's v2 of the series which started removing CHERRY_PICK_HEAD in the case of rebase conflicts. I couldn't find any discussion of v1 that recommended that change though). There is also another thread [2]. [1] https://public-inbox.org/git/1297876835-70613-1-git-send-email-jaysoffian@gmail.com/ [2] https://public-inbox.org/git/CAMP44s1EAwHjQ7S2ArLvhNg5qkR05DRJ70tQmP8sXYdOP=i_zQ@mail.gmail.com/ >> I think this is because 'rebase --continue' will skip an empty commit >> so the user _has_ to run 'git commit' manually to keep it. If it had >> been designed so that 'rebase --continue' kept the empty commit and >> 'rebase --skip' skipped it then we would not have this problem but >> it's a bit late to worry about that now. > > True. > >> I don't this patch can distinguish between an empty cherry-pick >> performed by the user while a rebase is in progress and an empty pick >> performed by rebase as both create CHERRY_PICK_HEAD while >> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was >> created by rebase and prints advise based on that which may or may not >> be the correct. I think we could distinguish the two by checking if >> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD. > > I guess we could, but then, I would rather worry about that in the next > cycle. In this cycle, I would rather fix the common case, which is that > a `git rebase -i` fails and tells me to `git cherry-pick --skip` instead > of `git rebase --skip`. > > And even if I performed a `git cherry-pick` during a `git rebase` and > the result would be an empty commit, I'd rather be told to `git rebase > --skip` to continue... > > But if you feel strongly that this should be fixed differently, I'll > gladly leave it to you ;-) I'm happy to wait until the next cycle once we've decided what to do about CHERRY_PICK_HEAD during rebases. Best Wishes Phillip > Ciao, > Dscho > >> >> Best Wishes >> >> >> Phillip >> >>> >>> Signed-off-by: Johannes Schindelin >>> --- >>> builtin/commit.c | 33 ++++++++++++++++++++++++--------- >>> t/t3403-rebase-skip.sh | 9 +++++++++ >>> 2 files changed, 33 insertions(+), 9 deletions(-) >>> >>> diff --git a/builtin/commit.c b/builtin/commit.c >>> index e588bc6ad3..2beae13620 100644 >>> --- a/builtin/commit.c >>> +++ b/builtin/commit.c >>> @@ -59,6 +59,9 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\ >>> " git commit --allow-empty\n" >>> "\n"); >>> >>> +static const char empty_rebase_advice[] = >>> +N_("Otherwise, please use 'git rebase --skip'\n"); >>> + >>> static const char empty_cherry_pick_advice_single[] = >>> N_("Otherwise, please use 'git cherry-pick --skip'\n"); >>> >>> @@ -122,7 +125,7 @@ static enum commit_msg_cleanup_mode cleanup_mode; >>> static const char *cleanup_arg; >>> >>> static enum commit_whence whence; >>> -static int sequencer_in_use; >>> +static int sequencer_in_use, rebase_in_progress; >>> static int use_editor = 1, include_status = 1; >>> static int have_option_m; >>> static struct strbuf message = STRBUF_INIT; >>> @@ -183,6 +186,8 @@ static void determine_whence(struct wt_status *s) >>> whence = FROM_CHERRY_PICK; >>> if (file_exists(git_path_seq_dir())) >>> sequencer_in_use = 1; >>> + if (file_exists(git_path_rebase_merge_dir())) >>> + rebase_in_progress = 1; >>> } >>> else >>> whence = FROM_COMMIT; >>> @@ -453,8 +458,11 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix >>> if (whence != FROM_COMMIT) { >>> if (whence == FROM_MERGE) >>> die(_("cannot do a partial commit during a merge.")); >>> - else if (whence == FROM_CHERRY_PICK) >>> + else if (whence == FROM_CHERRY_PICK) { >>> + if (rebase_in_progress && !sequencer_in_use) >>> + die(_("cannot do a partial commit during a rebase.")); >>> die(_("cannot do a partial commit during a cherry-pick.")); >>> + } >>> } >>> >>> if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec)) >>> @@ -950,10 +958,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix, >>> fputs(_(empty_amend_advice), stderr); >>> else if (whence == FROM_CHERRY_PICK) { >>> fputs(_(empty_cherry_pick_advice), stderr); >>> - if (!sequencer_in_use) >>> - fputs(_(empty_cherry_pick_advice_single), stderr); >>> - else >>> + if (sequencer_in_use) >>> fputs(_(empty_cherry_pick_advice_multi), stderr); >>> + else if (rebase_in_progress) >>> + fputs(_(empty_rebase_advice), stderr); >>> + else >>> + fputs(_(empty_cherry_pick_advice_single), stderr); >>> } >>> return 0; >>> } >>> @@ -1156,8 +1166,11 @@ static int parse_and_validate_options(int argc, const char *argv[], >>> if (amend && whence != FROM_COMMIT) { >>> if (whence == FROM_MERGE) >>> die(_("You are in the middle of a merge -- cannot amend.")); >>> - else if (whence == FROM_CHERRY_PICK) >>> + else if (whence == FROM_CHERRY_PICK) { >>> + if (rebase_in_progress && !sequencer_in_use) >>> + die(_("You are in the middle of a rebase -- cannot amend.")); >>> die(_("You are in the middle of a cherry-pick -- cannot amend.")); >>> + } >>> } >>> if (fixup_message && squash_message) >>> die(_("Options --squash and --fixup cannot be used together")); >>> @@ -1628,9 +1641,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix) >>> reduce_heads_replace(&parents); >>> } else { >>> if (!reflog_msg) >>> - reflog_msg = (whence == FROM_CHERRY_PICK) >>> - ? "commit (cherry-pick)" >>> - : "commit"; >>> + reflog_msg = (whence != FROM_CHERRY_PICK) >>> + ? "commit" >>> + : rebase_in_progress && !sequencer_in_use >>> + ? "commit (rebase)" >>> + : "commit (cherry-pick)"; >>> commit_list_insert(current_head, &parents); >>> } >>> >>> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh >>> index 1f5122b632..77b03ac49f 100755 >>> --- a/t/t3403-rebase-skip.sh >>> +++ b/t/t3403-rebase-skip.sh >>> @@ -76,4 +76,13 @@ test_expect_success 'moved back to branch correctly' ' >>> >>> test_debug 'gitk --all & sleep 1' >>> >>> +test_expect_success 'correct advice upon empty commit' ' >>> + git checkout -b rebase-skip && >>> + test_commit a1 && >>> + test_tick && >>> + git commit --amend -m amended --no-edit && >>> + test_must_fail git rebase -m --onto a1 HEAD^ 2>err && >>> + test_i18ngrep "git rebase --skip" err >>> +' >>> + >>> test_done >>> >> >>