Re: Should you use test_i18ngrep or GIT_TEST_GETTEXT_POISON=false?

On Sun, Jan 10 2021, Junio C Hamano wrote:

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

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'.

quoted

    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...