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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 2, 2018, 20:52 UTC
Message-ID: <nycvar.QRO.7.76.6.1802022054290.35@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
In-Reply-To: <9d0414b869f21f38b81f92ee0619fd76410cbcfc.1517552392.git.sky@genki.is>

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).

Show 18 quoted lines

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

Show 5 quoted lines

> +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).

Show 19 quoted lines

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

Show 29 quoted lines

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

Show 10 quoted lines

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