Re: [PATCH v10 2/3] t7507-commit-verbose: store output of grep in a file

> On Sun, Mar 27, 2016 at 8:37 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> So that we can see how many diffs were contained in the message and use
>>> them in individual tests where ever it is required. Also use
>>> write_script() to create the fake "editor".
>>
>> It is important to explain *why* you want to be able to count how many
>> diffs were in the editor message. In particular, you will be adding
>> new tests in a subsequent patch which will expect a specific number of
>> diffs (rather than any number of diffs)
>>
>> Also, you need to explain why you're changing the existing tests to
>> count the number of diffs. Is it simply "because you can" or is it
>> because you suspect that a change you're making in a subsequent patch
>> might accidentally cause too many diffs to show up in the existing
>> test cases?
>
> Sorry for not writing commit messages properly. It is a part I am working on.
> How does this paragraph sound to you in addition to the previous commit message?
> "A subsequent commit will introduce a scenario where it is important
> to be able to exactly determine how many diffs were present."
>
>>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>> ---
>>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
>>> @@ -3,9 +3,11 @@
>>> -cat >check-for-diff <<EOF
>>> -#!$SHELL_PATH
>>> -exec grep '^diff --git' "\$1"
>>> +write_script "check-for-diff" <<'EOF' &&
>>
>> The 'EOF' is more commonly written as \EOF in Git test scripts.
>>
>>> +! test -s out ||
>>> +rm out &&
>>
>> Why not just "rm -f out"? But, more importantly, why do you need to
>> remove the file at all? The '>' redirection operator (used below) will
>> overwrite the file, so no need to remove it beforehand.
>
> I wasn't aware about '-f' option. It is important to remove the file.
> I initially tried without removing the file and it caused problems.
> This is because let's say the previous test had 1 diff which was
> stored in out. Now, if in the next test, it is expected to have 0
> diffs then grep would fail to execute and nothing will be re-written
> to out and out will contain the 1 diff from previous test. An
> explanation for this should be mentioned in the commit message?
>
>>> +! grep '^diff --git' "$1" ||
>>> +grep '^diff --git' "$1" >out
>>
>> Um, what? Why two greps? I would have expected you to simply re-use
>> the existing grep (minus the backslash) while adding the redirection:
>>
>>     -exec grep '^diff --git' "\$1"
>>     +exec grep '^diff --git' "$1" >out
>
> The reason of two greps in that if in some test it fails to find any
> diffs, the editor will return an error. Of course we could test this
> scenario by using test_must_fail as in previous patches, but I think
> it would be more clearer if we could test for the existence of out
> which is done in patch 3/3. I will explanation for this in commit
> message.
>
>>>  EOF
>>>  chmod +x check-for-diff
>>
>> Mentioned previously[1]: Drop the 'chmod' since write_script() does it for you.
>>
>> [1]: http://article.gmane.org/gmane.comp.version-control.git/289832
>
> Then you mentioned that you were talking[1] about some other review.
> So I thought you mean to keep as it is. I guess I misinterpreted it.
> And I did not test that before. Now, I have tested that it works if we
> remove chmod.
> [1] : http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289832
>
>>>  test_set_editor "$PWD/check-for-diff"
>>> @@ -23,7 +25,8 @@ test_expect_success 'setup' '
>>>  test_expect_success 'initial commit shows verbose diff' '
>>> -       git commit --amend -v
>>> +       git commit --amend -v &&
>>> +       test_line_count = 1 out
>>>  '
>>>
>>>  test_expect_success 'second commit' '
>>> @@ -39,13 +42,15 @@ check_message() {
>>>
>>>  test_expect_success 'verbose diff is stripped out' '
>>>         git commit --amend -v &&
>>> -       check_message message
>>> +       check_message message &&
>>> +       test_line_count = 1 out
>>>  '
>>>
>>>  test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
>>>         git config diff.mnemonicprefix true &&
>>>         git commit --amend -v &&
>>> -       check_message message
>>> +       check_message message &&
>>> +       test_line_count = 1 out
>>>  '