[v3,1/2] t/lib-read-tree-m-3way: modernize style

Commit Message

Shaoxuan Yuan Feb. 2, 2022, 6:42 a.m. UTC

Many invocations of the test_expect_success command in this
file are written in old style where the command, an optional
prerequisite, and the test title are written on separate
lines, and the executable script string begins on its own
line, and these lines are pasted together with backslashes
as necessary.

An invocation of the test_expect_success command in modern
test scripts however writes the prerequisite and the title
on the same line as the test_expect_success command itself,
and ends the line with a single quote that begins the
executable script string.

Update the style for uniformity.

Signed-off-by: Shaoxuan Yuan <[e-mail]>
---
 t/lib-read-tree-m-3way.sh | 154 +++++++++++++++++++-------------------
 1 file changed, 77 insertions(+), 77 deletions(-)

Comments

Christian Couder Feb. 7, 2022, 11:41 a.m. UTC | #1

On Fri, Feb 4, 2022 at 6:00 AM Shaoxuan Yuan <[e-mail]> wrote:
>
> Many invocations of the test_expect_success command in this
> file are written in old style where the command, an optional
> prerequisite, and the test title are written on separate
> lines, and the executable script string begins on its own
> line, and these lines are pasted together with backslashes
> as necessary.

It's not very clear here if "these lines" means only the separate
lines with the command, an optional prerequisite, and the test title,
or if it also means the first (or maybe many) line(s) of the
executable script string.

> An invocation of the test_expect_success command in modern
> test scripts however writes the prerequisite and the title
> on the same line as the test_expect_success command itself,
> and ends the line with a single quote that begins the
> executable script string.

It could also be 'test_expect_failure' instead of 'test_expect_success'.

> Update the style for uniformity.
>
> Signed-off-by: Shaoxuan Yuan <[e-mail]>

>  for p in M? Z/M?
>  do
>      echo This is modified $p in the branch A. >$p
> -    test_expect_success \
> -       'change in branch A (modification)' \
> -        "git update-index $p"
> +    test_expect_success 'change in branch A (modification)' '
> +        git update-index $p
> +    '

The above is not just about moving single quotes from one line to
another, but it changes some double quotes to single quotes, which
means that $p might not be interpreted in the same way. This is not
just a style issue and it should be explained in the commit message
why it's ok to make this change.

>  done
>
>  for p in AN AA Z/AN Z/AA
>  do
>      echo This is added $p in the branch A. >$p
> -    test_expect_success \
> -       'change in branch A (addition)' \
> -       "git update-index --add $p"
> +    test_expect_success 'change in branch A (addition)' '
> +           git update-index --add $p
> +    '

Here also some double quotes are changed into single quotes.

>  done

>  to_remove=$(echo ?D Z/?D)
>  rm -f $to_remove
> -test_expect_success \
> -    'change in branch B (removal)' \
> -    "git update-index --remove $to_remove"
> +test_expect_success 'change in branch B (removal)' '
> +    git update-index --remove $to_remove
> +'

Here also.

>  for p in ?M Z/?M
>  do
>      echo This is modified $p in the branch B. >$p
> -    test_expect_success \
> -       'change in branch B (modification)' \
> -       "git update-index $p"
> +    test_expect_success 'change in branch B (modification)' '
> +           git update-index $p
> +    '

Here also.

>  done

Shaoxuan Yuan Feb. 8, 2022, 1:43 a.m. UTC | #2

Hi Christian,

On Mon, Feb 7, 2022 at 7:42 PM Christian Couder
<[e-mail]> wrote:
>
> On Fri, Feb 4, 2022 at 6:00 AM Shaoxuan Yuan <[e-mail]> wrote:
> >
> > Many invocations of the test_expect_success command in this
> > file are written in old style where the command, an optional
> > prerequisite, and the test title are written on separate
> > lines, and the executable script string begins on its own
> > line, and these lines are pasted together with backslashes
> > as necessary.
>
> It's not very clear here if "these lines" means only the separate
> lines with the command, an optional prerequisite, and the test title,
> or if it also means the first (or maybe many) line(s) of the
> executable script string.

"these lines" here means every occurrence of the "the command, an optional
prerequisite, and the test title" and "the executable script string"
(four parts) put together.

