Re: [PATCH v9 2/5] bugreport: add tool to generate debugging info

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Mon, 2 Mar 2020, Emily Shaffer wrote:

>  .gitignore                      |   1 +
>  Documentation/git-bugreport.txt |  46 ++++++++++++++
>  Makefile                        |   5 ++
>  bugreport.c                     | 105 ++++++++++++++++++++++++++++++++
>  command-list.txt                |   1 +
>  strbuf.c                        |   4 ++
>  strbuf.h                        |   1 +
>  t/t0091-bugreport.sh            |  61 +++++++++++++++++++
>  8 files changed, 224 insertions(+)
>  create mode 100644 Documentation/git-bugreport.txt
>  create mode 100644 bugreport.c
>  create mode 100755 t/t0091-bugreport.sh

Hmm. I am still _quite_ convinced that this would be much better as a
built-in. Remember, non-built-ins come with a footprint, and I do not
necessarily think that you will want to spend 3MB on a `git-bugreport`
executable when you could have it for a couple dozen kilobytes inside
`git` instead.

Ciao,
Dscho

>
> 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..1f9fde5cde
> --- /dev/null
> +++ b/Documentation/git-bugreport.txt
> @@ -0,0 +1,46 @@
> +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
> +
> +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.
> +
> +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 c552312d3f..9e6705061d 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
> @@ -2461,6 +2462,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..f473d606f2
> --- /dev/null
> +++ b/bugreport.c
> @@ -0,0 +1,105 @@
> +#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;
> +	int report = -1;
> +	time_t now = time(NULL);
> +	char *option_output = NULL;
> +	char *option_suffix = "%F-%H%M";
> +	int nongit_ok = 0;
> +	const char *prefix = NULL;
> +	const char *user_relative_path = NULL;
> +
> +	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()
> +	};
> +
> +	prefix = setup_git_directory_gently(&nongit_ok);
> +
> +	argc = parse_options(argc, argv, prefix, bugreport_options,
> +			     bugreport_usage, 0);
> +
> +	/* Prepare the path to put the result */
> +	strbuf_addstr(&report_path,
> +		      prefix_filename(prefix,
> +				      option_output ? 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");
> +
> +	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);
> +	}
> +
> +	/* Prepare the report contents */
> +	get_bug_template(&buffer);
> +
> +	/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */
> +	report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);
> +
> +	if (report < 0) {
> +		UNLEAK(report_path);
> +		die(_("couldn't create a new file at '%s'"), report_path.buf);
> +	}
> +
> +	strbuf_write_fd(&buffer, report);
> +	close(report);
> +
> +	/*
> +	 * We want to print the path relative to the user, but we still need the
> +	 * path relative to us to give to the editor.
> +	 */
> +	if (!(prefix && skip_prefix(report_path.buf, prefix, &user_relative_path)))
> +		user_relative_path = report_path.buf;
> +	fprintf(stderr, _("Created new report at '%s'.\n"),
> +		user_relative_path);
> +
> +	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/strbuf.c b/strbuf.c
> index f19da55b07..f1d66c7848 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -539,6 +539,10 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f)
>  	return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
>  }
>
> +ssize_t strbuf_write_fd(struct strbuf *sb, int fd)
> +{
> +	return sb->len ? write(fd, sb->buf, sb->len) : 0;
> +}
>
>  #define STRBUF_MAXLINK (2*PATH_MAX)
>
> diff --git a/strbuf.h b/strbuf.h
> index aae7ac3a82..bbf6204de7 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -462,6 +462,7 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
>   * NUL bytes.
>   */
>  ssize_t strbuf_write(struct strbuf *sb, FILE *stream);
> +ssize_t strbuf_write_fd(struct strbuf *sb, int fd);
>
>  /**
>   * Read a line from a FILE *, overwriting the existing contents of
> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> new file mode 100755
> index 0000000000..65f664fdac
> --- /dev/null
> +++ b/t/t0091-bugreport.sh
> @@ -0,0 +1,61 @@
> +#!/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_expect_success 'can create leading directories outside of a git dir' '
> +	nongit git bugreport -o foo/bar/baz &&
> +	test_when_finished rm -fr foo/bar/baz
> +'
> +
> +
> +test_done
> --
> 2.25.0.265.gbab2e86ba0-goog
>
>