[PATCH] trailer: change strbuf in-place in unfold_value()

[PATCH] trailer: change strbuf in-place in unfold_value()

[René Scharfe]

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) {
+			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()

[Ramsay Jones]

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 <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


>  		}
>  	}
> +	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()

[Jeff King]

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

Re: [PATCH] trailer: change strbuf in-place in unfold_value()

[Jeff King]

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

Re: [PATCH] trailer: change strbuf in-place in unfold_value()

[René Scharfe]

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é

[PATCH v2] trailer: change strbuf in-place in unfold_value()

[René Scharfe]

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(-)

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