[v5,01/15] bugreport: add tool to generate debugging info
diff mbox series

Message ID 20200124033436.81097-2-emilyshaffer@google.com
State New
Headers show
Series
  • add git-bugreport tool
Related show

Commit Message

Emily Shaffer Jan. 24, 2020, 3:34 a.m. UTC
From: Emily Shaffer <emilyshaffer@google.com>

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 | 43 ++++++++++++++++++++
 Makefile                        |  5 +++
 bugreport.c                     | 69 +++++++++++++++++++++++++++++++++
 t/t0091-bugreport.sh            | 41 ++++++++++++++++++++
 5 files changed, 159 insertions(+)
 create mode 100644 Documentation/git-bugreport.txt
 create mode 100644 bugreport.c
 create mode 100755 t/t0091-bugreport.sh

Comments

Martin Ågren Jan. 30, 2020, 10:18 p.m. UTC | #1
On Fri, 24 Jan 2020 at 05:56, <emilyshaffer@google.com> wrote:
>
> From: Emily Shaffer <emilyshaffer@google.com>
>
> 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.

("Later" meaning "later in this series", or "any day now"? ;-) )

> +SYNOPSIS
> +--------
> +[verse]
> +'git bugreport' [-o | --output <path>]

Hmm. Should that be "[(-o | --output) <path>]"?

> +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.

Nice description. Got it.

> +The following information is requested from the user:
> +
> + - Reproduction steps
> + - Expected behavior
> + - Actual behavior
> +
> +The following information is captured automatically:
> +
> + - Git version (`git version --build-options`)
> + - Machine information (`uname -a`)
> + - Versions of various dependencies
> + - Git config contents (`git config --show-origin --list`)
> + - The names of all configured git-hooks in `.git/hooks/`

I would have expected these points to appear later, both to make it
clear what this does commit does (and not), and to highlight what
user-visible (documentation-worthy) changes later commits bring along.

> +OPTIONS
> +-------
> +-o [<path>]::
> +--output [<path>]::

Drop the "[" and "]"? If you give -o, you'd better give a path as well?

> +       Place the resulting bug report file in <path> instead of the root of the

`<path>`

> +"Please review the rest of the bug report below.\n"
> +"You can delete any lines you don't wish to send.\n");

"send" sounds like we're *just* about to send this report somewhere, but
it's "only" going to be written to the disk. Maybe "share", instead?

> +       if (option_output) {
> +               strbuf_addstr(&report_path, option_output);
> +               strbuf_complete(&report_path, '/');
> +       }

I thought I'd use `-o` to indicate the filename, but it turns out it's
the *directory* where the file (of some semi-random, generated name)
will end up. Re-reading the docs further up, I can see how this is
consistent. I sort of wonder if this should be `--output*-directory*`
for symmetry with `git format-patch`.

> +       strbuf_addstr(&report_path, "git-bugreport-");
> +       strbuf_addftime(&report_path, "%F", gmtime(&now), 0, 0);
> +       strbuf_addstr(&report_path, ".txt");
> +
> +

(Double blank line?)

> +       get_bug_template(&buffer);
> +
> +       report = fopen_for_writing(report_path.buf);

Report might be NULL here.

If there's already such a file, we overwrite. Should we generate the
filename using not just today's date (two bug reports in a day wouldn't
be unheard of?) but also something like hh:mm:ss?

> +       strbuf_write(&buffer, report);
> +       fclose(report);

Maybe clear the strbuf around here...

> +       launch_editor(report_path.buf, NULL, NULL);
> +       return 0;

... and/or UNLEAK it here, together with report_path.

Maybe "return -launch_editor(...)"?

> +#!/bin/bash

Use /bin/sh instead?

> +# 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 [$(grep $HEADER_PATTERN $line)]; then

I think this is a bash-ism.

> +                       read -r nextline
> +                       if [-z $nextline]; then

Likewise.

> +                               return 1;
> +                       fi
> +               fi
> +       done
> +}
> +
> +test_expect_success 'creates a report with content in the right places' '
> +       git bugreport &&
> +       check_all_headers_populated <git-bugreport-* &&
> +       rm git-bugreport-*
> +'
> +
> +test_expect_success '--output puts the report in the provided dir' '
> +       mkdir foo/ &&

