Re: [PATCH] config.c: remove last remnant of GIT_TEST_GETTEXT_POISON

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

quoted

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index e0dd5d65ced..2280c2504ac 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh

@@ -679,7 +679,7 @@ test_expect_success 'invalid unit boolean' '
 	git config commit.gpgsign "1true" &&
 	test_cmp_config 1true commit.gpgsign &&
 	test_must_fail git config --bool --get commit.gpgsign 2>actual &&
-	test_i18ngrep "bad boolean config value .1true. for .commit.gpgsign." actual
+	grep "bad boolean config value .1true. for .commit.gpgsign." actual
 '

why are we losing test_i18ngrep here? The message is still marked for
translation. I know we've discussed dropping all of the test_i18n
helpers, but that seems unrelated to the rest of the patch.

For new tests we're suggesting not to use it, so while I'm holding off
on some general s/test_i18ngrep/grep/ refactoring, it seemed natural to
adjust the test added by the commit whose code I'm modifying.

It would have been understandable if the proposed log message said

    Remove a use of GIT_TEST_GETTEXT_POISON added in f276e2a4694 (config:
    improve error message for boolean config, 2021-02-11), together with
    a new test added.

and removed the test.

But the test still has value for the remaining codebase, just like
all the other tests that happen to use test_i18n{grep,cmp}.  In a
sense, the original mixed two separate things into one commit
(i.e. use of TEST_GETTEXT_POISON to decide if the message given to
die() is localized, and a test to see how "git config --bool --get"
behaves when a malformed boolean value is given), and that may have
been justifiable back in the world where GETTEXT_POISON was a thing,
making these two things closely interrelated.  Since we left that
world behind, I think we should treat them as two separate things.

In short, I do not think "The C code we are removing was added in
the same commit" is a good excuse for this "while at it" change.

Putting it another way, imagine back then there was the t1300 test
and there was no die_bad_bool().  The test may have been expecting
"bad numeric" or "invalid unit", with grep (with a known bug that
the test would not pass under GETTEXT_POISON).

In such an alternative past, the change you are reverting may have
been only to config.c to make the die(_()) work correctly with
GETTEXT_POISON, and turned grep to test_i18ngrep.  And your "we are
reverting the whole commit" may have made more sense.

But we are not living in such an alternative world.

Having said all that, I do not particularly care when, in which
exact commit, a use of test_i18n* in t/ among 1100+ of them lost
its i18n-ness (it just felt a bit out of place in this particular
commit, that's all).

Thanks.