[GSoC][PATCH] lib-read-tree-m-3way: modernize a test script (style)

[GSoC][PATCH] lib-read-tree-m-3way: modernize a test script (style)

[Shaoxuan Yuan]

As a microproject, I found another small fix regarding styling to do.

I changed the old style '\' (backslash) to new style "'" (single
quotes).

And I also fixed some double quotes misuse.

Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
Other than that, I forgot to introduce myself in the last patch and
here it goes:

I'm Shaoxuan Yuan, currently a sophomore majors in Computer Science and Engineering
(CSE) @ University of California, Irvine. 

I have prior open-source experience in which I was [maintaining|contributing to] the
Casbin community. My main language is Python, and I'm a C newbie
because I'm quite interested in contributing to git (since it is in my main daily
toolkit and it is a charm to wield :-) ).

And for now I'm still taking baby steps trying to crack some test script
styling issues. After getting more familiar with the git contribution
process, I will try something bigger (though not THAT big) to get a
firmer grasp of git.

 t/lib-read-tree-m-3way.sh | 154 +++++++++++++++++++-------------------
 1 file changed, 77 insertions(+), 77 deletions(-)

diff --git a/t/lib-read-tree-m-3way.sh b/t/lib-read-tree-m-3way.sh
index 168329adbc..e40739b8db 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
+'
-- 
2.25.1

Re: [GSoC][PATCH] lib-read-tree-m-3way: modernize a test script (style)

[Shaoxuan Yuan]

Not sure if this email is overlooked (I understand its content is
lackluster though).

I'm looking forward to participating in this year's GSoC :) And I will
be really appreciated if anyone could possibly reply to it (and my
self-intro).

--
Thanks,
Shaoxuan

On Sun, Jan 23, 2022 at 2:04 PM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> wrote:
>
> As a microproject, I found another small fix regarding styling to do.
>
> I changed the old style '\' (backslash) to new style "'" (single
> quotes).
>
> And I also fixed some double quotes misuse.
>
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
> Other than that, I forgot to introduce myself in the last patch and
> here it goes:
>
> I'm Shaoxuan Yuan, currently a sophomore majors in Computer Science and Engineering
> (CSE) @ University of California, Irvine.
>
> I have prior open-source experience in which I was [maintaining|contributing to] the
> Casbin community. My main language is Python, and I'm a C newbie
> because I'm quite interested in contributing to git (since it is in my main daily
> toolkit and it is a charm to wield :-) ).
>
> And for now I'm still taking baby steps trying to crack some test script
> styling issues. After getting more familiar with the git contribution
> process, I will try something bigger (though not THAT big) to get a
> firmer grasp of git.
>
>  t/lib-read-tree-m-3way.sh | 154 +++++++++++++++++++-------------------
>  1 file changed, 77 insertions(+), 77 deletions(-)
>
> diff --git a/t/lib-read-tree-m-3way.sh b/t/lib-read-tree-m-3way.sh
> index 168329adbc..e40739b8db 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
> +'
> --
> 2.25.1
>

Re: [GSoC][PATCH] lib-read-tree-m-3way: modernize a test script (style)

[Eric Sunshine]

On Mon, Jan 24, 2022 at 1:37 AM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> wrote:
> lib-read-tree-m-3way: modernize a test script (style)

Prefixing the subject with "t/" would help better explain which part
of the project this patch is touching. Perhaps:

    t/lib-read-tree-m-3way: modernize style

> As a microproject, I found another small fix regarding styling to do.

Everything above the "---" line below your sign-off will become part
of the permanent project history; everything below the "---" is mere
commentary for reviewers. Reviewers may be interested in knowing that
this is a microproject, but it likely won't be meaningful to future
readers of the project history. As such, place this sort of commentary
below the "---" line.

> I changed the old style '\' (backslash) to new style "'" (single
> quotes).

Not sure what this means. I _think_ you mean that you changed:

    test_expect_success \
        'title' \
        'body'

to:

    test_expect_success 'title' '
        body
    '

Sometimes you can convey such a meaning more clearly by a simple
example as illustrated above.

> And I also fixed some double quotes misuse.

Again, it is unclear what this means. Providing an example can help
readers understand what you changed.

> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
> Other than that, I forgot to introduce myself in the last patch and
> here it goes:
>
> I'm Shaoxuan Yuan, currently a sophomore majors in Computer Science and Engineering
> (CSE) @ University of California, Irvine.
>
> I have prior open-source experience in which I was [maintaining|contributing to] the
> Casbin community. My main language is Python, and I'm a C newbie
> because I'm quite interested in contributing to git (since it is in my main daily
> toolkit and it is a charm to wield :-) ).
>
> And for now I'm still taking baby steps trying to crack some test script
> styling issues. After getting more familiar with the git contribution
> process, I will try something bigger (though not THAT big) to get a
> firmer grasp of git.

