Should you use test_i18ngrep or GIT_TEST_GETTEXT_POISON=false?

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 10, 2021, 18:59 UTC
Message-ID: <87y2h062jd.fsf@evledraar.gmail.com>
In-Reply-To: <20210110132155.GT8396@szeder.dev>

On Sun, Jan 10 2021, SZEDER Gábor wrote:

Show 23 quoted lines

> On Tue, Jan 05, 2021 at 08:42:41PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> The verify_object() function in "mktag.c" is tasked with ensuring that
>> our tag refers to a valid object.
>> 
>> The existing test for this might fail because it was also testing that
>> "type taggg" didn't refer to a valid object type (it should be "type
>> tag"), or because we referred to a valid object but got the type
>> wrong.
>> 
>> Let's split these tests up, so we're testing all combinations of a
>> non-existing object and in invalid/wrong "type" lines.
>> 
>> We need to provide GIT_TEST_GETTEXT_POISON=false here because the
>> "invalid object type" error is emitted by
>> parse_loose_header_extended(), which has that message already marked
>> for translation. Another option would be to use test_i18ngrep, but I
>> prefer always running the test, not skipping it under gettext poison
>> testing.
>
> This is fairly unconvincing.  Why do you prefer it that way?  What is
> so special in these tests of 'git mktags' that could possibly warrant
> this special treatment WRT gettext poisoning, as opposed to the other
> ~1500 invocations of test_i18n{grep,cmp}?

You prodded me about this and Johannes also did off-list. So given that this is already in "next" I think it's best to let usage in this mktag series land as-is.

I promise I'll follow-up with something to (at least start to) fix this generally one way or the other on "master" after that. It's a trivial part of this particular series.

I.e. either document & migrate away from test_i18ngrep (mostly, in some cases it's still convenient), or git rid of this and the few existing GIT_TEST_GETTEXT_POISON=false uses.

Anyway, as both the initial author of this poison facility & of its current runtime incarnation in git.git, some context on it:

The need for test_i18ngrep was always a wart. When I submitted the initial i18n patches a big selling point at the time to a (rightly so) skeptical audience on the Git ML was that it could almost entirely be turned off at compile-time, and that we had something in place to make sure we didn't introduce plumbing bugs by overtranslating.

For a while the mass-i18n patches were about as noisy on-list as the sha256 conversion patches were in more recenty memory.

So if you look through the history you'll see that first we skipped whole tests with the C_LOCALE_OUTPUT prereq, and then test_i18ngrep was a more granular way to do that:

    git some-command >output && 
    test_i18ngrep "c locale string" output

But since 6cdccfce1e0 (i18n: make GETTEXT_POISON a runtime option, 2018-11-08) this hasn't been needed. But yes, we have a bit under 10 years of test_i18ngrep usage in the test suite now.

But that stuff is the wart, and purely a historical artifact of this having once been compile-time only. We don't do this sort of thing with any other GIT_TEST_* option, e.g. we have tests like:

    t/t5512-ls-remote.sh:   GIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref --heads . >actual &&
    t/t5512-ls-remote.sh-   test_cmp expect actual &&

Not:

    t/t5512-ls-remote.sh:   git ls-remote --symref --heads . >actual &&
    t/t5512-ls-remote.sh-   test_protocolv0cmp expect actual &&

Because that doesn't make sense. If we know we're testing protocol v0 here why wouldn't we just pass that as a parameter in the test? Same for GIT_TEST_GETTEXT_POISON=false.

But yes. looking at e.g. po/README now I dropped the ball on following through with some documentation etc. about this at the time.

I have also wondered if a better approach at this point isn't to just remove the thing entirely. We're just doing incremental i18n-ization at this point, so the odds that it's catching accidental plumbing translations is rather low compared to 2011-era git.git.