git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3 5/8] tests: make GIT_TEST_GETTEXT_POISON a boolean

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Jun 21, 2019, 10:18 UTC
Message-ID
<20190621101812.27300-6-avarab@gmail.com>
In-Reply-To
<20190620210915.11297-1-avarab@gmail.com>

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>
---
 ci/lib.sh                 |  2 +-
 config.c                  |  9 +++++++++
 gettext.c                 |  6 ++----
 git-sh-i18n.sh            |  4 +++-
 po/README                 |  2 +-
 t/README                  |  4 ++--
 t/t0017-env-helper.sh     | 16 ++++++++++++++++
 t/t0205-gettext-poison.sh |  7 ++++++-
 t/t1305-config-include.sh |  2 +-
 t/t7201-co.sh             |  2 +-
 t/t9902-completion.sh     |  2 +-
 t/test-lib.sh             |  8 +++-----
 12 files changed, 46 insertions(+), 18 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 288a5b3884..fd799ae663 100755
--- 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 374cb33005..b985d60fa4 100644
--- 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 d4021d690c..5c71f4c8b9 100644
--- 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 e1d917fd27..8eef60b43f 100644
--- 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 aa704ffcb7..07595d369b 100644
--- 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 9747971d58..9a131f472e 100644
--- 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 709bbbd275..c1ecf6aeac 100755
--- 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 a06269f38a..f9fa16ad83 100755
--- 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 6b388ba2d0..de294c990e 100755
--- 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 5990299fc9..b696bae5f5 100755
--- 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 43cf313a1c..75512c3403 100755
--- 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 4b346467df..ed5d69dfe5 100644
--- 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

