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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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