{"thread":{"id":"49637","subject":"[PATCH] Poison gettext with the Ook language","startedAt":"2018-10-22T15:36:49Z","lastAt":"2018-11-08T21:15:51Z","messageCount":49,"participants":["Nguyễn Thái Ngọc Duy","SZEDER Gábor","Ævar Arnfjörð Bjarmason","Johannes Schindelin","Junio C Hamano","Duy Nguyen","Jeff King","Ramsay Jones","Jakub Narebski","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"361168","messageId":"20181022153633.31757-1-pclouds@gmail.com","threadId":"49637","inReplyTo":null,"subject":"[PATCH] Poison gettext with the Ook language","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-22T15:36:33Z","receivedAt":"2018-10-22T15:36:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The current gettext() function just replaces all strings with\n'# GETTEXT POISON #' including format strings and hides the things\nthat we should be allowed to grep (like branch names, or some other\ncodes) even when gettext is poisoned.\n\nThis patch implements the poisoned _() with a universal and totally\nlegit language called Ook [1]. We could actually grep stuff even in\nwith this because format strings are preserved.\n\nLong term, we could implement an \"ook translator\" for test_i18ngrep\nand friends so that they translate English to Ook, allowing us to\nmatch full text while making sure the text in the code is still marked\nfor translation.\n\n[1] https://en.wikipedia.org/wiki/Unseen_University#Librarian\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n This started out as something fun to do while running the test suite\n last weekend. But it turns out actually working! If this patch ends\n up in git.git, the Librarian would be so proud!\n\n gettext.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++\n gettext.h | 7 ++++---\n t/lib-rebase.sh | 2 +-\n 3 files changed, 59 insertions(+), 4 deletions(-)\n\ndiff --git a/gettext.c b/gettext.c\nindex 7272771c8e..29901e1ddd 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -56,6 +56,60 @@ int use_gettext_poison(void)\n }\n #endif\n \n+const char *gettext_poison(const char *msgid)\n+{\n+\t/*\n+\t * gettext() returns a string that is always valid. We would\n+\t * need a hash map for that but let's stay simple and keep the\n+\t * last 64 gettext() results. Should be more than enough.\n+\t */\n+\tstatic char *bufs[64];\n+\tstatic int i;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tchar *buf;\n+\tconst char *p;\n+\tconst char *type_specifiers = \"diouxXeEfFgGaAcsCSpnm%\";\n+\n+\tif (!strchr(msgid, '%'))\n+\t\treturn \"Eek!\";\n+\n+\tp = msgid;\n+\twhile (*p) {\n+\t\tconst char *type;\n+\t\tswitch (*p) {\n+\t\tcase '%':\n+\t\t\t/*\n+\t\t\t * No strict parsing. We simply look for the end of a\n+\t\t\t * format string\n+\t\t\t */\n+\t\t\ttype = p + 1;\n+\t\t\twhile (*type && !strchr(type_specifiers, *type))\n+\t\t\t\ttype++;\n+\t\t\tif (*type)\n+\t\t\t\ttype++;\n+\t\t\tstrbuf_add(&sb, p, (int)(type - p));\n+\t\t\tp = type;\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tif (!isalpha(*p)) {\n+\t\t\t\tstrbuf_addch(&sb, *p);\n+\t\t\t\tp++;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tif (isupper(*p))\n+\t\t\t\tstrbuf_addstr(&sb, \"Ook\");\n+\t\t\telse\n+\t\t\t\tstrbuf_addstr(&sb, \"ook\");\n+\t\t\twhile (isalpha(*p))\n+\t\t\t\tp++;\n+\t\t}\n+\t}\n+\tbuf = bufs[(i++) % ARRAY_SIZE(bufs)];\n+\tfree(buf);\n+\tbuf = strbuf_detach(&sb, NULL);\n+\treturn buf;\n+}\n+\n #ifndef NO_GETTEXT\n static int test_vsnprintf(const char *fmt, ...)\n {\ndiff --git a/gettext.h b/gettext.h\nindex 7eee64a34f..dc9851a06a 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -41,8 +41,9 @@ static inline int gettext_width(const char *s)\n }\n #endif\n \n+const char *gettext_poison(const char *);\n #ifdef GETTEXT_POISON\n-extern int use_gettext_poison(void);\n+int use_gettext_poison(void);\n #else\n #define use_gettext_poison() 0\n #endif\n@@ -51,14 +52,14 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n {\n \tif (!*msgid)\n \t\treturn \"\";\n-\treturn use_gettext_poison() ? \"# GETTEXT POISON #\" : gettext(msgid);\n+\treturn use_gettext_poison() ? gettext_poison(msgid) : gettext(msgid);\n }\n \n static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)\n const char *Q_(const char *msgid, const char *plu, unsigned long n)\n {\n \tif (use_gettext_poison())\n-\t\treturn \"# GETTEXT POISON #\";\n+\t\treturn gettext_poison(msgid);\n \treturn ngettext(msgid, plu, n);\n }\n \ndiff --git a/t/lib-rebase.sh b/t/lib-rebase.sh\nindex 2ca9fb69d6..1e8440e935 100644\n--- a/t/lib-rebase.sh\n+++ b/t/lib-rebase.sh\n@@ -29,7 +29,7 @@ set_fake_editor () {\n \t*/COMMIT_EDITMSG)\n \t\ttest -z \"$EXPECT_HEADER_COUNT\" ||\n \t\t\ttest \"$EXPECT_HEADER_COUNT\" = \"$(sed -n '1s/^# This is a combination of \\(.*\\) commits\\./\\1/p' < \"$1\")\" ||\n-\t\t\ttest \"# # GETTEXT POISON #\" = \"$(sed -n '1p' < \"$1\")\" ||\n+\t\t\ttest \"$EXPECT_HEADER_COUNT\" = \"$(sed -n '1s/^# Ook ook ook ook ook \\(.*\\) ook\\./\\1/p' < \"$1\")\" ||\n \t\t\texit\n \t\ttest -z \"$FAKE_COMMIT_MESSAGE\" || echo \"$FAKE_COMMIT_MESSAGE\" > \"$1\"\n \t\ttest -z \"$FAKE_COMMIT_AMEND\" || echo \"$FAKE_COMMIT_AMEND\" >> \"$1\"\n-- \n2.19.1.647.g708186aaf9\n\n"},{"id":"361181","messageId":"20181022202241.18629-1-szeder.dev@gmail.com","threadId":"49637","inReplyTo":"20181022153633.31757-1-pclouds@gmail.com","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-22T20:22:33Z","receivedAt":"2018-10-22T20:23:20Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Oct 22, 2018 at 05:36:33PM +0200, Nguyễn Thái Ngọc Duy wrote:\n> The current gettext() function just replaces all strings with\n> '# GETTEXT POISON #' including format strings and hides the things\n> that we should be allowed to grep (like branch names, or some other\n> codes) even when gettext is poisoned.\n> \n> This patch implements the poisoned _() with a universal and totally\n> legit language called Ook [1]. We could actually grep stuff even in\n> with this because format strings are preserved.\n\nOnce upon a time a GETTEXT_POISON build job failed on me, and the\nerror message:\n\n error: # GETTEXT POISON #\n\nwas not particularly useful. Ook wouldn't help with that...\n\nSo I came up with the following couple of patches that implement a\n\"scrambled\" format that makes the poisoned output legible for humans\nbut still gibberish for machine consumption (i.e. grep-ing the text\npart would still fail):\n\n error: U.n.a.b.l.e. .t.o. .c.r.e.a.t.e. .'./home/szeder/src/git/t/trash directory.t1404-update-ref-errors/.git/packed-refs...l.o.c.k.'.:. .File exists...\n\nI have been running GETTEXT_POISON builds with this series for some\nmonths now, but haven't submitted it yet, because I haven't decided\nyet whether including strings (paths, refs, etc.) in the output as\nthey are is a feature or a flaw. And because it embarrassingly leaks\nevery single translated string... :)\n\n\nSZEDER Gábor (8):\n test-lib.sh: preserve GIT_GETTEXT_POISON from the environment\n gettext: don't poison if GIT_GETTEXT_POISON is set but empty\n lib-rebase: loosen GETTEXT_POISON check in fake editor\n gettext: #ifdef away GETTEXT POISON-related code from _() and Q_()\n gettext: put \"# GETTEXT POISON #\" string literal into a macro\n gettext: use an enum for the mode of GETTEXT POISONing\n gettext: introduce GIT_GETTEXT_POISON=scrambled\n travis-ci: run GETTEXT POISON build job in scrambled mode, too\n\n Makefile | 2 +-\n ci/lib-travisci.sh | 1 +\n ci/run-build-and-tests.sh | 10 +++++--\n gettext.c | 63 +++++++++++++++++++++++++++++++++++----\n gettext.h | 33 +++++++++++++++-----\n t/lib-rebase.sh | 2 +-\n t/test-lib.sh | 17 ++++++++++-\n 7 files changed, 110 insertions(+), 18 deletions(-)\n\n-- \n2.19.1.681.g6bd79da3f5\n\n"},{"id":"361182","messageId":"20181022202241.18629-2-szeder.dev@gmail.com","threadId":"49637","inReplyTo":"20181022202241.18629-1-szeder.dev@gmail.com","subject":"[PATCH 1/8] test-lib.sh: preserve GIT_GETTEXT_POISON from the environment","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-22T20:22:34Z","receivedAt":"2018-10-22T20:23:31Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Setting GIT_GETTEXT_POISON can be useful when testing translated\nbuilds.\n\nHowever, preserving its value from the environment is not as simple as\nadding it to the list of GIT_* variables that should not be scrubbed\nfrom the environment:\n\n - GIT_GETTEXT_POISON should not influence git commands executed\n during initialization of test-lib and the test repo.\n\n Save its value before it gets scrubbed from the environment, so it\n will be unset for the duration of the initialization, and restore\n its original value after initialization is finished.\n\n - When testing a GETTEXT_POISON build, 'test-lib.sh' always sets\n GIT_GETTEXT_POISON to 'YesPlease'. This was fine while all that\n mattered was whether it's set or not. However, the following\n patches will introduce meaningful values (e.g. \"set but empty\" to\n disable poisoning even in a GETTEXT_POISON build), so only set it\n like this if GIT_GETTEXT_POISON was not set in the environment.\n\nSigned-off-by: SZEDER Gábor \n---\n t/test-lib.sh | 17 ++++++++++++++++-\n 1 file changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex ea2bbaaa7a..282c05110d 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -95,6 +95,16 @@ PAGER=cat\n TZ=UTC\n export LANG LC_ALL PAGER TZ\n EDITOR=:\n+\n+# GIT_GETTEXT_POISON should not influence git commands executed during\n+# initialization of test-lib and the test repo.\n+# Back it up, unset and then restore after initialization is finished.\n+if test -n \"${GIT_GETTEXT_POISON-set}\"\n+then\n+\tgit_gettext_poison_backup=$GIT_GETTEXT_POISON\n+\tunset GIT_GETTEXT_POISON\n+fi\n+\n # A call to \"unset\" with no arguments causes at least Solaris 10\n # /usr/xpg4/bin/sh and /bin/ksh to bail out. So keep the unsets\n # deriving from the command substitution clustered with the other\n@@ -1073,7 +1083,12 @@ test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n # Can we rely on git's output in the C locale?\n if test -n \"$GETTEXT_POISON\"\n then\n-\tGIT_GETTEXT_POISON=YesPlease\n+\tif test -n \"${git_gettext_poison_backup-set}\"\n+\tthen\n+\t\tGIT_GETTEXT_POISON=$git_gettext_poison_backup\n+\telse\n+\t\tGIT_GETTEXT_POISON=YesPlease\n+\tfi\n \texport GIT_GETTEXT_POISON\n \ttest_set_prereq GETTEXT_POISON\n else\n-- \n2.19.1.681.g6bd79da3f5\n\n"},{"id":"361183","messageId":"20181022202241.18629-3-szeder.dev@gmail.com","threadId":"49637","inReplyTo":"20181022202241.18629-1-szeder.dev@gmail.com","subject":"[PATCH 2/8] gettext: don't poison if GIT_GETTEXT_POISON is set but empty","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-22T20:22:35Z","receivedAt":"2018-10-22T20:23:33Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This allows us to run test with non-GETTEXT POISON-ed behavior even in\na GETTEXT POISON build by running:\n\n GIT_GETTEXT_POISON= ./t1234-foo.sh\n\nSigned-off-by: SZEDER Gábor \n---\n Makefile | 2 +-\n gettext.c | 9 +++++++--\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex ad880d1fc5..7a165445cd 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -365,7 +365,7 @@ all::\n # Define GETTEXT_POISON if you are debugging the choice of strings marked\n # for translation. In a GETTEXT_POISON build, you can turn all strings marked\n # for translation into gibberish by setting the GIT_GETTEXT_POISON variable\n-# (to any value) in your environment.\n+# to a non-empty value in your environment.\n #\n # Define JSMIN to point to JavaScript minifier that functions as\n # a filter to have gitweb.js minified.\ndiff --git a/gettext.c b/gettext.c\nindex 7272771c8e..a9509a5df3 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -50,8 +50,13 @@ const char *get_preferred_languages(void)\n int use_gettext_poison(void)\n {\n \tstatic int poison_requested = -1;\n-\tif (poison_requested == -1)\n-\t\tpoison_requested = getenv(\"GIT_GETTEXT_POISON\") ? 1 : 0;\n+\tif (poison_requested == -1) {\n+\t\tconst char *v = getenv(\"GIT_GETTEXT_POISON\");\n+\t\tif (v && *v)\n+\t\t\tpoison_requested = 1;\n+\t\telse\n+\t\t\tpoison_requested = 0;\n+\t}\n \treturn poison_requested;\n }\n #endif\n-- \n2.19.1.681.g6bd79da3f5\n\n"},{"id":"361184","messageId":"20181022202241.18629-4-szeder.dev@gmail.com","threadId":"49637","inReplyTo":"20181022202241.18629-1-szeder.dev@gmail.com","subject":"[PATCH 3/8] lib-rebase: loosen GETTEXT_POISON check in fake editor","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-22T20:22:36Z","receivedAt":"2018-10-22T20:23:35Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The fake editor script created by 't/lib-rebase.sh' recognizes GETTEXT\nPOSION output when the first line of the file to be edited consists\nsolely of the GETTEXT POISON magic string as a comment. However, a\nlater patch will include additional text after that magic string, so\nthat check won't work anymore.\n\nSo instead of expecting an exact match in the first line, check\nwhether there are any lines starting with the commented out magic\nstring.\n\nSigned-off-by: SZEDER Gábor \n---\n t/lib-rebase.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-rebase.sh b/t/lib-rebase.sh\nindex 25a77ee5cb..530f8ec0a8 100644\n--- a/t/lib-rebase.sh\n+++ b/t/lib-rebase.sh\n@@ -29,7 +29,7 @@ set_fake_editor () {\n \t*/COMMIT_EDITMSG)\n \t\ttest -z \"$EXPECT_HEADER_COUNT\" ||\n \t\t\ttest \"$EXPECT_HEADER_COUNT\" = \"$(sed -n '1s/^# This is a combination of \\(.*\\) commits\\./\\1/p' < \"$1\")\" ||\n-\t\t\ttest \"# # GETTEXT POISON #\" = \"$(sed -n '1p' < \"$1\")\" ||\n+\t\t\t! grep -q \"^# # GETTEXT POISON #\" ||\n \t\t\texit\n \t\ttest -z \"$FAKE_COMMIT_MESSAGE\" || echo \"$FAKE_COMMIT_MESSAGE\" > \"$1\"\n \t\ttest -z \"$FAKE_COMMIT_AMEND\" || echo \"$FAKE_COMMIT_AMEND\" >> \"$1\"\n-- \n2.19.1.681.g6bd79da3f5\n\n"},{"id":"361185","messageId":"20181022202241.18629-5-szeder.dev@gmail.com","threadId":"49637","inReplyTo":"20181022202241.18629-1-szeder.dev@gmail.com","subject":"[PATCH 4/8] gettext: #ifdef away GETTEXT POISON-related code from _() and Q_()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-22T20:22:37Z","receivedAt":"2018-10-22T20:23:35Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The gettext wrapper functions _() and Q_() contain a GETTEXT\nPOISON-related conditional construct even in non-GETTEXT POISON\nbuilds, though both of those conditions are #define-d to be false\nalready at compile time. Both constructs will grow in a later patch,\nusing a GETTEXT POISON-specific enum type and calling another GETTEXT\nPOISON-specific function.\n\nPrepare for those future changes and hide the GETTEXT POISON-related\nparts of those functions behind an #ifdef.\n\nSigned-off-by: SZEDER Gábor \n---\n gettext.h | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/gettext.h b/gettext.h\nindex 7eee64a34f..c658942f7d 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -43,22 +43,26 @@ static inline int gettext_width(const char *s)\n \n #ifdef GETTEXT_POISON\n extern int use_gettext_poison(void);\n-#else\n-#define use_gettext_poison() 0\n #endif\n \n static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n {\n \tif (!*msgid)\n \t\treturn \"\";\n-\treturn use_gettext_poison() ? \"# GETTEXT POISON #\" : gettext(msgid);\n+#ifdef GETTEXT_POISON\n+\tif (use_gettext_poison())\n+\t\treturn \"# GETTEXT POISON #\";\n+#endif\n+\treturn gettext(msgid);\n }\n \n static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)\n const char *Q_(const char *msgid, const char *plu, unsigned long n)\n {\n+#ifdef GETTEXT_POISON\n \tif (use_gettext_poison())\n \t\treturn \"# GETTEXT POISON #\";\n+#endif\n \treturn ngettext(msgid, plu, n);\n }\n \n-- \n2.19.1.681.g6bd79da3f5\n\n"},{"id":"361186","messageId":"20181022202241.18629-6-szeder.dev@gmail.com","threadId":"49637","inReplyTo":"20181022202241.18629-1-szeder.dev@gmail.com","subject":"[PATCH 5/8] gettext: put \"# GETTEXT POISON #\" string literal into a macro","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-22T20:22:38Z","receivedAt":"2018-10-22T20:23:37Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The \"# GETTEXT POISON #\" string literal is currently used in two\nfunctions in 'gettext.h'. A later patch will add a function to\n'gettext.c' using this string literal as well.\n\nAvoid this duplication and put that string literal into a macro which\nis only available in GETTEXT POISON builds.\n\nSigned-off-by: SZEDER Gábor \n---\n gettext.h | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/gettext.h b/gettext.h\nindex c658942f7d..8e279622f6 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -43,6 +43,8 @@ static inline int gettext_width(const char *s)\n \n #ifdef GETTEXT_POISON\n extern int use_gettext_poison(void);\n+\n+#define GETTEXT_POISON_MAGIC \"# GETTEXT POISON #\"\n #endif\n \n static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n@@ -51,7 +53,7 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n \t\treturn \"\";\n #ifdef GETTEXT_POISON\n \tif (use_gettext_poison())\n-\t\treturn \"# GETTEXT POISON #\";\n+\t\treturn GETTEXT_POISON_MAGIC;\n #endif\n \treturn gettext(msgid);\n }\n@@ -61,7 +63,7 @@ const char *Q_(const char *msgid, const char *plu, unsigned long n)\n {\n #ifdef GETTEXT_POISON\n \tif (use_gettext_poison())\n-\t\treturn \"# GETTEXT POISON #\";\n+\t\treturn GETTEXT_POISON_MAGIC;\n #endif\n \treturn ngettext(msgid, plu, n);\n }\n-- \n2.19.1.681.g6bd79da3f5\n\n"},{"id":"361187","messageId":"20181022202241.18629-7-szeder.dev@gmail.com","threadId":"49637","inReplyTo":"20181022202241.18629-1-szeder.dev@gmail.com","subject":"[PATCH 6/8] gettext: use an enum for the mode of GETTEXT POISONing","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-22T20:22:39Z","receivedAt":"2018-10-22T20:23:39Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The next patch will add a different mode of GETTEXT POISON-ing,\ntherefore named constants will be better than magic numbers.\n\nSigned-off-by: SZEDER Gábor \n---\n gettext.c | 12 ++++++------\n gettext.h | 12 +++++++++---\n 2 files changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/gettext.c b/gettext.c\nindex a9509a5df3..c50d1e0377 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -47,17 +47,17 @@ const char *get_preferred_languages(void)\n }\n \n #ifdef GETTEXT_POISON\n-int use_gettext_poison(void)\n+enum poison_mode use_gettext_poison(void)\n {\n-\tstatic int poison_requested = -1;\n-\tif (poison_requested == -1) {\n+\tstatic enum poison_mode poison_mode = poison_mode_uninitialized;\n+\tif (poison_mode == poison_mode_uninitialized) {\n \t\tconst char *v = getenv(\"GIT_GETTEXT_POISON\");\n \t\tif (v && *v)\n-\t\t\tpoison_requested = 1;\n+\t\t\tpoison_mode = poison_mode_default;\n \t\telse\n-\t\t\tpoison_requested = 0;\n+\t\t\tpoison_mode = poison_mode_none;\n \t}\n-\treturn poison_requested;\n+\treturn poison_mode;\n }\n #endif\n \ndiff --git a/gettext.h b/gettext.h\nindex 8e279622f6..fcb6bfaa2c 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -42,7 +42,13 @@ static inline int gettext_width(const char *s)\n #endif\n \n #ifdef GETTEXT_POISON\n-extern int use_gettext_poison(void);\n+enum poison_mode {\n+\tpoison_mode_uninitialized = -1,\n+\tpoison_mode_none = 0,\n+\tpoison_mode_default\n+};\n+\n+extern enum poison_mode use_gettext_poison(void);\n \n #define GETTEXT_POISON_MAGIC \"# GETTEXT POISON #\"\n #endif\n@@ -52,7 +58,7 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n \tif (!*msgid)\n \t\treturn \"\";\n #ifdef GETTEXT_POISON\n-\tif (use_gettext_poison())\n+\tif (use_gettext_poison() == poison_mode_default)\n \t\treturn GETTEXT_POISON_MAGIC;\n #endif\n \treturn gettext(msgid);\n@@ -62,7 +68,7 @@ static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)\n const char *Q_(const char *msgid, const char *plu, unsigned long n)\n {\n #ifdef GETTEXT_POISON\n-\tif (use_gettext_poison())\n+\tif (use_gettext_poison() == poison_mode_default)\n \t\treturn GETTEXT_POISON_MAGIC;\n #endif\n \treturn ngettext(msgid, plu, n);\n-- \n2.19.1.681.g6bd79da3f5\n\n"},{"id":"361188","messageId":"20181022202241.18629-8-szeder.dev@gmail.com","threadId":"49637","inReplyTo":"20181022202241.18629-1-szeder.dev@gmail.com","subject":"[PATCH 7/8] gettext: introduce GIT_GETTEXT_POISON=scrambled","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-22T20:22:40Z","receivedAt":"2018-10-22T20:23:41Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Sometimes tests run with GETTEXT POISON fail because of a reason other\nthan a translated string that should not have been translated. In\nsuch a case an error message from a git command in the test's verbose\noutput is usually, well, less than helpful:\n\n error: # GETTEXT POISON #\n\nor\n\n fatal: # GETTEXT POISON #: No such file or directory\n\nIt's especially annoying on those rare occasions when a heisenbug\ndecides it's a good time to suddenly reveal its presence during a\nGETTEXT POISON test run, and all we get is an error message like these\n(yes, I did actually see both of the above error messages only once).\n\nMake builtin commands' GETTEXT POISON-ed error messages more useful\nfor debugging failures by introducing a new mode of poisoning: if\n$GIT_GETTEXT_POISON is set to 'scrambled', then include the original\nuntranslated message after that \"# GETTEXT_POISON #\" string in a\nscrambled form, interspersing a '.' after each character. This way\nthe messages will remain gibberish enough for machine consumption as\nthey were before, but at the same time they will be relatively easily\nlegible for humans. Take extra care to preserve printf() format\nconversion specifiers unaltered when inserting those dots.\n\nLeave 'git-sh-i18n.sh' unchanged, because translatable messages in\nscripts often include shell variables, and they could (though\ncurrently they don't) include printf format specifiers, parameter\nexpansions, command substitutions and whatnot, too. Dealing with\nthose in a shell script would be too much hassle without its worth.\n\nThere is an additional benefit: as this change considerably increases\nthe size of translated messages, it could detect cases when we try to\nformat a translated string into a too small buffer. E.g. this change\napplied on old versions causes test failures because of the bug that\nwas fixed in 2cfa83574c (bisect_next_all: convert xsnprintf to\nxstrfmt, 2017-02-16).\n\n[TODO: Fallout?\n A 'printf(_(\"foo: %s\"), var);' call includes the contents of\n 'var' unscrambled in the output. Could that hide the\n translation of a string that should not have been translated?\n I'm afraid yes: to check the output of that printf() a sloppy\n test could do:\n\n git plumbing-cmd >out && grep \"var's content\" out\n\n which would fail in a regular GETTEXT_POISON test run, but\n would succeed in a scrambled test run. Does this matter in\n practice, do we care at all?\n\n Does gettext_scramble() need a FORMAT_PRESERVING annotation?\n Seems to work fine without it so far...]\n\nSigned-off-by: SZEDER Gábor \n---\n gettext.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++---\n gettext.h | 11 +++++++++--\n 2 files changed, 60 insertions(+), 5 deletions(-)\n\ndiff --git a/gettext.c b/gettext.c\nindex c50d1e0377..8ba7fd0bea 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -52,13 +52,61 @@ enum poison_mode use_gettext_poison(void)\n \tstatic enum poison_mode poison_mode = poison_mode_uninitialized;\n \tif (poison_mode == poison_mode_uninitialized) {\n \t\tconst char *v = getenv(\"GIT_GETTEXT_POISON\");\n-\t\tif (v && *v)\n-\t\t\tpoison_mode = poison_mode_default;\n-\t\telse\n+\t\tif (v && *v) {\n+\t\t\tif (!strcmp(v, \"scrambled\"))\n+\t\t\t\tpoison_mode = poison_mode_scrambled;\n+\t\t\telse\n+\t\t\t\tpoison_mode = poison_mode_default;\n+\t\t} else\n \t\t\tpoison_mode = poison_mode_none;\n \t}\n \treturn poison_mode;\n }\n+\n+static int conversion_specifier_len(const char *s)\n+{\n+\tconst char printf_conversion_specifiers[] = \"diouxXeEfFgGaAcsCSpnm%\";\n+\tconst char *format_end;\n+\n+\tif (*s != '%')\n+\t\treturn 0;\n+\n+\tformat_end = strpbrk(s + 1, printf_conversion_specifiers);\n+\tif (format_end)\n+\t\treturn format_end - s;\n+\telse\n+\t\treturn 0;\n+}\n+\n+const char *gettext_scramble(const char *msg)\n+{\n+\tstruct strbuf sb;\n+\n+\tstrbuf_init(&sb,\n+\t\t /* \"# GETTEXT_POISON #\" + ' ' + \"m.e.s.s.a.g.e.\" + '\\0' */\n+\t\t strlen(GETTEXT_POISON_MAGIC) + 1 + 2 * strlen(msg) + 1);\n+\n+\tstrbuf_addch(&sb, ' ');\n+\twhile (*msg) {\n+\t\tif (*msg == '\\n') {\n+\t\t\tstrbuf_addch(&sb, *(msg++));\n+\t\t\tcontinue;\n+\t\t} else if (*msg == '%') {\n+\t\t\tint spec_len = conversion_specifier_len(msg);\n+\t\t\tif (spec_len) {\n+\t\t\t\tstrbuf_add(&sb, msg, spec_len);\n+\t\t\t\tmsg += spec_len;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\n+\t\tstrbuf_addch(&sb, *(msg++));\n+\t\tstrbuf_addch(&sb, '.');\n+\t}\n+\n+\t/* This will be leaked... */\n+\treturn strbuf_detach(&sb, NULL);\n+}\n #endif\n \n #ifndef NO_GETTEXT\ndiff --git a/gettext.h b/gettext.h\nindex fcb6bfaa2c..d21346d9fa 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -45,10 +45,12 @@ static inline int gettext_width(const char *s)\n enum poison_mode {\n \tpoison_mode_uninitialized = -1,\n \tpoison_mode_none = 0,\n-\tpoison_mode_default\n+\tpoison_mode_default,\n+\tpoison_mode_scrambled\n };\n \n extern enum poison_mode use_gettext_poison(void);\n+extern const char *gettext_scramble(const char *msg);\n \n #define GETTEXT_POISON_MAGIC \"# GETTEXT POISON #\"\n #endif\n@@ -60,6 +62,8 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n #ifdef GETTEXT_POISON\n \tif (use_gettext_poison() == poison_mode_default)\n \t\treturn GETTEXT_POISON_MAGIC;\n+\telse if (use_gettext_poison() == poison_mode_scrambled)\n+\t\treturn gettext_scramble(gettext(msgid));\n #endif\n \treturn gettext(msgid);\n }\n@@ -67,11 +71,14 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)\n const char *Q_(const char *msgid, const char *plu, unsigned long n)\n {\n+\tconst char *msg = ngettext(msgid, plu, n);\n #ifdef GETTEXT_POISON\n \tif (use_gettext_poison() == poison_mode_default)\n \t\treturn GETTEXT_POISON_MAGIC;\n+\telse if (use_gettext_poison() == poison_mode_scrambled)\n+\t\treturn gettext_scramble(msg);\n #endif\n-\treturn ngettext(msgid, plu, n);\n+\treturn msg;\n }\n \n /* Mark msgid for translation but do not translate it. */\n-- \n2.19.1.681.g6bd79da3f5\n\n"},{"id":"361189","messageId":"20181022202241.18629-9-szeder.dev@gmail.com","threadId":"49637","inReplyTo":"20181022202241.18629-1-szeder.dev@gmail.com","subject":"[PATCH 8/8] travis-ci: run GETTEXT POISON build job in scrambled mode, too","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-10-22T20:22:41Z","receivedAt":"2018-10-22T20:23:42Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Run the test suite twice in the GETTEXT POISON build: first with\nGIT_GETTEXT_POISON=scrambled and then with \"regular\" poisoning, to see\nwhether the scrambled mode hid any mis-translations.\n\nSigned-off-by: SZEDER Gábor \n---\n ci/lib-travisci.sh | 1 +\n ci/run-build-and-tests.sh | 10 +++++++---\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh\nindex 109ef280da..fdfa4e035b 100755\n--- a/ci/lib-travisci.sh\n+++ b/ci/lib-travisci.sh\n@@ -122,5 +122,6 @@ osx-clang|osx-gcc)\n \t;;\n GETTEXT_POISON)\n \texport GETTEXT_POISON=YesPlease\n+\texport GIT_GETTEXT_POISON=scrambled\n \t;;\n esac\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex 3735ce413f..74ba05e152 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -9,10 +9,14 @@ ln -s \"$cache_dir/.prove\" t/.prove\n \n make --jobs=2\n make --quiet test\n-if test \"$jobname\" = \"linux-gcc\"\n-then\n+case \"$jobname\" in\n+linux-gcc)\n \tGIT_TEST_SPLIT_INDEX=YesPlease make --quiet test\n-fi\n+\t;;\n+GETTEXT_POISON)\n+\tGIT_GETTEXT_POISON=YesPlease make --quiet test\n+\t;;\n+esac\n \n check_unignored_build_artifacts\n \n-- \n2.19.1.681.g6bd79da3f5\n\n"},{"id":"361190","messageId":"87a7n5d8dv.fsf@evledraar.gmail.com","threadId":"49637","inReplyTo":"20181022202241.18629-3-szeder.dev@gmail.com","subject":"Re: [PATCH 2/8] gettext: don't poison if GIT_GETTEXT_POISON is set but empty","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-22T20:38:36Z","receivedAt":"2018-10-22T20:38:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 22 2018, SZEDER Gábor wrote:\n\n> This allows us to run test with non-GETTEXT POISON-ed behavior even in\n> a GETTEXT POISON build by running:\n>\n> GIT_GETTEXT_POISON= ./t1234-foo.sh\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> Makefile | 2 +-\n> gettext.c | 9 +++++++--\n> 2 files changed, 8 insertions(+), 3 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index ad880d1fc5..7a165445cd 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -365,7 +365,7 @@ all::\n> # Define GETTEXT_POISON if you are debugging the choice of strings marked\n> # for translation. In a GETTEXT_POISON build, you can turn all strings marked\n> # for translation into gibberish by setting the GIT_GETTEXT_POISON variable\n> -# (to any value) in your environment.\n> +# to a non-empty value in your environment.\n> #\n> # Define JSMIN to point to JavaScript minifier that functions as\n> # a filter to have gitweb.js minified.\n> diff --git a/gettext.c b/gettext.c\n> index 7272771c8e..a9509a5df3 100644\n> --- a/gettext.c\n> +++ b/gettext.c\n> @@ -50,8 +50,13 @@ const char *get_preferred_languages(void)\n> int use_gettext_poison(void)\n> {\n> \tstatic int poison_requested = -1;\n> -\tif (poison_requested == -1)\n> -\t\tpoison_requested = getenv(\"GIT_GETTEXT_POISON\") ? 1 : 0;\n> +\tif (poison_requested == -1) {\n> +\t\tconst char *v = getenv(\"GIT_GETTEXT_POISON\");\n> +\t\tif (v && *v)\n> +\t\t\tpoison_requested = 1;\n> +\t\telse\n> +\t\t\tpoison_requested = 0;\n> +\t}\n> \treturn poison_requested;\n> }\n> #endif\n\nFixing this is good. But when the initial support for conditional runs\nwas added in 309552295a (\"i18n: do not poison translations unless\nGIT_GETTEXT_POISON envvar is set\", 2011-02-22) we didn't have the\nconvention of using git_env_bool() for these, which a few recent\nfollow-up patches have also done.\n\nSo let's just use git_env_bool() here. It's less code, and consistent\nwith the rest. See 4c2db93807 (\"read-cache.c: make $GIT_TEST_SPLIT_INDEX\nboolean\", 2018-04-14). Also makes sense to document this in the \"Running\ntests with special setups\" setup that patch added.\n"},{"id":"361199","messageId":"nycvar.QRO.7.76.6.1810222252250.4546@tvgsbejvaqbjf.bet","threadId":"49637","inReplyTo":"20181022153633.31757-1-pclouds@gmail.com","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-22T20:52:54Z","receivedAt":"2018-10-22T20:52:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Mon, 22 Oct 2018, Nguyễn Thái Ngọc Duy wrote:\n\n> The current gettext() function just replaces all strings with\n> '# GETTEXT POISON #' including format strings and hides the things\n> that we should be allowed to grep (like branch names, or some other\n> codes) even when gettext is poisoned.\n> \n> This patch implements the poisoned _() with a universal and totally\n> legit language called Ook [1]. We could actually grep stuff even in\n> with this because format strings are preserved.\n> \n> Long term, we could implement an \"ook translator\" for test_i18ngrep\n> and friends so that they translate English to Ook, allowing us to\n> match full text while making sure the text in the code is still marked\n> for translation.\n> \n> [1] https://en.wikipedia.org/wiki/Unseen_University#Librarian\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> This started out as something fun to do while running the test suite\n> last weekend. But it turns out actually working! If this patch ends\n> up in git.git, the Librarian would be so proud!\n\nCute.\nDscho\n\n> \n> gettext.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++\n> gettext.h | 7 ++++---\n> t/lib-rebase.sh | 2 +-\n> 3 files changed, 59 insertions(+), 4 deletions(-)\n> \n> diff --git a/gettext.c b/gettext.c\n> index 7272771c8e..29901e1ddd 100644\n> --- a/gettext.c\n> +++ b/gettext.c\n> @@ -56,6 +56,60 @@ int use_gettext_poison(void)\n> }\n> #endif\n> \n> +const char *gettext_poison(const char *msgid)\n> +{\n> +\t/*\n> +\t * gettext() returns a string that is always valid. We would\n> +\t * need a hash map for that but let's stay simple and keep the\n> +\t * last 64 gettext() results. Should be more than enough.\n> +\t */\n> +\tstatic char *bufs[64];\n> +\tstatic int i;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tchar *buf;\n> +\tconst char *p;\n> +\tconst char *type_specifiers = \"diouxXeEfFgGaAcsCSpnm%\";\n> +\n> +\tif (!strchr(msgid, '%'))\n> +\t\treturn \"Eek!\";\n> +\n> +\tp = msgid;\n> +\twhile (*p) {\n> +\t\tconst char *type;\n> +\t\tswitch (*p) {\n> +\t\tcase '%':\n> +\t\t\t/*\n> +\t\t\t * No strict parsing. We simply look for the end of a\n> +\t\t\t * format string\n> +\t\t\t */\n> +\t\t\ttype = p + 1;\n> +\t\t\twhile (*type && !strchr(type_specifiers, *type))\n> +\t\t\t\ttype++;\n> +\t\t\tif (*type)\n> +\t\t\t\ttype++;\n> +\t\t\tstrbuf_add(&sb, p, (int)(type - p));\n> +\t\t\tp = type;\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tif (!isalpha(*p)) {\n> +\t\t\t\tstrbuf_addch(&sb, *p);\n> +\t\t\t\tp++;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t\tif (isupper(*p))\n> +\t\t\t\tstrbuf_addstr(&sb, \"Ook\");\n> +\t\t\telse\n> +\t\t\t\tstrbuf_addstr(&sb, \"ook\");\n> +\t\t\twhile (isalpha(*p))\n> +\t\t\t\tp++;\n> +\t\t}\n> +\t}\n> +\tbuf = bufs[(i++) % ARRAY_SIZE(bufs)];\n> +\tfree(buf);\n> +\tbuf = strbuf_detach(&sb, NULL);\n> +\treturn buf;\n> +}\n> +\n> #ifndef NO_GETTEXT\n> static int test_vsnprintf(const char *fmt, ...)\n> {\n> diff --git a/gettext.h b/gettext.h\n> index 7eee64a34f..dc9851a06a 100644\n> --- a/gettext.h\n> +++ b/gettext.h\n> @@ -41,8 +41,9 @@ static inline int gettext_width(const char *s)\n> }\n> #endif\n> \n> +const char *gettext_poison(const char *);\n> #ifdef GETTEXT_POISON\n> -extern int use_gettext_poison(void);\n> +int use_gettext_poison(void);\n> #else\n> #define use_gettext_poison() 0\n> #endif\n> @@ -51,14 +52,14 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n> {\n> \tif (!*msgid)\n> \t\treturn \"\";\n> -\treturn use_gettext_poison() ? \"# GETTEXT POISON #\" : gettext(msgid);\n> +\treturn use_gettext_poison() ? gettext_poison(msgid) : gettext(msgid);\n> }\n> \n> static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)\n> const char *Q_(const char *msgid, const char *plu, unsigned long n)\n> {\n> \tif (use_gettext_poison())\n> -\t\treturn \"# GETTEXT POISON #\";\n> +\t\treturn gettext_poison(msgid);\n> \treturn ngettext(msgid, plu, n);\n> }\n> \n> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh\n> index 2ca9fb69d6..1e8440e935 100644\n> --- a/t/lib-rebase.sh\n> +++ b/t/lib-rebase.sh\n> @@ -29,7 +29,7 @@ set_fake_editor () {\n> \t*/COMMIT_EDITMSG)\n> \t\ttest -z \"$EXPECT_HEADER_COUNT\" ||\n> \t\t\ttest \"$EXPECT_HEADER_COUNT\" = \"$(sed -n '1s/^# This is a combination of \\(.*\\) commits\\./\\1/p' < \"$1\")\" ||\n> -\t\t\ttest \"# # GETTEXT POISON #\" = \"$(sed -n '1p' < \"$1\")\" ||\n> +\t\t\ttest \"$EXPECT_HEADER_COUNT\" = \"$(sed -n '1s/^# Ook ook ook ook ook \\(.*\\) ook\\./\\1/p' < \"$1\")\" ||\n> \t\t\texit\n> \t\ttest -z \"$FAKE_COMMIT_MESSAGE\" || echo \"$FAKE_COMMIT_MESSAGE\" > \"$1\"\n> \t\ttest -z \"$FAKE_COMMIT_AMEND\" || echo \"$FAKE_COMMIT_AMEND\" >> \"$1\"\n> -- \n> 2.19.1.647.g708186aaf9\n> \n> "},{"id":"361200","messageId":"878t2pd6yu.fsf@evledraar.gmail.com","threadId":"49637","inReplyTo":"20181022153633.31757-1-pclouds@gmail.com","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-22T21:09:13Z","receivedAt":"2018-10-22T21:09:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 22 2018, Nguyễn Thái Ngọc Duy wrote:\n\n> The current gettext() function just replaces all strings with\n> '# GETTEXT POISON #' including format strings and hides the things\n> that we should be allowed to grep (like branch names, or some other\n> codes) even when gettext is poisoned.\n>\n> This patch implements the poisoned _() with a universal and totally\n> legit language called Ook [1]. We could actually grep stuff even in\n> with this because format strings are preserved.\n>\n> Long term, we could implement an \"ook translator\" for test_i18ngrep\n> and friends so that they translate English to Ook, allowing us to\n> match full text while making sure the text in the code is still marked\n> for translation.\n\nReplying to both this patch, and SZEDER's series for brevity. Thanks\nboth for working on this. It's obvious that this stuff needs some\npolishing, and it's great that's being done, and sorry for my part of\nhaving left this in the current state.\n\na)\n\nBoth this patch and SZEDER's 7/8 are addressing the issue that the\npoison mode doesn't preserve format specifiers.\n\nI haven't tried to dig this up in the list archive, and maybe it only\nexists in my mind, but I seem to remember that this was explicitly\ndiscussed as a goal at the time.\n\nI.e. we were expecting the lack of this to break tests, and that was\nconsidered a feature as it would spot plumbing messages we shouldn't be\ntranslating.\n\nHowever, it's my opinion that this was just a bad idea to begin with,\nI've CC'd a couple of prolific markers of i18n from my log grepping (and\nfeel free to add more) to see if they disagre.\n\nI think it probably helped me a bit in the beginning when i18n was\nbootstrapping and there was a *lot* to mark for translation, but we've\nlong since stabilized and aren't doing that anymore, so it's much easier\nto just review the patches to see if they translate plumbing.\n\nAll of which is to say that I think something like your patch here is\nfine to just accept, and the only improvement I'd suggest is some note\nin the commit message saying that this was always intentional, but\nnobody can name a case where it helped, so let's just drop it.\n\nIn SZEDER's case that we shouldn't have GIT_GETTEXT_POISON=scrambled,\nlet's just make it boolean and make \"scrambled\" (i.e. preserving format\nspecifiecs) the default.\n\nb)\n\nSZEDER's series, and your patch (although it's smaller) still preserve\nthe notion that there's such a thing as a GETTEXT_POISON *build* and you\nactually need to compile git with that flag to make use of it.\n\nThis, as I recall, was just paranoia on my part back in 2010/2011. I\nwanted to be able to present a version of this i18n stuff where people\nwho didn't like it could be assured that if they didn't opt-in to it\nthey wouldn't get any of the code (sans a few trivial and obviously\ncorrect wrapper functions).\n\nSo I think the only reason to keep it compile-time is performance, but I\ndon't think that matters. It's not like we're printing gigabytes of _()\nformatted output. Everything where formatting matters is plumbing which\ndoesn't use this API. These messages are always for human consumption.\n\nSo shouldn't we just drop this notion of GETTEXT_POISON at compile-time\nentirely, and make GIT_GETTEXT_POISON= a test flag that all\nbuilds of git are aware of? I think so.\n"},{"id":"361206","messageId":"875zxtd59e.fsf@evledraar.gmail.com","threadId":"49637","inReplyTo":"878t2pd6yu.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-22T21:46:05Z","receivedAt":"2018-10-22T21:46:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 22 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> On Mon, Oct 22 2018, Nguyễn Thái Ngọc Duy wrote:\n>\n>> The current gettext() function just replaces all strings with\n>> '# GETTEXT POISON #' including format strings and hides the things\n>> that we should be allowed to grep (like branch names, or some other\n>> codes) even when gettext is poisoned.\n>>\n>> This patch implements the poisoned _() with a universal and totally\n>> legit language called Ook [1]. We could actually grep stuff even in\n>> with this because format strings are preserved.\n>>\n>> Long term, we could implement an \"ook translator\" for test_i18ngrep\n>> and friends so that they translate English to Ook, allowing us to\n>> match full text while making sure the text in the code is still marked\n>> for translation.\n>\n> Replying to both this patch, and SZEDER's series for brevity. Thanks\n> both for working on this. It's obvious that this stuff needs some\n> polishing, and it's great that's being done, and sorry for my part of\n> having left this in the current state.\n>\n> a)\n>\n> Both this patch and SZEDER's 7/8 are addressing the issue that the\n> poison mode doesn't preserve format specifiers.\n>\n> I haven't tried to dig this up in the list archive, and maybe it only\n> exists in my mind, but I seem to remember that this was explicitly\n> discussed as a goal at the time.\n>\n> I.e. we were expecting the lack of this to break tests, and that was\n> considered a feature as it would spot plumbing messages we shouldn't be\n> translating.\n>\n> However, it's my opinion that this was just a bad idea to begin with,\n> I've CC'd a couple of prolific markers of i18n from my log grepping (and\n> feel free to add more) to see if they disagre.\n>\n> I think it probably helped me a bit in the beginning when i18n was\n> bootstrapping and there was a *lot* to mark for translation, but we've\n> long since stabilized and aren't doing that anymore, so it's much easier\n> to just review the patches to see if they translate plumbing.\n>\n> All of which is to say that I think something like your patch here is\n> fine to just accept, and the only improvement I'd suggest is some note\n> in the commit message saying that this was always intentional, but\n> nobody can name a case where it helped, so let's just drop it.\n>\n> In SZEDER's case that we shouldn't have GIT_GETTEXT_POISON=scrambled,\n> let's just make it boolean and make \"scrambled\" (i.e. preserving format\n> specifiecs) the default.\n\n[...]\n\n> b)\n>\n> SZEDER's series, and your patch (although it's smaller) still preserve\n> the notion that there's such a thing as a GETTEXT_POISON *build* and you\n> actually need to compile git with that flag to make use of it.\n>\n> This, as I recall, was just paranoia on my part back in 2010/2011. I\n> wanted to be able to present a version of this i18n stuff where people\n> who didn't like it could be assured that if they didn't opt-in to it\n> they wouldn't get any of the code (sans a few trivial and obviously\n> correct wrapper functions).\n>\n> So I think the only reason to keep it compile-time is performance, but I\n> don't think that matters. It's not like we're printing gigabytes of _()\n> formatted output. Everything where formatting matters is plumbing which\n> doesn't use this API. These messages are always for human consumption.\n>\n> So shouldn't we just drop this notion of GETTEXT_POISON at compile-time\n> entirely, and make GIT_GETTEXT_POISON= a test flag that all\n> builds of git are aware of? I think so.\n\nSomething like this, untested except I compiled it and ran one test\nwith/without GIT_GETTEXT_POISON, so it may have bugs:\n\n Makefile | 10 +---------\n gettext.c | 4 +---\n gettext.h | 4 ----\n po/README | 13 ++++---------\n t/README | 5 +++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/test-lib-functions.sh | 8 ++++----\n t/test-lib.sh | 12 +++---------\n 9 files changed, 20 insertions(+), 38 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex d18ab0fe78..78190ee9d9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -362,11 +362,6 @@ all::\n # Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the\n # user.\n #\n-# Define GETTEXT_POISON if you are debugging the choice of strings marked\n-# for translation. In a GETTEXT_POISON build, you can turn all strings marked\n-# for translation into gibberish by setting the GIT_GETTEXT_POISON variable\n-# (to any value) in your environment.\n-#\n # Define JSMIN to point to JavaScript minifier that functions as\n # a filter to have gitweb.js minified.\n #\n@@ -714,6 +709,7 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o\n TEST_BUILTINS_OBJS += test-dump-untracked-cache.o\n TEST_BUILTINS_OBJS += test-example-decorate.o\n TEST_BUILTINS_OBJS += test-genrandom.o\n+TEST_BUILTINS_OBJS += test-gettext-poison.o\n TEST_BUILTINS_OBJS += test-hashmap.o\n TEST_BUILTINS_OBJS += test-index-version.o\n TEST_BUILTINS_OBJS += test-json-writer.o\n@@ -1439,9 +1435,6 @@ endif\n ifdef NO_SYMLINK_HEAD\n \tBASIC_CFLAGS += -DNO_SYMLINK_HEAD\n endif\n-ifdef GETTEXT_POISON\n-\tBASIC_CFLAGS += -DGETTEXT_POISON\n-endif\n ifdef NO_GETTEXT\n \tBASIC_CFLAGS += -DNO_GETTEXT\n \tUSE_GETTEXT_SCHEME ?= fallthrough\n@@ -2591,7 +2584,6 @@ ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT\n \t@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@+\n endif\n \t@echo NO_GETTEXT=\\''$(subst ','\\'',$(subst ','\\'',$(NO_GETTEXT)))'\\' >>$@+\n-\t@echo GETTEXT_POISON=\\''$(subst ','\\'',$(subst ','\\'',$(GETTEXT_POISON)))'\\' >>$@+\n ifdef GIT_PERF_REPEAT_COUNT\n \t@echo GIT_PERF_REPEAT_COUNT=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_PERF_REPEAT_COUNT)))'\\' >>$@+\n endif\ndiff --git a/gettext.c b/gettext.c\nindex 7272771c8e..d71e394284 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -46,15 +46,13 @@ const char *get_preferred_languages(void)\n \treturn NULL;\n }\n\n-#ifdef GETTEXT_POISON\n int use_gettext_poison(void)\n {\n \tstatic int poison_requested = -1;\n \tif (poison_requested == -1)\n-\t\tpoison_requested = getenv(\"GIT_GETTEXT_POISON\") ? 1 : 0;\n+\t\tpoison_requested = git_env_bool(\"GIT_GETTEXT_POISON\", 0);\n \treturn poison_requested;\n }\n-#endif\n\n #ifndef NO_GETTEXT\n static int test_vsnprintf(const char *fmt, ...)\ndiff --git a/gettext.h b/gettext.h\nindex 7eee64a34f..4c492d9f57 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -41,11 +41,7 @@ static inline int gettext_width(const char *s)\n }\n #endif\n\n-#ifdef GETTEXT_POISON\n extern int use_gettext_poison(void);\n-#else\n-#define use_gettext_poison() 0\n-#endif\n\n static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n {\ndiff --git a/po/README b/po/README\nindex fef4c0f0b5..9dad277e33 100644\n--- a/po/README\n+++ b/po/README\n@@ -289,16 +289,11 @@ something in the test suite might still depend on the US English\n version of the strings, e.g. to grep some error message or other\n output.\n\n-To smoke out issues like these Git can be compiled with gettext poison\n-support, at the top-level:\n+To smoke out issues like these Git tested with a translation mode that\n+emits gibberish on every call to gettext. To use it run the test suite\n+with it, e.g.:\n\n- make GETTEXT_POISON=YesPlease\n-\n-That'll give you a git which emits gibberish on every call to\n-gettext. It's obviously not meant to be installed, but you should run\n-the test suite with it:\n-\n- cd t && prove -j 9 ./t[0-9]*.sh\n+ cd t && GIT_GETTEXT_POISON=true prove -j 9 ./t[0-9]*.sh\n\n If tests break with it you should inspect them manually and see if\n what you're translating is sane, i.e. that you're not translating\ndiff --git a/t/README b/t/README\nindex 8847489640..b3b2fa0b11 100644\n--- a/t/README\n+++ b/t/README\n@@ -301,6 +301,11 @@ that cannot be easily covered by a few specific test cases. These\n could be enabled by running the test suite with correct GIT_TEST_\n environment set.\n\n+GIT_GETTEXT_POISON= turns all strings marked for translation\n+into gibberish. Used for spotting those tests that need to be marked\n+with a C_LOCALE_OUTPUT prerequisite when adding more strings for\n+translation. See \"Testing marked strings\" in po/README for details.\n+\n GIT_TEST_SPLIT_INDEX= forces split-index mode on the whole\n test suite. Accept any boolean values that are accepted by git-config.\n\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 6b5836dc1b..3e75672a37 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -19,6 +19,7 @@ static struct test_cmd cmds[] = {\n \t{ \"dump-untracked-cache\", cmd__dump_untracked_cache },\n \t{ \"example-decorate\", cmd__example_decorate },\n \t{ \"genrandom\", cmd__genrandom },\n+\t{ \"gettext-poison\", cmd__gettext_poison },\n \t{ \"hashmap\", cmd__hashmap },\n \t{ \"index-version\", cmd__index_version },\n \t{ \"json-writer\", cmd__json_writer },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex e4890566da..04f033b7fc 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -15,6 +15,7 @@ int cmd__dump_split_index(int argc, const char **argv);\n int cmd__dump_untracked_cache(int argc, const char **argv);\n int cmd__example_decorate(int argc, const char **argv);\n int cmd__genrandom(int argc, const char **argv);\n+int cmd__gettext_poison(int argc, const char **argv);\n int cmd__hashmap(int argc, const char **argv);\n int cmd__index_version(int argc, const char **argv);\n int cmd__json_writer(int argc, const char **argv);\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 78d8c3783b..4e8683addd 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -755,16 +755,16 @@ test_cmp_bin() {\n\n # Use this instead of test_cmp to compare files that contain expected and\n # actual output from git commands that can be translated. When running\n-# under GETTEXT_POISON this pretends that the command produced expected\n+# under GIT_GETTEXT_POISON this pretends that the command produced expected\n # results.\n test_i18ncmp () {\n-\ttest -n \"$GETTEXT_POISON\" || test_cmp \"$@\"\n+\t! test_have_prereq C_LOCALE_OUTPUT || test_cmp \"$@\"\n }\n\n # Use this instead of \"grep expected-string actual\" to see if the\n # output from a git command that can be translated either contains an\n # expected string, or does not contain an unwanted one. When running\n-# under GETTEXT_POISON this pretends that the command produced expected\n+# under GIT_GETTEXT_POISON this pretends that the command produced expected\n # results.\n test_i18ngrep () {\n \teval \"last_arg=\\${$#}\"\n@@ -779,7 +779,7 @@ test_i18ngrep () {\n \t\terror \"bug in the test script: too few parameters to test_i18ngrep\"\n \tfi\n\n-\tif test -n \"$GETTEXT_POISON\"\n+\tif ! test_have_prereq C_LOCALE_OUTPUT\n \tthen\n \t\t# pretend success\n \t\treturn 0\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 897e6fcc94..f82d2149a2 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -105,6 +105,7 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $(\"$PERL_PATH\" -e '\n \t\tTRACE\n \t\tDEBUG\n \t\tTEST\n+\t\tGETTEXT_POISON\n \t\t.*_TEST\n \t\tPROVE\n \t\tVALGRIND\n@@ -1104,15 +1105,8 @@ test -n \"$USE_LIBPCRE1\" && test_set_prereq LIBPCRE1\n test -n \"$USE_LIBPCRE2\" && test_set_prereq LIBPCRE2\n test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n\n-# Can we rely on git's output in the C locale?\n-if test -n \"$GETTEXT_POISON\"\n-then\n-\tGIT_GETTEXT_POISON=YesPlease\n-\texport GIT_GETTEXT_POISON\n-\ttest_set_prereq GETTEXT_POISON\n-else\n-\ttest_set_prereq C_LOCALE_OUTPUT\n-fi\n+test_lazy_prereq GETTEXT_POISON 'test-tool gettext-poison'\n+test_lazy_prereq C_LOCALE_OUTPUT '! test-tool gettext-poison'\n\n if test -z \"$GIT_TEST_CHECK_CACHE_TREE\"\n then\n"},{"id":"361227","messageId":"xmqqsh0xtwgb.fsf@gitster-ct.c.googlers.com","threadId":"49637","inReplyTo":"878t2pd6yu.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-22T23:04:20Z","receivedAt":"2018-10-22T23:04:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> So I think the only reason to keep it compile-time is performance, but I\n> don't think that matters. It's not like we're printing gigabytes of _()\n> formatted output. Everything where formatting matters is plumbing which\n> doesn't use this API. These messages are always for human consumption.\n>\n> So shouldn't we just drop this notion of GETTEXT_POISON at compile-time\n> entirely, and make GIT_GETTEXT_POISON= a test flag that all\n> builds of git are aware of? I think so.\n\nHaven't thought things through, but that sounds like a good thing to\naim for. Keep the ideas coming...\n"},{"id":"361263","messageId":"nycvar.QRO.7.76.6.1810231126470.4546@tvgsbejvaqbjf.bet","threadId":"49637","inReplyTo":"878t2pd6yu.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-23T09:30:29Z","receivedAt":"2018-10-23T09:30:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Mon, 22 Oct 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> So I think the only reason to keep it [GETTEXT_POISON] compile-time is\n> performance, but I don't think that matters. It's not like we're\n> printing gigabytes of _() formatted output. Everything where formatting\n> matters is plumbing which doesn't use this API. These messages are\n> always for human consumption.\n\nWell, let's make sure that your impression is correct before going too\nfar. I, too, had the impression that gettext cannot possibly be expensive,\nespecifally in Git for Windows' settings, where we do not even ship\ntranslations. Yet see the commit message of cc5e1bf99247 (gettext: avoid\ninitialization if the locale dir is not present, 2018-04-21):\n\n\tThe runtime of a simple `git.exe version` call on Windows is\n\tcurrently dominated by the gettext setup, adding a whopping ~150ms\n\tto the ~210ms total.\n\nI would be in favor of your change to make this a runtime option, of\ncourse, as long as it does not affect performance greatly (in particular\non Windows, where we fight an uphill battle to make Git faster).\n\nCiao,\nDscho"},{"id":"361270","messageId":"8736sxc6gt.fsf@evledraar.gmail.com","threadId":"49637","inReplyTo":"nycvar.QRO.7.76.6.1810231126470.4546@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-23T10:17:38Z","receivedAt":"2018-10-23T10:17:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 23 2018, Johannes Schindelin wrote:\n\n> Hi Ævar,\n>\n> On Mon, 22 Oct 2018, Ævar Arnfjörð Bjarmason wrote:\n>\n>> So I think the only reason to keep it [GETTEXT_POISON] compile-time is\n>> performance, but I don't think that matters. It's not like we're\n>> printing gigabytes of _() formatted output. Everything where formatting\n>> matters is plumbing which doesn't use this API. These messages are\n>> always for human consumption.\n>\n> Well, let's make sure that your impression is correct before going too\n> far. I, too, had the impression that gettext cannot possibly be expensive,\n> especifally in Git for Windows' settings, where we do not even ship\n> translations. Yet see the commit message of cc5e1bf99247 (gettext: avoid\n> initialization if the locale dir is not present, 2018-04-21):\n>\n> \tThe runtime of a simple `git.exe version` call on Windows is\n> \tcurrently dominated by the gettext setup, adding a whopping ~150ms\n> \tto the ~210ms total.\n>\n> I would be in favor of your change to make this a runtime option, of\n> course, as long as it does not affect performance greatly (in particular\n> on Windows, where we fight an uphill battle to make Git faster).\n\nHow expensive gettext() may or may not be isn't relevant to the\nGETTEXT_POISON compile-time option.\n\nThe effect of what I'm suggesting here, and which my WIP patch in\n<875zxtd59e.fsf@evledraar.gmail.com> implements is that we'd do a\none-time getenv() for each process that prints a _() message that we\naren't doing now, and for each message call a function that would check\na boolean \"are we in poison mode\" static global variable.\n\nPerhaps some of that's expensive on Windows, but given the recent\npatches by Microsoft employees to add GIT_TEST_* env options I assumed\nnot, but in any case it won't have anything to do with how expensive\ngettext may or may not be, you'll already be paying that cost now (or\nnot, with NO_GETTEXT).\n"},{"id":"361281","messageId":"nycvar.QRO.7.76.6.1810231305520.4546@tvgsbejvaqbjf.bet","threadId":"49637","inReplyTo":"8736sxc6gt.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-23T11:07:04Z","receivedAt":"2018-10-23T11:07:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Tue, 23 Oct 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> On Tue, Oct 23 2018, Johannes Schindelin wrote:\n> \n> > On Mon, 22 Oct 2018, Ævar Arnfjörð Bjarmason wrote:\n> >\n> >> So I think the only reason to keep it [GETTEXT_POISON] compile-time is\n> >> performance, but I don't think that matters. It's not like we're\n> >> printing gigabytes of _() formatted output. Everything where formatting\n> >> matters is plumbing which doesn't use this API. These messages are\n> >> always for human consumption.\n> >\n> > Well, let's make sure that your impression is correct before going too\n> > far. I, too, had the impression that gettext cannot possibly be expensive,\n> > especifally in Git for Windows' settings, where we do not even ship\n> > translations. Yet see the commit message of cc5e1bf99247 (gettext: avoid\n> > initialization if the locale dir is not present, 2018-04-21):\n> >\n> > \tThe runtime of a simple `git.exe version` call on Windows is\n> > \tcurrently dominated by the gettext setup, adding a whopping ~150ms\n> > \tto the ~210ms total.\n> >\n> > I would be in favor of your change to make this a runtime option, of\n> > course, as long as it does not affect performance greatly (in particular\n> > on Windows, where we fight an uphill battle to make Git faster).\n> \n> How expensive gettext() may or may not be isn't relevant to the\n> GETTEXT_POISON compile-time option.\n> \n> The effect of what I'm suggesting here, and which my WIP patch in\n> <875zxtd59e.fsf@evledraar.gmail.com> implements is that we'd do a\n> one-time getenv() for each process that prints a _() message that we\n> aren't doing now, and for each message call a function that would check\n> a boolean \"are we in poison mode\" static global variable.\n\nYep, you are right, we are *already* going through _() as a function,\nwhether gettext() is initialized or not. My bad.\n\n> Perhaps some of that's expensive on Windows, but given the recent\n> patches by Microsoft employees to add GIT_TEST_* env options I assumed\n> not, but in any case it won't have anything to do with how expensive\n> gettext may or may not be, you'll already be paying that cost now (or\n> not, with NO_GETTEXT).\n\nIndeed, we want to measure performance better, and that's what those\nenvironment variables are for.\n\nThanks,\nDscho"},{"id":"361289","messageId":"CACsJy8COKa1JZFOSQBDZoJKQ7aJgEvKFmcu0sFa2BDcuJa33Pw@mail.gmail.com","threadId":"49637","inReplyTo":"20181022202241.18629-1-szeder.dev@gmail.com","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-23T14:37:33Z","receivedAt":"2018-10-23T14:38:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Oct 22, 2018 at 10:23 PM SZEDER Gábor wrote:\n> Once upon a time a GETTEXT_POISON build job failed on me, and the\n> error message:\n>\n> error: # GETTEXT POISON #\n>\n> was not particularly useful. Ook wouldn't help with that...\n\nOook?\n\n> So I came up with the following couple of patches that implement a\n> \"scrambled\" format that makes the poisoned output legible for humans\n> but still gibberish for machine consumption (i.e. grep-ing the text\n> part would still fail):\n>\n> error: U.n.a.b.l.e. .t.o. .c.r.e.a.t.e. .'./home/szeder/src/git/t/trash directory.t1404-update-ref-errors/.git/packed-refs...l.o.c.k.'.:. .File exists...\n>\n> I have been running GETTEXT_POISON builds with this series for some\n> months now, but haven't submitted it yet, because I haven't decided\n> yet whether including strings (paths, refs, etc.) in the output as\n> they are is a feature or a flaw.\n\nA feature to me. The only thing that got through and but should not be\ngrep-able is strerror(). But that's orthogonal to this scrambling\nstuff.\n\n> And because it embarrassingly leaks every single translated string... :)\n\nEasy to fix. I hope this means you have no more excuse to not push\nthis forward ;-)\n-- \nDuy\n"},{"id":"361290","messageId":"CACsJy8DEta1f8v5XPK53bMdarWX4TYiQAx0Qz1LCRA0nvZANwg@mail.gmail.com","threadId":"49637","inReplyTo":"20181022202241.18629-8-szeder.dev@gmail.com","subject":"Re: [PATCH 7/8] gettext: introduce GIT_GETTEXT_POISON=scrambled","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-23T14:44:38Z","receivedAt":"2018-10-23T14:45:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Oct 22, 2018 at 10:23 PM SZEDER Gábor wrote:\n> [TODO: Fallout?\n> A 'printf(_(\"foo: %s\"), var);' call includes the contents of\n> 'var' unscrambled in the output. Could that hide the\n> translation of a string that should not have been translated?\n> I'm afraid yes: to check the output of that printf() a sloppy\n> test could do:\n>\n> git plumbing-cmd >out && grep \"var's content\" out\n>\n> which would fail in a regular GETTEXT_POISON test run, but\n> would succeed in a scrambled test run. Does this matter in\n> practice, do we care at all?\n\nIf var is supposed to be translated, _() must have been called before\nthe final string is stored in var and the content is already\nscrambled. Whatever left unscrambled is machine-generated and should\nbe ok to grep, or we have found new strings that should be _() but\nnot.\n\nPS. Another thing I'd like to have is to mark the beginning and end of\na scrambled text. For example, _(\"foo\") produces \"[f.o.o]\" or\nsomething like that. If we have it, it's easier to see \"text legos\"\n(instead of full sentences) that makes translator's life harder. But\nit could interfere with stuff (e.g. some strings must start with '#')\nso let's forget it for now.\n\n>\n> Does gettext_scramble() need a FORMAT_PRESERVING annotation?\n> Seems to work fine without it so far...]\n\nI don't think you can. _() can be called on plain strings that just\nhappen to have '%' in them.\n-- \nDuy\n"},{"id":"361291","messageId":"CACsJy8CX78EbANbv8a354djJaO6dKRpXshHhHJTspJvOSewgpA@mail.gmail.com","threadId":"49637","inReplyTo":"8736sxc6gt.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-23T15:00:52Z","receivedAt":"2018-10-23T15:01:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Oct 23, 2018 at 12:17 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Tue, Oct 23 2018, Johannes Schindelin wrote:\n>\n> > Hi Ævar,\n> >\n> > On Mon, 22 Oct 2018, Ævar Arnfjörð Bjarmason wrote:\n> >\n> >> So I think the only reason to keep it [GETTEXT_POISON] compile-time is\n> >> performance, but I don't think that matters. It's not like we're\n> >> printing gigabytes of _() formatted output. Everything where formatting\n> >> matters is plumbing which doesn't use this API. These messages are\n> >> always for human consumption.\n> >\n> > Well, let's make sure that your impression is correct before going too\n> > far. I, too, had the impression that gettext cannot possibly be expensive,\n> > especifally in Git for Windows' settings, where we do not even ship\n> > translations. Yet see the commit message of cc5e1bf99247 (gettext: avoid\n> > initialization if the locale dir is not present, 2018-04-21):\n> >\n> > The runtime of a simple `git.exe version` call on Windows is\n> > currently dominated by the gettext setup, adding a whopping ~150ms\n> > to the ~210ms total.\n> >\n> > I would be in favor of your change to make this a runtime option, of\n> > course, as long as it does not affect performance greatly (in particular\n> > on Windows, where we fight an uphill battle to make Git faster).\n>\n> How expensive gettext() may or may not be isn't relevant to the\n> GETTEXT_POISON compile-time option.\n\nIf a user requests NO_GETTEXT, they should have zero (or near zero)\ncost related to gettext. Which is true until now (the inline _()\nversion is expanded to pretty much no-op with the exception of NULL\nstring)\n\n> The effect of what I'm suggesting here, and which my WIP patch in\n> <875zxtd59e.fsf@evledraar.gmail.com> implements is that we'd do a\n> one-time getenv() for each process that prints a _() message that we\n> aren't doing now, and for each message call a function that would check\n> a boolean \"are we in poison mode\" static global variable.\n\nJust don't do the getenv() inside _() even if you just do it one time.\ngetenv() may invalidate whatever value from the previous call. I would\nnot want to find out my code broke because of an getenv() inside some\ninnocent _()... And we should still respect NO_GETTEXT, not doing any\nextra work when it's defined.\n-- \nDuy\n"},{"id":"361298","messageId":"871s8gd32p.fsf@evledraar.gmail.com","threadId":"49637","inReplyTo":"CACsJy8CX78EbANbv8a354djJaO6dKRpXshHhHJTspJvOSewgpA@mail.gmail.com","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-23T16:45:34Z","receivedAt":"2018-10-23T16:45:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 23 2018, Duy Nguyen wrote:\n\n> On Tue, Oct 23, 2018 at 12:17 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>>\n>> On Tue, Oct 23 2018, Johannes Schindelin wrote:\n>>\n>> > Hi Ævar,\n>> >\n>> > On Mon, 22 Oct 2018, Ævar Arnfjörð Bjarmason wrote:\n>> >\n>> >> So I think the only reason to keep it [GETTEXT_POISON] compile-time is\n>> >> performance, but I don't think that matters. It's not like we're\n>> >> printing gigabytes of _() formatted output. Everything where formatting\n>> >> matters is plumbing which doesn't use this API. These messages are\n>> >> always for human consumption.\n>> >\n>> > Well, let's make sure that your impression is correct before going too\n>> > far. I, too, had the impression that gettext cannot possibly be expensive,\n>> > especifally in Git for Windows' settings, where we do not even ship\n>> > translations. Yet see the commit message of cc5e1bf99247 (gettext: avoid\n>> > initialization if the locale dir is not present, 2018-04-21):\n>> >\n>> > The runtime of a simple `git.exe version` call on Windows is\n>> > currently dominated by the gettext setup, adding a whopping ~150ms\n>> > to the ~210ms total.\n>> >\n>> > I would be in favor of your change to make this a runtime option, of\n>> > course, as long as it does not affect performance greatly (in particular\n>> > on Windows, where we fight an uphill battle to make Git faster).\n>>\n>> How expensive gettext() may or may not be isn't relevant to the\n>> GETTEXT_POISON compile-time option.\n>\n> If a user requests NO_GETTEXT, they should have zero (or near zero)\n> cost related to gettext. Which is true until now (the inline _()\n> version is expanded to pretty much no-op with the exception of NULL\n> string)\n\nI'm assuming by \"until now\" you mean until my RFC patch in\nhttps://public-inbox.org/git/875zxtd59e.fsf@evledraar.gmail.com/\n\nYeah it's more expensive, but I don't see how it matters. We'd be\nnano-optimizing a thing that's never going to become a\nbottleneck. I.e. the patch is basically taking the commented-out\ncodepath in this test program:\n\n #include \n #include \n #include \n\n int have_flag(void)\n {\n return 0;\n /*\n static int flag = -1;\n if (flag == -1)\n flag = strlen(getenv(\"GIT_TEST_FOO\"));\n return flag;\n */\n }\n\n\n int main(void)\n {\n while (1) {\n const char *out = have_flag() ? \"foo\" : \"bar\";\n puts(out);\n }\n }\n\nWhen I test that on my laptop with gcc 8.2.0 I get ~230MB/s of output on\nboth versions, i.e. where have_flag() can be trivially constant folded,\nand where we need to call getenv() for both of:\n\n GIT_TEST_FOO= ./main | pv >/dev/null\n GIT_TEST_FOO=1 ./main | pv >/dev/null\n\nWe don't have any codepaths where we're calling gettext() to output more\nthan a screenfull or two of output, so optimizing this doesn't make\nsense.\n\n>> The effect of what I'm suggesting here, and which my WIP patch in\n>> <875zxtd59e.fsf@evledraar.gmail.com> implements is that we'd do a\n>> one-time getenv() for each process that prints a _() message that we\n>> aren't doing now, and for each message call a function that would check\n>> a boolean \"are we in poison mode\" static global variable.\n>\n> Just don't do the getenv() inside _() even if you just do it one time.\n> getenv() may invalidate whatever value from the previous call. I would\n> not want to find out my code broke because of an getenv() inside some\n> innocent _()...\n\nHow would any code break? We have various getenv(\"GIT_TEST_*...\")\nalready that work the same way. Unless you set that in the environment\nthe function is idempotent, and I don't see how anyone would do that\naccidentally.\n\n> And we should still respect NO_GETTEXT, not doing any extra work when\n> it's defined.\n\nThis is not how any of our NO_* defines work. *Sometimes* defining them\nguarantees you do no extra work, but in other cases we've decided that\nbending over backwards with #ifdef in various places isn't worth it.\n\nE.g. grep_opt in grep.h is a good example of this. Even if you don't\ncompile with PCRE you're pointlessly memzero-ing out members of the\nstruct that'll never be used in your config, because it's easier to\nmaintain the code that way (types always checked at compile-time).\n\n(And no, despite my involvement in PCRE I didn't introduce that\nparticular pattern)\n\nFor the reasons noted above I think NO_GETTEXT is another such\ncase. Just like GIT_TEST_SPLIT_INDEX (added in your d6e3c181bc\n(\"read-cache: force split index mode with GIT_TEST_SPLIT_INDEX\",\n2014-06-13)) etc.\n"},{"id":"361327","messageId":"20181023210154.32507-1-avarab@gmail.com","threadId":"49637","inReplyTo":"xmqqsh0xtwgb.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] i18n: make GETTEXT_POISON a runtime option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-23T21:01:54Z","receivedAt":"2018-10-23T21:02:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON\ntest parameter to only be a GIT_TEST_GETTEXT_POISON= runtime\nparameter, to be consistent with other parameters documented in\n\"Running tests with special setups\" in t/README.\n\nWhen I added GETTEXT_POISON in bb946bba76 (\"i18n: add GETTEXT_POISON\nto simulate unfriendly translator\", 2011-02-22) I was concerned with\nensuring that the _() function would get constant folded if NO_GETTEXT\nwas defined or if it wasn't and GETTEXT_POISON wasn't defined.\n\nBut as the benchmark in my [1] shows doing a one-off runtime\ngetenv(\"GIT_TEST_[...]\") is trivial, and since GETTEXT_POISON was\noriginally added this has become the common idiom in the codebase for\nturning on special test setups.\n\nSo change GETTEXT_POISON to work the same way. Now the\nGETTEXT_POISON=YesPlease compile-time option is gone, and running the\ntests with GIT_TEST_GETTEXT_POISON=[true|false] can be toggled on/off\nwithout recompiling.\n\nThis allows for conditionally amending tests to test with/without\npoison, similar to what 859fdc0c3c (\"commit-graph: define\nGIT_TEST_COMMIT_GRAPH\", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH, but\nthis patch doesn't change any of the existing tests to work like that.\n\nNotes on the implementation:\n\n * The only reason we need a new \"git-sh-i18n--helper\" and the\n corresponding \"test-tool gettext-poison\" is to expose\n git_env_bool() to shellscripts, since git-sh-i18n and friends need\n to inspect the $GIT_TEST_GETTEXT_POISON variable.\n\n We only call these if $GIT_TEST_GETTEXT_POISON is set, or in the\n test suite, and this code can go away for non-test code once the\n last i18n-using shellscript is rewritten in C.\n\n * We still compile a dedicated GETTEXT_POISON build in Travis CI,\n this is probably the wrong thing to do and should be followed-up\n with something similar to ae59a4e44f (\"travis: run tests with\n GIT_TEST_SPLIT_INDEX\", 2018-01-07) to re-use an existing test setup\n for running in the GIT_TEST_GETTEXT_POISON mode.\n\n * The reason for not doing:\n\n test_lazy_prereq GETTEXT_POISON 'test-tool gettext-poison'\n test_lazy_prereq C_LOCALE_OUTPUT '! test-tool gettext-poison'\n\n In test-lib.sh is because there's some interpolation problem with\n that syntax which makes t6040-tracking-info.sh fail. Hence using\n the simpler test_set_prereq.\n\nSee also\nhttps://public-inbox.org/git/871s8gd32p.fsf@evledraar.gmail.com/ for\nmore discussion.\n\n1. https://public-inbox.org/git/871s8gd32p.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nOn Mon, Oct 22 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> So I think the only reason to keep it compile-time is performance, but I\n>> don't think that matters. It's not like we're printing gigabytes of _()\n>> formatted output. Everything where formatting matters is plumbing which\n>> doesn't use this API. These messages are always for human consumption.\n>>\n>> So shouldn't we just drop this notion of GETTEXT_POISON at compile-time\n>> entirely, and make GIT_GETTEXT_POISON= a test flag that all\n>> builds of git are aware of? I think so.\n>\n> Haven't thought things through, but that sounds like a good thing to\n> aim for. Keep the ideas coming...\n\nHere's a polished version of that. I think it makes sense to queue\nthis up before any other refactoring of GETTEXT_POISON, and some patch\nto unconditionally preserve format specifiers as I suggested upthread\ncould go on top of this.\n\n .gitignore | 1 +\n .travis.yml | 2 +-\n Makefile | 11 ++---------\n builtin.h | 1 +\n builtin/sh-i18n--helper.c | 27 +++++++++++++++++++++++++++\n ci/lib-travisci.sh | 4 ++--\n gettext.c | 5 ++---\n gettext.h | 4 ----\n git-sh-i18n.sh | 3 ++-\n git.c | 1 +\n po/README | 13 ++++---------\n t/README | 6 ++++++\n t/helper/test-gettext-poison.c | 9 +++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t0000-basic.sh | 2 +-\n t/t3406-rebase-message.sh | 2 +-\n t/t7201-co.sh | 5 ++++-\n t/test-lib-functions.sh | 8 ++++----\n t/test-lib.sh | 11 ++---------\n 20 files changed, 72 insertions(+), 45 deletions(-)\n create mode 100644 builtin/sh-i18n--helper.c\n create mode 100644 t/helper/test-gettext-poison.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 9d1363a1eb..f7b7977910 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -148,6 +148,7 @@\n /git-serve\n /git-sh-i18n\n /git-sh-i18n--envsubst\n+/git-sh-i18n--helper\n /git-sh-setup\n /git-sh-i18n\n /git-shell\ndiff --git a/.travis.yml b/.travis.yml\nindex 4d4e26c9df..4523a2e5ec 100644\n--- a/.travis.yml\n+++ b/.travis.yml\n@@ -26,7 +26,7 @@ addons:\n \n matrix:\n include:\n- - env: jobname=GETTEXT_POISON\n+ - env: jobname=GIT_TEST_GETTEXT_POISON\n os: linux\n compiler:\n addons:\ndiff --git a/Makefile b/Makefile\nindex d18ab0fe78..684dec4d39 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -362,11 +362,6 @@ all::\n # Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the\n # user.\n #\n-# Define GETTEXT_POISON if you are debugging the choice of strings marked\n-# for translation. In a GETTEXT_POISON build, you can turn all strings marked\n-# for translation into gibberish by setting the GIT_GETTEXT_POISON variable\n-# (to any value) in your environment.\n-#\n # Define JSMIN to point to JavaScript minifier that functions as\n # a filter to have gitweb.js minified.\n #\n@@ -714,6 +709,7 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o\n TEST_BUILTINS_OBJS += test-dump-untracked-cache.o\n TEST_BUILTINS_OBJS += test-example-decorate.o\n TEST_BUILTINS_OBJS += test-genrandom.o\n+TEST_BUILTINS_OBJS += test-gettext-poison.o\n TEST_BUILTINS_OBJS += test-hashmap.o\n TEST_BUILTINS_OBJS += test-index-version.o\n TEST_BUILTINS_OBJS += test-json-writer.o\n@@ -1099,6 +1095,7 @@ BUILTIN_OBJS += builtin/revert.o\n BUILTIN_OBJS += builtin/rm.o\n BUILTIN_OBJS += builtin/send-pack.o\n BUILTIN_OBJS += builtin/serve.o\n+BUILTIN_OBJS += builtin/sh-i18n--helper.o\n BUILTIN_OBJS += builtin/shortlog.o\n BUILTIN_OBJS += builtin/show-branch.o\n BUILTIN_OBJS += builtin/show-index.o\n@@ -1439,9 +1436,6 @@ endif\n ifdef NO_SYMLINK_HEAD\n \tBASIC_CFLAGS += -DNO_SYMLINK_HEAD\n endif\n-ifdef GETTEXT_POISON\n-\tBASIC_CFLAGS += -DGETTEXT_POISON\n-endif\n ifdef NO_GETTEXT\n \tBASIC_CFLAGS += -DNO_GETTEXT\n \tUSE_GETTEXT_SCHEME ?= fallthrough\n@@ -2591,7 +2585,6 @@ ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT\n \t@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@+\n endif\n \t@echo NO_GETTEXT=\\''$(subst ','\\'',$(subst ','\\'',$(NO_GETTEXT)))'\\' >>$@+\n-\t@echo GETTEXT_POISON=\\''$(subst ','\\'',$(subst ','\\'',$(GETTEXT_POISON)))'\\' >>$@+\n ifdef GIT_PERF_REPEAT_COUNT\n \t@echo GIT_PERF_REPEAT_COUNT=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_PERF_REPEAT_COUNT)))'\\' >>$@+\n endif\ndiff --git a/builtin.h b/builtin.h\nindex 962f0489ab..a40c56e7a2 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -219,6 +219,7 @@ extern int cmd_revert(int argc, const char **argv, const char *prefix);\n extern int cmd_rm(int argc, const char **argv, const char *prefix);\n extern int cmd_send_pack(int argc, const char **argv, const char *prefix);\n extern int cmd_serve(int argc, const char **argv, const char *prefix);\n+extern int cmd_sh_i18n__helper(int argc, const char **argv, const char *prefix);\n extern int cmd_shortlog(int argc, const char **argv, const char *prefix);\n extern int cmd_show(int argc, const char **argv, const char *prefix);\n extern int cmd_show_branch(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/sh-i18n--helper.c b/builtin/sh-i18n--helper.c\nnew file mode 100644\nindex 0000000000..1fba8b902b\n--- /dev/null\n+++ b/builtin/sh-i18n--helper.c\n@@ -0,0 +1,27 @@\n+#include \"builtin.h\"\n+#include \"cache.h\"\n+#include \"config.h\"\n+#include \"parse-options.h\"\n+\n+static const char * const builtin_sh_i18n_helper_usage[] = {\n+\tN_(\"git sh-i18n--helper []\"),\n+\tNULL\n+};\n+\n+int cmd_sh_i18n__helper(int argc, const char **argv, const char *prefix)\n+{\n+\tint poison = -1;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"git-test-gettext-poison\", &poison,\n+\t\t\t N_(\"is GIT_TEST_GETTEXT_POISON in effect?\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, NULL, options,\n+\t\t\t builtin_sh_i18n_helper_usage, PARSE_OPT_KEEP_ARGV0);\n+\n+\tif (poison != -1)\n+\t\treturn !git_env_bool(\"GIT_TEST_GETTEXT_POISON\", 0);\n+\n+\tusage_with_options(builtin_sh_i18n_helper_usage, options);\n+}\ndiff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh\nindex 06970f7213..6a89d0d7d8 100755\n--- a/ci/lib-travisci.sh\n+++ b/ci/lib-travisci.sh\n@@ -123,7 +123,7 @@ osx-clang|osx-gcc)\n \t# Travis CI OS X\n \texport GIT_SKIP_TESTS=\"t9810 t9816\"\n \t;;\n-GETTEXT_POISON)\n-\texport GETTEXT_POISON=YesPlease\n+GIT_TEST_GETTEXT_POISON)\n+\texport GIT_TEST_GETTEXT_POISON=true\n \t;;\n esac\ndiff --git a/gettext.c b/gettext.c\nindex 7272771c8e..722a2f726c 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -7,6 +7,7 @@\n #include \"gettext.h\"\n #include \"strbuf.h\"\n #include \"utf8.h\"\n+#include \"config.h\"\n \n #ifndef NO_GETTEXT\n #\tinclude \n@@ -46,15 +47,13 @@ const char *get_preferred_languages(void)\n \treturn NULL;\n }\n \n-#ifdef GETTEXT_POISON\n int use_gettext_poison(void)\n {\n \tstatic int poison_requested = -1;\n \tif (poison_requested == -1)\n-\t\tpoison_requested = getenv(\"GIT_GETTEXT_POISON\") ? 1 : 0;\n+\t\tpoison_requested = git_env_bool(\"GIT_TEST_GETTEXT_POISON\", 0);\n \treturn poison_requested;\n }\n-#endif\n \n #ifndef NO_GETTEXT\n static int test_vsnprintf(const char *fmt, ...)\ndiff --git a/gettext.h b/gettext.h\nindex 7eee64a34f..4c492d9f57 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -41,11 +41,7 @@ static inline int gettext_width(const char *s)\n }\n #endif\n \n-#ifdef GETTEXT_POISON\n extern int use_gettext_poison(void);\n-#else\n-#define use_gettext_poison() 0\n-#endif\n \n static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n {\ndiff --git a/git-sh-i18n.sh b/git-sh-i18n.sh\nindex 9d065fb4bf..c0713b1ee9 100644\n--- a/git-sh-i18n.sh\n+++ b/git-sh-i18n.sh\n@@ -17,7 +17,8 @@ export TEXTDOMAINDIR\n \n # First decide what scheme to use...\n GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough\n-if test -n \"$GIT_GETTEXT_POISON\"\n+if test -n \"$GIT_TEST_GETTEXT_POISON\" &&\n+\t git sh-i18n--helper --git-test-gettext-poison\n then\n \tGIT_INTERNAL_GETTEXT_SH_SCHEME=poison\n elif test -n \"@@USE_GETTEXT_SCHEME@@\"\ndiff --git a/git.c b/git.c\nindex 5920f8019b..125c523720 100644\n--- a/git.c\n+++ b/git.c\n@@ -539,6 +539,7 @@ static struct cmd_struct commands[] = {\n \t{ \"rm\", cmd_rm, RUN_SETUP },\n \t{ \"send-pack\", cmd_send_pack, RUN_SETUP },\n \t{ \"serve\", cmd_serve, RUN_SETUP },\n+\t{ \"sh-i18n--helper\", cmd_sh_i18n__helper, 0 },\n \t{ \"shortlog\", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER },\n \t{ \"show\", cmd_show, RUN_SETUP },\n \t{ \"show-branch\", cmd_show_branch, RUN_SETUP },\ndiff --git a/po/README b/po/README\nindex fef4c0f0b5..dba46c4a40 100644\n--- a/po/README\n+++ b/po/README\n@@ -289,16 +289,11 @@ something in the test suite might still depend on the US English\n version of the strings, e.g. to grep some error message or other\n output.\n \n-To smoke out issues like these Git can be compiled with gettext poison\n-support, at the top-level:\n+To smoke out issues like these Git tested with a translation mode that\n+emits gibberish on every call to gettext. To use it run the test suite\n+with it, e.g.:\n \n- make GETTEXT_POISON=YesPlease\n-\n-That'll give you a git which emits gibberish on every call to\n-gettext. It's obviously not meant to be installed, but you should run\n-the test suite with it:\n-\n- cd t && prove -j 9 ./t[0-9]*.sh\n+ cd t && GIT_TEST_GETTEXT_POISON=true prove -j 9 ./t[0-9]*.sh\n \n If tests break with it you should inspect them manually and see if\n what you're translating is sane, i.e. that you're not translating\ndiff --git a/t/README b/t/README\nindex 8847489640..53c3dee7a9 100644\n--- a/t/README\n+++ b/t/README\n@@ -301,6 +301,12 @@ that cannot be easily covered by a few specific test cases. These\n could be enabled by running the test suite with correct GIT_TEST_\n environment set.\n \n+GIT_TEST_GETTEXT_POISON= turns all strings marked for\n+translation into gibberish. Used for spotting those tests that need to\n+be marked with a C_LOCALE_OUTPUT prerequisite when adding more strings\n+for translation. See \"Testing marked strings\" in po/README for\n+details.\n+\n GIT_TEST_SPLIT_INDEX= forces split-index mode on the whole\n test suite. Accept any boolean values that are accepted by git-config.\n \ndiff --git a/t/helper/test-gettext-poison.c b/t/helper/test-gettext-poison.c\nnew file mode 100644\nindex 0000000000..476be95da5\n--- /dev/null\n+++ b/t/helper/test-gettext-poison.c\n@@ -0,0 +1,9 @@\n+#include \"test-tool.h\"\n+#include \"git-compat-util.h\"\n+#include \"thread-utils.h\"\n+#include \"gettext.h\"\n+\n+int cmd__gettext_poison(int argc, const char **argv)\n+{\n+\treturn use_gettext_poison() ? 0 : 1;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 6b5836dc1b..3e75672a37 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -19,6 +19,7 @@ static struct test_cmd cmds[] = {\n \t{ \"dump-untracked-cache\", cmd__dump_untracked_cache },\n \t{ \"example-decorate\", cmd__example_decorate },\n \t{ \"genrandom\", cmd__genrandom },\n+\t{ \"gettext-poison\", cmd__gettext_poison },\n \t{ \"hashmap\", cmd__hashmap },\n \t{ \"index-version\", cmd__index_version },\n \t{ \"json-writer\", cmd__json_writer },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex e4890566da..04f033b7fc 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -15,6 +15,7 @@ int cmd__dump_split_index(int argc, const char **argv);\n int cmd__dump_untracked_cache(int argc, const char **argv);\n int cmd__example_decorate(int argc, const char **argv);\n int cmd__genrandom(int argc, const char **argv);\n+int cmd__gettext_poison(int argc, const char **argv);\n int cmd__hashmap(int argc, const char **argv);\n int cmd__index_version(int argc, const char **argv);\n int cmd__json_writer(int argc, const char **argv);\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 4d23373526..b6566003dd 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -274,7 +274,7 @@ test_expect_success 'pretend we have a mix of all possible results' \"\n \tEOF\n \"\n \n-test_expect_success 'test --verbose' '\n+test_expect_success C_LOCALE_OUTPUT 'test --verbose' '\n \ttest_must_fail run_sub_test_lib_test \\\n \t\ttest-verbose \"test verbose\" --verbose <<-\\EOF &&\n \ttest_expect_success \"passing test\" true\ndiff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\nindex 0392e36d23..2bdcf83808 100755\n--- a/t/t3406-rebase-message.sh\n+++ b/t/t3406-rebase-message.sh\n@@ -77,7 +77,7 @@ test_expect_success 'rebase -n overrides config rebase.stat config' '\n # \"Does not point to a valid commit: invalid-ref\"\n #\n # NEEDSWORK: This \"grep\" is fine in real non-C locales, but\n-# GETTEXT_POISON poisons the refname along with the enclosing\n+# GIT_TEST_GETTEXT_POISON poisons the refname along with the enclosing\n # error message.\n test_expect_success 'rebase --onto outputs the invalid ref' '\n \ttest_must_fail git rebase --onto invalid-ref HEAD HEAD 2>err &&\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 826987ca80..cb2c8cf3f3 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -256,7 +256,10 @@ test_expect_success 'checkout to detach HEAD' '\n \tgit checkout -f renamer && git clean -f &&\n \tgit checkout renamer^ 2>messages &&\n \ttest_i18ngrep \"HEAD is now at 7329388\" messages &&\n-\t(test_line_count -gt 1 messages || test -n \"$GETTEXT_POISON\") &&\n+\t(\n+\t\ttest_line_count -gt 1 messages ||\n+\t\ttest_have_prereq GETTEXT_POISON\n+\t) &&\n \tH=$(git rev-parse --verify HEAD) &&\n \tM=$(git show-ref -s --verify refs/heads/master) &&\n \ttest \"z$H\" = \"z$M\" &&\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 78d8c3783b..f46e21cfa0 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -755,16 +755,16 @@ test_cmp_bin() {\n \n # Use this instead of test_cmp to compare files that contain expected and\n # actual output from git commands that can be translated. When running\n-# under GETTEXT_POISON this pretends that the command produced expected\n+# under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected\n # results.\n test_i18ncmp () {\n-\ttest -n \"$GETTEXT_POISON\" || test_cmp \"$@\"\n+\t! test_have_prereq C_LOCALE_OUTPUT || test_cmp \"$@\"\n }\n \n # Use this instead of \"grep expected-string actual\" to see if the\n # output from a git command that can be translated either contains an\n # expected string, or does not contain an unwanted one. When running\n-# under GETTEXT_POISON this pretends that the command produced expected\n+# under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected\n # results.\n test_i18ngrep () {\n \teval \"last_arg=\\${$#}\"\n@@ -779,7 +779,7 @@ test_i18ngrep () {\n \t\terror \"bug in the test script: too few parameters to test_i18ngrep\"\n \tfi\n \n-\tif test -n \"$GETTEXT_POISON\"\n+\tif ! test_have_prereq C_LOCALE_OUTPUT\n \tthen\n \t\t# pretend success\n \t\treturn 0\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 897e6fcc94..ec77aa57d4 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1104,15 +1104,8 @@ test -n \"$USE_LIBPCRE1\" && test_set_prereq LIBPCRE1\n test -n \"$USE_LIBPCRE2\" && test_set_prereq LIBPCRE2\n test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n \n-# Can we rely on git's output in the C locale?\n-if test -n \"$GETTEXT_POISON\"\n-then\n-\tGIT_GETTEXT_POISON=YesPlease\n-\texport GIT_GETTEXT_POISON\n-\ttest_set_prereq GETTEXT_POISON\n-else\n-\ttest_set_prereq C_LOCALE_OUTPUT\n-fi\n+test-tool gettext-poison && test_set_prereq GETTEXT_POISON\n+test-tool gettext-poison || test_set_prereq C_LOCALE_OUTPUT\n \n if test -z \"$GIT_TEST_CHECK_CACHE_TREE\"\n then\n-- \n2.19.1.568.g152ad8e336\n\n"},{"id":"361368","messageId":"xmqqefcfoq2a.fsf@gitster-ct.c.googlers.com","threadId":"49637","inReplyTo":"20181023210154.32507-1-avarab@gmail.com","subject":"Re: [PATCH] i18n: make GETTEXT_POISON a runtime option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-24T05:45:49Z","receivedAt":"2018-10-24T05:45:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Notes on the implementation:\n>\n> * The only reason we need a new \"git-sh-i18n--helper\" and the\n> corresponding \"test-tool gettext-poison\" is to expose\n> git_env_bool() to shellscripts, since git-sh-i18n and friends need\n> to inspect the $GIT_TEST_GETTEXT_POISON variable.\n>\n> We only call these if $GIT_TEST_GETTEXT_POISON is set, or in the\n> test suite, and this code can go away for non-test code once the\n> last i18n-using shellscript is rewritten in C.\n\nMakes me wonder if we want to teach \"git config\" a new \"--env-bool\"\noption that can be used by third-party scripts. Or would it be just\nthe matter of writing\n\n\tgit config --default=false --type=bool \"$GIT_WHATEVER_ENV\"\n\nin these third-party scripts and we do not need to add such a thing?\n\n> * The reason for not doing:\n>\n> test_lazy_prereq GETTEXT_POISON 'test-tool gettext-poison'\n> test_lazy_prereq C_LOCALE_OUTPUT '! test-tool gettext-poison'\n>\n> In test-lib.sh is because there's some interpolation problem with\n> that syntax which makes t6040-tracking-info.sh fail. Hence using\n> the simpler test_set_prereq.\n\ns/In/in/, as you haven't finished the sentence yet. But more\nimportantly, what is this \"some interpolation problem\"? Are you\nsaying that test_lazy_prereq implementation is somehow broken and\ncannot take certain strings? If so, perhaps we want to fix that,\nand people other than you can help to do so, once you let them know\nwhat the problem is.\n\n> See also\n> https://public-inbox.org/git/871s8gd32p.fsf@evledraar.gmail.com/ for\n> more discussion.\n\n\"See also [*1*] for more discussion\" as you've already have\nreference below.\n\n>\n> 1. https://public-inbox.org/git/871s8gd32p.fsf@evledraar.gmail.com/\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n>\n> Here's a polished version of that. I think it makes sense to queue\n> this up before any other refactoring of GETTEXT_POISON, and some patch\n> to unconditionally preserve format specifiers as I suggested upthread\n> could go on top of this.\n> ...\n> +int cmd_sh_i18n__helper(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint poison = -1;\n> +\tstruct option options[] = {\n> +\t\tOPT_BOOL(0, \"git-test-gettext-poison\", &poison,\n> +\t\t\t N_(\"is GIT_TEST_GETTEXT_POISON in effect?\")),\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, NULL, options,\n> +\t\t\t builtin_sh_i18n_helper_usage, PARSE_OPT_KEEP_ARGV0);\n> +\n> +\tif (poison != -1)\n> +\t\treturn !git_env_bool(\"GIT_TEST_GETTEXT_POISON\", 0);\n\nHmm? If \"--[no-]git-test-gettext-poison\" is given, poison is either\n0 or 1, and we return the value we read from the environment? What\nconvoluted way to implement the option is that, or is there anything\nsubtle that I am not getting?\n\nIf the \"default\" parameter to git_env_bool() were poison, and then\nthe option was renamed to \"--get-git-text-gettext-poison\", then I\nsort of understand the code, though (it's like \"git config\" with\n\"--default\" option).\n\nBut if there is nothing subtle, it may make sense to implement this\nas an opt-cmdmode instead.\n\n> diff --git a/po/README b/po/README\n> index fef4c0f0b5..dba46c4a40 100644\n> --- a/po/README\n> +++ b/po/README\n> @@ -289,16 +289,11 @@ something in the test suite might still depend on the US English\n> version of the strings, e.g. to grep some error message or other\n> output.\n> \n> -To smoke out issues like these Git can be compiled with gettext poison\n> -support, at the top-level:\n> +To smoke out issues like these Git tested with a translation mode that\n> +emits gibberish on every call to gettext. To use it run the test suite\n> +with it, e.g.:\n\ns/these Git tested/these, Git can be tested/; even though the comma\nis not a new issue you introduced.\n"},{"id":"361381","messageId":"20181024074400.GA31239@sigill.intra.peff.net","threadId":"49637","inReplyTo":"xmqqefcfoq2a.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] i18n: make GETTEXT_POISON a runtime option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-24T07:44:00Z","receivedAt":"2018-10-24T07:44:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 24, 2018 at 02:45:49PM +0900, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > Notes on the implementation:\n> >\n> > * The only reason we need a new \"git-sh-i18n--helper\" and the\n> > corresponding \"test-tool gettext-poison\" is to expose\n> > git_env_bool() to shellscripts, since git-sh-i18n and friends need\n> > to inspect the $GIT_TEST_GETTEXT_POISON variable.\n> >\n> > We only call these if $GIT_TEST_GETTEXT_POISON is set, or in the\n> > test suite, and this code can go away for non-test code once the\n> > last i18n-using shellscript is rewritten in C.\n> \n> Makes me wonder if we want to teach \"git config\" a new \"--env-bool\"\n> option that can be used by third-party scripts. Or would it be just\n> the matter of writing\n> \n> \tgit config --default=false --type=bool \"$GIT_WHATEVER_ENV\"\n> \n> in these third-party scripts and we do not need to add such a thing?\n\nThe config command only takes names, not values. So you'd have to write\nsomething like:\n\n git -c env.bool=\"$GIT_WHATEVER_ENV\" config --type=bool env.bool\n\nbut then you lose the default handling. I think if we added a new\noption, it would either be:\n\n # interpret a value directly; use default on empty, I guess?\n git config --default=false --type=bool --interpret-value \"$GIT_WHATEVER_ENV\"\n\nor\n\n # less flexible, but the --default semantics are more obvious\n git config --default=false --type=bool --get-env GIT_WHATEVER_ENV\n\n-Peff\n"},{"id":"361405","messageId":"20181024114725.3927-1-avarab@gmail.com","threadId":"49637","inReplyTo":"xmqqefcfoq2a.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2] i18n: make GETTEXT_POISON a runtime option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-24T11:47:25Z","receivedAt":"2018-10-24T11:47:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON\ntest parameter to only be a GIT_TEST_GETTEXT_POISON= runtime\nparameter, to be consistent with other parameters documented in\n\"Running tests with special setups\" in t/README.\n\nWhen I added GETTEXT_POISON in bb946bba76 (\"i18n: add GETTEXT_POISON\nto simulate unfriendly translator\", 2011-02-22) I was concerned with\nensuring that the _() function would get constant folded if NO_GETTEXT\nwas defined or if it wasn't and GETTEXT_POISON wasn't defined.\n\nBut as the benchmark in my [1] shows doing a one-off runtime\ngetenv(\"GIT_TEST_[...]\") is trivial, and since GETTEXT_POISON was\noriginally added this has become the common idiom in the codebase for\nturning on special test setups.\n\nSo change GETTEXT_POISON to work the same way. Now the\nGETTEXT_POISON=YesPlease compile-time option is gone, and running the\ntests with GIT_TEST_GETTEXT_POISON=[true|false] can be toggled on/off\nwithout recompiling.\n\nThis allows for conditionally amending tests to test with/without\npoison, similar to what 859fdc0c3c (\"commit-graph: define\nGIT_TEST_COMMIT_GRAPH\", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH. Do\nsome of that, now we e.g. always run the t0205-gettext-poison.sh test.\n\nI did enough there to remove the GETTEXT_POISON prerequisite, but its\ninverse C_LOCALE_OUTPUT is still around, and surely some tests using\nit can be converted to e.g. always set GIT_TEST_GETTEXT_POISON=false.\n\nNotes on the implementation:\n\n * The only reason we need a new \"git-sh-i18n--helper\" is to expose\n git_env_bool() to shellscripts, since git-sh-i18n and friends need\n to inspect the $GIT_TEST_GETTEXT_POISON variable.\n\n We only call it if $GIT_TEST_GETTEXT_POISON is set, or in the test\n suite. This code can go away for non-test code once the last\n i18n-using shellscript is rewritten in C, or if \"git config\" learns\n to combine --bool and a value fed into it, see [2].\n\n * We still compile a dedicated GETTEXT_POISON build in Travis CI,\n this is probably the wrong thing to do and should be followed-up\n with something similar to ae59a4e44f (\"travis: run tests with\n GIT_TEST_SPLIT_INDEX\", 2018-01-07) to re-use an existing test setup\n for running in the GIT_TEST_GETTEXT_POISON mode.\n\n * The reason for not doing:\n\n test_lazy_prereq C_LOCALE_OUTPUT [...]\n\n in test-lib.sh is because there's some interpolation problem with\n that syntax which makes t6040-tracking-info.sh fail. Hence using\n the simpler test_set_prereq. It'll fail with:\n\n + git branch -vv\n + sed -n -e\n mkdir -p \"$TRASH_DIRECTORY/prereq-test-dir\" &&\n (\n cd \"$TRASH_DIRECTORY/prereq-test-dir\" &&! git sh-i18n--helper --git-test-gettext-poison\n )\n sed: -e expression #1, char 2: unknown command: `m'\n error: last command exited with $?=1\n not ok 3 - branch -vv\n\n This is some interpolation problem with how test_lazy_prereq works\n that doesn't affect test_set_prereq.\n\n * We now skip a test in t0000-basic.sh under\n GIT_TEST_GETTEXT_POISON=true that wasn't skipped before. This test\n relies on C locale output, but due to an edge case in how the\n previous implementation of GETTEXT_POISON worked (reading it from\n GIT-BUILD-OPTIONS) wasn't enabling poison correctly. Now it does,\n and needs to be skipped.\n\nSee also\nhttps://public-inbox.org/git/878t2pd6yu.fsf@evledraar.gmail.com/ for\nmore discussion.\n\n1. https://public-inbox.org/git/871s8gd32p.fsf@evledraar.gmail.com/\n\n2. https://public-inbox.org/git/20181024074400.GA31239@sigill.intra.peff.net/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nOn Wed, Oct 24 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Notes on the implementation:\n>>\n>> * The only reason we need a new \"git-sh-i18n--helper\" and the\n>> corresponding \"test-tool gettext-poison\" is to expose\n>> git_env_bool() to shellscripts, since git-sh-i18n and friends need\n>> to inspect the $GIT_TEST_GETTEXT_POISON variable.\n>>\n>> We only call these if $GIT_TEST_GETTEXT_POISON is set, or in the\n>> test suite, and this code can go away for non-test code once the\n>> last i18n-using shellscript is rewritten in C.\n>\n> Makes me wonder if we want to teach \"git config\" a new \"--env-bool\"\n> option that can be used by third-party scripts. Or would it be just\n> the matter of writing\n>\n> \tgit config --default=false --type=bool \"$GIT_WHATEVER_ENV\"\n>\n> in these third-party scripts and we do not need to add such a thing?\n\nAs Jeff notes in a follow-up reply git-config can't quite do this\nyet. So in lieu of implementing that I have a more minimal helper,\nwhich could be migrated to something like the --get-env mode Jeff is\ntalking about if we add that.\n\n>> * The reason for not doing:\n>>\n>> test_lazy_prereq GETTEXT_POISON 'test-tool gettext-poison'\n>> test_lazy_prereq C_LOCALE_OUTPUT '! test-tool gettext-poison'\n>>\n>> In test-lib.sh is because there's some interpolation problem with\n>> that syntax which makes t6040-tracking-info.sh fail. Hence using\n>> the simpler test_set_prereq.\n>\n> s/In/in/, as you haven't finished the sentence yet. But more\n> importantly, what is this \"some interpolation problem\"? Are you\n> saying that test_lazy_prereq implementation is somehow broken and\n> cannot take certain strings? If so, perhaps we want to fix that,\n> and people other than you can help to do so, once you let them know\n> what the problem is.\n\nI haven't fixed (don't know what the fix is) this issue, but the new\nversion elaborates on what the error is.\n\n>> See also\n>> https://public-inbox.org/git/871s8gd32p.fsf@evledraar.gmail.com/ for\n>> more discussion.\n>\n> \"See also [*1*] for more discussion\" as you've already have\n> reference below.\n\nThanks, mispasted E-Mail ID. Fixed.\n\n>>\n>> 1. https://public-inbox.org/git/871s8gd32p.fsf@evledraar.gmail.com/\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>>\n>> Here's a polished version of that. I think it makes sense to queue\n>> this up before any other refactoring of GETTEXT_POISON, and some patch\n>> to unconditionally preserve format specifiers as I suggested upthread\n>> could go on top of this.\n>> ...\n>> +int cmd_sh_i18n__helper(int argc, const char **argv, const char *prefix)\n>> +{\n>> +\tint poison = -1;\n>> +\tstruct option options[] = {\n>> +\t\tOPT_BOOL(0, \"git-test-gettext-poison\", &poison,\n>> +\t\t\t N_(\"is GIT_TEST_GETTEXT_POISON in effect?\")),\n>> +\t\tOPT_END()\n>> +\t};\n>> +\n>> +\targc = parse_options(argc, argv, NULL, options,\n>> +\t\t\t builtin_sh_i18n_helper_usage, PARSE_OPT_KEEP_ARGV0);\n>> +\n>> +\tif (poison != -1)\n>> +\t\treturn !git_env_bool(\"GIT_TEST_GETTEXT_POISON\", 0);\n>\n> Hmm? If \"--[no-]git-test-gettext-poison\" is given, poison is either\n> 0 or 1, and we return the value we read from the environment? What\n> convoluted way to implement the option is that, or is there anything\n> subtle that I am not getting?\n>\n> If the \"default\" parameter to git_env_bool() were poison, and then\n> the option was renamed to \"--get-git-text-gettext-poison\", then I\n> sort of understand the code, though (it's like \"git config\" with\n> \"--default\" option).\n\nYeah this didn't make much sense. The test helper is now gone, and the\nmain helper doesn't use git_env_bool() but the utility function in\ngettext.h.\n\n> But if there is nothing subtle, it may make sense to implement this\n> as an opt-cmdmode instead.\n\nYou mean something that works like \"helper foo\" instead of an option\nvia \"helper --foo\"? Seemed easier just to use the opt parse mechanism.\n\n>> diff --git a/po/README b/po/README\n>> index fef4c0f0b5..dba46c4a40 100644\n>> --- a/po/README\n>> +++ b/po/README\n>> @@ -289,16 +289,11 @@ something in the test suite might still depend on the US English\n>> version of the strings, e.g. to grep some error message or other\n>> output.\n>> \n>> -To smoke out issues like these Git can be compiled with gettext poison\n>> -support, at the top-level:\n>> +To smoke out issues like these Git tested with a translation mode that\n>> +emits gibberish on every call to gettext. To use it run the test suite\n>> +with it, e.g.:\n>\n> s/these Git tested/these, Git can be tested/; even though the comma\n> is not a new issue you introduced.\n\nFixed. Range-diff of the whole thing below:\n\n1: c93bf2f23f ! 1: 7bcb95a82d i18n: make GETTEXT_POISON a runtime option\n @@ -24,19 +24,23 @@\n \n This allows for conditionally amending tests to test with/without\n poison, similar to what 859fdc0c3c (\"commit-graph: define\n - GIT_TEST_COMMIT_GRAPH\", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH, but\n - this patch doesn't change any of the existing tests to work like that.\n + GIT_TEST_COMMIT_GRAPH\", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH. Do\n + some of that, now we e.g. always run the t0205-gettext-poison.sh test.\n +\n + I did enough there to remove the GETTEXT_POISON prerequisite, but its\n + inverse C_LOCALE_OUTPUT is still around, and surely some tests using\n + it can be converted to e.g. always set GIT_TEST_GETTEXT_POISON=false.\n \n Notes on the implementation:\n \n - * The only reason we need a new \"git-sh-i18n--helper\" and the\n - corresponding \"test-tool gettext-poison\" is to expose\n + * The only reason we need a new \"git-sh-i18n--helper\" is to expose\n git_env_bool() to shellscripts, since git-sh-i18n and friends need\n to inspect the $GIT_TEST_GETTEXT_POISON variable.\n \n - We only call these if $GIT_TEST_GETTEXT_POISON is set, or in the\n - test suite, and this code can go away for non-test code once the\n - last i18n-using shellscript is rewritten in C.\n + We only call it if $GIT_TEST_GETTEXT_POISON is set, or in the test\n + suite. This code can go away for non-test code once the last\n + i18n-using shellscript is rewritten in C, or if \"git config\" learns\n + to combine --bool and a value fed into it, see [2].\n \n * We still compile a dedicated GETTEXT_POISON build in Travis CI,\n this is probably the wrong thing to do and should be followed-up\n @@ -46,19 +50,40 @@\n \n * The reason for not doing:\n \n - test_lazy_prereq GETTEXT_POISON 'test-tool gettext-poison'\n - test_lazy_prereq C_LOCALE_OUTPUT '! test-tool gettext-poison'\n + test_lazy_prereq C_LOCALE_OUTPUT [...]\n \n - In test-lib.sh is because there's some interpolation problem with\n + in test-lib.sh is because there's some interpolation problem with\n that syntax which makes t6040-tracking-info.sh fail. Hence using\n - the simpler test_set_prereq.\n + the simpler test_set_prereq. It'll fail with:\n +\n + + git branch -vv\n + + sed -n -e\n + mkdir -p \"$TRASH_DIRECTORY/prereq-test-dir\" &&\n + (\n + cd \"$TRASH_DIRECTORY/prereq-test-dir\" &&! git sh-i18n--helper --git-test-gettext-poison\n + )\n + sed: -e expression #1, char 2: unknown command: `m'\n + error: last command exited with $?=1\n + not ok 3 - branch -vv\n +\n + This is some interpolation problem with how test_lazy_prereq works\n + that doesn't affect test_set_prereq.\n +\n + * We now skip a test in t0000-basic.sh under\n + GIT_TEST_GETTEXT_POISON=true that wasn't skipped before. This test\n + relies on C locale output, but due to an edge case in how the\n + previous implementation of GETTEXT_POISON worked (reading it from\n + GIT-BUILD-OPTIONS) wasn't enabling poison correctly. Now it does,\n + and needs to be skipped.\n \n See also\n - https://public-inbox.org/git/871s8gd32p.fsf@evledraar.gmail.com/ for\n + https://public-inbox.org/git/878t2pd6yu.fsf@evledraar.gmail.com/ for\n more discussion.\n \n 1. https://public-inbox.org/git/871s8gd32p.fsf@evledraar.gmail.com/\n \n + 2. https://public-inbox.org/git/20181024074400.GA31239@sigill.intra.peff.net/\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n diff --git a/.gitignore b/.gitignore\n @@ -101,14 +126,6 @@\n # Define JSMIN to point to JavaScript minifier that functions as\n # a filter to have gitweb.js minified.\n #\n -@@\n - TEST_BUILTINS_OBJS += test-dump-untracked-cache.o\n - TEST_BUILTINS_OBJS += test-example-decorate.o\n - TEST_BUILTINS_OBJS += test-genrandom.o\n -+TEST_BUILTINS_OBJS += test-gettext-poison.o\n - TEST_BUILTINS_OBJS += test-hashmap.o\n - TEST_BUILTINS_OBJS += test-index-version.o\n - TEST_BUILTINS_OBJS += test-json-writer.o\n @@\n BUILTIN_OBJS += builtin/rm.o\n BUILTIN_OBJS += builtin/send-pack.o\n @@ -167,8 +184,9 @@\n +{\n +\tint poison = -1;\n +\tstruct option options[] = {\n -+\t\tOPT_BOOL(0, \"git-test-gettext-poison\", &poison,\n -+\t\t\t N_(\"is GIT_TEST_GETTEXT_POISON in effect?\")),\n ++\t\tOPT_BOOL_F(0, \"git-test-gettext-poison\", &poison,\n ++\t\t\t N_(\"is GIT_TEST_GETTEXT_POISON in effect?\"),\n ++\t\t\t PARSE_OPT_NONEG),\n +\t\tOPT_END()\n +\t};\n +\n @@ -176,7 +194,7 @@\n +\t\t\t builtin_sh_i18n_helper_usage, PARSE_OPT_KEEP_ARGV0);\n +\n +\tif (poison != -1)\n -+\t\treturn !git_env_bool(\"GIT_TEST_GETTEXT_POISON\", 0);\n ++\t\treturn !use_gettext_poison();\n +\n +\tusage_with_options(builtin_sh_i18n_helper_usage, options);\n +}\n @@ -275,7 +293,7 @@\n \n -To smoke out issues like these Git can be compiled with gettext poison\n -support, at the top-level:\n -+To smoke out issues like these Git tested with a translation mode that\n ++To smoke out issues like these, Git tested with a translation mode that\n +emits gibberish on every call to gettext. To use it run the test suite\n +with it, e.g.:\n \n @@ -308,44 +326,18 @@\n test suite. Accept any boolean values that are accepted by git-config.\n \n \n - diff --git a/t/helper/test-gettext-poison.c b/t/helper/test-gettext-poison.c\n - new file mode 100644\n - --- /dev/null\n - +++ b/t/helper/test-gettext-poison.c\n + diff --git a/t/lib-gettext.sh b/t/lib-gettext.sh\n + --- a/t/lib-gettext.sh\n + +++ b/t/lib-gettext.sh\n @@\n -+#include \"test-tool.h\"\n -+#include \"git-compat-util.h\"\n -+#include \"thread-utils.h\"\n -+#include \"gettext.h\"\n -+\n -+int cmd__gettext_poison(int argc, const char **argv)\n -+{\n -+\treturn use_gettext_poison() ? 0 : 1;\n -+}\n -\n - diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\n - --- a/t/helper/test-tool.c\n - +++ b/t/helper/test-tool.c\n -@@\n - \t{ \"dump-untracked-cache\", cmd__dump_untracked_cache },\n - \t{ \"example-decorate\", cmd__example_decorate },\n - \t{ \"genrandom\", cmd__genrandom },\n -+\t{ \"gettext-poison\", cmd__gettext_poison },\n - \t{ \"hashmap\", cmd__hashmap },\n - \t{ \"index-version\", cmd__index_version },\n - \t{ \"json-writer\", cmd__json_writer },\n -\n - diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\n - --- a/t/helper/test-tool.h\n - +++ b/t/helper/test-tool.h\n -@@\n - int cmd__dump_untracked_cache(int argc, const char **argv);\n - int cmd__example_decorate(int argc, const char **argv);\n - int cmd__genrandom(int argc, const char **argv);\n -+int cmd__gettext_poison(int argc, const char **argv);\n - int cmd__hashmap(int argc, const char **argv);\n - int cmd__index_version(int argc, const char **argv);\n - int cmd__json_writer(int argc, const char **argv);\n + \n + . \"$GIT_BUILD_DIR\"/git-sh-i18n\n + \n +-if test_have_prereq GETTEXT && ! test_have_prereq GETTEXT_POISON\n ++if test_have_prereq GETTEXT && test_have_prereq C_LOCALE_OUTPUT\n + then\n + \t# is_IS.UTF-8 on Solaris and FreeBSD, is_IS.utf8 on Debian\n + \tis_IS_locale=$(locale -a 2>/dev/null |\n \n diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n --- a/t/t0000-basic.sh\n @@ -360,6 +352,37 @@\n \t\ttest-verbose \"test verbose\" --verbose <<-\\EOF &&\n \ttest_expect_success \"passing test\" true\n \n + diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh\n + --- a/t/t0205-gettext-poison.sh\n + +++ b/t/t0205-gettext-poison.sh\n +@@\n + \n + test_description='Gettext Shell poison'\n + \n ++GIT_TEST_GETTEXT_POISON=true\n ++export GIT_TEST_GETTEXT_POISON\n + . ./lib-gettext.sh\n + \n +-test_expect_success GETTEXT_POISON 'sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME\" is poison' '\n ++test_expect_success 'sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME\" is poison' '\n + test \"$GIT_INTERNAL_GETTEXT_SH_SCHEME\" = \"poison\"\n + '\n + \n +-test_expect_success GETTEXT_POISON 'gettext: our gettext() fallback has poison semantics' '\n ++test_expect_success 'gettext: our gettext() fallback has poison semantics' '\n + printf \"# GETTEXT POISON #\" >expect &&\n + gettext \"test\" >actual &&\n + test_cmp expect actual &&\n +@@\n + test_cmp expect actual\n + '\n + \n +-test_expect_success GETTEXT_POISON 'eval_gettext: our eval_gettext() fallback has poison semantics' '\n ++test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semantics' '\n + printf \"# GETTEXT POISON #\" >expect &&\n + eval_gettext \"test\" >actual &&\n + test_cmp expect actual &&\n +\n diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\n --- a/t/t3406-rebase-message.sh\n +++ b/t/t3406-rebase-message.sh\n @@ -377,18 +400,33 @@\n --- a/t/t7201-co.sh\n +++ b/t/t7201-co.sh\n @@\n + test_expect_success 'checkout to detach HEAD' '\n + \tgit config advice.detachedHead true &&\n \tgit checkout -f renamer && git clean -f &&\n - \tgit checkout renamer^ 2>messages &&\n - \ttest_i18ngrep \"HEAD is now at 7329388\" messages &&\n +-\tgit checkout renamer^ 2>messages &&\n +-\ttest_i18ngrep \"HEAD is now at 7329388\" messages &&\n -\t(test_line_count -gt 1 messages || test -n \"$GETTEXT_POISON\") &&\n -+\t(\n -+\t\ttest_line_count -gt 1 messages ||\n -+\t\ttest_have_prereq GETTEXT_POISON\n -+\t) &&\n ++\tGIT_TEST_GETTEXT_POISON=false git checkout renamer^ 2>messages &&\n ++\tgrep \"HEAD is now at 7329388\" messages &&\n ++\ttest_line_count -gt 1 messages &&\n \tH=$(git rev-parse --verify HEAD) &&\n \tM=$(git show-ref -s --verify refs/heads/master) &&\n \ttest \"z$H\" = \"z$M\" &&\n \n + diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n + --- a/t/t9902-completion.sh\n + +++ b/t/t9902-completion.sh\n +@@\n + \tverbose test -z \"$__git_all_commands\"\n + '\n + \n +-test_expect_success !GETTEXT_POISON 'sourcing the completion script clears cached merge strategies' '\n ++test_expect_success 'sourcing the completion script clears cached merge strategies' '\n ++\tGIT_TEST_GETTEXT_POISON=false &&\n + \t__git_compute_merge_strategies &&\n + \tverbose test -n \"$__git_merge_strategies\" &&\n + \t. \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n +\n diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n --- a/t/test-lib-functions.sh\n +++ b/t/test-lib-functions.sh\n @@ -417,7 +455,7 @@\n \tfi\n \n -\tif test -n \"$GETTEXT_POISON\"\n -+\tif ! test_have_prereq C_LOCALE_OUTPUT\n ++\tif test_have_prereq !C_LOCALE_OUTPUT\n \tthen\n \t\t# pretend success\n \t\treturn 0\n @@ -438,8 +476,9 @@\n -else\n -\ttest_set_prereq C_LOCALE_OUTPUT\n -fi\n -+test-tool gettext-poison && test_set_prereq GETTEXT_POISON\n -+test-tool gettext-poison || test_set_prereq C_LOCALE_OUTPUT\n ++## Fails in the 'branch -vv' test in t6040-tracking-info.sh\n ++#test_lazy_prereq C_LOCALE_OUTPUT '! git sh-i18n--helper --git-test-gettext-poison'\n ++git sh-i18n--helper --git-test-gettext-poison || test_set_prereq C_LOCALE_OUTPUT\n \n if test -z \"$GIT_TEST_CHECK_CACHE_TREE\"\n then\n\n .gitignore | 1 +\n .travis.yml | 2 +-\n Makefile | 10 +---------\n builtin.h | 1 +\n builtin/sh-i18n--helper.c | 28 ++++++++++++++++++++++++++++\n ci/lib-travisci.sh | 4 ++--\n gettext.c | 5 ++---\n gettext.h | 4 ----\n git-sh-i18n.sh | 3 ++-\n git.c | 1 +\n po/README | 13 ++++---------\n t/README | 6 ++++++\n t/lib-gettext.sh | 2 +-\n t/t0000-basic.sh | 2 +-\n t/t0205-gettext-poison.sh | 8 +++++---\n t/t3406-rebase-message.sh | 2 +-\n t/t7201-co.sh | 6 +++---\n t/t9902-completion.sh | 3 ++-\n t/test-lib-functions.sh | 8 ++++----\n t/test-lib.sh | 12 +++---------\n 20 files changed, 69 insertions(+), 52 deletions(-)\n create mode 100644 builtin/sh-i18n--helper.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 9d1363a1eb..f7b7977910 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -148,6 +148,7 @@\n /git-serve\n /git-sh-i18n\n /git-sh-i18n--envsubst\n+/git-sh-i18n--helper\n /git-sh-setup\n /git-sh-i18n\n /git-shell\ndiff --git a/.travis.yml b/.travis.yml\nindex 4d4e26c9df..4523a2e5ec 100644\n--- a/.travis.yml\n+++ b/.travis.yml\n@@ -26,7 +26,7 @@ addons:\n \n matrix:\n include:\n- - env: jobname=GETTEXT_POISON\n+ - env: jobname=GIT_TEST_GETTEXT_POISON\n os: linux\n compiler:\n addons:\ndiff --git a/Makefile b/Makefile\nindex d18ab0fe78..086600b099 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -362,11 +362,6 @@ all::\n # Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the\n # user.\n #\n-# Define GETTEXT_POISON if you are debugging the choice of strings marked\n-# for translation. In a GETTEXT_POISON build, you can turn all strings marked\n-# for translation into gibberish by setting the GIT_GETTEXT_POISON variable\n-# (to any value) in your environment.\n-#\n # Define JSMIN to point to JavaScript minifier that functions as\n # a filter to have gitweb.js minified.\n #\n@@ -1099,6 +1094,7 @@ BUILTIN_OBJS += builtin/revert.o\n BUILTIN_OBJS += builtin/rm.o\n BUILTIN_OBJS += builtin/send-pack.o\n BUILTIN_OBJS += builtin/serve.o\n+BUILTIN_OBJS += builtin/sh-i18n--helper.o\n BUILTIN_OBJS += builtin/shortlog.o\n BUILTIN_OBJS += builtin/show-branch.o\n BUILTIN_OBJS += builtin/show-index.o\n@@ -1439,9 +1435,6 @@ endif\n ifdef NO_SYMLINK_HEAD\n \tBASIC_CFLAGS += -DNO_SYMLINK_HEAD\n endif\n-ifdef GETTEXT_POISON\n-\tBASIC_CFLAGS += -DGETTEXT_POISON\n-endif\n ifdef NO_GETTEXT\n \tBASIC_CFLAGS += -DNO_GETTEXT\n \tUSE_GETTEXT_SCHEME ?= fallthrough\n@@ -2591,7 +2584,6 @@ ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT\n \t@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@+\n endif\n \t@echo NO_GETTEXT=\\''$(subst ','\\'',$(subst ','\\'',$(NO_GETTEXT)))'\\' >>$@+\n-\t@echo GETTEXT_POISON=\\''$(subst ','\\'',$(subst ','\\'',$(GETTEXT_POISON)))'\\' >>$@+\n ifdef GIT_PERF_REPEAT_COUNT\n \t@echo GIT_PERF_REPEAT_COUNT=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_PERF_REPEAT_COUNT)))'\\' >>$@+\n endif\ndiff --git a/builtin.h b/builtin.h\nindex 962f0489ab..a40c56e7a2 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -219,6 +219,7 @@ extern int cmd_revert(int argc, const char **argv, const char *prefix);\n extern int cmd_rm(int argc, const char **argv, const char *prefix);\n extern int cmd_send_pack(int argc, const char **argv, const char *prefix);\n extern int cmd_serve(int argc, const char **argv, const char *prefix);\n+extern int cmd_sh_i18n__helper(int argc, const char **argv, const char *prefix);\n extern int cmd_shortlog(int argc, const char **argv, const char *prefix);\n extern int cmd_show(int argc, const char **argv, const char *prefix);\n extern int cmd_show_branch(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/sh-i18n--helper.c b/builtin/sh-i18n--helper.c\nnew file mode 100644\nindex 0000000000..aa62787514\n--- /dev/null\n+++ b/builtin/sh-i18n--helper.c\n@@ -0,0 +1,28 @@\n+#include \"builtin.h\"\n+#include \"cache.h\"\n+#include \"config.h\"\n+#include \"parse-options.h\"\n+\n+static const char * const builtin_sh_i18n_helper_usage[] = {\n+\tN_(\"git sh-i18n--helper []\"),\n+\tNULL\n+};\n+\n+int cmd_sh_i18n__helper(int argc, const char **argv, const char *prefix)\n+{\n+\tint poison = -1;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL_F(0, \"git-test-gettext-poison\", &poison,\n+\t\t\t N_(\"is GIT_TEST_GETTEXT_POISON in effect?\"),\n+\t\t\t PARSE_OPT_NONEG),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, NULL, options,\n+\t\t\t builtin_sh_i18n_helper_usage, PARSE_OPT_KEEP_ARGV0);\n+\n+\tif (poison != -1)\n+\t\treturn !use_gettext_poison();\n+\n+\tusage_with_options(builtin_sh_i18n_helper_usage, options);\n+}\ndiff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh\nindex 06970f7213..6a89d0d7d8 100755\n--- a/ci/lib-travisci.sh\n+++ b/ci/lib-travisci.sh\n@@ -123,7 +123,7 @@ osx-clang|osx-gcc)\n \t# Travis CI OS X\n \texport GIT_SKIP_TESTS=\"t9810 t9816\"\n \t;;\n-GETTEXT_POISON)\n-\texport GETTEXT_POISON=YesPlease\n+GIT_TEST_GETTEXT_POISON)\n+\texport GIT_TEST_GETTEXT_POISON=true\n \t;;\n esac\ndiff --git a/gettext.c b/gettext.c\nindex 7272771c8e..722a2f726c 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -7,6 +7,7 @@\n #include \"gettext.h\"\n #include \"strbuf.h\"\n #include \"utf8.h\"\n+#include \"config.h\"\n \n #ifndef NO_GETTEXT\n #\tinclude \n@@ -46,15 +47,13 @@ const char *get_preferred_languages(void)\n \treturn NULL;\n }\n \n-#ifdef GETTEXT_POISON\n int use_gettext_poison(void)\n {\n \tstatic int poison_requested = -1;\n \tif (poison_requested == -1)\n-\t\tpoison_requested = getenv(\"GIT_GETTEXT_POISON\") ? 1 : 0;\n+\t\tpoison_requested = git_env_bool(\"GIT_TEST_GETTEXT_POISON\", 0);\n \treturn poison_requested;\n }\n-#endif\n \n #ifndef NO_GETTEXT\n static int test_vsnprintf(const char *fmt, ...)\ndiff --git a/gettext.h b/gettext.h\nindex 7eee64a34f..4c492d9f57 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -41,11 +41,7 @@ static inline int gettext_width(const char *s)\n }\n #endif\n \n-#ifdef GETTEXT_POISON\n extern int use_gettext_poison(void);\n-#else\n-#define use_gettext_poison() 0\n-#endif\n \n static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n {\ndiff --git a/git-sh-i18n.sh b/git-sh-i18n.sh\nindex 9d065fb4bf..c0713b1ee9 100644\n--- a/git-sh-i18n.sh\n+++ b/git-sh-i18n.sh\n@@ -17,7 +17,8 @@ export TEXTDOMAINDIR\n \n # First decide what scheme to use...\n GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough\n-if test -n \"$GIT_GETTEXT_POISON\"\n+if test -n \"$GIT_TEST_GETTEXT_POISON\" &&\n+\t git sh-i18n--helper --git-test-gettext-poison\n then\n \tGIT_INTERNAL_GETTEXT_SH_SCHEME=poison\n elif test -n \"@@USE_GETTEXT_SCHEME@@\"\ndiff --git a/git.c b/git.c\nindex 5920f8019b..125c523720 100644\n--- a/git.c\n+++ b/git.c\n@@ -539,6 +539,7 @@ static struct cmd_struct commands[] = {\n \t{ \"rm\", cmd_rm, RUN_SETUP },\n \t{ \"send-pack\", cmd_send_pack, RUN_SETUP },\n \t{ \"serve\", cmd_serve, RUN_SETUP },\n+\t{ \"sh-i18n--helper\", cmd_sh_i18n__helper, 0 },\n \t{ \"shortlog\", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER },\n \t{ \"show\", cmd_show, RUN_SETUP },\n \t{ \"show-branch\", cmd_show_branch, RUN_SETUP },\ndiff --git a/po/README b/po/README\nindex fef4c0f0b5..07595d369b 100644\n--- a/po/README\n+++ b/po/README\n@@ -289,16 +289,11 @@ something in the test suite might still depend on the US English\n version of the strings, e.g. to grep some error message or other\n output.\n \n-To smoke out issues like these Git can be compiled with gettext poison\n-support, at the top-level:\n+To smoke out issues like these, Git tested with a translation mode that\n+emits gibberish on every call to gettext. To use it run the test suite\n+with it, e.g.:\n \n- make GETTEXT_POISON=YesPlease\n-\n-That'll give you a git which emits gibberish on every call to\n-gettext. It's obviously not meant to be installed, but you should run\n-the test suite with it:\n-\n- cd t && prove -j 9 ./t[0-9]*.sh\n+ cd t && GIT_TEST_GETTEXT_POISON=true prove -j 9 ./t[0-9]*.sh\n \n If tests break with it you should inspect them manually and see if\n what you're translating is sane, i.e. that you're not translating\ndiff --git a/t/README b/t/README\nindex 8847489640..53c3dee7a9 100644\n--- a/t/README\n+++ b/t/README\n@@ -301,6 +301,12 @@ that cannot be easily covered by a few specific test cases. These\n could be enabled by running the test suite with correct GIT_TEST_\n environment set.\n \n+GIT_TEST_GETTEXT_POISON= turns all strings marked for\n+translation into gibberish. Used for spotting those tests that need to\n+be marked with a C_LOCALE_OUTPUT prerequisite when adding more strings\n+for translation. See \"Testing marked strings\" in po/README for\n+details.\n+\n GIT_TEST_SPLIT_INDEX= forces split-index mode on the whole\n test suite. Accept any boolean values that are accepted by git-config.\n \ndiff --git a/t/lib-gettext.sh b/t/lib-gettext.sh\nindex eec757f104..755f421431 100644\n--- a/t/lib-gettext.sh\n+++ b/t/lib-gettext.sh\n@@ -12,7 +12,7 @@ export GIT_TEXTDOMAINDIR GIT_PO_PATH\n \n . \"$GIT_BUILD_DIR\"/git-sh-i18n\n \n-if test_have_prereq GETTEXT && ! test_have_prereq GETTEXT_POISON\n+if test_have_prereq GETTEXT && test_have_prereq C_LOCALE_OUTPUT\n then\n \t# is_IS.UTF-8 on Solaris and FreeBSD, is_IS.utf8 on Debian\n \tis_IS_locale=$(locale -a 2>/dev/null |\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 4d23373526..b6566003dd 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -274,7 +274,7 @@ test_expect_success 'pretend we have a mix of all possible results' \"\n \tEOF\n \"\n \n-test_expect_success 'test --verbose' '\n+test_expect_success C_LOCALE_OUTPUT 'test --verbose' '\n \ttest_must_fail run_sub_test_lib_test \\\n \t\ttest-verbose \"test verbose\" --verbose <<-\\EOF &&\n \ttest_expect_success \"passing test\" true\ndiff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh\nindex 438e778d6a..1675d3e171 100755\n--- a/t/t0205-gettext-poison.sh\n+++ b/t/t0205-gettext-poison.sh\n@@ -5,13 +5,15 @@\n \n test_description='Gettext Shell poison'\n \n+GIT_TEST_GETTEXT_POISON=true\n+export GIT_TEST_GETTEXT_POISON\n . ./lib-gettext.sh\n \n-test_expect_success GETTEXT_POISON 'sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME\" is poison' '\n+test_expect_success 'sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME\" is poison' '\n test \"$GIT_INTERNAL_GETTEXT_SH_SCHEME\" = \"poison\"\n '\n \n-test_expect_success GETTEXT_POISON 'gettext: our gettext() fallback has poison semantics' '\n+test_expect_success 'gettext: our gettext() fallback has poison semantics' '\n printf \"# GETTEXT POISON #\" >expect &&\n gettext \"test\" >actual &&\n test_cmp expect actual &&\n@@ -20,7 +22,7 @@ test_expect_success GETTEXT_POISON 'gettext: our gettext() fallback has poison s\n test_cmp expect actual\n '\n \n-test_expect_success GETTEXT_POISON 'eval_gettext: our eval_gettext() fallback has poison semantics' '\n+test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semantics' '\n printf \"# GETTEXT POISON #\" >expect &&\n eval_gettext \"test\" >actual &&\n test_cmp expect actual &&\ndiff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\nindex 0392e36d23..2bdcf83808 100755\n--- a/t/t3406-rebase-message.sh\n+++ b/t/t3406-rebase-message.sh\n@@ -77,7 +77,7 @@ test_expect_success 'rebase -n overrides config rebase.stat config' '\n # \"Does not point to a valid commit: invalid-ref\"\n #\n # NEEDSWORK: This \"grep\" is fine in real non-C locales, but\n-# GETTEXT_POISON poisons the refname along with the enclosing\n+# GIT_TEST_GETTEXT_POISON poisons the refname along with the enclosing\n # error message.\n test_expect_success 'rebase --onto outputs the invalid ref' '\n \ttest_must_fail git rebase --onto invalid-ref HEAD HEAD 2>err &&\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 826987ca80..71e8e0167f 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -254,9 +254,9 @@ test_expect_success 'checkout to detach HEAD (with advice declined)' '\n test_expect_success 'checkout to detach HEAD' '\n \tgit config advice.detachedHead true &&\n \tgit checkout -f renamer && git clean -f &&\n-\tgit checkout renamer^ 2>messages &&\n-\ttest_i18ngrep \"HEAD is now at 7329388\" messages &&\n-\t(test_line_count -gt 1 messages || test -n \"$GETTEXT_POISON\") &&\n+\tGIT_TEST_GETTEXT_POISON=false git checkout renamer^ 2>messages &&\n+\tgrep \"HEAD is now at 7329388\" messages &&\n+\ttest_line_count -gt 1 messages &&\n \tH=$(git rev-parse --verify HEAD) &&\n \tM=$(git show-ref -s --verify refs/heads/master) &&\n \ttest \"z$H\" = \"z$M\" &&\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 175f83d704..537f5fdada 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1697,7 +1697,8 @@ test_expect_success 'sourcing the completion script clears cached commands' '\n \tverbose test -z \"$__git_all_commands\"\n '\n \n-test_expect_success !GETTEXT_POISON 'sourcing the completion script clears cached merge strategies' '\n+test_expect_success 'sourcing the completion script clears cached merge strategies' '\n+\tGIT_TEST_GETTEXT_POISON=false &&\n \t__git_compute_merge_strategies &&\n \tverbose test -n \"$__git_merge_strategies\" &&\n \t. \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 78d8c3783b..2f42b3653c 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -755,16 +755,16 @@ test_cmp_bin() {\n \n # Use this instead of test_cmp to compare files that contain expected and\n # actual output from git commands that can be translated. When running\n-# under GETTEXT_POISON this pretends that the command produced expected\n+# under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected\n # results.\n test_i18ncmp () {\n-\ttest -n \"$GETTEXT_POISON\" || test_cmp \"$@\"\n+\t! test_have_prereq C_LOCALE_OUTPUT || test_cmp \"$@\"\n }\n \n # Use this instead of \"grep expected-string actual\" to see if the\n # output from a git command that can be translated either contains an\n # expected string, or does not contain an unwanted one. When running\n-# under GETTEXT_POISON this pretends that the command produced expected\n+# under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected\n # results.\n test_i18ngrep () {\n \teval \"last_arg=\\${$#}\"\n@@ -779,7 +779,7 @@ test_i18ngrep () {\n \t\terror \"bug in the test script: too few parameters to test_i18ngrep\"\n \tfi\n \n-\tif test -n \"$GETTEXT_POISON\"\n+\tif test_have_prereq !C_LOCALE_OUTPUT\n \tthen\n \t\t# pretend success\n \t\treturn 0\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 897e6fcc94..892591ac84 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1104,15 +1104,9 @@ test -n \"$USE_LIBPCRE1\" && test_set_prereq LIBPCRE1\n test -n \"$USE_LIBPCRE2\" && test_set_prereq LIBPCRE2\n test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n \n-# Can we rely on git's output in the C locale?\n-if test -n \"$GETTEXT_POISON\"\n-then\n-\tGIT_GETTEXT_POISON=YesPlease\n-\texport GIT_GETTEXT_POISON\n-\ttest_set_prereq GETTEXT_POISON\n-else\n-\ttest_set_prereq C_LOCALE_OUTPUT\n-fi\n+## Fails in the 'branch -vv' test in t6040-tracking-info.sh\n+#test_lazy_prereq C_LOCALE_OUTPUT '! git sh-i18n--helper --git-test-gettext-poison'\n+git sh-i18n--helper --git-test-gettext-poison || test_set_prereq C_LOCALE_OUTPUT\n \n if test -z \"$GIT_TEST_CHECK_CACHE_TREE\"\n then\n-- \n2.19.1.568.g152ad8e336\n\n"},{"id":"361412","messageId":"CACsJy8Dex3VYEXmvRZv5_ot1-cwjJtir=kvupzKe7-Z2qPZw+Q@mail.gmail.com","threadId":"49637","inReplyTo":"871s8gd32p.fsf@evledraar.gmail.com","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-10-24T14:41:57Z","receivedAt":"2018-10-24T14:42:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Oct 23, 2018 at 6:45 PM Ævar Arnfjörð Bjarmason\n wrote:\n> >> The effect of what I'm suggesting here, and which my WIP patch in\n> >> <875zxtd59e.fsf@evledraar.gmail.com> implements is that we'd do a\n> >> one-time getenv() for each process that prints a _() message that we\n> >> aren't doing now, and for each message call a function that would check\n> >> a boolean \"are we in poison mode\" static global variable.\n> >\n> > Just don't do the getenv() inside _() even if you just do it one time.\n> > getenv() may invalidate whatever value from the previous call. I would\n> > not want to find out my code broke because of an getenv() inside some\n> > innocent _()...\n>\n> How would any code break? We have various getenv(\"GIT_TEST_*...\")\n> already that work the same way. Unless you set that in the environment\n> the function is idempotent, and I don't see how anyone would do that\n> accidentally.\n\nI didn't check those GIT_TEST_ but I think the difference is in\ncomplexity. When you write\n\n var = getenv(\"foo\");\n complexFunction();\n\nyou probably start to feel scary and wrap getenv() with a strdup()\nbecause you usually don't know exactly what complexFunction() can call\n(and even if you do, you can't be sure what it may call in the\nfuture).\n\nThe person who writes\n\n printf(_(\"%s\"), getenv(\"foo\"));\n\nmay not go through the same thought process as with complexFunction().\nIf _() calls getenv(), because you the order of parameter evaluation\nis unspecified, you cannot be sure if getenv(\"foo\") will be called\nbefore or after the one inside _(). One of them may screw up the\nother.\n\n> > And we should still respect NO_GETTEXT, not doing any extra work when\n> > it's defined.\n>\n> This is not how any of our NO_* defines work. *Sometimes* defining them\n> guarantees you do no extra work, but in other cases we've decided that\n> bending over backwards with #ifdef in various places isn't worth it.\n\nI guess it boils down to \"worth it\". For me #ifdef NO_GETTEXT would be\nlimited to gettext.h and it's not that much work. But you do the\nactual work. You decide.\n-- \nDuy\n"},{"id":"361432","messageId":"87woq7b58k.fsf@evledraar.gmail.com","threadId":"49637","inReplyTo":"CACsJy8Dex3VYEXmvRZv5_ot1-cwjJtir=kvupzKe7-Z2qPZw+Q@mail.gmail.com","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-24T17:54:03Z","receivedAt":"2018-10-24T17:54:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Oct 24 2018, Duy Nguyen wrote:\n\n> On Tue, Oct 23, 2018 at 6:45 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>> >> The effect of what I'm suggesting here, and which my WIP patch in\n>> >> <875zxtd59e.fsf@evledraar.gmail.com> implements is that we'd do a\n>> >> one-time getenv() for each process that prints a _() message that we\n>> >> aren't doing now, and for each message call a function that would check\n>> >> a boolean \"are we in poison mode\" static global variable.\n>> >\n>> > Just don't do the getenv() inside _() even if you just do it one time.\n>> > getenv() may invalidate whatever value from the previous call. I would\n>> > not want to find out my code broke because of an getenv() inside some\n>> > innocent _()...\n>>\n>> How would any code break? We have various getenv(\"GIT_TEST_*...\")\n>> already that work the same way. Unless you set that in the environment\n>> the function is idempotent, and I don't see how anyone would do that\n>> accidentally.\n>\n> I didn't check those GIT_TEST_ but I think the difference is in\n> complexity. When you write\n>\n> var = getenv(\"foo\");\n> complexFunction();\n>\n> you probably start to feel scary and wrap getenv() with a strdup()\n> because you usually don't know exactly what complexFunction() can call\n> (and even if you do, you can't be sure what it may call in the\n> future).\n>\n> The person who writes\n>\n> printf(_(\"%s\"), getenv(\"foo\"));\n>\n> may not go through the same thought process as with complexFunction().\n> If _() calls getenv(), because you the order of parameter evaluation\n> is unspecified, you cannot be sure if getenv(\"foo\") will be called\n> before or after the one inside _(). One of them may screw up the\n> other.\n\nAh, you mean because getenv() is not reentrant such a construct may\ncause us to return something else entirely for getenv(\"bar\") (e.g. in\nthis case the value for getenv(\"bar\")).\n\nIs that something you or anyone else has seen in practice? My intuition\nis that while POSIX doesn't make that promise it isn't likely that\nthere's any implementation that would mutate the env purely on a\ngetenv(), but setenv() (munging some static \"env\" area in-place) might\nbe another matter.\n\nBut we could always call use_gettext_poison() really early from\ngit_setup_gettext() (called from our main()) if we're worried about\nthis, it would then set the static \"poison_requested\" variable and we\nwouldn't call getenv() again, e.g. if we're later calling it in the\nmiddle of multithreaded code, or within the same same sequence point.\n\n>> > And we should still respect NO_GETTEXT, not doing any extra work when\n>> > it's defined.\n>>\n>> This is not how any of our NO_* defines work. *Sometimes* defining them\n>> guarantees you do no extra work, but in other cases we've decided that\n>> bending over backwards with #ifdef in various places isn't worth it.\n>\n> I guess it boils down to \"worth it\". For me #ifdef NO_GETTEXT would be\n> limited to gettext.h and it's not that much work. But you do the\n> actual work. You decide.\n\nYeah the ifdef is pretty small and not really worth worrynig about, the\nmain benefit is being able to run tests in this mode without\nrecompiling.\n\nI hadn't been running with GETTEXT_POISON when I build git because I\nhadn't been bothered to build twice just for it, but am now running it\nalongside other GIT_TEST_* modes.\n"},{"id":"361440","messageId":"xmqqefcen8ls.fsf@gitster-ct.c.googlers.com","threadId":"49637","inReplyTo":"20181024074400.GA31239@sigill.intra.peff.net","subject":"Re: [PATCH] i18n: make GETTEXT_POISON a runtime option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-25T01:00:31Z","receivedAt":"2018-10-25T01:00:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> but then you lose the default handling. I think if we added a new\n> option, it would either be:\n>\n> # interpret a value directly; use default on empty, I guess?\n> git config --default=false --type=bool --interpret-value \"$GIT_WHATEVER_ENV\"\n>\n> or\n>\n> # less flexible, but the --default semantics are more obvious\n> git config --default=false --type=bool --get-env GIT_WHATEVER_ENV\n\nYeah, my thinko. The latter would be closer to what this patch\nwants to have, but obviously the former would be more flexible and\nuseful in wider context. Both have the \"Huh?\" factor---what they\nare doing has little to do with \"config\", but I did not think of a\nbetter kitchen-sink (and our default kitchen-sink \"rev-parse\" is\neven further than \"config\", I would think, for this one).\n"},{"id":"361442","messageId":"20181025010905.GA4458@sigill.intra.peff.net","threadId":"49637","inReplyTo":"xmqqefcen8ls.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] i18n: make GETTEXT_POISON a runtime option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-25T01:09:05Z","receivedAt":"2018-10-25T01:09:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 25, 2018 at 10:00:31AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > but then you lose the default handling. I think if we added a new\n> > option, it would either be:\n> >\n> > # interpret a value directly; use default on empty, I guess?\n> > git config --default=false --type=bool --interpret-value \"$GIT_WHATEVER_ENV\"\n> >\n> > or\n> >\n> > # less flexible, but the --default semantics are more obvious\n> > git config --default=false --type=bool --get-env GIT_WHATEVER_ENV\n> \n> Yeah, my thinko. The latter would be closer to what this patch\n> wants to have, but obviously the former would be more flexible and\n> useful in wider context. Both have the \"Huh?\" factor---what they\n> are doing has little to do with \"config\", but I did not think of a\n> better kitchen-sink (and our default kitchen-sink \"rev-parse\" is\n> even further than \"config\", I would think, for this one).\n\nHeh, I thought through the exact sequence in your paragraph when writing\nmy other message. That's probably a good sign that we should probably\nnot pursue this further unless we see the use case come up again a few\nmore times (and if we do, then consider \"config\" the least-bad place to\ndo it).\n\n-Peff\n"},{"id":"361445","messageId":"a5535030-65c1-613c-e4ea-d4379b243b87@ramsayjones.plus.com","threadId":"49637","inReplyTo":"20181025010905.GA4458@sigill.intra.peff.net","subject":"Re: [PATCH] i18n: make GETTEXT_POISON a runtime option","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-10-25T01:24:41Z","receivedAt":"2018-10-25T01:24:47Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 25/10/2018 02:09, Jeff King wrote:\n> On Thu, Oct 25, 2018 at 10:00:31AM +0900, Junio C Hamano wrote:\n> \n>> Jeff King writes:\n>>\n>>> but then you lose the default handling. I think if we added a new\n>>> option, it would either be:\n>>>\n>>> # interpret a value directly; use default on empty, I guess?\n>>> git config --default=false --type=bool --interpret-value \"$GIT_WHATEVER_ENV\"\n>>>\n>>> or\n>>>\n>>> # less flexible, but the --default semantics are more obvious\n>>> git config --default=false --type=bool --get-env GIT_WHATEVER_ENV\n>>\n>> Yeah, my thinko. The latter would be closer to what this patch\n>> wants to have, but obviously the former would be more flexible and\n>> useful in wider context. Both have the \"Huh?\" factor---what they\n>> are doing has little to do with \"config\", but I did not think of a\n>> better kitchen-sink (and our default kitchen-sink \"rev-parse\" is\n>> even further than \"config\", I would think, for this one).\n> \n> Heh, I thought through the exact sequence in your paragraph when writing\n> my other message. That's probably a good sign that we should probably\n> not pursue this further unless we see the use case come up again a few\n> more times (and if we do, then consider \"config\" the least-bad place to\n> do it).\n\nI was thinking:\n\n $ git var -e GIT_WHATEVER_ENV\n\n[-e for environment].\n\n... but that is really no different than git-config. ;-)\n\nATB,\nRamsay Jones\n\n\n\n"},{"id":"361464","messageId":"xmqqh8halm20.fsf@gitster-ct.c.googlers.com","threadId":"49637","inReplyTo":"CACsJy8Dex3VYEXmvRZv5_ot1-cwjJtir=kvupzKe7-Z2qPZw+Q@mail.gmail.com","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-25T03:52:55Z","receivedAt":"2018-10-25T03:53:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> The person who writes\n>\n> printf(_(\"%s\"), getenv(\"foo\"));\n>\n> may not go through the same thought process as with complexFunction().\n> If _() calls getenv(), because you the order of parameter evaluation\n> is unspecified, you cannot be sure if getenv(\"foo\") will be called\n> before or after the one inside _(). One of them may screw up the\n> other.\n\nYup, sometimes we've been sloppy but we should strive to mimick\nefforts like f4ef5173 (\"determine_author_info(): copy getenv\noutput\", 2014-08-27).\n"},{"id":"361478","messageId":"20181025062037.GC11460@sigill.intra.peff.net","threadId":"49637","inReplyTo":"xmqqh8halm20.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-25T06:20:38Z","receivedAt":"2018-10-25T06:20:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 25, 2018 at 12:52:55PM +0900, Junio C Hamano wrote:\n\n> Duy Nguyen writes:\n> \n> > The person who writes\n> >\n> > printf(_(\"%s\"), getenv(\"foo\"));\n> >\n> > may not go through the same thought process as with complexFunction().\n> > If _() calls getenv(), because you the order of parameter evaluation\n> > is unspecified, you cannot be sure if getenv(\"foo\") will be called\n> > before or after the one inside _(). One of them may screw up the\n> > other.\n> \n> Yup, sometimes we've been sloppy but we should strive to mimick\n> efforts like f4ef5173 (\"determine_author_info(): copy getenv\n> output\", 2014-08-27).\n\nI've wondered about this before. Even calling:\n\n foo = xstrdup(getenv(\"bar\"));\n\nis not necessarily correct, because xstrdup() relies on xmalloc(), which\nmay check GIT_ALLOC_LIMIT (we do cache that result, but it can happen on\nthe first malloc).\n\nI also wouldn't be surprised if there are cases in our threaded code\nthat use getenv() without taking a lock.\n\nI've definitely run into setenv()/getenv() races on Linux (inside Git,\neven, though it was while working on custom code). But I wonder how\ncommon it is for getenv() to be invalidated by another getenv() call.\nCertainly POSIX allows it, but every implementation I've seen (which is\nadmittedly few) is passing back pointers to a chunk of environment\nmemory.\n\nI.e., could we mostly ignore this problem as not applying to most modern\nsystems? And if there is such a system, give it a fallback like:\n\n /*\n * For systems that use a single buffer for getenv(), this hacks\n * around it by giving it _four_ buffers. That's just punting on\n * the problem, but it at least gives enough breathing room for\n * the caller to do something sane like use non-trivial functions\n * to copy the string. It still does nothing for threading, but\n * hopefully such systems don't support pthreads in the first place. ;)\n */\n const char *xgetenv(const char *key)\n {\n\tstatic struct strbuf bufs[] = {\n\t\tSTRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT\n\t};\n\tstatic unsigned int cur;\n\tstruct strbuf *buf;\n\tconst char *value;\n\tsize_t len;\n\n\tvalue = getenv(key);\n\tif (!value)\n\t\treturn NULL;\n\n\tbuf = bufs[cur++];\n\tcur %= ARRAY_SIZE(bufs);\n\n\t/*\n\t * We have to do this length check ourselves, because allocating\n\t * the strbuf may invalidate \"value\"!\n\t */\n\tlen = strlen(value);\n\tif (buf->alloc <= len) {\n\t\tstrbuf_grow(buf, len);\n\t\tvalue = getenv(key);\n\t\tif (!value)\n\t\t\treturn NULL; /* whoops, it went away! */\n\t\tlen = strlen(value); /* paranoia that it didn't change */\n\t}\n\n\tstrbuf_reset(buf);\n\tstrbuf_add(buf, value, len);\n\n\treturn buf->buf;\n }\n\nI dunno. Maybe I am being overly optimistic. But I strongly suspect we\nhave such bugs already in our code base, and nobody has run into them\n(OTOH, they are quite finicky due to things like the caching I\nmentioned).\n\n-Peff\n"},{"id":"361563","messageId":"20181025212358.GA23257@sigill.intra.peff.net","threadId":"49637","inReplyTo":"a5535030-65c1-613c-e4ea-d4379b243b87@ramsayjones.plus.com","subject":"Re: [PATCH] i18n: make GETTEXT_POISON a runtime option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-25T21:23:58Z","receivedAt":"2018-10-25T21:24:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 25, 2018 at 02:24:41AM +0100, Ramsay Jones wrote:\n\n> >> Yeah, my thinko. The latter would be closer to what this patch\n> >> wants to have, but obviously the former would be more flexible and\n> >> useful in wider context. Both have the \"Huh?\" factor---what they\n> >> are doing has little to do with \"config\", but I did not think of a\n> >> better kitchen-sink (and our default kitchen-sink \"rev-parse\" is\n> >> even further than \"config\", I would think, for this one).\n> > \n> > Heh, I thought through the exact sequence in your paragraph when writing\n> > my other message. That's probably a good sign that we should probably\n> > not pursue this further unless we see the use case come up again a few\n> > more times (and if we do, then consider \"config\" the least-bad place to\n> > do it).\n> \n> I was thinking:\n> \n> $ git var -e GIT_WHATEVER_ENV\n> \n> [-e for environment].\n> \n> ... but that is really no different than git-config. ;-)\n\nActually, \"git var\" already does pull bits from the environment. It\ndoesn't know about all of the type-specific parsing that git-config\ndoes, but it might be a reasonable path forward to teach it that. (But I\nstill think we should do nothing for now and see how often this comes\nup).\n\n-Peff\n"},{"id":"361634","messageId":"87pnvwldk7.fsf@evledraar.gmail.com","threadId":"49637","inReplyTo":"20181025212358.GA23257@sigill.intra.peff.net","subject":"Re: [PATCH] i18n: make GETTEXT_POISON a runtime option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-26T19:20:56Z","receivedAt":"2018-10-26T19:21:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 25 2018, Jeff King wrote:\n\n> On Thu, Oct 25, 2018 at 02:24:41AM +0100, Ramsay Jones wrote:\n>\n>> >> Yeah, my thinko. The latter would be closer to what this patch\n>> >> wants to have, but obviously the former would be more flexible and\n>> >> useful in wider context. Both have the \"Huh?\" factor---what they\n>> >> are doing has little to do with \"config\", but I did not think of a\n>> >> better kitchen-sink (and our default kitchen-sink \"rev-parse\" is\n>> >> even further than \"config\", I would think, for this one).\n>> >\n>> > Heh, I thought through the exact sequence in your paragraph when writing\n>> > my other message. That's probably a good sign that we should probably\n>> > not pursue this further unless we see the use case come up again a few\n>> > more times (and if we do, then consider \"config\" the least-bad place to\n>> > do it).\n>>\n>> I was thinking:\n>>\n>> $ git var -e GIT_WHATEVER_ENV\n>>\n>> [-e for environment].\n>>\n>> ... but that is really no different than git-config. ;-)\n>\n> Actually, \"git var\" already does pull bits from the environment. It\n> doesn't know about all of the type-specific parsing that git-config\n> does, but it might be a reasonable path forward to teach it that. (But I\n> still think we should do nothing for now and see how often this comes\n> up).\n\nFor myself / Junio picking this up: Does that mean you've read v2 and\nthink it's OK to pick up in its current form? I think it is, just\nlooking for some Acks on that since it's not in the latest \"What's\nCooking\".\n"},{"id":"361748","messageId":"xmqqftwrc20q.fsf@gitster-ct.c.googlers.com","threadId":"49637","inReplyTo":"20181025062037.GC11460@sigill.intra.peff.net","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-27T06:55:01Z","receivedAt":"2018-10-27T06:55:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I.e., could we mostly ignore this problem as not applying to most modern\n> systems? And if there is such a system, give it a fallback like:\n>\n> /*\n> * For systems that use a single buffer for getenv(), this hacks\n> * around it by giving it _four_ buffers. That's just punting on\n> * the problem, but it at least gives enough breathing room for\n> * the caller to do something sane like use non-trivial functions\n> * to copy the string. It still does nothing for threading, but\n> * hopefully such systems don't support pthreads in the first place. ;)\n> */\n\nTempting.\n\n> I dunno. Maybe I am being overly optimistic. But I strongly suspect we\n> have such bugs already in our code base, and nobody has run into them\n\nYeah, I tend to agree that I would not be surprised at all if we\nhave many such \"bugs\" that triggers for nobody on modern platforms.\n\n"},{"id":"361749","messageId":"20181027065952.GA17203@sigill.intra.peff.net","threadId":"49637","inReplyTo":"87pnvwldk7.fsf@evledraar.gmail.com","subject":"Re: [PATCH] i18n: make GETTEXT_POISON a runtime option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-27T06:59:52Z","receivedAt":"2018-10-27T06:59:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 26, 2018 at 09:20:56PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> >> I was thinking:\n> >>\n> >> $ git var -e GIT_WHATEVER_ENV\n> >>\n> >> [-e for environment].\n> >>\n> >> ... but that is really no different than git-config. ;-)\n> >\n> > Actually, \"git var\" already does pull bits from the environment. It\n> > doesn't know about all of the type-specific parsing that git-config\n> > does, but it might be a reasonable path forward to teach it that. (But I\n> > still think we should do nothing for now and see how often this comes\n> > up).\n> \n> For myself / Junio picking this up: Does that mean you've read v2 and\n> think it's OK to pick up in its current form? I think it is, just\n> looking for some Acks on that since it's not in the latest \"What's\n> Cooking\".\n\nI'm not sure if you're asking whether I looked at the rest of the patch.\nIf so, then no, not really (so no objection, but I also did not review\nit).\n\n-Peff\n"},{"id":"361781","messageId":"86va5n4s3d.fsf@gmail.com","threadId":"49637","inReplyTo":"20181022202241.18629-1-szeder.dev@gmail.com","subject":"Re: [PATCH] Poison gettext with the Ook language","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2018-10-27T10:11:18Z","receivedAt":"2018-10-27T10:16:04Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"SZEDER Gábor writes:\n> On Mon, Oct 22, 2018 at 05:36:33PM +0200, Nguyễn Thái Ngọc Duy wrote:\n>>\n>> The current gettext() function just replaces all strings with\n>> '# GETTEXT POISON #' including format strings and hides the things\n>> that we should be allowed to grep (like branch names, or some other\n>> codes) even when gettext is poisoned.\n>> \n>> This patch implements the poisoned _() with a universal and totally\n>> legit language called Ook [1]. We could actually grep stuff even in\n>> with this because format strings are preserved.\n>\n> Once upon a time a GETTEXT_POISON build job failed on me, and the\n> error message:\n>\n> error: # GETTEXT POISON #\n>\n> was not particularly useful. Ook wouldn't help with that...\n>\n> So I came up with the following couple of patches that implement a\n> \"scrambled\" format that makes the poisoned output legible for humans\n> but still gibberish for machine consumption (i.e. grep-ing the text\n> part would still fail):\n>\n> error: U.n.a.b.l.e. .t.o. .c.r.e.a.t.e. .'./home/szeder/src/git/t/trash directory.t1404-update-ref-errors/.git/packed-refs...l.o.c.k.'.:. .File exists...\n>\n> I have been running GETTEXT_POISON builds with this series for some\n> months now, but haven't submitted it yet, because I haven't decided\n> yet whether including strings (paths, refs, etc.) in the output as\n> they are is a feature or a flaw. And because it embarrassingly leaks\n> every single translated string... :)\n\nThere is similar technique called \"pseudolocalization\", meant for\ntesting i18n aspect of software.\n\nIn one of most common forms, the string\n\n Edit program settings\n\nwoukd be translated to\n\n [!!! εÐiţ Þr0ģЯãm səTτıИğ§ !!!]\n\n(possibly using mirrored locale, i.e. right-to-left order).\n\nThe brackets [!!! ... !!!] are used as a \"poison\", to detect\ntranslatable text, and to spot issues with truncation; it also helps\nwith finding \"lego\" translation.\n\nIt would also stress-test Unicode handling...\n\nRegards,\n-- \nJakub Narębski\n\n"},{"id":"361782","messageId":"87muqzllh0.fsf@evledraar.gmail.com","threadId":"49637","inReplyTo":"20181027065952.GA17203@sigill.intra.peff.net","subject":"Re: [PATCH] i18n: make GETTEXT_POISON a runtime option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-10-27T10:42:19Z","receivedAt":"2018-10-27T10:42:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Oct 27 2018, Jeff King wrote:\n\n> On Fri, Oct 26, 2018 at 09:20:56PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> >> I was thinking:\n>> >>\n>> >> $ git var -e GIT_WHATEVER_ENV\n>> >>\n>> >> [-e for environment].\n>> >>\n>> >> ... but that is really no different than git-config. ;-)\n>> >\n>> > Actually, \"git var\" already does pull bits from the environment. It\n>> > doesn't know about all of the type-specific parsing that git-config\n>> > does, but it might be a reasonable path forward to teach it that. (But I\n>> > still think we should do nothing for now and see how often this comes\n>> > up).\n>>\n>> For myself / Junio picking this up: Does that mean you've read v2 and\n>> think it's OK to pick up in its current form? I think it is, just\n>> looking for some Acks on that since it's not in the latest \"What's\n>> Cooking\".\n>\n> I'm not sure if you're asking whether I looked at the rest of the patch.\n> If so, then no, not really (so no objection, but I also did not review\n> it).\n\nYeah I'm fishing for a more general review than just the problem of how\nwe turn an env variable into a boolean, so if you or anyone else is up\nfor it it would be most welcome :)\n"},{"id":"362208","messageId":"20181101193115.32681-1-avarab@gmail.com","threadId":"49637","inReplyTo":"20181024114725.3927-1-avarab@gmail.com","subject":"[PATCH v3] i18n: make GETTEXT_POISON a runtime option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-01T19:31:15Z","receivedAt":"2018-11-01T19:31:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON\ntest parameter to only be a GIT_TEST_GETTEXT_POISON=\nruntime parameter, to be consistent with other parameters documented\nin \"Running tests with special setups\" in t/README.\n\nWhen I added GETTEXT_POISON in bb946bba76 (\"i18n: add GETTEXT_POISON\nto simulate unfriendly translator\", 2011-02-22) I was concerned with\nensuring that the _() function would get constant folded if NO_GETTEXT\nwas defined, and likewise that GETTEXT_POISON would be compiled out\nunless it was defined.\n\nBut as the benchmark in my [1] shows doing a one-off runtime\ngetenv(\"GIT_TEST_[...]\") is trivial, and since GETTEXT_POISON was\noriginally added the GIT_TEST_* env variables have become the common\nidiom for turning on special test setups.\n\nSo change GETTEXT_POISON to work the same way. Now the\nGETTEXT_POISON=YesPlease compile-time option is gone, and running the\ntests with GIT_TEST_GETTEXT_POISON=[YesPlease|] can be toggled on/off\nwithout recompiling.\n\nThis allows for conditionally amending tests to test with/without\npoison, similar to what 859fdc0c3c (\"commit-graph: define\nGIT_TEST_COMMIT_GRAPH\", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH. Do\nsome of that, now we e.g. always run the t0205-gettext-poison.sh test.\n\nI did enough there to remove the GETTEXT_POISON prerequisite, but its\ninverse C_LOCALE_OUTPUT is still around, and surely some tests using\nit can be converted to e.g. always set GIT_TEST_GETTEXT_POISON=.\n\nNotes on the implementation:\n\n * We still compile a dedicated GETTEXT_POISON build in Travis CI.\n This is probably the wrong thing to do and should be followed-up\n with something similar to ae59a4e44f (\"travis: run tests with\n GIT_TEST_SPLIT_INDEX\", 2018-01-07) to re-use an existing test setup\n for running in the GIT_TEST_GETTEXT_POISON mode.\n\n * We now skip a test in t0000-basic.sh under\n GIT_TEST_GETTEXT_POISON=YesPlease that wasn't skipped before. This\n test relies on C locale output, but due to an edge case in how the\n previous implementation of GETTEXT_POISON worked (reading it from\n GIT-BUILD-OPTIONS) wasn't enabling poison correctly. Now it does,\n and needs to be skipped.\n\n * The getenv() function is not reentrant, so out of paranoia about\n code of the form:\n\n printf(_(\"%s\"), getenv(\"some-env\"));\n\n call use_gettext_poison() in our early setup in git_setup_gettext()\n so we populate the \"poison_requested\" variable in a codepath that's\n won't suffer from that race condition.\n\nSee also [3] for more on the motivation behind this patch, and the\nhistory of the GETTEXT_POISON facility.\n\n1. https://public-inbox.org/git/871s8gd32p.fsf@evledraar.gmail.com/\n2. https://public-inbox.org/git/87woq7b58k.fsf@evledraar.gmail.com/\n3. https://public-inbox.org/git/878t2pd6yu.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nNow:\n\n * The new i18n helper is gone. We just use \"test -n\" semantics for\n $GIT_TEST_GETTEXT_POISON\n\n * We error out in the Makefile if you're still saying\n GETTEXT_POISON=YesPlease.\n\n This makes more sense than just making it a synonym since now this\n also needs to be defined at runtime.\n\n * The caveat with avoiding test_lazy_prereq is gone (although there's\n still some unrelated bug there worth looking into).\n\n * We call use_gettext_poison() really early to avoid any reentrancy\n issue with getenv().\n\n\n .travis.yml | 2 +-\n Makefile | 8 +-------\n ci/lib-travisci.sh | 4 ++--\n gettext.c | 11 +++++++----\n gettext.h | 9 +++------\n git-sh-i18n.sh | 2 +-\n po/README | 13 ++++---------\n t/README | 6 ++++++\n t/lib-gettext.sh | 2 +-\n t/t0000-basic.sh | 2 +-\n t/t0205-gettext-poison.sh | 8 +++++---\n t/t3406-rebase-message.sh | 2 +-\n t/t7201-co.sh | 6 +++---\n t/t9902-completion.sh | 3 ++-\n t/test-lib-functions.sh | 8 ++++----\n t/test-lib.sh | 6 +-----\n 16 files changed, 43 insertions(+), 49 deletions(-)\n\ndiff --git a/.travis.yml b/.travis.yml\nindex 4d4e26c9df..4523a2e5ec 100644\n--- a/.travis.yml\n+++ b/.travis.yml\n@@ -26,7 +26,7 @@ addons:\n \n matrix:\n include:\n- - env: jobname=GETTEXT_POISON\n+ - env: jobname=GIT_TEST_GETTEXT_POISON\n os: linux\n compiler:\n addons:\ndiff --git a/Makefile b/Makefile\nindex b08d5ea258..3a08626db0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -362,11 +362,6 @@ all::\n # Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the\n # user.\n #\n-# Define GETTEXT_POISON if you are debugging the choice of strings marked\n-# for translation. In a GETTEXT_POISON build, you can turn all strings marked\n-# for translation into gibberish by setting the GIT_GETTEXT_POISON variable\n-# (to any value) in your environment.\n-#\n # Define JSMIN to point to JavaScript minifier that functions as\n # a filter to have gitweb.js minified.\n #\n@@ -1450,7 +1445,7 @@ ifdef NO_SYMLINK_HEAD\n \tBASIC_CFLAGS += -DNO_SYMLINK_HEAD\n endif\n ifdef GETTEXT_POISON\n-\tBASIC_CFLAGS += -DGETTEXT_POISON\n+$(error The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!)\n endif\n ifdef NO_GETTEXT\n \tBASIC_CFLAGS += -DNO_GETTEXT\n@@ -2602,7 +2597,6 @@ ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT\n \t@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@+\n endif\n \t@echo NO_GETTEXT=\\''$(subst ','\\'',$(subst ','\\'',$(NO_GETTEXT)))'\\' >>$@+\n-\t@echo GETTEXT_POISON=\\''$(subst ','\\'',$(subst ','\\'',$(GETTEXT_POISON)))'\\' >>$@+\n ifdef GIT_PERF_REPEAT_COUNT\n \t@echo GIT_PERF_REPEAT_COUNT=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_PERF_REPEAT_COUNT)))'\\' >>$@+\n endif\ndiff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh\nindex 06970f7213..69dff4d1ec 100755\n--- a/ci/lib-travisci.sh\n+++ b/ci/lib-travisci.sh\n@@ -123,7 +123,7 @@ osx-clang|osx-gcc)\n \t# Travis CI OS X\n \texport GIT_SKIP_TESTS=\"t9810 t9816\"\n \t;;\n-GETTEXT_POISON)\n-\texport GETTEXT_POISON=YesPlease\n+GIT_TEST_GETTEXT_POISON)\n+\texport GIT_TEST_GETTEXT_POISON=YesPlease\n \t;;\n esac\ndiff --git a/gettext.c b/gettext.c\nindex 7272771c8e..d4021d690c 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -7,6 +7,7 @@\n #include \"gettext.h\"\n #include \"strbuf.h\"\n #include \"utf8.h\"\n+#include \"config.h\"\n \n #ifndef NO_GETTEXT\n #\tinclude \n@@ -46,15 +47,15 @@ const char *get_preferred_languages(void)\n \treturn NULL;\n }\n \n-#ifdef GETTEXT_POISON\n int use_gettext_poison(void)\n {\n \tstatic int poison_requested = -1;\n-\tif (poison_requested == -1)\n-\t\tpoison_requested = getenv(\"GIT_GETTEXT_POISON\") ? 1 : 0;\n+\tif (poison_requested == -1) {\n+\t\tconst char *v = getenv(\"GIT_TEST_GETTEXT_POISON\");\n+\t\tpoison_requested = v && strlen(v) ? 1 : 0;\n+\t}\n \treturn poison_requested;\n }\n-#endif\n \n #ifndef NO_GETTEXT\n static int test_vsnprintf(const char *fmt, ...)\n@@ -164,6 +165,8 @@ void git_setup_gettext(void)\n \tif (!podir)\n \t\tpodir = p = system_path(GIT_LOCALE_PATH);\n \n+\tuse_gettext_poison(); /* getenv() reentrancy paranoia */\n+\n \tif (!is_directory(podir)) {\n \t\tfree(p);\n \t\treturn;\ndiff --git a/gettext.h b/gettext.h\nindex 7eee64a34f..43d991a2df 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -28,12 +28,15 @@\n \n #define FORMAT_PRESERVING(n) __attribute__((format_arg(n)))\n \n+extern int use_gettext_poison(void);\n+\n #ifndef NO_GETTEXT\n extern void git_setup_gettext(void);\n extern int gettext_width(const char *s);\n #else\n static inline void git_setup_gettext(void)\n {\n+\tuse_gettext_poison();; /* getenv() reentrancy paranoia */\n }\n static inline int gettext_width(const char *s)\n {\n@@ -41,12 +44,6 @@ static inline int gettext_width(const char *s)\n }\n #endif\n \n-#ifdef GETTEXT_POISON\n-extern int use_gettext_poison(void);\n-#else\n-#define use_gettext_poison() 0\n-#endif\n-\n static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n {\n \tif (!*msgid)\ndiff --git a/git-sh-i18n.sh b/git-sh-i18n.sh\nindex 9d065fb4bf..e1d917fd27 100644\n--- a/git-sh-i18n.sh\n+++ b/git-sh-i18n.sh\n@@ -17,7 +17,7 @@ export TEXTDOMAINDIR\n \n # First decide what scheme to use...\n GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough\n-if test -n \"$GIT_GETTEXT_POISON\"\n+if test -n \"$GIT_TEST_GETTEXT_POISON\"\n then\n \tGIT_INTERNAL_GETTEXT_SH_SCHEME=poison\n elif test -n \"@@USE_GETTEXT_SCHEME@@\"\ndiff --git a/po/README b/po/README\nindex fef4c0f0b5..aa704ffcb7 100644\n--- a/po/README\n+++ b/po/README\n@@ -289,16 +289,11 @@ something in the test suite might still depend on the US English\n version of the strings, e.g. to grep some error message or other\n output.\n \n-To smoke out issues like these Git can be compiled with gettext poison\n-support, at the top-level:\n+To smoke out issues like these, Git tested with a translation mode that\n+emits gibberish on every call to gettext. To use it run the test suite\n+with it, e.g.:\n \n- make GETTEXT_POISON=YesPlease\n-\n-That'll give you a git which emits gibberish on every call to\n-gettext. It's obviously not meant to be installed, but you should run\n-the test suite with it:\n-\n- cd t && prove -j 9 ./t[0-9]*.sh\n+ cd t && GIT_TEST_GETTEXT_POISON=YesPlease prove -j 9 ./t[0-9]*.sh\n \n If tests break with it you should inspect them manually and see if\n what you're translating is sane, i.e. that you're not translating\ndiff --git a/t/README b/t/README\nindex 8847489640..25c4ba3419 100644\n--- a/t/README\n+++ b/t/README\n@@ -301,6 +301,12 @@ that cannot be easily covered by a few specific test cases. These\n could be enabled by running the test suite with correct GIT_TEST_\n environment set.\n \n+GIT_TEST_GETTEXT_POISON= turns all strings marked for\n+translation into gibberish if non-empty (think \"test -n\"). Used for\n+spotting those tests that need to be marked with a C_LOCALE_OUTPUT\n+prerequisite when adding more strings for translation. See \"Testing\n+marked strings\" in po/README for details.\n+\n GIT_TEST_SPLIT_INDEX= forces split-index mode on the whole\n test suite. Accept any boolean values that are accepted by git-config.\n \ndiff --git a/t/lib-gettext.sh b/t/lib-gettext.sh\nindex eec757f104..755f421431 100644\n--- a/t/lib-gettext.sh\n+++ b/t/lib-gettext.sh\n@@ -12,7 +12,7 @@ export GIT_TEXTDOMAINDIR GIT_PO_PATH\n \n . \"$GIT_BUILD_DIR\"/git-sh-i18n\n \n-if test_have_prereq GETTEXT && ! test_have_prereq GETTEXT_POISON\n+if test_have_prereq GETTEXT && test_have_prereq C_LOCALE_OUTPUT\n then\n \t# is_IS.UTF-8 on Solaris and FreeBSD, is_IS.utf8 on Debian\n \tis_IS_locale=$(locale -a 2>/dev/null |\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 4d23373526..b6566003dd 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -274,7 +274,7 @@ test_expect_success 'pretend we have a mix of all possible results' \"\n \tEOF\n \"\n \n-test_expect_success 'test --verbose' '\n+test_expect_success C_LOCALE_OUTPUT 'test --verbose' '\n \ttest_must_fail run_sub_test_lib_test \\\n \t\ttest-verbose \"test verbose\" --verbose <<-\\EOF &&\n \ttest_expect_success \"passing test\" true\ndiff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh\nindex 438e778d6a..a06269f38a 100755\n--- a/t/t0205-gettext-poison.sh\n+++ b/t/t0205-gettext-poison.sh\n@@ -5,13 +5,15 @@\n \n test_description='Gettext Shell poison'\n \n+GIT_TEST_GETTEXT_POISON=YesPlease\n+export GIT_TEST_GETTEXT_POISON\n . ./lib-gettext.sh\n \n-test_expect_success GETTEXT_POISON 'sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME\" is poison' '\n+test_expect_success 'sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME\" is poison' '\n test \"$GIT_INTERNAL_GETTEXT_SH_SCHEME\" = \"poison\"\n '\n \n-test_expect_success GETTEXT_POISON 'gettext: our gettext() fallback has poison semantics' '\n+test_expect_success 'gettext: our gettext() fallback has poison semantics' '\n printf \"# GETTEXT POISON #\" >expect &&\n gettext \"test\" >actual &&\n test_cmp expect actual &&\n@@ -20,7 +22,7 @@ test_expect_success GETTEXT_POISON 'gettext: our gettext() fallback has poison s\n test_cmp expect actual\n '\n \n-test_expect_success GETTEXT_POISON 'eval_gettext: our eval_gettext() fallback has poison semantics' '\n+test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semantics' '\n printf \"# GETTEXT POISON #\" >expect &&\n eval_gettext \"test\" >actual &&\n test_cmp expect actual &&\ndiff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\nindex 0392e36d23..2bdcf83808 100755\n--- a/t/t3406-rebase-message.sh\n+++ b/t/t3406-rebase-message.sh\n@@ -77,7 +77,7 @@ test_expect_success 'rebase -n overrides config rebase.stat config' '\n # \"Does not point to a valid commit: invalid-ref\"\n #\n # NEEDSWORK: This \"grep\" is fine in real non-C locales, but\n-# GETTEXT_POISON poisons the refname along with the enclosing\n+# GIT_TEST_GETTEXT_POISON poisons the refname along with the enclosing\n # error message.\n test_expect_success 'rebase --onto outputs the invalid ref' '\n \ttest_must_fail git rebase --onto invalid-ref HEAD HEAD 2>err &&\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 826987ca80..72b9b375ba 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -254,9 +254,9 @@ test_expect_success 'checkout to detach HEAD (with advice declined)' '\n test_expect_success 'checkout to detach HEAD' '\n \tgit config advice.detachedHead true &&\n \tgit checkout -f renamer && git clean -f &&\n-\tgit checkout renamer^ 2>messages &&\n-\ttest_i18ngrep \"HEAD is now at 7329388\" messages &&\n-\t(test_line_count -gt 1 messages || test -n \"$GETTEXT_POISON\") &&\n+\tGIT_TEST_GETTEXT_POISON= git checkout renamer^ 2>messages &&\n+\tgrep \"HEAD is now at 7329388\" messages &&\n+\ttest_line_count -gt 1 messages &&\n \tH=$(git rev-parse --verify HEAD) &&\n \tM=$(git show-ref -s --verify refs/heads/master) &&\n \ttest \"z$H\" = \"z$M\" &&\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 175f83d704..3c6b185b60 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1697,7 +1697,8 @@ test_expect_success 'sourcing the completion script clears cached commands' '\n \tverbose test -z \"$__git_all_commands\"\n '\n \n-test_expect_success !GETTEXT_POISON 'sourcing the completion script clears cached merge strategies' '\n+test_expect_success 'sourcing the completion script clears cached merge strategies' '\n+\tGIT_TEST_GETTEXT_POISON= &&\n \t__git_compute_merge_strategies &&\n \tverbose test -n \"$__git_merge_strategies\" &&\n \t. \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 78d8c3783b..2f42b3653c 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -755,16 +755,16 @@ test_cmp_bin() {\n \n # Use this instead of test_cmp to compare files that contain expected and\n # actual output from git commands that can be translated. When running\n-# under GETTEXT_POISON this pretends that the command produced expected\n+# under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected\n # results.\n test_i18ncmp () {\n-\ttest -n \"$GETTEXT_POISON\" || test_cmp \"$@\"\n+\t! test_have_prereq C_LOCALE_OUTPUT || test_cmp \"$@\"\n }\n \n # Use this instead of \"grep expected-string actual\" to see if the\n # output from a git command that can be translated either contains an\n # expected string, or does not contain an unwanted one. When running\n-# under GETTEXT_POISON this pretends that the command produced expected\n+# under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected\n # results.\n test_i18ngrep () {\n \teval \"last_arg=\\${$#}\"\n@@ -779,7 +779,7 @@ test_i18ngrep () {\n \t\terror \"bug in the test script: too few parameters to test_i18ngrep\"\n \tfi\n \n-\tif test -n \"$GETTEXT_POISON\"\n+\tif test_have_prereq !C_LOCALE_OUTPUT\n \tthen\n \t\t# pretend success\n \t\treturn 0\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 897e6fcc94..370a4821e1 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1105,12 +1105,8 @@ test -n \"$USE_LIBPCRE2\" && test_set_prereq LIBPCRE2\n test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n \n # Can we rely on git's output in the C locale?\n-if test -n \"$GETTEXT_POISON\"\n+if test -z \"$GIT_TEST_GETTEXT_POISON\"\n then\n-\tGIT_GETTEXT_POISON=YesPlease\n-\texport GIT_GETTEXT_POISON\n-\ttest_set_prereq GETTEXT_POISON\n-else\n \ttest_set_prereq C_LOCALE_OUTPUT\n fi\n \n-- \n2.19.1.899.g0250525e69\n\n"},{"id":"362231","messageId":"xmqqlg6cqikj.fsf@gitster-ct.c.googlers.com","threadId":"49637","inReplyTo":"20181101193115.32681-1-avarab@gmail.com","subject":"Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-02T03:11:56Z","receivedAt":"2018-11-02T03:12:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON\n> test parameter to only be a GIT_TEST_GETTEXT_POISON=\n> runtime parameter, to be consistent with other parameters documented\n> in \"Running tests with special setups\" in t/README.\n> ...\n> #ifndef NO_GETTEXT\n> extern void git_setup_gettext(void);\n> extern int gettext_width(const char *s);\n> #else\n> static inline void git_setup_gettext(void)\n> {\n> +\tuse_gettext_poison();; /* getenv() reentrancy paranoia */\n> }\n\nToo many \"case/esac\" statement in shell scripting?\n\n"},{"id":"362283","messageId":"20181102163725.GY30222@szeder.dev","threadId":"49637","inReplyTo":"20181101193115.32681-1-avarab@gmail.com","subject":"Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-11-02T16:37:25Z","receivedAt":"2018-11-02T16:37:33Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Nov 01, 2018 at 07:31:15PM +0000, Ævar Arnfjörð Bjarmason wrote:\n> Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON\n> test parameter to only be a GIT_TEST_GETTEXT_POISON=\n> runtime parameter, to be consistent with other parameters documented\n> in \"Running tests with special setups\" in t/README.\n> \n> When I added GETTEXT_POISON in bb946bba76 (\"i18n: add GETTEXT_POISON\n> to simulate unfriendly translator\", 2011-02-22) I was concerned with\n> ensuring that the _() function would get constant folded if NO_GETTEXT\n> was defined, and likewise that GETTEXT_POISON would be compiled out\n> unless it was defined.\n> \n> But as the benchmark in my [1] shows doing a one-off runtime\n> getenv(\"GIT_TEST_[...]\") is trivial, and since GETTEXT_POISON was\n> originally added the GIT_TEST_* env variables have become the common\n> idiom for turning on special test setups.\n> \n> So change GETTEXT_POISON to work the same way. Now the\n> GETTEXT_POISON=YesPlease compile-time option is gone, and running the\n> tests with GIT_TEST_GETTEXT_POISON=[YesPlease|] can be toggled on/off\n> without recompiling.\n> \n> This allows for conditionally amending tests to test with/without\n> poison, similar to what 859fdc0c3c (\"commit-graph: define\n> GIT_TEST_COMMIT_GRAPH\", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH. Do\n> some of that, now we e.g. always run the t0205-gettext-poison.sh test.\n> \n> I did enough there to remove the GETTEXT_POISON prerequisite, but its\n> inverse C_LOCALE_OUTPUT is still around, and surely some tests using\n> it can be converted to e.g. always set GIT_TEST_GETTEXT_POISON=.\n> \n> Notes on the implementation:\n> \n> * We still compile a dedicated GETTEXT_POISON build in Travis CI.\n> This is probably the wrong thing to do and should be followed-up\n> with something similar to ae59a4e44f (\"travis: run tests with\n> GIT_TEST_SPLIT_INDEX\", 2018-01-07) to re-use an existing test setup\n> for running in the GIT_TEST_GETTEXT_POISON mode.\n\nI'm of two minds about this. Sure, now it's not a compile time\noption, so we can spare a dedicated compilation, and sparing resources\nis good, of course.\n\nHowever, checking test failures in the 'linux-gcc' build job is always\na bit of a hassle, because it's not enough to look at the output of\nthe failed test, but I also have to check which one of the two test\nruns failed (i.e. the \"regular\" or one with all the GIT_TEST_* knobs\nturned on). Adding a second test run with GIT_TEST_GETTEXT_POISON\nenabled to an other build job (e.g. 'linux-clang') would then bring\nthis hassle to that build job as well.\n\nFurthermore, occasionally a build job is slower than usual (whatever\nthe reason might be), which can be an issue when running the test\nsuite twice, as it can exceed the time limit.\n\nAnyway, we can think about this later.\n\nIn any case, GIT_TEST_GETTEXT_POISON definitely should not be enabled\nin the same \"catch-all\" test run with all other GIT_TEST_* variables,\nbecause it would mess up any translated error messages that might pop\nup in a test failure, and then we wouldn't have any idea about what\nwent wrong.\n\n> * We now skip a test in t0000-basic.sh under\n> GIT_TEST_GETTEXT_POISON=YesPlease that wasn't skipped before. This\n> test relies on C locale output, but due to an edge case in how the\n> previous implementation of GETTEXT_POISON worked (reading it from\n> GIT-BUILD-OPTIONS) wasn't enabling poison correctly. Now it does,\n> and needs to be skipped.\n> \n> * The getenv() function is not reentrant, so out of paranoia about\n> code of the form:\n> \n> printf(_(\"%s\"), getenv(\"some-env\"));\n> \n> call use_gettext_poison() in our early setup in git_setup_gettext()\n> so we populate the \"poison_requested\" variable in a codepath that's\n> won't suffer from that race condition.\n> \n> See also [3] for more on the motivation behind this patch, and the\n> history of the GETTEXT_POISON facility.\n> \n> 1. https://public-inbox.org/git/871s8gd32p.fsf@evledraar.gmail.com/\n> 2. https://public-inbox.org/git/87woq7b58k.fsf@evledraar.gmail.com/\n> 3. https://public-inbox.org/git/878t2pd6yu.fsf@evledraar.gmail.com/\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> \n> Now:\n> \n> * The new i18n helper is gone. We just use \"test -n\" semantics for\n> $GIT_TEST_GETTEXT_POISON\n> \n> * We error out in the Makefile if you're still saying\n> GETTEXT_POISON=YesPlease.\n> \n> This makes more sense than just making it a synonym since now this\n> also needs to be defined at runtime.\n\nOK, I think erroring out is better than silently ignoring\nGETTEXT_POISON=YesPlease. However, the commit message only mentions\nthat GETTEXT_POISON is gone, but perhaps this should be mentioned\nthere as well.\n\n> * The caveat with avoiding test_lazy_prereq is gone (although there's\n> still some unrelated bug there worth looking into).\n> \n> * We call use_gettext_poison() really early to avoid any reentrancy\n> issue with getenv().\n\n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index 175f83d704..3c6b185b60 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -1697,7 +1697,8 @@ test_expect_success 'sourcing the completion script clears cached commands' '\n> \tverbose test -z \"$__git_all_commands\"\n> '\n> \n> -test_expect_success !GETTEXT_POISON 'sourcing the completion script clears cached merge strategies' '\n> +test_expect_success 'sourcing the completion script clears cached merge strategies' '\n> +\tGIT_TEST_GETTEXT_POISON= &&\n> \t__git_compute_merge_strategies &&\n\nOK, makes sense.\n\n> \tverbose test -n \"$__git_merge_strategies\" &&\n> \t. \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 78d8c3783b..2f42b3653c 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -755,16 +755,16 @@ test_cmp_bin() {\n> \n> # Use this instead of test_cmp to compare files that contain expected and\n> # actual output from git commands that can be translated. When running\n> -# under GETTEXT_POISON this pretends that the command produced expected\n> +# under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected\n> # results.\n> test_i18ncmp () {\n> -\ttest -n \"$GETTEXT_POISON\" || test_cmp \"$@\"\n> +\t! test_have_prereq C_LOCALE_OUTPUT || test_cmp \"$@\"\n> }\n\n> test_i18ngrep () {\n> \teval \"last_arg=\\${$#}\"\n> @@ -779,7 +779,7 @@ test_i18ngrep () {\n> \t\terror \"bug in the test script: too few parameters to test_i18ngrep\"\n> \tfi\n> \n> -\tif test -n \"$GETTEXT_POISON\"\n> +\tif test_have_prereq !C_LOCALE_OUTPUT\n\nWhy do these two helper functions call test_have_prereq (a function\nthat doesn't even fit on my screen) instead of a simple\n\n test -n \"$GIT_TEST_GETTEXT_POISON\"\n\n> \tthen\n> \t\t# pretend success\n> \t\treturn 0\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 897e6fcc94..370a4821e1 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1105,12 +1105,8 @@ test -n \"$USE_LIBPCRE2\" && test_set_prereq LIBPCRE2\n> test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n> \n> # Can we rely on git's output in the C locale?\n> -if test -n \"$GETTEXT_POISON\"\n> +if test -z \"$GIT_TEST_GETTEXT_POISON\"\n> then\n> -\tGIT_GETTEXT_POISON=YesPlease\n> -\texport GIT_GETTEXT_POISON\n> -\ttest_set_prereq GETTEXT_POISON\n> -else\n> \ttest_set_prereq C_LOCALE_OUTPUT\n> fi\n\nGIT_TEST_GETTEXT_POISON=1 will influence even those git commands that\nare executed during initialization of test-lib and the test repo:\n\n $ GIT_TEST_GETTEXT_POISON=1 ./t0000-basic.sh -v\n # GETTEXT POISON #expecting success: \n <....>\n\nIt should say:\n\n Initialized empty Git repository in <... path ...>\n expecting success:\n\nSee https://public-inbox.org/git/20181022202241.18629-2-szeder.dev@gmail.com/\nfor a bit of code that you might find worthy to steal.\n\n"},{"id":"362694","messageId":"xmqqpnvg8d5z.fsf@gitster-ct.c.googlers.com","threadId":"49637","inReplyTo":"20181101193115.32681-1-avarab@gmail.com","subject":"Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-08T03:24:08Z","receivedAt":"2018-11-08T03:24:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> * We error out in the Makefile if you're still saying\n> GETTEXT_POISON=YesPlease.\n\nI expected this would be irritating, but it turns out it is worse\nthan mere irritation but is a severe hinderance to affect my\nperformance, as I (and my bots) keep building and testing different\nversions of Git under different configurations.\n\nI know it was done to help those who only ever build a single track\nat a time and mostly moving forward, but I'm very much tempted to\nremove this part, perhaps demote it to a warning of some sort.\n\n\n> ifdef GETTEXT_POISON\n> -\tBASIC_CFLAGS += -DGETTEXT_POISON\n> +$(error The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!)\n> endif\n\n-- >8 --\nMakefile: ease dynamic-gettext-poison transition\n\nEarlier we made the entire build to fail when GETTEXT_POISON=Yes is\ngiven to make, to notify those who did not notice that text poisoning\nis now a runtime behaviour.\n\nIt turns out that this too irritating for those who need to build\nand test different versions of Git that cross the boundary between\nhistory with and without this topic to switch between two\nenvironment variables. Demote the error to a warning, so that you\ncan say something like\n\n\tmake GETTEXT_POISON=Yes GIT_TEST_GETTEXT_POISON test\n\nduring the transition period, without having to worry about whether\nexact version you are testing has or does not have this topic.\n\nSigned-off-by: Junio C Hamano \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex f3a9995e50..6b492f44a6 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1447,7 +1447,7 @@ ifdef NO_SYMLINK_HEAD\n \tBASIC_CFLAGS += -DNO_SYMLINK_HEAD\n endif\n ifdef GETTEXT_POISON\n-$(error The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!)\n+$(warning The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!)\n endif\n ifdef NO_GETTEXT\n \tBASIC_CFLAGS += -DNO_GETTEXT\n"},{"id":"362749","messageId":"CAPig+cQR++wfWLrODNOioTu=FuLBKWX=P74kaS8X92Nrp6h_pA@mail.gmail.com","threadId":"49637","inReplyTo":"xmqqpnvg8d5z.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-11-08T19:12:34Z","receivedAt":"2018-11-08T19:12:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 7, 2018 at 10:24 PM Junio C Hamano wrote:\n> Makefile: ease dynamic-gettext-poison transition\n>\n> Earlier we made the entire build to fail when GETTEXT_POISON=Yes is\n> given to make, to notify those who did not notice that text poisoning\n> is now a runtime behaviour.\n>\n> It turns out that this too irritating for those who need to build\n\ns/too/is &/\n\n> and test different versions of Git that cross the boundary between\n> history with and without this topic to switch between two\n> environment variables. Demote the error to a warning, so that you\n> can say something like\n>\n> make GETTEXT_POISON=Yes GIT_TEST_GETTEXT_POISON test\n>\n> during the transition period, without having to worry about whether\n> exact version you are testing has or does not have this topic.\n>\n> Signed-off-by: Junio C Hamano \n"},{"id":"362750","messageId":"87muqj5n9w.fsf@evledraar.gmail.com","threadId":"49637","inReplyTo":"20181102163725.GY30222@szeder.dev","subject":"Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-08T20:26:19Z","receivedAt":"2018-11-08T20:26:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 02 2018, SZEDER Gábor wrote:\n\n> On Thu, Nov 01, 2018 at 07:31:15PM +0000, Ævar Arnfjörð Bjarmason wrote:\n>> Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON\n>> test parameter to only be a GIT_TEST_GETTEXT_POISON=\n>> runtime parameter, to be consistent with other parameters documented\n>> in \"Running tests with special setups\" in t/README.\n>>\n>> When I added GETTEXT_POISON in bb946bba76 (\"i18n: add GETTEXT_POISON\n>> to simulate unfriendly translator\", 2011-02-22) I was concerned with\n>> ensuring that the _() function would get constant folded if NO_GETTEXT\n>> was defined, and likewise that GETTEXT_POISON would be compiled out\n>> unless it was defined.\n>>\n>> But as the benchmark in my [1] shows doing a one-off runtime\n>> getenv(\"GIT_TEST_[...]\") is trivial, and since GETTEXT_POISON was\n>> originally added the GIT_TEST_* env variables have become the common\n>> idiom for turning on special test setups.\n>>\n>> So change GETTEXT_POISON to work the same way. Now the\n>> GETTEXT_POISON=YesPlease compile-time option is gone, and running the\n>> tests with GIT_TEST_GETTEXT_POISON=[YesPlease|] can be toggled on/off\n>> without recompiling.\n>>\n>> This allows for conditionally amending tests to test with/without\n>> poison, similar to what 859fdc0c3c (\"commit-graph: define\n>> GIT_TEST_COMMIT_GRAPH\", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH. Do\n>> some of that, now we e.g. always run the t0205-gettext-poison.sh test.\n>>\n>> I did enough there to remove the GETTEXT_POISON prerequisite, but its\n>> inverse C_LOCALE_OUTPUT is still around, and surely some tests using\n>> it can be converted to e.g. always set GIT_TEST_GETTEXT_POISON=.\n>>\n>> Notes on the implementation:\n>>\n>> * We still compile a dedicated GETTEXT_POISON build in Travis CI.\n>> This is probably the wrong thing to do and should be followed-up\n>> with something similar to ae59a4e44f (\"travis: run tests with\n>> GIT_TEST_SPLIT_INDEX\", 2018-01-07) to re-use an existing test setup\n>> for running in the GIT_TEST_GETTEXT_POISON mode.\n>\n> I'm of two minds about this. Sure, now it's not a compile time\n> option, so we can spare a dedicated compilation, and sparing resources\n> is good, of course.\n>\n> However, checking test failures in the 'linux-gcc' build job is always\n> a bit of a hassle, because it's not enough to look at the output of\n> the failed test, but I also have to check which one of the two test\n> runs failed (i.e. the \"regular\" or one with all the GIT_TEST_* knobs\n> turned on). Adding a second test run with GIT_TEST_GETTEXT_POISON\n> enabled to an other build job (e.g. 'linux-clang') would then bring\n> this hassle to that build job as well.\n>\n> Furthermore, occasionally a build job is slower than usual (whatever\n> the reason might be), which can be an issue when running the test\n> suite twice, as it can exceed the time limit.\n>\n> Anyway, we can think about this later.\n>\n> In any case, GIT_TEST_GETTEXT_POISON definitely should not be enabled\n> in the same \"catch-all\" test run with all other GIT_TEST_* variables,\n> because it would mess up any translated error messages that might pop\n> up in a test failure, and then we wouldn't have any idea about what\n> went wrong.\n\nWill clarify this in v3. I.e. \"let's think about this...\".\n\n>> * We now skip a test in t0000-basic.sh under\n>> GIT_TEST_GETTEXT_POISON=YesPlease that wasn't skipped before. This\n>> test relies on C locale output, but due to an edge case in how the\n>> previous implementation of GETTEXT_POISON worked (reading it from\n>> GIT-BUILD-OPTIONS) wasn't enabling poison correctly. Now it does,\n>> and needs to be skipped.\n>>\n>> * The getenv() function is not reentrant, so out of paranoia about\n>> code of the form:\n>>\n>> printf(_(\"%s\"), getenv(\"some-env\"));\n>>\n>> call use_gettext_poison() in our early setup in git_setup_gettext()\n>> so we populate the \"poison_requested\" variable in a codepath that's\n>> won't suffer from that race condition.\n>>\n>> See also [3] for more on the motivation behind this patch, and the\n>> history of the GETTEXT_POISON facility.\n>>\n>> 1. https://public-inbox.org/git/871s8gd32p.fsf@evledraar.gmail.com/\n>> 2. https://public-inbox.org/git/87woq7b58k.fsf@evledraar.gmail.com/\n>> 3. https://public-inbox.org/git/878t2pd6yu.fsf@evledraar.gmail.com/\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>>\n>> Now:\n>>\n>> * The new i18n helper is gone. We just use \"test -n\" semantics for\n>> $GIT_TEST_GETTEXT_POISON\n>>\n>> * We error out in the Makefile if you're still saying\n>> GETTEXT_POISON=YesPlease.\n>>\n>> This makes more sense than just making it a synonym since now this\n>> also needs to be defined at runtime.\n>\n> OK, I think erroring out is better than silently ignoring\n> GETTEXT_POISON=YesPlease. However, the commit message only mentions\n> that GETTEXT_POISON is gone, but perhaps this should be mentioned\n> there as well.\n\nWill mention.\n\n>> * The caveat with avoiding test_lazy_prereq is gone (although there's\n>> still some unrelated bug there worth looking into).\n>>\n>> * We call use_gettext_poison() really early to avoid any reentrancy\n>> issue with getenv().\n>\n>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n>> index 175f83d704..3c6b185b60 100755\n>> --- a/t/t9902-completion.sh\n>> +++ b/t/t9902-completion.sh\n>> @@ -1697,7 +1697,8 @@ test_expect_success 'sourcing the completion script clears cached commands' '\n>> \tverbose test -z \"$__git_all_commands\"\n>> '\n>>\n>> -test_expect_success !GETTEXT_POISON 'sourcing the completion script clears cached merge strategies' '\n>> +test_expect_success 'sourcing the completion script clears cached merge strategies' '\n>> +\tGIT_TEST_GETTEXT_POISON= &&\n>> \t__git_compute_merge_strategies &&\n>\n> OK, makes sense.\n>\n>> \tverbose test -n \"$__git_merge_strategies\" &&\n>> \t. \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n>\n>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n>> index 78d8c3783b..2f42b3653c 100644\n>> --- a/t/test-lib-functions.sh\n>> +++ b/t/test-lib-functions.sh\n>> @@ -755,16 +755,16 @@ test_cmp_bin() {\n>>\n>> # Use this instead of test_cmp to compare files that contain expected and\n>> # actual output from git commands that can be translated. When running\n>> -# under GETTEXT_POISON this pretends that the command produced expected\n>> +# under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected\n>> # results.\n>> test_i18ncmp () {\n>> -\ttest -n \"$GETTEXT_POISON\" || test_cmp \"$@\"\n>> +\t! test_have_prereq C_LOCALE_OUTPUT || test_cmp \"$@\"\n>> }\n>\n>> test_i18ngrep () {\n>> \teval \"last_arg=\\${$#}\"\n>> @@ -779,7 +779,7 @@ test_i18ngrep () {\n>> \t\terror \"bug in the test script: too few parameters to test_i18ngrep\"\n>> \tfi\n>>\n>> -\tif test -n \"$GETTEXT_POISON\"\n>> +\tif test_have_prereq !C_LOCALE_OUTPUT\n>\n> Why do these two helper functions call test_have_prereq (a function\n> that doesn't even fit on my screen) instead of a simple\n>\n> test -n \"$GIT_TEST_GETTEXT_POISON\"\n\nI'm going to keep the call to test_have_prereq, it's consistent with\nother use of the helper in the test lib, and doesn't confuse the reader\nby giving the impression that we're in some really early setup where we\nhaven't set the prereq at this point (we have).\n\n>> \tthen\n>> \t\t# pretend success\n>> \t\treturn 0\n>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>> index 897e6fcc94..370a4821e1 100644\n>> --- a/t/test-lib.sh\n>> +++ b/t/test-lib.sh\n>> @@ -1105,12 +1105,8 @@ test -n \"$USE_LIBPCRE2\" && test_set_prereq LIBPCRE2\n>> test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n>>\n>> # Can we rely on git's output in the C locale?\n>> -if test -n \"$GETTEXT_POISON\"\n>> +if test -z \"$GIT_TEST_GETTEXT_POISON\"\n>> then\n>> -\tGIT_GETTEXT_POISON=YesPlease\n>> -\texport GIT_GETTEXT_POISON\n>> -\ttest_set_prereq GETTEXT_POISON\n>> -else\n>> \ttest_set_prereq C_LOCALE_OUTPUT\n>> fi\n>\n> GIT_TEST_GETTEXT_POISON=1 will influence even those git commands that\n> are executed during initialization of test-lib and the test repo:\n>\n> $ GIT_TEST_GETTEXT_POISON=1 ./t0000-basic.sh -v\n> # GETTEXT POISON #expecting success:\n> <....>\n>\n> It should say:\n>\n> Initialized empty Git repository in <... path ...>\n> expecting success:\n>\n> See https://public-inbox.org/git/20181022202241.18629-2-szeder.dev@gmail.com/\n> for a bit of code that you might find worthy to steal.\n\nThanks. Fixing.\n"},{"id":"362751","messageId":"20181108205110.GB30222@szeder.dev","threadId":"49637","inReplyTo":"87muqj5n9w.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3] i18n: make GETTEXT_POISON a runtime option","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-11-08T20:51:10Z","receivedAt":"2018-11-08T20:51:17Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Nov 08, 2018 at 09:26:19PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> On Fri, Nov 02 2018, SZEDER Gábor wrote:\n\n> >> * We error out in the Makefile if you're still saying\n> >> GETTEXT_POISON=YesPlease.\n> >>\n> >> This makes more sense than just making it a synonym since now this\n> >> also needs to be defined at runtime.\n> >\n> > OK, I think erroring out is better than silently ignoring\n> > GETTEXT_POISON=YesPlease. However, the commit message only mentions\n> > that GETTEXT_POISON is gone, but perhaps this should be mentioned\n> > there as well.\n> \n> Will mention.\n\nWith the benefit of hindsight gained from looking into a failing\nGETTEXT_POISON build [1], I have to agree with Junio that flat-out\nerroring out when GETTEXT_POISON=YesPlease is set is really a hassle\n[2].\n\n[1] https://public-inbox.org/git/20181107130950.GA30222@szeder.dev/\n (the failure is not related to this patch)\n[2] https://public-inbox.org/git/xmqqpnvg8d5z.fsf@gitster-ct.c.googlers.com/\n\n> >> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> >> index 78d8c3783b..2f42b3653c 100644\n> >> --- a/t/test-lib-functions.sh\n> >> +++ b/t/test-lib-functions.sh\n> >> @@ -755,16 +755,16 @@ test_cmp_bin() {\n> >>\n> >> # Use this instead of test_cmp to compare files that contain expected and\n> >> # actual output from git commands that can be translated. When running\n> >> -# under GETTEXT_POISON this pretends that the command produced expected\n> >> +# under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected\n> >> # results.\n> >> test_i18ncmp () {\n> >> -\ttest -n \"$GETTEXT_POISON\" || test_cmp \"$@\"\n> >> +\t! test_have_prereq C_LOCALE_OUTPUT || test_cmp \"$@\"\n> >> }\n> >\n> >> test_i18ngrep () {\n> >> \teval \"last_arg=\\${$#}\"\n> >> @@ -779,7 +779,7 @@ test_i18ngrep () {\n> >> \t\terror \"bug in the test script: too few parameters to test_i18ngrep\"\n> >> \tfi\n> >>\n> >> -\tif test -n \"$GETTEXT_POISON\"\n> >> +\tif test_have_prereq !C_LOCALE_OUTPUT\n> >\n> > Why do these two helper functions call test_have_prereq (a function\n> > that doesn't even fit on my screen) instead of a simple\n> >\n> > test -n \"$GIT_TEST_GETTEXT_POISON\"\n> \n> I'm going to keep the call to test_have_prereq, it's consistent with\n> other use of the helper in the test lib, and doesn't confuse the reader\n> by giving the impression that we're in some really early setup where we\n> haven't set the prereq at this point (we have).\n\nUsing the prereq in the first place is what confused (and is still\nconfusing...) the reader.\n\n"},{"id":"362755","messageId":"20181108211530.29017-1-avarab@gmail.com","threadId":"49637","inReplyTo":"20181101193115.32681-1-avarab@gmail.com","subject":"[PATCH v4 0/2] i18n: make GETTEXT_POISON a runtime option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-08T21:15:28Z","receivedAt":"2018-11-08T21:15:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Addresses all the feedback against v3. Includes a patch by Junio\nsitting in \"pu\" (and I fixed the grammar error Eric pointed out in its\ncommit message).\n\nRange-diff:\n\n1: cc24ba8de8 ! 1: 2210ee8bd9 i18n: make GETTEXT_POISON a runtime option\n @@ -34,11 +34,11 @@\n \n Notes on the implementation:\n \n - * We still compile a dedicated GETTEXT_POISON build in Travis CI.\n - This is probably the wrong thing to do and should be followed-up\n - with something similar to ae59a4e44f (\"travis: run tests with\n - GIT_TEST_SPLIT_INDEX\", 2018-01-07) to re-use an existing test setup\n - for running in the GIT_TEST_GETTEXT_POISON mode.\n + * We still compile a dedicated GETTEXT_POISON build in Travis\n + CI. Perhaps this should be revisited and integrated into the\n + \"linux-gcc\" build, see ae59a4e44f (\"travis: run tests with\n + GIT_TEST_SPLIT_INDEX\", 2018-01-07) for prior art in that area. Then\n + again maybe not, see [2].\n \n * We now skip a test in t0000-basic.sh under\n GIT_TEST_GETTEXT_POISON=YesPlease that wasn't skipped before. This\n @@ -56,12 +56,22 @@\n so we populate the \"poison_requested\" variable in a codepath that's\n won't suffer from that race condition.\n \n - See also [3] for more on the motivation behind this patch, and the\n + * We error out in the Makefile if you're still saying\n + GETTEXT_POISON=YesPlease to prompt users to change their\n + invocation.\n +\n + * We should not print out poisoned messages during the test\n + initialization itself to keep it more readable, so the test library\n + hides the variable if set in $GIT_TEST_GETTEXT_POISON_ORIG during\n + setup. See [3].\n +\n + See also [4] for more on the motivation behind this patch, and the\n history of the GETTEXT_POISON facility.\n \n 1. https://public-inbox.org/git/871s8gd32p.fsf@evledraar.gmail.com/\n - 2. https://public-inbox.org/git/87woq7b58k.fsf@evledraar.gmail.com/\n - 3. https://public-inbox.org/git/878t2pd6yu.fsf@evledraar.gmail.com/\n + 2. https://public-inbox.org/git/20181102163725.GY30222@szeder.dev/\n + 3. https://public-inbox.org/git/20181022202241.18629-2-szeder.dev@gmail.com/\n + 4. https://public-inbox.org/git/878t2pd6yu.fsf@evledraar.gmail.com/\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ -181,7 +191,7 @@\n #else\n static inline void git_setup_gettext(void)\n {\n -+\tuse_gettext_poison();; /* getenv() reentrancy paranoia */\n ++\tuse_gettext_poison(); /* getenv() reentrancy paranoia */\n }\n static inline int gettext_width(const char *s)\n {\n @@ -392,8 +402,32 @@\n --- a/t/test-lib.sh\n +++ b/t/test-lib.sh\n @@\n + TZ=UTC\n + export LANG LC_ALL PAGER TZ\n + EDITOR=:\n ++\n ++# GIT_TEST_GETTEXT_POISON should not influence git commands executed\n ++# during initialization of test-lib and the test repo. Back it up,\n ++# unset and then restore after initialization is finished.\n ++if test -n \"$GIT_TEST_GETTEXT_POISON\"\n ++then\n ++\tGIT_TEST_GETTEXT_POISON_ORIG=$GIT_TEST_GETTEXT_POISON\n ++\tunset GIT_TEST_GETTEXT_POISON\n ++fi\n ++\n + # A call to \"unset\" with no arguments causes at least Solaris 10\n + # /usr/xpg4/bin/sh and /bin/ksh to bail out. So keep the unsets\n + # deriving from the command substitution clustered with the other\n +@@\n + test -n \"$USE_LIBPCRE2\" && test_set_prereq LIBPCRE2\n test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n \n ++if test -n \"$GIT_TEST_GETTEXT_POISON_ORIG\"\n ++then\n ++\tGIT_TEST_GETTEXT_POISON=$GIT_TEST_GETTEXT_POISON_ORIG\n ++\tunset GIT_TEST_GETTEXT_POISON_ORIG\n ++fi\n ++\n # Can we rely on git's output in the C locale?\n -if test -n \"$GETTEXT_POISON\"\n +if test -z \"$GIT_TEST_GETTEXT_POISON\"\n-: ---------- > 2: a6948d936a Makefile: ease dynamic-gettext-poison transition\n\n\nJunio C Hamano (1):\n Makefile: ease dynamic-gettext-poison transition\n\nÆvar Arnfjörð Bjarmason (1):\n i18n: make GETTEXT_POISON a runtime option\n\n .travis.yml | 2 +-\n Makefile | 8 +-------\n ci/lib-travisci.sh | 4 ++--\n gettext.c | 11 +++++++----\n gettext.h | 9 +++------\n git-sh-i18n.sh | 2 +-\n po/README | 13 ++++---------\n t/README | 6 ++++++\n t/lib-gettext.sh | 2 +-\n t/t0000-basic.sh | 2 +-\n t/t0205-gettext-poison.sh | 8 +++++---\n t/t3406-rebase-message.sh | 2 +-\n t/t7201-co.sh | 6 +++---\n t/t9902-completion.sh | 3 ++-\n t/test-lib-functions.sh | 8 ++++----\n t/test-lib.sh | 22 +++++++++++++++++-----\n 16 files changed, 59 insertions(+), 49 deletions(-)\n\n-- \n2.19.1.930.g4563a0d9d0\n\n"},{"id":"362756","messageId":"20181108211530.29017-2-avarab@gmail.com","threadId":"49637","inReplyTo":"20181101193115.32681-1-avarab@gmail.com","subject":"[PATCH v4 1/2] i18n: make GETTEXT_POISON a runtime option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-08T21:15:29Z","receivedAt":"2018-11-08T21:15:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON\ntest parameter to only be a GIT_TEST_GETTEXT_POISON=\nruntime parameter, to be consistent with other parameters documented\nin \"Running tests with special setups\" in t/README.\n\nWhen I added GETTEXT_POISON in bb946bba76 (\"i18n: add GETTEXT_POISON\nto simulate unfriendly translator\", 2011-02-22) I was concerned with\nensuring that the _() function would get constant folded if NO_GETTEXT\nwas defined, and likewise that GETTEXT_POISON would be compiled out\nunless it was defined.\n\nBut as the benchmark in my [1] shows doing a one-off runtime\ngetenv(\"GIT_TEST_[...]\") is trivial, and since GETTEXT_POISON was\noriginally added the GIT_TEST_* env variables have become the common\nidiom for turning on special test setups.\n\nSo change GETTEXT_POISON to work the same way. Now the\nGETTEXT_POISON=YesPlease compile-time option is gone, and running the\ntests with GIT_TEST_GETTEXT_POISON=[YesPlease|] can be toggled on/off\nwithout recompiling.\n\nThis allows for conditionally amending tests to test with/without\npoison, similar to what 859fdc0c3c (\"commit-graph: define\nGIT_TEST_COMMIT_GRAPH\", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH. Do\nsome of that, now we e.g. always run the t0205-gettext-poison.sh test.\n\nI did enough there to remove the GETTEXT_POISON prerequisite, but its\ninverse C_LOCALE_OUTPUT is still around, and surely some tests using\nit can be converted to e.g. always set GIT_TEST_GETTEXT_POISON=.\n\nNotes on the implementation:\n\n * We still compile a dedicated GETTEXT_POISON build in Travis\n CI. Perhaps this should be revisited and integrated into the\n \"linux-gcc\" build, see ae59a4e44f (\"travis: run tests with\n GIT_TEST_SPLIT_INDEX\", 2018-01-07) for prior art in that area. Then\n again maybe not, see [2].\n\n * We now skip a test in t0000-basic.sh under\n GIT_TEST_GETTEXT_POISON=YesPlease that wasn't skipped before. This\n test relies on C locale output, but due to an edge case in how the\n previous implementation of GETTEXT_POISON worked (reading it from\n GIT-BUILD-OPTIONS) wasn't enabling poison correctly. Now it does,\n and needs to be skipped.\n\n * The getenv() function is not reentrant, so out of paranoia about\n code of the form:\n\n printf(_(\"%s\"), getenv(\"some-env\"));\n\n call use_gettext_poison() in our early setup in git_setup_gettext()\n so we populate the \"poison_requested\" variable in a codepath that's\n won't suffer from that race condition.\n\n * We error out in the Makefile if you're still saying\n GETTEXT_POISON=YesPlease to prompt users to change their\n invocation.\n\n * We should not print out poisoned messages during the test\n initialization itself to keep it more readable, so the test library\n hides the variable if set in $GIT_TEST_GETTEXT_POISON_ORIG during\n setup. See [3].\n\nSee also [4] for more on the motivation behind this patch, and the\nhistory of the GETTEXT_POISON facility.\n\n1. https://public-inbox.org/git/871s8gd32p.fsf@evledraar.gmail.com/\n2. https://public-inbox.org/git/20181102163725.GY30222@szeder.dev/\n3. https://public-inbox.org/git/20181022202241.18629-2-szeder.dev@gmail.com/\n4. https://public-inbox.org/git/878t2pd6yu.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n .travis.yml | 2 +-\n Makefile | 8 +-------\n ci/lib-travisci.sh | 4 ++--\n gettext.c | 11 +++++++----\n gettext.h | 9 +++------\n git-sh-i18n.sh | 2 +-\n po/README | 13 ++++---------\n t/README | 6 ++++++\n t/lib-gettext.sh | 2 +-\n t/t0000-basic.sh | 2 +-\n t/t0205-gettext-poison.sh | 8 +++++---\n t/t3406-rebase-message.sh | 2 +-\n t/t7201-co.sh | 6 +++---\n t/t9902-completion.sh | 3 ++-\n t/test-lib-functions.sh | 8 ++++----\n t/test-lib.sh | 22 +++++++++++++++++-----\n 16 files changed, 59 insertions(+), 49 deletions(-)\n\ndiff --git a/.travis.yml b/.travis.yml\nindex 8d2499739e..a329a0add6 100644\n--- a/.travis.yml\n+++ b/.travis.yml\n@@ -24,7 +24,7 @@ addons:\n \n matrix:\n include:\n- - env: jobname=GETTEXT_POISON\n+ - env: jobname=GIT_TEST_GETTEXT_POISON\n os: linux\n compiler:\n addons:\ndiff --git a/Makefile b/Makefile\nindex bbfbb4292d..f3a9995e50 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -362,11 +362,6 @@ all::\n # Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the\n # user.\n #\n-# Define GETTEXT_POISON if you are debugging the choice of strings marked\n-# for translation. In a GETTEXT_POISON build, you can turn all strings marked\n-# for translation into gibberish by setting the GIT_GETTEXT_POISON variable\n-# (to any value) in your environment.\n-#\n # Define JSMIN to point to JavaScript minifier that functions as\n # a filter to have gitweb.js minified.\n #\n@@ -1452,7 +1447,7 @@ ifdef NO_SYMLINK_HEAD\n \tBASIC_CFLAGS += -DNO_SYMLINK_HEAD\n endif\n ifdef GETTEXT_POISON\n-\tBASIC_CFLAGS += -DGETTEXT_POISON\n+$(error The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!)\n endif\n ifdef NO_GETTEXT\n \tBASIC_CFLAGS += -DNO_GETTEXT\n@@ -2603,7 +2598,6 @@ ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT\n \t@echo GIT_TEST_CMP_USE_COPIED_CONTEXT=YesPlease >>$@+\n endif\n \t@echo NO_GETTEXT=\\''$(subst ','\\'',$(subst ','\\'',$(NO_GETTEXT)))'\\' >>$@+\n-\t@echo GETTEXT_POISON=\\''$(subst ','\\'',$(subst ','\\'',$(GETTEXT_POISON)))'\\' >>$@+\n ifdef GIT_PERF_REPEAT_COUNT\n \t@echo GIT_PERF_REPEAT_COUNT=\\''$(subst ','\\'',$(subst ','\\'',$(GIT_PERF_REPEAT_COUNT)))'\\' >>$@+\n endif\ndiff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh\nindex 06970f7213..69dff4d1ec 100755\n--- a/ci/lib-travisci.sh\n+++ b/ci/lib-travisci.sh\n@@ -123,7 +123,7 @@ osx-clang|osx-gcc)\n \t# Travis CI OS X\n \texport GIT_SKIP_TESTS=\"t9810 t9816\"\n \t;;\n-GETTEXT_POISON)\n-\texport GETTEXT_POISON=YesPlease\n+GIT_TEST_GETTEXT_POISON)\n+\texport GIT_TEST_GETTEXT_POISON=YesPlease\n \t;;\n esac\ndiff --git a/gettext.c b/gettext.c\nindex 7272771c8e..d4021d690c 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -7,6 +7,7 @@\n #include \"gettext.h\"\n #include \"strbuf.h\"\n #include \"utf8.h\"\n+#include \"config.h\"\n \n #ifndef NO_GETTEXT\n #\tinclude \n@@ -46,15 +47,15 @@ const char *get_preferred_languages(void)\n \treturn NULL;\n }\n \n-#ifdef GETTEXT_POISON\n int use_gettext_poison(void)\n {\n \tstatic int poison_requested = -1;\n-\tif (poison_requested == -1)\n-\t\tpoison_requested = getenv(\"GIT_GETTEXT_POISON\") ? 1 : 0;\n+\tif (poison_requested == -1) {\n+\t\tconst char *v = getenv(\"GIT_TEST_GETTEXT_POISON\");\n+\t\tpoison_requested = v && strlen(v) ? 1 : 0;\n+\t}\n \treturn poison_requested;\n }\n-#endif\n \n #ifndef NO_GETTEXT\n static int test_vsnprintf(const char *fmt, ...)\n@@ -164,6 +165,8 @@ void git_setup_gettext(void)\n \tif (!podir)\n \t\tpodir = p = system_path(GIT_LOCALE_PATH);\n \n+\tuse_gettext_poison(); /* getenv() reentrancy paranoia */\n+\n \tif (!is_directory(podir)) {\n \t\tfree(p);\n \t\treturn;\ndiff --git a/gettext.h b/gettext.h\nindex 7eee64a34f..71255e503e 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -28,12 +28,15 @@\n \n #define FORMAT_PRESERVING(n) __attribute__((format_arg(n)))\n \n+extern int use_gettext_poison(void);\n+\n #ifndef NO_GETTEXT\n extern void git_setup_gettext(void);\n extern int gettext_width(const char *s);\n #else\n static inline void git_setup_gettext(void)\n {\n+\tuse_gettext_poison(); /* getenv() reentrancy paranoia */\n }\n static inline int gettext_width(const char *s)\n {\n@@ -41,12 +44,6 @@ static inline int gettext_width(const char *s)\n }\n #endif\n \n-#ifdef GETTEXT_POISON\n-extern int use_gettext_poison(void);\n-#else\n-#define use_gettext_poison() 0\n-#endif\n-\n static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n {\n \tif (!*msgid)\ndiff --git a/git-sh-i18n.sh b/git-sh-i18n.sh\nindex 9d065fb4bf..e1d917fd27 100644\n--- a/git-sh-i18n.sh\n+++ b/git-sh-i18n.sh\n@@ -17,7 +17,7 @@ export TEXTDOMAINDIR\n \n # First decide what scheme to use...\n GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough\n-if test -n \"$GIT_GETTEXT_POISON\"\n+if test -n \"$GIT_TEST_GETTEXT_POISON\"\n then\n \tGIT_INTERNAL_GETTEXT_SH_SCHEME=poison\n elif test -n \"@@USE_GETTEXT_SCHEME@@\"\ndiff --git a/po/README b/po/README\nindex fef4c0f0b5..aa704ffcb7 100644\n--- a/po/README\n+++ b/po/README\n@@ -289,16 +289,11 @@ something in the test suite might still depend on the US English\n version of the strings, e.g. to grep some error message or other\n output.\n \n-To smoke out issues like these Git can be compiled with gettext poison\n-support, at the top-level:\n+To smoke out issues like these, Git tested with a translation mode that\n+emits gibberish on every call to gettext. To use it run the test suite\n+with it, e.g.:\n \n- make GETTEXT_POISON=YesPlease\n-\n-That'll give you a git which emits gibberish on every call to\n-gettext. It's obviously not meant to be installed, but you should run\n-the test suite with it:\n-\n- cd t && prove -j 9 ./t[0-9]*.sh\n+ cd t && GIT_TEST_GETTEXT_POISON=YesPlease prove -j 9 ./t[0-9]*.sh\n \n If tests break with it you should inspect them manually and see if\n what you're translating is sane, i.e. that you're not translating\ndiff --git a/t/README b/t/README\nindex 2e9bef2852..bb2db49615 100644\n--- a/t/README\n+++ b/t/README\n@@ -302,6 +302,12 @@ that cannot be easily covered by a few specific test cases. These\n could be enabled by running the test suite with correct GIT_TEST_\n environment set.\n \n+GIT_TEST_GETTEXT_POISON= turns all strings marked for\n+translation into gibberish if non-empty (think \"test -n\"). Used for\n+spotting those tests that need to be marked with a C_LOCALE_OUTPUT\n+prerequisite when adding more strings for translation. See \"Testing\n+marked strings\" in po/README for details.\n+\n GIT_TEST_SPLIT_INDEX= forces split-index mode on the whole\n test suite. Accept any boolean values that are accepted by git-config.\n \ndiff --git a/t/lib-gettext.sh b/t/lib-gettext.sh\nindex eec757f104..755f421431 100644\n--- a/t/lib-gettext.sh\n+++ b/t/lib-gettext.sh\n@@ -12,7 +12,7 @@ export GIT_TEXTDOMAINDIR GIT_PO_PATH\n \n . \"$GIT_BUILD_DIR\"/git-sh-i18n\n \n-if test_have_prereq GETTEXT && ! test_have_prereq GETTEXT_POISON\n+if test_have_prereq GETTEXT && test_have_prereq C_LOCALE_OUTPUT\n then\n \t# is_IS.UTF-8 on Solaris and FreeBSD, is_IS.utf8 on Debian\n \tis_IS_locale=$(locale -a 2>/dev/null |\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 4d23373526..b6566003dd 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -274,7 +274,7 @@ test_expect_success 'pretend we have a mix of all possible results' \"\n \tEOF\n \"\n \n-test_expect_success 'test --verbose' '\n+test_expect_success C_LOCALE_OUTPUT 'test --verbose' '\n \ttest_must_fail run_sub_test_lib_test \\\n \t\ttest-verbose \"test verbose\" --verbose <<-\\EOF &&\n \ttest_expect_success \"passing test\" true\ndiff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh\nindex 438e778d6a..a06269f38a 100755\n--- a/t/t0205-gettext-poison.sh\n+++ b/t/t0205-gettext-poison.sh\n@@ -5,13 +5,15 @@\n \n test_description='Gettext Shell poison'\n \n+GIT_TEST_GETTEXT_POISON=YesPlease\n+export GIT_TEST_GETTEXT_POISON\n . ./lib-gettext.sh\n \n-test_expect_success GETTEXT_POISON 'sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME\" is poison' '\n+test_expect_success 'sanity: $GIT_INTERNAL_GETTEXT_SH_SCHEME\" is poison' '\n test \"$GIT_INTERNAL_GETTEXT_SH_SCHEME\" = \"poison\"\n '\n \n-test_expect_success GETTEXT_POISON 'gettext: our gettext() fallback has poison semantics' '\n+test_expect_success 'gettext: our gettext() fallback has poison semantics' '\n printf \"# GETTEXT POISON #\" >expect &&\n gettext \"test\" >actual &&\n test_cmp expect actual &&\n@@ -20,7 +22,7 @@ test_expect_success GETTEXT_POISON 'gettext: our gettext() fallback has poison s\n test_cmp expect actual\n '\n \n-test_expect_success GETTEXT_POISON 'eval_gettext: our eval_gettext() fallback has poison semantics' '\n+test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semantics' '\n printf \"# GETTEXT POISON #\" >expect &&\n eval_gettext \"test\" >actual &&\n test_cmp expect actual &&\ndiff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\nindex 0392e36d23..2bdcf83808 100755\n--- a/t/t3406-rebase-message.sh\n+++ b/t/t3406-rebase-message.sh\n@@ -77,7 +77,7 @@ test_expect_success 'rebase -n overrides config rebase.stat config' '\n # \"Does not point to a valid commit: invalid-ref\"\n #\n # NEEDSWORK: This \"grep\" is fine in real non-C locales, but\n-# GETTEXT_POISON poisons the refname along with the enclosing\n+# GIT_TEST_GETTEXT_POISON poisons the refname along with the enclosing\n # error message.\n test_expect_success 'rebase --onto outputs the invalid ref' '\n \ttest_must_fail git rebase --onto invalid-ref HEAD HEAD 2>err &&\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 826987ca80..72b9b375ba 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -254,9 +254,9 @@ test_expect_success 'checkout to detach HEAD (with advice declined)' '\n test_expect_success 'checkout to detach HEAD' '\n \tgit config advice.detachedHead true &&\n \tgit checkout -f renamer && git clean -f &&\n-\tgit checkout renamer^ 2>messages &&\n-\ttest_i18ngrep \"HEAD is now at 7329388\" messages &&\n-\t(test_line_count -gt 1 messages || test -n \"$GETTEXT_POISON\") &&\n+\tGIT_TEST_GETTEXT_POISON= git checkout renamer^ 2>messages &&\n+\tgrep \"HEAD is now at 7329388\" messages &&\n+\ttest_line_count -gt 1 messages &&\n \tH=$(git rev-parse --verify HEAD) &&\n \tM=$(git show-ref -s --verify refs/heads/master) &&\n \ttest \"z$H\" = \"z$M\" &&\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 175f83d704..3c6b185b60 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1697,7 +1697,8 @@ test_expect_success 'sourcing the completion script clears cached commands' '\n \tverbose test -z \"$__git_all_commands\"\n '\n \n-test_expect_success !GETTEXT_POISON 'sourcing the completion script clears cached merge strategies' '\n+test_expect_success 'sourcing the completion script clears cached merge strategies' '\n+\tGIT_TEST_GETTEXT_POISON= &&\n \t__git_compute_merge_strategies &&\n \tverbose test -n \"$__git_merge_strategies\" &&\n \t. \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 78d8c3783b..2f42b3653c 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -755,16 +755,16 @@ test_cmp_bin() {\n \n # Use this instead of test_cmp to compare files that contain expected and\n # actual output from git commands that can be translated. When running\n-# under GETTEXT_POISON this pretends that the command produced expected\n+# under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected\n # results.\n test_i18ncmp () {\n-\ttest -n \"$GETTEXT_POISON\" || test_cmp \"$@\"\n+\t! test_have_prereq C_LOCALE_OUTPUT || test_cmp \"$@\"\n }\n \n # Use this instead of \"grep expected-string actual\" to see if the\n # output from a git command that can be translated either contains an\n # expected string, or does not contain an unwanted one. When running\n-# under GETTEXT_POISON this pretends that the command produced expected\n+# under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected\n # results.\n test_i18ngrep () {\n \teval \"last_arg=\\${$#}\"\n@@ -779,7 +779,7 @@ test_i18ngrep () {\n \t\terror \"bug in the test script: too few parameters to test_i18ngrep\"\n \tfi\n \n-\tif test -n \"$GETTEXT_POISON\"\n+\tif test_have_prereq !C_LOCALE_OUTPUT\n \tthen\n \t\t# pretend success\n \t\treturn 0\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 47a99aa0ed..b50f934dcd 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -95,6 +95,16 @@ PAGER=cat\n TZ=UTC\n export LANG LC_ALL PAGER TZ\n EDITOR=:\n+\n+# GIT_TEST_GETTEXT_POISON should not influence git commands executed\n+# during initialization of test-lib and the test repo. Back it up,\n+# unset and then restore after initialization is finished.\n+if test -n \"$GIT_TEST_GETTEXT_POISON\"\n+then\n+\tGIT_TEST_GETTEXT_POISON_ORIG=$GIT_TEST_GETTEXT_POISON\n+\tunset GIT_TEST_GETTEXT_POISON\n+fi\n+\n # A call to \"unset\" with no arguments causes at least Solaris 10\n # /usr/xpg4/bin/sh and /bin/ksh to bail out. So keep the unsets\n # deriving from the command substitution clustered with the other\n@@ -1104,13 +1114,15 @@ test -n \"$USE_LIBPCRE1\" && test_set_prereq LIBPCRE1\n test -n \"$USE_LIBPCRE2\" && test_set_prereq LIBPCRE2\n test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n \n+if test -n \"$GIT_TEST_GETTEXT_POISON_ORIG\"\n+then\n+\tGIT_TEST_GETTEXT_POISON=$GIT_TEST_GETTEXT_POISON_ORIG\n+\tunset GIT_TEST_GETTEXT_POISON_ORIG\n+fi\n+\n # Can we rely on git's output in the C locale?\n-if test -n \"$GETTEXT_POISON\"\n+if test -z \"$GIT_TEST_GETTEXT_POISON\"\n then\n-\tGIT_GETTEXT_POISON=YesPlease\n-\texport GIT_GETTEXT_POISON\n-\ttest_set_prereq GETTEXT_POISON\n-else\n \ttest_set_prereq C_LOCALE_OUTPUT\n fi\n \n-- \n2.19.1.930.g4563a0d9d0\n\n"},{"id":"362757","messageId":"20181108211530.29017-3-avarab@gmail.com","threadId":"49637","inReplyTo":"20181101193115.32681-1-avarab@gmail.com","subject":"[PATCH v4 2/2] Makefile: ease dynamic-gettext-poison transition","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-08T21:15:30Z","receivedAt":"2018-11-08T21:15:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Junio C Hamano \n\nEarlier we made the entire build to fail when GETTEXT_POISON=Yes is\ngiven to make, to notify those who did not notice that text poisoning\nis now a runtime behaviour.\n\nIt turns out that this is too irritating for those who need to build\nand test different versions of Git that cross the boundary between\nhistory with and without this topic to switch between two\nenvironment variables. Demote the error to a warning, so that you\ncan say something like\n\n make GETTEXT_POISON=Yes GIT_TEST_GETTEXT_POISON=Yes test\n\nduring the transition period, without having to worry about whether\nexact version you are testing has or does not have this topic.\n\nSigned-off-by: Junio C Hamano \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex f3a9995e50..6b492f44a6 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1447,7 +1447,7 @@ ifdef NO_SYMLINK_HEAD\n \tBASIC_CFLAGS += -DNO_SYMLINK_HEAD\n endif\n ifdef GETTEXT_POISON\n-$(error The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!)\n+$(warning The GETTEXT_POISON option has been removed in favor of runtime GIT_TEST_GETTEXT_POISON. See t/README!)\n endif\n ifdef NO_GETTEXT\n \tBASIC_CFLAGS += -DNO_GETTEXT\n-- \n2.19.1.930.g4563a0d9d0\n\n"}]}