On Thu, Nov 08, 2018 at 09:26:19PM +0100, Ævar Arnfjörð Bjarmason wrote: > On Fri, Nov 02 2018, SZEDER Gábor wrote:
> >> * 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. With the benefit of hindsight gained from looking into a failing GETTEXT_POISON build [1], I have to agree with Junio that flat-out erroring out when GETTEXT_POISON=YesPlease is set is really a hassle [2]. [1] https://public-inbox.org/git/20181107130950.ga30...@szeder.dev/ (the failure is not related to this patch) [2] https://public-inbox.org/git/xmqqpnvg8d5z....@gitster-ct.c.googlers.com/ > >> 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). Using the prereq in the first place is what confused (and is still confusing...) the reader.