> > An invocation of the test_expect_success command in modern
> > test scripts however writes the prerequisite and the title
> > on the same line as the test_expect_success command itself,
> > and ends the line with a single quote that begins the
> > executable script string.
>
> It could also be 'test_expect_failure' instead of 'test_expect_success'.

Agree, it should be put in a more general way, for example, "test functions
such as 'test_expect_success' and 'test_expect_failure' ", for accuracy.

> > Update the style for uniformity.
> >
> > Signed-off-by: Shaoxuan Yuan <[e-mail]>
>
> >  for p in M? Z/M?
> >  do
> >      echo This is modified $p in the branch A. >$p
> > -    test_expect_success \
> > -       'change in branch A (modification)' \
> > -        "git update-index $p"
> > +    test_expect_success 'change in branch A (modification)' '
> > +        git update-index $p
> > +    '
>
> The above is not just about moving single quotes from one line to
> another, but it changes some double quotes to single quotes, which
> means that $p might not be interpreted in the same way. This is not
> just a style issue and it should be explained in the commit message
> why it's ok to make this change.

Yes, I learned the reason behind from Eric[1] and I also went through
t/test-lib.sh and t/test-lib-functions.sh to see it myself.
And the reason should be written in the commit message for a potential
explanation.

Overall, thanks for the review, and I will ship a v4 along with the
modifications.

[1]: https://lore.kernel.org/git/[e-mail]/

--
Thanks,
Shaoxuan

Patch

diff --git a/t/lib-read-tree-m-3way.sh b/t/lib-read-tree-m-3way.sh
index 168329adbc..3c17cb7f80 100644
--- a/t/lib-read-tree-m-3way.sh
+++ b/t/lib-read-tree-m-3way.sh
@@ -8,16 +8,16 @@  do
         p=$a$b
 	echo This is $p from the original tree. >$p
 	echo This is Z/$p from the original tree. >Z/$p
-	test_expect_success \
-	    "adding test file $p and Z/$p" \
-	    'git update-index --add $p &&
-	    git update-index --add Z/$p'
+	test_expect_success "adding test file $p and Z/$p" '
+	    git update-index --add $p &&
+	    git update-index --add Z/$p
+    '
     done
 done
 echo This is SS from the original tree. >SS
-test_expect_success \
-    'adding test file SS' \
-    'git update-index --add SS'
+test_expect_success 'adding test file SS' '
+    git update-index --add SS
+'
 cat >TT <<\EOF
 This is a trivial merge sample text.
 Branch A is expected to upcase this word, here.
@@ -30,12 +30,12 @@  At the very end, here comes another line, that is
 the word, expected to be upcased by Branch B.
 This concludes the trivial merge sample file.
 EOF
-test_expect_success \
-    'adding test file TT' \
-    'git update-index --add TT'
-test_expect_success \
-    'prepare initial tree' \
-    'tree_O=$(git write-tree)'
+test_expect_success 'adding test file TT' '
+    git update-index --add TT
+'
+test_expect_success 'prepare initial tree' '
+    tree_O=$(git write-tree)
+'
 
 ################################################################
 # Branch A and B makes the changes according to the above matrix.
@@ -45,48 +45,48 @@  test_expect_success \
 
 to_remove=$(echo D? Z/D?)
 rm -f $to_remove
-test_expect_success \
-    'change in branch A (removal)' \
-    'git update-index --remove $to_remove'
+test_expect_success 'change in branch A (removal)' '
+    git update-index --remove $to_remove
+'
 
 for p in M? Z/M?
 do
     echo This is modified $p in the branch A. >$p
-    test_expect_success \
-	'change in branch A (modification)' \
-        "git update-index $p"
+    test_expect_success 'change in branch A (modification)' '
+        git update-index $p
+    '
 done
 
 for p in AN AA Z/AN Z/AA
 do
     echo This is added $p in the branch A. >$p
-    test_expect_success \
-	'change in branch A (addition)' \
-	"git update-index --add $p"
+    test_expect_success 'change in branch A (addition)' '
+	    git update-index --add $p
+    '
 done
 
 echo This is SS from the modified tree. >SS
 echo This is LL from the modified tree. >LL
-test_expect_success \
-    'change in branch A (addition)' \
-    'git update-index --add LL &&
-     git update-index SS'
+test_expect_success 'change in branch A (addition)' '
+    git update-index --add LL &&
+    git update-index SS
+'
 mv TT TT-
 sed -e '/Branch A/s/word/WORD/g' <TT- >TT
 rm -f TT-
