[PATCH 0/6] Cleaning up t4012

[PATCH 0/6] Cleaning up t4012

[Alexander Strasser]

Hi,

  during my recent bug hunting expedition I had the honor to modify
t/t4012-diff-binary.sh . After submitting my previous patch set it
quickly became clear that this file does not adhere to currently
practiced style of Git tests.

  It took me some time to get back to this but here finally comes a
patch set to modernize t4012. While this patch set did not confront
me with great conceptual problems it still is a bit problematic and
depends to some point on personal preferences. That is part of the
reason why I tried to make as small and focused commits as possible.
(Other part is I find it easier to review/manage my changes for myself
with a finer commit granularity). The patches can still be squashed
together after review if this is preferred.

  Also thanks again to Zbigniew who pointed out most of the addressed
issues.

Alexander Strasser (6):
  t4012: Use different style for quoting
  t4012: Unquote git command fragment in test title
  t4012: Actually quote the sed script
  t4012: Break up pipe into serial redirections
  t4012: Re-indent test snippets
  t4012: Make --shortstat more robust

 t/t4012-diff-binary.sh | 92 +++++++++++++++++++++++++++-----------------------
 1 file changed, 49 insertions(+), 43 deletions(-)

-- 
1.7.10.2.552.gaa3bb87

[PATCH 7/6?] t4012: use 'printf' instead of 'dd' to generate a binary file

[Johannes Sixt]

From: Johannes Sixt <j6t@kdbg.org>

For some reason, 'echo X | dd bs=1k seek=1' creates a file with 2050 bytes
on Windows instead of the expected 1026 bytes, so that a test fails. Since
the actual contents of the file are irrelevant as long as there is at
least one zero byte so that the diff machinery recognizes it as binary,
use printf to generate it.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 While the focus is on t4012, maybe you can add this patch to the series.

 t/t4012-diff-binary.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
index 6cebb39..8c018ab 100755
--- a/t/t4012-diff-binary.sh
+++ b/t/t4012-diff-binary.sh
@@ -125,7 +125,7 @@ cat >expect <<EOF
 EOF
 
 test_expect_success 'diff --stat with binary files and big change count' '
-	echo X | dd of=binfile bs=1k seek=1 &&
+	printf "\01\00%1024d" 1 >binfile &&
 	git add binfile &&
 	i=0 &&
 	while test $i -lt 10000; do
-- 
1.7.11.1.1304.g11834c6

Re: [PATCH 7/6?] t4012: use 'printf' instead of 'dd' to generate a binary file

[Alexander Strasser]

Hi, 

Johannes Sixt wrote:
> From: Johannes Sixt <j6t@kdbg.org>
> 
> For some reason, 'echo X | dd bs=1k seek=1' creates a file with 2050 bytes
> on Windows instead of the expected 1026 bytes, so that a test fails. Since
> the actual contents of the file are irrelevant as long as there is at
> least one zero byte so that the diff machinery recognizes it as binary,
> use printf to generate it.
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  While the focus is on t4012, maybe you can add this patch to the series.

  Your patch looks good to me and works here. If I hear no
objections I will include it as number 7 when resending this
series.

  Alexander

>  t/t4012-diff-binary.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
> index 6cebb39..8c018ab 100755
> --- a/t/t4012-diff-binary.sh
> +++ b/t/t4012-diff-binary.sh
> @@ -125,7 +125,7 @@ cat >expect <<EOF
>  EOF
>  
>  test_expect_success 'diff --stat with binary files and big change count' '
> -	echo X | dd of=binfile bs=1k seek=1 &&
> +	printf "\01\00%1024d" 1 >binfile &&
>  	git add binfile &&
>  	i=0 &&
>  	while test $i -lt 10000; do
> -- 
> 1.7.11.1.1304.g11834c6

[PATCH v2 0/8] Cleaning up t4012

[Alexander Strasser]

Hi,

  here goes the reroll. I corrected the spelling errors pointed out
