Re: git-rebase ignores or squashes GIT_REFLOG_ACTION

Re: git-rebase ignores or squashes GIT_REFLOG_ACTION

[Jonathan Nieder]

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

Re: git-rebase ignores or squashes GIT_REFLOG_ACTION

[Ian Jackson]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 5483 bytes --]

Jonathan Nieder writes ("Re: git-rebase ignores or squashes GIT_REFLOG_ACTION"):
> 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.

Ah.  Interesting.  A co-worker had a case recently where "merge" did
the wrong thing (silently misapplied a hunk!) but "apply" did the
right thing and I remmber thinking that "merge" ought to be the
default (since it can do a better job by using all of the available
information).  So I applaud that change.

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

git-debrebase invokes git-rebase both with and without -i, depending
on the user's own choice.  In this case, git-debrebase is invoking
git-rebase *without* -i.

But maybe I have misunderstood.  Maybe you mean `are you sure the test
case doesn't demand that "-i" appears in the reflog' ?  In which case,
yes.  The failing line in the test case (which is a shell script) is:
  git reflog | egrep 'debrebase new-upstream.*checkout'

Here is a repro for the problem.  Run the attached script, optionally
with "-i" as a single argument.  The script will "rm -rf d" and
recreate it.  It sets GIT_REFLOG_ACTION="plim" and does a nontrivial
rebase in a fresh tree, and prints the resulting reflog.

On older git, without -i, quoting only the relevant bits:
  fc9165e HEAD@{0}: rebase finished: returning to refs/heads/master
  fc9165e HEAD@{1}: plim: 3
  ebf6515 HEAD@{2}: plim: checkout HEAD~2
This is right except for the final finish message.

On older git, with -i:
  4f6cff0 HEAD@{0}: plim: checkout HEAD~2: returning to refs/heads/master
  4f6cff0 HEAD@{1}: plim: checkout HEAD~2: 3
  9f5e72d HEAD@{2}: plim: checkout HEAD~2
This seems to be wrong for all but the initial checkout.

On newer git, I get this output both with and without -i:
  30067e1 HEAD@{0}: rebase (finish): returning to refs/heads/master
  30067e1 HEAD@{1}: rebase (pick): 3
  8a9a5fd HEAD@{2}: rebase (start): checkout HEAD~2
This is wrong because it doesn't mention "plim" at all.  That's
what is spotted by my test case.

I think the best output would be this:
  30067e1 HEAD@{0}: plim (finish): returning to refs/heads/master
  30067e1 HEAD@{1}: plim (pick): 3
  8a9a5fd HEAD@{2}: plim (start): checkout HEAD~2
or this:
  30067e1 HEAD@{0}: plim: rebase (finish): returning to refs/heads/master
  30067e1 HEAD@{1}: plim: rebase (pick): 3
  8a9a5fd HEAD@{2}: plim: rebase (start): checkout HEAD~2

Which of those two is better depends on whether you think callers
ought to put something to do with "rebase" in GIT_REFLOG_ACTION
somewhere.  In this particular case, git-debrebase sets it to
something like
  debrebase new-upstream $new_version: rebase
because it thinks that its GIT_REFLOG_ACTION value will replace
the usual "rebase", rather than supplementing it.

I think either choice on git-rebase's part would be reasonable and the
output with current git-debrebase behaviour is good enough either way.
The former choice on git-rebase's part would produce the prettiest
output with my current setting of GIT_REFLOG_ACTION.  And it allows
the caller the most control over the reflog messages.  Unconditionally
adding the top-level command being invoked could be simulated by the
caller (whereas adding things like "(finish)" cannot).  So if it were
up to me I would have git-rebase produce the first of my two expected
outputs, ie
  30067e1 HEAD@{0}: plim (finish): returning to refs/heads/master
etc.

But my test case merely demands that "plim" appears *somewhere*.


Aside:

Obviously with an interactive rebase, it might well stop, and the user
will then later presumably git-rebase --continue (or maybe --abort)
etc.  Those commands will no longer (necessarily) have
GIT_REFLOG_ACTION in their environment.  So, with the obvious
implementation, depending on the circumstances the reflog for later
parts of the rebase might not contain all the relevant information.

This is unavoidable unless git-rebase were to make a note of the value
of GIT_REFLOG_ACTION somewhere - and even in that case, it wouldn't
affect git commit (which is often a thing that is run in the middle of
an interactive rebase) unless this note had a global effect.  It seems
to me that it would be a bad idea for git-rebase to do anything like
this.  Not only would it have to squirrel away GIT_REFLOG_ACTION and
(perhaps surprisingly) honour it later, it would presumably have to
try to combine it with the value in force during later commands.

This would all be complicated and provide plenty of opportunity for
weird corner case bugs, in order to do something which (to say the
least) it's not clear is desirable.

Instead, I think that the GIT_REFLOG_ACTION in the environment of
git-rebase should take effect for all the things that are done
by or on behalf of that git-rebase command until that git-rebase
command has exited.  Future commands should honour the
GIT_REFLOG_ACTION in force at that later time.  I hope you
agree :-), and I'm giving this aside for completeness.


I hope this is helpful.

Ian.


[-- Attachment #2: t.sh --]
[-- Type: text/plain, Size: 261 bytes --]

#!/bin/bash
set -e
rm -rf d
mkdir d
cd d

export EDITOR=true
export VISUAL=true

git init

for x in 1 2 3; do
    echo $x >$x
    git add $x
    git commit -m $x
done

git branch old

GIT_REFLOG_ACTION=plim git rebase "$@" --onto HEAD~2 HEAD~1

git reflog |cat

[-- Attachment #3: .signature --]
[-- Type: text/plain, Size: 206 bytes --]


-- 
Ian Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.

Re: git-rebase ignores or squashes GIT_REFLOG_ACTION

[Elijah Newren]

On Sat, Mar 28, 2020 at 4:55 AM Ian Jackson
<ijackson@chiark.greenend.org.uk> wrote:
>
> Jonathan Nieder writes ("Re: git-rebase ignores or squashes GIT_REFLOG_ACTION"):
> > 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.
>
> Ah.  Interesting.  A co-worker had a case recently where "merge" did
> the wrong thing (silently misapplied a hunk!) but "apply" did the
> right thing and I remmber thinking that "merge" ought to be the
> default (since it can do a better job by using all of the available
> information).  So I applaud that change.
>
> > 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?
>
> git-debrebase invokes git-rebase both with and without -i, depending
> on the user's own choice.  In this case, git-debrebase is invoking
> git-rebase *without* -i.
>
> But maybe I have misunderstood.  Maybe you mean `are you sure the test
> case doesn't demand that "-i" appears in the reflog' ?  In which case,
> yes.  The failing line in the test case (which is a shell script) is:
>   git reflog | egrep 'debrebase new-upstream.*checkout'
>
> Here is a repro for the problem.  Run the attached script, optionally
> with "-i" as a single argument.  The script will "rm -rf d" and
> recreate it.  It sets GIT_REFLOG_ACTION="plim" and does a nontrivial
> rebase in a fresh tree, and prints the resulting reflog.
>
> On older git, without -i, quoting only the relevant bits:
>   fc9165e HEAD@{0}: rebase finished: returning to refs/heads/master
>   fc9165e HEAD@{1}: plim: 3
>   ebf6515 HEAD@{2}: plim: checkout HEAD~2
> This is right except for the final finish message.
>
> On older git, with -i:
>   4f6cff0 HEAD@{0}: plim: checkout HEAD~2: returning to refs/heads/master
>   4f6cff0 HEAD@{1}: plim: checkout HEAD~2: 3
>   9f5e72d HEAD@{2}: plim: checkout HEAD~2
> This seems to be wrong for all but the initial checkout.
>
> On newer git, I get this output both with and without -i:
>   30067e1 HEAD@{0}: rebase (finish): returning to refs/heads/master
>   30067e1 HEAD@{1}: rebase (pick): 3
>   8a9a5fd HEAD@{2}: rebase (start): checkout HEAD~2
> This is wrong because it doesn't mention "plim" at all.  That's
> what is spotted by my test case.
>
> I think the best output would be this:
>   30067e1 HEAD@{0}: plim (finish): returning to refs/heads/master
>   30067e1 HEAD@{1}: plim (pick): 3
>   8a9a5fd HEAD@{2}: plim (start): checkout HEAD~2
> or this:
>   30067e1 HEAD@{0}: plim: rebase (finish): returning to refs/heads/master
>   30067e1 HEAD@{1}: plim: rebase (pick): 3
>   8a9a5fd HEAD@{2}: plim: rebase (start): checkout HEAD~2
>
> Which of those two is better depends on whether you think callers
> ought to put something to do with "rebase" in GIT_REFLOG_ACTION
> somewhere.  In this particular case, git-debrebase sets it to
> something like
>   debrebase new-upstream $new_version: rebase
> because it thinks that its GIT_REFLOG_ACTION value will replace
> the usual "rebase", rather than supplementing it.
>
> I think either choice on git-rebase's part would be reasonable and the
> output with current git-debrebase behaviour is good enough either way.
> The former choice on git-rebase's part would produce the prettiest
> output with my current setting of GIT_REFLOG_ACTION.  And it allows
> the caller the most control over the reflog messages.  Unconditionally
> adding the top-level command being invoked could be simulated by the
> caller (whereas adding things like "(finish)" cannot).  So if it were
> up to me I would have git-rebase produce the first of my two expected
> outputs, ie
>   30067e1 HEAD@{0}: plim (finish): returning to refs/heads/master
> etc.
>
> But my test case merely demands that "plim" appears *somewhere*.
>
>
> Aside:
>
> Obviously with an interactive rebase, it might well stop, and the user
> will then later presumably git-rebase --continue (or maybe --abort)
> etc.  Those commands will no longer (necessarily) have
> GIT_REFLOG_ACTION in their environment.  So, with the obvious
> implementation, depending on the circumstances the reflog for later
> parts of the rebase might not contain all the relevant information.
>
> This is unavoidable unless git-rebase were to make a note of the value
> of GIT_REFLOG_ACTION somewhere - and even in that case, it wouldn't
> affect git commit (which is often a thing that is run in the middle of
> an interactive rebase) unless this note had a global effect.  It seems
> to me that it would be a bad idea for git-rebase to do anything like
> this.  Not only would it have to squirrel away GIT_REFLOG_ACTION and
> (perhaps surprisingly) honour it later, it would presumably have to
> try to combine it with the value in force during later commands.
>
> This would all be complicated and provide plenty of opportunity for
> weird corner case bugs, in order to do something which (to say the
> least) it's not clear is desirable.
>
> Instead, I think that the GIT_REFLOG_ACTION in the environment of
> git-rebase should take effect for all the things that are done
> by or on behalf of that git-rebase command until that git-rebase
> command has exited.  Future commands should honour the
> GIT_REFLOG_ACTION in force at that later time.  I hope you
> agree :-), and I'm giving this aside for completeness.
>
>
> I hope this is helpful.

Thanks for the report and details.  Sorry for my slow response; I've
got this thread marked as important, just haven't gotten back to it
with the other things in the queue yet.