Re: [PATCH 1/3] Update t4001 to use modern syntax.

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

"Mark C. Chu-Carroll via B4 Relay"
<devnull+markchucarroll.fastmail.com@kernel.org> writes:

> Subject: Re: [PATCH 1/3] Update t4001 to use modern syntax.

Documentation/SubmittingPatches::[[describe-changes]]
Documentation/SubmittingPatches::[[summary-section]]

> From: "Mark C. Chu-Carroll" <markchucarroll@fastmail.com>
>
> ---

Documentation/SubmittingPatches::[[sign-off]]

>  t/t4001-diff-rename.sh   | 31 ++++++++++++++++---------------
>  t/t4009-diff-rename-4.sh |  8 ++++----
>  2 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
> index ad474100af..2aa161c217 100755
> --- a/t/t4001-diff-rename.sh
> +++ b/t/t4001-diff-rename.sh
> @@ -88,28 +88,29 @@ test_expect_success 'setup' '
>  	EOF
>  '

There are a bit more in the differences between this ancient style
and the modern style.  Not just the title appearing on the first line
and the body is opened with a single quote at the end of the first
line, the body is indented with a single tab.

>  
> -test_expect_success \
> -    'update-index --add a file.' \
> -    'git update-index --add path0'
> +test_expect_success 'update-index --add a file.' '
> +    git update-index --add path0
> +'

Also in "modern style", the tests are split at more logical
boundaries.  As the topic of this test is "diff rename", our purpose
of this test script is not to catch a crashing "update-index --add".
We are not interested in finding "update-index --add" to fail and
see "not ok" for such a failure.  This step is merely the first step
of building the tree object to be compared later with a modified
index.

> -test_expect_success \
> -    'write that tree.' \
> -    'tree=$(git write-tree) && echo $tree'
> +test_expect_success 'write that tree.' '
> +    tree=$(git write-tree) && echo $tree
> +'

Likewise, we are not interested to find out what object name the
resulting tree object gets.  "echo" here were placed long ago merely
for debugging purposes.

>  sed -e 's/line/Line/' <path0 >path1
>  rm -f path0

And in "modern style" tests, we strongly frown upon tests doing
anything outside test_expect_success blocks.  This is a preparation
to pretend that path0 was "renamed" to path1, and it is concluded ...

> -test_expect_success \
> -    'renamed and edited the file.' \
> -    'git update-index --add --remove path0 path1'
>  
> -test_expect_success \
> -    'git diff-index -p -M after rename and editing.' \
> -    'git diff-index -p -M $tree >current'
> +test_expect_success 'renamed and edited the file.' '
> +    git update-index --add --remove path0 path1
> +'

... with this step.

> +test_expect_success 'git diff-index -p -M after rename and editing.' '
> +    git diff-index -p -M $tree >current
> +'

And the output is obtained.  Again, it is not like we are happy that
this "diff-index" does not crash, so in "modern style", we do not
split a logically test like this at this point.  We want to see the
command produce, without segfaulting, its output to the file "current",
and we also want to see that the result matches what we expect.

> -test_expect_success \
> -    'validate the output.' \
> -    'compare_diff_patch current expected'
> +test_expect_success 'validate the output.' '
> +    compare_diff_patch current expected
> +'

In addition, in "modern" style, it is more common to name the file
that the actual output goes "actual", and the file that has the
expected contents "expect", and compare "expect" with "actual".
This test has compared contents in two files with wrong names, and
compares them in a wrong order.

Taking all together, it would look more like this, I would imagine.
Of course as "expected" has been renamed to "expect" in the initial
set-up part, the fallouts in the remainder of the test script also
needs to be dealt with, which is left as an exercise to the reader.

 t/t4001-diff-rename.sh | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git c/t/t4001-diff-rename.sh w/t/t4001-diff-rename.sh
index ad474100af..61d651d1db 100755
--- c/t/t4001-diff-rename.sh
+++ w/t/t4001-diff-rename.sh
@@ -26,7 +26,7 @@ test_expect_success 'setup' '
 	Line 14
 	Line 15
 	EOF
-	cat >expected <<-\EOF &&
+	cat >expect <<-\EOF &&
 	diff --git a/path0 b/path1
 	rename from path0
 	rename to path1
@@ -88,28 +88,19 @@ test_expect_success 'setup' '
 	EOF
 '
 
-test_expect_success \
-    'update-index --add a file.' \
-    'git update-index --add path0'
-
-test_expect_success \
-    'write that tree.' \
-    'tree=$(git write-tree) && echo $tree'
+test_expect_success 'path0 renamed to path1 with minor edit' '
+	git update-index --add path0 &&
+	tree=$(git write-tree) &&
 
-sed -e 's/line/Line/' <path0 >path1
-rm -f path0
-test_expect_success \
-    'renamed and edited the file.' \
-    'git update-index --add --remove path0 path1'
+	# edit and rename
+	sed -e 's/line/Line/' <path0 >path1 &&
+	rm -f path0 &&
+	git update-index --add --remove path0 path1 &&
 
-test_expect_success \
-    'git diff-index -p -M after rename and editing.' \
-    'git diff-index -p -M $tree >current'
+	git diff-index -p -M $tree >actual &&
 
-
-test_expect_success \
-    'validate the output.' \
-    'compare_diff_patch current expected'
+	compare_diff_patch expect actual
+'
 
 test_expect_success 'test diff.renames=true' '
 	git -c diff.renames=true diff --cached $tree >current &&