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

[Jeff King <peff@peff.net>]

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