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

[Eric Sunshine <sunshine@sunshineco.com>]

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.