Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On Sat, Oct 27 2018, Jeff King wrote: > On Fri, Oct 26, 2018 at 09:20:56PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> >> I was thinking: >> >> >> >> $ git var -e GIT_WHATEVER_ENV >> >> >> >> [-e for environment]. >> >> >> >> ... but that is really no different than git-config. ;-) >> > >> > Actually, "git var" already does pull bits from the environment. It >> > doesn't know about all of the type-specific parsing that git-config >> > does, but it might be a reasonable path forward to teach it that. (But I >> > still think we should do nothing for now and see how often this comes >> > up). >> >> For myself / Junio picking this up: Does that mean you've read v2 and >> think it's OK to pick up in its current form? I think it is, just >> looking for some Acks on that since it's not in the latest "What's >> Cooking". > > I'm not sure if you're asking whether I looked at the rest of the patch. > If so, then no, not really (so no objection, but I also did not review > it). Yeah I'm fishing for a more general review than just the problem of how we turn an env variable into a boolean, so if you or anyone else is up for it it would be most welcome :)
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On Fri, Oct 26, 2018 at 09:20:56PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> I was thinking: > >> > >> $ git var -e GIT_WHATEVER_ENV > >> > >> [-e for environment]. > >> > >> ... but that is really no different than git-config. ;-) > > > > Actually, "git var" already does pull bits from the environment. It > > doesn't know about all of the type-specific parsing that git-config > > does, but it might be a reasonable path forward to teach it that. (But I > > still think we should do nothing for now and see how often this comes > > up). > > For myself / Junio picking this up: Does that mean you've read v2 and > think it's OK to pick up in its current form? I think it is, just > looking for some Acks on that since it's not in the latest "What's > Cooking". I'm not sure if you're asking whether I looked at the rest of the patch. If so, then no, not really (so no objection, but I also did not review it). -Peff
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On Thu, Oct 25 2018, Jeff King wrote: > On Thu, Oct 25, 2018 at 02:24:41AM +0100, Ramsay Jones wrote: > >> >> Yeah, my thinko. The latter would be closer to what this patch >> >> wants to have, but obviously the former would be more flexible and >> >> useful in wider context. Both have the "Huh?" factor---what they >> >> are doing has little to do with "config", but I did not think of a >> >> better kitchen-sink (and our default kitchen-sink "rev-parse" is >> >> even further than "config", I would think, for this one). >> > >> > Heh, I thought through the exact sequence in your paragraph when writing >> > my other message. That's probably a good sign that we should probably >> > not pursue this further unless we see the use case come up again a few >> > more times (and if we do, then consider "config" the least-bad place to >> > do it). >> >> I was thinking: >> >> $ git var -e GIT_WHATEVER_ENV >> >> [-e for environment]. >> >> ... but that is really no different than git-config. ;-) > > Actually, "git var" already does pull bits from the environment. It > doesn't know about all of the type-specific parsing that git-config > does, but it might be a reasonable path forward to teach it that. (But I > still think we should do nothing for now and see how often this comes > up). For myself / Junio picking this up: Does that mean you've read v2 and think it's OK to pick up in its current form? I think it is, just looking for some Acks on that since it's not in the latest "What's Cooking".
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On Thu, Oct 25, 2018 at 02:24:41AM +0100, Ramsay Jones wrote: > >> Yeah, my thinko. The latter would be closer to what this patch > >> wants to have, but obviously the former would be more flexible and > >> useful in wider context. Both have the "Huh?" factor---what they > >> are doing has little to do with "config", but I did not think of a > >> better kitchen-sink (and our default kitchen-sink "rev-parse" is > >> even further than "config", I would think, for this one). > > > > Heh, I thought through the exact sequence in your paragraph when writing > > my other message. That's probably a good sign that we should probably > > not pursue this further unless we see the use case come up again a few > > more times (and if we do, then consider "config" the least-bad place to > > do it). > > I was thinking: > > $ git var -e GIT_WHATEVER_ENV > > [-e for environment]. > > ... but that is really no different than git-config. ;-) Actually, "git var" already does pull bits from the environment. It doesn't know about all of the type-specific parsing that git-config does, but it might be a reasonable path forward to teach it that. (But I still think we should do nothing for now and see how often this comes up). -Peff
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On 25/10/2018 02:09, Jeff King wrote: > On Thu, Oct 25, 2018 at 10:00:31AM +0900, Junio C Hamano wrote: > >> Jeff King writes: >> >>> but then you lose the default handling. I think if we added a new >>> option, it would either be: >>> >>> # interpret a value directly; use default on empty, I guess? >>> git config --default=false --type=bool --interpret-value >>> "$GIT_WHATEVER_ENV" >>> >>> or >>> >>> # less flexible, but the --default semantics are more obvious >>> git config --default=false --type=bool --get-env GIT_WHATEVER_ENV >> >> Yeah, my thinko. The latter would be closer to what this patch >> wants to have, but obviously the former would be more flexible and >> useful in wider context. Both have the "Huh?" factor---what they >> are doing has little to do with "config", but I did not think of a >> better kitchen-sink (and our default kitchen-sink "rev-parse" is >> even further than "config", I would think, for this one). > > Heh, I thought through the exact sequence in your paragraph when writing > my other message. That's probably a good sign that we should probably > not pursue this further unless we see the use case come up again a few > more times (and if we do, then consider "config" the least-bad place to > do it). I was thinking: $ git var -e GIT_WHATEVER_ENV [-e for environment]. ... but that is really no different than git-config. ;-) ATB, Ramsay Jones
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On Thu, Oct 25, 2018 at 10:00:31AM +0900, Junio C Hamano wrote: > Jeff King writes: > > > but then you lose the default handling. I think if we added a new > > option, it would either be: > > > > # interpret a value directly; use default on empty, I guess? > > git config --default=false --type=bool --interpret-value > > "$GIT_WHATEVER_ENV" > > > > or > > > > # less flexible, but the --default semantics are more obvious > > git config --default=false --type=bool --get-env GIT_WHATEVER_ENV > > Yeah, my thinko. The latter would be closer to what this patch > wants to have, but obviously the former would be more flexible and > useful in wider context. Both have the "Huh?" factor---what they > are doing has little to do with "config", but I did not think of a > better kitchen-sink (and our default kitchen-sink "rev-parse" is > even further than "config", I would think, for this one). Heh, I thought through the exact sequence in your paragraph when writing my other message. That's probably a good sign that we should probably not pursue this further unless we see the use case come up again a few more times (and if we do, then consider "config" the least-bad place to do it). -Peff
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
Jeff King writes: > but then you lose the default handling. I think if we added a new > option, it would either be: > > # interpret a value directly; use default on empty, I guess? > git config --default=false --type=bool --interpret-value "$GIT_WHATEVER_ENV" > > or > > # less flexible, but the --default semantics are more obvious > git config --default=false --type=bool --get-env GIT_WHATEVER_ENV Yeah, my thinko. The latter would be closer to what this patch wants to have, but obviously the former would be more flexible and useful in wider context. Both have the "Huh?" factor---what they are doing has little to do with "config", but I did not think of a better kitchen-sink (and our default kitchen-sink "rev-parse" is even further than "config", I would think, for this one).
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
On Wed, Oct 24, 2018 at 02:45:49PM +0900, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > > > Notes on the implementation: > > > > * The only reason we need a new "git-sh-i18n--helper" and the > >corresponding "test-tool gettext-poison" is to expose > >git_env_bool() to shellscripts, since git-sh-i18n and friends need > >to inspect the $GIT_TEST_GETTEXT_POISON variable. > > > >We only call these if $GIT_TEST_GETTEXT_POISON is set, or in the > >test suite, and this code can go away for non-test code once the > >last i18n-using shellscript is rewritten in C. > > Makes me wonder if we want to teach "git config" a new "--env-bool" > option that can be used by third-party scripts. Or would it be just > the matter of writing > > git config --default=false --type=bool "$GIT_WHATEVER_ENV" > > in these third-party scripts and we do not need to add such a thing? The config command only takes names, not values. So you'd have to write something like: git -c env.bool="$GIT_WHATEVER_ENV" config --type=bool env.bool but then you lose the default handling. I think if we added a new option, it would either be: # interpret a value directly; use default on empty, I guess? git config --default=false --type=bool --interpret-value "$GIT_WHATEVER_ENV" or # less flexible, but the --default semantics are more obvious git config --default=false --type=bool --get-env GIT_WHATEVER_ENV -Peff
Re: [PATCH] i18n: make GETTEXT_POISON a runtime option
Ævar Arnfjörð Bjarmason writes:
> Notes on the implementation:
>
> * The only reason we need a new "git-sh-i18n--helper" and the
>corresponding "test-tool gettext-poison" is to expose
>git_env_bool() to shellscripts, since git-sh-i18n and friends need
>to inspect the $GIT_TEST_GETTEXT_POISON variable.
>
>We only call these if $GIT_TEST_GETTEXT_POISON is set, or in the
>test suite, and this code can go away for non-test code once the
>last i18n-using shellscript is rewritten in C.
Makes me wonder if we want to teach "git config" a new "--env-bool"
option that can be used by third-party scripts. Or would it be just
the matter of writing
git config --default=false --type=bool "$GIT_WHATEVER_ENV"
in these third-party scripts and we do not need to add such a thing?
> * The reason for not doing:
>
>test_lazy_prereq GETTEXT_POISON 'test-tool gettext-poison'
>test_lazy_prereq C_LOCALE_OUTPUT '! test-tool gettext-poison'
>
>In test-lib.sh is because there's some interpolation problem with
>that syntax which makes t6040-tracking-info.sh fail. Hence using
>the simpler test_set_prereq.
s/In/in/, as you haven't finished the sentence yet. But more
importantly, what is this "some interpolation problem"? Are you
saying that test_lazy_prereq implementation is somehow broken and
cannot take certain strings? If so, perhaps we want to fix that,
and people other than you can help to do so, once you let them know
what the problem is.
> See also
> https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/ for
> more discussion.
"See also [*1*] for more discussion" as you've already have
reference below.
>
> 1. https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
>
> Here's a polished version of that. I think it makes sense to queue
> this up before any other refactoring of GETTEXT_POISON, and some patch
> to unconditionally preserve format specifiers as I suggested upthread
> could go on top of this.
> ...
> +int cmd_sh_i18n__helper(int argc, const char **argv, const char *prefix)
> +{
> + int poison = -1;
> + struct option options[] = {
> + OPT_BOOL(0, "git-test-gettext-poison", &poison,
> + N_("is GIT_TEST_GETTEXT_POISON in effect?")),
> + OPT_END()
> + };
> +
> + argc = parse_options(argc, argv, NULL, options,
> + builtin_sh_i18n_helper_usage,
> PARSE_OPT_KEEP_ARGV0);
> +
> + if (poison != -1)
> + return !git_env_bool("GIT_TEST_GETTEXT_POISON", 0);
Hmm? If "--[no-]git-test-gettext-poison" is given, poison is either
0 or 1, and we return the value we read from the environment? What
convoluted way to implement the option is that, or is there anything
subtle that I am not getting?
If the "default" parameter to git_env_bool() were poison, and then
the option was renamed to "--get-git-text-gettext-poison", then I
sort of understand the code, though (it's like "git config" with
"--default" option).
But if there is nothing subtle, it may make sense to implement this
as an opt-cmdmode instead.
> diff --git a/po/README b/po/README
> index fef4c0f0b5..dba46c4a40 100644
> --- a/po/README
> +++ b/po/README
> @@ -289,16 +289,11 @@ something in the test suite might still depend on the
> US English
> version of the strings, e.g. to grep some error message or other
> output.
>
> -To smoke out issues like these Git can be compiled with gettext poison
> -support, at the top-level:
> +To smoke out issues like these Git tested with a translation mode that
> +emits gibberish on every call to gettext. To use it run the test suite
> +with it, e.g.:
s/these Git tested/these, Git can be tested/; even though the comma
is not a new issue you introduced.
[PATCH] 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 or if it wasn't and GETTEXT_POISON wasn't 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 this has become the common idiom in the codebase 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=[true|false] 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, but
this patch doesn't change any of the existing tests to work like that.
Notes on the implementation:
* The only reason we need a new "git-sh-i18n--helper" and the
corresponding "test-tool gettext-poison" is to expose
git_env_bool() to shellscripts, since git-sh-i18n and friends need
to inspect the $GIT_TEST_GETTEXT_POISON variable.
We only call these if $GIT_TEST_GETTEXT_POISON is set, or in the
test suite, and this code can go away for non-test code once the
last i18n-using shellscript is rewritten in C.
* 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.
* The reason for not doing:
test_lazy_prereq GETTEXT_POISON 'test-tool gettext-poison'
test_lazy_prereq C_LOCALE_OUTPUT '! test-tool gettext-poison'
In test-lib.sh is because there's some interpolation problem with
that syntax which makes t6040-tracking-info.sh fail. Hence using
the simpler test_set_prereq.
See also
https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/ for
more discussion.
1. https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason
---
On Mon, Oct 22 2018, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason writes:
>
>> So I think the only reason to keep it compile-time is performance, but I
>> don't think that matters. It's not like we're printing gigabytes of _()
>> formatted output. Everything where formatting matters is plumbing which
>> doesn't use this API. These messages are always for human consumption.
>>
>> So shouldn't we just drop this notion of GETTEXT_POISON at compile-time
>> entirely, and make GIT_GETTEXT_POISON= a test flag that all
>> builds of git are aware of? I think so.
>
> Haven't thought things through, but that sounds like a good thing to
> aim for. Keep the ideas coming...
Here's a polished version of that. I think it makes sense to queue
this up before any other refactoring of GETTEXT_POISON, and some patch
to unconditionally preserve format specifiers as I suggested upthread
could go on top of this.
.gitignore | 1 +
.travis.yml| 2 +-
Makefile | 11 ++-
builtin.h | 1 +
builtin/sh-i18n--helper.c | 27 +++
ci/lib-travisci.sh | 4 ++--
gettext.c | 5 ++---
gettext.h | 4
git-sh-i18n.sh | 3 ++-
git.c | 1 +
po/README | 13 -
t/README | 6 ++
t/helper/test-gettext-poison.c | 9 +
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t-basic.sh | 2 +-
t/t3406-rebase-message.sh | 2 +-
t/t7201-co.sh | 5 -
t/test-lib-functions.sh| 8
t/test-lib.sh | 11 ++-
20 files changed, 72 insertions(+), 45 deletions(-)
create mode 100644 builtin/sh-i18n--helper.c
create mode 100644 t/helper/test-gettext-poison.c
diff --git a/.gitignore b/.gitignore
index 9d1363a1eb..f7b7977910 100644
--- a/.gitignore
+++ b/.gitignore
@@ -148,6 +148,7 @@
/git-serve
/git-sh-i18n
/git-sh-i18n--envsubst
+/git-sh-i18n--helper
/git-sh-setup
/git-sh-i18n
/git-shell
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
