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). > diff --git a/t/t3430-rebase-empty-message.sh b/t/t3430-rebase-empty-message.sh > new file mode 100755 > index 000000000..74af73f3c > --- /dev/null > +++ b/t/t3430-rebase-empty-message.sh > @@ -0,0 +1,79 @@ > +#!/bin/sh > + > +test_description='rebase: option to allow empty commit messages' > + > +. ./test-lib.sh > +. "$TEST_DIRECTORY"/lib-rebase.sh > + > +test_expect_success 'setup: three regular commits' ' > + test_commit A && > + test_commit B && > + test_commit C > +' 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
