On Thu, Jan 30, 2020 at 11:18:55PM +0100, Martin Ågren wrote:

On Fri, 24 Jan 2020 at 05:56, [off-list ref] wrote:

quoted

From: Emily Shaffer <redacted>

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"? ;-) )

quoted

+SYNOPSIS
+--------
+[verse]
+'git bugreport' [-o | --output <path>]

Hmm. Should that be "[(-o | --output) <path>]"?

Done.

quoted

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

quoted

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

Sure, agreed.

quoted

+OPTIONS
+-------
+-o [<path>]::
+--output [<path>]::

Drop the "[" and "]"? If you give -o, you'd better give a path as well?

Done.

quoted

+       Place the resulting bug report file in <path> instead of the root of the

`<path>`

Done.

quoted

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

Nice turn of phrase, done.

quoted

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

Sure.

quoted

+       strbuf_addstr(&report_path, "git-bugreport-");
+       strbuf_addftime(&report_path, "%F", gmtime(&now), 0, 0);
+       strbuf_addstr(&report_path, ".txt");
+
+

(Double blank line?)

Done.
 

quoted

+       get_bug_template(&buffer);
+
+       report = fopen_for_writing(report_path.buf);

Report might be NULL here.

Nice.

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?

Sure. For the sake of brevity I'll probably neglect seconds; I hope
someone is spending more than 1 minute filling in the provided form.

I'm a little worried about including : in a filename, so I went for
'git-bugreport-YYYY-MM-DD-HHMM' (24-hour).

As I started to write a test to ensure duplicate filenames were handled
well, Jonathan Tan pointed out that it would be easy to add an arg like
--suffix to allow specifying a custom strftime string. That would allow
users to easily create a file named
`git-bugreport-fetch-failing.txt` or `git-bugreport-March-19.txt` or
whatever they want; it also makes testing easy. So I'll add this for the
next rollup.

quoted

+       strbuf_write(&buffer, report);
+       fclose(report);

Maybe clear the strbuf around here...

quoted

+       launch_editor(report_path.buf, NULL, NULL);
+       return 0;

... and/or UNLEAK it here, together with report_path.

Maybe "return -launch_editor(...)"?

Hm, sure. I see that builtin/tag.c does mark strbufs this way, so I
don't see a problem using UNLEAK and tail-calling launch_editor().


As a final bonus, I also added a line to report to stderr the name of
the file that was created. I noticed it's sort of unclear what the
command actually did otherwise.

quoted

+#!/bin/bash

Use /bin/sh instead?

Yeah, doing so immediately pointed out the bashisms you mentioned, plus
some more. :facepalm:

quoted

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

quoted

+                       read -r nextline
+                       if [-z $nextline]; then

Likewise.

quoted

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

Yeah, at the moment I just added a die() if we can't open the provided
path. I think other utilties can create the path (e.g. git-format-patch)
but where it makes sense to do so, I'd prefer to keep bugreport very
simple.

I'll die instead of overwriting, too; you're right that spending quite a
while on a bug report and then accidentally writing over it with a blank
one would be a very bad user experience.

quoted

+       git bugreport -o foo/ &&
+       test -f foo/git-bugreport-* &&

test_path_is_file

Sure.

quoted

+       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

OK.

Thanks very much, Martin, for the thorough review. This is incredibly
helpful.

 - Emily