Re: [PATCH 3/3] commit: add an option the reword HEAD

Hi Junio

On 21/09/2020 20:27, Junio C Hamano wrote:

Phillip Wood [off-list ref] writes:

quoted

quoted

quoted

diff --git a/t/t7501-commit-basic-functionality.sh b/t/t7501-commit-basic-functionality.sh

@@ -713,4 +713,60 @@ test_expect_success '--dry-run --short' '
+test_reword_opt () {
+       test_expect_success C_LOCALE_OUTPUT "--reword incompatible with $1" "
+               echo 'fatal: cannot combine --reword with $1' >expect &&
+               test_must_fail git commit --reword $1 2>actual &&
+               test_cmp expect actual
+       "
+}

These error messages are subject to localization, so you'd want to
use
test_i18ncmp() here, I think.
Same comment for other new tests.

I decided to use the C_LOCALE_OUTPUT prerequisite and test_cmp rather
than grep so I could check the exact output.

I do not think it is a good idea.  Dropping the C_LOCALE_OUTPUT
prerequisite and using test_i18ncmpw would be more appropriate.

A test run without GIT_TEST_GETTEXT_POISON will do the byte-for-byte
comparison like test_cmp.  It is only the poison test, whose purpose
is to catch commands that by mistake translated their messages, that
would want to mark a test that checks end-user facing messages like
this one as special with test_i18ncmp.

Thanks I wasn't aware of test_i18ncmp

quoted

... I should probably check that nothing is printed to stdout in
these tests

Perhaps, but that is not the point of "do we diagnose options thare
are incompatble with --reword?" test.

I think it depends if one views the test as checking "do we diagnose 
options there are incompatible with --reword?" or "what do we show the 
user when there are options that are incompatible with --reword". For 
the former we just want to check that the correct error message is 
printed, for the latter we want to check that only what we expect to be 
printed is actually printed.

Best Wishes

Phillip