git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Phillip Wood <phillip.wood@dunelm.org.uk>
Cc: Git Mailing List <git@vger.kernel.org>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2 8/9] [RFC] rebase: fix advice when a fixup creates an empty commit
Date: Wed, 26 Feb 2020 11:45:36 -0800	[thread overview]
Message-ID: <CABPp-BHbn=0UpR=0RWyxsX_zFEx0v1VUamA3vMcQgDY+2454aw@mail.gmail.com> (raw)
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:
>
> 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
>

  reply	other threads:[~2020-02-26 19:45 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-22 23:30 [PATCH 0/3] commit: fix advice for empty commits during rebases Johannes Schindelin via GitGitGadget
2019-10-22 23:30 ` [PATCH 1/3] cherry-pick: add test for `--skip` advice in `git commit` Johannes Schindelin via GitGitGadget
2019-10-22 23:30 ` [PATCH 2/3] sequencer: export the function to get the path of `.git/rebase-merge/` Johannes Schindelin via GitGitGadget
2019-10-22 23:30 ` [PATCH 3/3] commit: give correct advice for empty commit during a rebase Johannes Schindelin via GitGitGadget
2019-10-23  2:45   ` Junio C Hamano
2019-10-24 10:15   ` Phillip Wood
2019-10-25 11:48     ` Johannes Schindelin
2019-10-25 14:01       ` Phillip Wood
2019-11-08  5:28         ` Junio C Hamano
2019-11-08 14:09           ` Johannes Schindelin
2019-11-11 16:13             ` Phillip Wood
2019-10-23  3:02 ` [PATCH 0/3] commit: fix advice for empty commits during rebases Junio C Hamano
2019-10-25 12:11   ` Johannes Schindelin
2019-10-29  2:05     ` Junio C Hamano
2019-10-29 13:00       ` Johannes Schindelin
2019-12-06 16:06 ` [PATCH v2 0/9] " Phillip Wood
2019-12-06 16:06   ` [PATCH v2 1/9] t3404: use test_cmp_rev Phillip Wood
2019-12-06 17:39     ` Junio C Hamano
2019-12-06 16:06   ` [PATCH v2 2/9] cherry-pick: add test for `--skip` advice in `git commit` Phillip Wood
2019-12-06 16:06   ` [PATCH v2 3/9] cherry-pick: check commit error messages Phillip Wood
2019-12-06 16:06   ` [PATCH v2 4/9] sequencer: write CHERRY_PICK_HEAD for reword and edit Phillip Wood
2019-12-06 16:06   ` [PATCH v2 5/9] commit: use enum value for multiple cherry-picks Phillip Wood
2019-12-06 18:13     ` Junio C Hamano
2019-12-06 16:06   ` [PATCH v2 6/9] commit: encapsulate determine_whence() for sequencer Phillip Wood
2019-12-06 18:24     ` Junio C Hamano
2019-12-18 14:26       ` Phillip Wood
2019-12-06 16:06   ` [PATCH v2 7/9] commit: give correct advice for empty commit during a rebase Phillip Wood
2019-12-06 16:06   ` [PATCH v2 8/9] [RFC] rebase: fix advice when a fixup creates an empty commit Phillip Wood
2020-02-26 19:45     ` Elijah Newren [this message]
2019-12-06 16:06   ` [PATCH v2 9/9] [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts Phillip Wood
2019-12-18 14:35     ` Phillip Wood

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABPp-BHbn=0UpR=0RWyxsX_zFEx0v1VUamA3vMcQgDY+2454aw@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=phillip.wood@dunelm.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).