From 430b75f7209c554d88e3554eb54cebf4ba1e4608 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Fri, 6 Dec 2019 16:06:12 +0000 Subject: [PATCH] commit: give correct advice for empty commit during a rebase 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 or CHERRY_PICK_HEAD and REBASE_HEAD point to the same commit , we still want to advise to use `git cherry-pick --skip`. Original-patch-by: Johannes Schindelin Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- builtin/commit.c | 24 ++++++++++++++++----- sequencer.c | 39 ++++++++++++++++++++++++++++------- t/t3403-rebase-skip.sh | 36 +++++++++++++++++++++++++++----- t/t3404-rebase-interactive.sh | 26 +++++++++++++++++++++++ wt-status.h | 8 ++++++- 5 files changed, 114 insertions(+), 19 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 8a7c839d57..efc201d581 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_pick_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"); @@ -449,6 +452,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix die(_("cannot do a partial commit during a merge.")); else if (is_from_cherry_pick(whence)) die(_("cannot do a partial commit during a cherry-pick.")); + else if (is_from_rebase(whence)) + die(_("cannot do a partial commit during a rebase.")); } if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec)) @@ -765,7 +770,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, */ else if (whence == FROM_MERGE) hook_arg1 = "merge"; - else if (is_from_cherry_pick(whence)) { + else if (is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) { hook_arg1 = "commit"; hook_arg2 = "CHERRY_PICK_HEAD"; } @@ -942,12 +947,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix, run_status(stdout, index_file, prefix, 0, s); if (amend) fputs(_(empty_amend_advice), stderr); - else if (is_from_cherry_pick(whence)) { + else if (is_from_cherry_pick(whence) || + whence == FROM_REBASE_PICK) { fputs(_(empty_cherry_pick_advice), stderr); if (whence == FROM_CHERRY_PICK_SINGLE) fputs(_(empty_cherry_pick_advice_single), stderr); - else + else if (whence == FROM_CHERRY_PICK_MULTI) fputs(_(empty_cherry_pick_advice_multi), stderr); + else + fputs(_(empty_rebase_pick_advice), stderr); } return 0; } @@ -1152,6 +1160,8 @@ static int parse_and_validate_options(int argc, const char *argv[], die(_("You are in the middle of a merge -- cannot amend.")); else if (is_from_cherry_pick(whence)) die(_("You are in the middle of a cherry-pick -- cannot amend.")); + else if (whence == FROM_REBASE_PICK) + die(_("You are in the middle of a rebase -- cannot amend.")); } if (fixup_message && squash_message) die(_("Options --squash and --fixup cannot be used together")); @@ -1173,7 +1183,8 @@ static int parse_and_validate_options(int argc, const char *argv[], use_message = edit_message; if (amend && !use_message && !fixup_message) use_message = "HEAD"; - if (!use_message && !is_from_cherry_pick(whence) && renew_authorship) + if (!use_message && !is_from_cherry_pick(whence) && + !is_from_rebase(whence) && renew_authorship) die(_("--reset-author can be used only with -C, -c or --amend.")); if (use_message) { use_message_buffer = read_commit_message(use_message); @@ -1182,7 +1193,8 @@ static int parse_and_validate_options(int argc, const char *argv[], author_message_buffer = use_message_buffer; } } - if (is_from_cherry_pick(whence) && !renew_authorship) { + if ((is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) && + !renew_authorship) { author_message = "CHERRY_PICK_HEAD"; author_message_buffer = read_commit_message(author_message); } @@ -1602,6 +1614,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix) if (!reflog_msg) reflog_msg = is_from_cherry_pick(whence) ? "commit (cherry-pick)" + : is_from_rebase(whence) + ? "commit (rebase)" : "commit"; commit_list_insert(current_head, &parents); } diff --git a/sequencer.c b/sequencer.c index dcc2063d33..47e36b6a3c 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1429,9 +1429,19 @@ out: return res; } +static int write_rebase_head(struct object_id *oid) +{ + if (update_ref("rebase", "REBASE_HEAD", oid, + NULL, REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR)) + return error(_("could not update %s"), "REBASE_HEAD"); + + return 0; +} + static int do_commit(struct repository *r, const char *msg_file, const char *author, - struct replay_opts *opts, unsigned int flags) + struct replay_opts *opts, unsigned int flags, + struct object_id *oid) { int res = 1; @@ -1456,8 +1466,12 @@ static int do_commit(struct repository *r, return res; } } - if (res == 1) + if (res == 1) { + if (is_rebase_i(opts) && oid) + if (write_rebase_head(oid)) + return -1; return run_git_commit(r, msg_file, opts, flags); + } return res; } @@ -1945,7 +1959,8 @@ static int do_pick_commit(struct repository *r, flags |= ALLOW_EMPTY; if (!opts->no_commit) { if (author || command == TODO_REVERT || (flags & AMEND_MSG)) - res = do_commit(r, msg_file, author, opts, flags); + res = do_commit(r, msg_file, author, opts, flags, + commit? &commit->object.oid : NULL); else res = error(_("unable to parse commit author")); *check_todo = !!(flags & EDIT_MSG); @@ -2964,9 +2979,7 @@ static int make_patch(struct repository *r, p = short_commit_name(commit); if (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0) return -1; - if (update_ref("rebase", "REBASE_HEAD", &commit->object.oid, - NULL, REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR)) - res |= error(_("could not update %s"), "REBASE_HEAD"); + res |= write_rebase_head(&commit->object.oid); strbuf_addf(&buf, "%s/patch", get_dir(opts)); memset(&log_tree_opt, 0, sizeof(log_tree_opt)); @@ -5316,8 +5329,18 @@ int todo_list_rearrange_squash(struct todo_list *todo_list) int sequencer_determine_whence(struct repository *r, enum commit_whence *whence) { if (file_exists(git_path_cherry_pick_head(r))) { - *whence = file_exists(git_path_seq_dir()) ? - FROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE; + struct object_id cherry_pick_head, rebase_head; + + if (file_exists(git_path_seq_dir())) + *whence = FROM_CHERRY_PICK_MULTI; + if (file_exists(rebase_path()) && + !get_oid("REBASE_HEAD", &rebase_head) && + !get_oid("CHERRY_PICK_HEAD", &cherry_pick_head) && + oideq(&rebase_head, &cherry_pick_head)) + *whence = FROM_REBASE_PICK; + else + *whence = FROM_CHERRY_PICK_SINGLE; + return 1; } diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh index db7e917248..a927774910 100755 --- a/t/t3403-rebase-skip.sh +++ b/t/t3403-rebase-skip.sh @@ -97,9 +97,9 @@ test_expect_success 'correct advice upon picking empty commit' ' test_must_fail git rebase -i --onto goodbye \ amended-goodbye^ amended-goodbye 2>err && test_i18ngrep "previous cherry-pick is now empty" err && - test_i18ngrep "git cherry-pick --skip" err && + test_i18ngrep "git rebase --skip" err && test_must_fail git commit && - test_i18ngrep "git cherry-pick --skip" err + test_i18ngrep "git rebase --skip" err ' test_expect_success 'correct authorship when committing empty pick' ' @@ -120,9 +120,9 @@ test_expect_success 'correct advice upon rewording empty commit' ' --onto goodbye amended-goodbye^ amended-goodbye 2>err ) && test_i18ngrep "previous cherry-pick is now empty" err && - test_i18ngrep "git cherry-pick --skip" err && + test_i18ngrep "git rebase --skip" err && test_must_fail git commit && - test_i18ngrep "git cherry-pick --skip" err + test_i18ngrep "git rebase --skip" err ' test_expect_success 'correct advice upon editing empty commit' ' @@ -133,8 +133,34 @@ test_expect_success 'correct advice upon editing empty commit' ' --onto goodbye amended-goodbye^ amended-goodbye 2>err ) && test_i18ngrep "previous cherry-pick is now empty" err && - test_i18ngrep "git cherry-pick --skip" err && + test_i18ngrep "git rebase --skip" err && test_must_fail git commit && + test_i18ngrep "git rebase --skip" err +' + +test_expect_success 'correct advice upon cherry-picking an empty commit during a rebase' ' + test_when_finished "git rebase --abort" && + ( + set_fake_editor && + test_must_fail env FAKE_LINES="1 exec_git_cherry-pick_amended-goodbye" \ + git rebase -i goodbye^ goodbye 2>err + ) && + test_i18ngrep "previous cherry-pick is now empty" err && + test_i18ngrep "git cherry-pick --skip" err && + test_must_fail git commit 2>err && + test_i18ngrep "git cherry-pick --skip" err +' + +test_expect_success 'correct advice upon multi cherry-pick picking an empty commit during a rebase' ' + test_when_finished "git rebase --abort" && + ( + set_fake_editor && + test_must_fail env FAKE_LINES="1 exec_git_cherry-pick_goodbye_amended-goodbye" \ + git rebase -i goodbye^^ goodbye 2>err + ) && + test_i18ngrep "previous cherry-pick is now empty" err && + test_i18ngrep "git cherry-pick --skip" err && + test_must_fail git commit 2>err && test_i18ngrep "git cherry-pick --skip" err ' diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 99ecd2579f..cdbf763385 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1599,6 +1599,32 @@ test_expect_success 'post-commit hook is called' ' test_cmp expect actual ' +test_expect_success 'correct error message for partial commit after empty pick' ' + test_when_finished "git rebase --abort" && + ( + set_fake_editor && + FAKE_LINES="2 1 1" && + export FAKE_LINES && + test_must_fail git rebase -i A D + ) && + echo x >file1 && + test_must_fail git commit file1 2>err && + test_i18ngrep "cannot do a partial commit during a rebase." err +' + +test_expect_success 'correct error message for commit --amend after empty pick' ' + test_when_finished "git rebase --abort" && + ( + set_fake_editor && + FAKE_LINES="1 1" && + export FAKE_LINES && + test_must_fail git rebase -i A D + ) && + echo x>file1 && + test_must_fail git commit -a --amend 2>err && + test_i18ngrep "middle of a rebase -- cannot amend." err +' + # This must be the last test in this file test_expect_success '$EDITOR and friends are unchanged' ' test_editor_unchanged diff --git a/wt-status.h b/wt-status.h index e38e0942dc..73ab5d4da1 100644 --- a/wt-status.h +++ b/wt-status.h @@ -39,7 +39,8 @@ enum commit_whence { FROM_COMMIT, /* normal */ FROM_MERGE, /* commit came from merge */ FROM_CHERRY_PICK_SINGLE, /* commit came from cherry-pick */ - FROM_CHERRY_PICK_MULTI /* commit came from a sequence of cherry-picks */ + FROM_CHERRY_PICK_MULTI, /* commit came from a sequence of cherry-picks */ + FROM_REBASE_PICK /* commit came from a pick/reword/edit */ }; static inline int is_from_cherry_pick(enum commit_whence whence) @@ -48,6 +49,11 @@ static inline int is_from_cherry_pick(enum commit_whence whence) whence == FROM_CHERRY_PICK_MULTI; } +static inline int is_from_rebase(enum commit_whence whence) +{ + return whence == FROM_REBASE_PICK; +} + struct wt_status_change_data { int worktree_status; int index_status;