On Fri, 24 Jan 2020 at 05:56, <emilyshaffer@xxxxxxxxxx> wrote:
>
> From: Emily Shaffer <emilyshaffer@xxxxxxxxxx>
>
> Teach Git how to prompt the user for a good bug report: reproduction
> steps, expected behavior, and actual behavior. Later, Git can learn how
> to collect some diagnostic information from the repository.
("Later" meaning "later in this series", or "any day now"? ;-) )
> +SYNOPSIS
> +--------
> +[verse]
> +'git bugreport' [-o | --output <path>]
Hmm. Should that be "[(-o | --output) <path>]"?
> +DESCRIPTION
> +-----------
> +Captures information about the user's machine, Git client, and repository state,
> +as well as a form requesting information about the behavior the user observed,
> +into a single text file which the user can then share, for example to the Git
> +mailing list, in order to report an observed bug.
Nice description. Got it.
> +The following information is requested from the user:
> +
> + - Reproduction steps
> + - Expected behavior
> + - Actual behavior
> +
> +The following information is captured automatically:
> +
> + - Git version (`git version --build-options`)
> + - Machine information (`uname -a`)
> + - Versions of various dependencies
> + - Git config contents (`git config --show-origin --list`)
> + - The names of all configured git-hooks in `.git/hooks/`
I would have expected these points to appear later, both to make it
clear what this does commit does (and not), and to highlight what
user-visible (documentation-worthy) changes later commits bring along.
> +OPTIONS
> +-------
> +-o [<path>]::
> +--output [<path>]::
Drop the "[" and "]"? If you give -o, you'd better give a path as well?
> + Place the resulting bug report file in <path> instead of the root of the
`<path>`
> +"Please review the rest of the bug report below.\n"
> +"You can delete any lines you don't wish to send.\n");
"send" sounds like we're *just* about to send this report somewhere, but
it's "only" going to be written to the disk. Maybe "share", instead?
> + if (option_output) {
> + strbuf_addstr(&report_path, option_output);
> + strbuf_complete(&report_path, '/');
> + }
I thought I'd use `-o` to indicate the filename, but it turns out it's
the *directory* where the file (of some semi-random, generated name)
will end up. Re-reading the docs further up, I can see how this is
consistent. I sort of wonder if this should be `--output*-directory*`
for symmetry with `git format-patch`.
> + strbuf_addstr(&report_path, "git-bugreport-");
> + strbuf_addftime(&report_path, "%F", gmtime(&now), 0, 0);
> + strbuf_addstr(&report_path, ".txt");
> +
> +
(Double blank line?)
> + get_bug_template(&buffer);
> +
> + report = fopen_for_writing(report_path.buf);
Report might be NULL here.
If there's already such a file, we overwrite. Should we generate the
filename using not just today's date (two bug reports in a day wouldn't
be unheard of?) but also something like hh:mm:ss?
> + strbuf_write(&buffer, report);
> + fclose(report);
Maybe clear the strbuf around here...
> + launch_editor(report_path.buf, NULL, NULL);
> + return 0;
... and/or UNLEAK it here, together with report_path.
Maybe "return -launch_editor(...)"?
> +#!/bin/bash
Use /bin/sh instead?
> +# Headers "[System Info]" will be followed by a non-empty line if we put some
> +# information there; we can make sure all our headers were followed by some
> +# information to check if the command was successful.
> +HEADER_PATTERN="^\[.*\]$"
> +check_all_headers_populated() {
> + while read -r line; do
> + if [$(grep $HEADER_PATTERN $line)]; then
I think this is a bash-ism.
> + read -r nextline
> + if [-z $nextline]; then
Likewise.
> + return 1;
> + fi
> + fi
> + done
> +}
> +
> +test_expect_success 'creates a report with content in the right places' '
> + git bugreport &&
> + check_all_headers_populated <git-bugreport-* &&
> + rm git-bugreport-*
> +'
> +
> +test_expect_success '--output puts the report in the provided dir' '
> + mkdir foo/ &&
If foo isn't there, do we not create it? Apparently not -- in my
testing, we segfault. (We don't check for NULL after opening the file.)
> + git bugreport -o foo/ &&
> + test -f foo/git-bugreport-* &&
test_path_is_file
> + rm -fr foo/
> +'
> +
> +test_expect_success 'incorrect arguments abort with usage' '
> + test_must_fail git bugreport --false 2>output &&
> + grep usage output &&
> + test ! -f git-bugreport-*
test_path_is_missing
Martin
