On Sun, Jan 10 2021, SZEDER Gábor wrote:
> 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.
