Re: [PATCH] trailer: change strbuf in-place in unfold_value()
- From
Jeff King <peff@peff.net>
- Date
- May 15, 2026, 04:44 UTC
- Message-ID
- <20260515044447.GC83595@coredump.intra.peff.net>
- In-Reply-To
- <a4da346d-3800-40ea-8828-970b15088bf3@ramsayjones.plus.com>
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