From: Jeff King Date: Fri, 15 May 2026 04:44:47 GMT 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