| Message ID | 20191206160614.631724-9-phillip.wood123@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit: fix advice for empty commits during rebases | expand |
On Fri, Dec 6, 2019 at 8:10 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Add a specific message to advise the user what to do when a fixup or > squash command would create an empty commit. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > > Notes: > I'm slightly nervous of moving the call to determine_whence() to > finalized_deferred_config(). I think it is ok but need to do a more > thorough audit of the code to be sure. Any update? > > builtin/commit.c | 32 ++++++++++++++++++++++++++++---- > sequencer.c | 31 ++++++++++++++++++++++++++++++- > sequencer.h | 3 ++- > t/t3403-rebase-skip.sh | 18 ++++++++++++------ > wt-status.h | 5 +++-- > 5 files changed, 75 insertions(+), 14 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index c3b6b8a6bd..4ae984c470 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -52,6 +52,20 @@ N_("You asked to amend the most recent commit, but doing so would make\n" > "it empty. You can repeat your command with --allow-empty, or you can\n" > "remove the commit entirely with \"git reset HEAD^\".\n"); > > +static const char empty_rebase_fixup_advice[] = > +N_("The fixup would make the commit empty\n" > +"If you wish to commit it anyway use:\n" > +"\n" > +" git commit --amend --allow-empty\n" > +" git rebase --continue\n" > +"\n" > +"To remove the commit entirely use:\n" > +"\n" > +" git reset HEAD^\n" > +" git rebase --continue\n" > +"\n" > +"Otherwise, please use 'git rebase --skip' to skip it\n"); How does skipping differ from removing the commit entirely? Which one tosses just the changes from the fixup commit, and which tosses both the fixup and the commit it is fixing? Or do they both do the same thing? > static const char empty_cherry_pick_advice[] = > N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\n" > "If you wish to commit it anyway, use:\n" > @@ -181,8 +195,14 @@ static void determine_whence(struct wt_status *s) > { > if (file_exists(git_path_merge_head(the_repository))) > whence = FROM_MERGE; > - else if (!sequencer_determine_whence(the_repository, &whence)) > - whence = FROM_COMMIT; > + else { > + int res = sequencer_determine_whence(the_repository, &whence, > + amend); > + if (res < 0) > + die(_("could not read sequencer state")); sequencer_determine_whence() tries to determine what type of sequencer operation is in effect, and can return a range of values in whence. It can also fail to determine which type of sequencer operation is in effect -- in multiple ways. And it distinguishes those with via res < 0 vs. res == 0. I guess it makes sense, but it seems a bit weird. Not sure what to suggest. One possibly bad idea just to get the creative juices flowing: Maybe change the call signature to enum commit_whence sequencer_determine_whence(struct repository *r, int amending, enum commit_whence default); with callers passing default == FROM_COMMIT, and the function can return FROM_COMMIT if there is no sequencer state, or FROM_ERROR (FROM_INVALID? FROM_FAILURE?) if there is sequencer state but it cannot be read. (And maybe do something similar with patch 6?) > + else if (!res) > + whence = FROM_COMMIT; > + } > if (s) > s->whence = whence; > } > @@ -192,7 +212,6 @@ static void status_init_config(struct wt_status *s, config_fn_t fn) > wt_status_prepare(the_repository, s); > init_diff_ui_defaults(); > git_config(fn, s); > - determine_whence(s); > s->hints = advice_status_hints; /* must come after git_config() */ > } > > @@ -943,9 +962,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > */ > if (!committable && whence != FROM_MERGE && !allow_empty && > !(amend && is_a_merge(current_head))) { > + fprintf(stderr, "\nwhence = %d\n", whence); Leftover debugging? > s->display_comment_prefix = old_display_comment_prefix; > run_status(stdout, index_file, prefix, 0, s); > - if (amend) > + if (whence == FROM_REBASE_FIXUP) > + fputs(_(empty_rebase_fixup_advice), stderr); > + else if (amend) > fputs(_(empty_amend_advice), stderr); > else if (is_from_cherry_pick(whence) || > whence == FROM_REBASE_PICK) { > @@ -1114,6 +1136,8 @@ static void finalize_deferred_config(struct wt_status *s) > > if (s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED) > s->ahead_behind_flags = AHEAD_BEHIND_FULL; > + > + determine_whence(s); > } > > static int parse_and_validate_options(int argc, const char *argv[], > diff --git a/sequencer.c b/sequencer.c > index 6e153fea76..64242f4ce7 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -5270,7 +5270,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list) > return 0; > } > > -int sequencer_determine_whence(struct repository *r, enum commit_whence *whence) > +int sequencer_determine_whence(struct repository *r, enum commit_whence *whence, > + int amending) > { > if (file_exists(git_path_cherry_pick_head(r))) { > struct object_id cherry_pick_head, rebase_head; > @@ -5286,6 +5287,34 @@ int sequencer_determine_whence(struct repository *r, enum commit_whence *whence) > *whence = FROM_CHERRY_PICK_SINGLE; > > return 1; > + } else if (amending && file_exists(rebase_path_current_fixups()) && > + (file_exists(git_path_squash_msg(r)) || > + file_exists(git_path_merge_msg(r)))) { > + /* > + * If rebase_path_amend() exists the user is running `git > + * commit`, if not we're committing a fixup/squash directly from > + * the sequencer > + */ > + if (file_exists(rebase_path_amend())) { > + struct strbuf rev = STRBUF_INIT; > + struct object_id to_amend, head; > + > + if (get_oid("HEAD", &head)) > + return error(_("amending invalid head")); /* let commit deal with error */ > + if (!read_oneliner(&rev, rebase_path_amend(), 0)) > + return error(_("invalid file: '%s'"), > + rebase_path_amend()); > + if (get_oid_hex(rev.buf, &to_amend)) > + return error(_("invalid contents: '%s'"), > + rebase_path_amend()); > + if (oideq(&head, &to_amend)) { > + *whence = FROM_REBASE_FIXUP; > + return 1; > + } > + } else { > + *whence = FROM_REBASE_FIXUP; > + return 1; > + } > } > > return 0; > diff --git a/sequencer.h b/sequencer.h > index 8d451dbfcb..4e631900b4 100644 > --- a/sequencer.h > +++ b/sequencer.h > @@ -202,5 +202,6 @@ int write_basic_state(struct replay_opts *opts, const char *head_name, > void sequencer_post_commit_cleanup(struct repository *r); > int sequencer_get_last_command(struct repository* r, > enum replay_action *action); > -int sequencer_determine_whence(struct repository *r, enum commit_whence *whence); > +int sequencer_determine_whence(struct repository *r, enum commit_whence *whence, > + int amending); > #endif /* SEQUENCER_H */ > diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh > index a927774910..bc110b66b3 100755 > --- a/t/t3403-rebase-skip.sh > +++ b/t/t3403-rebase-skip.sh > @@ -164,22 +164,28 @@ test_expect_success 'correct advice upon multi cherry-pick picking an empty comm > test_i18ngrep "git cherry-pick --skip" err > ' > > -test_expect_success 'fixup that empties commit fails' ' > +test_expect_success 'correct advice when fixup empties commit' ' > test_when_finished "git rebase --abort" && > ( > set_fake_editor && > test_must_fail env FAKE_LINES="1 fixup 2" git rebase -i \ > - goodbye^ reverted-goodbye > - ) > + goodbye^ reverted-goodbye 2>err > + ) && > + test_i18ngrep "git rebase --skip" err && > + test_must_fail git commit --amend --no-edit 2>err && > + test_i18ngrep "git rebase --skip" err > ' > > -test_expect_success 'squash that empties commit fails' ' > +test_expect_success 'correct advice when squash empties commit' ' > test_when_finished "git rebase --abort" && > ( > set_fake_editor && > test_must_fail env FAKE_LINES="1 squash 2" git rebase -i \ > - goodbye^ reverted-goodbye > - ) > + goodbye^ reverted-goodbye 2>err > + ) && > + test_i18ngrep "git rebase --skip" err && > + test_must_fail git commit --amend --no-edit 2>err && > + test_i18ngrep "git rebase --skip" err > ' > > # Must be the last test in this file > diff --git a/wt-status.h b/wt-status.h > index 5f81bf7507..a4b7fe6de9 100644 > --- a/wt-status.h > +++ b/wt-status.h > @@ -41,7 +41,8 @@ enum commit_whence { > 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_REBASE_PICK /* commit came from a pick/reword/edit */ > + FROM_REBASE_PICK, /* commit came from a pick/reword/edit */ > + FROM_REBASE_FIXUP /* commit came from fixup/squash */ > }; > > static inline int is_from_cherry_pick(enum commit_whence whence) > @@ -52,7 +53,7 @@ static inline int is_from_cherry_pick(enum commit_whence whence) > > static inline int is_from_rebase(enum commit_whence whence) > { > - return whence == FROM_REBASE_PICK; > + return whence == FROM_REBASE_PICK || whence == FROM_REBASE_FIXUP; > } > > struct wt_status_change_data { > -- > 2.24.0 >
diff --git a/builtin/commit.c b/builtin/commit.c index c3b6b8a6bd..4ae984c470 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -52,6 +52,20 @@ N_("You asked to amend the most recent commit, but doing so would make\n" "it empty. You can repeat your command with --allow-empty, or you can\n" "remove the commit entirely with \"git reset HEAD^\".\n"); +static const char empty_rebase_fixup_advice[] = +N_("The fixup would make the commit empty\n" +"If you wish to commit it anyway use:\n" +"\n" +" git commit --amend --allow-empty\n" +" git rebase --continue\n" +"\n" +"To remove the commit entirely use:\n" +"\n" +" git reset HEAD^\n" +" git rebase --continue\n" +"\n" +"Otherwise, please use 'git rebase --skip' to skip it\n"); + static const char empty_cherry_pick_advice[] = N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\n" "If you wish to commit it anyway, use:\n" @@ -181,8 +195,14 @@ static void determine_whence(struct wt_status *s) { if (file_exists(git_path_merge_head(the_repository))) whence = FROM_MERGE; - else if (!sequencer_determine_whence(the_repository, &whence)) - whence = FROM_COMMIT; + else { + int res = sequencer_determine_whence(the_repository, &whence, + amend); + if (res < 0) + die(_("could not read sequencer state")); + else if (!res) + whence = FROM_COMMIT; + } if (s) s->whence = whence; } @@ -192,7 +212,6 @@ static void status_init_config(struct wt_status *s, config_fn_t fn) wt_status_prepare(the_repository, s); init_diff_ui_defaults(); git_config(fn, s); - determine_whence(s); s->hints = advice_status_hints; /* must come after git_config() */ } @@ -943,9 +962,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix, */ if (!committable && whence != FROM_MERGE && !allow_empty && !(amend && is_a_merge(current_head))) { + fprintf(stderr, "\nwhence = %d\n", whence); s->display_comment_prefix = old_display_comment_prefix; run_status(stdout, index_file, prefix, 0, s); - if (amend) + if (whence == FROM_REBASE_FIXUP) + fputs(_(empty_rebase_fixup_advice), stderr); + else if (amend) fputs(_(empty_amend_advice), stderr); else if (is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) { @@ -1114,6 +1136,8 @@ static void finalize_deferred_config(struct wt_status *s) if (s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED) s->ahead_behind_flags = AHEAD_BEHIND_FULL; + + determine_whence(s); } static int parse_and_validate_options(int argc, const char *argv[], diff --git a/sequencer.c b/sequencer.c index 6e153fea76..64242f4ce7 100644 --- a/sequencer.c +++ b/sequencer.c @@ -5270,7 +5270,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list) return 0; } -int sequencer_determine_whence(struct repository *r, enum commit_whence *whence) +int sequencer_determine_whence(struct repository *r, enum commit_whence *whence, + int amending) { if (file_exists(git_path_cherry_pick_head(r))) { struct object_id cherry_pick_head, rebase_head; @@ -5286,6 +5287,34 @@ int sequencer_determine_whence(struct repository *r, enum commit_whence *whence) *whence = FROM_CHERRY_PICK_SINGLE; return 1; + } else if (amending && file_exists(rebase_path_current_fixups()) && + (file_exists(git_path_squash_msg(r)) || + file_exists(git_path_merge_msg(r)))) { + /* + * If rebase_path_amend() exists the user is running `git + * commit`, if not we're committing a fixup/squash directly from + * the sequencer + */ + if (file_exists(rebase_path_amend())) { + struct strbuf rev = STRBUF_INIT; + struct object_id to_amend, head; + + if (get_oid("HEAD", &head)) + return error(_("amending invalid head")); /* let commit deal with error */ + if (!read_oneliner(&rev, rebase_path_amend(), 0)) + return error(_("invalid file: '%s'"), + rebase_path_amend()); + if (get_oid_hex(rev.buf, &to_amend)) + return error(_("invalid contents: '%s'"), + rebase_path_amend()); + if (oideq(&head, &to_amend)) { + *whence = FROM_REBASE_FIXUP; + return 1; + } + } else { + *whence = FROM_REBASE_FIXUP; + return 1; + } } return 0; diff --git a/sequencer.h b/sequencer.h index 8d451dbfcb..4e631900b4 100644 --- a/sequencer.h +++ b/sequencer.h @@ -202,5 +202,6 @@ int write_basic_state(struct replay_opts *opts, const char *head_name, void sequencer_post_commit_cleanup(struct repository *r); int sequencer_get_last_command(struct repository* r, enum replay_action *action); -int sequencer_determine_whence(struct repository *r, enum commit_whence *whence); +int sequencer_determine_whence(struct repository *r, enum commit_whence *whence, + int amending); #endif /* SEQUENCER_H */ diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh index a927774910..bc110b66b3 100755 --- a/t/t3403-rebase-skip.sh +++ b/t/t3403-rebase-skip.sh @@ -164,22 +164,28 @@ test_expect_success 'correct advice upon multi cherry-pick picking an empty comm test_i18ngrep "git cherry-pick --skip" err ' -test_expect_success 'fixup that empties commit fails' ' +test_expect_success 'correct advice when fixup empties commit' ' test_when_finished "git rebase --abort" && ( set_fake_editor && test_must_fail env FAKE_LINES="1 fixup 2" git rebase -i \ - goodbye^ reverted-goodbye - ) + goodbye^ reverted-goodbye 2>err + ) && + test_i18ngrep "git rebase --skip" err && + test_must_fail git commit --amend --no-edit 2>err && + test_i18ngrep "git rebase --skip" err ' -test_expect_success 'squash that empties commit fails' ' +test_expect_success 'correct advice when squash empties commit' ' test_when_finished "git rebase --abort" && ( set_fake_editor && test_must_fail env FAKE_LINES="1 squash 2" git rebase -i \ - goodbye^ reverted-goodbye - ) + goodbye^ reverted-goodbye 2>err + ) && + test_i18ngrep "git rebase --skip" err && + test_must_fail git commit --amend --no-edit 2>err && + test_i18ngrep "git rebase --skip" err ' # Must be the last test in this file diff --git a/wt-status.h b/wt-status.h index 5f81bf7507..a4b7fe6de9 100644 --- a/wt-status.h +++ b/wt-status.h @@ -41,7 +41,8 @@ enum commit_whence { 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_REBASE_PICK /* commit came from a pick/reword/edit */ + FROM_REBASE_PICK, /* commit came from a pick/reword/edit */ + FROM_REBASE_FIXUP /* commit came from fixup/squash */ }; static inline int is_from_cherry_pick(enum commit_whence whence) @@ -52,7 +53,7 @@ static inline int is_from_cherry_pick(enum commit_whence whence) static inline int is_from_rebase(enum commit_whence whence) { - return whence == FROM_REBASE_PICK; + return whence == FROM_REBASE_PICK || whence == FROM_REBASE_FIXUP; } struct wt_status_change_data {