[RFC PATCH 3/7] rebase -i: Stop on root commits with empty log messages

When `rebase` is executed with `--root` but no `--onto` is specified,
`rebase` creates a sentinel commit which is replaced with the root
commit in three steps. This combination of options results never in a
fast-forward.

 1. The sentinel commit is forced into the authorship of the root
    commit.

 2. The changes introduced by the root commit are applied to the index
    but not committed. If this step fails for whatever reason, all
    commit information will be there and the user can safely run
    `git-commit --amend` after resolving the problems.

 3. The new root commit is created by squashing the changes into the
    sentinel commit which already carries the authorship of the
    cherry-picked root commit.

The command line used to create the commit in the third step specifies
effectless and erroneous options. Remove those.

 - `--allow-empty-message` is erroneous: If the root's commit message is
   empty, the rebase shall fail like for any other commit that is on the
   to-do list and has an empty commit message.

   Fix the bug that git-rebase does not fail when the initial commit has
   an empty log message but is replayed using `--root` is specified.
   Add test.

 - `-C` is effectless: The commit being amended, which is the sentinel
   commit, already carries the authorship and log message of the
   cherry-picked root commit. The committer email and commit date fields
   are reset either way.

After all, if step two fails, `rebase --continue` won't include these
flags in the git-commit command line either.

