Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Johannes Schindelin]

Hi Stefan,

On Fri, 20 Apr 2018, Stefan Beller wrote:

> > Funny thing is: I tested this with GETTEXT_POISON=1, and it succeeded.
> 
> I actually wanted to review the code leading to this commit, and to find
> where to start reviewing I had 'git grep "This is a combination of"' which
> lead me to the translation files.

Heh...

> s/grep/test_i18ngrep/ doesn't work as the syntax is slightly off,
> s/ ! grep/ test_i18n_grep !/ would work, just pointing out the obvious.

Yes, I actually knew that, but my usage of a s/// as a shorthand for the
idea to replace `grep` with `test_i18ngrep` was indeed misleading. Sorry
about that, and thank you for helping me getting this right.

Ciao,
Dscho

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Johannes Schindelin]

Hi Eric,

On Fri, 20 Apr 2018, Eric Sunshine wrote:

> On Fri, Apr 20, 2018 at 3:29 PM, Eric Sunshine  
> wrote:
> > On Fri, Apr 20, 2018 at 8:17 AM, Johannes Schindelin
> >  wrote:
> >> +   test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
> >> +   git rebase -i HEAD~4 &&
> >> +
> >> +   : now there is a conflict, and comments in the commit message &&
> >> +   git show HEAD >out &&
> >> +   grep "This is a combination of" out &&
> >> +
> >> +   : skip and continue &&
> >> +   git rebase --skip &&
> >
> > I see that this test script doesn't utilize it, but do you want a
> >
> > test_when_finished "reset_rebase" &&
> >
> > before starting the rebase to clean up in case something before "git
> > rebase --skip" fails?

No, because then one of the next test cases fails:

not ok 15 - rebase -i --continue remembers --rerere-autoupdate

;-)

I'll use

test_when_finished "test_might_fail git rebase --abort"

instead, okay? ;-)

Ciao,
Dscho

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Stefan Beller]

Hi Johannes,

> Funny thing is: I tested this with GETTEXT_POISON=1, and it succeeded.

I actually wanted to review the code leading to this commit, and to find
where to start reviewing I had 'git grep "This is a combination of"' which
lead me to the translation files.

s/grep/test_i18ngrep/ doesn't work as the syntax is slightly off,
s/ ! grep/ test_i18n_grep !/ would work, just pointing out the obvious.

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Johannes Schindelin]

Hi Stefan,

On Fri, 20 Apr 2018, Johannes Schindelin wrote:

> A brief test shows, however, that it is not quite as easy as
> s/grep/test_i18ngrep/, something more seems to be broken.

It seems that this week is my Rabbit Hole Week.

Turns out that we have a really, really long-standing bug in our rebase -i
where we construct the commit messages for fixup/squash chains.

Background: when having multiple fixup!/squash! commits for the same
original commit, the intermediate commits have messages starting with the
message

# This is a combination of  commits.

and then every fixup/squash command increments that  and adds a header

# This is the commit message #:

before writing the respective commit message.

The problem arises from the fact that we deduce  from parsing the first
number in ASCII encoding on the first line.

That breaks e.g. when compiling with GETTEXT_POISON, and it is probably
not true in general, either.

So I introduced a patch that handles the absence of an ASCII-encoded
number gracefully, and now the test passes with and without
GETTEXT_POISON.

Thanks for the review that let me find and fix this bug!
Dscho

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Johannes Schindelin]

Hi Stefan,

On Fri, 20 Apr 2018, Stefan Beller wrote:

> On Fri, Apr 20, 2018 at 5:17 AM, Johannes Schindelin
>  wrote:
> > When multiple fixup/squash commands are processed and the last one
> > causes merge conflicts and is skipped, we leave the "This is a
> > combination of ..." comments in the commit message.
> >
> > Noticed by Eric Sunshine.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  t/t3418-rebase-continue.sh | 21 +
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> > index 9214d0bb511..b177baee322 100755
> > --- a/t/t3418-rebase-continue.sh
> > +++ b/t/t3418-rebase-continue.sh
> > @@ -88,6 +88,27 @@ test_expect_success 'rebase passes merge strategy 
> > options correctly' '
> > git rebase --continue
> >  '
> >
> > +test_expect_failure '--continue after failed fixup cleans commit message' '
> > +   git checkout -b with-conflicting-fixup &&
> > +   test_commit wants-fixup &&
> > +   test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 &&
> > +   test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 &&
> > +   test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 &&
> > +   test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
> > +   git rebase -i HEAD~4 &&
> > +
> > +   : now there is a conflict, and comments in the commit message &&
> > +   git show HEAD >out &&
> > +   grep "This is a combination of" out &&
> 
> test_i18n_grep ?

