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

Message ID 20200214015343.201946-4-emilyshaffer@google.com
State New
Headers show
Series
  • [v7,01/15] help: move list_config_help to builtin/help
Related show

Commit Message

Emily Shaffer Feb. 14, 2020, 1:53 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                     | 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

Comments

Junio C Hamano Feb. 14, 2020, 5:25 p.m. UTC | #1
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.
Emily Shaffer Feb. 15, 2020, 1:57 a.m. UTC | #2
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
Junio C Hamano Feb. 15, 2020, 6:24 p.m. UTC | #3
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.
Emily Shaffer Feb. 18, 2020, 11:46 p.m. UTC | #4
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
Emily Shaffer Feb. 18, 2020, 11:56 p.m. UTC | #5
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
Johannes Schindelin Feb. 19, 2020, 2:18 p.m. UTC | #6
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
Junio C Hamano Feb. 19, 2020, 4:55 p.m. UTC | #7
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.
Emily Shaffer Feb. 19, 2020, 9:52 p.m. UTC | #8
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
Junio C Hamano Feb. 19, 2020, 10:09 p.m. UTC | #9
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.
Emily Shaffer Feb. 19, 2020, 11:06 p.m. UTC | #10
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.
Emily Shaffer Feb. 19, 2020, 11:15 p.m. UTC | #11
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
Junio C Hamano Feb. 19, 2020, 11:24 p.m. UTC | #12
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.

Patch
diff mbox series

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