| Message ID | 20200220015858.181086-5-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add git-bugreport tool | expand |
Emily Shaffer <emilyshaffer@google.com> writes: > +static void get_system_info(struct strbuf *sys_info) > +{ > + /* get git version from native cmd */ > + strbuf_addstr(sys_info, "git version:\n"); > + get_version_info(sys_info, 1); > + strbuf_complete_line(sys_info); It is a bit curious use of "don't do anything if sys_info ends with a complete line, but complete it if it ends with an imcomplete line". That tells the readers that we do not know what get_version_info() will do (now or in the future) to its output buffer. > +} > ... > diff --git a/help.c b/help.c > index 190722fb0a..44cee69c11 100644 > --- a/help.c > +++ b/help.c > @@ -622,8 +622,33 @@ const char *help_unknown_cmd(const char *cmd) > exit(1); > } > > +void get_version_info(struct strbuf *buf, int show_build_options) > +{ > + /* > + * 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); This ends the output with a complete line when !show_build_options ... > + if (show_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_addstr(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)); > + strbuf_addf(buf, "shell-path: %s\n", SHELL_PATH); > + /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */ ... and the pattern indicates the output will end with a complete line when !!show_build_options, too. > + } > +} So, was the strbuf_complete_line() merely defensive programming? It may deserve a comment if it will stay there. > 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,26 +662,11 @@ 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); > + get_version_info(&buf, build_options); > + printf("%s", buf.buf); > + > + strbuf_release(&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)); > - printf("shell-path: %s\n", SHELL_PATH); > - /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */ > - } > return 0; Makes sense.
On Thu, Feb 20, 2020 at 12:07:46PM -0800, Junio C Hamano wrote: > Emily Shaffer <emilyshaffer@google.com> writes: > > > +static void get_system_info(struct strbuf *sys_info) > > +{ > > + /* get git version from native cmd */ > > + strbuf_addstr(sys_info, "git version:\n"); > > + get_version_info(sys_info, 1); > > + strbuf_complete_line(sys_info); > > It is a bit curious use of "don't do anything if sys_info ends with > a complete line, but complete it if it ends with an imcomplete > line". That tells the readers that we do not know what > get_version_info() will do (now or in the future) to its output > buffer. > > > +} > > ... > > diff --git a/help.c b/help.c > > index 190722fb0a..44cee69c11 100644 > > --- a/help.c > > +++ b/help.c > > @@ -622,8 +622,33 @@ const char *help_unknown_cmd(const char *cmd) > > exit(1); > > } > > > > +void get_version_info(struct strbuf *buf, int show_build_options) > > +{ > > + /* > > + * 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); > > This ends the output with a complete line when !show_build_options ... > > > + if (show_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_addstr(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)); > > + strbuf_addf(buf, "shell-path: %s\n", SHELL_PATH); > > + /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */ > > ... and the pattern indicates the output will end with a complete > line when !!show_build_options, too. > > > + } > > +} > > So, was the strbuf_complete_line() merely defensive programming? It > may deserve a comment if it will stay there. It was meant defensively, here and elsewhere in the series. I figured that for something like this, which is mostly bounded by human writing in an editor and then by file IO, spurious string-checking was not such a big deal. Are you suggesting to comment around the strbuf_complete_line() calls, or to comment around get_version_info() that it should end in newline? - Emily
Emily Shaffer <emilyshaffer@google.com> writes: > On Thu, Feb 20, 2020 at 12:07:46PM -0800, Junio C Hamano wrote: >> Emily Shaffer <emilyshaffer@google.com> writes: >> >> > +static void get_system_info(struct strbuf *sys_info) >> > +{ >> > + /* get git version from native cmd */ >> > + strbuf_addstr(sys_info, "git version:\n"); >> > + get_version_info(sys_info, 1); >> > + strbuf_complete_line(sys_info); >> >> It is a bit curious use of "don't do anything if sys_info ends with >> a complete line, but complete it if it ends with an imcomplete >> line". That tells the readers that we do not know what >> get_version_info() will do (now or in the future) to its output >> buffer. >> ... >> So, was the strbuf_complete_line() merely defensive programming? It >> may deserve a comment if it will stay there. > > It was meant defensively, here and elsewhere in the series. I figured > that for something like this, which is mostly bounded by human writing > in an editor and then by file IO, spurious string-checking was not such > a big deal. > > Are you suggesting to comment around the strbuf_complete_line() calls, > or to comment around get_version_info() that it should end in newline? I meant a comment in get_system_info() next to the use of this particular use of strbuf_complete_line(), if the use stays there. But after reading the whole series through, I saw no need to use strbuf_complete_line() in the first place, as there is no source of input that is not under our control (if we do not count sloppy programming, that is).
diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt index 1f9fde5cde..f44ae8cbe7 100644 --- a/Documentation/git-bugreport.txt +++ b/Documentation/git-bugreport.txt @@ -23,6 +23,10 @@ The following information is requested from the user: - Expected behavior - Actual behavior +The following information is captured automatically: + + - 'git version --build-options' + This tool is invoked via the typical Git setup process, which means that in some cases, it might not be able to launch - for example, if a relevant config file is unreadable. In this kind of scenario, it may be helpful to manually gather diff --git a/bugreport.c b/bugreport.c index 8d4a76fdac..27f813643d 100644 --- a/bugreport.c +++ b/bugreport.c @@ -1,8 +1,17 @@ -#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) +{ + /* get git version from native cmd */ + strbuf_addstr(sys_info, "git version:\n"); + get_version_info(sys_info, 1); + strbuf_complete_line(sys_info); +} static const char * const bugreport_usage[] = { N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"), @@ -32,6 +41,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; @@ -76,6 +90,9 @@ int cmd_main(int argc, const char **argv) get_bug_template(&buffer); + get_header(&buffer, "System Info"); + get_system_info(&buffer); + report = fopen_for_writing(report_path.buf); if (report == NULL) { diff --git a/help.c b/help.c index 190722fb0a..44cee69c11 100644 --- a/help.c +++ b/help.c @@ -622,8 +622,33 @@ const char *help_unknown_cmd(const char *cmd) exit(1); } +void get_version_info(struct strbuf *buf, int show_build_options) +{ + /* + * 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 (show_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_addstr(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)); + strbuf_addf(buf, "shell-path: %s\n", SHELL_PATH); + /* 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,26 +662,11 @@ 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); + get_version_info(&buf, build_options); + printf("%s", buf.buf); + + strbuf_release(&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)); - printf("shell-path: %s\n", SHELL_PATH); - /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */ - } return 0; } diff --git a/help.h b/help.h index 9071894e8c..500521b908 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 get_version_info(struct strbuf *buf, int show_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> --- Documentation/git-bugreport.txt | 4 +++ bugreport.c | 19 ++++++++++++- help.c | 48 ++++++++++++++++++++------------- help.h | 1 + 4 files changed, 52 insertions(+), 20 deletions(-)