{"thread":{"id":"31304","subject":"[PATCH] Build in gettext poison feature unconditionally","startedAt":"2012-08-21T04:39:30Z","lastAt":"2012-08-23T11:00:57Z","messageCount":10,"participants":["Nguyễn Thái Ngọc Duy","Jonathan Nieder","Junio C Hamano","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"197499","messageId":"1345523970-14914-1-git-send-email-pclouds@gmail.com","threadId":"31304","inReplyTo":null,"subject":"[PATCH] Build in gettext poison feature unconditionally","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-21T04:39:30Z","receivedAt":"2012-08-21T04:39:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Making gettext poison second citizen makes it easy to forget to run\nthe test suite with it. This is step one to running test suite with\ngettext poison by default.\n\nThe runtime cost should be small. \"gcc -O2\" inlines _() and\nuse_gettext_poison(). But even if it does not, performance should not\nbe impacted as _() calls are usually not on critical path. If some of\nthem are, we better fix there as gettext() may or may not be cheap\nanyway.\n\nA new target is added to run test with poisoned gettext: test-poison\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n I don't know the story behind this compile-time switch. The series [1]\n that introduces it does not say much.\n\n This at least makes it easier for me to run poison tests instead of\n building another binary, if I remember it. Next step could be make\n \"make test\" run both normal and poison modes, but I'm not sure how to\n do it nicely yet.\n\n [1] http://thread.gmane.org/gmane.comp.version-control.git/167307/focus=167438\n\n Makefile  | 12 +++---------\n gettext.c | 10 ----------\n gettext.h | 12 +++++++-----\n po/README | 13 ++++---------\n 4 files changed, 14 insertions(+), 33 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 6b0c961..8255fb9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -258,11 +258,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@@ -1594,9 +1589,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@@ -2497,7 +2489,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\n@@ -2545,6 +2536,9 @@ export NO_SVN_TESTS\n test: all\n \t$(MAKE) -C t/ all\n \n+poison-test: all\n+\t$(MAKE) POISON_GETTEXT=YesPlease -C t/ all\n+\n perf: all\n \t$(MAKE) -C t/perf/ all\n \ndiff --git a/gettext.c b/gettext.c\nindex f75bca7..6aa822c 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -16,16 +16,6 @@\n #\tendif\n #endif\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-\treturn poison_requested;\n-}\n-#endif\n-\n #ifndef NO_GETTEXT\n static void init_gettext_charset(const char *domain)\n {\ndiff --git a/gettext.h b/gettext.h\nindex 57ba8bb..a77554f 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -36,11 +36,13 @@ static inline void git_setup_gettext(void)\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+static inline 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+\treturn poison_requested;\n+}\n \n static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)\n {\ndiff --git a/po/README b/po/README\nindex c1520e8..e25752b 100644\n--- a/po/README\n+++ b/po/README\n@@ -270,16 +270,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 you should run the test suite with\n+gettext poison support, which emits gibberish on every call to\n+gettext:\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 && 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\n-- \n1.7.12.rc2\n"},{"id":"197502","messageId":"20120821052404.GB55686@mannheim-rule.local","threadId":"31304","inReplyTo":"1345523970-14914-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Build in gettext poison feature unconditionally","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-08-21T05:24:04Z","receivedAt":"2012-08-21T05:24:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyễn Thái Ngọc Duy wrote:\n\n> The runtime cost should be small. \"gcc -O2\" inlines _() and\n> use_gettext_poison(). But even if it does not, performance should not\n> be impacted as _() calls are usually not on critical path. If some of\n> them are, we better fix there as gettext() may or may not be cheap\n> anyway.\n\nThat seems reasonable to me.  The increase in code size of a commonly\ninlined function and the extra \"if\" in a common if not\nperformance-critical codepath is annoying (and I'd prefer to keep\nuse_gettext_poison() un-inlined), but in any event the cost would go\naway once the translation-based implementation of poison lands.\n\n[...]\n>  I don't know the story behind this compile-time switch. The series [1]\n>  that introduces it does not say much.\n\nI think it was just paranoia about performance regressions.\n\n>  This at least makes it easier for me to run poison tests instead of\n>  building another binary, if I remember it. Next step could be make\n>  \"make test\" run both normal and poison modes, but I'm not sure how to\n>  do it nicely yet.\n\nYes, that would be nice (or perhaps a mode to run most tests in\nthe current locale and rerun test assertions that use a test_i18n*\nhelper or C_LOCALE_OUTPUT prerequisite in the C locale).\n\nHope that helps,\nJonathan\n"},{"id":"197534","messageId":"7va9xofbgo.fsf@alter.siamese.dyndns.org","threadId":"31304","inReplyTo":"20120821052404.GB55686@mannheim-rule.local","subject":"Re: [PATCH] Build in gettext poison feature unconditionally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-21T16:37:11Z","receivedAt":"2012-08-21T16:37:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Nguyễn Thái Ngọc Duy wrote:\n>\n>> The runtime cost should be small. \"gcc -O2\" inlines _() and\n>> use_gettext_poison(). But even if it does not, performance should not\n>> be impacted as _() calls are usually not on critical path. If some of\n>> them are, we better fix there as gettext() may or may not be cheap\n>> anyway.\n>\n> That seems reasonable to me.  The increase in code size of a commonly\n> inlined function and the extra \"if\" in a common if not\n> performance-critical codepath is annoying (and I'd prefer to keep\n> use_gettext_poison() un-inlined), but in any event the cost would go\n> away once the translation-based implementation of poison lands.\n\nI would say it is not worse than just \"annoying\"; if the cost will\ngo away, I'd rather see this conversion postponed and is done as\npart of (and preferrably at the end of) the \"poison with a\npoison-locale\" series.\n\n> Yes, that would be nice (or perhaps a mode to run most tests in\n> the current locale and rerun test assertions that use a test_i18n*\n> helper or C_LOCALE_OUTPUT prerequisite in the C locale).\n\nYeah, I think that is the right direction to go; I suspect that\n\"poison with a poison-locale\" would have to be ready to allow us to\ngo there, though.\n"},{"id":"197571","messageId":"1345613246-4053-1-git-send-email-pclouds@gmail.com","threadId":"31304","inReplyTo":"7va9xofbgo.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Support generate poison .mo files for testing","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-22T05:27:26Z","receivedAt":"2012-08-22T05:27:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"test-poisongen does a similar job to gettext poison feature except\nthat it does it at build time. Gibberish .mo files are generated for\nall supported langauges and put in po/build/poison-locale. Target\n\"poison-locale\" is for this.\n\nUser can run the test with these .mo files by setting POISON_LOCALE\nwhile running the test suite. User must also set LANG/LC_* correctly\n(and the system is supposed to support that locale).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n On Tue, Aug 21, 2012 at 11:37 PM, Junio C Hamano <gitster@pobox.com> wrote:\n > I would say it is not worse than just \"annoying\"; if the cost will\n > go away, I'd rather see this conversion postponed and is done as\n > part of (and preferrably at the end of) the \"poison with a\n > poison-locale\" series.\n\n OK let me redo step one. test-poisongen requires libgettextpo. I'm\n not sure if this library if gnu specific. We may need another flag\n for it instead of NO_GETTEXT. We don't need a fake language code with\n this approach.\n\n Makefile         |  19 ++++++++\n t/test-lib.sh    |  10 +++-\n test-poisongen.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n wrap-for-bin.sh  |   6 ++-\n 4 files changed, 171 insertions(+), 3 deletions(-)\n mode change 100644 => 100755 t/test-lib.sh\n create mode 100644 test-poisongen.c\n mode change 100644 => 100755 wrap-for-bin.sh\n\ndiff --git a/Makefile b/Makefile\nindex 6b0c961..6ea2665 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -496,6 +496,9 @@ TEST_PROGRAMS_NEED_X += test-mergesort\n TEST_PROGRAMS_NEED_X += test-mktemp\n TEST_PROGRAMS_NEED_X += test-parse-options\n TEST_PROGRAMS_NEED_X += test-path-utils\n+ifndef NO_GETTEXT\n+TEST_PROGRAMS_NEED_X += test-poisongen\n+endif\n TEST_PROGRAMS_NEED_X += test-revision-walking\n TEST_PROGRAMS_NEED_X += test-run-command\n TEST_PROGRAMS_NEED_X += test-scrap-cache-tree\n@@ -2428,6 +2431,19 @@ endif\n po/build/locale/%/LC_MESSAGES/git.mo: po/%.po\n \t$(QUIET_MSGFMT)mkdir -p $(dir $@) && $(MSGFMT) -o $@ $<\n \n+ifndef NO_GETTEXT\n+POISON_MOFILES := $(patsubst po/%.po,po/build/poison-locale/%/LC_MESSAGES/git.mo,$(POFILES))\n+\n+po/build/poison-locale/%.po: po/%.po test-poisongen$X po/git.pot\n+\t$(QUIET_MSGFMT)mkdir -p $(dir $@) && \\\n+\t./test-poisongen po/git.pot $@\n+\n+po/build/poison-locale/%/LC_MESSAGES/git.mo: po/build/poison-locale/%.po\n+\t$(QUIET_MSGFMT)mkdir -p $(dir $@) && $(MSGFMT) -o $@ $<\n+\n+poison-locale: $(POISON_MOFILES)\n+endif\n+\n FIND_SOURCE_FILES = ( git ls-files '*.[hcS]' 2>/dev/null || \\\n \t\t\t$(FIND) . \\( -name .git -type d -prune \\) \\\n \t\t\t\t-o \\( -name '*.[hcS]' -type f -print \\) )\n@@ -2564,6 +2580,9 @@ test-svn-fe$X: vcs-svn/lib.a\n \n .PRECIOUS: $(TEST_OBJS)\n \n+test-poisongen$X: test-poisongen.o GIT-LDFLAGS $(GITLIBS)\n+\t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS) -lgettextpo\n+\n test-%$X: test-%.o GIT-LDFLAGS $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)\n \ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nold mode 100644\nnew mode 100755\nindex bb4f886..d4060e8\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -55,8 +55,10 @@ GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n export PERL_PATH SHELL_PATH\n \n # For repeatability, reset the environment to known value.\n-LANG=C\n-LC_ALL=C\n+if [ -z \"$POISON_LOCALE\" ]; then\n+\tLANG=C\n+\tLC_ALL=C\n+fi\n PAGER=cat\n TZ=UTC\n TERM=dumb\n@@ -92,6 +94,10 @@ export GIT_MERGE_VERBOSITY GIT_MERGE_AUTOEDIT\n export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME\n export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME\n export EDITOR\n+if test -n \"$POISON_LOCALE\"; then\n+\tGIT_POISON_LOCALE=yes\n+\texport GIT_POISON_LOCALE\n+fi\n \n # Protect ourselves from common misconfiguration to export\n # CDPATH into the environment\ndiff --git a/test-poisongen.c b/test-poisongen.c\nnew file mode 100644\nindex 0000000..5905aa9\n--- /dev/null\n+++ b/test-poisongen.c\n@@ -0,0 +1,139 @@\n+#include <gettext-po.h>\n+#include \"cache.h\"\n+#include \"strbuf.h\"\n+\n+static void xerror(int severity,\n+\t\t   po_message_t message,\n+\t\t   const char *filename, size_t lineno, size_t column,\n+\t\t   int multiline_p, const char *message_text)\n+{\n+\tdie(\"%s:%d:%d %s\", filename, lineno, column, message_text);\n+}\n+\n+static void xerror2(int severity,\n+\t\t    po_message_t message1,\n+\t\t    const char *filename1, size_t lineno1, size_t column1,\n+\t\t    int multiline_p1, const char *message_text1,\n+\t\t    po_message_t message2,\n+\t\t    const char *filename2, size_t lineno2, size_t column2,\n+\t\t    int multiline_p2, const char *message_text2)\n+{\n+\tdie(\"%s:%d:%d %s (%s:%d:%d %s)\",\n+\t    filename1, lineno1, column1, message_text1,\n+\t    filename2, lineno2, column2, message_text2);\n+}\n+\n+static void translate(const char *msg, struct strbuf *buf)\n+{\n+\tconst char *end = msg + strlen(msg);\n+\tconst char *text = \"* GETTEXT POISON *\";\n+\tint text_len = strlen(text);\n+\tint t = 0;\n+\n+\tstrbuf_reset(buf);\n+\t/* preserve \\n and printf format specifiers because msgfmt\n+\t   barfs otherwise. */\n+\twhile (msg < end) {\n+\t\t/* printf specifiers and shell variables, it's a quite\n+\t\t   relax check */\n+\t\tif ((*msg == '%' || *msg == '$') && msg+1 < end) {\n+\t\t\tstrbuf_addch(buf, *msg++);\n+\t\t\tdo\n+\t\t\t       strbuf_addch(buf, *msg);\n+\t\t\twhile (msg < end && !isspace(*msg++));\n+\t\t} else if (*msg == '\\n') {\n+\t\t\t/* we only need to preserve trailing newlines, doing\n+\t\t\t   more does not really harm */\n+\t\t\tstrbuf_addch(buf, '\\n');\n+\t\t\tmsg++;\n+\t\t} else {\n+\t\t\tstrbuf_addch(buf, text[t]);\n+\t\t\tt = (t + 1) % text_len;\n+\t\t\tmsg++;\n+\t\t}\n+\t}\n+}\n+\n+int main(int argc, char **argv)\n+{\n+\tconst char *project_header = \"Project-Id-Version: \";\n+\tconst char *charset_header = \"Content-Type: text/plain; charset=\";\n+\tconst char *plural_header = \"Plural-Forms: nplurals=\";\n+\tstruct po_xerror_handler handler = { xerror, xerror2 };\n+\tpo_file_t src;\n+\tconst char *file = argv[1];\n+\tpo_message_iterator_t iter;\n+\tpo_message_t msg;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tif (argc != 3)\n+\t\tdie(\"usage: test-poisongen <pot file> <poison po file>\");\n+\n+\tsrc = po_file_read(file, &handler);\n+\tif (src == NULL)\n+\t\tdie(\"could not open %s\\n\", file);\n+\n+\titer = po_message_iterator(src, \"messages\");\n+\twhile ((msg = po_next_message(iter)) != NULL) {\n+\t\t/* msgid \"\" is the header, special handling */\n+\t\tif (po_message_msgid(msg)[0] == '\\0') {\n+\t\t\tconst char *p;\n+\n+\t\t\t/* no fuzzy, msgfmt does not like it */\n+\t\t\tpo_message_set_fuzzy(msg, 0);\n+\n+\t\t\tstrbuf_reset(&buf);\n+\t\t\tstrbuf_addstr(&buf, po_message_msgstr(msg));\n+\n+\t\t\tif (!prefixcmp(buf.buf, project_header) &&\n+\t\t\t    !prefixcmp(buf.buf + strlen(project_header),\n+\t\t\t\t       \"PACKAGE VERSION\\n\")) {\n+\t\t\t\tstrbuf_splice(&buf, strlen(project_header),\n+\t\t\t\t\t      strlen(\"PACKAGE VERSION\"),\n+\t\t\t\t\t      \"git poison\",\n+\t\t\t\t\t      strlen(\"git poison\"));\n+\t\t\t}\n+\n+\t\t\t/* Content-Type: text/plain; charset=UTF-8 */\n+\t\t\tif ((p = strstr(buf.buf, charset_header)) != NULL &&\n+\t\t\t    !prefixcmp(p + strlen(charset_header), \"CHARSET\\n\")) {\n+\t\t\t\tp += strlen(charset_header);\n+\t\t\t\tstrbuf_splice(&buf, p - buf.buf, strlen(\"CHARSET\"),\n+\t\t\t\t\t      \"UTF-8\", strlen(\"UTF-8\"));\n+\t\t\t}\n+\n+\t\t\t/* Plural-Forms: nplurals=2; plural=1 */\n+\t\t\tif ((p = strstr(buf.buf, plural_header)) != NULL &&\n+\t\t\t    !prefixcmp(p + strlen(plural_header), \"INTEGER; plural=EXPRESSION\")) {\n+\t\t\t\tint offset;\n+\t\t\t\tp += strlen(plural_header);\n+\t\t\t\toffset = p - buf.buf;\n+\t\t\t\tstrbuf_splice(&buf, offset, strlen(\"INTEGER\"), \"2\", 1);\n+\t\t\t\toffset += 1;\n+\t\t\t\tassert(!prefixcmp(buf.buf + offset, \"; plural=EXPRESSION\"));\n+\t\t\t\toffset += strlen(\"; plural=\");\n+\t\t\t\tstrbuf_splice(&buf, offset, strlen(\"EXPRESSION\"), \"1\", 1);\n+\t\t\t}\n+\n+\t\t\tpo_message_set_msgstr(msg, buf.buf);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (po_message_msgid_plural(msg)) {\n+\t\t\tint index = 0;\n+\n+\t\t\ttranslate(po_message_msgid(msg), &buf);\n+\t\t\tpo_message_set_msgstr_plural(msg, index++, buf.buf);\n+\n+\t\t\twhile (po_message_msgstr_plural(msg, index)) {\n+\t\t\t\ttranslate(po_message_msgid_plural(msg), &buf);\n+\t\t\t\tpo_message_set_msgstr_plural(msg, index++, buf.buf);\n+\t\t\t}\n+\t\t} else {\n+\t\t\ttranslate(po_message_msgid(msg), &buf);\n+\t\t\tpo_message_set_msgstr(msg, buf.buf);\n+\t\t}\n+\t}\n+\n+\tpo_file_write(src, argv[2], &handler);\n+\treturn 0;\n+}\ndiff --git a/wrap-for-bin.sh b/wrap-for-bin.sh\nold mode 100644\nnew mode 100755\nindex 53a8dd0..99bc816\n--- a/wrap-for-bin.sh\n+++ b/wrap-for-bin.sh\n@@ -15,7 +15,11 @@ else\n \texport GIT_TEMPLATE_DIR\n fi\n GITPERLLIB='@@BUILD_DIR@@/perl/blib/lib'\n-GIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale'\n+if test -n \"$GIT_POISON_LOCALE\"; then\n+\tGIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/poison-locale'\n+else\n+\tGIT_TEXTDOMAINDIR='@@BUILD_DIR@@/po/build/locale'\n+fi\n PATH='@@BUILD_DIR@@/bin-wrappers:'\"$PATH\"\n export GIT_EXEC_PATH GITPERLLIB PATH GIT_TEXTDOMAINDIR\n \n-- \n1.7.12.rc2\n"},{"id":"197576","messageId":"7vboi3b2n8.fsf@alter.siamese.dyndns.org","threadId":"31304","inReplyTo":"1345613246-4053-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Support generate poison .mo files for testing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-22T11:13:31Z","receivedAt":"2012-08-22T11:13:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n\n> test-poisongen does a similar job to gettext poison feature except\n> that it does it at build time. Gibberish .mo files are generated for\n> all supported langauges and put in po/build/poison-locale. Target\n> \"poison-locale\" is for this.\n\nWhat is the significance of this locale being \"Gibberish\"?\nCurrently, for any string, we give \"### gettext poison ###\" or\nsomething but the only thing we care about in the poison mode is\nthat it is different from the message id, no?  I was wondering if\nthese phony translations can be something simple like \"Add QQ at the\nbeginning of the message id string\" and still can catch mistakenly\nmarked messages that come from the plumbing layer, or something.\n\nAs you have already written a printf skipper that looks fairly\nconservative, perhaps I shouldn't be worried too much about it, but\nwe seem to be spending considerable effort on the \"poison\", and it\nmakes me wonder (even though no better alternative comes to mind) if\nwe could do better.  The reason we do \"poison\" (be it the current\none or locale based one) in the first place is so that we want to\nmake sure messages from the plumbing are not marked for i18n, and we\ndo so by running our test under the \"poison\" mode that produces\noutput different from the in-code text that are marked for i18n,\nwhich somehow feels quite a roundabout way of doing so.\n\n> User can run the test with these .mo files by setting POISON_LOCALE\n> while running the test suite. User must also set LANG/LC_* correctly\n> (and the system is supposed to support that locale).\n\n>  OK let me redo step one. test-poisongen requires libgettextpo. I'm\n>  not sure if this library if gnu specific. We may need another flag\n>  for it instead of NO_GETTEXT. We don't need a fake language code with\n>  this approach.\n\t\t\nOK.\n\n>  Makefile         |  19 ++++++++\n>  t/test-lib.sh    |  10 +++-\n>  test-poisongen.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  wrap-for-bin.sh  |   6 ++-\n>  4 files changed, 171 insertions(+), 3 deletions(-)\n>  mode change 100644 => 100755 t/test-lib.sh\n>  create mode 100644 test-poisongen.c\n>  mode change 100644 => 100755 wrap-for-bin.sh\n\nThanks.  I suspect two mode changes weren't intentional?\n\n> +static void translate(const char *msg, struct strbuf *buf)\n> +{\n> +\tconst char *end = msg + strlen(msg);\n> +\tconst char *text = \"* GETTEXT POISON *\";\n> +\tint text_len = strlen(text);\n> +\tint t = 0;\n> +\n> +\tstrbuf_reset(buf);\n> +\t/* preserve \\n and printf format specifiers because msgfmt\n> +\t   barfs otherwise. */\n> +\twhile (msg < end) {\n> +\t\t/* printf specifiers and shell variables, it's a quite\n> +\t\t   relax check */\n> +\t\tif ((*msg == '%' || *msg == '$') && msg+1 < end) {\n> +\t\t\tstrbuf_addch(buf, *msg++);\n> +\t\t\tdo\n> +\t\t\t       strbuf_addch(buf, *msg);\n> +\t\t\twhile (msg < end && !isspace(*msg++));\n> +\t\t} else if (*msg == '\\n') {\n> +\t\t\t/* we only need to preserve trailing newlines, doing\n> +\t\t\t   more does not really harm */\n> +\t\t\tstrbuf_addch(buf, '\\n');\n> +\t\t\tmsg++;\n> +\t\t} else {\n> +\t\t\tstrbuf_addch(buf, text[t]);\n> +\t\t\tt = (t + 1) % text_len;\n> +\t\t\tmsg++;\n> +\t\t}\n> +\t}\n> +}\n"},{"id":"197579","messageId":"CACsJy8Dp5FZd9Ko6o18r5TeS5dfbjv61wOmP1452zyvHfX5LMQ@mail.gmail.com","threadId":"31304","inReplyTo":"7vboi3b2n8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Support generate poison .mo files for testing","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-22T12:37:04Z","receivedAt":"2012-08-22T12:37:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 22, 2012 at 6:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n>\n>> test-poisongen does a similar job to gettext poison feature except\n>> that it does it at build time. Gibberish .mo files are generated for\n>> all supported langauges and put in po/build/poison-locale. Target\n>> \"poison-locale\" is for this.\n>\n> What is the significance of this locale being \"Gibberish\"?\n> Currently, for any string, we give \"### gettext poison ###\" or\n> something but the only thing we care about in the poison mode is\n> that it is different from the message id, no?  I was wondering if\n> these phony translations can be something simple like \"Add QQ at the\n> beginning of the message id string\" and still can catch mistakenly\n> marked messages that come from the plumbing layer, or something.\n\nI'm gradually getting there, partly thanks to your question about\ngrepping \"tracked\" in another thread. This patch does not really\ngenerate random strings. It repeats the pattern \"* gettext poison *\"\nfor evey character that can be replaced. But a better way could be\nreplacing \"tracked\" with \"t r a c k e d\". We know the rule so we can\nrecreate the that string from \"tracked\" in test_i18n*. Or reverse the\nupper/lower case, whichever is easier for the recreation by test_i18n*\n\n>>  mode change 100644 => 100755 t/test-lib.sh\n>>  create mode 100644 test-poisongen.c\n>>  mode change 100644 => 100755 wrap-for-bin.sh\n>\n> Thanks.  I suspect two mode changes weren't intentional?\n\nIt's an emacs hook gone bad.\n-- \nDuy\n"},{"id":"197590","messageId":"7v393ec2w9.fsf@alter.siamese.dyndns.org","threadId":"31304","inReplyTo":"CACsJy8Dp5FZd9Ko6o18r5TeS5dfbjv61wOmP1452zyvHfX5LMQ@mail.gmail.com","subject":"Re: [PATCH] Support generate poison .mo files for testing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-22T16:22:46Z","receivedAt":"2012-08-22T16:22:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> But a better way could be\n> replacing \"tracked\" with \"t r a c k e d\". We know the rule so we can\n> recreate the that string from \"tracked\" in test_i18n*. Or reverse the\n> upper/lower case, whichever is easier for the recreation by test_i18n*\n\nThat does not make much sense to me, so either one of us must be\nslightly confused.  I thought the only purpose of testing with the\n\"poison\" was to find messages that must not be localized but were\nlocalized by mistake.  For that, we have to make sure that anything\nthat uses test_i18n* is reading from Porcelain, in other words, we\nmust use the byte-for-byte comparison without using test_i18n* when\nverifying the plumbing output.  And the primary requirement for this\narrangement to work is that the expected output in C locale and the\nactual ouptut in the synthetic poison locale are reliably different.\nThey do not have to be reversible (I was actually going to suggest\nrot13 of the original instead of cycling through the \"* gettext\npoison *\" in your patch --- prefixing with QQ would not work, as it\nis likely that the test with \"grep\" may not be anchored at the left\nend).\n\nTeaching test_i18n* to fuzzily match the expected output in C locale\nagainst the actual output in synthetic poison locale may or may not\nbe doable, but spending any cycle working on that sounds like a\ntotal waste of time (even though it might be fun).  It does not test\nthat we are translating Porcelain messages correctly.\n\nAm I missing something?  Puzzled...\n"},{"id":"197593","messageId":"7vtxvuand1.fsf@alter.siamese.dyndns.org","threadId":"31304","inReplyTo":"1345613246-4053-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Support generate poison .mo files for testing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-22T16:43:38Z","receivedAt":"2012-08-22T16:43:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> +static void translate(const char *msg, struct strbuf *buf)\n> +{\n> +\tconst char *end = msg + strlen(msg);\n> +\tconst char *text = \"* GETTEXT POISON *\";\n> +\tint text_len = strlen(text);\n> +\tint t = 0;\n> +\n> +\tstrbuf_reset(buf);\n> +\t/* preserve \\n and printf format specifiers because msgfmt\n> +\t   barfs otherwise. */\n> +\twhile (msg < end) {\n> +\t\t/* printf specifiers and shell variables, it's a quite\n> +\t\t   relax check */\n> +\t\tif ((*msg == '%' || *msg == '$') && msg+1 < end) {\n> +\t\t\tstrbuf_addch(buf, *msg++);\n> +\t\t\tdo\n> +\t\t\t       strbuf_addch(buf, *msg);\n> +\t\t\twhile (msg < end && !isspace(*msg++));\n\nAside from the Style:\n\n        do {\n                ...\n        } while ();\n\nwhy are you special casing a run of non-blank letters that begin\nwith a dollar sign (swapping two ints is done with \"%2$d %1$d\", a\npercent still at the beginning, so there must be something else I am\nmissing)?\n\nAlso why do you stop at isspace()?  Isn't a \" \" (space) a flag that\nmeans \"If the first character of a signed conversion is not a sign\nor if a signed conversion results in no characters, a <space> shall\nbe prefixed to the result.\"\n\nAs the flags, min-width, precision, and length do not share the same\ncharacter as the conversion that has to come at the end, I think you\nonly want to do something like\n\n\t/*\n         * conversion specifier characters, taken from:\n         * http://pubs.opengroup.org/onlinepubs/9699919799/functions/printf.html\n         */\n        static const char printf_conversion[] = \"diouxXfFeEgGaAcspnCS%\";\n\n        ...\n\n        while (msg < end) {\n                if (*msg == '%') {\n                        strbuf_addch(buf, *msg++);\n                        while (msg < end) {\n                                int ch = *msg++;\n                                strbuf_addch(buf, ch);\n                                if (strchr(printf_conversion, ch))\n                                        break;\n                        }\n                        /* copied the printf part literally */\n                        continue;\n\t\t}\n\t\t... keep \\n ...\n                ... muck with string ...\n\t}\n\nperhaps?\n"},{"id":"197709","messageId":"CACsJy8Bc=GZH+d_VLMqHFKz93zBed5oHjOjUgWa4piY-tpdUNA@mail.gmail.com","threadId":"31304","inReplyTo":"7v393ec2w9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Support generate poison .mo files for testing","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-23T10:53:52Z","receivedAt":"2012-08-23T10:53:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 22, 2012 at 11:22 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n>\n>> But a better way could be\n>> replacing \"tracked\" with \"t r a c k e d\". We know the rule so we can\n>> recreate the that string from \"tracked\" in test_i18n*. Or reverse the\n>> upper/lower case, whichever is easier for the recreation by test_i18n*\n>\n> That does not make much sense to me, so either one of us must be\n> slightly confused.  I thought the only purpose of testing with the\n> \"poison\" was to find messages that must not be localized but were\n> localized by mistake.  For that, we have to make sure that anything\n> that uses test_i18n* is reading from Porcelain, in other words, we\n> must use the byte-for-byte comparison without using test_i18n* when\n> verifying the plumbing output.  And the primary requirement for this\n> arrangement to work is that the expected output in C locale and the\n> actual ouptut in the synthetic poison locale are reliably different.\n> They do not have to be reversible (I was actually going to suggest\n> rot13 of the original instead of cycling through the \"* gettext\n> poison *\" in your patch --- prefixing with QQ would not work, as it\n> is likely that the test with \"grep\" may not be anchored at the left\n> end).\n\nYes, for verifying plumbing output that should be enough.\n\n> Teaching test_i18n* to fuzzily match the expected output in C locale\n> against the actual output in synthetic poison locale may or may not\n> be doable, but spending any cycle working on that sounds like a\n> total waste of time (even though it might be fun).  It does not test\n> that we are translating Porcelain messages correctly.\n\nRight. I was aiming at testing translated messages but obviously went\noff track trying to test a fake locale instead. Thanks for stopping\nme.\n-- \nDuy\n"},{"id":"197710","messageId":"CACsJy8Ak0iR9WytPys1uy=M52TVh+0y4DWrv63Efvie4nsRORA@mail.gmail.com","threadId":"31304","inReplyTo":"7vtxvuand1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Support generate poison .mo files for testing","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-08-23T11:00:57Z","receivedAt":"2012-08-23T11:00:57Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Aug 22, 2012 at 11:43 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> why are you special casing a run of non-blank letters that begin\n> with a dollar sign (swapping two ints is done with \"%2$d %1$d\", a\n> percent still at the beginning, so there must be something else I am\n> missing)?\n\n'$' is for shell variables. I should separate it from C messages. All\nthese because now that we run the (fake) translation through gettext\ntoolchain, it'll catch us if we don't preserve dynamic parts\ncorrectly.\n\n> Also why do you stop at isspace()?  Isn't a \" \" (space) a flag that\n> means \"If the first character of a signed conversion is not a sign\n> or if a signed conversion results in no characters, a <space> shall\n> be prefixed to the result.\"\n\nA hurry attempt to get past msgfmt. I should refine these code, either by...\n\n> As the flags, min-width, precision, and length do not share the same\n> character as the conversion that has to come at the end, I think you\n> only want to do something like\n>\n>         /*\n>          * conversion specifier characters, taken from:\n>          * http://pubs.opengroup.org/onlinepubs/9699919799/functions/printf.html\n>          */\n>         static const char printf_conversion[] = \"diouxXfFeEgGaAcspnCS%\";\n>\n>         ...\n>\n>         while (msg < end) {\n>                 if (*msg == '%') {\n>                         strbuf_addch(buf, *msg++);\n>                         while (msg < end) {\n>                                 int ch = *msg++;\n>                                 strbuf_addch(buf, ch);\n>                                 if (strchr(printf_conversion, ch))\n>                                         break;\n>                         }\n>                         /* copied the printf part literally */\n>                         continue;\n>                 }\n>                 ... keep \\n ...\n>                 ... muck with string ...\n>         }\n>\n> perhaps?\n\nfollowing this, or copying the matching logic from msgfmt source.\n-- \nDuy\n"}]}