If foo isn't there, do we not create it? Apparently not -- in my
testing, we segfault. (We don't check for NULL after opening the file.)

> +       git bugreport -o foo/ &&
> +       test -f foo/git-bugreport-* &&

test_path_is_file

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

test_path_is_missing


Martin
Emily Shaffer Feb. 4, 2020, 10 p.m. UTC | #2
On Thu, Jan 30, 2020 at 11:18:55PM +0100, Martin Ågren wrote:
> On Fri, 24 Jan 2020 at 05:56, <emilyshaffer@google.com> wrote:
> >
> > From: Emily Shaffer <emilyshaffer@google.com>
> >
> > 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.
> 
> ("Later" meaning "later in this series", or "any day now"? ;-) )
> 
> > +SYNOPSIS
> > +--------
> > +[verse]
> > +'git bugreport' [-o | --output <path>]
> 
> Hmm. Should that be "[(-o | --output) <path>]"?

Done.

> > +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.
> 
> Nice description. Got it.
> 
> > +The following information is requested from the user:
> > +
> > + - Reproduction steps
> > + - Expected behavior
> > + - Actual behavior
> > +
> > +The following information is captured automatically:
> > +
> > + - Git version (`git version --build-options`)
> > + - Machine information (`uname -a`)
> > + - Versions of various dependencies
> > + - Git config contents (`git config --show-origin --list`)
> > + - The names of all configured git-hooks in `.git/hooks/`
> 
> I would have expected these points to appear later, both to make it
> clear what this does commit does (and not), and to highlight what
> user-visible (documentation-worthy) changes later commits bring along.

Sure, agreed.

> 
> > +OPTIONS
> > +-------
> > +-o [<path>]::
> > +--output [<path>]::
> 
> Drop the "[" and "]"? If you give -o, you'd better give a path as well?

Done.

> > +       Place the resulting bug report file in <path> instead of the root of the
> 
> `<path>`

Done.

> > +"Please review the rest of the bug report below.\n"
> > +"You can delete any lines you don't wish to send.\n");
> 
> "send" sounds like we're *just* about to send this report somewhere, but
> it's "only" going to be written to the disk. Maybe "share", instead?

Nice turn of phrase, done.

> > +       if (option_output) {
> > +               strbuf_addstr(&report_path, option_output);
> > +               strbuf_complete(&report_path, '/');
> > +       }
> 
> I thought I'd use `-o` to indicate the filename, but it turns out it's
> the *directory* where the file (of some semi-random, generated name)
> will end up. Re-reading the docs further up, I can see how this is
> consistent. I sort of wonder if this should be `--output*-directory*`
> for symmetry with `git format-patch`.

Sure.

> > +       strbuf_addstr(&report_path, "git-bugreport-");
> > +       strbuf_addftime(&report_path, "%F", gmtime(&now), 0, 0);
> > +       strbuf_addstr(&report_path, ".txt");
> > +
> > +
> 
> (Double blank line?)

Done.
 
> > +       get_bug_template(&buffer);
> > +
> > +       report = fopen_for_writing(report_path.buf);
> 
> Report might be NULL here.

Nice.

> If there's already such a file, we overwrite. Should we generate the
> filename using not just today's date (two bug reports in a day wouldn't
> be unheard of?) but also something like hh:mm:ss?

Sure. For the sake of brevity I'll probably neglect seconds; I hope
someone is spending more than 1 minute filling in the provided form.

I'm a little worried about including : in a filename, so I went for
'git-bugreport-YYYY-MM-DD-HHMM' (24-hour).

As I started to write a test to ensure duplicate filenames were handled
well, Jonathan Tan pointed out that it would be easy to add an arg like
--suffix to allow specifying a custom strftime string. That would allow
users to easily create a file named
`git-bugreport-fetch-failing.txt` or `git-bugreport-March-19.txt` or
whatever they want; it also makes testing easy. So I'll add this for the
next rollup.

> 
> > +       strbuf_write(&buffer, report);
> > +       fclose(report);
> 
> Maybe clear the strbuf around here...
> 
> > +       launch_editor(report_path.buf, NULL, NULL);
> > +       return 0;
> 
> ... and/or UNLEAK it here, together with report_path.
> 
> Maybe "return -launch_editor(...)"?

