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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 10, 2018, 19:41 UTC
Message-ID: <xmqqsh4q6fpb.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180710184534.GA27535@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 14 quoted lines

>> -	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.