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

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Nov 8, 2018, 20:26 UTC
Message-ID
<87muqj5n9w.fsf@evledraar.gmail.com>
In-Reply-To
<20181102163725.GY30222@szeder.dev>
On Fri, Nov 02 2018, SZEDER Gábor wrote:
Show 62 quoted lines
> On Thu, Nov 01, 2018 at 07:31:15PM +0000, Ævar Arnfjörð Bjarmason wrote:
>> 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.
>>    This is probably the wrong thing to do and should be followed-up
>>    with something similar to ae59a4e44f ("travis: run tests with
>>    GIT_TEST_SPLIT_INDEX", 2018-01-07) to re-use an existing test setup
>>    for running in the GIT_TEST_GETTEXT_POISON mode.
>
> I'm of two minds about this.  Sure, now it's not a compile time
> option, so we can spare a dedicated compilation, and sparing resources
> is good, of course.
>
> However, checking test failures in the 'linux-gcc' build job is always
> a bit of a hassle, because it's not enough to look at the output of
> the failed test, but I also have to check which one of the two test
> runs failed (i.e. the "regular" or one with all the GIT_TEST_* knobs
> turned on).  Adding a second test run with GIT_TEST_GETTEXT_POISON
> enabled to an other build job (e.g. 'linux-clang') would then bring
> this hassle to that build job as well.
>
> Furthermore, occasionally a build job is slower than usual (whatever
> the reason might be), which can be an issue when running the test
> suite twice, as it can exceed the time limit.
>
> Anyway, we can think about this later.
>
> In any case, GIT_TEST_GETTEXT_POISON definitely should not be enabled
> in the same "catch-all" test run with all other GIT_TEST_* variables,
> because it would mess up any translated error messages that might pop
> up in a test failure, and then we wouldn't have any idea about what
> went wrong.
Will clarify this in v3. I.e. "let's think about this...".
Show 41 quoted lines
>>  * 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.
>>
>> See also [3] 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/87woq7b58k.fsf@evledraar.gmail.com/
>> 3. https://public-inbox.org/git/878t2pd6yu.fsf@evledraar.gmail.com/
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>
>> Now:
>>
>>  * The new i18n helper is gone. We just use "test -n" semantics for
>>    $GIT_TEST_GETTEXT_POISON
>>
>>  * 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.
Show 53 quoted lines
>>  * The caveat with avoiding test_lazy_prereq is gone (although there's
>>    still some unrelated bug there worth looking into).
>>
>>  * We call use_gettext_poison() really early to avoid any reentrancy
>>    issue with getenv().
>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 175f83d704..3c6b185b60 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -1697,7 +1697,8 @@ test_expect_success 'sourcing the completion script clears cached commands' '
>>  	verbose test -z "$__git_all_commands"
>>  '
>>
>> -test_expect_success !GETTEXT_POISON 'sourcing the completion script clears cached merge strategies' '
>> +test_expect_success 'sourcing the completion script clears cached merge strategies' '
>> +	GIT_TEST_GETTEXT_POISON= &&
>>  	__git_compute_merge_strategies &&
>
> OK, makes sense.
>
>>  	verbose test -n "$__git_merge_strategies" &&
>>  	. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
>
>> 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).