by Zbigniew and appended another patch for replacing the if-else
"Did the command fail?" constructs with test_must_fail.

  I based this series on the as/t4012-style-updates branch pushed by
Junio. So his fixes are included too.


Alexander Strasser (7):
  t4012: modernize style for quoting
  t4012: Unquote git command fragment in test title
  t4012: Actually quote the sed script
  t4012: Break up pipe into serial redirections
  t4012: Make --shortstat test more robust
  t4012: Re-indent test snippets
  t4012: Use test_must_fail instead of if-else

Johannes Sixt (1):
  t4012: use 'printf' instead of 'dd' to generate a binary file

 t/t4012-diff-binary.sh | 94 +++++++++++++++++++++++---------------------------
 1 file changed, 44 insertions(+), 50 deletions(-)

-- 
1.7.10.2.552.gaa3bb87

[PATCH v2 1/8] t4012: modernize style for quoting

[Alexander Strasser]

This quoting style is used by all newly added test code.

Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4012-diff-binary.sh | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
index 6cebb39..2a8b68e 100755
--- a/t/t4012-diff-binary.sh
+++ b/t/t4012-diff-binary.sh
@@ -15,13 +15,14 @@ cat >expect.binary-numstat <<\EOF
 -	-	d
 EOF
 
-test_expect_success 'prepare repository' \
-	'echo AIT >a && echo BIT >b && echo CIT >c && echo DIT >d &&
+test_expect_success 'prepare repository' '
+	 echo AIT >a && echo BIT >b && echo CIT >c && echo DIT >d &&
 	 git update-index --add a b c d &&
 	 echo git >a &&
 	 cat "$TEST_DIRECTORY"/test-binary-1.png >b &&
 	 echo git >c &&
-	 cat b b >d'
+	 cat b b >d
+'
 
 cat > expected <<\EOF
  a |    2 +-
@@ -62,8 +63,8 @@ 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' \
-	'git diff | sed -e 's/-CIT/xCIT/' >broken &&
+test_expect_success 'apply detecting corrupt patch correctly' '
+	 git diff | sed -e 's/-CIT/xCIT/' >broken &&
 	 if git apply --stat --summary broken 2>detected
 	 then
 		echo unhappy - should have detected an error
@@ -74,10 +75,11 @@ test_expect_success 'apply detecting corrupt patch correctly' \
 	 detected=`cat detected` &&
 	 detected=`expr "$detected" : "fatal.*at line \\([0-9]*\\)\$"` &&
 	 detected=`sed -ne "${detected}p" broken` &&
-	 test "$detected" = xCIT'
+	 test "$detected" = xCIT
+'
 
-test_expect_success 'apply detecting corrupt patch correctly' \
-	'git diff --binary | sed -e 's/-CIT/xCIT/' >broken &&
+test_expect_success 'apply detecting corrupt patch correctly' '
+	 git diff --binary | sed -e 's/-CIT/xCIT/' >broken &&
 	 if git apply --stat --summary broken 2>detected
 	 then
 		echo unhappy - should have detected an error
@@ -88,23 +90,26 @@ test_expect_success 'apply detecting corrupt patch correctly' \
 	 detected=`cat detected` &&
 	 detected=`expr "$detected" : "fatal.*at line \\([0-9]*\\)\$"` &&
 	 detected=`sed -ne "${detected}p" broken` &&
-	 test "$detected" = xCIT'
+	 test "$detected" = xCIT
+'
 
 test_expect_success 'initial commit' 'git commit -a -m initial'
 
 # Try removal (b), modification (d), and creation (e).
-test_expect_success 'diff-index with --binary' \
-	'echo AIT >a && mv b e && echo CIT >c && cat e >d &&
+test_expect_success 'diff-index with --binary' '
+	 echo AIT >a && mv b e && echo CIT >c && cat e >d &&
 	 git update-index --add --remove a b c d e &&
 	 tree0=`git write-tree` &&
 	 git diff --cached --binary >current &&
