Re: [PATCH v2 6/8] rebase --apply: make reflog messages match rebase --merge

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Phillip Wood <phillip.wood123@gmail.com>,
	Christian Couder <christian.couder@gmail.com>,
	Elijah Newren <newren@gmail.com>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH v2 6/8] rebase --apply: make reflog messages match rebase --merge
Date: Wed, 20 Apr 2022 12:22:54 +0200	[thread overview]
Message-ID: <220420.865yn4833u.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <95161f21e0004cff1bb0915aa39200b286e592e5.1650448612.git.gitgitgadget@gmail.com>

On Wed, Apr 20 2022, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The apply backend creates slightly different reflog messages to the
> merge backend when starting and finishing a rebase and when picking
> commits. The choice of backend is really an implementation detail so
> it is confusing to have the same command create different messages
> depending on which backend is selected. Change the apply backend so
> the reflog messages from the two backends match as closely as
> possible. Note that there is still a difference when committing a
> conflict resolution - the merge backend will use "(continue)" rather
> than "(pick)" in that case as it does not know which command created
> the conflict that it is committing.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c          | 9 +++++----
>  t/t3406-rebase-message.sh | 2 +-
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index e50361fc2a9..678339c7bf7 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -580,10 +580,10 @@ static int move_to_original_branch(struct rebase_options *opts)
>  	if (!opts->onto)
>  		BUG("move_to_original_branch without onto");
>  
> -	strbuf_addf(&branch_reflog, "%s finished: %s onto %s",
> +	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
>  		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
>  		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
> -	strbuf_addf(&head_reflog, "%s finished: returning to %s",
> +	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
>  		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), opts->head_name);
>  	ropts.branch = opts->head_name;
>  	ropts.flags = RESET_HEAD_REFS_ONLY;
> @@ -613,7 +613,8 @@ static int run_am(struct rebase_options *opts)
>  
>  	am.git_cmd = 1;
>  	strvec_push(&am.args, "am");
> -
> +	strvec_pushf(&am.env_array, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",
> +		     getenv(GIT_REFLOG_ACTION_ENVIRONMENT));
>  	if (opts->action && !strcmp("continue", opts->action)) {
>  		strvec_push(&am.args, "--resolved");
>  		strvec_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
> @@ -1763,7 +1764,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		printf(_("First, rewinding head to replay your work on top of "
>  			 "it...\n"));
>  
> -	strbuf_addf(&msg, "%s: checkout %s",
> +	strbuf_addf(&msg, "%s (start): checkout %s",
>  		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
>  	ropts.oid = &options.onto->object.oid;
>  	ropts.orig_head = &options.orig_head,
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index bb2a4949abc..5c6cd9af3bc 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -88,7 +88,7 @@ test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' '
>  write_reflog_expect () {
>  	if test $mode = --apply
>  	then
> -		sed 's/(finish)/finished/; s/ ([^)]*)//'
> +		sed 's/(continue)/(pick)/'
>  	else
>  		cat
>  	fi >expect

I like the new message better, but I wonder given that the old message
has been with us since 6fd2f5e60d4 (rebase: operate on a detached HEAD,
2007-11-08) whether this will break anything in the wild.