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

From: Emily Shaffer <[email-masked]>
Date: Thu, 19 Dec 2019 17:46:19 -0800

On Fri, Dec 13, 2019 at 01:06:29PM -0800, Junio C Hamano wrote:
> Emily Shaffer <[email-masked]> writes:
> 
> > 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.

Sure, dropped this entirely. I think with the helpers the code is
self-documenting there.

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

Hm, sure. I renamed it to get_version_info(); I had named it list_*
because all the other helpers in help.h are named list_*, and it does
print more than one piece of info. But I do see your point (all the info
is about the same version) so I've renamed it.

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

Agree, done.