Signed-off-by: Fabian Ruch <redacted>
---
 git-rebase--interactive.sh |  4 ++--
 t/t3412-rebase-root.sh     | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index fffdfa5..f09eeae 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -539,8 +539,8 @@ do_pick () {
 		git commit --allow-empty --allow-empty-message --amend \
 			   --no-post-rewrite -n -q -C $1 &&
 			pick_one -n $1 &&
-			git commit --allow-empty --allow-empty-message \
-				   --amend --no-post-rewrite -n -q -C $1 \
+			git commit --allow-empty --amend \
+				   --no-post-rewrite -n -q \
 				   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
 			die_with_patch $1 "Could not apply $1... $2"
 	else

diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 0b52105..3608db4 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh

@@ -278,4 +278,43 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' '
 	test_cmp expect-conflict-p out
 '
 
+test_expect_success 'stop rebase --root on empty root log message' '
+	# create a root commit with a non-empty tree so that rebase does
+	# not fail because of an empty commit, and an empty log message
+	echo root-commit >file &&
+	git add file &&
+	tree=$(git write-tree) &&
+	root=$(git commit-tree $tree </dev/null) &&
+	git checkout -b no-message-root-commit $root &&
+	# do not ff because otherwise neither the patch nor the message
+	# are looked at and checked for emptiness
+	test_must_fail env EDITOR=true git rebase -i --force-rebase --root &&
+	echo root-commit >file.expected &&
+	test_cmp file file.expected &&
+	git rebase --abort
+'
+
+test_expect_success 'stop rebase --root on empty child log message' '
+	# create a root commit with a non-empty tree and provide a log
+	# message so that rebase does not fail until the root commit is
+	# successfully replayed
+	echo root-commit >file &&
+	git add file &&
+	tree=$(git write-tree) &&
+	root=$(git commit-tree $tree -m root-commit) &&
+	git checkout -b no-message-child-commit $root &&
+	# create a child commit with a non-empty patch so that rebase
+	# does not fail because of an empty commit, but an empty log
+	# message
+	echo child-commit >file &&
+	git add file &&
+	git commit --allow-empty-message --no-edit &&
+	# do not ff because otherwise neither the patch nor the message
+	# are looked at and checked for emptiness
+	test_must_fail env EDITOR=true git rebase -i --force-rebase --root &&
+	echo child-commit >file.expected &&
+	test_cmp file file.expected &&
+	git rebase --abort
+'
+
 test_done

-- 
2.0.0

On Wed, Jun 18, 2014 at 11:28 PM, Fabian Ruch [off-list ref] wrote:

It is customary, and provides nicer diagnostic output upon failure, to
have the "expected" file mentioned first:

    test_cmp file.expected file &&

+       git rebase --abort

Do you want to place this under control of test_when_finished
somewhere above the "git rebase" invocation to ensure cleanup if
something fails before this point?

+'
+
+test_expect_success 'stop rebase --root on empty child log message' '
+       # create a root commit with a non-empty tree and provide a log
+       # message so that rebase does not fail until the root commit is
+       # successfully replayed
+       echo root-commit >file &&
+       git add file &&
+       tree=$(git write-tree) &&
+       root=$(git commit-tree $tree -m root-commit) &&
+       git checkout -b no-message-child-commit $root &&
+       # create a child commit with a non-empty patch so that rebase
+       # does not fail because of an empty commit, but an empty log
+       # message
+       echo child-commit >file &&
+       git add file &&
+       git commit --allow-empty-message --no-edit &&
+       # do not ff because otherwise neither the patch nor the message
+       # are looked at and checked for emptiness
+       test_must_fail env EDITOR=true git rebase -i --force-rebase --root &&
+       echo child-commit >file.expected &&
+       test_cmp file file.expected &&
+       git rebase --abort

Same two comments as for previous test.

+'
+
 test_done
--
2.0.0

Hi Eric,

On 06/21/2014 02:33 AM, Eric Sunshine wrote:

On Wed, Jun 18, 2014 at 11:28 PM, Fabian Ruch [off-list ref] wrote:

quoted

When `rebase` is executed with `--root` but no `--onto` is specified,
`rebase` creates a sentinel commit which is replaced with the root
commit in three steps. This combination of options results never in a
fast-forward.

 1. The sentinel commit is forced into the authorship of the root
    commit.

 2. The changes introduced by the root commit are applied to the index
    but not committed. If this step fails for whatever reason, all
    commit information will be there and the user can safely run
    `git-commit --amend` after resolving the problems.

 3. The new root commit is created by squashing the changes into the
    sentinel commit which already carries the authorship of the
    cherry-picked root commit.

The command line used to create the commit in the third step specifies
effectless and erroneous options. Remove those.

 - `--allow-empty-message` is erroneous: If the root's commit message is
   empty, the rebase shall fail like for any other commit that is on the
   to-do list and has an empty commit message.

   Fix the bug that git-rebase does not fail when the initial commit has
   an empty log message but is replayed using `--root` is specified.
   Add test.

 - `-C` is effectless: The commit being amended, which is the sentinel
   commit, already carries the authorship and log message of the
   cherry-picked root commit. The committer email and commit date fields
   are reset either way.

After all, if step two fails, `rebase --continue` won't include these
flags in the git-commit command line either.

Signed-off-by: Fabian Ruch <redacted>
---
 git-rebase--interactive.sh |  4 ++--
 t/t3412-rebase-root.sh     | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index fffdfa5..f09eeae 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -539,8 +539,8 @@ do_pick () {
                git commit --allow-empty --allow-empty-message --amend \
                           --no-post-rewrite -n -q -C $1 &&
                        pick_one -n $1 &&
-                       git commit --allow-empty --allow-empty-message \
-                                  --amend --no-post-rewrite -n -q -C $1 \
+                       git commit --allow-empty --amend \
+                                  --no-post-rewrite -n -q \
                                   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
                        die_with_patch $1 "Could not apply $1... $2"
        else

diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 0b52105..3608db4 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh

@@ -278,4 +278,43 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' '
        test_cmp expect-conflict-p out
 '

+test_expect_success 'stop rebase --root on empty root log message' '
+       # create a root commit with a non-empty tree so that rebase does
+       # not fail because of an empty commit, and an empty log message
+       echo root-commit >file &&
+       git add file &&
+       tree=$(git write-tree) &&
+       root=$(git commit-tree $tree </dev/null) &&
+       git checkout -b no-message-root-commit $root &&
+       # do not ff because otherwise neither the patch nor the message
+       # are looked at and checked for emptiness
+       test_must_fail env EDITOR=true git rebase -i --force-rebase --root &&
+       echo root-commit >file.expected &&
+       test_cmp file file.expected &&

It is customary, and provides nicer diagnostic output upon failure, to
have the "expected" file mentioned first:

    test_cmp file.expected file &&

Ok.

quoted

+       git rebase --abort

Do you want to place this under control of test_when_finished
somewhere above the "git rebase" invocation to ensure cleanup if
something fails before this point?

Thanks a lot for the hint, I will place the test_when_finished directly
above the rebase -i command line because it is not relevant before, the
exact position shouldn't matter though. I didn't know that test_run_
skips the cleanup code if -i (for immediate mode) is passed to the test
driver and the test case fails.

   Fabian

quoted

+'
+
+test_expect_success 'stop rebase --root on empty child log message' '
+       # create a root commit with a non-empty tree and provide a log
+       # message so that rebase does not fail until the root commit is
+       # successfully replayed
+       echo root-commit >file &&
+       git add file &&
+       tree=$(git write-tree) &&
+       root=$(git commit-tree $tree -m root-commit) &&
+       git checkout -b no-message-child-commit $root &&
+       # create a child commit with a non-empty patch so that rebase
+       # does not fail because of an empty commit, but an empty log
+       # message
+       echo child-commit >file &&
+       git add file &&
+       git commit --allow-empty-message --no-edit &&
+       # do not ff because otherwise neither the patch nor the message
+       # are looked at and checked for emptiness
+       test_must_fail env EDITOR=true git rebase -i --force-rebase --root &&
+       echo child-commit >file.expected &&
+       test_cmp file file.expected &&
+       git rebase --abort

Same two comments as for previous test.

Ok.

quoted

+'
+
 test_done