| Message ID | 20191213004312.169753-4-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,01/15] bugreport: add tool to generate debugging info | expand |
Emily Shaffer <emilyshaffer@google.com> 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. > 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?
Hi Emily, On Thu, 12 Dec 2019, Emily Shaffer wrote: > diff --git a/help.c b/help.c > index a21487db77..a43693fca5 100644 > --- a/help.c > +++ b/help.c > @@ -622,8 +622,33 @@ const char *help_unknown_cmd(const char *cmd) > exit(1); > } > > +void list_version_info(struct strbuf *buf, int build_options) > +{ > + strbuf_reset(buf); > + /* > + * The format of this string should be kept stable for compatibility > + * with external projects that rely on the output of "git version". > + * > + * Always show the version, even if other options are given. > + */ > + strbuf_addf(buf, "git version %s\n", git_version_string); > + > + 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. For details, see: https://dev.azure.com/gitgitgadget/8fc4a374-71dc-4558-a5ea-dd1c081ea621/_apis/build/builds/23830/logs/68 Ciao, Dscho > + strbuf_addf(buf, "sizeof-long: %d\n", (int)sizeof(long)); > + strbuf_addf(buf, "sizeof-size_t: %d\n", (int)sizeof(size_t)); > + /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */ > + } > +} > + > int cmd_version(int argc, const char **argv, const char *prefix) > { > + struct strbuf buf = STRBUF_INIT; > int build_options = 0; > const char * const usage[] = { > N_("git version [<options>]"), > @@ -637,25 +662,9 @@ int cmd_version(int argc, const char **argv, const char *prefix) > > argc = parse_options(argc, argv, prefix, options, usage, 0); > > - /* > - * The format of this string should be kept stable for compatibility > - * with external projects that rely on the output of "git version". > - * > - * Always show the version, even if other options are given. > - */ > - printf("git version %s\n", git_version_string); > + list_version_info(&buf, build_options); > + printf("%s", buf.buf); > > - if (build_options) { > - printf("cpu: %s\n", GIT_HOST_CPU); > - if (git_built_from_commit_string[0]) > - printf("built from commit: %s\n", > - git_built_from_commit_string); > - else > - printf("no commit associated with this build\n"); > - printf("sizeof-long: %d\n", (int)sizeof(long)); > - printf("sizeof-size_t: %d\n", (int)sizeof(size_t)); > - /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */ > - } > return 0; > } > > 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); > void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdnames *other_cmds); > +void list_version_info(struct strbuf *buf, int build_options); > > /* > * call this to die(), when it is suspected that the user mistyped a > -- > 2.24.1.735.g03f4e72817-goog > > >
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.
On Tue, Dec 17, 2019 at 12:34:53PM -0800, Junio C Hamano wrote: > 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
On Fri, Dec 13, 2019 at 01:06:29PM -0800, Junio C Hamano wrote: > Emily Shaffer <emilyshaffer@google.com> 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.
diff --git a/bugreport.c b/bugreport.c index 5495b31674..59d8b5a3af 100644 --- a/bugreport.c +++ b/bugreport.c @@ -1,8 +1,20 @@ -#include "builtin.h" +#include "cache.h" #include "parse-options.h" #include "stdio.h" #include "strbuf.h" #include "time.h" +#include "help.h" + +static void get_system_info(struct strbuf *sys_info) +{ + struct strbuf version_info = STRBUF_INIT; + + /* get git version from native cmd */ + strbuf_addstr(sys_info, "git version:\n"); + list_version_info(&version_info, 1); + strbuf_addbuf(sys_info, &version_info); + strbuf_complete_line(sys_info); +} static const char * const bugreport_usage[] = { N_("git bugreport [-o|--output <file>]"), @@ -32,6 +44,11 @@ static int get_bug_template(struct strbuf *template) return 0; } +static void get_header(struct strbuf *buf, const char *title) +{ + strbuf_addf(buf, "\n\n[%s]\n", title); +} + int cmd_main(int argc, const char **argv) { struct strbuf buffer = STRBUF_INIT; @@ -60,6 +77,10 @@ int cmd_main(int argc, const char **argv) get_bug_template(&buffer); + // add other contents + get_header(&buffer, "System Info"); + get_system_info(&buffer); + report = fopen_for_writing(report_path.buf); strbuf_write(&buffer, report); fclose(report); diff --git a/help.c b/help.c index a21487db77..a43693fca5 100644 --- a/help.c +++ b/help.c @@ -622,8 +622,33 @@ const char *help_unknown_cmd(const char *cmd) exit(1); } +void list_version_info(struct strbuf *buf, int build_options) +{ + strbuf_reset(buf); + /* + * The format of this string should be kept stable for compatibility + * with external projects that rely on the output of "git version". + * + * Always show the version, even if other options are given. + */ + strbuf_addf(buf, "git version %s\n", git_version_string); + + 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"); + strbuf_addf(buf, "sizeof-long: %d\n", (int)sizeof(long)); + strbuf_addf(buf, "sizeof-size_t: %d\n", (int)sizeof(size_t)); + /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */ + } +} + int cmd_version(int argc, const char **argv, const char *prefix) { + struct strbuf buf = STRBUF_INIT; int build_options = 0; const char * const usage[] = { N_("git version [<options>]"), @@ -637,25 +662,9 @@ int cmd_version(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, usage, 0); - /* - * The format of this string should be kept stable for compatibility - * with external projects that rely on the output of "git version". - * - * Always show the version, even if other options are given. - */ - printf("git version %s\n", git_version_string); + list_version_info(&buf, build_options); + printf("%s", buf.buf); - if (build_options) { - printf("cpu: %s\n", GIT_HOST_CPU); - if (git_built_from_commit_string[0]) - printf("built from commit: %s\n", - git_built_from_commit_string); - else - printf("no commit associated with this build\n"); - printf("sizeof-long: %d\n", (int)sizeof(long)); - printf("sizeof-size_t: %d\n", (int)sizeof(size_t)); - /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */ - } return 0; } 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); void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdnames *other_cmds); +void list_version_info(struct strbuf *buf, int build_options); /* * call this to die(), when it is suspected that the user mistyped a
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. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- bugreport.c | 23 ++++++++++++++++++++++- help.c | 45 +++++++++++++++++++++++++++------------------ help.h | 1 + 3 files changed, 50 insertions(+), 19 deletions(-)