diff mbox series

[v3,09/11] builtin/bugreport.c: create '--diagnose' option

Message ID 1a1eb2c980635415c04d5c8d9a62bd972482d7dc.1660174473.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Generalize 'scalar diagnose' into 'git diagnose' and 'git bugreport --diagnose' | expand

Commit Message

Victoria Dye Aug. 10, 2022, 11:34 p.m. UTC
From: Victoria Dye <vdye@github.com>

Create a '--diagnose' option for 'git bugreport' to collect additional
information about the repository and write it to a zipped archive.

The '--diagnose' option behaves effectively as an alias for simultaneously
running 'git bugreport' and 'git diagnose'. In the documentation, users are
explicitly recommended to attach the diagnostics alongside a bug report to
provide additional context to readers, ideally reducing some back-and-forth
between reporters and those debugging the issue.

Note that '--diagnose' may take an optional string arg (either 'stats' or
'all'). If specified without the arg, the behavior corresponds to running
'git diagnose' without '--mode'. As with 'git diagnose', this default is
intended to help reduce unintentional leaking of sensitive information).
Users can also explicitly specify '--diagnose=(stats|all)' to generate the
respective archive created by 'git diagnose --mode=(stats|all)'.

Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Helped-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/git-bugreport.txt | 18 +++++++++++++
 builtin/bugreport.c             | 27 ++++++++++++++++---
 t/t0091-bugreport.sh            | 48 +++++++++++++++++++++++++++++++++
 3 files changed, 90 insertions(+), 3 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Aug. 11, 2022, 10:53 a.m. UTC | #1
On Wed, Aug 10 2022, Victoria Dye via GitGitGadget wrote:

> From: Victoria Dye <vdye@github.com>
>
> Create a '--diagnose' option for 'git bugreport' to collect additional
> information about the repository and write it to a zipped archive.
> [...]
>  'git bugreport' [(-o | --output-directory) <path>] [(-s | --suffix) <format>]
> +		[--diagnose[=<mode>]]
> [...]
>  static const char * const bugreport_usage[] = {
> -	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
> +	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>] [--diagnose[=<mode>]"),
>  	NULL
>  };

This still has the SYNOPSIS v.s. -h discrepancy noted in
https://lore.kernel.org/git/220804.86v8r8ec4s.gmgdl@evledraar.gmail.com/
Victoria Dye Aug. 11, 2022, 3:40 p.m. UTC | #2
Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Aug 10 2022, Victoria Dye via GitGitGadget wrote:
> 
>> From: Victoria Dye <vdye@github.com>
>>
>> Create a '--diagnose' option for 'git bugreport' to collect additional
>> information about the repository and write it to a zipped archive.
>> [...]
>>  'git bugreport' [(-o | --output-directory) <path>] [(-s | --suffix) <format>]
>> +		[--diagnose[=<mode>]]
>> [...]
>>  static const char * const bugreport_usage[] = {
>> -	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
>> +	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>] [--diagnose[=<mode>]"),
>>  	NULL
>>  };
> 
> This still has the SYNOPSIS v.s. -h discrepancy noted in
> https://lore.kernel.org/git/220804.86v8r8ec4s.gmgdl@evledraar.gmail.com/

The discrepancy you pointed out was on 'git diagnose' (which has since been
fixed), this is a pre-existing one in 'git bugreport'. I decided against
fixing *this* one because it didn't really fit into any of the patches in
this series, so it would need its own patch. When balancing "leave things
better than you found them" vs. "stay focused on the purpose of the series",
I leaned towards the latter to avoid setting a precedent for other 'git
bugreport'-related scope creep.

If you have the patches to audit this sort of thing, I think a nice place to
fix this might be in a dedicated series fixing discrepancies tree-wide. Even
better, you could include the patches in your tree that detect them as part
of e.g. the 'static-analysis' CI job.
Ævar Arnfjörð Bjarmason Aug. 11, 2022, 8:30 p.m. UTC | #3
On Thu, Aug 11 2022, Victoria Dye wrote:

> Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Wed, Aug 10 2022, Victoria Dye via GitGitGadget wrote:
>> 
>>> From: Victoria Dye <vdye@github.com>
>>>
>>> Create a '--diagnose' option for 'git bugreport' to collect additional
>>> information about the repository and write it to a zipped archive.
>>> [...]
>>>  'git bugreport' [(-o | --output-directory) <path>] [(-s | --suffix) <format>]
>>> +		[--diagnose[=<mode>]]
>>> [...]
>>>  static const char * const bugreport_usage[] = {
>>> -	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
>>> +	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>] [--diagnose[=<mode>]"),
>>>  	NULL
>>>  };
>> 
>> This still has the SYNOPSIS v.s. -h discrepancy noted in
>> https://lore.kernel.org/git/220804.86v8r8ec4s.gmgdl@evledraar.gmail.com/
>
> The discrepancy you pointed out was on 'git diagnose' (which has since been
> fixed),

Ah, sorry. I missed that & conflated the two.

> this is a pre-existing one in 'git bugreport'. I decided against
> fixing *this* one because it didn't really fit into any of the patches in
> this series, so it would need its own patch. When balancing "leave things
> better than you found them" vs. "stay focused on the purpose of the series",
> I leaned towards the latter to avoid setting a precedent for other 'git
> bugreport'-related scope creep.

In any case, I'm pointing out the difference in one of them having
\n-wrapping inconsistent with the other, which is an addition in this
series, sorry about not being clear.

I see that there's also the difference in how they format "--suffix",
but that's pre-existing & we can leave it for now. I think that's what
you're pointing out here as pre-existing.

> If you have the patches to audit this sort of thing, I think a nice place to
> fix this might be in a dedicated series fixing discrepancies tree-wide. Even
> better, you could include the patches in your tree that detect them as part
> of e.g. the 'static-analysis' CI job.

Yeah I do have those, and will probably submit those sooner than later,
and I'll end up spotting differences once they land on "master"
(e.g. [1] is one such case).

But this one is just one I eyeballed during review.

1. https://lore.kernel.org/git/220811.86o7wrov26.gmgdl@evledraar.gmail.com/
diff mbox series

Patch

diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
index d8817bf3cec..eca726e5791 100644
--- a/Documentation/git-bugreport.txt
+++ b/Documentation/git-bugreport.txt
@@ -9,6 +9,7 @@  SYNOPSIS
 --------
 [verse]
 'git bugreport' [(-o | --output-directory) <path>] [(-s | --suffix) <format>]
+		[--diagnose[=<mode>]]
 
 DESCRIPTION
 -----------
@@ -31,6 +32,10 @@  The following information is captured automatically:
  - A list of enabled hooks
  - $SHELL
 
+Additional information may be gathered into a separate zip archive using the
+`--diagnose` option, and can be attached alongside the bugreport document to
+provide additional context to readers.
+
 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
@@ -49,6 +54,19 @@  OPTIONS
 	named 'git-bugreport-<formatted suffix>'. This should take the form of a
 	strftime(3) format string; the current local time will be used.
 
+--no-diagnose::
+--diagnose[=<mode>]::
+	Create a zip archive of supplemental information about the user's
+	machine, Git client, and repository state. The archive is written to the
+	same output directory as the bug report and is named
+	'git-diagnostics-<formatted suffix>'.
++
+Without `mode` specified, the diagnostic archive will contain the default set of
+statistics reported by `git diagnose`. An optional `mode` value may be specified
+to change which information is included in the archive. See
+linkgit:git-diagnose[1] for the list of valid values for `mode` and details
+about their usage.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 9de32bc96e7..530895be55f 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -5,6 +5,7 @@ 
 #include "compat/compiler.h"
 #include "hook.h"
 #include "hook-list.h"
+#include "diagnose.h"
 
 
 static void get_system_info(struct strbuf *sys_info)
@@ -59,7 +60,7 @@  static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 }
 
 static const char * const bugreport_usage[] = {
-	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
+	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>] [--diagnose[=<mode>]"),
 	NULL
 };
 
@@ -98,16 +99,21 @@  int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	int report = -1;
 	time_t now = time(NULL);
 	struct tm tm;
+	enum diagnose_mode diagnose = DIAGNOSE_NONE;
 	char *option_output = NULL;
 	char *option_suffix = "%Y-%m-%d-%H%M";
 	const char *user_relative_path = NULL;
 	char *prefixed_filename;
