Should you use test_i18ngrep or GIT_TEST_GETTEXT_POISON=false?

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

On Tue, Jan 05, 2021 at 08:42:41PM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

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.