Re: [PATCH v3 1/2] pretty: Add failing tests: user format ignores i18n.logOutputEncoding setting

[Junio C Hamano <gitster@pobox.com>]

Alexey Shumkin <zapped@mail.ru> writes:

> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
> ...
> @@ -48,28 +53,25 @@ head2=$(add_file sm1 foo3)
>  
>  test_expect_success 'modified submodule(forward)' "

As this is [PATCH 1/2], doesn't this patch make this test fail, calling
for test_expect_failure here (and then later in 2/2 to be flipped back to
test_expect_success)?

>  	git diff-index -p --submodule=log HEAD >actual &&
> -	cat >expected <<-EOF &&
> -Submodule sm1 $head1..$head2:
> -  > Add foo3
> -EOF
> +	printf \"Submodule sm1 $head1..$head2:\n\
> +  > Add foo3 ($added foo3)\n\
> +\" > expected &&
>  	test_cmp expected actual
>  "

Hmmm... why printf?  Is it worth to force reviewers wonder what would
happen if any of these $variables happened to have "%" in them?  Are you
benefiting from any printf features that you cannot achieve with the
original cat?

This hunk and others throughout your patch that change cat with here doc
into printf seem to make the tests less legible, at least to me.

Perhaps like this instead, if the "flushed left" of the original looked
ugly to your eyes?

@@ -49,27 +54,25 @@ head2=$(add_file sm1 foo3)
 test_expect_success 'modified submodule(forward)' "
 	git diff-index -p --submodule=log HEAD >actual &&
 	cat >expected <<-EOF &&
-Submodule sm1 $head1..$head2:
-  > Add foo3
-EOF
+	Submodule sm1 $head1..$head2:
+	  > Add foo3 ($added foo3)
+	EOF
 	test_cmp expected actual
 "