Re: [PATCH v9 2/3] t7507-commit-verbose: make test suite use write_script

> On Thu, Mar 24, 2016 at 7:00 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>> Also remove test_set_editor from global scope and use it in whichever
>>> test it is required.
>>
>> Why?
>>
>> test_set_editor sets and exports shell variables.  Since you don't
>> invoke test_set_editor in a subshell, after the first invocation the
>> editor will be part of the global scope anyway.
>
> Agreed that this needs proper justification in the commit message.
> And, the justification is to make each test more self-contained,
> particularly because a subsequent patch will introduce a second fake
> "editor", and by making tests responsible for setting the editor they
> need, they don't have to worry about bad interactions from "editors"
> set by earlier tests[1][2].

> Another issue is that the commit message is backward. The subject
> ("t7507-commit-verbose: make test suite use write_script") tries to
> sell this as primarily being about write_script(), but the real gist
> of the patch is that it is making each test more self-contained, and
> the subject should say so. The write_script() bit is just a minor
> aside which can be introduced with a "While here let's use
> write_script to..." at the end of the commit message.
>
>>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
>>> index 2ddf28c..cf95efb 100755
>>> --- a/t/t7507-commit-verbose.sh
>>> +++ b/t/t7507-commit-verbose.sh
>>> @@ -3,12 +3,11 @@
>>>  test_description='verbose commit template'
>>>  . ./test-lib.sh
>>>
>>> -cat >check-for-diff <<EOF
>>> -#!$SHELL_PATH
>>> -exec grep '^diff --git' "\$1"
>>> +write_script "check-for-diff" <<-\EOF &&
>>> +grep '^diff --git' "$1" >out &&
>>> +test $(wc -l <out) = 1
>>
>> Our test lib offers the test_line_count helper function, which
>> outputs a helpful error message in case the number of lines do not
>> match.
>
> test_line_count() was mentioned in [2], however, this line counting is
> done in the fake "editor" script, not in the test script proper, so
> the spelled-out form $(wc ...) was proposed[2].

>> The original didn't check the number of lines.  This change is not
>> mentioned at all in the commit message.
>
> Right, and this particular change really belongs in patch 3/3 where
> it's needed[2], and should be properly explained by the 3/3 commit
> message.

>>> -test_set_editor "$PWD/check-for-diff"
>>>
>>>  cat >message <<'EOF'
>>>  subject
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/289329
> [2]: http://article.gmane.org/gmane.comp.version-control.git/289370