Hi Christian, On Mon, Feb 7, 2022 at 7:42 PM Christian Couder <christian.couder@xxxxxxxxx> wrote: > > On Fri, Feb 4, 2022 at 6:00 AM Shaoxuan Yuan <shaoxuan.yuan02@xxxxxxxxx> 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@xxxxxxxxx> > > > 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@xxxxxxxxxxxxxx/ -- Thanks, Shaoxuan
