tests: make GIT_TEST_GETTEXT_POISON a boolean

Change the GIT_TEST_GETTEXT_POISON variable from being "non-empty?" to
being a more standard boolean variable.

Since it needed to be checked in both C code and shellscript (via test
-n) it was one of the remaining shellscript-like variables. Now that
we have "env--helper" we can change that.

There's a couple of tricky edge cases that arise because we're using
git_env_bool() early, and the config-reading "env--helper".

If GIT_TEST_GETTEXT_POISON is set to an invalid value die_bad_number()
will die, but to do so it would usually call gettext(). Let's detect
the special case of GIT_TEST_GETTEXT_POISON and always emit that
message in the C locale, lest we infinitely loop.

As seen in the updated tests in t0017-env-helper.sh there's also a
caveat related to "env--helper" needing to read the config for trace2
purposes.

Since the C_LOCALE_OUTPUT prerequisite is lazy and relies on
"env--helper" we could get invalid results if we failed to read the
config (e.g. because we'd loop on includes) when combined with
e.g. "test_i18ngrep" wanting to check with "env--helper" if
GIT_TEST_GETTEXT_POISON was true or not.

I'm crossing my fingers and hoping that a test similar to the one I
removed in the earlier "config tests: simplify include cycle test"
change in this series won't happen again, and testing for this
explicitly in "env--helper"'s own tests.

This change breaks existing uses of
e.g. GIT_TEST_GETTEXT_POISON=YesPlease, which we've documented in
po/README and other places. As noted in [1] we might want to consider
also accepting "YesPlease" in "env--helper" as a special-case.

But as the lack of uproar over 6cdccfce1e ("i18n: make GETTEXT_POISON
a runtime option", 2018-11-08) demonstrates the audience for this
option is a really narrow set of git developers, who shouldn't have
much trouble modifying their test scripts, so I think it's better to
deal with that minor headache now and make all the relevant GIT_TEST_*
variables boolean in the same way than carry the "YesPlease"
special-case forward.

1. https://public-inbox.org/git/xmqqtvckm3h8.fsf@gitster-ct.c.googlers.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/ci/lib.sh b/ci/lib.sh
index 288a5b3884ad825c99601ba5dc62ee81c62d7d64..fd799ae66311d62d2eb62b7fe5aaf23171312d15 100755 (executable)
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -184,7 +184,7 @@ osx-clang|osx-gcc)
        export GIT_SKIP_TESTS="t9810 t9816"
        ;;
 GIT_TEST_GETTEXT_POISON)
-       export GIT_TEST_GETTEXT_POISON=YesPlease
+       export GIT_TEST_GETTEXT_POISON=true
        ;;
 esac
 

diff --git a/config.c b/config.c
index 374cb33005007fb2d9f4d15ea21dbd39e4219e4e..b985d60fa4f89506f29d4dfd29f0b8b956588058 100644 (file)
--- a/config.c
+++ b/config.c
@@ -956,6 +956,15 @@ static void die_bad_number(const char *name, const char *value)
        if (!value)
                value = "";
 
+       if (!strcmp(name, "GIT_TEST_GETTEXT_POISON"))
+               /*
+                * We explicitly *don't* use _() here since it would
+                * cause an infinite loop with _() needing to call
+                * use_gettext_poison(). This is why marked up
+                * translations with N_() above.
+                */
+               die(bad_numeric, value, name, error_type);
+
        if (!(cf && cf->name))
                die(_(bad_numeric), value, name, _(error_type));
 

diff --git a/gettext.c b/gettext.c
index d4021d690c07237edefb0cf2869eb6795d227a54..5c71f4c8b93d0482941833c61567669e7ed8624f 100644 (file)
--- a/gettext.c
+++ b/gettext.c
@@ -50,10 +50,8 @@ const char *get_preferred_languages(void)
 int use_gettext_poison(void)
 {
        static int poison_requested = -1;
-       if (poison_requested == -1) {
-               const char *v = getenv("GIT_TEST_GETTEXT_POISON");
-               poison_requested = v && strlen(v) ? 1 : 0;
-       }
+       if (poison_requested == -1)
+               poison_requested = git_env_bool("GIT_TEST_GETTEXT_POISON", 0);
        return poison_requested;
 }
 

diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
index e1d917fd2796d1f2d952c509cb79d8cafe0cf89e..8eef60b43fd47a4a679c5c77ae2f9c5ab5907951 100644 (file)
--- a/git-sh-i18n.sh
+++ b/git-sh-i18n.sh
@@ -17,7 +17,9 @@ export TEXTDOMAINDIR
 
 # First decide what scheme to use...
 GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough
-if test -n "$GIT_TEST_GETTEXT_POISON"
+if test -n "$GIT_TEST_GETTEXT_POISON" &&
+           git env--helper --type=bool --default=0 --exit-code \
+               GIT_TEST_GETTEXT_POISON
 then
        GIT_INTERNAL_GETTEXT_SH_SCHEME=poison
 elif test -n "@@USE_GETTEXT_SCHEME@@"

diff --git a/po/README b/po/README
index aa704ffcb7f70af2c42494a2f26eeef17c5eed67..07595d369b0ab3cd3a90cef74cae81661f9cb95f 100644 (file)
--- a/po/README
+++ b/po/README
@@ -293,7 +293,7 @@ To smoke out issues like these, Git tested with a translation mode that
 emits gibberish on every call to gettext. To use it run the test suite
 with it, e.g.:
 
-    cd t && GIT_TEST_GETTEXT_POISON=YesPlease prove -j 9 ./t[0-9]*.sh
+    cd t && GIT_TEST_GETTEXT_POISON=true prove -j 9 ./t[0-9]*.sh
 
 If tests break with it you should inspect them manually and see if
 what you're translating is sane, i.e. that you're not translating

diff --git a/t/README b/t/README
index 9747971d58e1a5efaa85a8ce00772c82d7c5aba7..9a131f472e0ad8cf03a4c4c9aa6ffeabc03c7c9d 100644 (file)
--- a/t/README
+++ b/t/README
@@ -343,8 +343,8 @@ whether this mode is active, and e.g. skip some tests that are hard to
 refactor to deal with it. The "SYMLINKS" prerequisite is currently
 excluded as so much relies on it, but this might change in the future.
 
-GIT_TEST_GETTEXT_POISON=<non-empty?> turns all strings marked for
-translation into gibberish if non-empty (think "test -n"). Used for
+GIT_TEST_GETTEXT_POISON=<boolean> turns all strings marked for
+translation into gibberish if true. Used for
 spotting those tests that need to be marked with a C_LOCALE_OUTPUT
 prerequisite when adding more strings for translation. See "Testing
 marked strings" in po/README for details.

diff --git a/t/t0017-env-helper.sh b/t/t0017-env-helper.sh
index 709bbbd275491fbd45baac480a6a61e70a2e759f..c1ecf6aeac672056613c558fd27daaf6337ac1a6 100755 (executable)
--- a/t/t0017-env-helper.sh
+++ b/t/t0017-env-helper.sh
@@ -80,4 +80,20 @@ test_expect_success 'env--helper --type=ulong' '
        test_must_be_empty actual.err
 '
 
+test_expect_success 'env--helper reads config thanks to trace2' '
+       mkdir home &&
+       git config -f home/.gitconfig include.path cycle &&
+       git config -f home/cycle include.path .gitconfig &&
+
+       test_must_fail \
+               env HOME="$(pwd)/home" GIT_TEST_GETTEXT_POISON=false \
+               git config -l 2>err &&
+       grep "exceeded maximum include depth" err &&
+
+       test_must_fail \
+               env HOME="$(pwd)/home" GIT_TEST_GETTEXT_POISON=true \
+               git -C cycle env--helper --type=bool --default=0 --exit-code GIT_TEST_GETTEXT_POISON 2>err &&
+       grep "# GETTEXT POISON #" err
+'
+
 test_done

diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh
index a06269f38ae555f952b83b6fbc38374a725293d1..f9fa16ad8363a9e1c8611f37b5945f43ee2053c9 100755 (executable)
--- a/t/t0205-gettext-poison.sh
+++ b/t/t0205-gettext-poison.sh
@@ -5,7 +5,7 @@
 
 test_description='Gettext Shell poison'
 
-GIT_TEST_GETTEXT_POISON=YesPlease
+GIT_TEST_GETTEXT_POISON=true
 export GIT_TEST_GETTEXT_POISON
 . ./lib-gettext.sh
 
@@ -31,4 +31,9 @@ test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semant
     test_cmp expect actual
 '
 
+test_expect_success "gettext: invalid GIT_TEST_GETTEXT_POISON value doesn't infinitely loop" "
+       test_must_fail env GIT_TEST_GETTEXT_POISON=xyz git version 2>error &&
+       grep \"fatal: bad numeric config value 'xyz' for 'GIT_TEST_GETTEXT_POISON': invalid unit\" error
+"
+
 test_done

diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index 6b388ba2d047935c79d517bed75341b030860e13..de294c990e19b4aec8b6f4bed1c34518adeda79f 100755 (executable)
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -314,7 +314,7 @@ test_expect_success 'include cycles are detected' '
        git -C cycle config include.path cycle &&
        git config -f cycle/cycle include.path config &&
        test_must_fail \
-               env GIT_TEST_GETTEXT_POISON= \
+               env GIT_TEST_GETTEXT_POISON=false \
                git -C cycle config --get-all test.value 2>stderr &&
        grep "exceeded maximum include depth" stderr
 '

diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 5990299fc9555d54a59a5de462864a7a98e83e45..b696bae5f534e82609f8dd0d27e6f9abb85cd7cc 100755 (executable)
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -249,7 +249,7 @@ test_expect_success 'checkout to detach HEAD (with advice declined)' '
 test_expect_success 'checkout to detach HEAD' '
        git config advice.detachedHead true &&
        git checkout -f renamer && git clean -f &&
-       GIT_TEST_GETTEXT_POISON= git checkout renamer^ 2>messages &&
+       GIT_TEST_GETTEXT_POISON=false git checkout renamer^ 2>messages &&
        grep "HEAD is now at 7329388" messages &&
        test_line_count -gt 1 messages &&
        H=$(git rev-parse --verify HEAD) &&

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 43cf313a1c09ad8fb223ba775d5548c3f00c339c..75512c340366f3034c58effb62c8796d0b1463a8 100755 (executable)
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1706,7 +1706,7 @@ test_expect_success 'sourcing the completion script clears cached commands' '
 '
 
 test_expect_success 'sourcing the completion script clears cached merge strategies' '
-       GIT_TEST_GETTEXT_POISON= &&
+       GIT_TEST_GETTEXT_POISON=false &&
        __git_compute_merge_strategies &&
        verbose test -n "$__git_merge_strategies" &&
        . "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4b346467df7aaf2e5215f7ebb3bfb4ae13a3ffbc..ed5d69dfe5144fe7a6c85ad2541ba44b0e530f5c 100644 (file)
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1443,11 +1443,9 @@ then
        unset GIT_TEST_GETTEXT_POISON_ORIG
 fi
 
-# Can we rely on git's output in the C locale?
-if test -z "$GIT_TEST_GETTEXT_POISON"
-then
-       test_set_prereq C_LOCALE_OUTPUT
-fi
+test_lazy_prereq C_LOCALE_OUTPUT '
+       ! git env--helper --type=bool --default=0 --exit-code GIT_TEST_GETTEXT_POISON
+'
 
 if test -z "$GIT_TEST_CHECK_CACHE_TREE"
 then