Welcome. Please understand that all review comments (above and below)
are meant to be constructive and help familiarize you with the local
customs of the Git project; they are not meant as any sort of
criticism.

> diff --git a/t/lib-read-tree-m-3way.sh b/t/lib-read-tree-m-3way.sh
> @@ -8,16 +8,16 @@ do
>         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
> +    '

Taking into consideration the difference in behavior of single-quoted
strings and double-quoted strings, changing this test's title from
double- to single-quoted is undesirable since `$p` is supposed to be
interpolated into the title; the title should not contain a literal
"$p". (Unlike the test title which is simply echo'd/print'd, the test
body gets eval'd, which is why `$p` in the body is acceptable even
though the body is in single-quotes.)

> -test_expect_success \
> -    'adding test file SS' \
> -    'git update-index --add SS'
> +test_expect_success 'adding test file SS' '
> +    git update-index --add SS
> +'

Since you're touching this anyhow as part of fixing style issues, you
should also fix the indentation to use tabs rather than spaces. The
same comment applies to the rest of the patch, as well.

>  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

In this case, the indentation of the entire body of the for-loop needs
to be fixed to use tabs rather than spaces, however, fixing all the
indentation problems together with the other problems can make for a
too-noisy patch for reviewers to really see what is going on. As such,
it may be better to make this a multi-patch series in which one patch
fixes indentation problems only.

As mentioned above, changing the body of the test from double- to
single-quoted string should (presumably) be okay since the body gets
eval'd and `$p` will be visible at the time of `eval`, however, mixing
this sort of change in with all the other changes being made makes it
hard for reviewers to spot such little details, let alone reason about
correctness. As such, switching of quote types is also probably the
sort of change which should be split out into its own patch. The same
comment applies to other changes following this one.

Overall, with the exception of the test title which needs to
interpolate `$p`, the rest of the changes are probably reasonable and
benign. It's important to understand that lengthy patches like this
full of mechanical changes tend to be quite taxing on reviewers, so
it's a good idea to help in any way you can to ease the review burden.
This can be done, for instance, by making only a single type of change
per patch (i.e. indentation fixes), or by limiting the scope or
breadth of the changes, which is especially important for this sort of
"noise" change -- a change just for the sake of change -- which
doesn't have any immediate practical benefit.

Re: [GSoC][PATCH] lib-read-tree-m-3way: modernize a test script (style)

[Shaoxuan Yuan]

On Fri, Jan 28, 2022 at 4:34 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Mon, Jan 24, 2022 at 1:37 AM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> wrote:
> > lib-read-tree-m-3way: modernize a test script (style)
>
> Prefixing the subject with "t/" would help better explain which part
> of the project this patch is touching. Perhaps:
>
>     t/lib-read-tree-m-3way: modernize style

Thanks, this tip is really helpful and I forgot to add the directory prefix.

> > As a microproject, I found another small fix regarding styling to do.
>
> Everything above the "---" line below your sign-off will become part
> of the permanent project history; everything below the "---" is mere
> commentary for reviewers. Reviewers may be interested in knowing that
> this is a microproject, but it likely won't be meaningful to future
> readers of the project history. As such, place this sort of commentary
> below the "---" line.

Sure, I did not realize this. I should've put the comments below "---" instead.

> > I changed the old style '\' (backslash) to new style "'" (single
> > quotes).
>
> Not sure what this means. I _think_ you mean that you changed:
>
>     test_expect_success \
>         'title' \
>         'body'
>
> to:
>
>     test_expect_success 'title' '
>         body
>     '
>
> Sometimes you can convey such a meaning more clearly by a simple
> example as illustrated above.
>
> > And I also fixed some double quotes misuse.
>
> Again, it is unclear what this means. Providing an example can help
> readers understand what you changed.

Will do, definitely a helpful tip.

> > Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> > ---
> > Other than that, I forgot to introduce myself in the last patch and
> > here it goes:
> >
> > I'm Shaoxuan Yuan, currently a sophomore majors in Computer Science and Engineering
> > (CSE) @ University of California, Irvine.
> >
> > I have prior open-source experience in which I was [maintaining|contributing to] the
> > Casbin community. My main language is Python, and I'm a C newbie
> > because I'm quite interested in contributing to git (since it is in my main daily
> > toolkit and it is a charm to wield :-) ).
> >
> > And for now I'm still taking baby steps trying to crack some test script
> > styling issues. After getting more familiar with the git contribution
> > process, I will try something bigger (though not THAT big) to get a
> > firmer grasp of git.
>
> Welcome. Please understand that all review comments (above and below)
> are meant to be constructive and help familiarize you with the local
> customs of the Git project; they are not meant as any sort of
> criticism.

