Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Thu, Nov 08, 2018 at 09:26:19PM +0100, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Nov 02 2018, SZEDER Gábor wrote:

> >>  * We error out in the Makefile if you're still saying
> >>    GETTEXT_POISON=YesPlease.
> >>
> >>    This makes more sense than just making it a synonym since now this
> >>    also needs to be defined at runtime.
> >
> > OK, I think erroring out is better than silently ignoring
> > GETTEXT_POISON=YesPlease.  However, the commit message only mentions
> > that GETTEXT_POISON is gone, but perhaps this should be mentioned
> > there as well.
> 
> Will mention.

With the benefit of hindsight gained from looking into a failing
GETTEXT_POISON build [1], I have to agree with Junio that flat-out
erroring out when GETTEXT_POISON=YesPlease is set is really a hassle
[2].

[1] https://public-inbox.org/git/20181107130950.GA30222@szeder.dev/
    (the failure is not related to this patch)
[2] https://public-inbox.org/git/xmqqpnvg8d5z.fsf@gitster-ct.c.googlers.com/

> >> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> >> index 78d8c3783b..2f42b3653c 100644
> >> --- a/t/test-lib-functions.sh
> >> +++ b/t/test-lib-functions.sh
> >> @@ -755,16 +755,16 @@ test_cmp_bin() {
> >>
> >>  # Use this instead of test_cmp to compare files that contain expected and
> >>  # actual output from git commands that can be translated.  When running
> >> -# under GETTEXT_POISON this pretends that the command produced expected
> >> +# under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected
> >>  # results.
> >>  test_i18ncmp () {
> >> -	test -n "$GETTEXT_POISON" || test_cmp "$@"
> >> +	! test_have_prereq C_LOCALE_OUTPUT || test_cmp "$@"
> >>  }
> >
> >>  test_i18ngrep () {
> >>  	eval "last_arg=\${$#}"
> >> @@ -779,7 +779,7 @@ test_i18ngrep () {
> >>  		error "bug in the test script: too few parameters to test_i18ngrep"
> >>  	fi
> >>
> >> -	if test -n "$GETTEXT_POISON"
> >> +	if test_have_prereq !C_LOCALE_OUTPUT
> >
> > Why do these two helper functions call test_have_prereq (a function
> > that doesn't even fit on my screen) instead of a simple
> >
> >   test -n "$GIT_TEST_GETTEXT_POISON"
> 
> I'm going to keep the call to test_have_prereq, it's consistent with
> other use of the helper in the test lib, and doesn't confuse the reader
> by giving the impression that we're in some really early setup where we
> haven't set the prereq at this point (we have).

Using the prereq in the first place is what confused (and is still
confusing...) the reader.