Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>>> 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.