Hm, sure. I see that builtin/tag.c does mark strbufs this way, so I
don't see a problem using UNLEAK and tail-calling launch_editor().


As a final bonus, I also added a line to report to stderr the name of
the file that was created. I noticed it's sort of unclear what the
command actually did otherwise.

> > +#!/bin/bash
> 
> Use /bin/sh instead?

Yeah, doing so immediately pointed out the bashisms you mentioned, plus
some more. :facepalm:

> 
> > +# 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 [$(grep $HEADER_PATTERN $line)]; then
> 
> I think this is a bash-ism.
> 
> > +                       read -r nextline
> > +                       if [-z $nextline]; then
> 
> Likewise.
> 
> > +                               return 1;
> > +                       fi
> > +               fi
> > +       done
> > +}
> > +
> > +test_expect_success 'creates a report with content in the right places' '
> > +       git bugreport &&
> > +       check_all_headers_populated <git-bugreport-* &&
> > +       rm git-bugreport-*
> > +'
> > +
> > +test_expect_success '--output puts the report in the provided dir' '
> > +       mkdir foo/ &&
> 
> If foo isn't there, do we not create it? Apparently not -- in my
> testing, we segfault. (We don't check for NULL after opening the file.)

Yeah, at the moment I just added a die() if we can't open the provided
path. I think other utilties can create the path (e.g. git-format-patch)
but where it makes sense to do so, I'd prefer to keep bugreport very
simple.

I'll die instead of overwriting, too; you're right that spending quite a
while on a bug report and then accidentally writing over it with a blank
one would be a very bad user experience.

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

Sure.

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

OK.

Thanks very much, Martin, for the thorough review. This is incredibly
helpful.

 - Emily

Patch
diff mbox series

diff --git a/.gitignore b/.gitignore
index aebe7c0908..ca301bc890 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..75f0c80acf
--- /dev/null
+++ b/Documentation/git-bugreport.txt
@@ -0,0 +1,43 @@ 
+git-bugreport(1)
+================
+
+NAME
+----
+git-bugreport - Collect information for user to file a bug report
+
+SYNOPSIS
+--------
+[verse]
+'git bugreport' [-o | --output <path>]
+
+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
+
+The following information is captured automatically:
+
+ - Git version (`git version --build-options`)
+ - Machine information (`uname -a`)
+ - Versions of various dependencies
+ - Git config contents (`git config --show-origin --list`)
+ - The names of all configured git-hooks in `.git/hooks/`
+
+OPTIONS
+-------
+-o [<path>]::
+--output [<path>]::
+	Place the resulting bug report file in <path> instead of the root of the
+	Git repository.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 09f98b777c..f271619371 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
@@ -2450,6 +2451,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..5495b31674
--- /dev/null
+++ b/bugreport.c
@@ -0,0 +1,69 @@ 
+#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 <file>]"),
+	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 send.\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;
+
+	const struct option bugreport_options[] = {
+		OPT_STRING('o', "output", &option_output, N_("path"),
+			   N_("specify a destination for the bugreport file")),
+		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, "%F", gmtime(&now), 0, 0);
+	strbuf_addstr(&report_path, ".txt");
+
+
+	get_bug_template(&buffer);
+
+	report = fopen_for_writing(report_path.buf);
+	strbuf_write(&buffer, report);
+	fclose(report);
+
+	launch_editor(report_path.buf, NULL, NULL);
+	return 0;
+}
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
new file mode 100755
index 0000000000..6eb2ee4f66
--- /dev/null
+++ b/t/t0091-bugreport.sh
@@ -0,0 +1,41 @@ 
+#!/bin/bash
+
+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 [$(grep $HEADER_PATTERN $line)]; then
+			read -r nextline
+			if [-z $nextline]; then
+				return 1;
+			fi
+		fi
+	done
+}
+
+test_expect_success 'creates a report with content in the right places' '
+	git bugreport &&
+	check_all_headers_populated <git-bugreport-* &&
+	rm git-bugreport-*
+'
+
+test_expect_success '--output puts the report in the provided dir' '
+	mkdir foo/ &&
+	git bugreport -o foo/ &&
+	test -f 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 ! -f git-bugreport-*
+'
+
+test_done