@matheusbsbs / matheus-bagliano / commits / 6cdccfce1e

i18n: make GETTEXT_POISON a runtime option

Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON test parameter to only be a GIT_TEST_GETTEXT_POISON=<non-empty?> runtime parameter, to be consistent with other parameters documented in "Running tests with special setups" in t/README. When I added GETTEXT_POISON in bb946bba76 ("i18n: add GETTEXT_POISON to simulate unfriendly translator", 2011-02-22) I was concerned with ensuring that the _() function would get constant folded if NO_GETTEXT was defined, and likewise that GETTEXT_POISON would be compiled out unless it was defined. But as the benchmark in my [1] shows doing a one-off runtime getenv("GIT_TEST_[...]") is trivial, and since GETTEXT_POISON was originally added the GIT_TEST_* env variables have become the common idiom for turning on special test setups. So change GETTEXT_POISON to work the same way. Now the GETTEXT_POISON=YesPlease compile-time option is gone, and running the tests with GIT_TEST_GETTEXT_POISON=[YesPlease|] can be toggled on/off without recompiling. This allows for conditionally amending tests to test with/without poison, similar to what 859fdc0c3c ("commit-graph: define GIT_TEST_COMMIT_GRAPH", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH. Do some of that, now we e.g. always run the t0205-gettext-poison.sh test. I did enough there to remove the GETTEXT_POISON prerequisite, but its inverse C_LOCALE_OUTPUT is still around, and surely some tests using it can be converted to e.g. always set GIT_TEST_GETTEXT_POISON=. Notes on the implementation: * We still compile a dedicated GETTEXT_POISON build in Travis CI. Perhaps this should be revisited and integrated into the "linux-gcc" build, see ae59a4e44f ("travis: run tests with GIT_TEST_SPLIT_INDEX", 2018-01-07) for prior art in that area. Then again maybe not, see [2]. * We now skip a test in t0000-basic.sh under GIT_TEST_GETTEXT_POISON=YesPlease that wasn't skipped before. This test relies on C locale output, but due to an edge case in how the previous implementation of GETTEXT_POISON worked (reading it from GIT-BUILD-OPTIONS) wasn't enabling poison correctly. Now it does, and needs to be skipped. * The getenv() function is not reentrant, so out of paranoia about code of the form: printf(_("%s"), getenv("some-env")); call use_gettext_poison() in our early setup in git_setup_gettext() so we populate the "poison_requested" variable in a codepath that's won't suffer from that race condition. * We error out in the Makefile if you're still saying GETTEXT_POISON=YesPlease to prompt users to change their invocation. * We should not print out poisoned messages during the test initialization itself to keep it more readable, so the test library hides the variable if set in $GIT_TEST_GETTEXT_POISON_ORIG during setup. See [3]. See also [4] for more on the motivation behind this patch, and the history of the GETTEXT_POISON facility. 1. https://public-inbox.org/git/871s8gd32p.fsf@evledraar.gmail.com/ 2. https://public-inbox.org/git/20181102163725.GY30222@szeder.dev/ 3. https://public-inbox.org/git/20181022202241.18629-2-szeder.dev@gmail.com/ 4. https://public-inbox.org/git/878t2pd6yu.fsf@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Nov 8, 2018 at 21:15 UTC 6cdccfce1e0f218d4d26292b85ec18ed96c91be7

16 files changed +59 -49

.travis.yml

+1 -1

@@ -26,7 +26,7 @@ addons:

27 matrix:

28 include:

29 - - env: jobname=GETTEXT_POISON

29 + - env: jobname=GIT_TEST_GETTEXT_POISON

30 os: linux

31 compiler:

32 addons:

Makefile

+1 -7

@@ -362,11 +362,6 @@ all::

362 # Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the

363 # user.

364 #

365 -# Define GETTEXT_POISON if you are debugging the choice of strings marked

366 -# for translation. In a GETTEXT_POISON build, you can turn all strings marked

367 -# for translation into gibberish by setting the GIT_GETTEXT_POISON variable

368 -# (to any value) in your environment.

369 -#

365 # Define JSMIN to point to JavaScript minifier that functions as

366 # a filter to have gitweb.js minified.

367 #

@@ -1452,7 +1447,7 @@ ifdef NO_SYMLINK_HEAD

1447 BASIC_CFLAGS += -DNO_SYMLINK_HEAD

1448 endif

1449 ifdef GETTEXT_POISON

1455 - BASIC_CFLAGS += -DGETTEXT_POISON

1450 +$(error The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!)

1451 endif

1452 ifdef NO_GETTEXT

1453 BASIC_CFLAGS += -DNO_GETTEXT

@@ -2603,7 +2598,6 @@ ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT

2598 @echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@+

2599 endif

2600 @echo NO_GETTEXT=\''$(subst ','\'',$(subst ','\'',$(NO_GETTEXT)))'\' >>$@+

2606 - @echo GETTEXT_POISON=\''$(subst ','\'',$(subst ','\'',$(GETTEXT_POISON)))'\' >>$@+

2601 ifdef GIT_PERF_REPEAT_COUNT

2602 @echo GIT_PERF_REPEAT_COUNT=\''$(subst ','\'',$(subst ','\'',$(GIT_PERF_REPEAT_COUNT)))'\' >>$@+

2603 endif

ci/lib-travisci.sh

+2 -2

@@ -123,7 +123,7 @@ osx-clang|osx-gcc)

