[PATCH 0/2] rebase --root: fix `reword` on a root commit

[PATCH 0/2] rebase --root: fix `reword` on a root commit

[Johannes Schindelin via GitGitGadget]

From: GitGitGadget <gitgitgadget@gmail.com>

Todd Zullinger reported this bug in https://public-inbox.org/git/20180615043111.GS3094@zaya.teonanacatl.net/: when calling git rebase --root and trying to reword the root commit's message, a BUG is reported.

This fixes that.

IMO the bug fix is trivial enough to qualify for inclusion into v2.18.0, still.

Johannes Schindelin (1):
  rebase --root: fix amending root commit messages

Todd Zullinger (1):
  rebase --root: demonstrate a bug while amending root commit messages

 sequencer.c                   | 2 +-
 t/t3404-rebase-interactive.sh | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)


base-commit: 68372c88794aba15f853542008cda39def768372
-- 
2.17.0.windows.1

[PATCH 1/2] rebase --root: demonstrate a bug while amending root commit messages

[Johannes Schindelin via GitGitGadget]

From: Todd Zullinger <tmz@pobox.com>

When splitting a repository, running `git rebase -i --root` to reword
the initial commit, Git dies with

	BUG: sequencer.c:795: root commit without message.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3404-rebase-interactive.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index c65826dda..ca94c688d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -971,6 +971,15 @@ test_expect_success 'rebase -i --root fixup root commit' '
 	test 0 = $(git cat-file commit HEAD | grep -c ^parent\ )
 '
 
+test_expect_failure 'rebase -i --root reword root commit' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	git checkout -b reword-root-branch master &&
+	set_fake_editor &&
+	FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
+	git rebase -i --root &&
+	git show HEAD^ | grep "A changed"
+'
+
 test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on non-interactive rebase' '
 	git reset --hard &&
 	git checkout conflict-branch &&
-- 
2.17.0.windows.1

Re: [PATCH 1/2] rebase --root: demonstrate a bug while amending rootcommit messages

[Phillip Wood]

Hi Todd/Johannes

On 15/06/18 05:31, Johannes Schindelin via GitGitGadget wrote:
> 
> From: Todd Zullinger <tmz@pobox.com>
> 
> When splitting a repository, running `git rebase -i --root` to reword
> the initial commit, Git dies with
> 
> 	BUG: sequencer.c:795: root commit without message.
> 
> Signed-off-by: Todd Zullinger <tmz@pobox.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t3404-rebase-interactive.sh | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index c65826dda..ca94c688d 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -971,6 +971,15 @@ test_expect_success 'rebase -i --root fixup root commit' '
>  	test 0 = $(git cat-file commit HEAD | grep -c ^parent\ )
>  '
>  
> +test_expect_failure 'rebase -i --root reword root commit' '
> +	test_when_finished "test_might_fail git rebase --abort" &&
> +	git checkout -b reword-root-branch master &&
> +	set_fake_editor &&
> +	FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
> +	git rebase -i --root &&
> +	git show HEAD^ | grep "A changed"

I wonder if it should also check that HEAD^ is the root commit, to make
sure that the squash-onto commit that's created and then amended has
been squashed onto.

Best Wishes

Phillip

> +'
> +
>  test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on non-interactive rebase' '
>  	git reset --hard &&
>  	git checkout conflict-branch &&
> 

Re: [PATCH 1/2] rebase --root: demonstrate a bug while amending rootcommit messages

[Todd Zullinger]

Hi Phillip,

Phillip Wood wrote:
> On 15/06/18 05:31, Johannes Schindelin via GitGitGadget wrote:
>> 
>> From: Todd Zullinger <tmz@pobox.com>
>>  
>> +test_expect_failure 'rebase -i --root reword root commit' '
>> +	test_when_finished "test_might_fail git rebase --abort" &&
>> +	git checkout -b reword-root-branch master &&
>> +	set_fake_editor &&
>> +	FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
>> +	git rebase -i --root &&
>> +	git show HEAD^ | grep "A changed"
> 
> I wonder if it should also check that HEAD^ is the root commit, to make
> sure that the squash-onto commit that's created and then amended has
> been squashed onto.