-	 git apply --stat --summary current'
+	 git apply --stat --summary current
+'
 
-test_expect_success 'apply binary patch' \
-	'git reset --hard &&
+test_expect_success 'apply binary patch' '
+	 git reset --hard &&
 	 git apply --binary --index <current &&
 	 tree1=`git write-tree` &&
-	 test "$tree1" = "$tree0"'
+	 test "$tree1" = "$tree0"
+'
 
 test_expect_success 'diff --no-index with binary creation' '
 	echo Q | q_to_nul >binary &&
-- 
1.7.10.2.552.gaa3bb87

[PATCH v2 2/8] t4012: Unquote git command fragment in test title

[Alexander Strasser]

The command fragments are quoted nowhere else in title texts of
this file, thus make this one consistent with all other titles.

Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4012-diff-binary.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
index 2a8b68e..3c54269 100755
--- a/t/t4012-diff-binary.sh
+++ b/t/t4012-diff-binary.sh
@@ -31,7 +31,7 @@ cat > expected <<\EOF
  d |  Bin
  4 files changed, 2 insertions(+), 2 deletions(-)
 EOF
-test_expect_success '"apply --stat" output for binary file change' '
+test_expect_success 'apply --stat output for binary file change' '
 	git diff >diff &&
 	git apply --stat --summary <diff >current &&
 	test_i18ncmp expected current
-- 
1.7.10.2.552.gaa3bb87

[PATCH v2 3/8] t4012: Actually quote the sed script

[Alexander Strasser]

The nested quoting is not needed in this cases, thus the previous
version did work just fine. Nevertheless the usage is misleading,
so just achieve nested quoting by using double quotes instead. Lower
the probability of breakage in the future and make the code easier
to read.

NOTE: Just dropping the single quotes around the sed arguments would
      have also been possible.

Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4012-diff-binary.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
index 3c54269..60c2f6c 100755
--- a/t/t4012-diff-binary.sh
+++ b/t/t4012-diff-binary.sh
@@ -64,7 +64,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' '
-	 git diff | sed -e 's/-CIT/xCIT/' >broken &&
+	 git diff | sed -e "s/-CIT/xCIT/" >broken &&
 	 if git apply --stat --summary broken 2>detected
 	 then
 		echo unhappy - should have detected an error
@@ -79,7 +79,7 @@ test_expect_success 'apply detecting corrupt patch correctly' '
 '
 
 test_expect_success 'apply detecting corrupt patch correctly' '
-	 git diff --binary | sed -e 's/-CIT/xCIT/' >broken &&
+	 git diff --binary | sed -e "s/-CIT/xCIT/" >broken &&
 	 if git apply --stat --summary broken 2>detected
 	 then
 		echo unhappy - should have detected an error
-- 
1.7.10.2.552.gaa3bb87

[PATCH v2 4/8] t4012: Break up pipe into serial redirections

[Alexander Strasser]

Do not hide possible git errors by masking its process
exit status.

Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4012-diff-binary.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
index 60c2f6c..daf8234 100755
--- a/t/t4012-diff-binary.sh
+++ b/t/t4012-diff-binary.sh
@@ -64,7 +64,8 @@ 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' '
-	 git diff | sed -e "s/-CIT/xCIT/" >broken &&
+	 git diff >output &&
+	 sed -e "s/-CIT/xCIT/" <output >broken &&
 	 if git apply --stat --summary broken 2>detected
 	 then
 		echo unhappy - should have detected an error
-- 
1.7.10.2.552.gaa3bb87

[PATCH v2 5/8] t4012: Make --shortstat test more robust

[Alexander Strasser]

The --shortstat test depends on the same scenario as the --stat
test. Use the part of the same expected result for the --stat test
to avoid duplicating it manually.

Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4012-diff-binary.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
index daf8234..4767761 100755
--- a/t/t4012-diff-binary.sh
+++ b/t/t4012-diff-binary.sh
@@ -38,9 +38,9 @@ test_expect_success 'apply --stat output for binary file change' '
 '
 
 test_expect_success 'diff --shortstat output for binary file change' '
