Re: [PATCH v5 3/3] t4113: put executable lines to test_expect_success

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 15, 2023, 01:09 UTC
Message-ID: <CAPig+cTKCMwCQ2TxXyc1KquZmB0QWQAuuag52a1qmeYh6XPXYQ@mail.gmail.com>
In-Reply-To: <20230209154417.10763-4-cheskaqiqi@gmail.com>

On Thu, Feb 9, 2023 at 11:00 AM Shuqi Liang <cheskaqiqi@gmail.com> wrote:

> As t/README says, put all code inside test_expect_success and
> other assertions. This script is written in old style,where there are
> some executable lines outside test_expect_success. Put the executable
> lines inside the test_expect_success.

Although it's true that t/README explains why code should be placed inside tests, you can help readers out by simply explaining the reason here in the commit message. For instance, you might replace the above paragraph with:

    Some old test scripts have setup code outside of tests. This
    is problematic since any failures of the setup code will go
    unnoticed. Therefore, move setup code into the tests themselves
    so that failures are properly flagged.

As for the rest of the commit message...

Show 11 quoted lines

> As t/README says,use "<<-" instead of "<<"
> to strip leading TABs used for indentation. Change the "<<" to "<<-"
>
> for example:
> -cat >test-patch <<\EOF
> -diff a/file b/file
>
>  test_expect_success 'apply at the beginning' '
> +       cat >test-patch <<-\EOF
> +       diff a/file b/file
> +       --- a/file

Certain changes are considered obvious by reviewers, so you don't need to mention them explicitly in the commit message. This is one such change. Any reviewer who sees that you indented the here-doc body to match the indentation of the rest of the test body will understand why you changed `<<` to `<<-` without the commit message having to explain it.

Show 10 quoted lines

> As t/README says,chain test assertions.Chain this test assertions
> with &&.
>
> For example:
>
> -cat >test-patch <<\EOF
> -diff --git a/file b/file
>
> + cat >test-patch <<-\EOF &&
> + diff --git a/file b/file

Same thing. Reviewers understand that all code inside a test body must have an intact &&-chain, so you needn't mention this in the commit message.

Show 12 quoted lines

> This script is written in old style,where there are something like
>
>         echo x >file &&
>         echo y >>file &&
>         echo z >>file
>
>   Change it to this stlye :
>         {
>         echo x &&
>         echo y &&
>         echo z
>         } >file

This is similar. This is such a simple style change, and the code fragment itself is so tiny, that a reviewer can understand this change without the commit message spelling it out.

> In order to escape for executable lines inside the test_expect_success.
> Change ' in executable lines to '\'' in order to escape.

Likewise.

Reviewers appreciate well-explained commit messages, but they also appreciate succinctness. Although it may not always be obvious how much to write in a commit message, you can assume that reviewers will understand obvious changes simply by reading the patch itself, thus you don't need to mention every little detail in the commit message. The important thing to mention in the commit message is the explanation of _why_ the change is being made, plus any changes which might not be obvious. In this case, all the changes are obvious, so, really, you can collapse this entire commit message to just the first paragraph.

Show 6 quoted lines

> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
> diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
> @@ -8,46 +8,45 @@ test_description='git apply trying to add an ending line.
> -# setup
> -

Good to see that you got rid of the now-unnecessary comment.

Show 24 quoted lines

> -cat >test-patch <<\EOF
> -diff --git a/file b/file
> ---- a/file
> -+++ b/file
> -@@ -1,2 +1,3 @@
> - a
> - b
> -+c
> -EOF
> -
> -echo 'a' >file
> -echo 'b' >>file
> -echo 'c' >>file
> -
>  test_expect_success setup '
> +       cat >test-patch <<-\EOF &&
> +       diff --git a/file b/file
> +       --- a/file
> +       +++ b/file
> +       @@ -1,2 +1,3 @@
> +        a
> +        b
> +       +c
> +       EOF

Okay.

Show 5 quoted lines

> +       {
> +       echo '\''a'\'' &&
> +       echo '\''b'\'' &&
> +       echo '\''c'\''
> +       } >file &&

A few comments:

This is unnecessarily confusing. Although this does work, it would be sufficient just to change the single-quotes to double-quotes, like this:

    {
    echo "a" &&
    echo "b" &&
    echo "c"
    } >file &&

Even simpler, you could drop the quotes altogether for such a simple case:

    {
    echo a &&
    echo b &&
    echo c
    } >file &&

However, as mentioned elsewhere in this thread, a really succinct way to do this, taking advantage of modern style would be to use test_write_lines(), so the five lines collapse to a single line:

    test_write_lines a b c >file &&

Show 29 quoted lines

> -cat >test-patch <<\EOF
> -diff a/file b/file
> ---- a/file
> -+++ b/file
> -@@ -1,2 +1,3 @@
> -+a
> - b
> - c
> -EOF
> -
> -echo >file 'a
> -b
> -c'
> -git update-index file
>
>  test_expect_success 'apply at the beginning' '
> +       cat >test-patch <<-\EOF &&
> +       diff a/file b/file
> +       --- a/file
> +       +++ b/file
> +       @@ -1,2 +1,3 @@
> +       +a
> +        b
> +        c
> +       EOF
> +
> +       echo >file '\''a
> +       b
> +       c'\'' &&

Same comment about simply using double-quotes instead of single-quotes, however, this is also another really good place to use test_write_lines:

    test_write_lines a b c >file &&