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

[Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>]

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