Hi Ævar, On Tue, 12 Jan 2021, Ævar Arnfjörð Bjarmason wrote: > On Tue, Jan 12 2021, Johannes Schindelin via GitGitGadget wrote: > > > I implemented a GETTEXT_POISON=rot13 mode and ran the test suite to > > see whether we erroneously expect translated messages where they > > aren't, and related problems. > > I agree that rot13ing it makes it much more useful in the sense that > before we could over-tag things with test_i18n* and we'd just "return 0" > there in the poison mode, now we actually check if the string is > poisoned. Precisely. > In our off-list discussion you maintained that > "GIT_TEST_GETTEXT_POISON=false " was an anti-pattern. Was it > because you thought "test_i18n*" et al was actually doing some "is > poison?" [...] I maintain this view because the whole point of having a GETTEXT_POISON job is to have the entire test suite run in that mode. Using this anti-pattern says to me "the author could not make this work correctly in GETTEXT_POISON mode". Even more, _iff_ we expect certain validations to be skipped in the GETTEXT_POISON job, using `GIT_TEST_GETTEXT_POISON=false` opens the possibility that this job fails when it never should have failed. > > And the experiment delivered. It is just a demonstration (I only addressed a > > handful of the test suite failures, 124 test scripts still need to be > > inspected to find out why they fail), of course. Yet I think that finding > > e.g. the missing translations of sha1dc-cb and parse-options show that it > > would be very useful to complete this patch series and then use the rot13 > > mode in our CI jobs (instead of the current GETTEXT_POISON=true mode, which > > really is less useful). > > The following patch on top cuts down on the failures by more than half: Interesting! I forgot the shell half... Curious that the scripted parts _still_ affect so much of our test suite :-( > ----------------- > diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh > index 8eef60b43f..f9239d2917 100644 > --- a/git-sh-i18n.sh > +++ b/git-sh-i18n.sh > @@ -17,7 +17,10 @@ export TEXTDOMAINDIR > > # First decide what scheme to use... > GIT_INTERNAL_GETTEXT_SH_SCHEME=fallthrough > -if test -n "$GIT_TEST_GETTEXT_POISON" && > +if test -n "$GIT_TEST_GETTEXT_POISON" -a "$GIT_TEST_GETTEXT_POISON" = "rot13" > +then > + GIT_INTERNAL_GETTEXT_SH_SCHEME=rot13poison > +elif test -n "$GIT_TEST_GETTEXT_POISON" && > git env--helper --type=bool --default=0 --exit-code \ > GIT_TEST_GETTEXT_POISON > then > @@ -63,6 +66,21 @@ gettext_without_eval_gettext) > ) > } > ;; > +rot13poison) > + # Emit garbage so that tests that incorrectly rely on translatable > + # strings will fail. > + gettext () { > + printf "%s" "# SHELL GETTEXT POISON #" > + } > + > + eval_gettext () { > + printf "%s" "# SHELL GETTEXT POISON #" > + } > + > + eval_ngettext () { > + printf "%s" "# SHELL GETTEXT POISON #" > + } Right, and for that, I'd rather use the `test-tool i18n` helper (it is appropriate to expect `test-tool` to be in the `PATH` in GETTEXT_POISON mode, right?) > + ;; > poison) > # Emit garbage so that tests that incorrectly rely on translatable > # strings will fail. > diff --git a/t/helper/test-i18n.c b/t/helper/test-i18n.c > index 82efc66f1f..1f0fa3d041 100644 > --- a/t/helper/test-i18n.c > +++ b/t/helper/test-i18n.c > @@ -1,6 +1,8 @@ > #include "test-tool.h" > #include "cache.h" > #include "grep.h" > +#include "config.h" > > static const char *usage_msg = "\n" > " test-tool i18n cmp \n" > @@ -34,8 +36,12 @@ static size_t unrot13(char *buf) > > p += strlen(""); > end = strstr(p, ""); > - if (!end) > - BUG("could not find in\n%s", buf); > + if (!end) { > + if (git_env_bool("GIT_TEST_GETTEXT_POISON_PEDANTIC", 0)) > + BUG("could not find in\n%s", buf); > + else > + break; > + } > > while (p != end) > *(q++) = do_rot13(*(p++)); > @@ -67,8 +73,12 @@ static int i18n_cmp(const char **argv) > > if (strbuf_read_file(&a, path1, 0) < 0) > die_errno("could not read %s", path1); > + if (strstr(a.buf, "# SHELL GETTEXT POISON #")) > + return 0; > if (strbuf_read_file(&b, path2, 0) < 0) > die_errno("could not read %s", path2); > + if (strstr(b.buf, "# SHELL GETTEXT POISON #")) > + return 0; > unrot13_strbuf(&b); > > if (a.len != b.len || memcmp(a.buf, b.buf, a.len)) > @@ -79,7 +89,6 @@ static int i18n_cmp(const char **argv) > > static int i18n_grep(const char **argv) > { > - int dont_match = 0; > const char *pattern, *path; > > struct grep_opt opt; > @@ -87,11 +96,6 @@ static int i18n_grep(const char **argv) > struct strbuf buf = STRBUF_INIT; > int hit; > > - if (*argv && !strcmp("!", *argv)) { > - dont_match = 1; > - argv++; > - } > - > pattern = *(argv++); > path = *(argv++); > > @@ -104,13 +108,15 @@ static int i18n_grep(const char **argv) > > if (strbuf_read_file(&buf, path, 0) < 0) > die_errno("could not read %s", path); > + if (strstr(buf.buf, "# SHELL GETTEXT POISON #")) > + return 0; > unrot13_strbuf(&buf); > grep_source_init(&source, GREP_SOURCE_BUF, path, path, path); > source.buf = buf.buf; > source.size = buf.len; > hit = grep_source(&opt, &source); > strbuf_release(&buf); > - return dont_match ^ !hit; > + return !hit; > } > > int cmd__i18n(int argc, const char **argv) > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > index 394fd2ef5b..6c580a3000 100644 > --- a/t/test-lib-functions.sh > +++ b/t/test-lib-functions.sh > @@ -1019,14 +1019,12 @@ test_i18ngrep () { > BUG "too few parameters to test_i18ngrep" > fi > > - if test_have_prereq !C_LOCALE_OUTPUT > + grep_cmd=grep > + if test "$GIT_TEST_GETTEXT_POISON" == "rot13" > + then > + grep_cmd="test-tool i18n grep" > + elif test_have_prereq !C_LOCALE_OUTPUT > then > - if test rot13 = "$GIT_TEST_GETTEXT_POISON" > - then > - test-tool i18n grep "$@" > - return $? > - fi > - > # pretend success > return 0 > fi > @@ -1034,13 +1032,13 @@ test_i18ngrep () { > if test "x!" = "x$1" > then > shift > - ! grep "$@" && return 0 > + ! $grep_cmd "$@" && return 0 > > - echo >&4 "error: '! grep $@' did find a match in:" > + echo >&4 "error: '! $grep_cmd $@' did find a match in:" > else > - grep "$@" && return 0 > + $grep_cmd "$@" && return 0 > > - echo >&4 "error: 'grep $@' didn't find a match in:" > + echo >&4 "error: '$grep_cmd $@' didn't find a match in:" > fi > > if test -s "$last_arg" > ----------------- > > I.e. most of this was because you didn't add any support for this to the > shellscript translations. > > We still punt on that here, it should ideally preserve the shell > variables like the format specifiers, but I think that's not worth the > effort with us actively cutting down our shell code to nothing. I'm not sure how fast we'll get there, what with `git submodule` being slow to get over the finish line, and with `mergetool`/`difftool` still being scripted, and probably for a long time, too. > We still have failures e.g. due to "test_i18ngrep -F fixed file" not > being supported. Wouldn't a better/simpler approach be to just have a > light rot13 helper, and then call the actual "cmp"/"grep" with the > munged input/output? Hmm. I hoped that this work would open the door to introducing more C helpers in the test suite, thereby accelerating it on Windows. We could also potentially move the `cmp`/`grep` parts into separate helpers that give more useful output in case of a failure. (Right now, if anything fails in CI involving `grep` or `cmp`, the user investigating this pretty much _has_ to try to reproduce the issue locally because the output is so not helpful, and reproducing it might be a challenge e.g. when the failure is macOS-specific and the developer does not have access to a macOS setup). Ciao, Dscho