diff mbox series

[v6,03/15] bugreport: add tool to generate debugging info

Message ID 20200206004108.261317-4-emilyshaffer@google.com (mailing list archive)
State New, archived
Headers show
Series add git-bugreport tool | expand

Commit Message

Emily Shaffer Feb. 6, 2020, 12:40 a.m. UTC
Teach Git how to prompt the user for a good bug report: reproduction
steps, expected behavior, and actual behavior. Later, Git can learn how
to collect some diagnostic information from the repository.

If users can send us a well-written bug report which contains diagnostic
information we would otherwise need to ask the user for, we can reduce
the number of question-and-answer round trips between the reporter and
the Git contributor.

Users may also wish to send a report like this to their local "Git
expert" if they have put their repository into a state they are confused
by.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 .gitignore                      |  1 +
 Documentation/git-bugreport.txt | 41 ++++++++++++++++
 Makefile                        |  5 ++
 bugreport.c                     | 85 +++++++++++++++++++++++++++++++++
 t/t0091-bugreport.sh            | 49 +++++++++++++++++++
 5 files changed, 181 insertions(+)
 create mode 100644 Documentation/git-bugreport.txt
 create mode 100644 bugreport.c
 create mode 100755 t/t0091-bugreport.sh

Comments

SZEDER Gábor Feb. 7, 2020, 2:18 p.m. UTC | #1
On Wed, Feb 05, 2020 at 04:40:56PM -0800, Emily Shaffer wrote:
> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> new file mode 100755
> index 0000000000..451badff0c
> --- /dev/null
> +++ b/t/t0091-bugreport.sh
> @@ -0,0 +1,49 @@
> +#!/bin/sh
> +
> +test_description='git bugreport'
> +
> +. ./test-lib.sh
> +
> +# Headers "[System Info]" will be followed by a non-empty line if we put some
> +# information there; we can make sure all our headers were followed by some
> +# information to check if the command was successful.
> +HEADER_PATTERN="^\[.*\]$"
> +check_all_headers_populated() {
> +	while read -r line; do
> +		if test "$(grep "$HEADER_PATTERN" "$line")"; then
> +			echo "$line"
> +			read -r nextline
> +			if test -z "$nextline"; then
> +				return 1;
> +			fi
> +		fi
> +	done
> +}
> +
> +test_expect_success 'creates a report with content in the right places' '
> +	git bugreport &&
> +	REPORT="$(ls git-bugreport-*)" &&

What if the globbing were to match more than one file?

> +	check_all_headers_populated <$REPORT &&
> +	rm $REPORT

Please register a cleanup commands like this with 'test_when_finished'.

> +'
> +
> +test_expect_success 'dies if file with same name as report already exists' '
> +	touch git-bugreport-duplicate.txt &&
> +	test_must_fail git bugreport --suffix duplicate &&
> +	rm git-bugreport-duplicate.txt
> +'
> +
> +test_expect_success '--output-directory puts the report in the provided dir' '
> +	mkdir foo/ &&

Is it really necessary to create the directory in advance?  Or to put
it in another way: shouldn't 'git bugreport' create any missing
leading directories of the path given for its '-o' option?  FWIW, 'git
format-patch -o dir ...' does create all necessary directories.

> +	git bugreport -o foo/ &&
> +	test_path_is_file foo/git-bugreport-* &&

What if the globbing were to match more than one file? :)

> +	rm -fr foo/
> +'
> +
> +test_expect_success 'incorrect arguments abort with usage' '
> +	test_must_fail git bugreport --false 2>output &&
> +	grep usage output &&

This breaks the GETTEXT_POISON CI job, because "usage" is translated,
so the test should not look for it with plain 'grep'.  Please use
'test_i18ngrep' instead.

