Re: [PATCH v4 03/15] bugreport: gather git version and build info

From: Emily Shaffer <emilyshaffer@google.com>
Date: Dec 20, 2019, 01:25 UTC
Message-ID: <20191220012524.GB227872@google.com>
In-Reply-To: <xmqqfthig07m.fsf@gitster-ct.c.googlers.com>

On Tue, Dec 17, 2019 at 12:34:53PM -0800, Junio C Hamano wrote:

Show 25 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> +	if (build_options) {
> >> +		strbuf_addf(buf, "cpu: %s\n", GIT_HOST_CPU);
> >> +		if (git_built_from_commit_string[0])
> >> +			strbuf_addf(buf, "built from commit: %s\n",
> >> +			       git_built_from_commit_string);
> >> +		else
> >> +			strbuf_addf(buf, "no commit associated with this build\n");
> >
> > The "StaticAnalysis" job of the Azure Pipeline is not happy with this,
> > claiming that this should be an `strbuf_addstr()` call instead.
> 
> You mean the "else" clause, right?  That feels similar to say
> 
> 	printf("Hello world\n");
> 
> should better be written as
> 
> 	fputs("Hello world\n", stdout);
> 
> which I do not agree with at all.  IOW, I view the distinction more
> like "once it is written one way or the other way, it is not worth
> spending bits and braincycles to see if it is worth changing it"
> kind of minor stylistic preference.

I think I side with Junio here, although it's true that when strbuf_addstr() exists it doesn't make that much sense to use strbuf_addf(). Since there's some other comments, though, I'll change this too to make your CI shut up. :)

 - Emily