Re: [PATCH] bugreport: add tool to generate debugging info

[Junio C Hamano <gitster@pobox.com>]

Emily Shaffer <emilyshaffer@google.com> writes:

> diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
> new file mode 100644
> index 0000000000..c5f45bbee8
> --- /dev/null
> +++ b/Documentation/git-bugreport.txt
> @@ -0,0 +1,48 @@
> +git-bugreport(1)
> +================
> +
> +NAME
> +----
> +git-bugreport - Collect information for user to file a bug report
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'git bugreport' [-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.
> +
> +The following information is requested from the user:
> +
> + - Reproduction steps
> + - Expected behavior
> + - Actual behavior

+ - How the above two are different

It is often helpful to have users explain how the expected and
actual are different in their own words.

> +NOTE
> +----
> +Bug reports can be sent to git@vger.kernel.org.

I am not sure if this belongs here.

> diff --git a/git-bugreport.sh b/git-bugreport.sh
> new file mode 100755
> index 0000000000..2200703a51
> --- /dev/null
> +++ b/git-bugreport.sh
> @@ -0,0 +1,86 @@
> +#!/bin/sh
> +
> +print_filenames_and_content() {
> +while read -r line; do
> +	echo "$line";
> +	echo "========";
> +	cat "$line";
> +	echo;
> +done
> +}

Style.

 - have SP on both sides of ()
 - one more HT indent for the function body
 - "do" on its own line
 - no unnecessary semicolons when LF would do

You probably are better off asking xargs to do this instead of
relying on "read -r".

	find ... | xargs -n 1 sh -c 'echo "$1" && cat "$1"' -

or something like that, perhaps.

> +
> +generate_report_text() {
> +
> +	# Generate a form for the user to fill out with their issue.
> +	gettextln "Thank you for filling out a Git bug report!"
> +	gettextln "Please answer the following questions to help us understand your issue."
> +	echo
> +	gettextln "What did you do before the bug happened? (Steps to reproduce your issue)"
> +	echo
> +	gettextln "What did you expect to happen? (Expected behavior)"
> +	echo
> +	gettextln "What happened instead? (Actual behavior)"
> +	echo
> +	gettextln "Anything else you want to add:"
> +	echo
> +	gettextln "Please review the rest of the bug report below."
> +	gettextln "You can delete any lines you don't wish to send."
> +	echo

Would we on the receiving end be able to tell these section headers
in translated to 47 different languages?  I am sure that i18n is
used here to encourage non-C-locale users to file bugs in their own
languages, but are we prepared to react to them?

> +	echo "[System Information]"
> +	git version --build-options
> +	uname -a
> +	curl-config --version
> +	ldd --version

"curl-config: command not found" may be clear enough, but would
there be a case where errors from two or more consecutive commands
in the above sequence would make the output confusing to the person
sitting on the receiving end?  Would it help, as a convention, to
always ahve "echo [what am I reporting]" before each of these
commands?

> +# Create bugreport file
> +BUGREPORT_FILE="$basedir/git-bugreport-$(whoami)-$(hostname)-$(date -Iminutes)"

How portable is -Iminutes?

> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> new file mode 100755
> index 0000000000..6eb2ee4f66
> --- /dev/null
> +++ b/t/t0091-bugreport.sh
> @@ -0,0 +1,41 @@
> +#!/bin/bash

Make sure /bin/sh suffices to run the test script.

> +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 [$(grep $HEADER_PATTERN $line)]; then

Documentation/CodingGuidelines

I am not sure if the traits this test script checks about the
contents of the output is all that interesting.  Whenever we add new
sections to the main command because we want other kinds of
information collected, we'd update this test script because
otherwise the test would fail, but would it result in quality
bugreport tool, or is it just additional busywork?

If we decide later that a header and its body needs to be separated
with a blank line for better readablity, the check done here would
also need to be updated, but again, that does not feel like anything
more than just busywork to me.

The tests for "-o" and unknown options do make sense, though.

Thanks.

> +# 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
> +			read -r nextline
> +			if [-z $nextline]; then
> +				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/ &&
> +	git bugreport -o foo/ &&
> +	test -f foo/git-bugreport-* &&
> +	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_done