> +	test_path_is_missing git-bugreport-*
> +'
> +
> +test_done
> -- 
> 2.25.0.341.g760bfbb309-goog
>
SZEDER Gábor Feb. 7, 2020, 2:54 p.m. UTC | #2
On Wed, Feb 05, 2020 at 04:40:56PM -0800, Emily Shaffer wrote:
> Teach Git how to prompt the user for a good bug report: reproduction
> steps, expected behavior, and actual behavior. Later, Git can learn how
> to collect some diagnostic information from the repository.
> 
> If users can send us a well-written bug report which contains diagnostic
> information we would otherwise need to ask the user for, we can reduce
> the number of question-and-answer round trips between the reporter and
> the Git contributor.
> 
> Users may also wish to send a report like this to their local "Git
> expert" if they have put their repository into a state they are confused
> by.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  .gitignore                      |  1 +
>  Documentation/git-bugreport.txt | 41 ++++++++++++++++
>  Makefile                        |  5 ++
>  bugreport.c                     | 85 +++++++++++++++++++++++++++++++++
>  t/t0091-bugreport.sh            | 49 +++++++++++++++++++
>  5 files changed, 181 insertions(+)
>  create mode 100644 Documentation/git-bugreport.txt
>  create mode 100644 bugreport.c
>  create mode 100755 t/t0091-bugreport.sh

Please add a corresponding entry to 'command-list.txt' as well.
According to the comment at the beginning of that file, all, even
external commands should be included there.
Junio C Hamano Feb. 7, 2020, 6:51 p.m. UTC | #3
SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Wed, Feb 05, 2020 at 04:40:56PM -0800, Emily Shaffer wrote:
>> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
>> new file mode 100755
>> index 0000000000..451badff0c
>> --- /dev/null
>> +++ b/t/t0091-bugreport.sh
>> @@ -0,0 +1,49 @@
>> +#!/bin/sh
>> +
>> +test_description='git bugreport'
>> +
>> +. ./test-lib.sh
>> +
>> +# Headers "[System Info]" will be followed by a non-empty line if we put some
>> +# information there; we can make sure all our headers were followed by some
>> +# information to check if the command was successful.
>> +HEADER_PATTERN="^\[.*\]$"
>> +check_all_headers_populated() {
>> +	while read -r line; do
>> +		if test "$(grep "$HEADER_PATTERN" "$line")"; then
>> +			echo "$line"
>> +			read -r nextline
>> +			if test -z "$nextline"; then
>> +				return 1;
>> +			fi
>> +		fi
>> +	done
>> +}
>> +
>> +test_expect_success 'creates a report with content in the right places' '
>> +	git bugreport &&
>> +	REPORT="$(ls git-bugreport-*)" &&
>
> What if the globbing were to match more than one file?

An often-useful pattern is to make the command report the output
filename, i.e.

	REPORT=$(git butreport) &&

if the design insists that "git bugreport" should allocate a
filename in order to make it easy to guarantee uniqueness.

Of course, we can make the invoker supply filename, e.g.

	REPORT=$(generate-output-filename) &&
	git bugreport -o "$REPORT" ;# or git bugreport >"$REPORT"
Emily Shaffer Feb. 11, 2020, 10:40 p.m. UTC | #4
On Fri, Feb 07, 2020 at 10:51:29AM -0800, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > On Wed, Feb 05, 2020 at 04:40:56PM -0800, Emily Shaffer wrote:
> >> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> >> new file mode 100755
> >> index 0000000000..451badff0c
> >> --- /dev/null
> >> +++ b/t/t0091-bugreport.sh
> >> @@ -0,0 +1,49 @@
> >> +#!/bin/sh
> >> +
> >> +test_description='git bugreport'
> >> +
> >> +. ./test-lib.sh
> >> +
> >> +# Headers "[System Info]" will be followed by a non-empty line if we put some
> >> +# information there; we can make sure all our headers were followed by some
> >> +# information to check if the command was successful.
> >> +HEADER_PATTERN="^\[.*\]$"
> >> +check_all_headers_populated() {
> >> +	while read -r line; do
> >> +		if test "$(grep "$HEADER_PATTERN" "$line")"; then
> >> +			echo "$line"
> >> +			read -r nextline
> >> +			if test -z "$nextline"; then
> >> +				return 1;
> >> +			fi
> >> +		fi
> >> +	done
> >> +}
> >> +
> >> +test_expect_success 'creates a report with content in the right places' '
> >> +	git bugreport &&
> >> +	REPORT="$(ls git-bugreport-*)" &&
> >
> > What if the globbing were to match more than one file?
> 
> An often-useful pattern is to make the command report the output
> filename, i.e.
> 
> 	REPORT=$(git butreport) &&
> 
> if the design insists that "git bugreport" should allocate a
> filename in order to make it easy to guarantee uniqueness.
> 
> Of course, we can make the invoker supply filename, e.g.
> 
> 	REPORT=$(generate-output-filename) &&
> 	git bugreport -o "$REPORT" ;# or git bugreport >"$REPORT"

