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

[Shuqi Liang <cheskaqiqi@gmail.com>]

On Tue, Feb 14, 2023 at 8:09 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

> 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.
>
Thanks, that really makes the change more clear for the readers. I learn a lot.

> 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.

Yeah, the commit looks very wordy. I'll make it more succinct.
>     test_write_lines a b c >file &&

> 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 &&

Thanks for the tips!

(Sorry for sending the V6 to you twice I send it by accident .)

-------------------------
Thanks
Shuqi