rebase --root: fix `reword` on a root commit

PATCH In review Johannes Schindelin via GitGitGadget 5 participants 17 messages Jun 16, 2018 – Jun 20, 2018

lore

Share & tools

lore

Johannes Schindelin via GitGitGadget

author

2018-06-16 19:26 UTC #0 lore raw

cover rebase --root: fix `reword` on a root commit

From: GitGitGadget <[email protected]>

Todd Zullinger reported this bug in https://public-inbox.org/git/[email protected]/: 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

Signature · 1 line

2.17.0.windows.1

Johannes Schindelin via GitGitGadget

author

2018-06-16 19:26 UTC #1 lore raw

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

From: Todd Zullinger <[email protected]>

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 <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

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

t/t3404-rebase-interactive.sh +9 -0

index c65826dda..ca94c688d 100755
@@ -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 &&

Signature · 1 line

2.17.0.windows.1

Phillip Wood

2018-06-18 10:25 UTC #2 lore raw

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

Hi Todd/Johannes

Quoted 19 lines · Johannes Schindelin via GitGitGadget

From: Todd Zullinger <[email protected]>
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 <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>
---
 t/t3404-rebase-interactive.sh | 9 +++++++++
 1 file changed, 9 insertions(+)
t/t3404-rebase-interactive.sh +0 -0
index c65826dda..ca94c688d 100755

t/t3404-rebase-interactive.sh test_expect_success 'rebase -i --root fixup root commit' ' · line 980

Quoted 10 lines

t/t3404-rebase-interactive.sh +7 -0

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

Quoted 6 lines

t/t3404-rebase-interactive.sh +2 -0

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

Todd Zullinger

2018-06-18 16:50 UTC #3 lore raw

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

Hi Phillip,

Quoted 4 lines · Phillip Wood

Quoted 3 lines · Johannes Schindelin via GitGitGadget
From: Todd Zullinger <[email protected]>

Quoted 11 lines

t/t3404-rebase-interactive.sh test_expect_success 'rebase -i --root fixup root commit' ' · line 980
Quoted 7 lines
t/t3404-rebase-interactive.sh +7 -0
+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.

Signature · 4 lines

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

Johannes Schindelin

2018-06-18 21:46 UTC #4 lore raw

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

Hi Todd,

Quoted 17 lines · Todd Zullinger

Quoted 4 lines · Phillip Wood
Quoted 3 lines · Johannes Schindelin via GitGitGadget
From: Todd Zullinger <[email protected]>
Quoted 11 lines
t/t3404-rebase-interactive.sh test_expect_success 'rebase -i --root fixup root commit' ' · line 980
Quoted 7 lines
t/t3404-rebase-interactive.sh +7 -0
+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.

Quoted 7 lines

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

Todd Zullinger

2018-06-18 22:19 UTC #5 lore raw

PATCH 1/1 t3404: check root commit in 'rebase -i --root reword root commit'

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

Helped-by Johannes Schindelin <[email protected]>

Signed-off-by Todd Zullinger <[email protected]>

---
Hi Johannes,

Quoted 26 lines · Johannes Schindelin

Quoted 17 lines · Todd Zullinger
Quoted 12 lines · Phillip Wood
Review
Quoted 10 lines · Johannes Schindelin via GitGitGadget
From: Todd Zullinger <[email protected]>
(quoted diff) +7 -0
+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"
Quoted 3 lines
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.
Quoted 7 lines
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.

Quoted 7 lines

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(-)

t/t3404-rebase-interactive.sh +2 -1

index e500d7c320..352a52e59d 100755
@@ -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' '

Signature · 5 lines

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"

Johannes Schindelin

2018-06-19 06:54 UTC #6 lore raw

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

Hi Todd,

Quoted 6 lines · 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 <[email protected]>
Helped-by Johannes Schindelin <[email protected]>
Signed-off-by Todd Zullinger <[email protected]>