-- 
2.22.0.455.g172b71a6c5
Previous: Ævar Arnfjörð BjarmasonNext: Junio C Hamano
Message 19 of 61 in “fetch: only run 'gc' once when fetching multiple remotes”
  1. fetch: only run 'gc' once when fetching multiple remotesNguyễn Thái Ngọc Duy, Jun 19, 2019
  2. gc: run more pre-detach operations under lockÆvar Arnfjörð Bjarmason, Jun 19, 2019
  3. Duy NguyenJun 19, 2019
  4. Ævar Arnfjörð BjarmasonJun 19, 2019
  5. Jeff KingJun 19, 2019
  6. Ævar Arnfjörð BjarmasonJun 19, 2019
  7. 0/6 Change <non-empty?> GIT_TEST_* variables to <boolean>Ævar Arnfjörð Bjarmason, Jun 19, 2019
  8. Junio C HamanoJun 20, 2019
  9. Ævar Arnfjörð BjarmasonJun 20, 2019
  10. Junio C HamanoJun 20, 2019
  11. 0/8 Change <non-empty?> GIT_TEST_* variables to <boolean>Ævar Arnfjörð Bjarmason, Jun 20, 2019
  12. 1/8 config tests: simplify include cycle testÆvar Arnfjörð Bjarmason, Jun 21, 2019
  13. 0/8 Change <non-empty?> GIT_TEST_* variables to <boolean>Ævar Arnfjörð Bjarmason, Jun 21, 2019
  14. 2/8 env--helper: new undocumented builtin wrapping git_env_*()Ævar Arnfjörð Bjarmason, Jun 21, 2019
  15. Junio C HamanoJun 21, 2019
  16. 3/8 config.c: refactor die_bad_number() to not call gettext() earlyÆvar Arnfjörð Bjarmason, Jun 21, 2019
  17. 4/8 t6040 test: stop using global "script" variableÆvar Arnfjörð Bjarmason, Jun 21, 2019
  18. 6/8 tests README: re-flow a previously changed paragraphÆvar Arnfjörð Bjarmason, Jun 21, 2019
  19. 5/8 tests: make GIT_TEST_GETTEXT_POISON a booleanÆvar Arnfjörð Bjarmason, Jun 21, 2019
  20. Junio C HamanoJun 24, 2019
  21. 7/8 tests: replace test_tristate with "git env--helper"Ævar Arnfjörð Bjarmason, Jun 21, 2019
  22. 1/2 t/lib-git-svn.sh: check GIT_TEST_SVN_HTTPD when running SVN HTTP testsSZEDER Gábor, Sep 6, 2019
  23. 2/2 ci: restore running httpd testsSZEDER Gábor, Sep 6, 2019
  24. Junio C HamanoSep 6, 2019
  25. Jeff KingSep 6, 2019
  26. SZEDER GáborSep 7, 2019
  27. 0/2 tests: catch non-bool GIT_TEST_* valuesSZEDER Gábor, Nov 22, 2019
  28. 1/2 tests: add 'test_bool_env' to catch non-bool GIT_TEST_* valuesSZEDER Gábor, Nov 22, 2019
  29. Jeff KingNov 25, 2019
  30. 2/2 t5608-clone-2gb.sh: turn GIT_TEST_CLONE_2GB into a boolSZEDER Gábor, Nov 22, 2019
  31. Jeff KingNov 25, 2019
  32. 8/8 tests: make GIT_TEST_FAIL_PREREQS a booleanÆvar Arnfjörð Bjarmason, Jun 21, 2019
  33. 1/8 config tests: simplify include cycle testÆvar Arnfjörð Bjarmason, Jun 20, 2019
  34. 2/8 env--helper: new undocumented builtin wrapping git_env_*()Ævar Arnfjörð Bjarmason, Jun 20, 2019
  35. Junio C HamanoJun 20, 2019
  36. Junio C HamanoJun 20, 2019
  37. Ævar Arnfjörð BjarmasonJun 21, 2019
  38. Junio C HamanoJun 21, 2019
  39. 3/8 config.c: refactor die_bad_number() to not call gettext() earlyÆvar Arnfjörð Bjarmason, Jun 20, 2019
  40. 4/8 t6040 test: stop using global "script" variableÆvar Arnfjörð Bjarmason, Jun 20, 2019
  41. 5/8 tests: make GIT_TEST_GETTEXT_POISON a booleanÆvar Arnfjörð Bjarmason, Jun 20, 2019
  42. 7/8 tests: replace test_tristate with "git env--helper"Ævar Arnfjörð Bjarmason, Jun 20, 2019
  43. 6/8 tests README: re-flow a previously changed paragraphÆvar Arnfjörð Bjarmason, Jun 20, 2019
  44. 8/8 tests: make GIT_TEST_FAIL_PREREQS a booleanÆvar Arnfjörð Bjarmason, Jun 20, 2019
  45. 1/6 env--helper: new undocumented builtin wrapping git_env_*()Ævar Arnfjörð Bjarmason, Jun 19, 2019
  46. Junio C HamanoJun 20, 2019
  47. 2/6 t6040 test: stop using global "script" variableÆvar Arnfjörð Bjarmason, Jun 19, 2019
  48. Junio C HamanoJun 20, 2019
  49. 3/6 tests: make GIT_TEST_GETTEXT_POISON a booleanÆvar Arnfjörð Bjarmason, Jun 19, 2019
  50. Junio C HamanoJun 20, 2019
  51. 5/6 tests: replace test_tristate with "git env--helper"Ævar Arnfjörð Bjarmason, Jun 19, 2019
  52. 4/6 tests README: re-flow a previously changed paragraphÆvar Arnfjörð Bjarmason, Jun 19, 2019
  53. 6/6 tests: make GIT_TEST_FAIL_PREREQS a booleanÆvar Arnfjörð Bjarmason, Jun 19, 2019
  54. Duy NguyenJun 20, 2019
  55. Ævar Arnfjörð BjarmasonJun 20, 2019
  56. Jeff KingJun 20, 2019
  57. Junio C HamanoJun 20, 2019
  58. Jeff KingJun 19, 2019
  59. Jeff KingJun 19, 2019
  60. Duy NguyenJun 20, 2019
  61. Jeff KingJun 20, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.