Show 35 quoted lines
>>  	then
>>  		# pretend success
>>  		return 0
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 897e6fcc94..370a4821e1 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -1105,12 +1105,8 @@ test -n "$USE_LIBPCRE2" && test_set_prereq LIBPCRE2
>>  test -z "$NO_GETTEXT" && test_set_prereq GETTEXT
>>
>>  # Can we rely on git's output in the C locale?
>> -if test -n "$GETTEXT_POISON"
>> +if test -z "$GIT_TEST_GETTEXT_POISON"
>>  then
>> -	GIT_GETTEXT_POISON=YesPlease
>> -	export GIT_GETTEXT_POISON
>> -	test_set_prereq GETTEXT_POISON
>> -else
>>  	test_set_prereq C_LOCALE_OUTPUT
>>  fi
>
> GIT_TEST_GETTEXT_POISON=1 will influence even those git commands that
> are executed during initialization of test-lib and the test repo:
>
>   $ GIT_TEST_GETTEXT_POISON=1 ./t0000-basic.sh -v
>   # GETTEXT POISON #expecting success:
>   <....>
>
> It should say:
>
>   Initialized empty Git repository in <... path ...>
>   expecting success:
>
> See https://public-inbox.org/git/20181022202241.18629-2-szeder.dev@gmail.com/
> for a bit of code that you might find worthy to steal.
Thanks. Fixing.
Previous: SZEDER GáborNext: SZEDER Gábor
Message 33 of 49 in “Poison gettext with the Ook language”
  1. Poison gettext with the Ook languageNguyễn Thái Ngọc Duy, Oct 22, 2018
  2. SZEDER GáborOct 22, 2018
  3. 1/8 test-lib.sh: preserve GIT_GETTEXT_POISON from the environmentSZEDER Gábor, Oct 22, 2018
  4. 2/8 gettext: don't poison if GIT_GETTEXT_POISON is set but emptySZEDER Gábor, Oct 22, 2018
  5. Ævar Arnfjörð BjarmasonOct 22, 2018
  6. 3/8 lib-rebase: loosen GETTEXT_POISON check in fake editorSZEDER Gábor, Oct 22, 2018
  7. 4/8 gettext: #ifdef away GETTEXT POISON-related code from _() and Q_()SZEDER Gábor, Oct 22, 2018
  8. 5/8 gettext: put "# GETTEXT POISON #" string literal into a macroSZEDER Gábor, Oct 22, 2018
  9. 6/8 gettext: use an enum for the mode of GETTEXT POISONingSZEDER Gábor, Oct 22, 2018
  10. 7/8 gettext: introduce GIT_GETTEXT_POISON=scrambledSZEDER Gábor, Oct 22, 2018
  11. Duy NguyenOct 23, 2018
  12. 8/8 travis-ci: run GETTEXT POISON build job in scrambled mode, tooSZEDER Gábor, Oct 22, 2018
  13. Duy NguyenOct 23, 2018
  14. Jakub NarebskiOct 27, 2018
  15. Johannes SchindelinOct 22, 2018
  16. Ævar Arnfjörð BjarmasonOct 22, 2018
  17. Ævar Arnfjörð BjarmasonOct 22, 2018
  18. Junio C HamanoOct 22, 2018
  19. i18n: make GETTEXT_POISON a runtime optionÆvar Arnfjörð Bjarmason, Oct 23, 2018
  20. Junio C HamanoOct 24, 2018
  21. Jeff KingOct 24, 2018
  22. Junio C HamanoOct 25, 2018
  23. Jeff KingOct 25, 2018
  24. Ramsay JonesOct 25, 2018
  25. Jeff KingOct 25, 2018
  26. Ævar Arnfjörð BjarmasonOct 26, 2018
  27. Jeff KingOct 27, 2018
  28. Ævar Arnfjörð BjarmasonOct 27, 2018
  29. i18n: make GETTEXT_POISON a runtime optionÆvar Arnfjörð Bjarmason, Oct 24, 2018
  30. i18n: make GETTEXT_POISON a runtime optionÆvar Arnfjörð Bjarmason, Nov 1, 2018
  31. Junio C HamanoNov 2, 2018
  32. SZEDER GáborNov 2, 2018
  33. Ævar Arnfjörð BjarmasonNov 8, 2018
  34. SZEDER GáborNov 8, 2018
  35. Junio C HamanoNov 8, 2018
  36. Eric SunshineNov 8, 2018
  37. 0/2 i18n: make GETTEXT_POISON a runtime optionÆvar Arnfjörð Bjarmason, Nov 8, 2018
  38. 1/2 i18n: make GETTEXT_POISON a runtime optionÆvar Arnfjörð Bjarmason, Nov 8, 2018
  39. 2/2 Makefile: ease dynamic-gettext-poison transitionÆvar Arnfjörð Bjarmason, Nov 8, 2018
  40. Johannes SchindelinOct 23, 2018
  41. Ævar Arnfjörð BjarmasonOct 23, 2018
  42. Johannes SchindelinOct 23, 2018
  43. Duy NguyenOct 23, 2018
  44. Ævar Arnfjörð BjarmasonOct 23, 2018
  45. Duy NguyenOct 24, 2018
  46. Ævar Arnfjörð BjarmasonOct 24, 2018
  47. Junio C HamanoOct 25, 2018
  48. Jeff KingOct 25, 2018
  49. Junio C HamanoOct 27, 2018

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.