diff mbox series

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

Message ID 95161f21e0004cff1bb0915aa39200b286e592e5.1650448612.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series rebase: make reflog messages independent of the backend | expand

Commit Message

Phillip Wood April 20, 2022, 9:56 a.m. UTC 
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(-)

Comments

Ævar Arnfjörð Bjarmason April 20, 2022, 10:22 a.m. UTC | #1 
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.
Junio C Hamano April 20, 2022, 10:15 p.m. UTC | #2 
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

I do not agree with that line of thought.

When choosing backend, the user clearly cares which one is used to
do the job, so it is far from an "implementation detail" that is
better hidden from the users.  In other words, the message being
different is not source of confusion in this case.  Producing
different messages depending on the version of Git may create worse
confusion, though.

> 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.

That is OK, unless we can teach the backend to produce a more proper
one (if 'continue' is considered less proper, that is, than 'pick';
I do not think I would personally care between the two words too
much).

> -	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;

These convey the same information so it is "Meh---I do not care too
strongly about them" kind of change, unless the justification is
"let's show the (word) in parehtheses consistently in these reflog
entries", in which case they are welcome change.

Overall, I'd rather see us not to apply this patch at this point in
time, especially ...

> +	strvec_pushf(&am.env_array, GIT_REFLOG_ACTION_ENVIRONMENT "=%s (pick)",

... this new use of .env_array member semantically conflicts with
the removal of .env member from the structure being proposed in
another topic in flight.

> +		     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);

I think this one is a strict improvement with or without () around
the word that tells us what phase of the rebase operation we are at,
and if it happens to match what the other backend produces, that
would be a tolerable small regression, even though it makes it hard
to tell them apart.
diff mbox series

Patch

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