patchtrailer: change strbuf in-place in unfold_value()
6 messages between May 14, 2026 and May 15, 2026, from René Scharfe, Ramsay Jones, Jeff King.
Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.
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 <l.s.r@web.de>
---
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(-)
Show changes to trailer.c +6 −10
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
Re: [PATCH] trailer: change strbuf in-place in unfold_value()
On 14/05/2026 7:40 pm, René Scharfe wrote:
Show 38 quoted lines
> 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 <l.s.r@web.de>
> ---
> 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
Show 14 quoted lines
> }
> }
> + 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)
Re: [PATCH] trailer: change strbuf in-place in unfold_value()
On Thu, May 14, 2026 at 10:30:37PM +0100, Ramsay Jones wrote:
Show 24 quoted lines
> > 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
Re: [PATCH] trailer: change strbuf in-place in unfold_value()
On Thu, May 14, 2026 at 08:40:56PM +0200, René Scharfe wrote:
Show 8 quoted lines
> 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
Re: [PATCH] trailer: change strbuf in-place in unfold_value()
On 5/14/26 11:30 PM, Ramsay Jones wrote:
Show 29 quoted lines
>
>> 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é
[PATCH v2] trailer: change strbuf in-place in unfold_value()
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 <l.s.r@web.de>
---
Formatted with --function-context for easier review.
Changes since v1:
- Removed always-true comparison.
trailer.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
Show changes to trailer.c +5 −10
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