Hmm, is that something which other tests don't cover or an
issue that could affect 'rebase -i --root' with reword
differently than other 'rebase -i' commands?

I admit I'm not well-versed in the rebase -i tests and I
focused only on creating a test which demonstrated the bug I
noticed.

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The average woman would rather be beautiful than smart because the
average man can see better than he can think.

Re: [PATCH 1/2] rebase --root: demonstrate a bug while amending rootcommit messages

[Johannes Schindelin]

Hi Todd,

On Mon, 18 Jun 2018, Todd Zullinger wrote:

> Phillip Wood wrote:
> > On 15/06/18 05:31, Johannes Schindelin via GitGitGadget wrote:
> >> 
> >> From: Todd Zullinger <tmz@pobox.com>
> >>  
> >> +test_expect_failure 'rebase -i --root reword root commit' '
> >> +	test_when_finished "test_might_fail git rebase --abort" &&
> >> +	git checkout -b reword-root-branch master &&
> >> +	set_fake_editor &&
> >> +	FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
> >> +	git rebase -i --root &&
> >> +	git show HEAD^ | grep "A changed"
> > 
> > I wonder if it should also check that HEAD^ is the root commit, to make
> > sure that the squash-onto commit that's created and then amended has
> > been squashed onto.
> 
> Hmm, is that something which other tests don't cover or an
> issue that could affect 'rebase -i --root' with reword
> differently than other 'rebase -i' commands?
> 
> I admit I'm not well-versed in the rebase -i tests and I
> focused only on creating a test which demonstrated the bug I
> noticed.

I think we should test this here, to make sure it is tested, and it should
be as easy as:

	test -z "$(git show -s --format=%p HEAD^)"

Hopefully you beat me to it, otherwise I will try to take care of this
tomorrow.

Ciao,
Dscho

[PATCH] t3404: check root commit in 'rebase -i --root reword root commit'

[Todd Zullinger]

When testing a reworded root commit, ensure that the squash-onto commit
which is created and amended is still the root commit.

Suggested-by: Phillip Wood <phillip.wood@talktalk.net>
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
Hi Johannes,

Johannes Schindelin wrote:
>On Mon, 18 Jun 2018, Todd Zullinger wrote:
>> Phillip Wood wrote:
>>> On 15/06/18 05:31, Johannes Schindelin via GitGitGadget wrote:
>>>>
>>>> From: Todd Zullinger <tmz@pobox.com>
>>>>
>>>> +test_expect_failure 'rebase -i --root reword root commit' '
>>>> +  test_when_finished "test_might_fail git rebase --abort" &&
>>>> +  git checkout -b reword-root-branch master &&
>>>> +  set_fake_editor &&
>>>> +  FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
>>>> +  git rebase -i --root &&
>>>> +  git show HEAD^ | grep "A changed"
>>>
>>> I wonder if it should also check that HEAD^ is the root commit, to make
>>> sure that the squash-onto commit that's created and then amended has
>>> been squashed onto.
>>
>> Hmm, is that something which other tests don't cover or an
>> issue that could affect 'rebase -i --root' with reword
>> differently than other 'rebase -i' commands?
>>
>> I admit I'm not well-versed in the rebase -i tests and I
>> focused only on creating a test which demonstrated the bug I
>> noticed.
>
> I think we should test this here, to make sure it is tested, and it should
> be as easy as:
>
>        test -z "$(git show -s --format=%p HEAD^)"
>
> Hopefully you beat me to it, otherwise I will try to take care of this
> tomorrow.

With luck, this will save you a few minutes, assuming the
commit message is reasonable (or can be improved with help
from Phillip and others). :)

Or Junio may just squash this onto js/rebase-i-root-fix.

Thanks.

 t/t3404-rebase-interactive.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e500d7c320..352a52e59d 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -977,7 +977,8 @@ test_expect_success 'rebase -i --root reword root commit' '
 	set_fake_editor &&
 	FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
 	git rebase -i --root &&
