| Message ID | 20200214015343.201946-4-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7,01/15] help: move list_config_help to builtin/help | expand |
Emily Shaffer <emilyshaffer@google.com> writes:
> + switch (safe_create_leading_directories(report_path.buf)) {
This helper is about creating paths in the working tree and Git
repository, hence it has a call to adjust_shared_perm() which in
turn calls get_shared_repo(), i.e. requiring a repository.
I thought I read somewhere that this tool is meant to be usable
outside a repository? If that is not the case, then the use of this
helper is OK. If not, we may want to make sure that it will stay to
be safe to use the helper (I think it happens to be OK right now,
but if the reason why the user is trying to run the tool is because
the user broke Git by writing garbage into .git/config, we may
die("your configuration file is broken") before this helper returns).
Thanks.
On Fri, Feb 14, 2020 at 09:25:12AM -0800, Junio C Hamano wrote: > Emily Shaffer <emilyshaffer@google.com> writes: > > > + switch (safe_create_leading_directories(report_path.buf)) { > > This helper is about creating paths in the working tree and Git > repository, It's also used by cmd_format_patch() with --output-directory specified, which is how I found it. My usual workflow is to run format-patch with -o ~/mailed-patches/topic/ specified so I don't clutter my repo, so I'm surprised to hear the intent of safe_create_leading_directores() is for paths in the working tree or repo. > hence it has a call to adjust_shared_perm() which in > turn calls get_shared_repo(), i.e. requiring a repository. Hmmm. I was able to run it pretty happily from a nongit dir: emilyshaffer@podkayne:~$ tg bugreport -o other/very/deep/path/ Created new report at 'other/very/deep/path/git-bugreport-2020-02-14-1721.txt'. emilyshaffer@podkayne:~$ cat other/very/deep/path/git-bugreport-2020-02-14-1721.txt Thank you for filling out a Git bug report! > I thought I read somewhere that this tool is meant to be usable > outside a repository? If that is not the case, then the use of this > helper is OK. If not, we may want to make sure that it will stay to > be safe to use the helper (I think it happens to be OK right now, > but if the reason why the user is trying to run the tool is because > the user broke Git by writing garbage into .git/config, we may > die("your configuration file is broken") before this helper returns). Can you explain a little more about what you mean? A broken local .git/config seems to be preventing git.c from dispatching the 'bugreport' subcommand (lots of other Git commands are broken) at all - breakpoints in cmd_main() right after the variable declarations are not being hit. With junk in the .git/config, I can't run bugreport from within that repo. With junk in ~/.gitconfig, I can't run bugreport anywhere. (To make the configs invalid, I leaned on my keyboard and added a line 'garbaghe~*~$%)%)(@' to the bottom of the config file. Most commands terminate early with "fatal: bad config line 37 in file .git/config".) Do you mean there's some specific config that could be misconfigured and prevent that utility from working? - Emily
Emily Shaffer <emilyshaffer@google.com> writes: > On Fri, Feb 14, 2020 at 09:25:12AM -0800, Junio C Hamano wrote: >> Emily Shaffer <emilyshaffer@google.com> writes: >> >> > + switch (safe_create_leading_directories(report_path.buf)) { >> >> This helper is about creating paths in the working tree and Git >> repository, > > It's also used by cmd_format_patch() with --output-directory specified, > which is how I found it. And that is an example of a good use of this helper. What will be written out there should be visible by the same people as those who have access to the repository; get_shared_repo() and adjust_perm() based on what the repository you are taking patches from is perfectly sensible. And as it is format-patch, we know we have get_shared_repo() working, and we know which repository we are working on. Output directory for bugreport is on the same boat when we know the users are producing a report on their use of Git within a repository. It is not clear if the tool is started without any repository---it happens to do a random safe thing (i.e. I think get_shared_repo() gives PERM_UMASK, which tells adjust_perm() not to do anything especial) right now, but there is no guarantee that we will keep it working like that. Somebody may come and demaind get_shared_perm() to die() when outside a repository, for example, and that would break the nice property that lets bugreport working outside a repository. I just wanted to make sure that somebody will be keeping an eye to remind those who propose such a change in the future. A comment near where get_shared_repo() happens may be something that can be done with a minimum effort when code that depends on that property is introduced at the same time. >> I thought I read somewhere that this tool is meant to be usable >> outside a repository? If that is not the case, then the use of this >> helper is OK. If not, we may want to make sure that it will stay to >> be safe to use the helper (I think it happens to be OK right now, I am actually OK if we limit the use of this tool to "use with a repository that is not corrupt", as coping with these kinds of breakages that in the main Git executable we deem "needs manual intervention" inside a single process is too painful (it would have been easier to cope with these too if we stuck with a script that invokes many discrete commands and acts on their errors, but that is optimizing for rare case and not recommended). But we should tell users about the limitation and encourage them to ask for help in non automatable means. Thanks.
On Sat, Feb 15, 2020 at 10:24:40AM -0800, Junio C Hamano wrote: > Emily Shaffer <emilyshaffer@google.com> writes: > > > On Fri, Feb 14, 2020 at 09:25:12AM -0800, Junio C Hamano wrote: > >> Emily Shaffer <emilyshaffer@google.com> writes: > >> > >> > + switch (safe_create_leading_directories(report_path.buf)) { > >> > >> This helper is about creating paths in the working tree and Git > >> repository, > > > > It's also used by cmd_format_patch() with --output-directory specified, > > which is how I found it. > > And that is an example of a good use of this helper. What will be > written out there should be visible by the same people as those who > have access to the repository; get_shared_repo() and adjust_perm() > based on what the repository you are taking patches from is > perfectly sensible. And as it is format-patch, we know we have > get_shared_repo() working, and we know which repository we are > working on. > > Output directory for bugreport is on the same boat when we know the > users are producing a report on their use of Git within a > repository. It is not clear if the tool is started without any > repository---it happens to do a random safe thing (i.e. I think > get_shared_repo() gives PERM_UMASK, which tells adjust_perm() not to > do anything especial) right now, but there is no guarantee that we > will keep it working like that. Somebody may come and demaind > get_shared_perm() to die() when outside a repository, for example, > and that would break the nice property that lets bugreport working > outside a repository. Hm, this would break the convention of the name of safe_create_leading_directories() too though right? As I understood it, "safe_foo" in this codebase means "this function will not die()"? > > I just wanted to make sure that somebody will be keeping an eye to > remind those who propose such a change in the future. A comment > near where get_shared_repo() happens may be something that can be > done with a minimum effort when code that depends on that property > is introduced at the same time. Ok. I want to make sure I understand you right. I think you're saying, "This is OK, except if someone changes get_shared_repo() it could break, so we need a way to warn someone that it could break." It sounds like you suggested leaving a comment in the safe_create_leading_directories() helper. My own preference is to write a test so that it's explicit that we depend on that behavior, instead - it's easy to gloss over a comment or read a different part of the codebase, but it's hard to ignore a breaking test. It'd be trivial to add one, so I will in v8 - unless I misunderstood you. > > >> I thought I read somewhere that this tool is meant to be usable > >> outside a repository? If that is not the case, then the use of this > >> helper is OK. If not, we may want to make sure that it will stay to > >> be safe to use the helper (I think it happens to be OK right now, > > I am actually OK if we limit the use of this tool to "use with a > repository that is not corrupt", as coping with these kinds of > breakages that in the main Git executable we deem "needs manual > intervention" inside a single process is too painful (it would have > been easier to cope with these too if we stuck with a script that > invokes many discrete commands and acts on their errors, but that is > optimizing for rare case and not recommended). But we should tell > users about the limitation and encourage them to ask for help in non > automatable means. I think you're saying, "Mention this drawback in the manpage for git-bugreport." Sounds like a good idea to me, so I'll add it for v8. - Emily
On Tue, Feb 18, 2020 at 03:46:28PM -0800, Emily Shaffer wrote: > On Sat, Feb 15, 2020 at 10:24:40AM -0800, Junio C Hamano wrote: > > I am actually OK if we limit the use of this tool to "use with a > > repository that is not corrupt", as coping with these kinds of > > breakages that in the main Git executable we deem "needs manual > > intervention" inside a single process is too painful (it would have > > been easier to cope with these too if we stuck with a script that > > invokes many discrete commands and acts on their errors, but that is > > optimizing for rare case and not recommended). But we should tell > > users about the limitation and encourage them to ask for help in non > > automatable means. > > I think you're saying, "Mention this drawback in the manpage for > git-bugreport." Sounds like a good idea to me, so I'll add it for v8. I'm pretty unsure about how you wanted this to sound; rather than sending a v8 only to revise it a lot, I'll send the paragraph for wordsmithing beforehand. Is this what you meant? 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. - Emily
Hi Emily, On Thu, 13 Feb 2020, Emily Shaffer wrote: > diff --git a/bugreport.c b/bugreport.c > new file mode 100644 > index 0000000000..a9398e6a2a > --- /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); This would be the first time (at least that _I_ know of) that we use `-` in this way. We seem to use `!!` a lot more often. And now I wonder whether there is a reason for that `-` that I missed? Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> +int cmd_main(int argc, const char **argv) >> +{ >> +... >> + 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); > > This would be the first time (at least that _I_ know of) that we use `-` > in this way. We seem to use `!!` a lot more often. And now I wonder > whether there is a reason for that `-` that I missed? In general, our preferred way to report an error from API functions is to return a negative number and a success is reported by returning zero. The argument to exit(3), which the return value from the main() function essentially is, on the other hand, is expected to be a small non-negative integer. As long as we are in tight control of the range of the returned value from launch_editor() (i.e. it must return a small non-positive integer whose negation is suitable to be fed to exit(3)), the above is fine. The idiom "return !!fn();" is to canonicalize the value to 0 or 1 for the caller who asked "so, did fn() give us 0 or non-zero?", which can also be used as the value given to exit(3), and could be more appropriate under some conditions: - we MUST know launch_editor() returns zero if and only if it is successful. - we do not have to be confident to be in tight control of the range of the returned value from launch_editor() (e.g. it could return a positive upon an error). - we MUST NOT care to differenciate different error codes returned from launch_editor(). IOW, we must be fine to give the invoker of the program only 0 (success) or 1 (unspecified failure). Use of "return !!fn();" that is not to give to exit(3) is probably more common in our codebase. See apply.c::try_create_file() and the comment before the function about its possible return values for example.
On Wed, Feb 19, 2020 at 08:55:04AM -0800, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > >> +int cmd_main(int argc, const char **argv) > >> +{ > >> +... > >> + 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); > > > > This would be the first time (at least that _I_ know of) that we use `-` > > in this way. We seem to use `!!` a lot more often. And now I wonder > > whether there is a reason for that `-` that I missed? > > In general, our preferred way to report an error from API functions > is to return a negative number and a success is reported by > returning zero. > > The argument to exit(3), which the return value from the main() > function essentially is, on the other hand, is expected to be a > small non-negative integer. As long as we are in tight control of > the range of the returned value from launch_editor() (i.e. it must > return a small non-positive integer whose negation is suitable to be > fed to exit(3)), the above is fine. > > The idiom "return !!fn();" is to canonicalize the value to 0 or 1 > for the caller who asked "so, did fn() give us 0 or non-zero?", > which can also be used as the value given to exit(3), and could be > more appropriate under some conditions: In editor.c, launch_editor() returns launch_specified_editor() without altering the return code. > > - we MUST know launch_editor() returns zero if and only if it is > successful. launch_specified_editor() has a handful of exit points, of three kinds: 1. return error(something) 2. raise(sigsomething) 3. return 0 a. when the editor process closed happily, but the user supplied NULL instead of a buffer. That is, the user didn't want the contents of the editor given back to them in a strbuf. b. when the editor process closed happily and the user's supplied buffer was filled with the file's contents with no issue. So I think we can check "yes" here. > > - we do not have to be confident to be in tight control of the > range of the returned value from launch_editor() (e.g. it could > return a positive upon an error). According to 'man 3 raise' (POSIX), raise() exits the current thread. If it can't exit itself(?) it seems it returns "nonzero for failure". We've also got a mingw_raise() in compat/mingw.h which could be used instead; this one seems to return -1 or the result of raise(), presumably the one from the MSC runtime[1], which also returns "a nonzero value" if not successful. So it's true that we aren't confident that launch_editor() returns a positive or negative value. > - we MUST NOT care to differenciate different error codes returned > from launch_editor(). IOW, we must be fine to give the invoker > of the program only 0 (success) or 1 (unspecified failure). This part is a little sticking point for me; I think I'd rather give the user some hint of what's broken than no hint at all, especially in this scenario, where it's conceivable someone could say "I tried to run 'git-bugreport' and it crashed, here is the return code and I have manually collected some relevant info too". The uncertainty coming from raise() (POSIX and MSCR both) leads me to believe if I do want to return the error on exit, I should at least check the sign before I flip it. Anybody else have opinions? - Emily [1] https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/raise?view=vs-2019
Emily Shaffer <emilyshaffer@google.com> writes: > launch_specified_editor() has a handful of exit points, of three kinds: > 1. return error(something) > 2. raise(sigsomething) > 3. return 0 > a. when the editor process closed happily, but the user supplied > NULL instead of a buffer. That is, the user didn't want the > contents of the editor given back to them in a strbuf. > b. when the editor process closed happily and the user's supplied > buffer was filled with the file's contents with no issue. > > So I think we can check "yes" here. Heh. If we raised a signal to kill ourselves, then we won't be returning a value from launch_editor() anyway. That case won't affect the "between returning negation or !!, which is more appropriate?" discussion, I think. >> - we MUST NOT care to differenciate different error codes returned >> from launch_editor(). IOW, we must be fine to give the invoker >> of the program only 0 (success) or 1 (unspecified failure). I actually think this holds for the codepath. A failure from start_command() returns error(), and finish_command() that waits for the spawned editor process to complete yields the exit status from the editor, but unless we re-raise the signal that killed the editor process to ourselves, we just turn any non-zero exit into "return error()", so it is safe to say launch_editor() can return either 0 or -1 and nothing else. Would we later want to tell callers of launch_editor() how/why the editor session failed by returning something else? I do not offhand think of any---we do not even differenciate between failure to start (e.g. misspelt command name for the editor) and other failures WITH the return value and consider it sufficient to tell the user with different error message right now. So in practice returning -launch_editor() and !!launch_editor() would not make any difference, I would think.
On Wed, Feb 19, 2020 at 02:09:48PM -0800, Junio C Hamano wrote: > Emily Shaffer <emilyshaffer@google.com> writes: > > > launch_specified_editor() has a handful of exit points, of three kinds: > > 1. return error(something) > > 2. raise(sigsomething) > > 3. return 0 > > a. when the editor process closed happily, but the user supplied > > NULL instead of a buffer. That is, the user didn't want the > > contents of the editor given back to them in a strbuf. > > b. when the editor process closed happily and the user's supplied > > buffer was filled with the file's contents with no issue. > > > > So I think we can check "yes" here. > > Heh. If we raised a signal to kill ourselves, then we won't be > returning a value from launch_editor() anyway. That case won't > affect the "between returning negation or !!, which is more > appropriate?" discussion, I think. > > >> - we MUST NOT care to differenciate different error codes returned > >> from launch_editor(). IOW, we must be fine to give the invoker > >> of the program only 0 (success) or 1 (unspecified failure). > > I actually think this holds for the codepath. A failure from > start_command() returns error(), and finish_command() that waits for > the spawned editor process to complete yields the exit status from > the editor, but unless we re-raise the signal that killed the editor > process to ourselves, we just turn any non-zero exit into "return > error()", so it is safe to say launch_editor() can return either 0 > or -1 and nothing else. Would we later want to tell callers of > launch_editor() how/why the editor session failed by returning > something else? I do not offhand think of any---we do not even > differenciate between failure to start (e.g. misspelt command name > for the editor) and other failures WITH the return value and > consider it sufficient to tell the user with different error > message right now. > > So in practice returning -launch_editor() and !!launch_editor() > would not make any difference, I would think. Then, let's do the least surprising thing. I'll switch it to !! for the next reroll.
On Tue, Feb 18, 2020 at 03:56:22PM -0800, Emily Shaffer wrote: > On Tue, Feb 18, 2020 at 03:46:28PM -0800, Emily Shaffer wrote: > > On Sat, Feb 15, 2020 at 10:24:40AM -0800, Junio C Hamano wrote: > > > I am actually OK if we limit the use of this tool to "use with a > > > repository that is not corrupt", as coping with these kinds of > > > breakages that in the main Git executable we deem "needs manual > > > intervention" inside a single process is too painful (it would have > > > been easier to cope with these too if we stuck with a script that > > > invokes many discrete commands and acts on their errors, but that is > > > optimizing for rare case and not recommended). But we should tell > > > users about the limitation and encourage them to ask for help in non > > > automatable means. > > > > I think you're saying, "Mention this drawback in the manpage for > > git-bugreport." Sounds like a good idea to me, so I'll add it for v8. > > I'm pretty unsure about how you wanted this to sound; rather than > sending a v8 only to revise it a lot, I'll send the paragraph for > wordsmithing beforehand. Is this what you meant? > > 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. Since I saw lots of replies from Junio elsewhere in this thread, I'll take the silence here as assent. v8 on its way momentarily. - Emily
Emily Shaffer <emilyshaffer@google.com> writes: > On Tue, Feb 18, 2020 at 03:56:22PM -0800, Emily Shaffer wrote: >> On Tue, Feb 18, 2020 at 03:46:28PM -0800, Emily Shaffer wrote: >> > On Sat, Feb 15, 2020 at 10:24:40AM -0800, Junio C Hamano wrote: >> > > I am actually OK if we limit the use of this tool to "use with a >> > > repository that is not corrupt", as coping with these kinds of >> > > breakages that in the main Git executable we deem "needs manual >> > > intervention" inside a single process is too painful (it would have >> > > been easier to cope with these too if we stuck with a script that >> > > invokes many discrete commands and acts on their errors, but that is >> > > optimizing for rare case and not recommended). But we should tell >> > > users about the limitation and encourage them to ask for help in non >> > > automatable means. >> > >> > I think you're saying, "Mention this drawback in the manpage for >> > git-bugreport." Sounds like a good idea to me, so I'll add it for v8. >> >> I'm pretty unsure about how you wanted this to sound; rather than >> sending a v8 only to revise it a lot, I'll send the paragraph for >> wordsmithing beforehand. Is this what you meant? >> >> 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. > > Since I saw lots of replies from Junio elsewhere in this thread, I'll > take the silence here as assent. v8 on its way momentarily. Heh, it's just I have too little time to allocate on this single topic X-<. Don't take silence as anything else.
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..a9398e6a2a --- /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..6585a2d144 --- /dev/null +++ b/t/t0091-bugreport.sh @@ -0,0 +1,55 @@ +#!/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_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 | 41 ++++++++++++++ Makefile | 5 ++ bugreport.c | 94 +++++++++++++++++++++++++++++++++ command-list.txt | 1 + t/t0091-bugreport.sh | 55 +++++++++++++++++++ 6 files changed, 197 insertions(+) create mode 100644 Documentation/git-bugreport.txt create mode 100644 bugreport.c create mode 100755 t/t0091-bugreport.sh