Thanks, I learned a lot from your review comments :)

> > diff --git a/t/lib-read-tree-m-3way.sh b/t/lib-read-tree-m-3way.sh
> > @@ -8,16 +8,16 @@ do
> >         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
> > +    '
>
> Taking into consideration the difference in behavior of single-quoted
> strings and double-quoted strings, changing this test's title from
> double- to single-quoted is undesirable since `$p` is supposed to be
> interpolated into the title; the title should not contain a literal
> "$p". (Unlike the test title which is simply echo'd/print'd, the test
> body gets eval'd, which is why `$p` in the body is acceptable even
> though the body is in single-quotes.)

Agree, I didn't realize the difference between single and double
quotes in shell scripts
(I was just imitating other similar style fixes).

> > -test_expect_success \
> > -    'adding test file SS' \
> > -    'git update-index --add SS'
> > +test_expect_success 'adding test file SS' '
> > +    git update-index --add SS
> > +'
>
> Since you're touching this anyhow as part of fixing style issues, you
> should also fix the indentation to use tabs rather than spaces. The
> same comment applies to the rest of the patch, as well.

Sure.

> >  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
>
> In this case, the indentation of the entire body of the for-loop needs
> to be fixed to use tabs rather than spaces, however, fixing all the
> indentation problems together with the other problems can make for a
> too-noisy patch for reviewers to really see what is going on. As such,
> it may be better to make this a multi-patch series in which one patch
> fixes indentation problems only.

> As mentioned above, changing the body of the test from double- to
> single-quoted string should (presumably) be okay since the body gets
> eval'd and `$p` will be visible at the time of `eval`, however, mixing
> this sort of change in with all the other changes being made makes it
> hard for reviewers to spot such little details, let alone reason about
> correctness. As such, switching of quote types is also probably the
> sort of change which should be split out into its own patch. The same
> comment applies to other changes following this one.

I think so. I was thinking fixing all the general styling issues in one
patch (since they are all style related), but now I realize that the general
style patch can be divided into sub-patches for clearer review experience.

And my question is, should I do this "multi-patch series" thing in the form of
"-v<n>" (all under this thread), e.g. "v2" or "v3"? Or I just submit
multiple patches
separately (a new thread for each one)?

> Overall, with the exception of the test title which needs to
> interpolate `$p`, the rest of the changes are probably reasonable and
> benign. It's important to understand that lengthy patches like this
> full of mechanical changes tend to be quite taxing on reviewers, so
> it's a good idea to help in any way you can to ease the review burden.
> This can be done, for instance, by making only a single type of change
> per patch (i.e. indentation fixes), or by limiting the scope or
> breadth of the changes, which is especially important for this sort of

I'm not quite sure what this means, and I quote, "limiting the scope or
breadth of the changes". Are you suggesting, for example,
fixing fewer occurrences of tab indentation issue in a patch; or, for
example, limiting the
fix to a specific "test_expect_success" block, and do multiple patches
sequentially?

> "noise" change -- a change just for the sake of change -- which
> doesn't have any immediate practical benefit.

Overall, thanks for the reply and it is really helpful! I did make a lot of
mistakes in this patch, but I'm learning a lot :)
And I will amend my patch base on these suggestions then submit a v2.

--
Thanks,
Shaoxuan

[PATCH v2 0/2] t/lib-read-tree-m-3way: modernize a test script

[Shaoxuan Yuan]

Modernized the test script t/lib-read-tree-m-3way.

Comparing to v1:
1. Kept the test title's double quotes if the title needs to 
    be interpolated.
    e.g. in the following block

    -	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
    +    '

    where the title "adding test file $p and Z/$p" is kept
    to use double quotes, since the "$p" needs to be interpolated.

2. Added a commit to replace spaces with tabs.

Shaoxuan Yuan (2):
  t/lib-read-tree-m-3way: replace double quotes with single quotes
  t/lib-read-tree-m-3way: replace spaces with tabs

 t/lib-read-tree-m-3way.sh | 168 +++++++++++++++++++-------------------
 1 file changed, 84 insertions(+), 84 deletions(-)

Range-diff against v1:
1:  0069b1f385 = 1:  0069b1f385 t/lib-read-tree-m-3way: replace double quotes with single quotes
-:  ---------- > 2:  92e2e6294b t/lib-read-tree-m-3way: replace spaces with tabs
-- 
2.35.0

[PATCH v2 1/2] t/lib-read-tree-m-3way: replace double quotes with single quotes

[Shaoxuan Yuan]

Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---

