Re: [PATCH 2/5] t6036, t6042: use test_line_count instead of wc -l

[Elijah Newren <newren@gmail.com>]

On Thu, May 24, 2018 at 6:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>>> -            test 2 = $(git ls-files -s | wc -l) &&
>>> -            test 2 = $(git ls-files -u | wc -l) &&
>>> -            test 2 = $(git ls-files -o | wc -l) &&
>>
>> Here 'git ls-files -o' should have listed two untracked files ...
>>
>>> +            git ls-files -s >out &&
>>> +            test_line_count = 2 out &&
>>> +            git ls-files -u >out &&
>>> +            test_line_count = 2 out &&
>>> +            git ls-files -o >out &&
>>> +            test_line_count = 3 out &&
>>
>> ... but now you expect it to list three.  I was about to point out the
>> typo, but then noticed that you expect it to list one more untracked
>> file than before in all subsequent tests...  now that can't be just a
>> typo, can it?
>>
>> Please mention in the commit message that when using an intermediate
>> file to store the output, 'git ls-files -o' will list that file, too,
>> that's why the number of expected untracked files had to be adjusted;
>> so future readers won't have to figure this out themselves.
>
> I'd expect that a reader of the commit who cares enough to bother to
> wonder by looking at the patch and seeing that 2 became 3 would know
> why already.  And a reader of the resulting file would not know that
> the 3 used to be 2, and won't be helped by "we used to count to 2,
> now we have 'out' also counted" that much, especially in the commit
> log message.  What would help the latter would be to name which
> three paths we expect to see in the comment (or test against the
> exact list of paths, instead of using test_line_count).
>
>> An alternative to consider would be to add a .gitignore file in the
>> initial commit to ignore 'out', then the number of untracked files
>> don't have to be adjusted.
>
> I think that is a preferred solution that we've used in ls-files and
> status tests successfully.

...except that if we add a .gitignore to each initial commit (we use
test_create_repo for nearly every test to keep them separable meaning
we'd have to do this many times), then four lines above we have to
adjust the number of expected tracked files.  And, for it to work,
we'd have to add an --exclude-standard flag to ls-files -o.

I can make that change if you both want it, but it seems like it's
actually making it harder to follow the changes rather than easier.