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

[Junio C Hamano <gitster@pobox.com>]

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