git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 8/9] [RFC] rebase: fix advice when a fixup creates an empty commit

From
Elijah Newren <newren@gmail.com>
Date
Feb 26, 2020, 19:45 UTC
Message-ID
<CABPp-BHbn=0UpR=0RWyxsX_zFEx0v1VUamA3vMcQgDY+2454aw@mail.gmail.com>
In-Reply-To
<20191206160614.631724-9-phillip.wood123@gmail.com>
On Fri, Dec 6, 2019 at 8:10 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
Show 13 quoted lines
>
> 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?
Show 29 quoted lines
>
>  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?

Show 14 quoted lines
>  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?)
Show 19 quoted lines
> +               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?
Show 144 quoted lines
>                 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
>
Previous: Phillip Wood
Message 31 of 31 in “commit: fix advice for empty commits during rebases”
  1. 0/3 commit: fix advice for empty commits during rebasesJohannes Schindelin via GitGitGadget, Oct 22, 2019
  2. 2/3 sequencer: export the function to get the path of `.git/rebase-merge/`Johannes Schindelin via GitGitGadget, Oct 22, 2019
  3. 1/3 cherry-pick: add test for `--skip` advice in `git commit`Johannes Schindelin via GitGitGadget, Oct 22, 2019
  4. 3/3 commit: give correct advice for empty commit during a rebaseJohannes Schindelin via GitGitGadget, Oct 22, 2019
  5. Junio C HamanoOct 23, 2019
  6. Phillip WoodOct 24, 2019
  7. Johannes SchindelinOct 25, 2019
  8. Phillip WoodOct 25, 2019
  9. Junio C HamanoNov 8, 2019
  10. Johannes SchindelinNov 8, 2019
  11. Phillip WoodNov 11, 2019
  12. Junio C HamanoOct 23, 2019
  13. Johannes SchindelinOct 25, 2019
  14. Junio C HamanoOct 29, 2019
  15. Johannes SchindelinOct 29, 2019
  16. 0/9 commit: fix advice for empty commits during rebasesPhillip Wood, Dec 6, 2019
  17. 2/9 cherry-pick: add test for `--skip` advice in `git commit`Phillip Wood, Dec 6, 2019
  18. 4/9 sequencer: write CHERRY_PICK_HEAD for reword and editPhillip Wood, Dec 6, 2019
  19. 3/9 cherry-pick: check commit error messagesPhillip Wood, Dec 6, 2019
  20. 1/9 t3404: use test_cmp_revPhillip Wood, Dec 6, 2019
  21. Junio C HamanoDec 6, 2019
  22. 6/9 commit: encapsulate determine_whence() for sequencerPhillip Wood, Dec 6, 2019
  23. Junio C HamanoDec 6, 2019
  24. Phillip WoodDec 18, 2019
  25. 5/9 commit: use enum value for multiple cherry-picksPhillip Wood, Dec 6, 2019
  26. Junio C HamanoDec 6, 2019
  27. 9/9 [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflictsPhillip Wood, Dec 6, 2019
  28. Phillip WoodDec 18, 2019
  29. 7/9 commit: give correct advice for empty commit during a rebasePhillip Wood, Dec 6, 2019
  30. 8/9 [RFC] rebase: fix advice when a fixup creates an empty commitPhillip Wood, Dec 6, 2019
  31. Elijah NewrenFeb 26, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.