-	git show HEAD^ | grep "A changed"
+	git show HEAD^ | grep "A changed" &&
+	test -z "$(git show -s --format=%p HEAD^)"
 '
 
 test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on non-interactive rebase' '
-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Anyone who is capable of getting themselves made President should on
no account be allowed to do the job.
    -- Douglas Adams, "The Hitchhiker's Guide to the Galaxy"

Re: [PATCH] t3404: check root commit in 'rebase -i --root reword root commit'

[Johannes Schindelin]

Hi Todd,

On Mon, 18 Jun 2018, Todd Zullinger wrote:

> When testing a reworded root commit, ensure that the squash-onto commit
> which is created and amended is still the root commit.
> 
> Suggested-by: Phillip Wood <phillip.wood@talktalk.net>
> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Todd Zullinger <tmz@pobox.com>

Trusting that this test passes: ACK!

Ciao,
Dscho

Re: [PATCH] t3404: check root commit in 'rebase -i --root reword root commit'

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

> With luck, this will save you a few minutes, assuming the
> commit message is reasonable (or can be improved with help
> from Phillip and others). :)

OK.

> Or Junio may just squash this onto js/rebase-i-root-fix.

Nah, not for a hotfix on the last couple of days before the final.
We'd need to build on top, not "squash".

>
> Thanks.
>
>  t/t3404-rebase-interactive.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index e500d7c320..352a52e59d 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -977,7 +977,8 @@ test_expect_success 'rebase -i --root reword root commit' '
>  	set_fake_editor &&
>  	FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
>  	git rebase -i --root &&
> -	git show HEAD^ | grep "A changed"
> +	git show HEAD^ | grep "A changed" &&
> +	test -z "$(git show -s --format=%p HEAD^)"
>  '
>  
>  test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on non-interactive rebase' '

Re: [PATCH] t3404: check root commit in 'rebase -i --root reword root commit'

[Todd Zullinger]

Junio C Hamano wrote:
> Todd Zullinger <tmz@pobox.com> writes:
>> Or Junio may just squash this onto js/rebase-i-root-fix.
> 
> Nah, not for a hotfix on the last couple of days before the final.
> We'd need to build on top, not "squash".

Indeed.  I somehow missed that you'd merged and pushed the
changes to master and next when I set this.  I was
mistakenly thinking it was only on the js/rebase-i-root-fix
integration branch.

Thanks,

-- 
Todd

Re: [PATCH] t3404: check root commit in 'rebase -i --root reword root commit'

[Johannes Schindelin]

Hi Junio,

On Tue, 19 Jun 2018, Junio C Hamano wrote:

> Todd Zullinger <tmz@pobox.com> writes:
> 
> > With luck, this will save you a few minutes, assuming the
> > commit message is reasonable (or can be improved with help
> > from Phillip and others). :)
> 
> OK.
> 
> > Or Junio may just squash this onto js/rebase-i-root-fix.
> 
> Nah, not for a hotfix on the last couple of days before the final.
> We'd need to build on top, not "squash".

Right. Can we take this on top, at a leisurely pace? I mean: we verified
that this works in the upcoming v2.18.0, and it would be nice to have that
extra regression test safety in the future, but it is not crucial to
include it in v2.18.0 itself.

Ciao,
Dscho

Re: [PATCH] t3404: check root commit in 'rebase -i --root reword root commit'

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

> index e500d7c320..352a52e59d 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -977,7 +977,8 @@ test_expect_success 'rebase -i --root reword root commit' '
>  	set_fake_editor &&
>  	FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
>  	git rebase -i --root &&
> -	git show HEAD^ | grep "A changed"
> +	git show HEAD^ | grep "A changed" &&
> +	test -z "$(git show -s --format=%p HEAD^)"
>  '

The additional test probably will pass when HEAD is a root commit by
failing to refer to HEAD^, resulting an empty output from show.  The
previous step would also give an error and won't emit anything that
would match "A changed", so it probably is OK, though.

