Re: [PATCH 02/11] t8001/t8002: blame: demonstrate -L bounds checking bug

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 5, 2013, 23:30 UTC
Message-ID: <7vk3jziulg.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAPig+cRYASsHyujx3uhHtRqSnfC8e1cAEMAa2thS3mgOyF2Ozw@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 12 quoted lines

>> The problem exists with and without the incomplete line. The comment
>> mentioning "incomplete line" and "wc" was inserted to explain why it
>> was necessary to add one to the result of wc, which might not
>> otherwise be obvious.
>
> Would the comment be clearer if phrased like this?
>
>   # We want to test -LX where X is the last line of the file, so we need
>   # to compute the number of lines in the file, which normally would be
>   # done via 'wc -l'.  In this case, however, the last line of 'file' is
>   # incomplete, so 'wc' reports one fewer than the actual line count. To
>   # adjust for this anomaly, we must add one to the result of 'wc'.

If the patch picked a place where the test vector does not have to involve a file that ends with an incomplete line, the test would not have had to do anything tricky that required such a comment to explain why it is doing something mysterious.

Such a change would be much cleaner and may be worth the effort, but I do not think just rewording the comment is worth the bother.

I didn't see if there is such a place in the existing test sequence, though.