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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 13, 2019, 21:06 UTC
Message-ID: <xmqqr218hr56.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20191213004312.169753-4-emilyshaffer@google.com>

Emily Shaffer <emilyshaffer@google.com> writes:

Show 9 quoted lines

> Knowing which version of Git a user has and how it was built allows us
> to more precisely pin down the circumstances when a certain issue
> occurs, so teach bugreport how to tell us the same output as 'git
> version --build-options'.
>
> It's not ideal to directly call 'git version --build-options' because
> that output goes to stdout. Instead, wrap the version string in a helper
> within help.[ch] library, and call that helper from within the bugreport
> library.

Move to strbuf() from stdio makes sense.

> +	// add other contents

Style.

Show 7 quoted lines

> diff --git a/help.h b/help.h
> index 9071894e8c..54f6b5f793 100644
> --- a/help.h
> +++ b/help.h
> @@ -37,6 +37,7 @@ void add_cmdname(struct cmdnames *cmds, const char *name, int len);
>  void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);
>  int is_in_cmdlist(struct cmdnames *cmds, const char *name);

Many new helpers are called get_frotz(), but only one among these is called list_version_info(). I do not think the naming of the get_*() ones, that are private to the bugreport tool, matters that much, but unlike "list_commands()" whose purpose is to list the commands ;-), the new function does not list versions---it just gives information about a single version which is the one that is being run, so perhaps it is a misnomer.

>  void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdnames *other_cmds);
> +void list_version_info(struct strbuf *buf, int build_options);

It is not clear to the readers build_options is a boolean that tells the function to include (or not to include) build options. Perhaps rename it to "int show_build_options" or something?