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

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Oct 19, 2015 at 1:03 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> 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.

In this project, where all user-facing functionality must be supported
for the life of the project, each new command, command-line option,
configuration setting, and environment variable exacts additional
costs beyond the initial implementation cost. With this in mind, my
question was also indirectly asking whether there was sufficient
justification of the long-term cost of a --count-lines option. The
argument that --count-lines would help test a proposed
strbuf_count_lines() likely does not outweigh that cost.

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

Rather than saddling the project with the cost of a new user-facing,
but otherwise unneeded option, a more direct way to test the proposed
strbuf_count_lines() would be to add a test-strbuf program, akin to
test-config, test-string-list, etc. This has the added benefit of
providing a home for strbuf-based tests beyond line counting.