On Sun, Jan 10 2021, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>
>> 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.
>
> For an issue like this "test_i18ngrep or not?" that would end up
> giving a useful hint to future developers, a follow-up commit with
> an explanation why test_i18ngrep is preferred (or not) would be a
> great way to go forward (if the "fix" is needed in the first place,
> that is). It is easy to add a patch to fix things up while a series
> is cooking in 'next', without having to remember doing so after the
> series hits 'master'.
>
>> 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.
>
> I did not immediately see why that commit changes any equation, but
> I am inclined to say that your argument makes sort-of sense, if not
> 100%, to me. POISON test is not about testing features that ought
> to work (that is what non-POISON tests are for). The primary
> objective of POISON test is to ensure that we didn't over-localize;
> if all the output from some-command is expected to be fully
> localized, between the above and
>
> GIT_TEST_GETTEXT_POISON=no git some-command >output &&
> grep "c locale string" output
>
> there wouldn't be much difference.
>
> But if the output contains strings, some of which are expected to be
> localized (e.g. human facing messages) and some are not (e.g.
> protocol dump), and the test output is inspected for both types of
> strings, it would not be an equivalent test.
*nod* we have a few tests like that, but most of it is "run git & do one
grep".
> Having said that, it may be OK that a mixed-output-command is tested
> only in C locale without localization. Our tests do *not* make sure
> that the strings that ought to be localized are indeed localized
> anyway, so as long as the inspection of the output string does not
> check for string that are *not* to be localized, we won't break the
> primary objective of having POISON tests.
What do you think about just removing it? I.e. make setting it a noop?
> In any case, if you want to push forward in that direction to use
> more GIT_TEST_* settings in the test to replace i18ncmp/grep, please
> make sure you propose something easier to read than "GETTEXT_POISON"
> for the environment variable name. It is overly long and makes the
> tests unreadable by pushing the part of the command that are more
> important off of the right edge of the screen.
Yeah, that sucks a bit. The common case though is that the "git"
invocation itself isn't reindented by more than a "\t". E.g. in my patch
upthread:
- test_must_fail git mktag <tag.sig 2>message &&
+ test_must_fail env GIT_TEST_GETTEXT_POISON=false \
+ git mktag <tag.sig 2>message &&
I wondered if I could support:
git --lang=C mktag [...]
But the order of argv parsing & gettext setup makes that a bit
inconvenient. The argv parsing itself emits i18n messages, it could be
done in the C code.
Also a pain to pass it over to the Perl + sh side without changing
semantics (simplest would be to setenv(LC_ALL, "C"), but then anything
downstream of us changes too.
Hrm, but I see I never added the "poison" support to the Perl side, just
the SH code.
Now I seem to recall that the last time I looked at this I punted on it
because I thought I'd just wait until the Shell built-ins needing i18n
disappeared, which makes any changes to it easier.
To me it makes sense just to use it. GETTEXT_POISON isn't *that* long of
a name, and any shortening-ing of it seems not worth it to e.g. break
shell history / needing to alter existing C settings etc. again.
It's a typical length for for the GIT_TEST_* variables, so if we think
their use makes tests too verbose it makes sense to re-visit that as a
more general topic.
But I also think it makes more sense to just get rid of it
entirely...