Re: [PATCH] t3404: check root commit in 'rebase -i --root reword root commit'

[Johannes Schindelin]

Hi Junio,

On Tue, 19 Jun 2018, Junio C Hamano wrote:

> Todd Zullinger <tmz@pobox.com> writes:
> 
> > index e500d7c320..352a52e59d 100755
> > --- a/t/t3404-rebase-interactive.sh
> > +++ b/t/t3404-rebase-interactive.sh
> > @@ -977,7 +977,8 @@ test_expect_success 'rebase -i --root reword root commit' '
> >  	set_fake_editor &&
> >  	FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
> >  	git rebase -i --root &&
> > -	git show HEAD^ | grep "A changed"
> > +	git show HEAD^ | grep "A changed" &&
> > +	test -z "$(git show -s --format=%p HEAD^)"
> >  '
> 
> The additional test probably will pass when HEAD is a root commit by
> failing to refer to HEAD^, resulting an empty output from show.  The
> previous step would also give an error and won't emit anything that
> would match "A changed", so it probably is OK, though.

That matches my thinking. Otherwise, we would have had to do the

	git show -s --format=%p HEAD^ >out &&
	test_must_be_empty out

dance.

Ciao,
Dscho

[PATCH 2/2] rebase --root: fix amending root commit messages

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The code path that triggered that "BUG" really does not want to run
without an explicit commit message. In the case where we want to amend a
commit message, we have an *implicit* commit message, though: the one of
the commit to amend. Therefore, this code path should not even be
entered.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c                   | 2 +-
 t/t3404-rebase-interactive.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index cca968043..4034c0461 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -784,7 +784,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	const char *value;
 
