Re: [PATCH 2/2] Add C_LOCALE_OUTPUT prereq to test cases that require English text matching

[Jonathan Nieder <jrnieder@gmail.com>]

Nguyễn Thái Ngọc Duy wrote:

> This fixes all GETTEXT_POISON breakages caused by recent i18n changes.

First, thanks much for this.

Lots of these could be fixed in a more targetted way by using
test_i18ngrep, but the C_LOCALE_OUTPUT prereq works just as well as a
way to double-check that the newly translated strings are hopefully
not disrupting any functionality people rely on.

[...]
> --- a/t/t0006-date.sh
> +++ b/t/t0006-date.sh
> @@ -9,7 +9,7 @@ TEST_DATE_NOW=1251660000; export TEST_DATE_NOW
>  check_show() {
>  	t=$(($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-date show $t >actual &&
>  	test_cmp expect actual
>  	"

Could use test_i18ncmp so we catch if test-date crashes, but anyway,
yeah, we can't expect to be able to meaningfully test date formatting
in another language.


> @@ -29,7 +29,7 @@ check_show 62985600 '2 years ago'
>  
>  check_parse() {
>  	echo "$1 -> $2" >expect
> -	test_expect_${4:-success} "parse date ($1${3:+ TZ=$3})" "
> +	test_expect_${4:-success} C_LOCALE_OUTPUT "parse date ($1${3:+ TZ=$3})" "
>  	TZ=${3:-$TZ} test-date parse '$1' >actual &&
>  	test_cmp expect actual
>  	"

This one is less pleasant.  Could test-date be convinced to produce
machine-readable output (e.g., seconds since the epoque)?  I'd be
especially concerned to check that the parser still accepts the
same input strings in other locales.

> @@ -50,7 +50,7 @@ check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5
>  
>  check_approxidate() {
>  	echo "$1 -> $2 +0000" >expect
> -	test_expect_${3:-success} "parse approxidate ($1)" "
> +	test_expect_${3:-success} C_LOCALE_OUTPUT "parse approxidate ($1)" "
>  	test-date approxidate '$1' >actual &&
>  	test_cmp expect actual
>  	"

Likewise.

[...]
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -48,7 +48,7 @@ Standard options
>  
>  EOF
>  
> -test_expect_success 'test help' '
> +test_expect_success C_LOCALE_OUTPUT 'test help' '
>  	test_must_fail test-parse-options -h > output 2> output.err &&
>  	test ! -s output.err &&
>  	test_cmp expect output

Sensible.

> @@ -104,8 +104,8 @@ test_expect_success 'OPT_BOOL() is idempotent #2' 'check boolean: 1 -DB'
>  test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes'
>  test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D --no-no-doubt'
>  
> -test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown --fear'
> -test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown --no-no-fear'
> +test_expect_success C_LOCALE_OUTPUT 'OPT_BOOL() no negation #1' 'check_unknown --fear'
> +test_expect_success C_LOCALE_OUTPUT 'OPT_BOOL() no negation #2' 'check_unknown --no-no-fear'

Simpler to do

      --- i/t/t0040-parse-options.sh
      +++ w/t/t0040-parse-options.sh
      @@ -89,7 +89,7 @@ check_unknown() {
	      cat expect.err >>expect &&
	      test_must_fail test-parse-options $* >output 2>output.err &&
	      test ! -s output &&
      -	test_cmp expect output.err
      +	test_i18ncmp expect output.err
       }
       
       test_expect_success 'OPT_BOOL() #1' 'check boolean: 1 --yes'

[...]
> @@ -308,7 +308,7 @@ cat > expect <<EOF
>  Callback: "not set", 1
>  EOF
>  
> -test_expect_success 'OPT_CALLBACK() and callback errors work' '
> +test_expect_success C_LOCALE_OUTPUT 'OPT_CALLBACK() and callback errors work' '
>  	test_must_fail test-parse-options --no-length > output 2> output.err &&
>  	test_cmp expect output &&
>  	test_cmp expect.err output.err

Using test_i18ncmp on stderr would allow us to keep checking stdout.

[...]
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -389,7 +389,7 @@ test_expect_success 'get bool variable with empty value' \
>  	'git config --bool emptyvalue.variable > output &&
>  	 cmp output expect'
>  
> -test_expect_success 'no arguments, but no crash' '
> +test_expect_success C_LOCALE_OUTPUT 'no arguments, but no crash' '
>  	test_must_fail git config >output 2>&1 &&
>  	grep usage output
>  '

It's mostly about "no crash", so test_i18ngrep would be more
comforting.

[...]
> --- a/t/t1502-rev-parse-parseopt.sh
> +++ b/t/t1502-rev-parse-parseopt.sh
> @@ -39,7 +39,7 @@ Extras
>  extra1    line above used to cause a segfault but no longer does
>  EOF
>  
> -test_expect_success 'test --parseopt help output' '
> +test_expect_success C_LOCALE_OUTPUT 'test --parseopt help output' '
>  	test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec &&
>  	test_cmp expect output
>  '

Seems fine (though test_i18ncmp is more targetted).

> --- a/t/t2006-checkout-index-basic.sh
> +++ b/t/t2006-checkout-index-basic.sh
> @@ -5,12 +5,12 @@ test_description='basic checkout-index tests
>  
>  . ./test-lib.sh
>  
> -test_expect_success 'checkout-index --gobbledegook' '
> +test_expect_success C_LOCALE_OUTPUT 'checkout-index --gobbledegook' '
[...]
> -test_expect_success 'checkout-index -h in broken repository' '
> +test_expect_success C_LOCALE_OUTPUT 'checkout-index -h in broken repository' '
[...]
> +++ b/t/t2107-update-index-basic.sh
[...]
> -test_expect_success 'update-index --nonsense dumps usage' '
> +test_expect_success C_LOCALE_OUTPUT 'update-index --nonsense dumps usage' '
[...]
> -test_expect_success 'update-index -h with corrupt index' '
> +test_expect_success C_LOCALE_OUTPUT 'update-index -h with corrupt index' '
[...]
> +++ b/t/t3004-ls-files-basic.sh
[...]
> -test_expect_success 'ls-files with nonsense option' '
> +test_expect_success C_LOCALE_OUTPUT 'ls-files with nonsense option' '
[...]
> -test_expect_success 'ls-files -h in corrupt repository' '
> +test_expect_success C_LOCALE_OUTPUT 'ls-files -h in corrupt repository' '
[...]
> +++ b/t/t3200-branch.sh
[...]
> -test_expect_success 'branch -h in broken repository' '
> +test_expect_success C_LOCALE_OUTPUT 'branch -h in broken repository' '
[...]
> -test_expect_success \
> +test_expect_success C_LOCALE_OUTPUT \
>      'git branch -m dumps usage' \
[...]
> +++ b/t/t3501-revert-cherry-pick.sh
[...]
> -test_expect_success 'cherry-pick --nonsense' '
> +test_expect_success C_LOCALE_OUTPUT 'cherry-pick --nonsense' '
[...]
> -test_expect_success 'revert --nonsense' '
> +test_expect_success C_LOCALE_OUTPUT 'revert --nonsense' '

Likewise.

> --- a/t/t4006-diff-mode.sh
> +++ b/t/t4006-diff-mode.sh
> @@ -32,26 +32,26 @@ test_expect_success 'prepare binary file' '
>  	git commit -m binbin
>  '
>  
> -test_expect_success '--stat output after text chmod' '
> +test_expect_success C_LOCALE_OUTPUT '--stat output after text chmod' '
>  	test_chmod -x rezrov &&
>  	echo " 0 files changed" >expect &&
>  	git diff HEAD --stat >actual &&
>  	test_cmp expect actual
>  '

Yeah, sensible enough.

It would be possible to recover some of the test by adding another
that does --numstat.

[...]
> --- a/t/t4012-diff-binary.sh
> +++ b/t/t4012-diff-binary.sh
> @@ -62,7 +62,7 @@ test_expect_success 'apply --numstat understands diff --binary format' '
>  
>  # apply needs to be able to skip the binary material correctly
>  # in order to report the line number of a corrupt patch.
> -test_expect_success 'apply detecting corrupt patch correctly' \
> +test_expect_success C_LOCALE_OUTPUT 'apply detecting corrupt patch correctly' \
>  	'git diff | sed -e 's/-CIT/xCIT/' >broken &&
>  	 if git apply --stat --summary broken 2>detected
>  	 then

This is an old one and uses a weird style.  I guess I'd go for

	git diff >patch &&
	sed -e "s/-CIT/xCIT/" <patch >broken &&
	test_must_fail git apply --stat --summary broken 2>detected &&

	if test_have_prereq C_LOCALE_OUTPUT
	then
		l=$(sed -n "s/.*fatal.*at line \([0-9]*\).*/\1/p" <detected) &&
		echo xCIT >expect &&
		sed -n "$l p" <broken >actual &&
		test_cmp expect actual
	fi

A translator is free to use %Id for the line number, so it's hard to
parse this in an arbitrary locale.

[...]
> -test_expect_success 'apply detecting corrupt patch correctly' \
> +test_expect_success C_LOCALE_OUTPUT 'apply detecting corrupt patch correctly' \

Likewise.

[...]
> --- a/t/t4120-apply-popt.sh
> +++ b/t/t4120-apply-popt.sh
[...]
> -test_expect_success 'apply with too large -p' '
> +test_expect_success C_LOCALE_OUTPUT 'apply with too large -p' '
>  	cp file1.saved file1 &&
>  	test_must_fail git apply --stat -p3 patch.file 2>err &&
>  	grep "removing 3 leading" err

It's mostly about making sure "git apply" fails gracefully and the
functionality involves parsing (which can get screwed up by locale
settings), so keeping the test running in other locales using
test_i18ngrep would be a comfort.  

[...]
> -test_expect_success 'apply with too large -p and fancy filename' '
> +test_expect_success C_LOCALE_OUTPUT 'apply with too large -p and fancy filename' '

Likewise.

> --- a/t/t4133-apply-filenames.sh
> +++ b/t/t4133-apply-filenames.sh
> @@ -28,7 +28,7 @@ index d00491f..0000000
[...]
> -test_expect_success 'apply diff with inconsistent filenames in headers' '
> +test_expect_success C_LOCALE_OUTPUT 'apply diff with inconsistent filenames in headers' '
>  	test_must_fail git apply bad1.patch 2>err &&
>  	grep "inconsistent new filename" err &&
>  	test_must_fail git apply bad2.patch 2>err &&
[...]
> +++ b/t/t4200-rerere.sh
[...]
> +test_expect_success C_LOCALE_OUTPUT 'rerere --no-no-rerere-autoupdate' '
[...]
> +test_expect_success C_LOCALE_OUTPUT 'rerere -h' '
[...]
> +++ b/t/t4202-log.sh
[...]
> +test_expect_success C_LOCALE_OUTPUT 'log --graph with diff and stats' '

Seems fine.

> --- a/t/t5300-pack-object.sh
> +++ b/t/t5300-pack-object.sh
> @@ -413,7 +413,7 @@ test_expect_success \
[...]
> -test_expect_success \
> +test_expect_success C_LOCALE_OUTPUT \
>      'make sure index-pack detects the SHA1 collision' \
>      'test_must_fail git index-pack -o bad.idx test-3.pack 2>msg &&
>       grep "SHA1 COLLISION FOUND" msg'

I'd drop the grep, personally.

[...]
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -24,7 +24,7 @@ setup_repository () {
>  tokens_match () {
>  	echo "$1" | tr ' ' '\012' | sort | sed -e '/^$/d' >expect &&
>  	echo "$2" | tr ' ' '\012' | sort | sed -e '/^$/d' >actual &&
> -	test_cmp expect actual
> +	test_i18ncmp expect actual

Sensible.

[...]
> @@ -694,7 +694,7 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
>  	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
>  '
>  
> -test_expect_success 'remote prune to cause a dangling symref' '
> +test_expect_success C_LOCALE_OUTPUT 'remote prune to cause a dangling symref' '
>  	git clone one seven &&

test_i18ngrep message

and

test_i18ngrep ! message

work well for this kind of thing.

[...]
> --- a/t/t6500-gc.sh
> +++ b/t/t6500-gc.sh
[...]
> +test_expect_success C_LOCALE_OUTPUT 'gc --gobbledegook' '
[...]
> +test_expect_success C_LOCALE_OUTPUT 'gc -h with invalid configuration' '
[...]
> +++ b/t/t7508-status.sh
[...]
> +test_expect_success C_LOCALE_OUTPUT 'status --column' '
[...]
> +test_expect_success C_LOCALE_OUTPUT 'merge -h with invalid index' '

Sane.

Hope that helps,
Jonathan