• Ævar Arnfjörð Bjarmason's avatar
    i18n: make GETTEXT_POISON a runtime option · 6cdccfce
    Ævar Arnfjörð Bjarmason authored
    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 bb946bba ("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 859fdc0c ("commit-graph: define
    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 ae59a4e4 ("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
     * 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/[email protected]/
    2. https://public-inbox.org/git/[email protected]/
    3. https://public-inbox.org/git/[email protected]/
    4. https://public-inbox.org/git/[email protected]/Signed-off-by: Ævar Arnfjörð Bjarmason's avatarÆvar Arnfjörð Bjarmason <[email protected]>
    Signed-off-by: default avatarJunio C Hamano <[email protected]>