From: Ben Peart Date: Tue, 10 Jul 2018 20:57:09 GMT Subject: Re: [PATCH v1] convert log_ref_write_fd() to use strbuf Message-ID: <9666fa00-607c-a212-3104-a9ef8ff2a60b@gmail.com> In-Reply-To: <20180710202128.GA6886@sigill.intra.peff.net> On 7/10/2018 4:21 PM, Jeff King wrote: > On Tue, Jul 10, 2018 at 12:41:52PM -0700, Junio C Hamano wrote: > >> Jeff King writes: >> >>>> - while (buf < cp && isspace(cp[-1])) >>>> - cp--; >>>> - *cp++ = '\n'; >>>> - return cp - buf; >>>> + strbuf_rtrim(sb); >>> >>> Using rtrim is a nice reduction in complexity. A pure translation would >>> include a final strbuf_addch(sb, '\n'). It looks like you moved that to >>> the caller. There's only one, so that's OK now, but it may affect topics >>> in flight (and I do in fact have an old topic that calls it). >>> >>> But I think it's OK, as the change in function signature means that any >>> callers will need updated anyway. So there's little risk of a silent >>> mis-merge. >> >> It is interesting that we came to a slightly different conclusion, >> after doing pretty much the same analysis ;-). Unless Ben has a >> plan to use a version that trims the trailing LF elsewhere, there is >> no point changing what the function does, especially because the >> existing and only caller does want the terminating LF at the end. > > The original actually does a funny thing. It writes the newline into the > buffer, and then maybe calls copy_reflog_msg(). If it does, then we > actually subtract one from the length we feed to the function, to roll > back over the newline. That's harder to do with a strbuf, as those kinds > of manual length shenanigans are discouraged (you'd use strbuf_setlen() > to roll it back). At which point, you are much better off not adding it > in the first place, and building the whole thing sequentially: > > 1. add the early bits that are in all entries > > 2. (maybe) add the tab and message if there is one > > 3. add the trailing newline > > And that's exactly what Ben's patch does. > > So I think the end result is much cleaner that way. My concern was just > that the function semantics were changed. > > -Peff > And that is exactly why I ended up moving the logic to append the newline out to the caller. I wrote it the other way first but it was pretty messy - since there were no other callers, it was cleaner/simpler to move it out. :) For any future callers, it is pretty trivial to add the call to strbuf_addch(&sb, '\n') if they want a trailing newline.