-	if (flags & CREATE_ROOT_COMMIT) {
+	if ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {
 		struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
 		const char *author = is_rebase_i(opts) ?
 			read_author_ident(&script) : NULL;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ca94c688d..e500d7c32 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -971,7 +971,7 @@ test_expect_success 'rebase -i --root fixup root commit' '
 	test 0 = $(git cat-file commit HEAD | grep -c ^parent\ )
 '
 
-test_expect_failure 'rebase -i --root reword root commit' '
+test_expect_success 'rebase -i --root reword root commit' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout -b reword-root-branch master &&
 	set_fake_editor &&
-- 
2.17.0.windows.1

Re: [PATCH 0/2] rebase --root: fix `reword` on a root commit

[Todd Zullinger]

Hi Johannes,

Johannes Schindelin via GitGitGadget wrote:
> From: GitGitGadget <gitgitgadget@gmail.com>
> 
> Todd Zullinger reported this bug in
> https://public-inbox.org/git/20180615043111.GS3094@zaya.teonanacatl.net/:
> when calling git rebase --root and trying to reword the
> root commit's message, a BUG is reported.
>
> This fixes that.
> 
> IMO the bug fix is trivial enough to qualify for inclusion into v2.18.0, still.

It does indeed fix the issue.  I agree it would be nice to
see it in 2.18.0.  As a fix for a minor regression
introduced in this cycle, that seems reasonable.

> Johannes Schindelin (1):
>   rebase --root: fix amending root commit messages
> 
> Todd Zullinger (1):
>   rebase --root: demonstrate a bug while amending root commit messages
> 
>  sequencer.c                   | 2 +-
>  t/t3404-rebase-interactive.sh | 9 +++++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> 
> base-commit: 68372c88794aba15f853542008cda39def768372

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I don't mean to sound cold, or cruel, or vicious,
but I am, so that's the way it comes out.
    -- Bill Hicks

Re: [PATCH 0/2] rebase --root: fix `reword` on a root commit

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

> Hi Johannes,
>
> Johannes Schindelin via GitGitGadget wrote:
>> From: GitGitGadget <gitgitgadget@gmail.com>
>> 
>> Todd Zullinger reported this bug in
>> https://public-inbox.org/git/20180615043111.GS3094@zaya.teonanacatl.net/:
>> when calling git rebase --root and trying to reword the
>> root commit's message, a BUG is reported.
>>
>> This fixes that.
>> 
>> IMO the bug fix is trivial enough to qualify for inclusion into v2.18.0, still.
>
> It does indeed fix the issue.  I agree it would be nice to
> see it in 2.18.0.  As a fix for a minor regression
> introduced in this cycle, that seems reasonable.

Offhand it is not clear from the proposed log message where the
original breakage happened, but if this is to fix a regression
between v2.17.0 and v2.18.0, then let's have it.  As -rc2 slipped
for a few days, it is reasonable to delay the final by a couple of
days as well, if only to give the last minute fixes and translators
reasonable time to breathe.

Thanks.


>
>> Johannes Schindelin (1):
>>   rebase --root: fix amending root commit messages
>> 
>> Todd Zullinger (1):
>>   rebase --root: demonstrate a bug while amending root commit messages
>> 
>>  sequencer.c                   | 2 +-
>>  t/t3404-rebase-interactive.sh | 9 +++++++++
>>  2 files changed, 10 insertions(+), 1 deletion(-)
>> 
>> 
>> base-commit: 68372c88794aba15f853542008cda39def768372

Re: [PATCH 0/2] rebase --root: fix `reword` on a root commit

[Todd Zullinger]

Hi,

Junio C Hamano wrote:
> Todd Zullinger <tmz@pobox.com> writes:
> 
>> Hi Johannes,
>>
>> Johannes Schindelin via GitGitGadget wrote:
>>> From: GitGitGadget <gitgitgadget@gmail.com>
>>> 
>>> Todd Zullinger reported this bug in
>>> https://public-inbox.org/git/20180615043111.GS3094@zaya.teonanacatl.net/:
>>> when calling git rebase --root and trying to reword the
>>> root commit's message, a BUG is reported.
>>>
>>> This fixes that.
>>> 
>>> IMO the bug fix is trivial enough to qualify for inclusion into v2.18.0, still.
>>
>> It does indeed fix the issue.  I agree it would be nice to
>> see it in 2.18.0.  As a fix for a minor regression
>> introduced in this cycle, that seems reasonable.
> 
> Offhand it is not clear from the proposed log message where the
> original breakage happened, but if this is to fix a regression
> between v2.17.0 and v2.18.0, then let's have it.  As -rc2 slipped
> for a few days, it is reasonable to delay the final by a couple of
> days as well, if only to give the last minute fixes and translators
> reasonable time to breathe.

Perhaps replacing the first paragraph with this would make
it clearer?

    Since 21d0764c82 ("rebase -i --root: let the sequencer handle even the
    initial part", 2018-05-04), when splitting a repository, running `git
    rebase -i --root` to reword the initial commit, Git dies with

Alternately, a similar note could be added at the end.

    This regression was recently introduced in 21d0764c82 ("rebase -i 
    --root: let the sequencer handle even the initial part", 2018-05-04).

-- 
Todd

Re: [PATCH 0/2] rebase --root: fix `reword` on a root commit

[Junio C Hamano]

Todd Zullinger <tmz@pobox.com> writes:

>> Offhand it is not clear from the proposed log message where the
>> original breakage happened, but if this is to fix a regression
>> between v2.17.0 and v2.18.0, then let's have it.  As -rc2 slipped
>> for a few days, it is reasonable to delay the final by a couple of
>> days as well, if only to give the last minute fixes and translators
>> reasonable time to breathe.
>
> Perhaps replacing the first paragraph with this would make
> it clearer?
>
>     Since 21d0764c82 ("rebase -i --root: let the sequencer handle even the
>     initial part", 2018-05-04), when splitting a repository, running `git
>     rebase -i --root` to reword the initial commit, Git dies with
>
> Alternately, a similar note could be added at the end.
>
>     This regression was recently introduced in 21d0764c82 ("rebase -i 
>     --root: let the sequencer handle even the initial part", 2018-05-04).

These certainly are ways to require one less hop to the readers than
the original ;-)  Having said that, I've already merged it down to
'next' and want to have these in 'master' before final, so no need
to further fix-up the log message anymore.

Thanks.