[BUG] test suite broken with GETTEXT_POISON=YesPlease

[BUG] test suite broken with GETTEXT_POISON=YesPlease

[Ævar Arnfjörð Bjarmason]

As a refresh of everyone's memory (because mine needed it). This is a
feature I added back in 2011 when the i18n support was initially
added.

There was concern at the time that we would inadvertently mark
plumbing messages for translation, particularly something in a shared
code path, and this was a way to hopefully smoke out those issues with
the test suite.

However compiling with it breaks a couple of dozen tests, I stopped
digging when I saw some broke back in 2014.

What should be done about this? I think if we're going to keep them
they need to be run regularly by something like Travis (Lars CC'd),
however empirical evidence suggests that not running them is just fine
too, so should we just remove support for this test mode?

I don't care, but I can come up with the patch either way, but would
only be motivated to write the one-time fix for it if some CI system
is actually running them regularly, otherwise they'll just be subject
to bitrotting again.

Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease

[Michael J Gruber]

Ævar Arnfjörð Bjarmason venit, vidit, dixit 20.04.2017 23:58:
> As a refresh of everyone's memory (because mine needed it). This is a
> feature I added back in 2011 when the i18n support was initially
> added.
> 
> There was concern at the time that we would inadvertently mark
> plumbing messages for translation, particularly something in a shared
> code path, and this was a way to hopefully smoke out those issues with
> the test suite.
> 
> However compiling with it breaks a couple of dozen tests, I stopped
> digging when I saw some broke back in 2014.
> 
> What should be done about this? I think if we're going to keep them
> they need to be run regularly by something like Travis (Lars CC'd),
> however empirical evidence suggests that not running them is just fine
> too, so should we just remove support for this test mode?
> 
> I don't care, but I can come up with the patch either way, but would
> only be motivated to write the one-time fix for it if some CI system
> is actually running them regularly, otherwise they'll just be subject
> to bitrotting again.
> 

I use that switch when I change something that involves l10n, but
usually I run specific tests only. To be honest: I have to make sure not
to get confused by (nor forget one of) the build flag GETTEXT_POISON and
the environment variable GIT_GETTEXT_POISON. I'm not sure I always
tested what I meant to test...

Michael

Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease

[Lars Schneider]

> Am 20.04.2017 um 23:58 schrieb Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
> 
> As a refresh of everyone's memory (because mine needed it). This is a
> feature I added back in 2011 when the i18n support was initially
> added.
> 
> There was concern at the time that we would inadvertently mark
> plumbing messages for translation, particularly something in a shared
> code path, and this was a way to hopefully smoke out those issues with
> the test suite.
> 
> However compiling with it breaks a couple of dozen tests, I stopped
> digging when I saw some broke back in 2014.
> 
> What should be done about this? I think if we're going to keep them
> they need to be run regularly by something like Travis (Lars CC'd),
> however empirical evidence suggests that not running them is just fine
> too, so should we just remove support for this test mode?

Right now we are building and testing Git in the following configurations:

1. Linux, gcc, stable Perforce an GitLFS version (used by git-p4 tests)
2. Linux, gcc, stable Perforce an GitLFS version (used by git-p4 tests) *
3. OSX, clang, latest Perforce an GitLFS version (used by git-p4 tests)
4. OSX, clang, latest Perforce an GitLFS version (used by git-p4 tests) *
5. Linux32, gcc, no git-p4 tests
6. Windows, gcc, no git-p4 tests

1-4 run the same tests right now. This was especially useful in the beginning to identify flaky tests (t0025 is still flaky!).

We could easily run the tests in 1-4 with different configurations. E.g. enable GETTEXT_POISON in 2.

Cheers,
Lars

*) 2 and 4 use the wrong compiler right now. 2 should use clang on Linux and 4 should use gcc. A patch is on my todo list.

Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease

[Ævar Arnfjörð Bjarmason]

On Fri, Apr 21, 2017 at 4:47 PM, Michael J Gruber <git@grubix.eu> wrote:
> Ævar Arnfjörð Bjarmason venit, vidit, dixit 20.04.2017 23:58:
>> As a refresh of everyone's memory (because mine needed it). This is a
>> feature I added back in 2011 when the i18n support was initially
>> added.
>>
>> There was concern at the time that we would inadvertently mark
>> plumbing messages for translation, particularly something in a shared
>> code path, and this was a way to hopefully smoke out those issues with
>> the test suite.
>>
>> However compiling with it breaks a couple of dozen tests, I stopped
>> digging when I saw some broke back in 2014.
>>
>> What should be done about this? I think if we're going to keep them
>> they need to be run regularly by something like Travis (Lars CC'd),
>> however empirical evidence suggests that not running them is just fine
>> too, so should we just remove support for this test mode?
>>
>> I don't care, but I can come up with the patch either way, but would
>> only be motivated to write the one-time fix for it if some CI system
>> is actually running them regularly, otherwise they'll just be subject
>> to bitrotting again.
>>
>
> I use that switch when I change something that involves l10n, but
> usually I run specific tests only. To be honest: I have to make sure not
> to get confused by (nor forget one of) the build flag GETTEXT_POISON and
> the environment variable GIT_GETTEXT_POISON. I'm not sure I always
> tested what I meant to test...

For any of the built-in tests, you just need to compile git with
GETTEXT_POISON=YesPlease, the env var is set by test-lib.sh for you,
you only need to set GIT_GETTEXT_POISON=1 if you're ad-hoc running
some git command yourself, e.g.:

$ ./git status
On branch master
Your branch is up-to-date with 'origin/master'.
nothing to commit, working tree clean
$ GIT_GETTEXT_POISON=1 ./git status
# GETTEXT POISON #master
# GETTEXT POISON #

Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease

[Ævar Arnfjörð Bjarmason]

On Fri, Apr 21, 2017 at 4:54 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
>> Am 20.04.2017 um 23:58 schrieb Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
>>
>> As a refresh of everyone's memory (because mine needed it). This is a
>> feature I added back in 2011 when the i18n support was initially
>> added.
>>
>> There was concern at the time that we would inadvertently mark
>> plumbing messages for translation, particularly something in a shared
>> code path, and this was a way to hopefully smoke out those issues with
>> the test suite.
>>
>> However compiling with it breaks a couple of dozen tests, I stopped
>> digging when I saw some broke back in 2014.
>>
>> What should be done about this? I think if we're going to keep them
>> they need to be run regularly by something like Travis (Lars CC'd),
>> however empirical evidence suggests that not running them is just fine
>> too, so should we just remove support for this test mode?
>
> Right now we are building and testing Git in the following configurations:
>
> 1. Linux, gcc, stable Perforce an GitLFS version (used by git-p4 tests)
> 2. Linux, gcc, stable Perforce an GitLFS version (used by git-p4 tests) *
> 3. OSX, clang, latest Perforce an GitLFS version (used by git-p4 tests)
> 4. OSX, clang, latest Perforce an GitLFS version (used by git-p4 tests) *
> 5. Linux32, gcc, no git-p4 tests
> 6. Windows, gcc, no git-p4 tests
>
> 1-4 run the same tests right now. This was especially useful in the beginning to identify flaky tests (t0025 is still flaky!).
>
> We could easily run the tests in 1-4 with different configurations. E.g. enable GETTEXT_POISON in 2.
>
> Cheers,
> Lars
>
> *) 2 and 4 use the wrong compiler right now. 2 should use clang on Linux and 4 should use gcc. A patch is on my todo list.

Great, thanks. I'll fixup the poison tests then.

[PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease

[Ævar Arnfjörð Bjarmason]

The GETTEXT_POISON=YesPlease compile-time testing option added in my
bb946bba76 ("i18n: add GETTEXT_POISON to simulate unfriendly
translator", 2011-02-22) has been slowly bitrotting as strings have
been marked for translation, and new tests have been added without
running it.

I brought this up on the list ("[BUG] test suite broken with
GETTEXT_POISON=YesPlease", [1]) asking whether this mode was useful at
all anymore. At least one person occasionally uses it, and Lars
Schneider offered to change one of the the Travis builds to run in
this mode, so fix up the failing ones.

My test setup runs most of the tests, with the notable exception of
skipping all the p4 tests, so it's possible that there's still some
lurking regressions I haven't fixed.

1. <CACBZZX62+acvi1dpkknadTL827mtCm_QesGSZ=6+UnyeMpg8+Q@mail.gmail.com>

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1309-early-config.sh          |  2 +-
 t/t1430-bad-ref-name.sh          |  2 +-
 t/t3203-branch-output.sh         |  2 +-
 t/t3404-rebase-interactive.sh    | 14 +++++++-------
 t/t3415-rebase-autosquash.sh     | 10 +++++-----
 t/t3903-stash.sh                 |  4 ++--
 t/t4205-log-pretty-formats.sh    |  4 ++--
 t/t5316-pack-delta-depth.sh      |  8 ++++++--
 t/t6134-pathspec-in-submodule.sh |  4 ++--
 t/t7004-tag.sh                   |  4 ++--
 t/t7406-submodule-update.sh      |  2 +-
 t/t7509-commit.sh                |  4 ++--
 t/t7800-difftool.sh              |  4 ++--
 13 files changed, 34 insertions(+), 30 deletions(-)

diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
index b97357b8ab..016e43b8d5 100755
--- a/t/t1309-early-config.sh
+++ b/t/t1309-early-config.sh
@@ -59,7 +59,7 @@ test_with_config () {
 
 test_expect_success 'ignore .git/ with incompatible repository version' '
 	test_with_config "[core]repositoryformatversion = 999999" 2>err &&
-	grep "warning:.* Expected git repo version <= [1-9]" err
+	test_i18ngrep "warning:.* Expected git repo version <= [1-9]" err
 '
 
 test_expect_failure 'ignore .git/ with invalid repository version' '
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index 8937e25e49..2003ec7907 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -122,7 +122,7 @@ test_expect_success 'push cannot create a badly named ref' '
 	! grep -e "broken\.\.\.ref" output
 '
 
-test_expect_failure 'push --mirror can delete badly named ref' '
+test_expect_failure !GETTEXT_POISON 'push --mirror can delete badly named ref' '
 	top=$(pwd) &&
 	git init src &&
 	git init dest &&
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 5778c0afe1..a428ae6703 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -236,7 +236,7 @@ test_expect_success 'git branch --format option' '
 	Refname is refs/heads/ref-to-remote
 	EOF
 	git branch --format="Refname is %(refname)" >actual &&
-	test_cmp expect actual
+	test_i18ncmp expect actual
 '
 
 test_done
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 33d392ba11..e07d6d8cd2 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -366,7 +366,7 @@ test_expect_success 'verbose flag is heeded, even after --continue' '
 	grep "^ file1 | 2 +-$" output
 '
 
-test_expect_success 'multi-squash only fires up editor once' '
+test_expect_success !GETTEXT_POISON 'multi-squash only fires up editor once' '
 	base=$(git rev-parse HEAD~4) &&
 	set_fake_editor &&
 	FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 squash 2 squash 3 squash 4" \
@@ -376,7 +376,7 @@ test_expect_success 'multi-squash only fires up editor once' '
 	test 1 = $(git show | grep ONCE | wc -l)
 '
 
-test_expect_success 'multi-fixup does not fire up editor' '
+test_expect_success !GETTEXT_POISON 'multi-fixup does not fire up editor' '
 	git checkout -b multi-fixup E &&
 	base=$(git rev-parse HEAD~4) &&
 	set_fake_editor &&
@@ -426,7 +426,7 @@ D
 ONCE
 EOF
 
-test_expect_success 'squash and fixup generate correct log messages' '
+test_expect_success !GETTEXT_POISON 'squash and fixup generate correct log messages' '
 	git checkout -b squash-fixup E &&
 	base=$(git rev-parse HEAD~4) &&
 	set_fake_editor &&
@@ -439,7 +439,7 @@ test_expect_success 'squash and fixup generate correct log messages' '
 	git branch -D squash-fixup
 '
 
-test_expect_success 'squash ignores comments' '
+test_expect_success !GETTEXT_POISON 'squash ignores comments' '
 	git checkout -b skip-comments E &&
 	base=$(git rev-parse HEAD~4) &&
 	set_fake_editor &&
@@ -452,7 +452,7 @@ test_expect_success 'squash ignores comments' '
 	git branch -D skip-comments
 '
 
-test_expect_success 'squash ignores blank lines' '
+test_expect_success !GETTEXT_POISON 'squash ignores blank lines' '
 	git checkout -b skip-blank-lines E &&
 	base=$(git rev-parse HEAD~4) &&
 	set_fake_editor &&
@@ -860,7 +860,7 @@ test_expect_success 'rebase -ix with several instances of --exec' '
 	test_cmp expected actual
 '
 
-test_expect_success 'rebase -ix with --autosquash' '
+test_expect_success !GETTEXT_POISON 'rebase -ix with --autosquash' '
 	git reset --hard execute &&
 	git checkout -b autosquash &&
 	echo second >second.txt &&
@@ -943,7 +943,7 @@ test_expect_success 'rebase -i --root fixup root commit' '
 	test 0 = $(git cat-file commit HEAD | grep -c ^parent\ )
 '
 
-test_expect_success 'rebase --edit-todo does not works on non-interactive rebase' '
+test_expect_success !GETTEXT_POISON 'rebase --edit-todo does not work on non-interactive rebase' '
 	git reset --hard &&
 	git checkout conflict-branch &&
 	set_fake_editor &&
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 48346f1cc0..b7265460f5 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -234,23 +234,23 @@ test_auto_fixup_fixup () {
 	fi
 }
 
-test_expect_success 'fixup! fixup!' '
+test_expect_success !GETTEXT_POISON 'fixup! fixup!' '
 	test_auto_fixup_fixup fixup fixup
 '
 
-test_expect_success 'fixup! squash!' '
+test_expect_success !GETTEXT_POISON 'fixup! squash!' '
 	test_auto_fixup_fixup fixup squash
 '
 
-test_expect_success 'squash! squash!' '
+test_expect_success !GETTEXT_POISON 'squash! squash!' '
 	test_auto_fixup_fixup squash squash
 '
 
-test_expect_success 'squash! fixup!' '
+test_expect_success !GETTEXT_POISON 'squash! fixup!' '
 	test_auto_fixup_fixup squash fixup
 '
 
-test_expect_success 'autosquash with custom inst format' '
+test_expect_success !GETTEXT_POISON 'autosquash with custom inst format' '
 	git reset --hard base &&
 	git config --add rebase.instructionFormat "[%an @ %ar] %s"  &&
 	echo 2 >file1 &&
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b71d1e659e..3b4bed5c9a 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -865,7 +865,7 @@ test_expect_success 'stash push -p with pathspec shows no changes only once' '
 	git stash push -p foo >actual &&
 	echo "No local changes to save" >expect &&
 	git reset --hard HEAD~ &&
-	test_cmp expect actual
+	test_i18ncmp expect actual
 '
 
 test_expect_success 'stash push with pathspec shows no changes when there are none' '
@@ -875,7 +875,7 @@ test_expect_success 'stash push with pathspec shows no changes when there are no
 	git stash push foo >actual &&
 	echo "No local changes to save" >expect &&
 	git reset --hard HEAD~ &&
-	test_cmp expect actual
+	test_i18ncmp expect actual
 '
 
 test_expect_success 'stash push with pathspec not in the repository errors out' '
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 21eb8c8587..1322bd461b 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -126,12 +126,12 @@ test_expect_success 'NUL separation with --stat' '
 	test_i18ncmp expected actual
 '
 
-test_expect_failure 'NUL termination with --stat' '
+test_expect_failure !GETTEXT_POISON 'NUL termination with --stat' '
 	stat0_part=$(git diff --stat HEAD^ HEAD) &&
 	stat1_part=$(git diff-tree --no-commit-id --stat --root HEAD^) &&
 	printf "add bar\n$stat0_part\n\0$(commit_msg)\n$stat1_part\n0" >expected &&
 	git log -z --stat --pretty="tformat:%s" >actual &&
-	test_i18ncmp expected actual
+	test_cmp expected actual
 '
 
 test_expect_success 'setup more commits' '
diff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh
index 37143ea0ac..2ed479b712 100755
--- a/t/t5316-pack-delta-depth.sh
+++ b/t/t5316-pack-delta-depth.sh
@@ -82,12 +82,16 @@ test_expect_success 'packing produces a long delta' '
 	# Use --window=0 to make sure we are seeing reused deltas,
 	# not computing a new long chain.
 	pack=$(git pack-objects --all --window=0 </dev/null pack) &&
-	test 9 = "$(max_chain pack-$pack.pack)"
+	echo 9 >expect &&
+	max_chain pack-$pack.pack >actual &&
+	test_i18ncmp expect actual
 '
 
 test_expect_success '--depth limits depth' '
 	pack=$(git pack-objects --all --depth=5 </dev/null pack) &&
-	test 5 = "$(max_chain pack-$pack.pack)"
+	echo 5 >expect &&
+	max_chain pack-$pack.pack >actual &&
+	test_i18ncmp expect actual
 '
 
 test_done
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
index fd401ca605..99a8982ab1 100755
--- a/t/t6134-pathspec-in-submodule.sh
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -21,7 +21,7 @@ EOF
 test_expect_success 'error message for path inside submodule' '
 	echo a >sub/a &&
 	test_must_fail git add sub/a 2>actual &&
-	test_cmp expect actual
+	test_i18ncmp expect actual
 '
 
 cat <<EOF >expect
@@ -30,7 +30,7 @@ EOF
 
 test_expect_success 'error message for path inside submodule from within submodule' '
 	test_must_fail git -C sub add . 2>actual &&
-	test_cmp expect actual
+	test_i18ncmp expect actual
 '
 
 test_done
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index bb2e4d704d..0ef7b94394 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -87,7 +87,7 @@ test_expect_success 'creating a tag with --create-reflog should create reflog' '
 	git tag --create-reflog tag_with_reflog &&
 	git reflog exists refs/tags/tag_with_reflog &&
 	sed -e "s/^.*	//" .git/logs/refs/tags/tag_with_reflog >actual &&
-	test_cmp expected actual
+	test_i18ncmp expected actual
 '
 
 test_expect_success 'annotated tag with --create-reflog has correct message' '
@@ -98,7 +98,7 @@ test_expect_success 'annotated tag with --create-reflog has correct message' '
 	git tag -m "annotated tag" --create-reflog tag_with_reflog &&
 	git reflog exists refs/tags/tag_with_reflog &&
 	sed -e "s/^.*	//" .git/logs/refs/tags/tag_with_reflog >actual &&
-	test_cmp expected actual
+	test_i18ncmp expected actual
 '
 
 test_expect_success '--create-reflog does not create reflog on failure' '
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 4ac386d98b..034914a14f 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -447,7 +447,7 @@ test_expect_success 'submodule update - command run for initial population of su
 	EOF
 	rm -rf super/submodule &&
 	test_must_fail git -C super submodule update 2>actual &&
-	test_cmp expect actual &&
+	test_i18ncmp expect actual &&
 	git -C super submodule update --checkout
 '
 
diff --git a/t/t7509-commit.sh b/t/t7509-commit.sh
index db9774e345..ddef7ea6b0 100755
--- a/t/t7509-commit.sh
+++ b/t/t7509-commit.sh
@@ -101,7 +101,7 @@ test_expect_success '--amend option with empty author' '
 	echo "Empty author test" >>foo &&
 	test_tick &&
 	test_must_fail git commit -a -m "empty author" --amend 2>err &&
-	grep "empty ident" err
+	test_i18ngrep "empty ident" err
 '
 
 test_expect_success '--amend option with missing author' '
@@ -114,7 +114,7 @@ test_expect_success '--amend option with missing author' '
 	echo "Missing author test" >>foo &&
 	test_tick &&
 	test_must_fail git commit -a -m "malformed author" --amend 2>err &&
-	grep "empty ident" err
+	test_i18ngrep "empty ident" err
 '
 
 test_expect_success '--reset-author makes the commit ours even with --amend option' '
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 7f09867478..668bbee73c 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -25,14 +25,14 @@ prompt_given ()
 
 test_expect_success 'basic usage requires no repo' '
 	test_expect_code 129 git difftool -h >output &&
-	grep ^usage: output &&
+	test_i18ngrep ^usage: output &&
 	# create a ceiling directory to prevent Git from finding a repo
 	mkdir -p not/repo &&
 	test_when_finished rm -r not &&
 	test_expect_code 129 \
 	env GIT_CEILING_DIRECTORIES="$(pwd)/not" \
 	git -C not/repo difftool -h >output &&
-	grep ^usage: output
+	test_i18ngrep ^usage: output
 '
 
 # Create a file on master and change it on branch
-- 
2.11.0

Re: [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The GETTEXT_POISON=YesPlease compile-time testing option added in my
> bb946bba76 ("i18n: add GETTEXT_POISON to simulate unfriendly
> translator", 2011-02-22) has been slowly bitrotting as strings have
> been marked for translation, and new tests have been added without
> running it.
>
> I brought this up on the list ("[BUG] test suite broken with
> GETTEXT_POISON=YesPlease", [1]) asking whether this mode was useful at
> all anymore. At least one person occasionally uses it, and Lars
> Schneider offered to change one of the the Travis builds to run in
> this mode, so fix up the failing ones.
>
> My test setup runs most of the tests, with the notable exception of
> skipping all the p4 tests, so it's possible that there's still some
> lurking regressions I haven't fixed.
>
> 1. <CACBZZX62+acvi1dpkknadTL827mtCm_QesGSZ=6+UnyeMpg8+Q@mail.gmail.com>

To be honest, I feel quite uneasy about this patch.  It is no
brainer to take fixes like the one to 1309 where we used grep on a
localizable string to use test_i18ngrep instead---they are obviously
good changes.

But changes that skip tests with !GETTEXT_POISON look suspicious.

> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
> index 8937e25e49..2003ec7907 100755
> --- a/t/t1430-bad-ref-name.sh
> +++ b/t/t1430-bad-ref-name.sh
> @@ -122,7 +122,7 @@ test_expect_success 'push cannot create a badly named ref' '
>  	! grep -e "broken\.\.\.ref" output
>  '
>  
> -test_expect_failure 'push --mirror can delete badly named ref' '
> +test_expect_failure !GETTEXT_POISON 'push --mirror can delete badly named ref' '
>  	top=$(pwd) &&
>  	git init src &&
>  	git init dest &&

This test affects only src and dest repositories that does not seem
to be looked at by later tests, so skipping it is presumably safe.

I am guessing that the reason why this is skipped is because the
error message is looked at with "! grep ", not "grep", but by
skipping the entire test, we are not checking that "push" succeeds
(which should happen regardless of the locale).

It also is VERY curious that the test before this one (whose tail is
visible in the pre-context) does not need to be skipped.  Is that
because we can expect "broken...ref", which is litrally a part of a
refname, would be emitted intact in any locale (including the
poisoned one)?  If that is the case it is curous why this one needs
to be skipped.

> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> index 5778c0afe1..a428ae6703 100755
> --- a/t/t3203-branch-output.sh
> +++ b/t/t3203-branch-output.sh
> @@ -236,7 +236,7 @@ test_expect_success 'git branch --format option' '
>  	Refname is refs/heads/ref-to-remote
>  	EOF
>  	git branch --format="Refname is %(refname)" >actual &&
> -	test_cmp expect actual
> +	test_i18ncmp expect actual
>  '

This is a strange change.  Filling the placeholder in a format
string "Refname is %(refname)" should be affeced by i18n???

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 33d392ba11..e07d6d8cd2 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -366,7 +366,7 @@ test_expect_success 'verbose flag is heeded, even after --
> ...
> -test_expect_success 'multi-squash only fires up editor once' '
> +test_expect_success !GETTEXT_POISON 'multi-squash only fires up editor once' '
>  	base=$(git rev-parse HEAD~4) &&
>  	set_fake_editor &&
>  	FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 squash 2 squash 3 squash 4" \

This is also curious.  Is this because the poison locale does not substitute
anything passed to format template and literal strings like ONCE and
the instructions do not reach the edited file?

 (skipping many changes, not because I find nothing to comment on them)

> diff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh
> index 37143ea0ac..2ed479b712 100755
> --- a/t/t5316-pack-delta-depth.sh
> +++ b/t/t5316-pack-delta-depth.sh
> @@ -82,12 +82,16 @@ test_expect_success 'packing produces a long delta' '
>  	# Use --window=0 to make sure we are seeing reused deltas,
>  	# not computing a new long chain.
>  	pack=$(git pack-objects --all --window=0 </dev/null pack) &&
> -	test 9 = "$(max_chain pack-$pack.pack)"
> +	echo 9 >expect &&
> +	max_chain pack-$pack.pack >actual &&
> +	test_i18ncmp expect actual
>  '

This is also curious.  Why do we needto protect comparision with a
line whose contents is '9' from poison locale?  If the last one were
test_cmp I think this is a good change, by the way.

>  test_expect_success '--depth limits depth' '
>  	pack=$(git pack-objects --all --depth=5 </dev/null pack) &&
> -	test 5 = "$(max_chain pack-$pack.pack)"
> +	echo 5 >expect &&
> +	max_chain pack-$pack.pack >actual &&
> +	test_i18ncmp expect actual
>  '

Likewise.

Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease

[Junio C Hamano]

Michael J Gruber <git@grubix.eu> writes:

> Ævar Arnfjörð Bjarmason venit, vidit, dixit 20.04.2017 23:58:
>> As a refresh of everyone's memory (because mine needed it). This is a
>> feature I added back in 2011 when the i18n support was initially
>> added.
>> 
>> There was concern at the time that we would inadvertently mark
>> plumbing messages for translation, particularly something in a shared
>> code path, and this was a way to hopefully smoke out those issues with
>> the test suite.
>> 
>> However compiling with it breaks a couple of dozen tests, I stopped
>> digging when I saw some broke back in 2014.
>> 
>> What should be done about this? I think if we're going to keep them
>> they need to be run regularly by something like Travis (Lars CC'd),
>> however empirical evidence suggests that not running them is just fine
>> too, so should we just remove support for this test mode?
>> 
>> I don't care, but I can come up with the patch either way, but would
>> only be motivated to write the one-time fix for it if some CI system
>> is actually running them regularly, otherwise they'll just be subject
>> to bitrotting again.
>
> I use that switch when I change something that involves l10n, but
> usually I run specific tests only. To be honest: I have to make sure not
> to get confused by (nor forget one of) the build flag GETTEXT_POISON and
> the environment variable GIT_GETTEXT_POISON. I'm not sure I always
> tested what I meant to test...

To be quite honest, I have always felt that we are just as likely
inadvertently use test_i18ncmp when we should use test_cmp (and vice
versa) as we would mark plumbing messages with _() by mistake with
this approach, and even with constant monitoring by something like
Travis, GETTEXT_POISON may be able to catch mistakes only some of
the time (i.e. when we do not make mistakes in writing our tests).
Without constant monitoring, I agree that the mechanism does not
work well to catch our mistakes.

Re: [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 24, 2017 at 3:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> The GETTEXT_POISON=YesPlease compile-time testing option added in my
>> bb946bba76 ("i18n: add GETTEXT_POISON to simulate unfriendly
>> translator", 2011-02-22) has been slowly bitrotting as strings have
>> been marked for translation, and new tests have been added without
>> running it.
>>
>> I brought this up on the list ("[BUG] test suite broken with
>> GETTEXT_POISON=YesPlease", [1]) asking whether this mode was useful at
>> all anymore. At least one person occasionally uses it, and Lars
>> Schneider offered to change one of the the Travis builds to run in
>> this mode, so fix up the failing ones.
>>
>> My test setup runs most of the tests, with the notable exception of
>> skipping all the p4 tests, so it's possible that there's still some
>> lurking regressions I haven't fixed.
>>
>> 1. <CACBZZX62+acvi1dpkknadTL827mtCm_QesGSZ=6+UnyeMpg8+Q@mail.gmail.com>
>
> To be honest, I feel quite uneasy about this patch.  It is no
> brainer to take fixes like the one to 1309 where we used grep on a
> localizable string to use test_i18ngrep instead---they are obviously
> good changes.
>
> But changes that skip tests with !GETTEXT_POISON look suspicious.
>
>> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
>> index 8937e25e49..2003ec7907 100755
>> --- a/t/t1430-bad-ref-name.sh
>> +++ b/t/t1430-bad-ref-name.sh
>> @@ -122,7 +122,7 @@ test_expect_success 'push cannot create a badly named ref' '
>>       ! grep -e "broken\.\.\.ref" output
>>  '
>>
>> -test_expect_failure 'push --mirror can delete badly named ref' '
>> +test_expect_failure !GETTEXT_POISON 'push --mirror can delete badly named ref' '
>>       top=$(pwd) &&
>>       git init src &&
>>       git init dest &&
>
> This test affects only src and dest repositories that does not seem
> to be looked at by later tests, so skipping it is presumably safe.
>
> I am guessing that the reason why this is skipped is because the
> error message is looked at with "! grep ", not "grep", but by
> skipping the entire test, we are not checking that "push" succeeds
> (which should happen regardless of the locale).
>
> It also is VERY curious that the test before this one (whose tail is
> visible in the pre-context) does not need to be skipped.  Is that
> because we can expect "broken...ref", which is litrally a part of a
> refname, would be emitted intact in any locale (including the
> poisoned one)?  If that is the case it is curous why this one needs
> to be skipped.

This turns out to just be a mistake of mine that didn't need
!GETTEXT_POISON at all. Are you OK with fixing that up, or do you want
a v2 due to comments below...?

>> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
>> index 5778c0afe1..a428ae6703 100755
>> --- a/t/t3203-branch-output.sh
>> +++ b/t/t3203-branch-output.sh
>> @@ -236,7 +236,7 @@ test_expect_success 'git branch --format option' '
>>       Refname is refs/heads/ref-to-remote
>>       EOF
>>       git branch --format="Refname is %(refname)" >actual &&
>> -     test_cmp expect actual
>> +     test_i18ncmp expect actual
>>  '
>
> This is a strange change.  Filling the placeholder in a format
> string "Refname is %(refname)" should be affeced by i18n???

It's because %(refname) itself will get i18n'd:

    -Refname is (HEAD detached from fromtag)
    +Refname is # GETTEXT POISON #

See c8183cd285 ("branch: show more information when HEAD is detached",
2013-03-13).

>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index 33d392ba11..e07d6d8cd2 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -366,7 +366,7 @@ test_expect_success 'verbose flag is heeded, even after --
>> ...
>> -test_expect_success 'multi-squash only fires up editor once' '
>> +test_expect_success !GETTEXT_POISON 'multi-squash only fires up editor once' '
>>       base=$(git rev-parse HEAD~4) &&
>>       set_fake_editor &&
>>       FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 squash 2 squash 3 squash 4" \
>
> This is also curious.  Is this because the poison locale does not substitute
> anything passed to format template and literal strings like ONCE and
> the instructions do not reach the edited file?

I don't know the root cause for why that fails, I spent some time
re-digging into that for ~10m and didn't come up with anything.

We've marked many of these rebase messages in the past, it's usually
some stray newline being needed which the poison machinery didn't emit
or something like that.

My general approach when writing & maintaining this poison has been
that it's fine if we skip some tests, even though we could be bending
over backwards to run them, or even if we don't know the root cause
beyond "the rebase machinery is always broken with poison".

This is because once I'm satisfied that the breaking test isn't
because of some new plumbing message that got i18n'd I don't see the
point of keeping digging, it's fine to just skip the test, because we
run it when we're not under poison, and we're satisfied that it's not
breaking because of a new plumbing message being i18n'd we've
fulfilled the entire reason for why this poison facility exists in the
first place.

>  (skipping many changes, not because I find nothing to comment on them)
>
>> diff --git a/t/t5316-pack-delta-depth.sh b/t/t5316-pack-delta-depth.sh
>> index 37143ea0ac..2ed479b712 100755
>> --- a/t/t5316-pack-delta-depth.sh
>> +++ b/t/t5316-pack-delta-depth.sh
>> @@ -82,12 +82,16 @@ test_expect_success 'packing produces a long delta' '
>>       # Use --window=0 to make sure we are seeing reused deltas,
>>       # not computing a new long chain.
>>       pack=$(git pack-objects --all --window=0 </dev/null pack) &&
>> -     test 9 = "$(max_chain pack-$pack.pack)"
>> +     echo 9 >expect &&
>> +     max_chain pack-$pack.pack >actual &&
>> +     test_i18ncmp expect actual
>>  '
>
> This is also curious.  Why do we needto protect comparision with a
> line whose contents is '9' from poison locale?  If the last one were
> test_cmp I think this is a good change, by the way.
>
>>  test_expect_success '--depth limits depth' '
>>       pack=$(git pack-objects --all --depth=5 </dev/null pack) &&
>> -     test 5 = "$(max_chain pack-$pack.pack)"
>> +     echo 5 >expect &&
>> +     max_chain pack-$pack.pack >actual &&
>> +     test_i18ncmp expect actual
>>  '

The 9/5 value here is the number of lines that match /chain length/ as
determined my max_chain, that message is i18n'd.

Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease

[Duy Nguyen]

On Fri, Apr 21, 2017 at 9:54 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
>> Am 20.04.2017 um 23:58 schrieb Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
>>
>> As a refresh of everyone's memory (because mine needed it). This is a
>> feature I added back in 2011 when the i18n support was initially
>> added.
>>
>> There was concern at the time that we would inadvertently mark
>> plumbing messages for translation, particularly something in a shared
>> code path, and this was a way to hopefully smoke out those issues with
>> the test suite.
>>
>> However compiling with it breaks a couple of dozen tests, I stopped
>> digging when I saw some broke back in 2014.
>>
>> What should be done about this? I think if we're going to keep them
>> they need to be run regularly by something like Travis (Lars CC'd),
>> however empirical evidence suggests that not running them is just fine
>> too, so should we just remove support for this test mode?
>
> Right now we are building and testing Git in the following configurations:
>
> 1. Linux, gcc, stable Perforce an GitLFS version (used by git-p4 tests)
> 2. Linux, gcc, stable Perforce an GitLFS version (used by git-p4 tests) *
> 3. OSX, clang, latest Perforce an GitLFS version (used by git-p4 tests)
> 4. OSX, clang, latest Perforce an GitLFS version (used by git-p4 tests) *
> 5. Linux32, gcc, no git-p4 tests
> 6. Windows, gcc, no git-p4 tests
>
> 1-4 run the same tests right now. This was especially useful in the beginning to identify flaky tests (t0025 is still flaky!).
>
> We could easily run the tests in 1-4 with different configurations. E.g. enable GETTEXT_POISON in 2.

If CI stays idle some time, I suggest adding a test run with valgrind
on 'pu' or 'next'. If there's still spare capacity I would add another
test run with GIT_TEST_SPLIT_INDEX=1 (but not right now, it's broken)

Off topic, is it possible to receive mail notifications from Travis
when a fault is found in either 'pu', 'next' or 'master'? I know how
to do it in Jenkins, but I'm not familiar with Travis and there's no
obvious button from the web page..
-- 
Duy

Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease

[Jeff King]

On Mon, Apr 24, 2017 at 08:22:36PM +0700, Duy Nguyen wrote:

> Off topic, is it possible to receive mail notifications from Travis
> when a fault is found in either 'pu', 'next' or 'master'? I know how
> to do it in Jenkins, but I'm not familiar with Travis and there's no
> obvious button from the web page..

I looked into this a bit for my personal builds. Notification config has
to go into the .travis.yml file[1].  So I think the best we could do is
send a notification email to some mailing list, and then let people
subscribe to that (or it could go to git@vger; I don't know how noisy it
would be).

-Peff

[1] I wanted to set up an email just to me for my personal fork, but I
    couldn't make it work. AFAICT there's no way to override upstream's
    config short of adding another commit, which would mean I'd have to
    base all my branches on a "fix up .travis.yml" commit.

Re: [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> My general approach when writing & maintaining this poison has been
> that it's fine if we skip some tests, even though we could be bending
> over backwards to run them, or even if we don't know the root cause
> beyond "the rebase machinery is always broken with poison".
>
> This is because once I'm satisfied that the breaking test isn't
> because of some new plumbing message that got i18n'd I don't see the
> point of keeping digging, it's fine to just skip the test, because we
> run it when we're not under poison, and we're satisfied that it's not
> breaking because of a new plumbing message being i18n'd we've
> fulfilled the entire reason for why this poison facility exists in the
> first place.

As to skipping tests, I am worried mostly because it is very easy to
mark one test as skipped under poison build, even where the side
effect from that test left behind in the trash repository is a
prerequisite for a later test to succeed.  For example, a test that
creates a tag may be marked as skipped-under-poison.  Then a new
test that is added to such a test may want to do something using
that tag, and it will succeed in the usual test.  As most people do
not test poison build, when somebody notices that the new test fails
under poison build, it is unclear if the breakage is due to new i18n
issues or something else, like a missing prerequisite tag due to
skipping an earlier test.

Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease

[Lars Schneider]

> On 24 Apr 2017, at 22:37, Jeff King <peff@peff.net> wrote:
> 
> On Mon, Apr 24, 2017 at 08:22:36PM +0700, Duy Nguyen wrote:
> 
>> Off topic, is it possible to receive mail notifications from Travis
>> when a fault is found in either 'pu', 'next' or 'master'? I know how
>> to do it in Jenkins, but I'm not familiar with Travis and there's no
>> obvious button from the web page..
> 
> I looked into this a bit for my personal builds. Notification config has
> to go into the .travis.yml file[1].  So I think the best we could do is
> send a notification email to some mailing list, and then let people
> subscribe to that (or it could go to git@vger; I don't know how noisy it
> would be).

A separate mailing list sounds like a very good idea to me!
Maybe "git-builds@vger.kernel.org" or something?
What would it take to set something up like this?

- Lars

Re: [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 25, 2017 at 6:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> My general approach when writing & maintaining this poison has been
>> that it's fine if we skip some tests, even though we could be bending
>> over backwards to run them, or even if we don't know the root cause
>> beyond "the rebase machinery is always broken with poison".
>>
>> This is because once I'm satisfied that the breaking test isn't
>> because of some new plumbing message that got i18n'd I don't see the
>> point of keeping digging, it's fine to just skip the test, because we
>> run it when we're not under poison, and we're satisfied that it's not
>> breaking because of a new plumbing message being i18n'd we've
>> fulfilled the entire reason for why this poison facility exists in the
>> first place.
>
> As to skipping tests, I am worried mostly because it is very easy to
> mark one test as skipped under poison build, even where the side
> effect from that test left behind in the trash repository is a
> prerequisite for a later test to succeed.  For example, a test that
> creates a tag may be marked as skipped-under-poison.  Then a new
> test that is added to such a test may want to do something using
> that tag, and it will succeed in the usual test.  As most people do
> not test poison build, when somebody notices that the new test fails
> under poison build, it is unclear if the breakage is due to new i18n
> issues or something else, like a missing prerequisite tag due to
> skipping an earlier test.

Indeed, I've tried to be careful not to introduce bugs like that, but
in this skipped case the tests look completely stand-alone to me.

In any case, I like my other patch to just remove this whole thing better.

Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease

[Jeff King]

On Tue, Apr 25, 2017 at 10:51:20AM +0200, Lars Schneider wrote:

> >> Off topic, is it possible to receive mail notifications from Travis
> >> when a fault is found in either 'pu', 'next' or 'master'? I know how
> >> to do it in Jenkins, but I'm not familiar with Travis and there's no
> >> obvious button from the web page..
> > 
> > I looked into this a bit for my personal builds. Notification config has
> > to go into the .travis.yml file[1].  So I think the best we could do is
> > send a notification email to some mailing list, and then let people
> > subscribe to that (or it could go to git@vger; I don't know how noisy it
> > would be).
> 
> A separate mailing list sounds like a very good idea to me!
> Maybe "git-builds@vger.kernel.org" or something?
> What would it take to set something up like this?

I suspect that emailing the vger admins is the right place (or that they
can point us in the right direction, or tell us to get lost). The best
address is probably postmaster@vger.kernel.org.

(I resisted just cc-ing them here to see if other people had opinions on
just sending the output to the regular list).

-Peff

Re: [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Indeed, I've tried to be careful not to introduce bugs like that, but
> in this skipped case the tests look completely stand-alone to me.

Yes, the ones I commented on in the upthread looked like their side
effect were not felt in the later tests.

> In any case, I like my other patch to just remove this whole thing better.

Hmph, did I miss a patch message?

Re: [PATCH] tests: fix tests broken under GETTEXT_POISON=YesPlease

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 26, 2017 at 3:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Indeed, I've tried to be careful not to introduce bugs like that, but
>> in this skipped case the tests look completely stand-alone to me.
>
> Yes, the ones I commented on in the upthread looked like their side
> effect were not felt in the later tests.
>
>> In any case, I like my other patch to just remove this whole thing better.
>
> Hmph, did I miss a patch message?

I sent it to you & CC'd the list, but it looks like it didn't make it
on-list. It's "[PATCH] tests: remove the GETTEXT_POISON compile-time
option to test i18n marking"
(<20170424110434.27689-1-avarab@gmail.com>). It's 240K & 6k lines, so
perhaps the list is rejecting it, here's the patch:

https://raw.githubusercontent.com/avar/git-patches/master/remove-gettext-poison/0001-tests-remove-the-GETTEXT_POISON-compile-time-option-.patch

Re: [PATCH] tests: remove the GETTEXT_POISON compile-time option to test i18n marking

[Michael J Gruber]

[Turns out I still can't operate gmail's web interface. Sorry for the dupe.]

2017-04-24 13:04 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
> Remove the GETTEXT_POISON=YesPlease compile-time which turns all of
> git's LC_*=C output into strings like "# GETTEXT POISON #" instead of
> gettext(msgid).
>
> See commit bb946bba76 ("i18n: add GETTEXT_POISON to simulate
> unfriendly translator", 2011-02-22) for what this was originally
> intended for.
>
> This facility has been broken for quite a while and has been subjected
> to frequent bitrot. The initial idea behind it back when it was added
> in 2011 was to prevent the accidental translation of plumbing
> messages.
>
> This isn't a big concern anymore as git isn't mass-adding i18n
> messages for a newly developed i18n facility as it was back then,
> maintaining this facility incurs a burden, and in actuality this has
> often been broken long enough for potential plumbing messages to be
> translated & make their way into major releases anyway.
>
> Most of this patch consists of search/replacing the test suite for:
>
>     test_i18ngrep ! -> ! grep
>     test_i18ngrep   -> grep
>     test_i18ncmp    -> test_cmp
>
> 1. <AANLkTi=5MrU-JyeQ3UVNbVwzn-8FbstUXafgcQaLWXDB@mail.gmail.com>
>    (https://public-inbox.org/git/AANLkTi=5MrU-JyeQ3UVNbVwzn-8FbstUXafgcQaLWXDB@mail.gmail.com/)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> On Mon, Apr 24, 2017 at 3:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Michael J Gruber <git@grubix.eu> writes:
>>
>>> Ævar Arnfjörð Bjarmason venit, vidit, dixit 20.04.2017 23:58:
>>>> As a refresh of everyone's memory (because mine needed it). This is a
>>>> feature I added back in 2011 when the i18n support was initially
>>>> added.
>>>>
>>>> There was concern at the time that we would inadvertently mark
>>>> plumbing messages for translation, particularly something in a shared
>>>> code path, and this was a way to hopefully smoke out those issues with
>>>> the test suite.
>>>>
>>>> However compiling with it breaks a couple of dozen tests, I stopped
>>>> digging when I saw some broke back in 2014.
>>>>
>>>> What should be done about this? I think if we're going to keep them
>>>> they need to be run regularly by something like Travis (Lars CC'd),
>>>> however empirical evidence suggests that not running them is just fine
>>>> too, so should we just remove support for this test mode?
>>>>
>>>> I don't care, but I can come up with the patch either way, but would
>>>> only be motivated to write the one-time fix for it if some CI system
>>>> is actually running them regularly, otherwise they'll just be subject
>>>> to bitrotting again.
>>>
>>> I use that switch when I change something that involves l10n, but
>>> usually I run specific tests only. To be honest: I have to make sure not
>>> to get confused by (nor forget one of) the build flag GETTEXT_POISON and
>>> the environment variable GIT_GETTEXT_POISON. I'm not sure I always
>>> tested what I meant to test...
>>
>> To be quite honest, I have always felt that we are just as likely
>> inadvertently use test_i18ncmp when we should use test_cmp (and vice
>> versa) as we would mark plumbing messages with _() by mistake with
>> this approach, and even with constant monitoring by something like
>> Travis, GETTEXT_POISON may be able to catch mistakes only some of
>> the time (i.e. when we do not make mistakes in writing our tests).
>> Without constant monitoring, I agree that the mechanism does not
>> work well to catch our mistakes.
>
> Here's an alternate patch to just remove it entirely. I think we
> should apply this instead, the only reason I sent the patch to fix it
> up was because of Michael's comment that he was occasionally using it.

Yes, I think test_i18ngrep and test_i18ncmp gave the impression that
they are i18n-aware grep and cmp, whereas in fact they turned off
these tese test lines completely.
Combined with the fact that GETTEXT_POSON builds turned on
GIT_GETTEXT_POISON, this sounds somewhat dangerous - we test more
aspects of plumbing commands but turn off (some) tests for porcelain.

I'm still wondering wether we couldn't generate a test locale
automatically by mangling the english strings (but preserving format
specifiers). That way, tests that test porcelain output could require
LANG=C while others could run in the mangled locale. (tt_TT is taken,
though ;) )

Michael

Re: [PATCH] tests: remove the GETTEXT_POISON compile-time option to test i18n marking

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 26, 2017 at 11:17 AM, Michael J Gruber <git@grubix.eu> wrote:
> [Turns out I still can't operate gmail's web interface. Sorry for the dupe.]
>
> 2017-04-24 13:04 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
>> Remove the GETTEXT_POISON=YesPlease compile-time which turns all of
>> git's LC_*=C output into strings like "# GETTEXT POISON #" instead of
>> gettext(msgid).
>>
>> See commit bb946bba76 ("i18n: add GETTEXT_POISON to simulate
>> unfriendly translator", 2011-02-22) for what this was originally
>> intended for.
>>
>> This facility has been broken for quite a while and has been subjected
>> to frequent bitrot. The initial idea behind it back when it was added
>> in 2011 was to prevent the accidental translation of plumbing
>> messages.
>>
>> This isn't a big concern anymore as git isn't mass-adding i18n
>> messages for a newly developed i18n facility as it was back then,
>> maintaining this facility incurs a burden, and in actuality this has
>> often been broken long enough for potential plumbing messages to be
>> translated & make their way into major releases anyway.
>>
>> Most of this patch consists of search/replacing the test suite for:
>>
>>     test_i18ngrep ! -> ! grep
>>     test_i18ngrep   -> grep
>>     test_i18ncmp    -> test_cmp
>>
>> 1. <AANLkTi=5MrU-JyeQ3UVNbVwzn-8FbstUXafgcQaLWXDB@mail.gmail.com>
>>    (https://public-inbox.org/git/AANLkTi=5MrU-JyeQ3UVNbVwzn-8FbstUXafgcQaLWXDB@mail.gmail.com/)
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>
>> On Mon, Apr 24, 2017 at 3:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Michael J Gruber <git@grubix.eu> writes:
>>>
>>>> Ævar Arnfjörð Bjarmason venit, vidit, dixit 20.04.2017 23:58:
>>>>> As a refresh of everyone's memory (because mine needed it). This is a
>>>>> feature I added back in 2011 when the i18n support was initially
>>>>> added.
>>>>>
>>>>> There was concern at the time that we would inadvertently mark
>>>>> plumbing messages for translation, particularly something in a shared
>>>>> code path, and this was a way to hopefully smoke out those issues with
>>>>> the test suite.
>>>>>
>>>>> However compiling with it breaks a couple of dozen tests, I stopped
>>>>> digging when I saw some broke back in 2014.
>>>>>
>>>>> What should be done about this? I think if we're going to keep them
>>>>> they need to be run regularly by something like Travis (Lars CC'd),
>>>>> however empirical evidence suggests that not running them is just fine
>>>>> too, so should we just remove support for this test mode?
>>>>>
>>>>> I don't care, but I can come up with the patch either way, but would
>>>>> only be motivated to write the one-time fix for it if some CI system
>>>>> is actually running them regularly, otherwise they'll just be subject
>>>>> to bitrotting again.
>>>>
>>>> I use that switch when I change something that involves l10n, but
>>>> usually I run specific tests only. To be honest: I have to make sure not
>>>> to get confused by (nor forget one of) the build flag GETTEXT_POISON and
>>>> the environment variable GIT_GETTEXT_POISON. I'm not sure I always
>>>> tested what I meant to test...
>>>
>>> To be quite honest, I have always felt that we are just as likely
>>> inadvertently use test_i18ncmp when we should use test_cmp (and vice
>>> versa) as we would mark plumbing messages with _() by mistake with
>>> this approach, and even with constant monitoring by something like
>>> Travis, GETTEXT_POISON may be able to catch mistakes only some of
>>> the time (i.e. when we do not make mistakes in writing our tests).
>>> Without constant monitoring, I agree that the mechanism does not
>>> work well to catch our mistakes.
>>
>> Here's an alternate patch to just remove it entirely. I think we
>> should apply this instead, the only reason I sent the patch to fix it
>> up was because of Michael's comment that he was occasionally using it.
>
> Yes, I think test_i18ngrep and test_i18ncmp gave the impression that
> they are i18n-aware grep and cmp, whereas in fact they turned off
> these tese test lines completely.
> Combined with the fact that GETTEXT_POSON builds turned on
> GIT_GETTEXT_POISON, this sounds somewhat dangerous - we test more
> aspects of plumbing commands but turn off (some) tests for porcelain.

It gives us no less test coverage because we still run the tests
without GETTEXT_POSON. Thus running first without that & then with
gives 100% coverage.

The entire point of the GETTEXT_POSON mode is that we've already
manually gone over things that broke in the past, and are skipping
them with C_LOCALE_OUTPUT or one of these i18n functions, and when
adding new translations we keep an eye out for new breakages that
haven't been marked yet.

These are then a candidate either for marking to skip, if they're
porcelain commands using our new gettext()-utilizing code, or we've
made a mistake and translated some plumbing, in which case that would
hopefully be caught by list review or Travis.

I still think it's better to just get rid of it, but this aspect of it
is working exactly as intended.

> I'm still wondering wether we couldn't generate a test locale
> automatically by mangling the english strings (but preserving format
> specifiers). That way, tests that test porcelain output could require
> LANG=C while others could run in the mangled locale. (tt_TT is taken,
> though ;) )

If we wanted to keep GETTEXT_POSON this would be going further in the
wrong direction. The reason the message starts with a "#", has no
trailing \n & most importantly ignores any format specifiers the real
translation would have is exactly because we'd like as many things as
can break break under this mode.

If anything we should consider changing "# GETTEXT_POISON #" to
"<random binary garbage>" if we keep it.

Including format specifiers would just reduce the set of things that
would break, consider e.g. a test that merely greps for some substring
that would be emitted by a "%s branch %d commits ahead" format, now we
change that to "%s GIBBER %d ISH", but the test still passes because
it just does $(awk '{print $1}'), but it might be a plumbing message
that could be translated as e.g. "%d WORD1 %s WORD2 WORD3", breaking
the API.

Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease

[Lars Schneider]

> On 25 Apr 2017, at 12:01, Jeff King <peff@peff.net> wrote:
> 
> On Tue, Apr 25, 2017 at 10:51:20AM +0200, Lars Schneider wrote:
> 
>>>> Off topic, is it possible to receive mail notifications from Travis
>>>> when a fault is found in either 'pu', 'next' or 'master'? I know how
>>>> to do it in Jenkins, but I'm not familiar with Travis and there's no
>>>> obvious button from the web page..
>>> 
>>> I looked into this a bit for my personal builds. Notification config has
>>> to go into the .travis.yml file[1].  So I think the best we could do is
>>> send a notification email to some mailing list, and then let people
>>> subscribe to that (or it could go to git@vger; I don't know how noisy it
>>> would be).
>> 
>> A separate mailing list sounds like a very good idea to me!
>> Maybe "git-builds@vger.kernel.org" or something?
>> What would it take to set something up like this?
> 
> I suspect that emailing the vger admins is the right place (or that they
> can point us in the right direction, or tell us to get lost). The best
> address is probably postmaster@vger.kernel.org.
> 
> (I resisted just cc-ing them here to see if other people had opinions on
> just sending the output to the regular list).

OK, I'll send them an email. I just realized a strong reason for a
separate mailing list: I haven't found a knob to make TravisCI
send plain text emails.

- Lars

Re: [BUG] test suite broken with GETTEXT_POISON=YesPlease

[Jeff King]

On Fri, Apr 28, 2017 at 10:42:10AM +0200, Lars Schneider wrote:

> >> A separate mailing list sounds like a very good idea to me!
> >> Maybe "git-builds@vger.kernel.org" or something?
> >> What would it take to set something up like this?
> > 
> > I suspect that emailing the vger admins is the right place (or that they
> > can point us in the right direction, or tell us to get lost). The best
> > address is probably postmaster@vger.kernel.org.
> > 
> > (I resisted just cc-ing them here to see if other people had opinions on
> > just sending the output to the regular list).
> 
> OK, I'll send them an email. I just realized a strong reason for a
> separate mailing list: I haven't found a knob to make TravisCI
> send plain text emails.

I think that's going to be a problem for any list on vger. I suspect the
spam filtering all happens at an early stage.

You might consider just setting up a google group or some other list as
an initial experiment. It wouldn't be that hard to switch the list
later (people will need to resubscribe, but you can notify them by
sending an email to...the list).

-Peff