On Thu, Feb 9, 2023 at 11:00 AM Shuqi Liang <cheskaqiqi@xxxxxxxxx> 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...
> 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.
> 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.
> 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.
> Signed-off-by: Shuqi Liang <cheskaqiqi@xxxxxxxxx>
> ---
> 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.
> -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.
> + {
> + 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 &&
> -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 &&
