On Fri, Dec 13, 2019 at 01:06:29PM -0800, Junio C Hamano wrote: > Emily Shaffer <emilyshaffer@xxxxxxxxxx> 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.
