# [PATCH] trailer: change strbuf in-place in unfold_value() 6 messages from 2026-05-14 to 2026-05-15. Participants: René Scharfe, Ramsay Jones, Jeff King. Thread: https://gitlist.dev/t/65639 ## René Scharfe, 2026-05-14 18:40 Subject: [PATCH] trailer: change strbuf in-place in unfold_value() Message-ID: <9629b0c1-b28f-4cd2-8d59-67d909ca9052@web.de> ``` Avoid an allocation by doing s/\n\s*/ /g (replacing NL and any following whitespace with a SP) right in the strbuf instead of copying the result to a temporary one and swapping them in the end. We can safely do that because the replacement is never longer than the original string. Signed-off-by: René Scharfe --- Formatted with --function-context for easier review. Inspired by https://lore.kernel.org/git/20260513185408.GA147423@coredump.intra.peff.net/ trailer.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/trailer.c b/trailer.c index 470f86a4a2..b89fa12fe7 100644 --- a/trailer.c +++ b/trailer.c @@ -988,29 +988,25 @@ static int ends_with_blank_line(const char *buf, size_t len) static void unfold_value(struct strbuf *val) { - struct strbuf out = STRBUF_INIT; size_t i; + size_t pos = 0; - strbuf_grow(&out, val->len); i = 0; while (i < val->len) { char c = val->buf[i++]; if (c == '\n') { /* Collapse continuation down to a single space. */ while (i < val->len && isspace(val->buf[i])) i++; - strbuf_addch(&out, ' '); - } else { - strbuf_addch(&out, c); + val->buf[pos++] = ' '; + } else if (pos != i) { + val->buf[pos++] = c; } } + strbuf_setlen(val, pos); /* Empty lines may have left us with whitespace cruft at the edges */ - strbuf_trim(&out); - - /* output goes back to val as if we modified it in-place */ - strbuf_swap(&out, val); - strbuf_release(&out); + strbuf_trim(val); } static struct trailer_block *trailer_block_new(void) -- 2.54.0 ``` ## Ramsay Jones, 2026-05-14 21:30 Subject: Re: [PATCH] trailer: change strbuf in-place in unfold_value() Message-ID: In-Reply-To: <9629b0c1-b28f-4cd2-8d59-67d909ca9052@web.de> ``` On 14/05/2026 7:40 pm, René Scharfe wrote: > Avoid an allocation by doing s/\n\s*/ /g (replacing NL and any following > whitespace with a SP) right in the strbuf instead of copying the result > to a temporary one and swapping them in the end. We can safely do that > because the replacement is never longer than the original string. > > Signed-off-by: René Scharfe > --- > Formatted with --function-context for easier review. > Inspired by https://lore.kernel.org/git/20260513185408.GA147423@coredump.intra.peff.net/ > > trailer.c | 16 ++++++---------- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/trailer.c b/trailer.c > index 470f86a4a2..b89fa12fe7 100644 > --- a/trailer.c > +++ b/trailer.c > @@ -988,29 +988,25 @@ static int ends_with_blank_line(const char *buf, size_t len) > > static void unfold_value(struct strbuf *val) > { > - struct strbuf out = STRBUF_INIT; > size_t i; > + size_t pos = 0; > > - strbuf_grow(&out, val->len); > i = 0; > while (i < val->len) { > char c = val->buf[i++]; > if (c == '\n') { > /* Collapse continuation down to a single space. */ > while (i < val->len && isspace(val->buf[i])) > i++; > - strbuf_addch(&out, ' '); > - } else { > - strbuf_addch(&out, c); > + val->buf[pos++] = ' '; > + } else if (pos != i) { Hmm, isn't 'pos' strictly (always) less than 'i' here? (note the post update of 'i' when setting 'c' at the head of the loop). > + val->buf[pos++] = c; So, this (non-newline-or-'trailing'-space char) is always copied. Not that it matters much (depending on how long the first line is, I doubt the difference is measurable :) ). [Unless I'm not reading it correctly, of course - in which case, oops!] ATB, Ramsay Jones > } > } > + strbuf_setlen(val, pos); > > /* Empty lines may have left us with whitespace cruft at the edges */ > - strbuf_trim(&out); > - > - /* output goes back to val as if we modified it in-place */ > - strbuf_swap(&out, val); > - strbuf_release(&out); > + strbuf_trim(val); > } > > static struct trailer_block *trailer_block_new(void) ``` ## Jeff King, 2026-05-15 04:44 Subject: Re: [PATCH] trailer: change strbuf in-place in unfold_value() Message-ID: <20260515044447.GC83595@coredump.intra.peff.net> In-Reply-To: ``` On Thu, May 14, 2026 at 10:30:37PM +0100, Ramsay Jones wrote: > > i = 0; > > while (i < val->len) { > > char c = val->buf[i++]; > > if (c == '\n') { > > /* Collapse continuation down to a single space. */ > > while (i < val->len && isspace(val->buf[i])) > > i++; > > - strbuf_addch(&out, ' '); > > - } else { > > - strbuf_addch(&out, c); > > + val->buf[pos++] = ' '; > > + } else if (pos != i) { > > Hmm, isn't 'pos' strictly (always) less than 'i' here? (note the post update > of 'i' when setting 'c' at the head of the loop). > > > + val->buf[pos++] = c; > > So, this (non-newline-or-'trailing'-space char) is always copied. > > Not that it matters much (depending on how long the first line is, I doubt > the difference is measurable :) ). > > [Unless I'm not reading it correctly, of course - in which case, oops!] Yeah, I think you're right. If it were a for-loop which incremented "i" at the end then the comparison could make sense. But even then, I think usually in such modify-in-place loops we don't bother trying to skip self-assignment (e.g., see remove_space() in builtin/patch-id.c). In practice I don't know which is worse: the extra branch or a pointless memory store. -Peff ``` ## Jeff King, 2026-05-15 04:47 Subject: Re: [PATCH] trailer: change strbuf in-place in unfold_value() Message-ID: <20260515044703.GD83595@coredump.intra.peff.net> In-Reply-To: <9629b0c1-b28f-4cd2-8d59-67d909ca9052@web.de> ``` On Thu, May 14, 2026 at 08:40:56PM +0200, René Scharfe wrote: > Avoid an allocation by doing s/\n\s*/ /g (replacing NL and any following > whitespace with a SP) right in the strbuf instead of copying the result > to a temporary one and swapping them in the end. We can safely do that > because the replacement is never longer than the original string. > > [...] > > Inspired by https://lore.kernel.org/git/20260513185408.GA147423@coredump.intra.peff.net/ Cute. Modulo the issue raised by Ramsay, this looks correct to me. In the discussion you referenced I was mostly expecting people to find spots where the solution would be to just remove the strbuf_grow() call. This one is quite a bit trickier, and I am glad to have somebody careful looking at it. ;) -Peff ``` ## René Scharfe, 2026-05-15 06:47 Subject: Re: [PATCH] trailer: change strbuf in-place in unfold_value() Message-ID: <0b673b25-1f0e-44f5-b24c-7f7183d58cee@web.de> In-Reply-To: ``` On 5/14/26 11:30 PM, Ramsay Jones wrote: > >> diff --git a/trailer.c b/trailer.c >> index 470f86a4a2..b89fa12fe7 100644 >> --- a/trailer.c >> +++ b/trailer.c >> @@ -988,29 +988,25 @@ static int ends_with_blank_line(const char *buf, size_t len) >> >> static void unfold_value(struct strbuf *val) >> { >> - struct strbuf out = STRBUF_INIT; >> size_t i; >> + size_t pos = 0; >> >> - strbuf_grow(&out, val->len); >> i = 0; >> while (i < val->len) { >> char c = val->buf[i++]; >> if (c == '\n') { >> /* Collapse continuation down to a single space. */ >> while (i < val->len && isspace(val->buf[i])) >> i++; >> - strbuf_addch(&out, ' '); >> - } else { >> - strbuf_addch(&out, c); >> + val->buf[pos++] = ' '; >> + } else if (pos != i) { > > Hmm, isn't 'pos' strictly (always) less than 'i' here? (note the post update > of 'i' when setting 'c' at the head of the loop). Ah, yes, good find. Initially I used a for loop which incremented i only at the end, but converted it back to minimize the patch and forgot to adjust this comparison. René ``` ## René Scharfe, 2026-05-15 07:33 Subject: [PATCH v2] trailer: change strbuf in-place in unfold_value() Message-ID: <816be07e-2cd6-48fe-ae93-57fa0f2543ed@web.de> In-Reply-To: <9629b0c1-b28f-4cd2-8d59-67d909ca9052@web.de> ``` Avoid an allocation by doing s/\n\s*/ /g (replacing NL and any following whitespace with a SP) right in the strbuf instead of copying the result to a temporary one and swapping them in the end. We can safely do that because the replacement is never longer than the original string. Signed-off-by: René Scharfe --- Formatted with --function-context for easier review. Changes since v1: - Removed always-true comparison. trailer.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/trailer.c b/trailer.c index 470f86a4a2..6d8ec7fa8d 100644 --- a/trailer.c +++ b/trailer.c @@ -988,29 +988,24 @@ static int ends_with_blank_line(const char *buf, size_t len) static void unfold_value(struct strbuf *val) { - struct strbuf out = STRBUF_INIT; size_t i; + size_t pos = 0; - strbuf_grow(&out, val->len); i = 0; while (i < val->len) { char c = val->buf[i++]; if (c == '\n') { /* Collapse continuation down to a single space. */ while (i < val->len && isspace(val->buf[i])) i++; - strbuf_addch(&out, ' '); - } else { - strbuf_addch(&out, c); + c = ' '; } + val->buf[pos++] = c; } + strbuf_setlen(val, pos); /* Empty lines may have left us with whitespace cruft at the edges */ - strbuf_trim(&out); - - /* output goes back to val as if we modified it in-place */ - strbuf_swap(&out, val); - strbuf_release(&out); + strbuf_trim(val); } static struct trailer_block *trailer_block_new(void) Interdiff against v1: diff --git a/trailer.c b/trailer.c index b89fa12fe7..6d8ec7fa8d 100644 --- a/trailer.c +++ b/trailer.c @@ -989,22 +989,21 @@ static int ends_with_blank_line(const char *buf, size_t len) static void unfold_value(struct strbuf *val) { size_t i; size_t pos = 0; i = 0; while (i < val->len) { char c = val->buf[i++]; if (c == '\n') { /* Collapse continuation down to a single space. */ while (i < val->len && isspace(val->buf[i])) i++; - val->buf[pos++] = ' '; - } else if (pos != i) { - val->buf[pos++] = c; + c = ' '; } + val->buf[pos++] = c; } strbuf_setlen(val, pos); /* Empty lines may have left us with whitespace cruft at the edges */ strbuf_trim(val); } -- 2.54.0 ```