Trusting that this test passes: ACK!

Ciao,
Dscho

Junio C Hamano

2018-06-19 16:00 UTC #7 lore raw

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

In reply to Todd Zullinger

Quoted 3 lines · Todd Zullinger

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.

Quoted 1 line

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

Quoted 20 lines

Thanks.

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

t/t3404-rebase-interactive.sh +2 -1

index e500d7c320..352a52e59d 100755
@@ -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 Zullinger

2018-06-19 19:13 UTC #8 lore raw

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

Quoted 5 lines · Junio C Hamano

Quoted 1 line · Todd Zullinger
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,

Signature · 1 line

Todd

Johannes Schindelin

2018-06-20 11:03 UTC #9 lore raw

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

In reply to Junio C Hamano

Hi Junio,

Quoted 6 lines · Junio C Hamano

Quoted 3 lines · Todd Zullinger
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). :)

Quoted 6 lines

OK.
Quoted 1 line
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

Junio C Hamano

2018-06-19 16:21 UTC #10 lore raw

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

In reply to Todd Zullinger

t/t3404-rebase-interactive.sh test_expect_success 'rebase -i --root reword root commit' ' · line 982

Quoted 11 lines · Todd Zullinger

index e500d7c320..352a52e59d 100755

t/t3404-rebase-interactive.sh +2 -1

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

Johannes Schindelin

2018-06-20 11:05 UTC #11 lore raw

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

Hi Junio,

Quoted 18 lines · Junio C Hamano

t/t3404-rebase-interactive.sh test_expect_success 'rebase -i --root reword root commit' ' · line 982
Quoted 11 lines · Todd Zullinger
index e500d7c320..352a52e59d 100755
t/t3404-rebase-interactive.sh +2 -1
@@ -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

Johannes Schindelin via GitGitGadget

author

2018-06-16 19:26 UTC #12 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 sequencer.c                   | 2 +-
 t/t3404-rebase-interactive.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

sequencer.c +1 -1

index cca968043..4034c0461 100644
@@ -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;

t/t3404-rebase-interactive.sh +1 -1

index ca94c688d..e500d7c32 100755
@@ -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 &&

Signature · 1 line

2.17.0.windows.1

Todd Zullinger

2018-06-16 20:11 UTC #13 lore raw

In reply to Johannes Schindelin via GitGitGadget

Hi Johannes,

Quoted 10 lines · Johannes Schindelin via GitGitGadget

From: GitGitGadget <[email protected]>
Todd Zullinger reported this bug in
https://public-inbox.org/git/[email protected]/:
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.

Quoted 12 lines

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

Signature · 5 lines

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

Junio C Hamano

2018-06-18 16:21 UTC #14 lore raw

Quoted 10 lines · Todd Zullinger

Hi Johannes,
Quoted 7 lines · Johannes Schindelin via GitGitGadget
From: GitGitGadget <[email protected]>
Todd Zullinger reported this bug in
https://public-inbox.org/git/[email protected]/:
when calling git rebase --root and trying to reword the root commit's message, a BUG is reported.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 13 lines

Quoted 13 lines
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 Zullinger

2018-06-18 16:41 UTC #15 lore raw

Hi,

Quoted 20 lines · Junio C Hamano

Quoted 10 lines · Todd Zullinger
Hi Johannes,
Quoted 7 lines · Johannes Schindelin via GitGitGadget
From: GitGitGadget <[email protected]>
Todd Zullinger reported this bug in
https://public-inbox.org/git/[email protected]/:
when calling git rebase --root and trying to reword the root commit's message, a BUG is reported.
Quoted 7 lines
Quoted 3 lines
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.

Quoted 6 lines

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

Signature · 1 line

Todd

Junio C Hamano

2018-06-18 19:25 UTC #16 lore raw

Quoted 10 lines · Todd Zullinger

Quoted 6 lines
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?

Quoted 8 lines

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.