| Message ID | 20200220015858.181086-4-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add git-bugreport tool | expand |
Emily Shaffer <emilyshaffer@google.com> writes: > + argc = parse_options(argc, argv, "", bugreport_options, > + bugreport_usage, 0); Which one between an empty string and NULL is more appropriate to be passed as "prefix" here? I assume that this is *not* really a git program, and no repository/working-tree discovery is involved, and you won't be using OPT_FILENAME, so it would probably be OK. > + > + 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); Missed i18n/l10n, but I do not think it is a useful thing for this check to be here in the first place. > + switch (safe_create_leading_directories(report_path.buf)) { > + case SCLD_OK: > + case SCLD_EXISTS: > + break; > + default: > + die(_("could not create leading directories for '%s'"), > + report_path.buf); > + } > + > + get_bug_template(&buffer); > + > + report = fopen_for_writing(report_path.buf); fopen_for_writing() does not give good semantics for what you seem to want to do here (i.e. to make sure you do not overwrite an existing one). It even has "if we cannot open it, retry after unlinking" logic in it, which screams "this function is designed for those who want to overwrite the file", and if you look at its callers, they are _all_ about opening a file for writing with a well known name like FETCH_HEAD, COMMIT_EDITMSG, etc. Besides, a stat() that dies when a file exists would not help---since that check, you've spent non-zero time, creating leading directories and preparing boilerplate in the buffer, and there is no guarantee somebody else used the same filename in the meantime. If you want to avoid overwriting an existing file (which I think is a good idea---I just do not think the earlier "if (!stat()) die()" is a good way to do so), the only sensible way to do so is to ask your open/creat to fail if there already is a file---that is how you'd avoid racing with another process that may be creating such a file. Grep for O_EXCL to find where the flag is used with O_CREAT to see how it is done. > + if (report == NULL) { Style. "if (!report)" > + strbuf_release(&report_path); > + die("couldn't open '%s' for writing", report_path.buf); Do we see a use-after-free here? Also missed i18n/l10n. It is embarrassing on the reviewers' side that this (which is not new in this round at all and hasn't changed since v6) hasn't been caught for a few rounds X-<. Thanks.
On Thu, Feb 20, 2020 at 11:33:26AM -0800, Junio C Hamano wrote: > Emily Shaffer <emilyshaffer@google.com> writes: > > > + argc = parse_options(argc, argv, "", bugreport_options, > > + bugreport_usage, 0); > > Which one between an empty string and NULL is more appropriate to be > passed as "prefix" here? I assume that this is *not* really a git > program, and no repository/working-tree discovery is involved, and > you won't be using OPT_FILENAME, so it would probably be OK. > > > + > > + 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); > > Missed i18n/l10n, but I do not think it is a useful thing for this > check to be here in the first place. > > > + switch (safe_create_leading_directories(report_path.buf)) { > > + case SCLD_OK: > > + case SCLD_EXISTS: > > + break; > > + default: > > + die(_("could not create leading directories for '%s'"), > > + report_path.buf); > > + } > > + > > + get_bug_template(&buffer); > > + > > + report = fopen_for_writing(report_path.buf); > > fopen_for_writing() does not give good semantics for what you seem > to want to do here (i.e. to make sure you do not overwrite an > existing one). It even has "if we cannot open it, retry after > unlinking" logic in it, which screams "this function is designed for > those who want to overwrite the file", and if you look at its > callers, they are _all_ about opening a file for writing with a well > known name like FETCH_HEAD, COMMIT_EDITMSG, etc. > > Besides, a stat() that dies when a file exists would not > help---since that check, you've spent non-zero time, creating > leading directories and preparing boilerplate in the buffer, > and there is no guarantee somebody else used the same filename > in the meantime. Good point. Ouch. > > If you want to avoid overwriting an existing file (which I think is > a good idea---I just do not think the earlier "if (!stat()) die()" > is a good way to do so), the only sensible way to do so is to ask > your open/creat to fail if there already is a file---that is how > you'd avoid racing with another process that may be creating such a > file. Grep for O_EXCL to find where the flag is used with O_CREAT > to see how it is done. Sure. Thanks, I reworked it to use open(). By the way, I noticed reading the GNU manual that file descriptors may not be supported outside of GNU environments; but I also noticed that 1) Git uses open() lots of places to use O_EXCL, and 2) fopen() doesn't support exclusive opens (except in glibc, and nobody is using that particular option in the Git codebase now). So I guess it's safe to use open()... > > > + if (report == NULL) { > > Style. "if (!report)" The type of 'report' changes using open(), so this check changes too. Now it says "if (report < 0)" - per the open() doc, it returns a positive fd or -1. > > > + strbuf_release(&report_path); > > + die("couldn't open '%s' for writing", report_path.buf); > > Do we see a use-after-free here? Also missed i18n/l10n. Hm. I suppose it's OK to UNLEAK() like we do at the successful exit since the die() will end the process. Added i18n too. > It is embarrassing on the reviewers' side that this (which is not > new in this round at all and hasn't changed since v6) hasn't been > caught for a few rounds X-<. Well, I'm embarrassed to have written it in the first place. - Emily
Hi Emily, On Wed, 19 Feb 2020, Emily Shaffer wrote: > diff --git a/bugreport.c b/bugreport.c > new file mode 100644 > index 0000000000..8d4a76fdac > --- /dev/null > +++ b/bugreport.c > @@ -0,0 +1,94 @@ > +#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"; This is not a portable `strftime()` format. Let's squash this in? -- snipsnap -- Subject: [PATCH] fixup??? bugreport: add tool to generate debugging info The `%F` format is an optional extension to the ISO C standard, see https://pubs.opengroup.org/onlinepubs/9699919799/functions/strftime.html And sure enough, in Git for Windows, this leads to a test failure because that format is not supported in the version of the MSVC runtime that we're stuck with. Let's just use the long-hand instead. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- bugreport.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bugreport.c b/bugreport.c index a3528a2e2b8..3f78db182e6 100644 --- a/bugreport.c +++ b/bugreport.c @@ -338,7 +338,7 @@ int cmd_main(int argc, const char **argv) FILE *report; time_t now = time(NULL); char *option_output = NULL; - char *option_suffix = "%F-%H%M"; + char *option_suffix = "%Y-%m-%d-%H%M"; struct stat statbuf; int nongit_ok = 0; -- 2.25.1.windows.1
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..1f9fde5cde --- /dev/null +++ b/Documentation/git-bugreport.txt @@ -0,0 +1,46 @@ +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 + +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 +the kind of information listed above when manually asking for help. + +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 c552312d3f..9e6705061d 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 @@ -2461,6 +2462,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..8d4a76fdac --- /dev/null +++ b/bugreport.c @@ -0,0 +1,94 @@ +#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); + + switch (safe_create_leading_directories(report_path.buf)) { + case SCLD_OK: + case SCLD_EXISTS: + break; + default: + die(_("could not create leading directories for '%s'"), + 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/command-list.txt b/command-list.txt index 2087894655..185e5e3f05 100644 --- a/command-list.txt +++ b/command-list.txt @@ -54,6 +54,7 @@ git-archive mainporcelain git-bisect mainporcelain info git-blame ancillaryinterrogators complete git-branch mainporcelain history +git-bugreport ancillaryinterrogators git-bundle mainporcelain git-cat-file plumbinginterrogators git-check-attr purehelpers diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh new file mode 100755 index 0000000000..65f664fdac --- /dev/null +++ b/t/t0091-bugreport.sh @@ -0,0 +1,61 @@ +#!/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 -s check-headers && + check_all_headers_populated <git-bugreport-check-headers.txt && + test_when_finished rm git-bugreport-check-headers.txt +' + +test_expect_success 'dies if file with same name as report already exists' ' + >>git-bugreport-duplicate.txt && + test_must_fail git bugreport --suffix duplicate && + test_when_finished rm git-bugreport-duplicate.txt +' + +test_expect_success '--output-directory puts the report in the provided dir' ' + git bugreport -o foo/ && + test_path_is_file foo/git-bugreport-* && + test_when_finished rm -fr foo/ +' + +test_expect_success 'incorrect arguments abort with usage' ' + test_must_fail git bugreport --false 2>output && + test_i18ngrep usage output && + test_path_is_missing git-bugreport-* +' + +test_expect_success 'runs outside of a git dir' ' + nongit git bugreport && + test_when_finished rm non-repo/git-bugreport-* +' + +test_expect_success 'can create leading directories outside of a git dir' ' + nongit git bugreport -o foo/bar/baz && + test_when_finished rm -fr foo/bar/baz +' + + +test_done
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 | 46 ++++++++++++++++ Makefile | 5 ++ bugreport.c | 94 +++++++++++++++++++++++++++++++++ command-list.txt | 1 + t/t0091-bugreport.sh | 61 +++++++++++++++++++++ 6 files changed, 208 insertions(+) create mode 100644 Documentation/git-bugreport.txt create mode 100644 bugreport.c create mode 100755 t/t0091-bugreport.sh