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

> On Sun, Mar 27, 2016 at 1:42 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> 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:
>>> 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."
>
> That's fine for explaining why 'check-for-diff' is being updated to
> store the output, but you still need to explain why you're changing
> the existing tests.
>
>>>> -cat >check-for-diff <<EOF
>>>> -#!$SHELL_PATH
>>>> -exec grep '^diff --git' "\$1"
>>>> +write_script "check-for-diff" <<'EOF' &&
>>>> +! 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?
>
> No. Rather than trying to explain all this in the commit message, it
> would be better to retain a simple implementation of 'check-for-diff'
> rather than adding several levels of complication. More about this
> below...
>
>>>> +! 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.
>
> This sounds unnecessarily complicated. It's not too difficult to
> reason about a script named 'check-for-diff' actually "checking for
> presence of diffs" and failing if no diff is present, from which it
> follows naturally that new tests which don't expect any diffs would
> use test_must_fail. Keeping it simple like this also makes this patch
> much less noisy since it doesn't require changing existing tests.
>
> Likewise, it keeps most of the new tests in 3/3 small because the bulk
> of them only want to know that a diff was present, but don't care
> about the number of diffs. However, it's not necessarily a bad thing
> to ensure that you got the number of diffs you expected (for instance,
> that a single -v really behaved as -v, and not as -v -v), so that can
> be used as an argument for overhauling the old tests, as well as using
> counting in all new tests.
>
> However, even if you take the approach of making 'check-for-diff'
> succeed unconditionally and always count diffs, the current
> implementation is still overly complicated. It would be much simpler
> to let 'check-for-diff' always create the output file, and then use
> "test_line_count = 0 out" when expecting no diffs than to sometimes
> create the output file and sometimes not. The shell '>' operator will
> truncate the file to zero size even before grep is invoked, so you
> don't need to worry that results from an earlier test will pollute
> 'out' for a subsequent test, even if grep finds no matches. Thus,
> 'check-for-diff' collapses to this tiny implementation:
>
>     grep '^diff --git' "$1" >out
>     exit 0
>
> Or, if you want to be terse:
>
>     grep '^diff --git' "$1" || exit 0 >out
>
> A couple notes: First, "out" isn't a great name for this file; perhaps
> come up with a better name. Second, under this scenario,
> "check-for-diff" isn't the most accurate name; it's now simply
> collecting diffs rather than checking for presence.
>
> An alternative would be for the output file to contain the actual
> count of diffs, rather than the diff lines themselves. For instance:
>
>     write_script count-diffs <<\EOF &&
>     grep -c '^diff --git' "$1" >out
>     exit 0
>     EOF
>
> Whether that's actually better is subjective.
>
> So, what's my final word? I quite like the idea of keeping everything
> as simple as possible, thus not altering existing tests and only doing
> line count in the two or three new tests which actually care about the
> number of diffs. However, I can also be sold on retrofitting existing
> tests and having new tests do line counting as a way to improve test
> coverage by catching cases where "-v" incorrectly behaves as "-v -v".
> If you go that route, then the commit message of this patch needs to
> sell it as such (an improvement in test coverage).