123 # Travis CI OS X

124 export GIT_SKIP_TESTS="t9810 t9816"

125 ;;

126 -GETTEXT_POISON)

127 - export GETTEXT_POISON=YesPlease

126 +GIT_TEST_GETTEXT_POISON)

127 + export GIT_TEST_GETTEXT_POISON=YesPlease

128 ;;

129 esac

gettext.c

+7 -4

@@ -7,6 +7,7 @@

7 #include "gettext.h"

8 #include "strbuf.h"

9 #include "utf8.h"

10 +#include "config.h"

12 #ifndef NO_GETTEXT

13 # include <locale.h>

@@ -46,15 +47,15 @@ const char *get_preferred_languages(void)

47 return NULL;

48 }

49 -#ifdef GETTEXT_POISON

50 int use_gettext_poison(void)

51 {

52 static int poison_requested = -1;

53 - if (poison_requested == -1)

54 - poison_requested = getenv("GIT_GETTEXT_POISON") ? 1 : 0;

53 + if (poison_requested == -1) {

54 + const char *v = getenv("GIT_TEST_GETTEXT_POISON");

55 + poison_requested = v && strlen(v) ? 1 : 0;

56 + }

57 return poison_requested;

58 }

57 -#endif

60 #ifndef NO_GETTEXT

61 static int test_vsnprintf(const char *fmt, ...)

@@ -164,6 +165,8 @@ void git_setup_gettext(void)

165 if (!podir)

166 podir = p = system_path(GIT_LOCALE_PATH);

167

168 + use_gettext_poison(); /* getenv() reentrancy paranoia */

169 +

170 if (!is_directory(podir)) {

171 free(p);

172 return;

gettext.h

+3 -6

@@ -28,12 +28,15 @@

29 #define FORMAT_PRESERVING(n) __attribute__((format_arg(n)))

31 +extern int use_gettext_poison(void);

32 +

33 #ifndef NO_GETTEXT

34 extern void git_setup_gettext(void);

35 extern int gettext_width(const char *s);

36 #else

37 static inline void git_setup_gettext(void)

38 {

39 + use_gettext_poison(); /* getenv() reentrancy paranoia */

40 }

41 static inline int gettext_width(const char *s)

42 {

@@ -41,12 +44,6 @@ static inline int gettext_width(const char *s)

44 }

45 #endif

44 -#ifdef GETTEXT_POISON

45 -extern int use_gettext_poison(void);

46 -#else

47 -#define use_gettext_poison() 0

48 -#endif

49 -

47 static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)

