Re: [PATCH v5 01/15] bugreport: add tool to generate debugging info

> 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?

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

> 
> > +       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(...)"?

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