Re: [PATCH 6/7] rebase --abort: improve reflog message

[Elijah Newren <newren@gmail.com>]

On Mon, Feb 21, 2022 at 3:19 PM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When aborting a rebase the reflog message looks like
>
>         rebase (abort): updating HEAD
>
> which is not very informative. Improve the message by mentioning the
> branch that we are returning to as we do at the end of a successful
> rebase so it looks like.
>
>         rebase (abort): returning to refs/heads/topic

That is _so_ much nicer.

> If GIT_REFLOG_ACTION is set in the environment we no longer omit
> "(abort)" from the reflog message. We don't omit "(start)" and
> "(finish)" when starting and finishing a rebase in that case so we
> shouldn't omit "(abort)".

I agree.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c          | 28 ++++++---------------
>  t/t3406-rebase-message.sh | 51 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+), 20 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 678339c7bf7..70426e17b40 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -992,23 +992,6 @@ static void NORETURN error_on_missing_default_upstream(void)
>         exit(1);
>  }
>
> -static void set_reflog_action(struct rebase_options *options)
> -{
> -       const char *env;
> -       struct strbuf buf = STRBUF_INIT;
> -
> -       if (!is_merge(options))
> -               return;
> -
> -       env = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
> -       if (env && strcmp("rebase", env))
> -               return; /* only override it if it is "rebase" */
> -
> -       strbuf_addf(&buf, "rebase (%s)", options->action);
> -       setenv(GIT_REFLOG_ACTION_ENVIRONMENT, buf.buf, 1);
> -       strbuf_release(&buf);
> -}
> -
>  static int check_exec_cmd(const char *cmd)
>  {
>         if (strchr(cmd, '\n'))
> @@ -1287,18 +1270,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>         }
>         case ACTION_ABORT: {
>                 struct string_list merge_rr = STRING_LIST_INIT_DUP;
> -               options.action = "abort";
> -               set_reflog_action(&options);
> +               struct strbuf head_msg = STRBUF_INIT;
>
> +               options.action = "abort";
>                 rerere_clear(the_repository, &merge_rr);
>                 string_list_clear(&merge_rr, 1);
>
>                 if (read_basic_state(&options))
>                         exit(1);
> +
> +               strbuf_addf(&head_msg, "%s (abort): returning to %s",
> +                           getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
> +                           options.head_name ? options.head_name
> +                                             : oid_to_hex(&options.orig_head));
>                 ropts.oid = &options.orig_head;
> +               ropts.head_msg = head_msg.buf;
>                 ropts.branch = options.head_name;
>                 ropts.flags = RESET_HEAD_HARD;
> -               ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
>                 if (reset_head(the_repository, &ropts) < 0)
>                         die(_("could not move back to %s"),
>                             oid_to_hex(&options.orig_head));
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index 5c6cd9af3bc..ceca1600053 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -187,6 +187,57 @@ test_reflog () {
>         EOF
>         test_cmp expect actual
>         '
> +
> +       test_expect_success "rebase $mode --abort reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
> +       git checkout conflicts &&
> +       test_when_finished "git reset --hard Q" &&
> +
> +       git log -g -1 conflicts >branch-expect &&
> +       (
> +               if test -n "$reflog_action"
> +               then
> +                       GIT_REFLOG_ACTION="$reflog_action" &&
> +                       export GIT_REFLOG_ACTION
> +               fi &&
> +               test_must_fail git rebase $mode main &&
> +               git rebase --abort
> +       ) &&
> +
> +       git log -g --format=%gs -3 >actual &&
> +       write_reflog_expect <<-EOF &&
> +       ${reflog_action:-rebase} (abort): returning to refs/heads/conflicts
> +       ${reflog_action:-rebase} (pick): P
> +       ${reflog_action:-rebase} (start): checkout main
> +       EOF
> +       test_cmp expect actual &&

I was confused by the ordering at first, before I realized that `git
log -g` is going to give you reflogs from newest to oldest instead of
vice-versa.  Looks good.

> +
> +       # check branch reflog is unchanged
> +       git log -g -1 conflicts >branch-actual &&
> +       test_cmp branch-expect branch-actual
> +       '
> +
> +       test_expect_success "rebase $mode --abort detached HEAD reflog${reflog_action:+ GIT_REFLOG_ACTION=$reflog_action}" '
> +       git checkout Q &&
> +       test_when_finished "git reset --hard Q" &&
> +
> +       (
> +               if test -n "$reflog_action"
> +               then
> +                       GIT_REFLOG_ACTION="$reflog_action" &&
> +                       export GIT_REFLOG_ACTION
> +               fi &&
> +               test_must_fail git rebase $mode main &&
> +               git rebase --abort
> +       ) &&
> +
> +       git log -g --format=%gs -3 >actual &&
> +       write_reflog_expect <<-EOF &&
> +       ${reflog_action:-rebase} (abort): returning to $(git rev-parse Q)
> +       ${reflog_action:-rebase} (pick): P
> +       ${reflog_action:-rebase} (start): checkout main
> +       EOF
> +       test_cmp expect actual
> +       '
>  }

And thanks for checking the detached HEAD case too.