git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Oct 18, 2015, 17:18 UTC
Message-ID
<xmqqwpukayde.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<CAPig+cQ=8FO8yFY4sHUwr0mYuyvMu4d-eizHZeadE9f0BgpXpQ@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
Show 12 quoted lines
> 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.

Show 9 quoted lines
>> +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.

Thanks for your comments.
Previous: Eric SunshineNext: Tobias Klauser
Message 13 of 23 in “stripspace: Implement and use --count-lines option”
  1. 0/4 stripspace: Implement and use --count-lines optionTobias Klauser, Oct 16, 2015
  2. 1/4 strbuf: make stripspace() part of strbufTobias Klauser, Oct 16, 2015
  3. 2/4 stripspace: Use parse-options for command-line parsingTobias Klauser, Oct 16, 2015
  4. Junio C HamanoOct 16, 2015
  5. Junio C HamanoOct 16, 2015
  6. Tobias KlauserOct 17, 2015
  7. Junio C HamanoOct 17, 2015
  8. Tobias KlauserOct 20, 2015
  9. Junio C HamanoOct 20, 2015
  10. Tobias KlauserOct 17, 2015
  11. 3/4 stripspace: Implement --count-lines optionTobias Klauser, Oct 16, 2015
  12. Eric SunshineOct 17, 2015
  13. Junio C HamanoOct 18, 2015
  14. Tobias KlauserOct 19, 2015
  15. Christian CouderOct 19, 2015
  16. Eric SunshineOct 19, 2015
  17. Matthieu MoyOct 19, 2015
  18. Tobias KlauserOct 19, 2015
  19. 4/4 git rebase -i: Use newly added --count-lines option for stripspaceTobias Klauser, Oct 16, 2015
  20. Junio C HamanoOct 16, 2015
  21. Tobias KlauserOct 17, 2015
  22. Matthieu MoyOct 16, 2015
  23. Tobias KlauserOct 17, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.