| Message ID | 20200406224526.256074-4-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bugreport: add tool to generate debugging info | expand |
Emily Shaffer <emilyshaffer@google.com> writes: > 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. This means that "git-bugreport" that can be a different binary from say "git-remote-curl" (or "git-version" for that matter) would still report whatever version string that was compiled into "bugreport". Reporting the version of "bugreport" is *not* useless, and what this step adds to the report is good. But the version number of "bugreport" may not have much to do with the binary the end user is having trouble with, so we'd also want the version of the main part of "git", and other standalone "git" subprograms like "git-remote-curl", reported separately, probably together with $PATH, $GIT_EXEC_PATH and what appears in the directories listed on these environment variables. If "git version --build-options" writes to its standard output stream, that is a good thing, as it makes it easy to spawn and read what it says via the run_command() API, and there is one less thing to worry about (it would be a bit more cumbersome if the output goes to the standard error stream). As "git-remote-curl" would also be a separate binary, we'd have to use the same technique to report version number (and perhaps curl's library version and its configuration?), perhaps by teaching the subcommand a new option to dump such details to its standard output. Using the same technique to report the version about the "git" itself would be consistent thing to do, as a preparation for future steps that reports the details about "git-remote-curl". Thanks.
On Mon, Apr 06, 2020 at 04:17:51PM -0700, Junio C Hamano wrote: > Emily Shaffer <emilyshaffer@google.com> writes: > > > 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. > > This means that "git-bugreport" that can be a different binary from > say "git-remote-curl" (or "git-version" for that matter) would still > report whatever version string that was compiled into "bugreport". > > Reporting the version of "bugreport" is *not* useless, and what this > step adds to the report is good. > > But the version number of "bugreport" may not have much to do with > the binary the end user is having trouble with, so we'd also want > the version of the main part of "git", and other standalone "git" > subprograms like "git-remote-curl", reported separately, probably > together with $PATH, $GIT_EXEC_PATH and what appears in the > directories listed on these environment variables. > > If "git version --build-options" writes to its standard output > stream, that is a good thing, as it makes it easy to spawn and read > what it says via the run_command() API, and there is one less thing > to worry about (it would be a bit more cumbersome if the output goes > to the standard error stream). > > As "git-remote-curl" would also be a separate binary, we'd have to > use the same technique to report version number (and perhaps curl's > library version and its configuration?), perhaps by teaching the > subcommand a new option to dump such details to its standard output. > > Using the same technique to report the version about the "git" > itself would be consistent thing to do, as a preparation for future > steps that reports the details about "git-remote-curl". Sure. I will include a run_command() call to "git version --build-options" (or is it --build-info?) in the same series where I re-introduce the call to "git-remote-curl", since they will look pretty similar; around that time I will clean up what "git-bugreport" reports about its own version/build info, too. To be clear, do you want me to include the output of get_compiler_info() in "git version --build-options" when I do that change, too? - Emily
Emily Shaffer <emilyshaffer@google.com> writes: > To be clear, do you want me to include the output of get_compiler_info() > in "git version --build-options" when I do that change, too? In the endgame, there are two kinds of information we'd want to gather and report, I would think. Ones that can be different per-binary are things like: - what version of the source code the binary was compiled from - with what compiler options - using which compiler and - linking with what libraries, - where in the filesystem is the binary located The others are various properties of the system the user is using, and having trouble using, Git on: - how many CPUs do we have, - how much free memory, - is the repository's filesystem case sensitive, - what version of 'wish' is being used. We'd want the former to be reported for each binary that matters, so "git version --build-options" would want to say it, "git remote-curl --build-options" would want to say it, and being different binaries, they may say different things. There is not much point in duplicating the latter that are not binary specific, so it probably makes sense to gather them inside, and report them from, "git bugreport" itself. Thanks.
On Tue, Apr 07, 2020 at 01:05:02PM -0700, Junio C Hamano wrote: > Emily Shaffer <emilyshaffer@google.com> writes: > > > To be clear, do you want me to include the output of get_compiler_info() > > in "git version --build-options" when I do that change, too? > > In the endgame, there are two kinds of information we'd want to > gather and report, I would think. > > Ones that can be different per-binary are things like: > > - what version of the source code the binary was compiled from > - with what compiler options > - using which compiler and > - linking with what libraries, > - where in the filesystem is the binary located > > The others are various properties of the system the user is using, > and having trouble using, Git on: > > - how many CPUs do we have, > - how much free memory, > - is the repository's filesystem case sensitive, > - what version of 'wish' is being used. > > We'd want the former to be reported for each binary that matters, so > "git version --build-options" would want to say it, "git remote-curl > --build-options" would want to say it, and being different binaries, > they may say different things. > > There is not much point in duplicating the latter that are not > binary specific, so it probably makes sense to gather them inside, > and report them from, "git bugreport" itself. Sure. Sounds great. Thanks for laying it out, this is very clear. - Emily
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 f6f53a5e8e..4cdb58bbaa 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; @@ -79,6 +93,9 @@ int cmd_main(int argc, const char **argv) /* Prepare the report contents */ get_bug_template(&buffer); + get_header(&buffer, _("System Info")); + get_system_info(&buffer); + /* fopen doesn't offer us an O_EXCL alternative, except with glibc. */ report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666); diff --git a/help.c b/help.c index a21487db77..1de9c0d589 100644 --- a/help.c +++ b/help.c @@ -622,8 +622,32 @@ 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)); + /* 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 +661,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)); - /* 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 | 46 ++++++++++++++++++++------------- help.h | 1 + 4 files changed, 51 insertions(+), 19 deletions(-)