Re: [PATCH v1] convert log_ref_write_fd() to use strbuf

[Ben Peart <peartben@gmail.com>]

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 <peff@peff.net> 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.