Re: git-rebase ignores or squashes GIT_REFLOG_ACTION

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Ian Jackson wrote[1]:

> [ some git-debrebase invocation etc. ]
> + git reflog
> + egrep 'debrebase new-upstream.*checkout'
> + rc=1
>
> I have looked at the log and repro'd the bug.
>
> git-debrebase (which lives in src:dgit but does not depend on dgit)
> must invoke git-rebase.  It sets GIT_REFLOG_ACTION so that the reflog
> is comprehensible to the user.  In previous versions of git this works
> as expected.  In 2.26.0-1 it does not.
>
> This is easy to reproduce by running
>   GIT_REFLOG_ACTION='zeeks!' git rebase --onto <something> <somethingelse>
> with arguments implying a nontrivial rebase.
>
> The test suite in src:dgit, which is the checks that its
> GIT_REFLOG_ACTION manipulation is effective, and it is this test which
> has now failed and which is blocking the migration of git.
>
> git-rebrebase in sid produces quite different looking reflog entries.
> I guess the code for generating the messages has changed and that
> git-rebase is now *setting* GIT_REFLOG_ACTION (or the equivalent
> internal variable) rather than adding to it.
>
> ISTM that to preserve the documented semantics, it is basically always
> wrong of anything to unconditionally set GIT_REFLOG_ACTION.  In
> src:dgit I have a small bit of code to arrange to always *add* to
> GIT_REFLOG_ACTION, if it is already set.  There might be several
> precise ways to add to GIT_REFLOG_ACTION but the failing test case
> here should be happy with any reasonable choice.

Thanks for reporting.

The main relevant change is that "git rebase" switched its default
backend from "apply" to "merge".  This makes it more robust by using
three-way merges in a similar way to "git cherry-pick".  The "merge"
backend was historically already used for interactive rebases.

I believe some reflog behavior changes were noticed in

 commit 980b482d28482c307648c3abbcb16ae60843c7ae
 Author: Elijah Newren <newren@gmail.com>
 Date:   Sat Feb 15 21:36:37 2020 +0000

     rebase tests: mark tests specific to the am-backend with --am

but we didn't investigate at the time (shame on me).  Anyway, we have
a chance to improve things now.

My first thought is to look at when rebase prepares its msg, in
builtin/rebase.c#set_reflog_action:

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

In the --am case, is_merge is false and this code isn't run.  But in
our case, GIT_REFLOG_ACTION is not rebase, so this code still
shouldn't be run.

My next thought is to look at when this function was changed:

 commit c2417d3af7574adc1fb14f7df31b862aa9551e2e
 Author: Elijah Newren <newren@gmail.com>
 Date:   Sat Feb 15 21:36:36 2020 +0000

     rebase: drop '-i' from the reflog for interactive-based rebases

If I am reading

 commit 13a5a9f0fdcf36270dcc2dcb7752c281bbea06f1
 Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
 Date:   Thu Nov 29 11:09:21 2018 -0800

     rebase: fix GIT_REFLOG_ACTION regression

correctly, then dgit requires that to be '-i' for interactive rebases.
Are we sure that that's not the issue here?

Thanks,
Jonathan

[1] https://bugs.debian.org/955152