-test_expect_success \
-    'change in branch A (edit)' \
-    'git update-index TT'
+test_expect_success 'change in branch A (edit)' '
+    git update-index TT
+'
 
 mkdir DF
 echo Branch A makes a file at DF/DF, creating a directory DF. >DF/DF
-test_expect_success \
-    'change in branch A (change file to directory)' \
-    'git update-index --add DF/DF'
+test_expect_success 'change in branch A (change file to directory)' '
+    git update-index --add DF/DF
+'
 
-test_expect_success \
-    'recording branch A tree' \
-    'tree_A=$(git write-tree)'
+test_expect_success 'recording branch A tree' '
+    tree_A=$(git write-tree)
+'
 
 ################################################################
 # Branch B
@@ -94,65 +94,65 @@  test_expect_success \
 
 rm -rf [NDMASLT][NDMASLT] Z DF
 mkdir Z
-test_expect_success \
-    'reading original tree and checking out' \
-    'git read-tree $tree_O &&
-     git checkout-index -a'
+test_expect_success 'reading original tree and checking out' '
+    git read-tree $tree_O &&
+    git checkout-index -a
+'
 
 to_remove=$(echo ?D Z/?D)
 rm -f $to_remove
-test_expect_success \
-    'change in branch B (removal)' \
-    "git update-index --remove $to_remove"
+test_expect_success 'change in branch B (removal)' '
+    git update-index --remove $to_remove
+'
 
 for p in ?M Z/?M
 do
     echo This is modified $p in the branch B. >$p
-    test_expect_success \
-	'change in branch B (modification)' \
-	"git update-index $p"
+    test_expect_success 'change in branch B (modification)' '
+	    git update-index $p
+    '
 done
 
 for p in NA AA Z/NA Z/AA
 do
     echo This is added $p in the branch B. >$p
-    test_expect_success \
-	'change in branch B (addition)' \
-	"git update-index --add $p"
+    test_expect_success 'change in branch B (addition)' '
+	    git update-index --add $p
+    '
 done
 echo This is SS from the modified tree. >SS
 echo This is LL from the modified tree. >LL
-test_expect_success \
-    'change in branch B (addition and modification)' \
-    'git update-index --add LL &&
-     git update-index SS'
+test_expect_success 'change in branch B (addition and modification)' '
+    git update-index --add LL &&
+    git update-index SS
+'
 mv TT TT-
 sed -e '/Branch B/s/word/WORD/g' <TT- >TT
 rm -f TT-
-test_expect_success \
-    'change in branch B (modification)' \
-    'git update-index TT'
+test_expect_success 'change in branch B (modification)' '
+    git update-index TT
+'
 
 echo Branch B makes a file at DF. >DF
-test_expect_success \
-    'change in branch B (addition of a file to conflict with directory)' \
-    'git update-index --add DF'
-
-test_expect_success \
-    'recording branch B tree' \
-    'tree_B=$(git write-tree)'
-
-test_expect_success \
-    'keep contents of 3 trees for easy access' \
-    'rm -f .git/index &&
-     git read-tree $tree_O &&
-     mkdir .orig-O &&
-     git checkout-index --prefix=.orig-O/ -f -q -a &&
-     rm -f .git/index &&
-     git read-tree $tree_A &&
-     mkdir .orig-A &&
-     git checkout-index --prefix=.orig-A/ -f -q -a &&
-     rm -f .git/index &&
-     git read-tree $tree_B &&
-     mkdir .orig-B &&
-     git checkout-index --prefix=.orig-B/ -f -q -a'
+test_expect_success 'change in branch B (addition of a file to conflict with directory)' '
+    git update-index --add DF
+'
+
+test_expect_success 'recording branch B tree' '
+    tree_B=$(git write-tree)
+'
+
+test_expect_success 'keep contents of 3 trees for easy access' '
+    rm -f .git/index &&
+    git read-tree $tree_O &&
+    mkdir .orig-O &&
+    git checkout-index --prefix=.orig-O/ -f -q -a &&
+    rm -f .git/index &&
+    git read-tree $tree_A &&
+    mkdir .orig-A &&
+    git checkout-index --prefix=.orig-A/ -f -q -a &&
+    rm -f .git/index &&
+    git read-tree $tree_B &&
+    mkdir .orig-B &&
+    git checkout-index --prefix=.orig-B/ -f -q -a
+'