+	size_t output_path_len;
 
 	const struct option bugreport_options[] = {
+		OPT_CALLBACK_F(0, "diagnose", &diagnose, N_("mode"),
+			       N_("create an additional zip archive of detailed diagnostics (default 'stats')"),
+			       PARSE_OPT_OPTARG, option_parse_diagnose),
 		OPT_STRING('o', "output-directory", &option_output, N_("path"),
-			   N_("specify a destination for the bugreport file")),
+			   N_("specify a destination for the bugreport file(s)")),
 		OPT_STRING('s', "suffix", &option_suffix, N_("format"),
-			   N_("specify a strftime format suffix for the filename")),
+			   N_("specify a strftime format suffix for the filename(s)")),
 		OPT_END()
 	};
 
@@ -119,6 +125,7 @@  int cmd_bugreport(int argc, const char **argv, const char *prefix)
 					    option_output ? option_output : "");
 	strbuf_addstr(&report_path, prefixed_filename);
 	strbuf_complete(&report_path, '/');
+	output_path_len = report_path.len;
 
 	strbuf_addstr(&report_path, "git-bugreport-");
 	strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
@@ -133,6 +140,20 @@  int cmd_bugreport(int argc, const char **argv, const char *prefix)
 		    report_path.buf);
 	}
 
+	/* Prepare diagnostics, if requested */
+	if (diagnose != DIAGNOSE_NONE) {
+		struct strbuf zip_path = STRBUF_INIT;
+		strbuf_add(&zip_path, report_path.buf, output_path_len);
+		strbuf_addstr(&zip_path, "git-diagnostics-");
+		strbuf_addftime(&zip_path, option_suffix, localtime_r(&now, &tm), 0, 0);
+		strbuf_addstr(&zip_path, ".zip");
+
+		if (create_diagnostics_archive(&zip_path, diagnose))
+			die_errno(_("unable to create diagnostics archive %s"), zip_path.buf);
+
+		strbuf_release(&zip_path);
+	}
+
 	/* Prepare the report contents */
 	get_bug_template(&buffer);
 
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index 08f5fe9caef..b6d2f591acd 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -78,4 +78,52 @@  test_expect_success 'indicates populated hooks' '
 	test_cmp expect actual
 '
 
+test_expect_success UNZIP '--diagnose creates diagnostics zip archive' '
+	test_when_finished rm -rf report &&
+
+	git bugreport --diagnose -o report -s test >out &&
+
+	zip_path=report/git-diagnostics-test.zip &&
+	grep "Available space" out &&
+	test_path_is_file "$zip_path" &&
+
+	# Check zipped archive content
+	"$GIT_UNZIP" -p "$zip_path" diagnostics.log >out &&
+	test_file_not_empty out &&
+
+	"$GIT_UNZIP" -p "$zip_path" packs-local.txt >out &&
+	grep ".git/objects" out &&
+
+	"$GIT_UNZIP" -p "$zip_path" objects-local.txt >out &&
+	grep "^Total: [0-9][0-9]*" out &&
+
+	# Should not include .git directory contents by default
+	! "$GIT_UNZIP" -l "$zip_path" | grep ".git/"
+'
+
+test_expect_success UNZIP '--diagnose=stats excludes .git dir contents' '
+	test_when_finished rm -rf report &&
+
+	git bugreport --diagnose=stats -o report -s test >out &&
+
+	# Includes pack quantity/size info
+	"$GIT_UNZIP" -p "$zip_path" packs-local.txt >out &&
+	grep ".git/objects" out &&
+
+	# Does not include .git directory contents
+	! "$GIT_UNZIP" -l "$zip_path" | grep ".git/"
+'
+
+test_expect_success UNZIP '--diagnose=all includes .git dir contents' '
+	test_when_finished rm -rf report &&
+
+	git bugreport --diagnose=all -o report -s test >out &&
+
+	# Includes .git directory contents
+	"$GIT_UNZIP" -l "$zip_path" | grep ".git/" &&
+
+	"$GIT_UNZIP" -p "$zip_path" .git/HEAD >out &&
+	test_file_not_empty out
+'
+
 test_done