-	echo " 4 files changed, 2 insertions(+), 2 deletions(-)" >expected &&
+	tail -n 1 expected >expect &&
 	git diff --shortstat >current &&
-	test_i18ncmp expected current
+	test_i18ncmp expect current
 '
 
 test_expect_success 'diff --shortstat output for binary file change only' '
-- 
1.7.10.2.552.gaa3bb87

[PATCH v2 6/8] t4012: Re-indent test snippets

[Alexander Strasser]

Most one-level indents were 1 HT (horizontal tab) followed by 1 SP.
Remove the SP.

Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4012-diff-binary.sh | 68 +++++++++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
index 4767761..7b0e759 100755
--- a/t/t4012-diff-binary.sh
+++ b/t/t4012-diff-binary.sh
@@ -16,12 +16,12 @@ cat >expect.binary-numstat <<\EOF
 EOF
 
 test_expect_success 'prepare repository' '
-	 echo AIT >a && echo BIT >b && echo CIT >c && echo DIT >d &&
-	 git update-index --add a b c d &&
-	 echo git >a &&
-	 cat "$TEST_DIRECTORY"/test-binary-1.png >b &&
-	 echo git >c &&
-	 cat b b >d
+	echo AIT >a && echo BIT >b && echo CIT >c && echo DIT >d &&
+	git update-index --add a b c d &&
+	echo git >a &&
+	cat "$TEST_DIRECTORY"/test-binary-1.png >b &&
+	echo git >c &&
+	cat b b >d
 '
 
 cat > expected <<\EOF
@@ -64,52 +64,52 @@ 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' '
-	 git diff >output &&
-	 sed -e "s/-CIT/xCIT/" <output >broken &&
-	 if git apply --stat --summary broken 2>detected
-	 then
+	git diff >output &&
+	sed -e "s/-CIT/xCIT/" <output >broken &&
+	if git apply --stat --summary broken 2>detected
+	then
 		echo unhappy - should have detected an error
 		(exit 1)
-	 else
+	else
 		echo happy
-	 fi &&
-	 detected=`cat detected` &&
-	 detected=`expr "$detected" : "fatal.*at line \\([0-9]*\\)\$"` &&
-	 detected=`sed -ne "${detected}p" broken` &&
-	 test "$detected" = xCIT
+	fi &&
+	detected=`cat detected` &&
+	detected=`expr "$detected" : "fatal.*at line \\([0-9]*\\)\$"` &&
+	detected=`sed -ne "${detected}p" broken` &&
+	test "$detected" = xCIT
 '
 
 test_expect_success 'apply detecting corrupt patch correctly' '
-	 git diff --binary | sed -e "s/-CIT/xCIT/" >broken &&
-	 if git apply --stat --summary broken 2>detected
-	 then
+	git diff --binary | sed -e "s/-CIT/xCIT/" >broken &&
+	if git apply --stat --summary broken 2>detected
+	then
 		echo unhappy - should have detected an error
 		(exit 1)
-	 else
+	else
 		echo happy
-	 fi &&
-	 detected=`cat detected` &&
-	 detected=`expr "$detected" : "fatal.*at line \\([0-9]*\\)\$"` &&
-	 detected=`sed -ne "${detected}p" broken` &&
-	 test "$detected" = xCIT
+	fi &&
+	detected=`cat detected` &&
+	detected=`expr "$detected" : "fatal.*at line \\([0-9]*\\)\$"` &&
+	detected=`sed -ne "${detected}p" broken` &&
+	test "$detected" = xCIT
 '
 
 test_expect_success 'initial commit' 'git commit -a -m initial'
 
 # Try removal (b), modification (d), and creation (e).
 test_expect_success 'diff-index with --binary' '