This commit simply converts the old style (double quotes) to
a modern style (single quotes), e.g.

-test_expect_success \
-    'adding test file SS' \
-    'git update-index --add SS'
+test_expect_success 'adding test file SS' '
+    git update-index --add SS
+'

 t/lib-read-tree-m-3way.sh | 154 +++++++++++++++++++-------------------
 1 file changed, 77 insertions(+), 77 deletions(-)

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
+'
-- 
2.35.0

[PATCH v2 2/2] t/lib-read-tree-m-3way: replace spaces with tabs

[Shaoxuan Yuan]

Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---

This commit simply replaced indentation spaces with tabs.
e.g.

-    git update-index --add SS
+	git update-index --add SS

 t/lib-read-tree-m-3way.sh | 96 +++++++++++++++++++--------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

diff --git a/t/lib-read-tree-m-3way.sh b/t/lib-read-tree-m-3way.sh
index 3c17cb7f80..2da25b3144 100644
--- a/t/lib-read-tree-m-3way.sh
+++ b/t/lib-read-tree-m-3way.sh
@@ -3,9 +3,9 @@
 mkdir Z
 for a in N D M
 do
-    for b in N D M
-    do
-        p=$a$b
+	for b in N D M
+	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" '
@@ -16,7 +16,7 @@ do
 done
 echo This is SS from the original tree. >SS
 test_expect_success 'adding test file SS' '
-    git update-index --add SS
+	git update-index --add SS
 '
 cat >TT <<\EOF
 This is a trivial merge sample text.
@@ -31,10 +31,10 @@ 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
+	git update-index --add TT
 '
 test_expect_success 'prepare initial tree' '
-    tree_O=$(git write-tree)
+	tree_O=$(git write-tree)
 '
 
 ################################################################
@@ -46,46 +46,46 @@ test_expect_success 'prepare initial tree' '
 to_remove=$(echo D? Z/D?)
 rm -f $to_remove
 test_expect_success 'change in branch A (removal)' '
-    git update-index --remove $to_remove
+	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
-    '
+	echo This is modified $p in the branch A. >$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
-    '
+	echo This is added $p in the branch A. >$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
+	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
+	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
+	git update-index --add DF/DF
 '
 
 test_expect_success 'recording branch A tree' '
-    tree_A=$(git write-tree)
+	tree_A=$(git write-tree)
 '
 
 ################################################################
@@ -95,64 +95,64 @@ test_expect_success 'recording branch A tree' '
 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
+	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
+	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
-    '
+	echo This is modified $p in the branch B. >$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
-    '
+	echo This is added $p in the branch B. >$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
+	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
+	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
+	git update-index --add DF
 '
 
 test_expect_success 'recording branch B tree' '
-    tree_B=$(git write-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
+	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
 '
-- 
2.35.0

Re: [PATCH v2 1/2] t/lib-read-tree-m-3way: replace double quotes with single quotes

[Junio C Hamano]

Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> writes:

> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>
> This commit simply converts the old style (double quotes) to
> a modern style (single quotes), e.g.
>
> -test_expect_success \
> -    'adding test file SS' \
> -    'git update-index --add SS'
> +test_expect_success 'adding test file SS' '
> +    git update-index --add SS
> +'

The old one does not use "double quotes", though ;-)

Also, the above belongs to the log message proper, as it would help
readers of "git log" in the future, as opposed to merely helping
reviewers only while the patch is under review (e.g. differences
between v1 and v2 is a good thing to write after "---", as "git log"
readers will not have access to v1, and will not even want to know
that there was v1).

Documentation/SubmittingPatches has more hints on the log message
writing to help anybody who wants to participate in this project.

 * The title summarizes what problem is being solved (yours is
   fine).

 * Then the status quo is explained in the present tense.

 * Readers are made to realize what is wrong about the status quo.

 * The approach taken to solve that problem is outlined.

 * Then orders are given to the codebase to "become like so" in
   imperative mood.

Applying the above to this patch:

	t/lib-read-tree-m-3way: modernize style

	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.

or something along that line, perhaps?

Re: [PATCH v2 2/2] t/lib-read-tree-m-3way: replace spaces with tabs

[Junio C Hamano]

Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> writes:

> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>
> This commit simply replaced indentation spaces with tabs.
> e.g.
>
> -    git update-index --add SS
> +	git update-index --add SS

The same comment applies wrt where the message is placed.

I'd view "replace spaces with" as a way to achieve "indent with
tabs".

    t/lib-read-tree-m-3way: indent with tabs

    As Documentation/CodingGuidelines says, our shell scripts
    (including tests) are to use HT for indentation, but this script
    uses 4-column indent with SP.  Fix this.

or something like that, perhaps?

