[v12,3/5] bugreport: gather git version and build info
diff mbox series

Message ID 20200406224526.256074-4-emilyshaffer@google.com
State New
Headers show
Series
  • bugreport: add tool to generate debugging info
Related show

Commit Message

Emily Shaffer April 6, 2020, 10:45 p.m. UTC
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(-)

Comments

Junio C Hamano April 6, 2020, 11:17 p.m. UTC | #1
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.
Emily Shaffer April 7, 2020, 6:42 p.m. UTC | #2
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
Junio C Hamano April 7, 2020, 8:05 p.m. UTC | #3
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.
Emily Shaffer April 7, 2020, 8:34 p.m. UTC | #4
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

Patch
diff mbox series

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