Funny thing is: I tested this with GETTEXT_POISON=1, and it succeeded. So
I dug deeper why: I never understood that this is a *build* option. I
always thought that would be a test-time option... Oh well ;-)

> > +
> > +   : skip and continue &&
> > +   git rebase --skip &&
> > +
> > +   : now the comments in the commit message should have been cleaned 
> > up &&
> > +   git show HEAD >out &&
> > +   ! grep "This is a combination of" out
> 
> same

Will work on a fix. A brief test shows, however, that it is not quite as
easy as s/grep/test_i18ngrep/, something more seems to be broken.

Will keep you posted,
Dscho

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Eric Sunshine]

On Fri, Apr 20, 2018 at 3:29 PM, Eric Sunshine  wrote:
> On Fri, Apr 20, 2018 at 8:17 AM, Johannes Schindelin
>  wrote:
>> +   test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
>> +   git rebase -i HEAD~4 &&
>> +
>> +   : now there is a conflict, and comments in the commit message &&
>> +   git show HEAD >out &&
>> +   grep "This is a combination of" out &&
>> +
>> +   : skip and continue &&
>> +   git rebase --skip &&
>
> I see that this test script doesn't utilize it, but do you want a
>
> test_when_finished "reset_rebase" &&
>
> before starting the rebase to clean up in case something before "git
> rebase --skip" fails?

Stated less ambiguously:

... in case something fails between "git rebase -i ..."
and "git rebase --skip"?

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Eric Sunshine]

On Fri, Apr 20, 2018 at 8:17 AM, Johannes Schindelin
 wrote:
> When multiple fixup/squash commands are processed and the last one
> causes merge conflicts and is skipped, we leave the "This is a
> combination of ..." comments in the commit message.
>
> Signed-off-by: Johannes Schindelin 
> ---
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> @@ -88,6 +88,27 @@ test_expect_success 'rebase passes merge strategy options 
> correctly' '
> +test_expect_failure '--continue after failed fixup cleans commit message' '
> +   git checkout -b with-conflicting-fixup &&
> +   test_commit wants-fixup &&
> +   test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 &&
> +   test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 &&
> +   test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 &&
> +   test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
> +   git rebase -i HEAD~4 &&
> +
> +   : now there is a conflict, and comments in the commit message &&
> +   git show HEAD >out &&
> +   grep "This is a combination of" out &&
> +
> +   : skip and continue &&
> +   git rebase --skip &&

I see that this test script doesn't utilize it, but do you want a

test_when_finished "reset_rebase" &&

before starting the rebase to clean up in case something before "git
rebase --skip" fails?

> +   : now the comments in the commit message should have been cleaned up 
> &&
> +   git show HEAD >out &&
> +   ! grep "This is a combination of" out
> +'

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Stefan Beller]

On Fri, Apr 20, 2018 at 5:17 AM, Johannes Schindelin
 wrote:
> When multiple fixup/squash commands are processed and the last one
> causes merge conflicts and is skipped, we leave the "This is a
> combination of ..." comments in the commit message.
>
> Noticed by Eric Sunshine.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  t/t3418-rebase-continue.sh | 21 +
>  1 file changed, 21 insertions(+)
>
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 9214d0bb511..b177baee322 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -88,6 +88,27 @@ test_expect_success 'rebase passes merge strategy options 
> correctly' '
> git rebase --continue
>  '
>
> +test_expect_failure '--continue after failed fixup cleans commit message' '
> +   git checkout -b with-conflicting-fixup &&
> +   test_commit wants-fixup &&
> +   test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 &&
> +   test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 &&
> +   test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 &&
> +   test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
> +   git rebase -i HEAD~4 &&
> +
> +   : now there is a conflict, and comments in the commit message &&
> +   git show HEAD >out &&
> +   grep "This is a combination of" out &&

test_i18n_grep ?

> +
> +   : skip and continue &&
> +   git rebase --skip &&
> +
> +   : now the comments in the commit message should have been cleaned up 
> &&
> +   git show HEAD >out &&
> +   ! grep "This is a combination of" out

same

Thanks,
Stefan