On Thu, Mar 24, 2016 at 7:00 AM, SZEDER Gábor <szeder@xxxxxxxxxx> 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.
>> EOF
>> chmod +x check-for-diff
Drop the 'chmod' line; write_script() does this for you.
>> -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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html