| Message ID | 20200124033436.81097-2-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add git-bugreport tool | expand |
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
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
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