Re: [PATCH v2 3/4] stripspace: Implement --count-lines option

> On Mon, Oct 19, 2015 at 3:46 PM, Tobias Klauser <tklauser@distanz.ch> wrote:
>> On 2015-10-18 at 19:18:53 +0200, Junio C Hamano <gitster@pobox.com> wrote:
>>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>> > Is there any application beyond git-rebase--interactive where a
>>> > --count-lines options is expected to be useful? It's not obvious from
>>> > the commit message that this change is necessarily a win for later
>>> > porting of git-rebase--interactive to C since the amount of extra code
>>> > and support material added by this patch probably outweighs the amount
>>> > of code a C version of git-rebase--interactive would need to count the
>>> > lines itself.
>>> >
>>> > Stated differently, are the two or three instances of piping through
>>> > 'wc' in git-rebase--interactive sufficient justification for
>>> > introducing extra complexity into git-stripspace and its documentation
>>> > and tests?
>>>
>>> Interesting thought.  When somebody rewrites "rebase -i" in C,
>>> nobody needs to count lines in "stripspace" output.  The rewritten
>>> "rebase -i" would internally run strbuf_stripspace() and the question
>>> becomes what is the best way to let that code find out how many lines
>>> the result contains.
>>>
>>> When viewed from that angle, I agree that "stripspace --count" does
>>> not add anything to further the goal of helping "rebase -i" to move
>>> to C.  Adding strbuf_count_lines() that counts the number of lines
>>> in the given strbuf (if there is no such helper yet; I didn't check),
>>> though.
>>
>> I check before implementing this series and didn't find any helper. I
>> also didn't find any other uses of line counting in the code.
>
> This shows that implementing "git stripspace --count-lines" could
> indirectly help porting "git rebase -i" to C as you could implement
> strbuf_count_lines() for the former and it could then be reused in the
> latter.

>>> >> +test_expect_success '--count-lines with newline only' '
>>> >> +       printf "0\n" >expect &&
>>> >> +       printf "\n" | git stripspace --count-lines >actual &&
>>> >> +       test_cmp expect actual
>>> >> +'
>>> >
>>> > What is the expected behavior when the input is an empty file, a file
>>> > with content but no newline, a file with one or more lines but lacking
>>> > a newline on the final line? Should these cases be tested, as well?
>>>
>>> Good point here, too.  If we were to add strbuf_count_lines()
>>> helper, whoever adds that function needs to take a possible
>>> incomplete line at the end into account.
>>
>> Yes, makes more sense like this (even though it doesn't correspond to
>> what 'wc -l' does).
>
> Tests for "git stripspace --count-lines" would test
> strbuf_count_lines() which would also help when porting git rebase -i
> to C.