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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Oct 19, 2015 at 3:46 PM, Tobias Klauser <tklauser@xxxxxxxxxx> wrote:
> On 2015-10-18 at 19:18:53 +0200, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.

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

It would be sad in my opinion.

>> >> +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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]