{"thread":{"id":"65639","subject":"[PATCH] trailer: change strbuf in-place in unfold_value()","startedAt":"2026-05-14T18:41:02Z","lastAt":"2026-05-15T07:33:55Z","messageCount":6,"participants":["René Scharfe","Ramsay Jones","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"543352","messageId":"9629b0c1-b28f-4cd2-8d59-67d909ca9052@web.de","threadId":"65639","inReplyTo":null,"subject":"[PATCH] trailer: change strbuf in-place in unfold_value()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-05-14T18:40:56Z","receivedAt":"2026-05-14T18:41:02Z","isPatch":true,"body":"Avoid an allocation by doing s/\\n\\s*/ /g (replacing NL and any following\nwhitespace with a SP) right in the strbuf instead of copying the result\nto a temporary one and swapping them in the end. We can safely do that\nbecause the replacement is never longer than the original string.\n\nSigned-off-by: René Scharfe \n---\nFormatted with --function-context for easier review.\nInspired by https://lore.kernel.org/git/20260513185408.GA147423@coredump.intra.peff.net/\n\n trailer.c | 16 ++++++----------\n 1 file changed, 6 insertions(+), 10 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 470f86a4a2..b89fa12fe7 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -988,29 +988,25 @@ static int ends_with_blank_line(const char *buf, size_t len)\n \n static void unfold_value(struct strbuf *val)\n {\n-\tstruct strbuf out = STRBUF_INIT;\n \tsize_t i;\n+\tsize_t pos = 0;\n \n-\tstrbuf_grow(&out, val->len);\n \ti = 0;\n \twhile (i < val->len) {\n \t\tchar c = val->buf[i++];\n \t\tif (c == '\\n') {\n \t\t\t/* Collapse continuation down to a single space. */\n \t\t\twhile (i < val->len && isspace(val->buf[i]))\n \t\t\t\ti++;\n-\t\t\tstrbuf_addch(&out, ' ');\n-\t\t} else {\n-\t\t\tstrbuf_addch(&out, c);\n+\t\t\tval->buf[pos++] = ' ';\n+\t\t} else if (pos != i) {\n+\t\t\tval->buf[pos++] = c;\n \t\t}\n \t}\n+\tstrbuf_setlen(val, pos);\n \n \t/* Empty lines may have left us with whitespace cruft at the edges */\n-\tstrbuf_trim(&out);\n-\n-\t/* output goes back to val as if we modified it in-place */\n-\tstrbuf_swap(&out, val);\n-\tstrbuf_release(&out);\n+\tstrbuf_trim(val);\n }\n \n static struct trailer_block *trailer_block_new(void)\n-- \n2.54.0\n\n"},{"id":"543364","messageId":"a4da346d-3800-40ea-8828-970b15088bf3@ramsayjones.plus.com","threadId":"65639","inReplyTo":"9629b0c1-b28f-4cd2-8d59-67d909ca9052@web.de","subject":"Re: [PATCH] trailer: change strbuf in-place in unfold_value()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2026-05-14T21:30:37Z","receivedAt":"2026-05-14T21:33:48Z","isPatch":true,"body":"\n\nOn 14/05/2026 7:40 pm, René Scharfe wrote:\n> Avoid an allocation by doing s/\\n\\s*/ /g (replacing NL and any following\n> whitespace with a SP) right in the strbuf instead of copying the result\n> to a temporary one and swapping them in the end. We can safely do that\n> because the replacement is never longer than the original string.\n> \n> Signed-off-by: René Scharfe \n> ---\n> Formatted with --function-context for easier review.\n> Inspired by https://lore.kernel.org/git/20260513185408.GA147423@coredump.intra.peff.net/\n> \n> trailer.c | 16 ++++++----------\n> 1 file changed, 6 insertions(+), 10 deletions(-)\n> \n> diff --git a/trailer.c b/trailer.c\n> index 470f86a4a2..b89fa12fe7 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -988,29 +988,25 @@ static int ends_with_blank_line(const char *buf, size_t len)\n> \n> static void unfold_value(struct strbuf *val)\n> {\n> -\tstruct strbuf out = STRBUF_INIT;\n> \tsize_t i;\n> +\tsize_t pos = 0;\n> \n> -\tstrbuf_grow(&out, val->len);\n> \ti = 0;\n> \twhile (i < val->len) {\n> \t\tchar c = val->buf[i++];\n> \t\tif (c == '\\n') {\n> \t\t\t/* Collapse continuation down to a single space. */\n> \t\t\twhile (i < val->len && isspace(val->buf[i]))\n> \t\t\t\ti++;\n> -\t\t\tstrbuf_addch(&out, ' ');\n> -\t\t} else {\n> -\t\t\tstrbuf_addch(&out, c);\n> +\t\t\tval->buf[pos++] = ' ';\n> +\t\t} else if (pos != i) {\n\nHmm, isn't 'pos' strictly (always) less than 'i' here? (note the post update\nof 'i' when setting 'c' at the head of the loop).\n\n> +\t\t\tval->buf[pos++] = c;\n\nSo, this (non-newline-or-'trailing'-space char) is always copied.\n\nNot that it matters much (depending on how long the first line is, I doubt\nthe difference is measurable :) ).\n\n[Unless I'm not reading it correctly, of course - in which case, oops!]\n\nATB,\nRamsay Jones\n\n\n> \t\t}\n> \t}\n> +\tstrbuf_setlen(val, pos);\n> \n> \t/* Empty lines may have left us with whitespace cruft at the edges */\n> -\tstrbuf_trim(&out);\n> -\n> -\t/* output goes back to val as if we modified it in-place */\n> -\tstrbuf_swap(&out, val);\n> -\tstrbuf_release(&out);\n> +\tstrbuf_trim(val);\n> }\n> \n> static struct trailer_block *trailer_block_new(void)\n\n"},{"id":"543375","messageId":"20260515044447.GC83595@coredump.intra.peff.net","threadId":"65639","inReplyTo":"a4da346d-3800-40ea-8828-970b15088bf3@ramsayjones.plus.com","subject":"Re: [PATCH] trailer: change strbuf in-place in unfold_value()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-15T04:44:47Z","receivedAt":"2026-05-15T04:44:48Z","isPatch":true,"body":"On Thu, May 14, 2026 at 10:30:37PM +0100, Ramsay Jones wrote:\n\n> > \ti = 0;\n> > \twhile (i < val->len) {\n> > \t\tchar c = val->buf[i++];\n> > \t\tif (c == '\\n') {\n> > \t\t\t/* Collapse continuation down to a single space. */\n> > \t\t\twhile (i < val->len && isspace(val->buf[i]))\n> > \t\t\t\ti++;\n> > -\t\t\tstrbuf_addch(&out, ' ');\n> > -\t\t} else {\n> > -\t\t\tstrbuf_addch(&out, c);\n> > +\t\t\tval->buf[pos++] = ' ';\n> > +\t\t} else if (pos != i) {\n> \n> Hmm, isn't 'pos' strictly (always) less than 'i' here? (note the post update\n> of 'i' when setting 'c' at the head of the loop).\n> \n> > +\t\t\tval->buf[pos++] = c;\n> \n> So, this (non-newline-or-'trailing'-space char) is always copied.\n> \n> Not that it matters much (depending on how long the first line is, I doubt\n> the difference is measurable :) ).\n> \n> [Unless I'm not reading it correctly, of course - in which case, oops!]\n\nYeah, I think you're right. If it were a for-loop which incremented \"i\"\nat the end then the comparison could make sense. But even then, I think\nusually in such modify-in-place loops we don't bother trying to skip\nself-assignment (e.g., see remove_space() in builtin/patch-id.c). In\npractice I don't know which is worse: the extra branch or a pointless\nmemory store.\n\n-Peff\n"},{"id":"543376","messageId":"20260515044703.GD83595@coredump.intra.peff.net","threadId":"65639","inReplyTo":"9629b0c1-b28f-4cd2-8d59-67d909ca9052@web.de","subject":"Re: [PATCH] trailer: change strbuf in-place in unfold_value()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-15T04:47:03Z","receivedAt":"2026-05-15T04:47:04Z","isPatch":true,"body":"On Thu, May 14, 2026 at 08:40:56PM +0200, René Scharfe wrote:\n\n> Avoid an allocation by doing s/\\n\\s*/ /g (replacing NL and any following\n> whitespace with a SP) right in the strbuf instead of copying the result\n> to a temporary one and swapping them in the end. We can safely do that\n> because the replacement is never longer than the original string.\n>\n> [...]\n>\n> Inspired by https://lore.kernel.org/git/20260513185408.GA147423@coredump.intra.peff.net/\n\nCute. Modulo the issue raised by Ramsay, this looks correct to me. In\nthe discussion you referenced I was mostly expecting people to find\nspots where the solution would be to just remove the strbuf_grow() call.\nThis one is quite a bit trickier, and I am glad to have somebody careful\nlooking at it. ;)\n\n-Peff\n"},{"id":"543379","messageId":"0b673b25-1f0e-44f5-b24c-7f7183d58cee@web.de","threadId":"65639","inReplyTo":"a4da346d-3800-40ea-8828-970b15088bf3@ramsayjones.plus.com","subject":"Re: [PATCH] trailer: change strbuf in-place in unfold_value()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-05-15T06:47:25Z","receivedAt":"2026-05-15T06:47:33Z","isPatch":true,"body":"On 5/14/26 11:30 PM, Ramsay Jones wrote:\n> \n>> diff --git a/trailer.c b/trailer.c\n>> index 470f86a4a2..b89fa12fe7 100644\n>> --- a/trailer.c\n>> +++ b/trailer.c\n>> @@ -988,29 +988,25 @@ static int ends_with_blank_line(const char *buf, size_t len)\n>> \n>> static void unfold_value(struct strbuf *val)\n>> {\n>> -\tstruct strbuf out = STRBUF_INIT;\n>> \tsize_t i;\n>> +\tsize_t pos = 0;\n>> \n>> -\tstrbuf_grow(&out, val->len);\n>> \ti = 0;\n>> \twhile (i < val->len) {\n>> \t\tchar c = val->buf[i++];\n>> \t\tif (c == '\\n') {\n>> \t\t\t/* Collapse continuation down to a single space. */\n>> \t\t\twhile (i < val->len && isspace(val->buf[i]))\n>> \t\t\t\ti++;\n>> -\t\t\tstrbuf_addch(&out, ' ');\n>> -\t\t} else {\n>> -\t\t\tstrbuf_addch(&out, c);\n>> +\t\t\tval->buf[pos++] = ' ';\n>> +\t\t} else if (pos != i) {\n> \n> Hmm, isn't 'pos' strictly (always) less than 'i' here? (note the post update\n> of 'i' when setting 'c' at the head of the loop).\n\nAh, yes, good find. Initially I used a for loop which incremented i\nonly at the end, but converted it back to minimize the patch and\nforgot to adjust this comparison.\n\nRené\n\n"},{"id":"543381","messageId":"816be07e-2cd6-48fe-ae93-57fa0f2543ed@web.de","threadId":"65639","inReplyTo":"9629b0c1-b28f-4cd2-8d59-67d909ca9052@web.de","subject":"[PATCH v2] trailer: change strbuf in-place in unfold_value()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-05-15T07:33:53Z","receivedAt":"2026-05-15T07:33:55Z","isPatch":true,"body":"Avoid an allocation by doing s/\\n\\s*/ /g (replacing NL and any following\nwhitespace with a SP) right in the strbuf instead of copying the result\nto a temporary one and swapping them in the end. We can safely do that\nbecause the replacement is never longer than the original string.\n\nSigned-off-by: René Scharfe \n---\nFormatted with --function-context for easier review.\nChanges since v1:\n- Removed always-true comparison.\n\n trailer.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 470f86a4a2..6d8ec7fa8d 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -988,29 +988,24 @@ static int ends_with_blank_line(const char *buf, size_t len)\n \n static void unfold_value(struct strbuf *val)\n {\n-\tstruct strbuf out = STRBUF_INIT;\n \tsize_t i;\n+\tsize_t pos = 0;\n \n-\tstrbuf_grow(&out, val->len);\n \ti = 0;\n \twhile (i < val->len) {\n \t\tchar c = val->buf[i++];\n \t\tif (c == '\\n') {\n \t\t\t/* Collapse continuation down to a single space. */\n \t\t\twhile (i < val->len && isspace(val->buf[i]))\n \t\t\t\ti++;\n-\t\t\tstrbuf_addch(&out, ' ');\n-\t\t} else {\n-\t\t\tstrbuf_addch(&out, c);\n+\t\t\tc = ' ';\n \t\t}\n+\t\tval->buf[pos++] = c;\n \t}\n+\tstrbuf_setlen(val, pos);\n \n \t/* Empty lines may have left us with whitespace cruft at the edges */\n-\tstrbuf_trim(&out);\n-\n-\t/* output goes back to val as if we modified it in-place */\n-\tstrbuf_swap(&out, val);\n-\tstrbuf_release(&out);\n+\tstrbuf_trim(val);\n }\n \n static struct trailer_block *trailer_block_new(void)\n\nInterdiff against v1:\n diff --git a/trailer.c b/trailer.c\n index b89fa12fe7..6d8ec7fa8d 100644\n --- a/trailer.c\n +++ b/trailer.c\n @@ -989,22 +989,21 @@ static int ends_with_blank_line(const char *buf, size_t len)\n static void unfold_value(struct strbuf *val)\n {\n \tsize_t i;\n \tsize_t pos = 0;\n \n \ti = 0;\n \twhile (i < val->len) {\n \t\tchar c = val->buf[i++];\n \t\tif (c == '\\n') {\n \t\t\t/* Collapse continuation down to a single space. */\n \t\t\twhile (i < val->len && isspace(val->buf[i]))\n \t\t\t\ti++;\n -\t\t\tval->buf[pos++] = ' ';\n -\t\t} else if (pos != i) {\n -\t\t\tval->buf[pos++] = c;\n +\t\t\tc = ' ';\n \t\t}\n +\t\tval->buf[pos++] = c;\n \t}\n \tstrbuf_setlen(val, pos);\n \n \t/* Empty lines may have left us with whitespace cruft at the edges */\n \tstrbuf_trim(val);\n }\n-- \n2.54.0\n"}]}