48 {

49 if (!*msgid)

git-sh-i18n.sh

+1 -1

@@ -17,7 +17,7 @@ export TEXTDOMAINDIR

18 # First decide what scheme to use...

19 GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough

20 -if test -n "$GIT_GETTEXT_POISON"

20 +if test -n "$GIT_TEST_GETTEXT_POISON"

21 then

22 GIT_INTERNAL_GETTEXT_SH_SCHEME=poison

23 elif test -n "@@USE_GETTEXT_SCHEME@@"

po/README

+4 -9

@@ -289,16 +289,11 @@ something in the test suite might still depend on the US English

289 version of the strings, e.g. to grep some error message or other

290 output.

291

292 -To smoke out issues like these Git can be compiled with gettext poison

293 -support, at the top-level:

292 +To smoke out issues like these, Git tested with a translation mode that

293 +emits gibberish on every call to gettext. To use it run the test suite

294 +with it, e.g.:

295

295 - make GETTEXT_POISON=YesPlease

296 -

297 -That'll give you a git which emits gibberish on every call to

298 -gettext. It's obviously not meant to be installed, but you should run

299 -the test suite with it:

300 -

301 - cd t && prove -j 9 ./t[0-9]*.sh

296 + cd t && GIT_TEST_GETTEXT_POISON=YesPlease prove -j 9 ./t[0-9]*.sh

297

298 If tests break with it you should inspect them manually and see if

299 what you're translating is sane, i.e. that you're not translating

t/README

@@ -301,6 +301,12 @@ that cannot be easily covered by a few specific test cases. These

301 could be enabled by running the test suite with correct GIT_TEST_

302 environment set.

303

304 +GIT_TEST_GETTEXT_POISON=<non-empty?> turns all strings marked for

305 +translation into gibberish if non-empty (think "test -n"). Used for

306 +spotting those tests that need to be marked with a C_LOCALE_OUTPUT

307 +prerequisite when adding more strings for translation. See "Testing

308 +marked strings" in po/README for details.

309 +

310 GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole

311 test suite. Accept any boolean values that are accepted by git-config.

312

t/lib-gettext.sh

+1 -1

@@ -12,7 +12,7 @@ export GIT_TEXTDOMAINDIR GIT_PO_PATH

13 . "$GIT_BUILD_DIR"/git-sh-i18n

15 -if test_have_prereq GETTEXT && ! test_have_prereq GETTEXT_POISON

15 +if test_have_prereq GETTEXT && test_have_prereq C_LOCALE_OUTPUT

16 then

17 # is_IS.UTF-8 on Solaris and FreeBSD, is_IS.utf8 on Debian

18 is_IS_locale=$(locale -a 2>/dev/null |

t/t0000-basic.sh

+1 -1

@@ -274,7 +274,7 @@ test_expect_success 'pretend we have a mix of all possible results' "

274 EOF

275 "

276

277 -test_expect_success 'test --verbose' '

277 +test_expect_success C_LOCALE_OUTPUT 'test --verbose' '

278 test_must_fail run_sub_test_lib_test \

279 test-verbose "test verbose" --verbose <<-\EOF &&

280 test_expect_success "passing test" true

t/t0205-gettext-poison.sh

+5 -3

@@ -5,13 +5,15 @@

6 test_description='Gettext Shell poison'

8 +GIT_TEST_GETTEXT_POISON=YesPlease

9 +export GIT_TEST_GETTEXT_POISON

10 . ./lib-gettext.sh

10 -test_expect_success GETTEXT_POISON 'sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME" is poison' '

12 +test_expect_success 'sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME" is poison' '

13 test "$GIT_INTERNAL_GETTEXT_SH_SCHEME" = "poison"

14 '

14 -test_expect_success GETTEXT_POISON 'gettext: our gettext() fallback has poison semantics' '

16 +test_expect_success 'gettext: our gettext() fallback has poison semantics' '

17 printf "# GETTEXT POISON #" >expect &&

18 gettext "test" >actual &&

19 test_cmp expect actual &&

@@ -20,7 +22,7 @@ test_expect_success GETTEXT_POISON 'gettext: our gettext() fallback has poison s

22 test_cmp expect actual

23 '

23 -test_expect_success GETTEXT_POISON 'eval_gettext: our eval_gettext() fallback has poison semantics' '

25 +test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semantics' '

26 printf "# GETTEXT POISON #" >expect &&

27 eval_gettext "test" >actual &&

28 test_cmp expect actual &&

t/t3406-rebase-message.sh

+1 -1

@@ -77,7 +77,7 @@ test_expect_success 'rebase -n overrides config rebase.stat config' '

77 # "Does not point to a valid commit: invalid-ref"

78 #

79 # NEEDSWORK: This "grep" is fine in real non-C locales, but

80 -# GETTEXT_POISON poisons the refname along with the enclosing

80 +# GIT_TEST_GETTEXT_POISON poisons the refname along with the enclosing

81 # error message.

82 test_expect_success 'rebase --onto outputs the invalid ref' '

83 test_must_fail git rebase --onto invalid-ref HEAD HEAD 2>err &&

t/t7201-co.sh

+3 -3

@@ -254,9 +254,9 @@ test_expect_success 'checkout to detach HEAD (with advice declined)' '

254 test_expect_success 'checkout to detach HEAD' '

255 git config advice.detachedHead true &&

256 git checkout -f renamer && git clean -f &&

257 - git checkout renamer^ 2>messages &&

258 - test_i18ngrep "HEAD is now at 7329388" messages &&

259 - (test_line_count -gt 1 messages || test -n "$GETTEXT_POISON") &&

257 + GIT_TEST_GETTEXT_POISON= git checkout renamer^ 2>messages &&

258 + grep "HEAD is now at 7329388" messages &&

259 + test_line_count -gt 1 messages &&

260 H=$(git rev-parse --verify HEAD) &&

261 M=$(git show-ref -s --verify refs/heads/master) &&

262 test "z$H" = "z$M" &&

t/t9902-completion.sh

+2 -1

@@ -1697,7 +1697,8 @@ test_expect_success 'sourcing the completion script clears cached commands' '

1697 verbose test -z "$__git_all_commands"

1698 '

1699

1700 -test_expect_success !GETTEXT_POISON 'sourcing the completion script clears cached merge strategies' '

1700 +test_expect_success 'sourcing the completion script clears cached merge strategies' '

1701 + GIT_TEST_GETTEXT_POISON= &&

1702 __git_compute_merge_strategies &&

1703 verbose test -n "$__git_merge_strategies" &&

1704 . "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&

t/test-lib-functions.sh

+4 -4

@@ -755,16 +755,16 @@ test_cmp_bin() {

755

756 # Use this instead of test_cmp to compare files that contain expected and

757 # actual output from git commands that can be translated. When running

758 -# under GETTEXT_POISON this pretends that the command produced expected

758 +# under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected

759 # results.

760 test_i18ncmp () {

761 - test -n "$GETTEXT_POISON" || test_cmp "$@"

761 + ! test_have_prereq C_LOCALE_OUTPUT || test_cmp "$@"

762 }

763

764 # Use this instead of "grep expected-string actual" to see if the

765 # output from a git command that can be translated either contains an

766 # expected string, or does not contain an unwanted one. When running

767 -# under GETTEXT_POISON this pretends that the command produced expected

767 +# under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected

768 # results.

769 test_i18ngrep () {

770 eval "last_arg=\${$#}"

@@ -779,7 +779,7 @@ test_i18ngrep () {

779 error "bug in the test script: too few parameters to test_i18ngrep"

780 fi

781

782 - if test -n "$GETTEXT_POISON"

782 + if test_have_prereq !C_LOCALE_OUTPUT

783 then

784 # pretend success

785 return 0

t/test-lib.sh

+17 -5

@@ -95,6 +95,16 @@ PAGER=cat

95 TZ=UTC

96 export LANG LC_ALL PAGER TZ

97 EDITOR=:

98 +

99 +# GIT_TEST_GETTEXT_POISON should not influence git commands executed

100 +# during initialization of test-lib and the test repo. Back it up,

101 +# unset and then restore after initialization is finished.

102 +if test -n "$GIT_TEST_GETTEXT_POISON"

103 +then

104 + GIT_TEST_GETTEXT_POISON_ORIG=$GIT_TEST_GETTEXT_POISON

105 + unset GIT_TEST_GETTEXT_POISON

106 +fi

107 +

108 # A call to "unset" with no arguments causes at least Solaris 10

109 # /usr/xpg4/bin/sh and /bin/ksh to bail out. So keep the unsets

110 # deriving from the command substitution clustered with the other

@@ -1104,13 +1114,15 @@ test -n "$USE_LIBPCRE1" && test_set_prereq LIBPCRE1

1114 test -n "$USE_LIBPCRE2" && test_set_prereq LIBPCRE2

1115 test -z "$NO_GETTEXT" && test_set_prereq GETTEXT

1116

1117 +if test -n "$GIT_TEST_GETTEXT_POISON_ORIG"

1118 +then

1119 + GIT_TEST_GETTEXT_POISON=$GIT_TEST_GETTEXT_POISON_ORIG

1120 + unset GIT_TEST_GETTEXT_POISON_ORIG

1121 +fi

1122 +

1123 # Can we rely on git's output in the C locale?

1108 -if test -n "$GETTEXT_POISON"

1124 +if test -z "$GIT_TEST_GETTEXT_POISON"

1125 then

1110 - GIT_GETTEXT_POISON=YesPlease

1111 - export GIT_GETTEXT_POISON

1112 - test_set_prereq GETTEXT_POISON

1113 -else

1126 test_set_prereq C_LOCALE_OUTPUT

1127 fi

1128