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

Re: [PATCH v2 4/7] string-list: optionally trim string pieces split by string_list_split*()

From
shejialuo <shejialuo@gmail.com>
Date
Aug 2, 2025, 01:51 UTC
Message-ID
<aI1vEzuD2DeiFSzW@mbp>
In-Reply-To
<xmqqa54iljw1.fsf@gitster.g>
On Fri, Aug 01, 2025 at 04:09:02PM -0700, Junio C Hamano wrote:
Show 27 quoted lines
> shejialuo <shejialuo@gmail.com> writes:
> 
> > I agree with you that we would introduce another variable. However, the
> > thing I quite dislike is that we do ltrim inside the current function
> > and we do rtrim inside `append_one`.
> 
> Your "quite dislike" does not matter unless backed by a reason why
> it is not good, and for that, you need to think a bit deeper.  Then
> you will hopefully appreciate why the current arrangement is more
> optimal ;-)
> 
> There is a clear separation of tasks between this caller-callee
> pair.  The caller is responsible for finding where the current token
> ends, and the callee is responsible for massaging the current token
> into the resulting list.
> 
> But ltrim needs to be done in the caller for this to be efficient.
> 
> Imagine the case where you want to allow both non-empty and trim
> behaviour at the same time, and use a whitespace character as
> delimiter.  If your token has leading whitespaces, instead of
> chopping them into zero-length ranges and feeding it to append_one()
> one by one, only to have them discarded (due to non-empty being
> set), ltrimming in the caller before it decides where the next token
> (i.e. "end") starts is far more efficient.  It may be more
> conceptually cleaner, but cleanliness is more subjective ;-)
> 
Yes, I agree. Thanks for the wonderful explanation.
Show 15 quoted lines
> > My thinking is that we should handle the [p, end) string in the same
> > place.
> 
> Again this sounds no more than a subjective "quite dislike".  Is
> there a reason why anybody would want to insist these two things be
> done at the same location?
> 
> You could satisfy the subjective "same place requirement" by
> inlining the helper function into its sole caller and still keep the
> current arrangement to ltrim before finding "end", of course.  But
> at that point, I would have to say that it is a tail wagging the
> dog.  You are making the code worse by destroying the caller-callee
> division of responsibilities, only to satisfy a subjetive "quite
> dislike" criteria.
> 

I get your point. In the later review, I think I should avoid commenting things using a subjetive idea. Really thanks for your suggestion.

