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

From: GitGitGadget <redacted>

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

From: Todd Zullinger <redacted>

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 <redacted>
Signed-off-by: Johannes Schindelin <redacted>
---
 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

From: Johannes Schindelin <redacted>

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

Hi Johannes,

Johannes Schindelin via GitGitGadget wrote:

From: GitGitGadget <redacted>

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

Hi Todd/Johannes

On 15/06/18 05:31, Johannes Schindelin via GitGitGadget wrote:

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

Hi Phillip,

Phillip Wood wrote:

On 15/06/18 05:31, Johannes Schindelin via GitGitGadget wrote:

quoted

From: Todd Zullinger <redacted>
 
+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.

Hi Todd,

On Mon, 18 Jun 2018, Todd Zullinger wrote:

Phillip Wood wrote:

quoted

On 15/06/18 05:31, Johannes Schindelin via GitGitGadget wrote:

quoted

From: Todd Zullinger <redacted>
 
+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

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 <redacted>
Helped-by: Johannes Schindelin [off-list ref]
Signed-off-by: Todd Zullinger <redacted>
---
Hi Johannes,

Johannes Schindelin wrote:

On Mon, 18 Jun 2018, Todd Zullinger wrote:

quoted

Phillip Wood wrote:

quoted

On 15/06/18 05:31, Johannes Schindelin via GitGitGadget wrote:

quoted

From: Todd Zullinger <redacted>

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

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 <redacted>
Helped-by: Johannes Schindelin [off-list ref]
Signed-off-by: Todd Zullinger <redacted>

Trusting that this test passes: ACK!

Ciao,
Dscho