On Wed, Apr 7, 2010 at 05:16, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> diff --git a/t/t7510-commit-allow-empty-message.sh b/t/t7510-commit-allow-empty-message.sh >> new file mode 100755 >> index 0000000..d7dc0da > > I do not think a separate script is worth it. I'd just add tests to t7500. > >> +test_expect_success 'a basic commit in an empty tree should succeed' ' >> + ( >> + echo content > foo && >> + git add foo && >> + git commit -m "initial commit" >> + ) && >> + commit_msg_is "initial commit" >> +' > > No need for this, especially if this becomes part of t7500. > >> +test_expect_success 'Commit no message with --allow-empty-message' ' >> + ( >> + echo "more content" >> foo && >> + git add foo && >> + printf "" | git commit --allow-empty-message >> + ) && >> + commit_msg_is "" >> +' > > No need for subprocess, nor printf piped to the command. > >> +test_expect_success 'Commit a message with --allow-empty-message' ' >> + ( >> + echo "even more content" >> foo && >> + git add foo && >> + git commit --allow-empty-message -m"hello there" >> + ) && >> + commit_msg_is "hello there" >> +' > > Ditto. > > You are falling into the same trap as everybody else does when showing off > your shiny new toy. A missing but very necessary test is that "commit" > with your patch does still fail when an empty message is given without the > new option. > > It takes a conscious effort to carefully make sure that your shiny new toy > does not trigger when it shouldn't, especially when you are so excited by > seeing it work when it should. > > But that is part of the art of writing good test scripts. Thanks for shepherding the patch and committing it, and for pointing out what I could have done better. I'll try not to make similar mistakes with future submissions. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html