Thanks,
Previous: Junio C HamanoNext: Patrick Steinhardt
Message 25 of 72 in “string_list_split*() updates”
  1. 0/5 string_list_split*() updatesJunio C Hamano, Jul 31, 2025
  2. 1/5 string-list: report programming error with BUGJunio C Hamano, Jul 31, 2025
  3. Eric SunshineJul 31, 2025
  4. Junio C HamanoJul 31, 2025
  5. 2/5 string-list: align string_list_split() with its _in_place() counterpartJunio C Hamano, Jul 31, 2025
  6. Eric SunshineJul 31, 2025
  7. 3/5 string-list: unify string_list_split* functionsJunio C Hamano, Jul 31, 2025
  8. 4/5 string-list: optionally trim string pieces split by string_list_split()Junio C Hamano, Jul 31, 2025
  9. 5/5 diff: simplify parsing of diff.colormovedwsJunio C Hamano, Jul 31, 2025
  10. Eric SunshineJul 31, 2025
  11. 0/7 string_list_split*() updatesJunio C Hamano, Jul 31, 2025
  12. 1/7 string-list: report programming error with BUGJunio C Hamano, Jul 31, 2025
  13. 2/7 string-list: align string_list_split() with its _in_place() counterpartJunio C Hamano, Jul 31, 2025
  14. shejialuoAug 1, 2025
  15. Junio C HamanoAug 1, 2025
  16. shejialuoAug 1, 2025
  17. Junio C HamanoAug 1, 2025
  18. 3/7 string-list: unify string_list_split* functionsJunio C Hamano, Jul 31, 2025
  19. shejialuoAug 1, 2025
  20. 4/7 string-list: optionally trim string pieces split by string_list_split*()Junio C Hamano, Jul 31, 2025
  21. shejialuoAug 1, 2025
  22. Junio C HamanoAug 1, 2025
  23. shejialuoAug 1, 2025
  24. Junio C HamanoAug 1, 2025
  25. shejialuoAug 2, 2025
  26. Patrick SteinhardtAug 1, 2025
  27. Junio C HamanoAug 1, 2025
  28. 5/7 diff: simplify parsing of diff.colormovedwsJunio C Hamano, Jul 31, 2025
  29. Patrick SteinhardtAug 1, 2025
  30. 6/7 string-list: optionally omit empty string pieces in string_list_split*()Junio C Hamano, Jul 31, 2025
  31. Eric SunshineJul 31, 2025
  32. shejialuoAug 1, 2025
  33. Patrick SteinhardtAug 1, 2025
  34. Junio C HamanoAug 1, 2025
  35. 7/7 string-list: split-then-remove-empty can be done while splittingJunio C Hamano, Jul 31, 2025
  36. Patrick SteinhardtAug 1, 2025
  37. 0/7 string_list_split*() updatesJunio C Hamano, Aug 1, 2025
  38. 1/7 string-list: report programming error with BUGJunio C Hamano, Aug 1, 2025
  39. 2/7 string-list: align string_list_split() with its _in_place() counterpartJunio C Hamano, Aug 1, 2025
  40. Jeff KingAug 2, 2025
  41. Junio C HamanoAug 2, 2025
  42. Jeff KingAug 2, 2025
  43. 3/7 string-list: unify string_list_split* functionsJunio C Hamano, Aug 1, 2025
  44. 4/7 string-list: optionally trim string pieces split by string_list_split*()Junio C Hamano, Aug 1, 2025
  45. Jeff KingAug 2, 2025
  46. Junio C HamanoAug 2, 2025
  47. Jeff KingAug 2, 2025
  48. 5/7 diff: simplify parsing of diff.colormovedwsJunio C Hamano, Aug 1, 2025
  49. 6/7 string-list: optionally omit empty string pieces in string_list_split*()Junio C Hamano, Aug 1, 2025
  50. 7/7 string-list: split-then-remove-empty can be done while splittingJunio C Hamano, Aug 1, 2025
  51. 0/7 string_list_split*() updatesJunio C Hamano, Aug 3, 2025
  52. 1/7 string-list: report programming error with BUGJunio C Hamano, Aug 3, 2025
  53. 2/7 string-list: align string_list_split() with its _in_place() counterpartJunio C Hamano, Aug 3, 2025
  54. 3/7 string-list: unify string_list_split* functionsJunio C Hamano, Aug 3, 2025
  55. 4/7 string-list: optionally trim string pieces split by string_list_split*()Junio C Hamano, Aug 3, 2025
  56. 5/7 diff: simplify parsing of diff.colormovedwsJunio C Hamano, Aug 3, 2025
  57. 6/7 string-list: optionally omit empty string pieces in string_list_split*()Junio C Hamano, Aug 3, 2025
  58. 7/7 string-list: split-then-remove-empty can be done while splittingJunio C Hamano, Aug 3, 2025
  59. Patrick SteinhardtAug 4, 2025
  60. 00/12 do not overuse strbuf_split*()Junio C Hamano, Aug 3, 2025
  61. 01/12 wt-status: avoid strbuf_split*()Junio C Hamano, Aug 3, 2025
  62. 02/12 clean: do not pass strbuf by valueJunio C Hamano, Aug 3, 2025
  63. 03/12 clean: do not use strbuf_split*() [part 1]Junio C Hamano, Aug 3, 2025
  64. 04/12 clean: do not pass the whole structure when it is not necessaryJunio C Hamano, Aug 3, 2025
  65. 05/12 clean: do not use strbuf_split*() [part 2]Junio C Hamano, Aug 3, 2025
  66. 06/12 merge-tree: do not use strbuf_split*()Junio C Hamano, Aug 3, 2025
  67. 07/12 notes: do not use strbuf_split*()Junio C Hamano, Aug 3, 2025
  68. 08/12 config: do not use strbuf_split()Junio C Hamano, Aug 3, 2025
  69. 09/12 environment: do not use strbuf_split*()Junio C Hamano, Aug 3, 2025
  70. 10/12 sub-process: do not use strbuf_split*()Junio C Hamano, Aug 3, 2025
  71. 11/12 trace2: trim_trailing_newline followed by trim is a no-opJunio C Hamano, Aug 3, 2025
  72. 12/12 trace2: do not use strbuf_split*()Junio C Hamano, Aug 3, 2025

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.