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

On Thu, Nov 08, 2018 at 09:26:19PM +0100, Ævar Arnfjörð Bjarmason wrote:

On Fri, Nov 02 2018, SZEDER Gábor wrote:

quoted

quoted

 * 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/

quoted

quoted

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 "$@"
 }

quoted

 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.