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

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

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