Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
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.
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
On Fri, Nov 02 2018, SZEDER Gábor wrote:
> On Thu, Nov 01, 2018 at 07:31:15PM +, Æ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=
>> 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...".
>> * We now skip a test in t-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@evledraar.gmail.com/
>> 2. https://public-inbox.org/git/87woq7b58k@evledraar.gmail.com/
>> 3. https://public-inbox.org/git/878t2pd6yu@evledraar.gmail.com/
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason
>> ---
>>
>> 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.
>> * The caveat with avoiding test_lazy_prereq is gone (although
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
On Wed, Nov 7, 2018 at 10:24 PM Junio C Hamano wrote: > Makefile: ease dynamic-gettext-poison transition > > Earlier we made the entire build to fail when GETTEXT_POISON=Yes is > given to make, to notify those who did not notice that text poisoning > is now a runtime behaviour. > > It turns out that this too irritating for those who need to build s/too/is &/ > and test different versions of Git that cross the boundary between > history with and without this topic to switch between two > environment variables. Demote the error to a warning, so that you > can say something like > > make GETTEXT_POISON=Yes GIT_TEST_GETTEXT_POISON test > > during the transition period, without having to worry about whether > exact version you are testing has or does not have this topic. > > Signed-off-by: Junio C Hamano
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
Ævar Arnfjörð Bjarmason writes: > * We error out in the Makefile if you're still saying >GETTEXT_POISON=YesPlease. I expected this would be irritating, but it turns out it is worse than mere irritation but is a severe hinderance to affect my performance, as I (and my bots) keep building and testing different versions of Git under different configurations. I know it was done to help those who only ever build a single track at a time and mostly moving forward, but I'm very much tempted to remove this part, perhaps demote it to a warning of some sort. > ifdef GETTEXT_POISON > - BASIC_CFLAGS += -DGETTEXT_POISON > +$(error The GETTEXT_POISON option has been removed in favor of runtime > GIT_TEST_GETTEXT_POISON. See t/README!) > endif -- >8 -- Makefile: ease dynamic-gettext-poison transition Earlier we made the entire build to fail when GETTEXT_POISON=Yes is given to make, to notify those who did not notice that text poisoning is now a runtime behaviour. It turns out that this too irritating for those who need to build and test different versions of Git that cross the boundary between history with and without this topic to switch between two environment variables. Demote the error to a warning, so that you can say something like make GETTEXT_POISON=Yes GIT_TEST_GETTEXT_POISON test during the transition period, without having to worry about whether exact version you are testing has or does not have this topic. Signed-off-by: Junio C Hamano --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index f3a9995e50..6b492f44a6 100644 --- a/Makefile +++ b/Makefile @@ -1447,7 +1447,7 @@ ifdef NO_SYMLINK_HEAD BASIC_CFLAGS += -DNO_SYMLINK_HEAD endif ifdef GETTEXT_POISON -$(error The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!) +$(warning The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!) endif ifdef NO_GETTEXT BASIC_CFLAGS += -DNO_GETTEXT
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
On Thu, Nov 01, 2018 at 07:31:15PM +, Æ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=
> 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.
> * We now skip a test in t-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@evledraar.gmail.com/
> 2. https://public-inbox.org/git/87woq7b58k@evledraar.gmail.com/
> 3. https://public-inbox.org/git/878t2pd6yu@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
>
> 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.
> * 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
>
Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option
Ævar Arnfjörð Bjarmason writes:
> Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON
> test parameter to only be a GIT_TEST_GETTEXT_POISON=
> runtime parameter, to be consistent with other parameters documented
> in "Running tests with special setups" in t/README.
> ...
> #ifndef NO_GETTEXT
> extern void git_setup_gettext(void);
> extern int gettext_width(const char *s);
> #else
> static inline void git_setup_gettext(void)
> {
> + use_gettext_poison();; /* getenv() reentrancy paranoia */
> }
Too many "case/esac" statement in shell scripting?
[PATCH v3] 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=
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.
* We now skip a test in t-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@evledraar.gmail.com/
2. https://public-inbox.org/git/87woq7b58k@evledraar.gmail.com/
3. https://public-inbox.org/git/878t2pd6yu@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason
---
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.
* 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().
.travis.yml | 2 +-
Makefile | 8 +---
ci/lib-travisci.sh| 4 ++--
gettext.c | 11 +++
gettext.h | 9 +++--
git-sh-i18n.sh| 2 +-
po/README | 13 -
t/README | 6 ++
t/lib-gettext.sh | 2 +-
t/t-basic.sh | 2 +-
t/t0205-gettext-poison.sh | 8 +---
t/t3406-rebase-message.sh | 2 +-
t/t7201-co.sh | 6 +++---
t/t9902-completion.sh | 3 ++-
t/test-lib-functions.sh | 8
t/test-lib.sh | 6 +-
16 files changed, 43 insertions(+), 49 deletions(-)
diff --git a/.travis.yml b/.travis.yml
index 4d4e26c9df..4523a2e5ec 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -26,7 +26,7 @@ addons:
matrix:
include:
-- env: jobname=GETTEXT_POISON
+- env: jobname=GIT_TEST_GETTEXT_POISON
os: linux
compiler:
addons:
diff --git a/Makefile b/Makefile
index b08d5ea258..3a08626db0 100644
--- a/Makefile
+++ b/Makefile
@@ -362,11 +362,6 @@ all::
# Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the
# user.
#
-# Define GETTEXT_POISON if you are debugging the choice of strings marked
-# for translation. In a GETTEXT_POISON build, you can turn all strings marked
-# for translation into gibberish by setting the GIT_GETTEXT_POISON variable
-# (to any value) in your environment.
-#
# Define JSMIN to point to JavaScript minifier that functions as
# a filter to have gitweb.js minified.
#
@@ -1450,7 +1445,7 @@ ifdef NO_SYMLINK_HEAD