-	 echo AIT >a && mv b e && echo CIT >c && cat e >d &&
-	 git update-index --add --remove a b c d e &&
-	 tree0=`git write-tree` &&
-	 git diff --cached --binary >current &&
-	 git apply --stat --summary current
+	echo AIT >a && mv b e && echo CIT >c && cat e >d &&
+	git update-index --add --remove a b c d e &&
+	tree0=`git write-tree` &&
+	git diff --cached --binary >current &&
+	git apply --stat --summary current
 '
 
 test_expect_success 'apply binary patch' '
-	 git reset --hard &&
-	 git apply --binary --index <current &&
-	 tree1=`git write-tree` &&
-	 test "$tree1" = "$tree0"
+	git reset --hard &&
+	git apply --binary --index <current &&
+	tree1=`git write-tree` &&
+	test "$tree1" = "$tree0"
 '
 
 test_expect_success 'diff --no-index with binary creation' '
-- 
1.7.10.2.552.gaa3bb87

[PATCH v2 7/8] t4012: use 'printf' instead of 'dd' to generate a binary file

[Alexander Strasser]

From: Johannes Sixt <j6t@kdbg.org>

For some reason, 'echo X | dd bs=1k seek=1' creates a file with 2050 bytes
on Windows instead of the expected 1026 bytes, so that a test fails. Since
the actual contents of the file are irrelevant as long as there is at
least one zero byte so that the diff machinery recognizes it as binary,
use printf to generate it.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
---
 t/t4012-diff-binary.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
index 7b0e759..077870e 100755
--- a/t/t4012-diff-binary.sh
+++ b/t/t4012-diff-binary.sh
@@ -131,7 +131,7 @@ cat >expect <<EOF
 EOF
 
 test_expect_success 'diff --stat with binary files and big change count' '
-	echo X | dd of=binfile bs=1k seek=1 &&
+	printf "\01\00%1024d" 1 >binfile &&
 	git add binfile &&
 	i=0 &&
 	while test $i -lt 10000; do
-- 
1.7.10.2.552.gaa3bb87

[PATCH v2 8/8] t4012: Use test_must_fail instead of if-else

[Alexander Strasser]

Make the code less bulky and easier to read. Also do not overlook
failures like e.g. git failing because of unexpected signals.

Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
---
 t/t4012-diff-binary.sh | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
index 077870e..ec4deea 100755
--- a/t/t4012-diff-binary.sh
+++ b/t/t4012-diff-binary.sh
@@ -66,13 +66,7 @@ test_expect_success 'apply --numstat understands diff --binary format' '
 test_expect_success 'apply detecting corrupt patch correctly' '
 	git diff >output &&
 	sed -e "s/-CIT/xCIT/" <output >broken &&
-	if git apply --stat --summary broken 2>detected
-	then
-		echo unhappy - should have detected an error
-		(exit 1)
-	else
-		echo happy
-	fi &&
+	test_must_fail git apply --stat --summary broken 2>detected &&
 	detected=`cat detected` &&
 	detected=`expr "$detected" : "fatal.*at line \\([0-9]*\\)\$"` &&
 	detected=`sed -ne "${detected}p" broken` &&
@@ -81,13 +75,7 @@ test_expect_success 'apply detecting corrupt patch correctly' '
 
 test_expect_success 'apply detecting corrupt patch correctly' '
 	git diff --binary | sed -e "s/-CIT/xCIT/" >broken &&
-	if git apply --stat --summary broken 2>detected
-	then
-		echo unhappy - should have detected an error
-		(exit 1)
-	else
-		echo happy
-	fi &&
+	test_must_fail git apply --stat --summary broken 2>detected &&
 	detected=`cat detected` &&
 	detected=`expr "$detected" : "fatal.*at line \\([0-9]*\\)\$"` &&
 	detected=`sed -ne "${detected}p" broken` &&
-- 
1.7.10.2.552.gaa3bb87

Re: [PATCH v2 0/8] Cleaning up t4012

[Junio C Hamano]

All looked reasonable; thanks folks.  Will requeue and advance it to
'next'.