On Sat, Mar 28, 2020 at 4:55 AM Ian Jackson
<ijackson@xxxxxxxxxxxxxxxxxxxxxx> 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@xxxxxx>
> > 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.
