[PATCH 00/11] [RFH] Introduce support for GETTEXT_POISON=rot13

This patch series is incomplete because I lack the time to finish it, and I
hope that there are interested developers who can help with it.

In https://lore.kernel.org/git/20181022153633.31757-1-pclouds@gmail.com/,
Duy proposed introducing a fake language they called "Ook" to be able to
test translations better. "Ook" would "translate" messages without any
printf format specifiers to "Eek", otherwise convert all upper-case letters
to "Ook" and lower-case letters to "ook".

Gábor replied with a different proposal that has the advantage of being
bijective, i.e. it is possible to reconstruct the untranslated message from
the translated one.

Ævar suggested recently
[https://lore.kernel.org/git/xmqqim836v6m.fsf@gitster.c.googlers.com/T/#m6fdc43d4f1eb3f20f841096c59e985b69c84875e]
that this whole GETTEXT_POISON business is totally useless.

I do not believe that it is useless. To back up my belief with something
tangible, 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.

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).

Note: in its current form, this patch series fails t0013 and t9902 in the
GETTEXT_POISON job
[https://github.com/gitgitgadget/git/pull/836/checks?check_run_id=1686715225#step:4:1590],
therefore it cannot be integrated into Git as-is, even if it does not switch
the GETTEXT_POISON job to use the rot13 mode yet.

Sadly, even though I want to see this patch series go further, I am
operating under serious time constraints and therefore had to admit to
myself, grudgingly, that I can't. So again, I hope that somebody else can
push this effort further.

Johannes Schindelin (11):
  tests: remove unnecessary GIT_TEST_GETTEXT_POISON=false constructs
  Support GIT_TEST_GETTEXT_POISON=rot13
  parse-options: add forgotten translations
  sha1dc: mark forgotten message for translation
  t0006: use `test_i18ncmp` only for C locales
  t0041: stop using `test_i18ngrep` on untranslated output
  t0027: mark a function as requiring the C locale
  t6301: do not expect the output of `for-each-ref` to be translated
  GETTEXT_POISON=rot13: do compare the output in `test_i18ncmp`
  GETTEXT_POISON=rot13: perform actual checks in `test_i18ngrep`
  test-tool i18n: do not complain about empty messages

 Makefile                       |   1 +
 gettext.c                      |  65 ++++++++++++++++-
 gettext.h                      |   5 +-
 parse-options-cb.c             |   4 +-
 sha1dc_git.c                   |   2 +-
 t/helper/test-i18n.c           | 129 +++++++++++++++++++++++++++++++++
 t/helper/test-tool.c           |   1 +
 t/helper/test-tool.h           |   1 +
 t/t0006-date.sh                |   4 +-
 t/t0017-env-helper.sh          |   6 +-
 t/t0027-auto-crlf.sh           |   3 +
 t/t0041-usage.sh               |   2 +-
 t/t1305-config-include.sh      |   6 +-
 t/t6301-for-each-ref-errors.sh |   2 +-
 t/t7201-co.sh                  |   4 +-
 t/t9902-completion.sh          |   1 -
 t/test-lib-functions.sh        |  14 +++-
 t/test-lib.sh                  |   1 +
 18 files changed, 228 insertions(+), 23 deletions(-)
 create mode 100644 t/helper/test-i18n.c


base-commit: 72c4083ddf91b489b7b7b812df67ee8842177d98
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-836%2Fdscho%2Fgettext-poison-should-rot13-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-836/dscho/gettext-poison-should-rot13-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/836
-- 
gitgitgadget

From: Johannes Schindelin <redacted>

Signed-off-by: Johannes Schindelin <redacted>
---
 parse-options-cb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index 4542d4d3f92..00862af486e 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c

@@ -91,7 +91,7 @@ int parse_opt_commits(const struct option *opt, const char *arg, int unset)
 	if (!arg)
 		return -1;
 	if (get_oid(arg, &oid))
-		return error("malformed object name %s", arg);
+		return error(_("malformed object name %s"), arg);
 	commit = lookup_commit_reference(the_repository, &oid);
 	if (!commit)
 		return error("no such commit %s", arg);

@@ -110,7 +110,7 @@ int parse_opt_commit(const struct option *opt, const char *arg, int unset)
 	if (!arg)
 		return -1;
 	if (get_oid(arg, &oid))
-		return error("malformed object name %s", arg);
+		return error(_("malformed object name %s"), arg);
 	commit = lookup_commit_reference(the_repository, &oid);
 	if (!commit)
 		return error("no such commit %s", arg);

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

Signed-off-by: Johannes Schindelin <redacted>
---
 gettext.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 gettext.h |  5 +++--
 2 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/gettext.c b/gettext.c
index 35d2c1218db..7e8966d8326 100644
--- a/gettext.c
+++ b/gettext.c

@@ -68,11 +68,72 @@ const char *get_preferred_languages(void)
 int use_gettext_poison(void)
 {
 	static int poison_requested = -1;
-	if (poison_requested == -1)
-		poison_requested = git_env_bool("GIT_TEST_GETTEXT_POISON", 0);
+	if (poison_requested == -1) {
+		const char *v = getenv("GIT_TEST_GETTEXT_POISON");
+		if (v && !strcmp(v, "rot13"))
+			poison_requested = 2;
+		else
+			poison_requested =
+				git_env_bool("GIT_TEST_GETTEXT_POISON", 0);
+	}
 	return poison_requested;
 }
 
+static inline char do_rot13(char c)
+{
+	if (c >= 'a' && c <= 'm')
+		return c + 'n' - 'a';
+	if (c >= 'n' && c <= 'z')
+		return c + 'a' - 'n';
+	if (c >= 'A' && c <= 'M')
+		return c + 'N' - 'A';
+	if (c >= 'N' && c <= 'Z')
+		return c + 'A' - 'N';
+	return c;
+}
+
+const char *gettext_maybe_rot13(const char *msgid)
+{
+	static struct strbuf round_robin[4] = {
+		STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
+	};
+	static int current;
+	struct strbuf *buf;
+
+	if (use_gettext_poison() != 2)
+		return "# GETTEXT POISON #";
+
+	buf = &round_robin[current++];
+	if (current >= ARRAY_SIZE(round_robin))
+		current = 0;
+
+	strbuf_reset(buf);
+	while (*msgid) {
+		const char *p = strchrnul(msgid, '%'), *spec;
+
+		while (*p && p[1] == '%')
+			p = strchrnul(p + 2, '%');
+
+		if (p != msgid) {
+			strbuf_addstr(buf, "<rot13>");
+			while (p != msgid)
+				strbuf_addch(buf, do_rot13(*(msgid++)));
+			strbuf_addstr(buf, "</rot13>");
+		}
+
+		if (!*p)
+			break;
+
+		spec = strpbrk(p + 1, "diouxXeEfFgGaAcsCSpnm%");
+		if (!spec)
+			BUG("Unrecognized format string: %s", p);
+		strbuf_add(buf, p, spec + 1 - p);
+		msgid = spec + 1;
+	}
+
+	return buf->buf;
+}
+
 #ifndef NO_GETTEXT
 static int test_vsnprintf(const char *fmt, ...)
 {

diff --git a/gettext.h b/gettext.h
index bee52eb1134..298adda7e15 100644
--- a/gettext.h
+++ b/gettext.h

@@ -29,6 +29,7 @@
 #define FORMAT_PRESERVING(n) __attribute__((format_arg(n)))
 
 int use_gettext_poison(void);
+const char *gettext_maybe_rot13(const char *msgid);
 
 #ifndef NO_GETTEXT
 void git_setup_gettext(void);

@@ -48,14 +49,14 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
 {
 	if (!*msgid)
 		return "";
-	return use_gettext_poison() ? "# GETTEXT POISON #" : gettext(msgid);
+	return use_gettext_poison() ? gettext_maybe_rot13(msgid) : gettext(msgid);
 }
 
 static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
 const char *Q_(const char *msgid, const char *plu, unsigned long n)
 {
 	if (use_gettext_poison())
-		return "# GETTEXT POISON #";
+		return gettext_maybe_rot13(n < 2 ? msgid : plu);
 	return ngettext(msgid, plu, n);
 }
 

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

The idea of the `GETTEXT_POISON` mode is to test translated messages, at
least _somewhat_.

There is not really any point in turning off that mode by force, except
_maybe_ to test the mode itself.

So let's avoid overriding `GIT_TEST_GETTEXT_POISON` in the test suite
unless testing the `GETTEXT_POISON` functionality itself.

Signed-off-by: Johannes Schindelin <redacted>
---
 t/t0017-env-helper.sh     | 6 ++----
 t/t1305-config-include.sh | 6 ++----
 t/t7201-co.sh             | 4 ++--
 t/t9902-completion.sh     | 1 -
 4 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/t/t0017-env-helper.sh b/t/t0017-env-helper.sh
index c1ecf6aeac6..e0931310306 100755
--- a/t/t0017-env-helper.sh
+++ b/t/t0017-env-helper.sh

@@ -85,10 +85,8 @@ test_expect_success 'env--helper reads config thanks to trace2' '
 	git config -f home/.gitconfig include.path cycle &&
 	git config -f home/cycle include.path .gitconfig &&
 
-	test_must_fail \
-		env HOME="$(pwd)/home" GIT_TEST_GETTEXT_POISON=false \
-		git config -l 2>err &&
-	grep "exceeded maximum include depth" err &&
+	test_must_fail env HOME="$(pwd)/home" git config -l 2>err &&
+	test_i18ngrep "exceeded maximum include depth" err &&
 
 	test_must_fail \
 		env HOME="$(pwd)/home" GIT_TEST_GETTEXT_POISON=true \

diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index f1e1b289f98..308c5d530b3 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh

@@ -352,10 +352,8 @@ test_expect_success 'include cycles are detected' '
 	git init --bare cycle &&
 	git -C cycle config include.path cycle &&
 	git config -f cycle/cycle include.path config &&
-	test_must_fail \
-		env GIT_TEST_GETTEXT_POISON=false \
-		git -C cycle config --get-all test.value 2>stderr &&
-	grep "exceeded maximum include depth" stderr
+	test_must_fail git -C cycle config --get-all test.value 2>stderr &&
+	test_i18ngrep "exceeded maximum include depth" stderr
 '
 
 test_done

diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index b36a93056fd..630406a73c5 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh

@@ -245,8 +245,8 @@ test_expect_success 'checkout to detach HEAD' '
 	rev=$(git rev-parse --short renamer^) &&
 	git checkout -f renamer &&
 	git clean -f &&
-	GIT_TEST_GETTEXT_POISON=false git checkout renamer^ 2>messages &&
-	grep "HEAD is now at $rev" messages &&
+	git checkout renamer^ 2>messages &&
+	test_i18ngrep "HEAD is now at $rev" messages &&
 	test_line_count -gt 1 messages &&
 	H=$(git rev-parse --verify HEAD) &&
 	M=$(git show-ref -s --verify refs/heads/master) &&

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a1c4f1f6d40..e5adee27d41 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh

@@ -2363,7 +2363,6 @@ test_expect_success 'sourcing the completion script clears cached commands' '
 '
 
 test_expect_success 'sourcing the completion script clears cached merge strategies' '
-	GIT_TEST_GETTEXT_POISON=false &&
 	__git_compute_merge_strategies &&
 	verbose test -n "$__git_merge_strategies" &&
 	. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

Signed-off-by: Johannes Schindelin <redacted>
---
 t/t0041-usage.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t0041-usage.sh b/t/t0041-usage.sh
index 5b927b76fe5..27309b1a8f8 100755
--- a/t/t0041-usage.sh
+++ b/t/t0041-usage.sh

@@ -42,7 +42,7 @@ test_expect_success 'tag usage error' '
 
 test_expect_success 'branch --contains <existent_commit>' '
 	git branch --contains "master" >actual 2>actual.err &&
-	test_i18ngrep "master" actual &&
+	grep "master" actual &&
 	test_line_count = 0 actual.err
 '
 

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

We cannot really test anything else, as we do not control the output of
`strftime()`.

Signed-off-by: Johannes Schindelin <redacted>
---
 t/t0006-date.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 75ee9a96b80..9f27e202d36 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh

@@ -9,7 +9,7 @@ GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
 check_relative() {
 	t=$(($GIT_TEST_DATE_NOW - $1))
 	echo "$t -> $2" >expect
-	test_expect_${3:-success} "relative date ($2)" "
+	test_expect_${3:-success} C_LOCALE_OUTPUT "relative date ($2)" "
 	test-tool date relative $t >actual &&
 	test_i18ncmp expect actual
 	"

@@ -137,7 +137,7 @@ check_approxidate '2009-12-01' '2009-12-01 19:20:00'
 check_date_format_human() {
 	t=$(($GIT_TEST_DATE_NOW - $1))
 	echo "$t -> $2" >expect
-	test_expect_success "human date $t" '
+	test_expect_success C_LOCALE_OUTPUT "human date $t" '
 		test-tool date human $t >actual &&
 		test_i18ncmp expect actual
 '

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

The `check_warning` function uses regular expressions matching English
text, it therefore won't work reliably with anything but the C locale.

In particular, we won't be able to match the needle 'in' under
GETTEXT_POISON=rot13.

Signed-off-by: Johannes Schindelin <redacted>
---
 t/t0027-auto-crlf.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 9fcd56fab37..baa5c6f435b 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh

@@ -76,6 +76,9 @@ create_NNO_MIX_files () {
 }
 
 check_warning () {
+	# The `grep` and `sed` below expect the locale to be "C"
+	test_have_prereq C_LOCALE_OUTPUT || return 0
+
 	case "$1" in
 	LF_CRLF) echo "warning: LF will be replaced by CRLF" >"$2".expect ;;
 	CRLF_LF) echo "warning: CRLF will be replaced by LF" >"$2".expect ;;

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

The `test_i18ncmp` function is reserved for output that we expect to be
translated.

Signed-off-by: Johannes Schindelin <redacted>
---
 t/t6301-for-each-ref-errors.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t6301-for-each-ref-errors.sh b/t/t6301-for-each-ref-errors.sh
index 809854fc0ce..0cf2a41c26c 100755
--- a/t/t6301-for-each-ref-errors.sh
+++ b/t/t6301-for-each-ref-errors.sh

@@ -20,7 +20,7 @@ test_expect_success 'Broken refs are reported correctly' '
 	test_when_finished "rm -f .git/$r" &&
 	echo "warning: ignoring broken ref $r" >broken-err &&
 	git for-each-ref >out 2>err &&
-	test_i18ncmp full-list out &&
+	test_cmp full-list out &&
 	test_i18ncmp broken-err err
 '
 

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

Signed-off-by: Johannes Schindelin <redacted>
---
 t/helper/test-i18n.c    | 42 ++++++++++++++++++++++++++++++++++++++++-
 t/test-lib-functions.sh |  6 ++++++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-i18n.c b/t/helper/test-i18n.c
index 4b572e6efad..8a16df6b40f 100644
--- a/t/helper/test-i18n.c
+++ b/t/helper/test-i18n.c

@@ -1,8 +1,10 @@
 #include "test-tool.h"
 #include "cache.h"
+#include "grep.h"
 
 static const char *usage_msg = "\n"
-"  test-tool i18n cmp <file1> <file2>\n";
+"  test-tool i18n cmp <file1> <file2>\n"
+"  test-tool i18n grep <regex> <file>\n";
 
 static inline char do_rot13(char c)
 {

@@ -75,6 +77,42 @@ static int i18n_cmp(const char **argv)
 	return 0;
 }
 
+static int i18n_grep(const char **argv)
+{
+	int dont_match = 0;
+	const char *pattern, *path;
+
+	struct grep_opt opt;
+	struct grep_source source;
+	struct strbuf buf = STRBUF_INIT;
+	int hit;
+
+	if (*argv && !strcmp("!", *argv)) {
+		dont_match = 1;
+		argv++;
+	}
+
+	pattern = *(argv++);
+	path = *(argv++);
+
+	if (!pattern || !path || *argv)
+		usage(usage_msg);
+
+	grep_init(&opt, the_repository, NULL);
+	append_grep_pattern(&opt, pattern, "command line", 0, GREP_PATTERN);
+	compile_grep_patterns(&opt);
+
+	if (strbuf_read_file(&buf, path, 0) < 0)
+		die_errno("could not read %s", path);
+	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;
+}
+
 int cmd__i18n(int argc, const char **argv)
 {
 	argv++;

@@ -82,6 +120,8 @@ int cmd__i18n(int argc, const char **argv)
 		usage(usage_msg);
 	if (!strcmp(*argv, "cmp"))
 		return i18n_cmp(argv+1);
+	else if (!strcmp(*argv, "grep"))
+		return i18n_grep(argv+1);
 	else
 		usage(usage_msg);
 

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 08731bae854..394fd2ef5be 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

@@ -1021,6 +1021,12 @@ test_i18ngrep () {
 
 	if 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

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

Signed-off-by: Johannes Schindelin <redacted>
---
 Makefile                |  1 +
 t/helper/test-i18n.c    | 89 +++++++++++++++++++++++++++++++++++++++++
 t/helper/test-tool.c    |  1 +
 t/helper/test-tool.h    |  1 +
 t/test-lib-functions.sh |  8 +++-
 t/test-lib.sh           |  1 +
 6 files changed, 100 insertions(+), 1 deletion(-)
 create mode 100644 t/helper/test-i18n.c

diff --git a/Makefile b/Makefile
index 7b64106930a..d49f80cb524 100644
--- a/Makefile
+++ b/Makefile

@@ -710,6 +710,7 @@ TEST_BUILTINS_OBJS += test-genzeros.o
 TEST_BUILTINS_OBJS += test-hash-speed.o
 TEST_BUILTINS_OBJS += test-hash.o
 TEST_BUILTINS_OBJS += test-hashmap.o
+TEST_BUILTINS_OBJS += test-i18n.o
 TEST_BUILTINS_OBJS += test-index-version.o
 TEST_BUILTINS_OBJS += test-json-writer.o
 TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o

diff --git a/t/helper/test-i18n.c b/t/helper/test-i18n.c
new file mode 100644
index 00000000000..4b572e6efad
--- /dev/null
+++ b/t/helper/test-i18n.c

@@ -0,0 +1,89 @@
+#include "test-tool.h"
+#include "cache.h"
+
+static const char *usage_msg = "\n"
+"  test-tool i18n cmp <file1> <file2>\n";
+
+static inline char do_rot13(char c)
+{
+	if (c >= 'a' && c <= 'm')
+		return c + 'n' - 'a';
+	if (c >= 'n' && c <= 'z')
+		return c + 'a' - 'n';
+	if (c >= 'A' && c <= 'M')
+		return c + 'N' - 'A';
+	if (c >= 'N' && c <= 'Z')
+		return c + 'A' - 'N';
+	return c;
+}
+
+static size_t unrot13(char *buf)
+{
+	char *p = buf, *q = buf;
+
+	while (*p) {
+		const char *begin = strstr(p, "<rot13>"), *end;
+
+		if (!begin)
+			break;
+
+		while (p != begin)
+			*(q++) = *(p++);
+
+		p += strlen("<rot13>");
+		end = strstr(p, "</rot13>");
+		if (!end)
+			BUG("could not find </rot13> in\n%s", buf);
+
+		while (p != end)
+			*(q++) = do_rot13(*(p++));
+		p += strlen("</rot13>");
+	}
+
+	while (*p)
+		*(q++) = *(p++);
+
+	return q - buf;
+}
+
+static void unrot13_strbuf(struct strbuf *buf)
+{
+	size_t len = unrot13(buf->buf);
+
+	if (len == buf->len)
+		die("not ROT13'ed:\n%s", buf->buf);
+	buf->len = len;
+}
+
+static int i18n_cmp(const char **argv)
+{
+	const char *path1 = *(argv++), *path2 = *(argv++);
+	struct strbuf a = STRBUF_INIT, b = STRBUF_INIT;
+
+	if (!path1 || !path2 || *argv)
+		usage(usage_msg);
+
+	if (strbuf_read_file(&a, path1, 0) < 0)
+		die_errno("could not read %s", path1);
+	if (strbuf_read_file(&b, path2, 0) < 0)
+		die_errno("could not read %s", path2);
+	unrot13_strbuf(&b);
+
+	if (a.len != b.len || memcmp(a.buf, b.buf, a.len))
+		return 1;
+
+	return 0;
+}
+
+int cmd__i18n(int argc, const char **argv)
+{
+	argv++;
+	if (!*argv)
+		usage(usage_msg);
+	if (!strcmp(*argv, "cmp"))
+		return i18n_cmp(argv+1);
+	else
+		usage(usage_msg);
+
+	return 0;
+}

diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 9d6d14d9293..7e1680ac108 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c

@@ -34,6 +34,7 @@ static struct test_cmd cmds[] = {
 	{ "genzeros", cmd__genzeros },
 	{ "hashmap", cmd__hashmap },
 	{ "hash-speed", cmd__hash_speed },
+	{ "i18n", cmd__i18n },
 	{ "index-version", cmd__index_version },
 	{ "json-writer", cmd__json_writer },
 	{ "lazy-init-name-hash", cmd__lazy_init_name_hash },

diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index a6470ff62c4..43282a520ea 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h

@@ -24,6 +24,7 @@ int cmd__genrandom(int argc, const char **argv);
 int cmd__genzeros(int argc, const char **argv);
 int cmd__hashmap(int argc, const char **argv);
 int cmd__hash_speed(int argc, const char **argv);
+int cmd__i18n(int argc, const char **argv);
 int cmd__index_version(int argc, const char **argv);
 int cmd__json_writer(int argc, const char **argv);
 int cmd__lazy_init_name_hash(int argc, const char **argv);

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 999982fe4a9..08731bae854 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh

@@ -993,7 +993,13 @@ test_cmp_bin () {
 # under GIT_TEST_GETTEXT_POISON this pretends that the command produced expected
 # results.
 test_i18ncmp () {
-	! test_have_prereq C_LOCALE_OUTPUT || test_cmp "$@"
+	if test rot13 = "$GIT_TEST_GETTEXT_POISON"
+	then
+		test-tool i18n cmp "$@"
+	elif test_have_prereq C_LOCALE_OUTPUT
+	then
+		test_cmp "$@"
+	fi
 }
 
 # Use this instead of "grep expected-string actual" to see if the

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9fa7c1d0f6d..c9f9e2804fd 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -1537,6 +1537,7 @@ then
 fi
 
 test_lazy_prereq C_LOCALE_OUTPUT '
+	test rot13 != "$GIT_TEST_GETTEXT_POISON" &&
 	! test_bool_env GIT_TEST_GETTEXT_POISON false
 '
 

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

They might, or might not be 'translated'. Let's not assume that they
aren't marked with `_()` for translation.

Signed-off-by: Johannes Schindelin <redacted>
---
 t/helper/test-i18n.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/helper/test-i18n.c b/t/helper/test-i18n.c
index 8a16df6b40f..82efc66f1f5 100644
--- a/t/helper/test-i18n.c
+++ b/t/helper/test-i18n.c

@@ -52,7 +52,7 @@ static void unrot13_strbuf(struct strbuf *buf)
 {
 	size_t len = unrot13(buf->buf);
 
-	if (len == buf->len)
+	if (len && len == buf->len)
 		die("not ROT13'ed:\n%s", buf->buf);
 	buf->len = len;
 }

-- 
gitgitgadget

From: Johannes Schindelin <redacted>

Signed-off-by: Johannes Schindelin <redacted>
---
 sha1dc_git.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sha1dc_git.c b/sha1dc_git.c
index 5c300e812e0..fecf5da1483 100644
--- a/sha1dc_git.c
+++ b/sha1dc_git.c

@@ -18,7 +18,7 @@ void git_SHA1DCFinal(unsigned char hash[20], SHA1_CTX *ctx)
 {
 	if (!SHA1DCFinal(hash, ctx))
 		return;
-	die("SHA-1 appears to be part of a collision attack: %s",
+	die(_("SHA-1 appears to be part of a collision attack: %s"),
 	    hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1]));
 }
 

-- 
gitgitgadget

On Tue, Jan 12, 2021 at 08:47:32AM +0000, Johannes Schindelin via GitGitGadget wrote:

From: Johannes Schindelin <redacted>

The idea of the `GETTEXT_POISON` mode is to test translated messages, at
least _somewhat_.

There is not really any point in turning off that mode by force, except
_maybe_ to test the mode itself.

So let's avoid overriding `GIT_TEST_GETTEXT_POISON` in the test suite
unless testing the `GETTEXT_POISON` functionality itself.

Signed-off-by: Johannes Schindelin <redacted>
---

I think this change caused the failure in t9902 that you mentioned in
the cover letter.  

If 'git merge' is invoked with a nonexisting merge strategy, then it
errors out with an error message that contains a list of available
merge strategies.  The completion script relies on this behavior and
"parses" this error message to get the available merge strategies, but
it breaks when it can't find the expected text because it was
poisoned.

 	__git_compute_merge_strategies &&
 	verbose test -n "$__git_merge_strategies" &&
 	. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
-- 
gitgitgadget

On Tue, Jan 12, 2021 at 08:47:31AM +0000, Johannes Schindelin via GitGitGadget wrote:

Ævar suggested recently
[https://lore.kernel.org/git/xmqqim836v6m.fsf@gitster.c.googlers.com/T/#m6fdc43d4f1eb3f20f841096c59e985b69c84875e]
that this whole GETTEXT_POISON business is totally useless.

I do not believe that it is useless. To back up my belief with something
tangible, 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.

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).

I'm not entirely convinced by this. The original point of the poison
code was not to find opportunities to translate strings, but to make
sure we did not accidentally translate a string that some script was
relying on. And I don't see any fixes for the latter here (and as Ævar
suggested in the linked thread, the fact that we're not combing through
existing code looking for translations makes such an error a lot less
likely).

Which isn't to say repurposing it in the other direction might not be
worthwhile. But I suspect a lot of the test failures are just false
positives. Until now it was always reasonable to conservatively use
test_i18ngrep for cases which could reasonably translated, even if they
were not yet.

Likewise, I'm not sure that one can reliably rot13 an output for
test_i18ncmp. It could contain mixed translated and untranslated bits
from different messages.

So I dunno. You did find two spots where translations could be used.
But if nobody actually saw them to care that they got translated, were
they important? I may be a bit biased as somebody who would not use the
translations in the first place, of course.

-Peff

On Tue, Jan 12, 2021 at 08:47:35AM +0000, Johannes Schindelin via GitGitGadget wrote:

I didn't find any list discussion, but I think I may have actually left
this untranslated intentionally. Like a BUG(), we'd expect it to come up
basically never. And when it does, being able to search for the exact
wording online may be more important than providing a translated
version.

It probably doesn't matter that much either way, though.

-Peff

On Tue, Jan 12 2021, Johannes Schindelin via GitGitGadget wrote:

This patch series is incomplete because I lack the time to finish it, and I
hope that there are interested developers who can help with it.

In https://lore.kernel.org/git/20181022153633.31757-1-pclouds@gmail.com/,
Duy proposed introducing a fake language they called "Ook" to be able to
test translations better. "Ook" would "translate" messages without any
printf format specifiers to "Eek", otherwise convert all upper-case letters
to "Ook" and lower-case letters to "ook".

Gábor replied with a different proposal that has the advantage of being
bijective, i.e. it is possible to reconstruct the untranslated message from
the translated one.

Ævar suggested recently
[https://lore.kernel.org/git/xmqqim836v6m.fsf@gitster.c.googlers.com/T/#m6fdc43d4f1eb3f20f841096c59e985b69c84875e]
that this whole GETTEXT_POISON business is totally useless.

To clarify what I really mean, but admittedly am not always the best at
articulating: I do not think GETTEXT_POISON is useless, it's useful.

The interesting question is "useful enough to bother?", or in economics
terms "is this investment in time/money worth the opportunity cost?".

Any hoops we make people jump through when writing tests takes away time
and attention from other things.

My motivation here is that I feel this whole poison business is all my
fault. Every time some newbie submits a patch and needs to re-roll a v2
because they used "grep" instead of "test_i18ngrep" in a case that
clearly didn't involve a plumbing string I cringe a little about the
technical debt.

So while I still think my series (just nuke it) would be better overall,
I'm secretly rooting for yours. If the consensus is that this is worth
keeping and improving perhaps we haven't been mostly wasting our time :)

On to discussing this series:

I do not believe that it is useless. To back up my belief with something
tangible, 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.

We could get most of the way there by checking e.g. if "GETTEXT POISON"
is in the output, but it wouldn't assert that the message is 100%
translated as this WIP series does.

In our off-list discussion you maintained that
"GIT_TEST_GETTEXT_POISON=false <cmd>" was an anti-pattern. Was it
because you thought "test_i18n*" et al was actually doing some "is
poison?" comparison as we now do, or just because you didn't
conceptually like a pattern like:

    GIT_TEST_GETTEXT_POISON=false git cmd >out &&
    grep string out

Being changed later to:

    GIT_TEST_GETTEXT_POISON=false git cmd >out &&
    grep string out &&
    grep plumbing-string out

Or whatever, as opposed to:

    git cmd >out &&
    test_i18ngrep string out &&
    test_i18ngrep plumbing-string out

I get the aesthetic preference, I'm just wondering if I'm missing
something else about it...

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:

-----------------

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 #"
+	}
+	;;
 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 <file1> <file2>\n"

@@ -34,8 +36,12 @@ static size_t unrot13(char *buf)
 
 		p += strlen("<rot13>");
 		end = strstr(p, "</rot13>");
-		if (!end)
-			BUG("could not find </rot13> in\n%s", buf);
+		if (!end) {
+			if (git_env_bool("GIT_TEST_GETTEXT_POISON_PEDANTIC", 0))
+				BUG("could not find </rot13> 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.

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?

Hi Peff,

On Tue, 12 Jan 2021, Jeff King wrote:

On Tue, Jan 12, 2021 at 08:47:35AM +0000, Johannes Schindelin via GitGitGadget wrote:

quoted

diff --git a/sha1dc_git.c b/sha1dc_git.c
index 5c300e812e0..fecf5da1483 100644
--- a/sha1dc_git.c
+++ b/sha1dc_git.c

@@ -18,7 +18,7 @@ void git_SHA1DCFinal(unsigned char hash[20], SHA1_CTX *ctx)
 {
 	if (!SHA1DCFinal(hash, ctx))
 		return;
-	die("SHA-1 appears to be part of a collision attack: %s",
+	die(_("SHA-1 appears to be part of a collision attack: %s"),
 	    hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1]));

I didn't find any list discussion, but I think I may have actually left
this untranslated intentionally. Like a BUG(), we'd expect it to come up
basically never. And when it does, being able to search for the exact
wording online may be more important than providing a translated
version.

I disagree with that reasoning. By that rationale, any message we deem to
be somewhat rare should be _untranslated_.

A much better rule, at least from my perspective is: is the target
audience the Git users? If so, the message is to be translated. If not,
then not.

In this instance, it is quite obviously targeting the Git users who need
to understand why the command they tried to run was failing. The test in
t0013 is totally agreeing with this:

	test_expect_success 'test-sha1 detects shattered pdf' '
		test_must_fail test-tool sha1 <"$TEST_DATA/shattered-1.pdf" 2>err &&
		test_i18ngrep collision err &&
		grep 38762cf7f55934b34d179ae6a4c80cadccbb7f0a err
	'

Notice that `test_i18ngrep`? It tells me that we expect this message to be
translated.

Ciao,
Dscho

Hi Ævar,


On Tue, 12 Jan 2021, Ævar Arnfjörð Bjarmason wrote:

On Tue, Jan 12 2021, Johannes Schindelin via GitGitGadget wrote:

quoted

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 <cmd>" 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.

quoted

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 :-(

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?)

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

On Fri, Jan 15, 2021 at 04:43:05PM +0100, Johannes Schindelin wrote:

quoted

quoted

-	die("SHA-1 appears to be part of a collision attack: %s",
+	die(_("SHA-1 appears to be part of a collision attack: %s"),
 	    hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1]));

I didn't find any list discussion, but I think I may have actually left
this untranslated intentionally. Like a BUG(), we'd expect it to come up
basically never. And when it does, being able to search for the exact
wording online may be more important than providing a translated
version.

I disagree with that reasoning. By that rationale, any message we deem to
be somewhat rare should be _untranslated_.

A much better rule, at least from my perspective is: is the target
audience the Git users? If so, the message is to be translated. If not,
then not.

That's what I was getting at. The audience is really Git developers,
just like it would be for a BUG(). We don't expect either of those
things to happen.

In this instance, it is quite obviously targeting the Git users who need
to understand why the command they tried to run was failing. The test in
t0013 is totally agreeing with this:

	test_expect_success 'test-sha1 detects shattered pdf' '
		test_must_fail test-tool sha1 <"$TEST_DATA/shattered-1.pdf" 2>err &&
		test_i18ngrep collision err &&
		grep 38762cf7f55934b34d179ae6a4c80cadccbb7f0a err
	'

Notice that `test_i18ngrep`? It tells me that we expect this message to be
translated.

Well, I wrote both that line and the untranslated original code, so I'm
not sure what we can deduce from that. ;)

But yeah, I am not strongly opposed to translating this. I brought it up
more in the line of "I don't think it's that big a deal that it was not
translated".

-Peff

Hi Peff,

On Fri, 15 Jan 2021, Jeff King wrote:

On Fri, Jan 15, 2021 at 04:43:05PM +0100, Johannes Schindelin wrote:

quoted

quoted

quoted

-	die("SHA-1 appears to be part of a collision attack: %s",
+	die(_("SHA-1 appears to be part of a collision attack: %s"),
 	    hash_to_hex_algop(hash, &hash_algos[GIT_HASH_SHA1]));

I didn't find any list discussion, but I think I may have actually left
this untranslated intentionally. Like a BUG(), we'd expect it to come up
basically never. And when it does, being able to search for the exact
wording online may be more important than providing a translated
version.

I disagree with that reasoning. By that rationale, any message we deem to
be somewhat rare should be _untranslated_.

A much better rule, at least from my perspective is: is the target
audience the Git users? If so, the message is to be translated. If not,
then not.

That's what I was getting at. The audience is really Git developers,
just like it would be for a BUG(). We don't expect either of those
things to happen.

While a SHA-1 collision might not be anything we expect to happen, I am
fairly certain it won't be a bug in Git causing it. Nor will it be
anything that core Git developers have to react on. For those reasons, I
disagree that core Git developers are the target audience of this message.

Ciao,
Dscho