Subject: Re: [PATCH v8 04/15] bugreport: gather git version and build info

From: Junio C Hamano <[email-masked]>
Date: Thu, 20 Feb 2020 15:18:14 -0800

Emily Shaffer <[email-masked]> writes:

> On Thu, Feb 20, 2020 at 12:07:46PM -0800, Junio C Hamano wrote:
>> Emily Shaffer <[email-masked]> writes:
>> 
>> > +static void get_system_info(struct strbuf *sys_info)
>> > +{
>> > +	/* get git version from native cmd */
>> > +	strbuf_addstr(sys_info, "git version:\n");
>> > +	get_version_info(sys_info, 1);
>> > +	strbuf_complete_line(sys_info);
>> 
>> It is a bit curious use of "don't do anything if sys_info ends with
>> a complete line, but complete it if it ends with an imcomplete
>> line".  That tells the readers that we do not know what
>> get_version_info() will do (now or in the future) to its output
>> buffer.
>>  ...
>> So, was the strbuf_complete_line() merely defensive programming?  It
>> may deserve a comment if it will stay there.
>
> It was meant defensively, here and elsewhere in the series. I figured
> that for something like this, which is mostly bounded by human writing
> in an editor and then by file IO, spurious string-checking was not such
> a big deal.
>
> Are you suggesting to comment around the strbuf_complete_line() calls,
> or to comment around get_version_info() that it should end in newline?

I meant a comment in get_system_info() next to the use of this
particular use of strbuf_complete_line(), if the use stays there.

But after reading the whole series through, I saw no need to use
strbuf_complete_line() in the first place, as there is no source of
input that is not under our control (if we do not count sloppy
programming, that is).