Re: [PATCH v2 1/2] t/lib-read-tree-m-3way: replace double quotes with single quotes

[Shaoxuan Yuan]

On Wed, Feb 2, 2022 at 7:51 AM Junio C Hamano <gitster@pobox.com> wrote:
> The old one does not use "double quotes", though ;-)
>
> Also, the above belongs to the log message proper, as it would help
> readers of "git log" in the future, as opposed to merely helping
> reviewers only while the patch is under review (e.g. differences
> between v1 and v2 is a good thing to write after "---", as "git log"
> readers will not have access to v1, and will not even want to know
> that there was v1).

Thanks, think I should put the explanation message above the "---"
so "git log" has it. It's a really helpful tip (I think I was too
cautious to put
more message above "---" so I just left it blank but a commit title).

> Documentation/SubmittingPatches has more hints on the log message
> writing to help anybody who wants to participate in this project.
>
>  * The title summarizes what problem is being solved (yours is
>    fine).
>
>  * Then the status quo is explained in the present tense.
>
>  * Readers are made to realize what is wrong about the status quo.
>
>  * The approach taken to solve that problem is outlined.
>
>  * Then orders are given to the codebase to "become like so" in
>    imperative mood.
>
> Applying the above to this patch:
>
>         t/lib-read-tree-m-3way: modernize style
>
>         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.
>
> or something along that line, perhaps?

Another thanks. I should've been reading throughout the details in
Documentation/SubmittingPatches and I'm doing so.

I will try to get the format straight as per the suggestions (learned
a lot from them)
and documentation then submit a v3 sooner :-)

--
Thanks,
Shaoxuan

Re: [PATCH v2 2/2] t/lib-read-tree-m-3way: replace spaces with tabs

[Shaoxuan Yuan]

On Wed, Feb 2, 2022 at 7:57 AM Junio C Hamano <gitster@pobox.com> wrote:
> I'd view "replace spaces with" as a way to achieve "indent with
> tabs".

Oh my. It looks so noob to say "replace spaces with".
And for sure this is a much better way to phrase it. Guess I was having some
turbulence writing this patch, I should phrase it like that ;-)

>     t/lib-read-tree-m-3way: indent with tabs
>
>     As Documentation/CodingGuidelines says, our shell scripts
>     (including tests) are to use HT for indentation, but this script
>     uses 4-column indent with SP.  Fix this.
>
> or something like that, perhaps?

Learned the present tense and imperative mood :)

--
Thanks,
Shaoxuan

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

[Shaoxuan Yuan]

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 <shaoxuan.yuan02@gmail.com>
---
 t/lib-read-tree-m-3way.sh | 154 +++++++++++++++++++-------------------
 1 file changed, 77 insertions(+), 77 deletions(-)

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
+'
-- 
2.35.0

[PATCH v3 2/2] t/lib-read-tree-m-3way: indent with tabs

[Shaoxuan Yuan]

As Documentation/CodingGuidelines says, our shell scripts
(including tests) are to use HT for indentation, but this script
uses 4-column indent with SP. Fix this.

Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 t/lib-read-tree-m-3way.sh | 96 +++++++++++++++++++--------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

diff --git a/t/lib-read-tree-m-3way.sh b/t/lib-read-tree-m-3way.sh
index 3c17cb7f80..2da25b3144 100644
--- a/t/lib-read-tree-m-3way.sh
+++ b/t/lib-read-tree-m-3way.sh
@@ -3,9 +3,9 @@
 mkdir Z
 for a in N D M
 do
-    for b in N D M
-    do
-        p=$a$b
+	for b in N D M
+	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" '
@@ -16,7 +16,7 @@ do
 done
 echo This is SS from the original tree. >SS
 test_expect_success 'adding test file SS' '
-    git update-index --add SS
+	git update-index --add SS
 '
 cat >TT <<\EOF
 This is a trivial merge sample text.
@@ -31,10 +31,10 @@ 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
+	git update-index --add TT
 '
 test_expect_success 'prepare initial tree' '
-    tree_O=$(git write-tree)
+	tree_O=$(git write-tree)
 '
 
 ################################################################
@@ -46,46 +46,46 @@ test_expect_success 'prepare initial tree' '
 to_remove=$(echo D? Z/D?)
 rm -f $to_remove
 test_expect_success 'change in branch A (removal)' '
-    git update-index --remove $to_remove
+	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
-    '
+	echo This is modified $p in the branch A. >$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
-    '
+	echo This is added $p in the branch A. >$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
+	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
+	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
+	git update-index --add DF/DF
 '
 
 test_expect_success 'recording branch A tree' '
-    tree_A=$(git write-tree)
+	tree_A=$(git write-tree)
 '
 
 ################################################################
