Re: [PATCH] rebase: add --allow-empty-message option

Hi,

On Fri, 2 Feb 2018, Genki Sky wrote:

--> Motivation

I won't comment on the motivation (leaving that up to the Git maintainer),
but on the patch.

The patch on the shell scripts and the C code looks straight-forward
enough, if a little repetitive (but that is hardly your fault).

None of these commits have empty commit messages, which is a little
curious for a 'setup' test case.

+test_expect_success 'rebase -i "reword" should fail to create an empty commit message' '
+	set_fake_editor &&
+	test_must_fail env FAKE_COMMIT_MESSAGE=" " FAKE_LINES="1 reword 2" \
+		git rebase -i HEAD~2
+'

Why not make this more focused via

	... FAKE_LINES="reword 1" git rebase -i HEAD^

The effect will be the same because the first pick will be skipped as an
unnecessary pick anyway.

+test_expect_success '... but should succeed with --allow-empty-message' '
+	git rebase --abort &&

This should be part of the previous test case:

	test_when_finished "test_might_fail git rebase --abort" &&

Also, I think this test case should be folded into the previous test case
(which would make that test_when_finished suggestion moot).

+	set_fake_editor &&
+	FAKE_COMMIT_MESSAGE=" " FAKE_LINES="1 reword 2" \
+		git rebase -i --allow-empty-message HEAD~2
+'
+
+test_expect_success 'rebase -i "fixup" should fail to fixup an empty commit message' '
+	test_commit D &&
+	set_fake_editor &&
+	test_must_fail env FAKE_LINES="1 fixup 2" git rebase -i HEAD~2
+'
+
+test_expect_success '... but should succeed with --allow-empty-message' '
+	git rebase --abort &&
+	FAKE_LINES="1 fixup 2" git rebase -i --allow-empty-message HEAD~2
+'
+
+# The test expects the following rebase to fail. It will only fail if it
+# actually has to cmd_commit() a new [empty message] commit. However, this
+# rebase makes no actual changes.

Don't you want to use `--force-rebase` here?

So if the date does not change in time, it is
+# possible for it to simply take the original commits exactly as they are.
+# So, we test_tick() just to be safe.
+test_expect_success 'rebase --root should fail on an empty commit message' '
+	test_tick &&
+	test_must_fail git rebase --root
+'
+
+test_expect_success '... but should succeed with --allow-empty-message' '
+	git rebase --abort &&
+	git rebase --root --allow-empty-message
+'
+
+test_expect_success 'setup: multiple branches' '
+	git checkout -b branch-keep-empty HEAD^1 &&
+	echo E >E &&
+	git add E &&
+	git commit --allow-empty-message -m "" &&
+	git branch branch-merge
+'
+
+test_expect_success 'rebase --keep-empty should fail on an empty commit message' '
+	test_must_fail git rebase --keep-empty master branch-keep-empty
+'
+
+test_expect_success '... but should succeed with --allow-empty-message' '
+	git cherry-pick --abort &&
+	git rebase --keep-empty --allow-empty-message master branch-keep-empty
+'

I do not really see why we have to test --keep-empty here. The code paths
overlap with what was tested previously.

+test_expect_success 'rebase -m should fail on an empty commit message' '
+	test_must_fail git rebase -m master branch-merge
+'
+
+test_expect_success '... but should succeed with --allow-empty-message' '
+	git rebase --abort &&
+	git rebase -m --allow-empty-message master branch-merge
+'
+
+test_done

In general, I would much rather fold the test cases that verify the
behavior with --allow-empty-message into the same test case as verifying
the behavior without --allow-empty-message.

One of my aims in the Git project is to avoid test bloat. I know that
several other active contributors could not care less, and even the Git
maintainer seems to be oblivious to the danger of making a test suite so
unsuitable (both in terms of run-time and in terms of being able to
understand regressions) as to make it less useful. I certainly had painful
discussions on this very mailing list about that, and I still don't feel
heard nor understood.

While your patch certainly is clear enough to make it really easy to
understand regressions, I find that it errs on the side of over-testing.

And this is not an academic consideration. The test suite takes an insane
70-90 minutes *on a fast* Windows machine, even skipping all of the
git-svn tests. That's insane. (And the fact that Git is optimized for
Linux and runs the test suite much faster there is only a very feeble
excuse, if you want my opinion.)

The reason? It's death by a thousand only partially necessary tests.

In that light, I would really like to make at least new tests a lot more
focused, and I would really like it if newly-added tests would be
considerate of the time they take to run, and not only on Linux.

Thank you,
Johannes