threads / patch / 30997

patch, 6 partsCleaning up t4012

Subject: [PATCH 0/6] Cleaning up t4012

## tl;dr

13 messages between Jul 11, 2012 and Jul 16, 2012. Diffs are folded; open one to read it.

replies: 12people: 3as markdown or json

Alexander Strasser· Jul 11, 2012, 22:11 UTC · lore

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

Johannes Sixt· Jul 12, 2012, 07:13 UTC · re: Alexander Strasser · lore

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

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

Show changes to t/t4012-diff-binary.sh +1 −1

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

Alexander Strasser· Jul 12, 2012, 22:25 UTC · re: Johannes Sixt · lore

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

Hi,

Johannes Sixt wrote:

Show 11 quoted lines

> 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

Show 18 quoted lines

>  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

Alexander Strasser· Jul 16, 2012, 20:40 UTC · re: Alexander Strasser · lore

[PATCH v2 0/8] Cleaning up t4012

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

Alexander Strasser· Jul 16, 2012, 20:43 UTC · re: Alexander Strasser · lore

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

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

Show changes to t/t4012-diff-binary.sh +20 −15

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

Alexander Strasser· Jul 16, 2012, 20:43 UTC · re: Alexander Strasser · lore

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

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

Show changes to t/t4012-diff-binary.sh +1 −1

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

Alexander Strasser· Jul 16, 2012, 20:43 UTC · re: Alexander Strasser · lore

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

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

Show changes to t/t4012-diff-binary.sh +2 −2

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

Alexander Strasser· Jul 16, 2012, 20:44 UTC · re: Alexander Strasser · lore

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

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

Show changes to t/t4012-diff-binary.sh +2 −1

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

Alexander Strasser· Jul 16, 2012, 20:45 UTC · re: Alexander Strasser · lore

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

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

Show changes to t/t4012-diff-binary.sh +2 −2

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

Alexander Strasser· Jul 16, 2012, 20:45 UTC · re: Alexander Strasser · lore

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

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

Show changes to t/t4012-diff-binary.sh +34 −34

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

Alexander Strasser· Jul 16, 2012, 20:46 UTC · re: Alexander Strasser · lore

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

From: Johannes Sixt <j6t@kdbg.org>

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

Show changes to t/t4012-diff-binary.sh +1 −1

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

Alexander Strasser· Jul 16, 2012, 20:47 UTC · re: Alexander Strasser · lore

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

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

Show changes to t/t4012-diff-binary.sh +2 −14

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

Junio C Hamano· Jul 16, 2012, 21:11 UTC · re: Alexander Strasser · lore

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

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