@@ -95,64 +95,64 @@ test_expect_success 'recording branch A tree' '
 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
+	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
+	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
-    '
+	echo This is modified $p in the branch B. >$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
-    '
+	echo This is added $p in the branch B. >$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
+	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
+	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
+	git update-index --add DF
 '
 
 test_expect_success 'recording branch B tree' '
-    tree_B=$(git write-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
+	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
 '
-- 
2.35.0

Re: [GSoC][PATCH] lib-read-tree-m-3way: modernize a test script (style)

[Eric Sunshine]

On Fri, Jan 28, 2022 at 4:51 AM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> wrote:
> On Fri, Jan 28, 2022 at 4:34 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > In this case, the indentation of the entire body of the for-loop needs
> > to be fixed to use tabs rather than spaces, however, fixing all the
> > indentation problems together with the other problems can make for a
> > too-noisy patch for reviewers to really see what is going on. As such,
> > it may be better to make this a multi-patch series in which one patch
> > fixes indentation problems only.
>
> > As mentioned above, changing the body of the test from double- to
> > single-quoted string should (presumably) be okay since the body gets
> > eval'd and `$p` will be visible at the time of `eval`, however, mixing
> > this sort of change in with all the other changes being made makes it
> > hard for reviewers to spot such little details, let alone reason about
> > correctness. As such, switching of quote types is also probably the
> > sort of change which should be split out into its own patch. The same
> > comment applies to other changes following this one.
>
> I think so. I was thinking fixing all the general styling issues in one
> patch (since they are all style related), but now I realize that the general
> style patch can be divided into sub-patches for clearer review experience.
>
> And my question is, should I do this "multi-patch series" thing in the form of
> "-v<n>" (all under this thread), e.g. "v2" or "v3"? Or I just submit
> multiple patches separately (a new thread for each one)?

A multi-patch series as v2, v3, etc. would indeed be appropriate, as
you already figured out[1] before I got around to answering belatedly.

> > Overall, with the exception of the test title which needs to
> > interpolate `$p`, the rest of the changes are probably reasonable and
> > benign. It's important to understand that lengthy patches like this
> > full of mechanical changes tend to be quite taxing on reviewers, so
> > it's a good idea to help in any way you can to ease the review burden.
> > This can be done, for instance, by making only a single type of change
> > per patch (i.e. indentation fixes), or by limiting the scope or
> > breadth of the changes, which is especially important for this sort of
>
> I'm not quite sure what this means, and I quote, "limiting the scope or
> breadth of the changes". Are you suggesting, for example,
> fixing fewer occurrences of tab indentation issue in a patch; or, for
> example, limiting the
> fix to a specific "test_expect_success" block, and do multiple patches
> sequentially?

Sorry for being unclear. I just meant that as a microproject, it would
have worked equally well to pick a much smaller test script with style
problems (if you could find one) rather than a long script. After all,
the purpose of a microproject is to give you experience with the
submission-review process and to give reviewers and mentors an idea of
how you interact. It's the process which is important, in this case,
not the size of the submission.

Anyhow, it looks like Junio is happy with your v3 and will be merging
it down to "next", so it all worked out.

[1]: https://lore.kernel.org/git/20220202064300.3601-1-shaoxuan.yuan02@gmail.com/
[2]: https://lore.kernel.org/git/xmqqr18jnr2t.fsf@gitster.g/

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

[Christian Couder]

On Fri, Feb 4, 2022 at 6:00 AM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> 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 <shaoxuan.yuan02@gmail.com>

>  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

Re: [PATCH v3 2/2] t/lib-read-tree-m-3way: indent with tabs

[Christian Couder]

On Sun, Feb 6, 2022 at 10:51 PM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> wrote:
>
> As Documentation/CodingGuidelines says, our shell scripts
> (including tests) are to use HT for indentation, but this script

Documentation/CodingGuidelines talks about "tabs" for indentation not
"HT", so it would be more consistent to talk about "tabs" here too, or
at least to say something like "are to use HT (horizontal tab) for
indentation".

> uses 4-column indent with SP. Fix this.

Same for "SP" here vs "space" in our doc. (Also note that `man ascii`
talks about "HT" and "SPACE", not "SP".)

> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>

Re: [GSoC][PATCH] lib-read-tree-m-3way: modernize a test script (style)

[Shaoxuan Yuan]

Got it, I'm learning along the way, and thanks for the reply!

--
Thanks,
Shaoxuan

On Sat, Feb 5, 2022 at 7:59 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Jan 28, 2022 at 4:51 AM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> wrote:
> > On Fri, Jan 28, 2022 at 4:34 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > In this case, the indentation of the entire body of the for-loop needs
> > > to be fixed to use tabs rather than spaces, however, fixing all the
> > > indentation problems together with the other problems can make for a
> > > too-noisy patch for reviewers to really see what is going on. As such,
> > > it may be better to make this a multi-patch series in which one patch
> > > fixes indentation problems only.
> >
> > > As mentioned above, changing the body of the test from double- to
> > > single-quoted string should (presumably) be okay since the body gets
> > > eval'd and `$p` will be visible at the time of `eval`, however, mixing
> > > this sort of change in with all the other changes being made makes it
> > > hard for reviewers to spot such little details, let alone reason about
> > > correctness. As such, switching of quote types is also probably the
> > > sort of change which should be split out into its own patch. The same
> > > comment applies to other changes following this one.
> >
> > I think so. I was thinking fixing all the general styling issues in one
> > patch (since they are all style related), but now I realize that the general
> > style patch can be divided into sub-patches for clearer review experience.
> >
> > And my question is, should I do this "multi-patch series" thing in the form of
> > "-v<n>" (all under this thread), e.g. "v2" or "v3"? Or I just submit
> > multiple patches separately (a new thread for each one)?
>
> A multi-patch series as v2, v3, etc. would indeed be appropriate, as
> you already figured out[1] before I got around to answering belatedly.
>
> > > Overall, with the exception of the test title which needs to
> > > interpolate `$p`, the rest of the changes are probably reasonable and
> > > benign. It's important to understand that lengthy patches like this
> > > full of mechanical changes tend to be quite taxing on reviewers, so
> > > it's a good idea to help in any way you can to ease the review burden.
> > > This can be done, for instance, by making only a single type of change
> > > per patch (i.e. indentation fixes), or by limiting the scope or
> > > breadth of the changes, which is especially important for this sort of
> >
> > I'm not quite sure what this means, and I quote, "limiting the scope or
> > breadth of the changes". Are you suggesting, for example,
> > fixing fewer occurrences of tab indentation issue in a patch; or, for
> > example, limiting the
> > fix to a specific "test_expect_success" block, and do multiple patches
> > sequentially?
>
> Sorry for being unclear. I just meant that as a microproject, it would
> have worked equally well to pick a much smaller test script with style
> problems (if you could find one) rather than a long script. After all,
> the purpose of a microproject is to give you experience with the
> submission-review process and to give reviewers and mentors an idea of
> how you interact. It's the process which is important, in this case,
> not the size of the submission.
>
> Anyhow, it looks like Junio is happy with your v3 and will be merging
> it down to "next", so it all worked out.
>
> [1]: https://lore.kernel.org/git/20220202064300.3601-1-shaoxuan.yuan02@gmail.com/
> [2]: https://lore.kernel.org/git/xmqqr18jnr2t.fsf@gitster.g/

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

[Shaoxuan Yuan]

Hi Christian,

On Mon, Feb 7, 2022 at 7:42 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Fri, Feb 4, 2022 at 6:00 AM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> 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 <shaoxuan.yuan02@gmail.com>
>
> >  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/CAPig+cS5tOr2NRJmAC1BNQPKYyeLXy0iy36q35-y7rFkrWewJw@mail.gmail.com/

--
Thanks,
Shaoxuan

Re: [PATCH v3 2/2] t/lib-read-tree-m-3way: indent with tabs

[Shaoxuan Yuan]

On Mon, Feb 7, 2022 at 7:54 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Sun, Feb 6, 2022 at 10:51 PM Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> wrote:
> >
> > As Documentation/CodingGuidelines says, our shell scripts
> > (including tests) are to use HT for indentation, but this script
>
> Documentation/CodingGuidelines talks about "tabs" for indentation not
> "HT", so it would be more consistent to talk about "tabs" here too, or
> at least to say something like "are to use HT (horizontal tab) for
> indentation".

Agree, it should be phrased in a more consistent and understandable way.

> > uses 4-column indent with SP. Fix this.
>
> Same for "SP" here vs "space" in our doc. (Also note that `man ascii`
> talks about "HT" and "SPACE", not "SP".)

Agree.

--
Thanks,
Shaoxuan

[PATCH v4 0/2] t/lib-read-tree-m-3way: modernize a test script

[Shaoxuan Yuan]

== Basic Summary ==

Modernize the test script t/lib-read-tree-m-3way.

Shaoxuan Yuan (2):
  t/lib-read-tree-m-3way: modernize style
  t/lib-read-tree-m-3way: indent with tabs

 t/lib-read-tree-m-3way.sh | 168 +++++++++++++++++++-------------------
 1 file changed, 84 insertions(+), 84 deletions(-)

Range-diff against v3:
1:  72323a7a57 ! 1:  6cbca771e5 t/lib-read-tree-m-3way: modernize style
    @@ Metadata
      ## Commit message ##
         t/lib-read-tree-m-3way: modernize style
     
    -    Many invocations of the test_expect_success command in this
    +    Many invocations of the test commands (e.g. test_expect_success
    +    or test_expect_failure) 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.
    +    prerequisite, the test title, and the executable script string
    +    are written on separate lines, with 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
    +    An invocation of the test command in modern
         test scripts however writes the prerequisite and the title
    -    on the same line as the test_expect_success command itself,
    +    on the same line as the test command itself,
         and ends the line with a single quote that begins the
         executable script string.
     
    +    It is worth notice that albeit all executable script strings
    +    are changed to use single quotes (for modern style uniformity),
    +    some of the test titles are kept untouched, e.g.
    +
    +    -       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
    +    +    '
    +
    +    see the "adding test file $p and Z/$p" part.
    +
    +    This is because the test title is simply echo'd/print'd, and
    +    double quotes are necessary for "$p" interpolation; however,
    +    the test body (executable script string) gets eval'd, and
    +    single quotes are acceptable in this case.
    +
         Update the style for uniformity.
     
         Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
2:  477b71497f ! 2:  e5d89ca1de t/lib-read-tree-m-3way: indent with tabs
    @@ Commit message
         t/lib-read-tree-m-3way: indent with tabs
     
         As Documentation/CodingGuidelines says, our shell scripts
    -    (including tests) are to use HT for indentation, but this script
    -    uses 4-column indent with SP. Fix this.
    +    (including tests) are to use tabs for indentation, but this script
    +    uses 4-column indent with space. Fix this.
     
         Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
     
-- 
2.35.1

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

[Shaoxuan Yuan]

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

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

It is worth notice that albeit all executable script strings
are changed to use single quotes (for modern style uniformity),
some of the test titles are kept untouched, e.g.

-	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
+    '

see the "adding test file $p and Z/$p" part.

This is because the test title is simply echo'd/print'd, and
double quotes are necessary for "$p" interpolation; however,
the test body (executable script string) gets eval'd, and
single quotes are acceptable in this case.

Update the style for uniformity.

Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 t/lib-read-tree-m-3way.sh | 154 +++++++++++++++++++-------------------
 1 file changed, 77 insertions(+), 77 deletions(-)

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
+'
-- 
2.35.1

[PATCH v4 2/2] t/lib-read-tree-m-3way: indent with tabs

[Shaoxuan Yuan]

As Documentation/CodingGuidelines says, our shell scripts
(including tests) are to use tabs for indentation, but this script
uses 4-column indent with space. Fix this.

Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
 t/lib-read-tree-m-3way.sh | 96 +++++++++++++++++++--------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

diff --git a/t/lib-read-tree-m-3way.sh b/t/lib-read-tree-m-3way.sh
index 3c17cb7f80..2da25b3144 100644
--- a/t/lib-read-tree-m-3way.sh
+++ b/t/lib-read-tree-m-3way.sh
@@ -3,9 +3,9 @@
 mkdir Z
 for a in N D M
 do
-    for b in N D M
-    do
-        p=$a$b
+	for b in N D M
+	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" '
@@ -16,7 +16,7 @@ do
 done
 echo This is SS from the original tree. >SS
 test_expect_success 'adding test file SS' '
-    git update-index --add SS
+	git update-index --add SS
 '
 cat >TT <<\EOF
 This is a trivial merge sample text.
@@ -31,10 +31,10 @@ 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
+	git update-index --add TT
 '
 test_expect_success 'prepare initial tree' '
-    tree_O=$(git write-tree)
+	tree_O=$(git write-tree)
 '
 
 ################################################################
@@ -46,46 +46,46 @@ test_expect_success 'prepare initial tree' '
 to_remove=$(echo D? Z/D?)
 rm -f $to_remove
 test_expect_success 'change in branch A (removal)' '
-    git update-index --remove $to_remove
+	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
-    '
+	echo This is modified $p in the branch A. >$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
-    '
+	echo This is added $p in the branch A. >$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
+	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
+	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
+	git update-index --add DF/DF
 '
 
 test_expect_success 'recording branch A tree' '
-    tree_A=$(git write-tree)
+	tree_A=$(git write-tree)
 '
 
 ################################################################
@@ -95,64 +95,64 @@ test_expect_success 'recording branch A tree' '
 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
+	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
+	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
-    '
+	echo This is modified $p in the branch B. >$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
-    '
+	echo This is added $p in the branch B. >$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
+	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
+	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
+	git update-index --add DF
 '
 
 test_expect_success 'recording branch B tree' '
-    tree_B=$(git write-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
+	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
 '
-- 
2.35.1