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