Bug#955152: git-rebase ignores or squashes GIT_REFLOG_ACTION

2020-03-27 Thread 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  
> 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 
 Date:   Sat Feb 15 21:36:37 2020 +

 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(, "rebase (%s)", options->action);
setenv(GIT_REFLOG_ACTION_ENVIRONMENT, buf.buf, 1);
strbuf_release();

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 
 Date:   Sat Feb 15 21:36:36 2020 +

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

If I am reading

 commit 13a5a9f0fdcf36270dcc2dcb7752c281bbea06f1
 Author: Johannes Schindelin 
 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



Bug#955152: git-rebase ignores or squashes GIT_REFLOG_ACTION

2020-03-27 Thread Ian Jackson
reassign 955152 git
found 955152 + 1:2.26.0-1
notfound 955152 + 1:2.25.1-1 1:2.11.0-3+deb9u5
retitle 955152 git-rebase ignores or squashes GIT_REFLOG_ACTION
user debian...@lists.debian.org
usertags 955152 - needs-update
thanks

Hi all.  Thanks to Paul for actually filing this bug.

tl;dr: I think this is a regression in src:git and that the test case
  in src:dgit is correct and will pass once the git regression is gone.

Paul Gevers writes ("Bug#955152: git breaks dgit autopkgtest: gdr-newupstream 
test failed"):
>passfail
> gitfrom testing1:2.26.0-1
> dgit   from testing9.10
> all others from testingfrom testing
...
> I copied some of the output at the bottom of this report. Unfortunately
> I couldn't spot where the real error is reported.

Sorry that the test output is not as readable as it could be.  I have
a patch to improve this but it would still not be very easy to see
since it's not clear from the log why the test is grepping as it does.
The real error, causing the test failure, is this:

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

So I think all will be well when there is a version of git in sid
which does not have this (new) bug.

> Currently this regression is blocking the migration of git to testing
> [1]. Due to the nature of this issue, I filed this bug report against
> both packages. Can you please investigate the situation and reassign the
> bug to the right package?

I hope what I have done with the bug (i) has the right syntax and did
what I hoped and (ii) meets with everyone's approval.

Regards,
Ian.

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