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

[Tobias Klauser <tklauser@distanz.ch>]

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.

After considering your and Eric's reply, I'll drop these patches for
now and only resubmit patches 1/4 and 2/4 for v3 (also see my reply to
Eric).

> >> +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).