Yeah, this is an option and I've changed the offending test - which
captures a variable based on globbing - to use --suffix and force a
filename.
Junio C Hamano Feb. 12, 2020, 6:06 p.m. UTC | #5
As this topic seems to break under GIT_TEST_GETTEXT_POISON=yes, I'll
apply the following band-aid on top before merging it to 'pu'.  

Most of them are style fixes, but quoting $REPORT, not just makes
the redirection honor the coding guidelines, will ensure that an
error is caught if git-bugreport-* glob matches no paths or more
than on paths.




 t/t0091-bugreport.sh | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index 451badff0c..f2186941ce 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -8,9 +8,12 @@ test_description='git bugreport'
 # information there; we can make sure all our headers were followed by some
 # information to check if the command was successful.
 HEADER_PATTERN="^\[.*\]$"
-check_all_headers_populated() {
-	while read -r line; do
-		if test "$(grep "$HEADER_PATTERN" "$line")"; then
+
+check_all_headers_populated () {
+	while read -r line
+	do
+		if test "$(grep "$HEADER_PATTERN" "$line")"
+		then
 			echo "$line"
 			read -r nextline
 			if test -z "$nextline"; then
@@ -23,12 +26,12 @@ check_all_headers_populated() {
 test_expect_success 'creates a report with content in the right places' '
 	git bugreport &&
 	REPORT="$(ls git-bugreport-*)" &&
-	check_all_headers_populated <$REPORT &&
-	rm $REPORT
+	check_all_headers_populated <"$REPORT" &&
+	rm "$REPORT"
 '
 
 test_expect_success 'dies if file with same name as report already exists' '
-	touch git-bugreport-duplicate.txt &&
+	>>git-bugreport-duplicate.txt &&
 	test_must_fail git bugreport --suffix duplicate &&
 	rm git-bugreport-duplicate.txt
 '
@@ -42,7 +45,7 @@ test_expect_success '--output-directory puts the report in the provided dir' '
 
 test_expect_success 'incorrect arguments abort with usage' '
 	test_must_fail git bugreport --false 2>output &&
-	grep usage output &&
+	test_i18ngrep usage output &&
 	test_path_is_missing git-bugreport-*
 '
Emily Shaffer Feb. 12, 2020, 10:36 p.m. UTC | #6
On Wed, Feb 12, 2020 at 10:06:44AM -0800, Junio C Hamano wrote:
> As this topic seems to break under GIT_TEST_GETTEXT_POISON=yes, I'll
> apply the following band-aid on top before merging it to 'pu'.  
> 
> Most of them are style fixes, but quoting $REPORT, not just makes
> the redirection honor the coding guidelines, will ensure that an
> error is caught if git-bugreport-* glob matches no paths or more
> than on paths.

Thanks. Most of these I have locally, but not all. I'll apply this as a
fixup on patch 3/n (the one that introduces the tests) and add a
Helped-by line.

I'm hoping to get a reroll out today; it was the first thing on my list
this week, and then a high-priority internal issue shows up (so it
goes). Sorry for the inconvenience with the CI failures.

 - Emily

>  t/t0091-bugreport.sh | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> index 451badff0c..f2186941ce 100755
> --- a/t/t0091-bugreport.sh
> +++ b/t/t0091-bugreport.sh
> @@ -8,9 +8,12 @@ test_description='git bugreport'
>  # information there; we can make sure all our headers were followed by some
>  # information to check if the command was successful.
>  HEADER_PATTERN="^\[.*\]$"
> -check_all_headers_populated() {
> -	while read -r line; do
> -		if test "$(grep "$HEADER_PATTERN" "$line")"; then
> +
> +check_all_headers_populated () {
> +	while read -r line
> +	do
> +		if test "$(grep "$HEADER_PATTERN" "$line")"
> +		then
>  			echo "$line"
>  			read -r nextline
>  			if test -z "$nextline"; then
> @@ -23,12 +26,12 @@ check_all_headers_populated() {
>  test_expect_success 'creates a report with content in the right places' '
>  	git bugreport &&
>  	REPORT="$(ls git-bugreport-*)" &&
> -	check_all_headers_populated <$REPORT &&
> -	rm $REPORT
> +	check_all_headers_populated <"$REPORT" &&
> +	rm "$REPORT"
>  '
>  
>  test_expect_success 'dies if file with same name as report already exists' '
> -	touch git-bugreport-duplicate.txt &&
> +	>>git-bugreport-duplicate.txt &&
>  	test_must_fail git bugreport --suffix duplicate &&
>  	rm git-bugreport-duplicate.txt
>  '
> @@ -42,7 +45,7 @@ test_expect_success '--output-directory puts the report in the provided dir' '
>  
>  test_expect_success 'incorrect arguments abort with usage' '
>  	test_must_fail git bugreport --false 2>output &&
> -	grep usage output &&
> +	test_i18ngrep usage output &&
>  	test_path_is_missing git-bugreport-*
>  '
>  
> -- 
> 2.25.0-455-gcf3c3a5ab4
>
diff mbox series

Patch

diff --git a/.gitignore b/.gitignore
index ea97de83f3..d89bf9e11e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -25,6 +25,7 @@ 
 /git-bisect--helper
 /git-blame
 /git-branch
+/git-bugreport
 /git-bundle
 /git-cat-file
 /git-check-attr
diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
new file mode 100644
index 0000000000..52d49ed7aa
--- /dev/null
+++ b/Documentation/git-bugreport.txt
@@ -0,0 +1,41 @@ 
+git-bugreport(1)
+================
+
+NAME
+----
+git-bugreport - Collect information for user to file a bug report
+
+SYNOPSIS
+--------
+[verse]
+'git bugreport' [(-o | --output-directory) <path>] [(-s | --suffix) <format>]
+
+DESCRIPTION
+-----------
+Captures information about the user's machine, Git client, and repository state,
+as well as a form requesting information about the behavior the user observed,
+into a single text file which the user can then share, for example to the Git
+mailing list, in order to report an observed bug.
+
+The following information is requested from the user:
+
+ - Reproduction steps
+ - Expected behavior
+ - Actual behavior
+
+OPTIONS
+-------
+-o <path>::
+--output-directory <path>::
+	Place the resulting bug report file in `<path>` instead of the root of
+	the Git repository.
+
+-s <format>::
+--suffix <format>::
+	Specify an alternate suffix for the bugreport name, to create a file
+	named 'git-bugreport-<formatted suffix>'. This should take the form of a
+	link:strftime[3] format string; the current local time will be used.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 5a022367d4..a01a050aa3 100644
--- a/Makefile
+++ b/Makefile
@@ -681,6 +681,7 @@  EXTRA_PROGRAMS =
 # ... and all the rest that could be moved out of bindir to gitexecdir
 PROGRAMS += $(EXTRA_PROGRAMS)
 
+PROGRAM_OBJS += bugreport.o
 PROGRAM_OBJS += credential-store.o
 PROGRAM_OBJS += daemon.o
 PROGRAM_OBJS += fast-import.o
@@ -2457,6 +2458,10 @@  endif
 git-%$X: %.o GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 
+git-bugreport$X: bugreport.o GIT-LDFLAGS $(GITLIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
+		$(LIBS)
+
 git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(IMAP_SEND_LDFLAGS) $(LIBS)
diff --git a/bugreport.c b/bugreport.c
new file mode 100644
index 0000000000..db46fb88be
--- /dev/null
+++ b/bugreport.c
@@ -0,0 +1,85 @@ 
+#include "builtin.h"
+#include "parse-options.h"
+#include "stdio.h"
+#include "strbuf.h"
+#include "time.h"
+
+static const char * const bugreport_usage[] = {
+	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
+	NULL
+};
+
+static int get_bug_template(struct strbuf *template)
+{
+	const char template_text[] = N_(
+"Thank you for filling out a Git bug report!\n"
+"Please answer the following questions to help us understand your issue.\n"
+"\n"
+"What did you do before the bug happened? (Steps to reproduce your issue)\n"
+"\n"
+"What did you expect to happen? (Expected behavior)\n"
+"\n"
+"What happened instead? (Actual behavior)\n"
+"\n"
+"What's different between what you expected and what actually happened?\n"
+"\n"
+"Anything else you want to add:\n"
+"\n"
+"Please review the rest of the bug report below.\n"
+"You can delete any lines you don't wish to share.\n");
+
+	strbuf_addstr(template, template_text);
+	return 0;
+}
+
+int cmd_main(int argc, const char **argv)
+{
+	struct strbuf buffer = STRBUF_INIT;
+	struct strbuf report_path = STRBUF_INIT;
+	FILE *report;
+	time_t now = time(NULL);
+	char *option_output = NULL;
+	char *option_suffix = "%F-%H%M";
+	struct stat statbuf;
+
+	const struct option bugreport_options[] = {
+		OPT_STRING('o', "output-directory", &option_output, N_("path"),
+			   N_("specify a destination for the bugreport file")),
+		OPT_STRING('s', "suffix", &option_suffix, N_("format"),
+			   N_("specify a strftime format suffix for the filename")),
+		OPT_END()
+	};
+	argc = parse_options(argc, argv, "", bugreport_options,
+			     bugreport_usage, 0);
+
+	if (option_output) {
+		strbuf_addstr(&report_path, option_output);
+		strbuf_complete(&report_path, '/');
+	}
+
+
+	strbuf_addstr(&report_path, "git-bugreport-");
+	strbuf_addftime(&report_path, option_suffix, localtime(&now), 0, 0);
+	strbuf_addstr(&report_path, ".txt");
+
+	if (!stat(report_path.buf, &statbuf))
+		die("'%s' already exists", report_path.buf);
+
+	get_bug_template(&buffer);
+
+	report = fopen_for_writing(report_path.buf);
+
+	if (report == NULL) {
+		strbuf_release(&report_path);
+		die("couldn't open '%s' for writing", report_path.buf);
+	}
+
+	strbuf_write(&buffer, report);
+	fclose(report);
+
+	fprintf(stderr, _("Created new report at '%s'.\n"), report_path.buf);
+
+	UNLEAK(buffer);
+	UNLEAK(report_path);
+	return -launch_editor(report_path.buf, NULL, NULL);
+}
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
new file mode 100755
index 0000000000..451badff0c
--- /dev/null
+++ b/t/t0091-bugreport.sh
@@ -0,0 +1,49 @@ 
+#!/bin/sh
+
+test_description='git bugreport'
+
+. ./test-lib.sh
+
+# Headers "[System Info]" will be followed by a non-empty line if we put some
+# information there; we can make sure all our headers were followed by some
+# information to check if the command was successful.
+HEADER_PATTERN="^\[.*\]$"
+check_all_headers_populated() {
+	while read -r line; do
+		if test "$(grep "$HEADER_PATTERN" "$line")"; then
+			echo "$line"
+			read -r nextline
+			if test -z "$nextline"; then
+				return 1;
+			fi
+		fi
+	done
+}
+
+test_expect_success 'creates a report with content in the right places' '
+	git bugreport &&
+	REPORT="$(ls git-bugreport-*)" &&
+	check_all_headers_populated <$REPORT &&
+	rm $REPORT
+'
+
+test_expect_success 'dies if file with same name as report already exists' '
+	touch git-bugreport-duplicate.txt &&
+	test_must_fail git bugreport --suffix duplicate &&
+	rm git-bugreport-duplicate.txt
+'
+
+test_expect_success '--output-directory puts the report in the provided dir' '
+	mkdir foo/ &&
+	git bugreport -o foo/ &&
+	test_path_is_file foo/git-bugreport-* &&
+	rm -fr foo/
+'
+
+test_expect_success 'incorrect arguments abort with usage' '
+	test_must_fail git bugreport --false 2>output &&
+	grep usage output &&
+	test_path_is_missing git-bugreport-*
+'
+
+test_done