patchbugreport: add tool to generate debugging info

> Teach Git how to prompt the user for a good bug report: reproduction
> steps, expected behavior, and actual behavior. Also, teach Git to write
> down its own version, the version of some of its dependencies, the
> operating system information, the effective gitconfig, the configured
> hooks, and the contents of $GIT_DIR/logs. Finally, make sure Git asks
> the user to review the contents of the report after it's generated.
> 
> If users can send us a well-written bug report complete with system
> information, a remote we may be able to clone, and a reflog showing us
> where the problem occurred, we can reduce the number of questions and
> answers which must travel between the reporter and the Git contributor.
> 
> Users may also wish to send a report like this to their local "Git
> expert" if they have put their repository into a state they are confused
> by.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> There are some things I'm not certain about from this patch, I'd
> appreciate feedback.
> 
>  - Do we want to advertise the Git mailing list for bug reports?

>  - At the moment, the test is trying to check "everything we thought we
>    would populate got populated" - that seemed to me like it would hold
>    up best to changes to the set of information being collected. But
>    maybe there's something more robust we can do.
> 
> And of course, if there are suggestions for other things to include in
> the report I'm interested in adding.
> 
> An example of the output can be found here:
> https://gist.github.com/nasamuffin/2e320892f5c2147e829cbcf5bd0759a2
> 
> Thanks.
>  - Emily
> 
>  .gitignore                      |  1 +
>  Documentation/git-bugreport.txt | 48 ++++++++++++++++++
>  Makefile                        |  1 +
>  command-list.txt                |  1 +
>  git-bugreport.sh                | 86 +++++++++++++++++++++++++++++++++
>  t/t0091-bugreport.sh            | 41 ++++++++++++++++
>  6 files changed, 178 insertions(+)
>  create mode 100644 Documentation/git-bugreport.txt
>  create mode 100755 git-bugreport.sh
>  create mode 100755 t/t0091-bugreport.sh
> 
> diff --git a/.gitignore b/.gitignore
> index 521d8f4fb4..b4f5433084 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..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
> +
> +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 contents of all configured git-hooks in `.git/hooks/`
> + - The contents of `.git/logs`
> +
> +OPTIONS
> +-------
> +-o [<path>]::
> +--output [<path>]::
> +	Place the resulting bug report file in <path> instead of the root of the
> +	Git repository.
> +
> +NOTE
> +----
> +Bug reports can be sent to git@vger.kernel.org.
> +
> +GIT
> +---
> +Part of the linkgit:git[1] suite
> diff --git a/Makefile b/Makefile
> index f9255344ae..69801a1c45 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -606,6 +606,7 @@ TEST_PROGRAMS_NEED_X =
>  unexport CDPATH
>  
>  SCRIPT_SH += git-bisect.sh
> +SCRIPT_SH += git-bugreport.sh
>  SCRIPT_SH += git-difftool--helper.sh
>  SCRIPT_SH += git-filter-branch.sh
>  SCRIPT_SH += git-merge-octopus.sh
> diff --git a/command-list.txt b/command-list.txt
> index a9ac72bef4..be5a605047 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/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
> +}
> +
> +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
> +
> +	echo "[System Information]"
> +	git version --build-options
> +	uname -a
> +	curl-config --version
> +	ldd --version
> +	echo
> +
> +	echo "[Git Config]"
> +	# TODO: Pass this through grep -v to avoid users sending us their credentials.
> +	git config --show-origin --list
> +	echo

> +
> +}
> +
> +USAGE="[-o | --output <path>]"
> +
> +SUBDIRECTORY_OK=t
> +OPTIONS_SPEC=
> +. git-sh-setup
> +. git-sh-i18n
> +
> +basedir="$PWD"
> +while :
> +do
> +	case "$1" in
> +	-o|--output)
> +		shift
> +		basedir="$1"
> +		shift
> +		continue
> +		;;
> +	"")
> +		break
> +		;;
> +	*)
> +		usage
> +		;;
> +	esac
> +done
> +
> +
> +# Create bugreport file
> +BUGREPORT_FILE="$basedir/git-bugreport-$(whoami)-$(hostname)-$(date -Iminutes)"
> +
> +generate_report_text >$BUGREPORT_FILE
> +
> +git_editor $BUGREPORT_FILE
> +
> +eval_gettextln "Your new bug report is in \$BUGREPORT_FILE."
> 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
> +
> +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
> +			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
> 

>> +	echo "[Configured Hooks]"
>> +	find "$GIT_DIR/hooks/" -type f | grep -v "\.sample$" | print_filenames_and_content
>> +	echo
>
> Remove the sample hooks, but focus on the others. Will this look like garbage if a hook
> is a binary file?

> Derrick Stolee <stolee@gmail.com> writes:
> 
> > Config options to consider stripping out:
> >
> > 	*url*
> > 	*pass* (anything "password" but also "sendmail.smtppass")
> 
> Blacklisting?  I wonder if users feel safer if these are limited to
> known-benign ones.

> 
> >> +	echo "[Configured Hooks]"
> >> +	find "$GIT_DIR/hooks/" -type f | grep -v "\.sample$" | print_filenames_and_content
> >> +	echo
> >
> > Remove the sample hooks, but focus on the others. Will this look like garbage if a hook
> > is a binary file?
> 
> This makes me feel very nervous.  $GIT_DIR/hooks/ are private and
> people can hardcode credentials in them; $GIT_DIR/hooks/pre-foo may
> be written toread from $GIT_DIR/hooks/mypassword with the knowledge
> that there won't be any "mypassword" hook.

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > I think a whitelist of config options to print would grow stale
> > immediately, and the options we're missing would be very likely to be
> > configs to turn on new experimental features - which is probably what we
> > most want the bugreport for.
> 
> The implementation of your "git bugreport" command comes from the
> same version of Git as such an experimental feature you want
> feedback to is shipped with, so I am not sure where your concern
> about staleness comes from.
> 
> If you really care about getting quality reports, you need to earn
> users' trust (one way of doing so would be to maintain a good "list
> of benign configuration knobs whose values help diagnosing issues"),
> and you need to make sure we obtain relevant pieces of information.
> Both of these things are something you need to actively work on.
> 
> We have trained ourselves to the point that we consider it a bug if
> you add a new command git-bugreport without adding a corresponding
> pattern in the .gitignore file.  And thanks to that discipline, we
> have been fairly good at keeping .gitignore up to date.  I do not
> see a reason why we cannot do something similar to the registry of
> configuration variables that we care about, which may be shipped as
> part of "git bugreport" command.

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > I think comparing this habit to the .gitignore isn't quite fair -
> > .gitignore tells me I forgot to add my new command binary to it, when I
> > run `git status` to see what I need to add to my commit with new
> > command.
> 
> That is why I said that we need to actively work on, if we care
> about getting quality reports.
> 
> I do not think it is unreasonable to expect the build procedure for
> "git bugreport" to involve scanning in Documentation/config/ to pick
> up variable names, annotated in such a way that is invisible to
> AsciiDoc to allow us tell which ones are sensitive and which ones
> are not.  A test in t/ could even check if a documented
> configuration variable has such an annotation.  A commit that adds
> configuration variables without documentiong them does exist, but
> variables without documentation are (1) bugs, and (2) are not worth
> serious engineering effort on until they get documented.

> On 8/14/2019 10:34 PM, Emily Shaffer wrote:
> > diff --git a/git-bugreport.sh b/git-bugreport.sh
> > new file mode 100755
> > index 0000000000..2200703a51
> > --- /dev/null
> > +++ b/git-bugreport.sh
>
> At first I was alarmed by "What? another shell script?" but this
> command should prioritize flexibility and extensibility over speed.
> Running multiple processes shouldn't be too taxing for what we are
> trying to do here.

> Hi,
> 
> On Thu, 15 Aug 2019, Derrick Stolee wrote:
> 
> > On 8/14/2019 10:34 PM, Emily Shaffer wrote:
> > > diff --git a/git-bugreport.sh b/git-bugreport.sh
> > > new file mode 100755
> > > index 0000000000..2200703a51
> > > --- /dev/null
> > > +++ b/git-bugreport.sh
> >
> > At first I was alarmed by "What? another shell script?" but this
> > command should prioritize flexibility and extensibility over speed.
> > Running multiple processes shouldn't be too taxing for what we are
> > trying to do here.
> 
> Git for Windows sometimes receives bug reports about Bash not being able
> to start (usually it is a DLL base address problem, related to the way
> Cygwin and MSYS2 emulate `fork()`).

> 
> In such a case, `git bugreport` would only be able to offer a reason for
> yet another bug report instead of adding useful metadata.
> 
> Something to keep in mind when deciding how to implement this command.
> 
> Ciao,
> Dscho

> On Thu, Aug 15, 2019 at 10:07:57PM +0200, Johannes Schindelin wrote:
> >
> > On Thu, 15 Aug 2019, Derrick Stolee wrote:
> >
> > > On 8/14/2019 10:34 PM, Emily Shaffer wrote:
> > > > diff --git a/git-bugreport.sh b/git-bugreport.sh
> > > > new file mode 100755
> > > > index 0000000000..2200703a51
> > > > --- /dev/null
> > > > +++ b/git-bugreport.sh
> > >
> > > At first I was alarmed by "What? another shell script?" but this
> > > command should prioritize flexibility and extensibility over speed.
> > > Running multiple processes shouldn't be too taxing for what we are
> > > trying to do here.
> >
> > Git for Windows sometimes receives bug reports about Bash not being able
> > to start (usually it is a DLL base address problem, related to the way
> > Cygwin and MSYS2 emulate `fork()`).
>
> Hmm. In a case like this, then, how is someone using GfW at all?

> > In such a case, `git bugreport` would only be able to offer a reason for
> > yet another bug report instead of adding useful metadata.
> >
> > Something to keep in mind when deciding how to implement this command.
>
> Yeah, that's an interesting point, thanks for bringing it up. So, in the
> case when bash won't start at all, what does that failure look like? How
> much of Git can users still use? For example, would they be able to get
> far enough to run a binary git-bugreport?

> 
> >  - Which config options should we filter to avoid accidentally receiving
> >    credentials?
> 
> The remote URLs are pretty sensitive. Not only do users sometimes put passwords
> or PATs into their URLs, the literal name of the repo could be a secret.

> > +	echo "[Git Config]"
> > +	# TODO: Pass this through grep -v to avoid users sending us their credentials.
> > +	git config --show-origin --list
> > +	echo
> 
> Config options to consider stripping out:
> 
> 	*url*
> 	*pass* (anything "password" but also "sendmail.smtppass")

> 
> > +	echo "[Configured Hooks]"
> > +	find "$GIT_DIR/hooks/" -type f | grep -v "\.sample$" | print_filenames_and_content
> > +	echo
> 
> Remove the sample hooks, but focus on the others. Will this look like garbage if a hook
> is a binary file?

> 
> > +
> > +	echo "[Git Logs]"
> > +	find "$GIT_DIR/logs" -type f | print_filenames_and_content
> > +	echo
> 
> As mentioned before, I've sometimes found it helpful to know the data shape for the object
> store. Having a few extra steps such as the following could be nice:
> 
> 	echo "[Loose Objects]"
> 	for objdir in $(find "$GIT_DIR/objects/??" -type d)
> 	do
> 		echo "$objdir: $(ls $objdir | wc -l)"

> 	done
> 	echo
> 
> 	echo "[Pack Data]"
> 	ls -l "$GIT_DIR/objects/pack"
> 	echo
> 
> 	echo "[Object Info Data]"
> 	ls -lR "$GIT_DIR/objects/info"
> 	echo
> 
> 	echo "[Alternates File]"
> 	echo "========"
> 	cat "$GIT_DIR/objects/info/alternates"
> 	echo
> 
> That last one will collect information on the commit-graph file, even if it is an
> incremental file.

> 
> I think this is a great start, and I'll take some time later to try it out.
> 
> Thanks,
> -Stolee

> 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

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

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

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

> 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

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

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

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

>> > +NOTE
>> > +----
>> > +Bug reports can be sent to git@vger.kernel.org.
>> 
>> I am not sure if this belongs here.
>
> Sure, I wasn't certain either. Would you rather I remove the "what to do
> with this bugreport" NOTE section entirely?

> So, what's your suggestion? Not to check the output at all? (This may
> actually be fine; it occurred to me while reading your review that if a
> user is filing a bug report about something, one of the diagnostic
> commands in bugreport might be what's broken for them. So perhaps it
> should be tolerant to missing information...)

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> >> > +NOTE
> >> > +----
> >> > +Bug reports can be sent to git@vger.kernel.org.
> >> 
> >> I am not sure if this belongs here.
> >
> > Sure, I wasn't certain either. Would you rather I remove the "what to do
> > with this bugreport" NOTE section entirely?
> 
> Not really.  You are invoking an editor to let the user edit the
> pre-populated report, and I would imagine that a comment in that
> file would be the best place to give instructions, not a manpage
> for "git bugreport" command.

> 
> > So, what's your suggestion? Not to check the output at all? (This may
> > actually be fine; it occurred to me while reading your review that if a
> > user is filing a bug report about something, one of the diagnostic
> > commands in bugreport might be what's broken for them. So perhaps it
> > should be tolerant to missing information...)
> 
> Right.

>
> Add a new step to the build to generate a whitelist of git-config
> variables which are appropriate to include in the output of
> git-bugreport. New variables can be added to the whitelist by annotating
> their documentation in Documentation/config with the line
> "// bugreport-include".

> --- a/Documentation/config/sendemail.txt
> +++ b/Documentation/config/sendemail.txt
> @@ -1,63 +1,63 @@
> -sendemail.identity::
> +sendemail.identity:: // bugreport-exclude
>         A configuration identity. When given, causes values in the

> -sendemail.aliasesFile::
> -sendemail.aliasFileType::
> -sendemail.annotate::
> +sendemail.aliasesFile:: // bugreport-exclude
> +sendemail.aliasFileType:: // bugreport-exclude
> +sendemail.annotate:: // bugreport-include

> On Sat, 17 Aug 2019 at 02:42, Emily Shaffer <emilyshaffer@google.com> wrote:
> >
> > Add a new step to the build to generate a whitelist of git-config
> > variables which are appropriate to include in the output of
> > git-bugreport. New variables can be added to the whitelist by annotating
> > their documentation in Documentation/config with the line
> > "// bugreport-include".
> 
> These "// bugreport-include" show up in the rendered manpages, both with
> AsciiDoc and Asciidoctor. :-(

> 
> > --- a/Documentation/config/sendemail.txt
> > +++ b/Documentation/config/sendemail.txt
> > @@ -1,63 +1,63 @@
> > -sendemail.identity::
> > +sendemail.identity:: // bugreport-exclude
> >         A configuration identity. When given, causes values in the
> 
> If I put each comment on a line of its own (after the config option, but
> I suppose before would work the same way), Asciidoctor truly ignores
> them and everything's fine. And AsciiDoc renders this one and others
> like it ok.
> 
> > -sendemail.aliasesFile::
> > -sendemail.aliasFileType::
> > -sendemail.annotate::
> > +sendemail.aliasesFile:: // bugreport-exclude
> > +sendemail.aliasFileType:: // bugreport-exclude
> > +sendemail.annotate:: // bugreport-include
> 
> However, AsciiDoc (version 8.6.10) seems to effectively replace those
> comments with an empty line during processing, and it makes quite the
> difference here. Instead of these appearing in a compact comma-separated
> list, they are treated as individual items in the description list with
> no supporting content.
> 
> FWIW, I like the idea of annotating things here to make it harder to
> forget this whitelisting when adding a new config item.
> 
> Below is what I came up with as an alternative approach. Feel free to
> steal, squash and/or ignore as you see fit.

> We should probably also do the usual "create a candidate output file,
> then move it into place" dance for robustness.
> 
> I do think we should test the generated whitelist in some minimal way,
> e.g., to check that it does contain something which objectively belongs
> in the whitelist and -- more importantly IMHO -- does *not* contain
> something that shouldn't be there, such as sendemail.smtpPass.

> 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.
> 
> If users can send us a well-written bug report which contains diagnostic
> information we would otherwise need to ask the user for, we can reduce
> the number of question-and-answer round trips between the reporter and
> the Git contributor.
> 
> Users may also wish to send a report like this to their local "Git
> expert" if they have put their repository into a state they are confused
> by.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  Makefile            |  1 +
>  builtin.h           |  1 +
>  builtin/bugreport.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
>  git.c               |  1 +
>  4 files changed, 53 insertions(+)
>  create mode 100644 builtin/bugreport.c
> 
> diff --git a/Makefile b/Makefile
> index 58b92af54b..132e2a52da 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1039,6 +1039,7 @@ BUILTIN_OBJS += builtin/archive.o
>  BUILTIN_OBJS += builtin/bisect--helper.o
>  BUILTIN_OBJS += builtin/blame.o
>  BUILTIN_OBJS += builtin/branch.o
> +BUILTIN_OBJS += builtin/bugreport.o
>  BUILTIN_OBJS += builtin/bundle.o
>  BUILTIN_OBJS += builtin/cat-file.o
>  BUILTIN_OBJS += builtin/check-attr.o
> diff --git a/builtin.h b/builtin.h
> index 5cf5df69f7..c6373d3289 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -135,6 +135,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix);
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix);
>  int cmd_blame(int argc, const char **argv, const char *prefix);
>  int cmd_branch(int argc, const char **argv, const char *prefix);
> +int cmd_bugreport(int argc, const char **argv, const char *prefix);
>  int cmd_bundle(int argc, const char **argv, const char *prefix);
>  int cmd_cat_file(int argc, const char **argv, const char *prefix);
>  int cmd_checkout(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> new file mode 100644
> index 0000000000..2ef16440a0
> --- /dev/null
> +++ b/builtin/bugreport.c
> @@ -0,0 +1,50 @@
> +#include "builtin.h"
> +#include "stdio.h"
> +#include "strbuf.h"
> +#include "time.h"
> +
> +int get_bug_template(struct strbuf *template)

> +{
> +	const char template_text[] =
> +"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 send.\n";
> +
> +	strbuf_reset(template);
> +	strbuf_add(template, template_text, strlen(template_text));
> +	return 0;
> +}
> +
> +int cmd_bugreport(int argc, const char **argv, const char *prefix)
> +{
> +	struct strbuf buffer = STRBUF_INIT;
> +	struct strbuf report_path = STRBUF_INIT;
> +	FILE *report;
> +	time_t now = time(NULL);
> +
> +	strbuf_addstr(&report_path, "git-bugreport-");
> +	strbuf_addftime(&report_path, "%F", gmtime(&now), 0, 0);
> +	strbuf_addstr(&report_path, ".txt");
> +
> +	report = fopen_for_writing(report_path.buf);
> +
> +	get_bug_template(&buffer);
> +	strbuf_write(&buffer, report);
> +
> +	fclose(report);
> +
> +	launch_editor(report_path.buf, NULL, NULL);
> +	return 0;
> +}
> diff --git a/git.c b/git.c
> index ce6ab0ece2..2d6a64f019 100644
> --- a/git.c
> +++ b/git.c
> @@ -473,6 +473,7 @@ static struct cmd_struct commands[] = {
>  	{ "bisect--helper", cmd_bisect__helper, RUN_SETUP },
>  	{ "blame", cmd_blame, RUN_SETUP },
>  	{ "branch", cmd_branch, RUN_SETUP | DELAY_PAGER_CONFIG },
> +	{ "bugreport", cmd_bugreport, RUN_SETUP },
>  	{ "bundle", cmd_bundle, RUN_SETUP_GENTLY | NO_PARSEOPT },
>  	{ "cat-file", cmd_cat_file, RUN_SETUP },
>  	{ "check-attr", cmd_check_attr, RUN_SETUP },
> -- 
> 2.24.0.rc0.303.g954a862665-goog

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > 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.
> 
> It makes sense, but I do not think of any good reason why this
> should be implemented as a builtin.  I'd expect it would probably
> need to collect more info on the running environment than otherwise
> necessary for the regular Git operation, and perhaps you'd want to
> even link with libraries that are not needed for the regular Git
> operation to achieve that.
> 
> Can you make it a standalone binary instead to avoid bloat of the
> main "git" binary?

> On Sat, Nov 16, 2019 at 12:11:38PM +0900, Junio C Hamano wrote:
> > Emily Shaffer <emilyshaffer@google.com> writes:
> >
> > > 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.
> >
> > It makes sense, but I do not think of any good reason why this
> > should be implemented as a builtin.  I'd expect it would probably
> > need to collect more info on the running environment than otherwise
> > necessary for the regular Git operation, and perhaps you'd want to
> > even link with libraries that are not needed for the regular Git
> > operation to achieve that.
> >
> > Can you make it a standalone binary instead to avoid bloat of the
> > main "git" binary?
>
> Sure. This would fix some other issues (needing to link against curl to
> get the curl version, which is exactly what you implied). I wasn't
> certain which circumstances a standalone binary was preferred, but I
> agree with your reasoning here for sure.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > FWIW I disagree with the idea that a tiny built-in command like
> > `bugreport` would "bloat" the main `git` binary.
> >
> > In contrast, I think that stand-alone commands _do_ bloat. Look here:
>
> I probably should have clarified that "bloat" in the context of this
> discussion is not about on-disk space.  It is about resources needed
> to run "git status/diff/etc" that are everyday local commands that
> are expected to be lightweight, i.e. the same criteria applied when
> making the networking part (which the user expects to be coffee time)
> separate from them.

> Hi Emily,
> 
> On Tue, 19 Nov 2019, Emily Shaffer wrote:
> 
> > [...] some other issues (needing to link against curl to get the curl
> > version, which is exactly what you implied) [...]
> 
> I did suggest on IRC to teach `git-remote-https` an option where it prints
> the cURL version (and build options) and exits.
> 
> This would have the further advantage of making sure that the correct
> information is included. If you have two separate binaries that both link
> to cURL, they could still be linked statically, to different versions of
> cURL (this could happen e.g. if you have a `git-remote-https` from a
> different build in your path).

>> Can you make it a standalone binary instead to avoid bloat of the
>> main "git" binary?
>
> Sure. This would fix some other issues (needing to link against curl to
> get the curl version, which is exactly what you implied).

> diff --git a/bugreport-generate-config-whitelist.sh b/bugreport-generate-config-whitelist.sh
> new file mode 100755
> index 0000000000..ca6b232024
> --- /dev/null
> +++ b/bugreport-generate-config-whitelist.sh
> @@ -0,0 +1,4 @@
> +#!/bin/sh
> +
> +grep -RhPo ".*(?=:: \/\/ bugreport-include)" Documentation/config \

> +  >git-bugreport-config-whitelist
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>

> diff --git a/bugreport.c b/bugreport.c
> new file mode 100644
> index 0000000000..ada54fe583
> --- /dev/null
> +++ b/bugreport.c
> @@ -0,0 +1,55 @@
> +#include "cache.h"
> +
> +#include "bugreport.h"
> +#include "help.h"
> +#include "run-command.h"
> +#include "version.h"
> +
> +void get_system_info(struct strbuf *sys_info)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf std_out = STRBUF_INIT;
> +
> +	strbuf_reset(sys_info);
> +
> +	// get git version from native cmd

> +	strbuf_addstr(sys_info, "git version: ");
> +	strbuf_addstr(sys_info, git_version_string);
> +	strbuf_complete_line(sys_info);
> +
> +	// system call for other version info
> +	argv_array_push(&cp.args, "uname");
> +	argv_array_push(&cp.args, "-a");
> +	capture_command(&cp, &std_out, 0);

> +
> +	strbuf_addstr(sys_info, "uname -a: ");
> +	strbuf_addbuf(sys_info, &std_out);
> +	strbuf_complete_line(sys_info);
> +
> +	argv_array_clear(&cp.args);
> +	strbuf_reset(&std_out);
> +
> +
> +	argv_array_push(&cp.args, "curl-config");
> +	argv_array_push(&cp.args, "--version");
> +	capture_command(&cp, &std_out, 0);

> +
> +	strbuf_addstr(sys_info, "curl-config --version: ");
> +	strbuf_addbuf(sys_info, &std_out);
> +	strbuf_complete_line(sys_info);
> +
> +	argv_array_clear(&cp.args);
> +	strbuf_reset(&std_out);
> +
> +
> +	argv_array_push(&cp.args, "ldd");
> +	argv_array_push(&cp.args, "--version");
> +	capture_command(&cp, &std_out, 0);

> +
> +	strbuf_addstr(sys_info, "ldd --version: ");
> +	strbuf_addbuf(sys_info, &std_out);
> +	strbuf_complete_line(sys_info);
> +
> +	argv_array_clear(&cp.args);
> +	strbuf_reset(&std_out);
> +}
> diff --git a/bugreport.h b/bugreport.h
> new file mode 100644
> index 0000000000..ba216acf3f
> --- /dev/null
> +++ b/bugreport.h
> @@ -0,0 +1,7 @@
> +#include "strbuf.h"
> +
> +/**
> + * Adds the Git version, `uname -a`, and `curl-config --version` to sys_info.
> + * The previous contents of sys_info will be discarded.
> + */
> +void get_system_info(struct strbuf *sys_info);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 2ef16440a0..7232d31be7 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -1,4 +1,5 @@
>  #include "builtin.h"
> +#include "bugreport.h"
>  #include "stdio.h"
>  #include "strbuf.h"
>  #include "time.h"
> @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template)
>  	return 0;
>  }
>
> +void add_header(FILE *report, const char *title)
> +{
> +	struct strbuf buffer = STRBUF_INIT;
> +	strbuf_addf(&buffer, "\n\n[%s]\n", title);
> +	strbuf_write(&buffer, report);

> +}
> +
>  int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  {
>  	struct strbuf buffer = STRBUF_INIT;
> @@ -43,6 +51,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	get_bug_template(&buffer);
>  	strbuf_write(&buffer, report);
>
> +	// add other contents
> +	add_header(report, "System Info");
> +	get_system_info(&buffer);
> +	strbuf_write(&buffer, report);
> +
>  	fclose(report);
>
>  	launch_editor(report_path.buf, NULL, NULL);
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>

> Hi Emily,
> 
> On Thu, 24 Oct 2019, Emily Shaffer wrote:
> 
> > diff --git a/bugreport.c b/bugreport.c
> > new file mode 100644
> > index 0000000000..ada54fe583
> > --- /dev/null
> > +++ b/bugreport.c
> > @@ -0,0 +1,55 @@
> > +#include "cache.h"
> > +
> > +#include "bugreport.h"
> > +#include "help.h"
> > +#include "run-command.h"
> > +#include "version.h"
> > +
> > +void get_system_info(struct strbuf *sys_info)
> > +{
> > +	struct child_process cp = CHILD_PROCESS_INIT;
> > +	struct strbuf std_out = STRBUF_INIT;
> > +
> > +	strbuf_reset(sys_info);
> > +
> > +	// get git version from native cmd
> 
> Please use C-style comments instead of C++ ones.
> 
> > +	strbuf_addstr(sys_info, "git version: ");
> > +	strbuf_addstr(sys_info, git_version_string);
> > +	strbuf_complete_line(sys_info);
> > +
> > +	// system call for other version info
> > +	argv_array_push(&cp.args, "uname");
> > +	argv_array_push(&cp.args, "-a");
> > +	capture_command(&cp, &std_out, 0);
> 
> Mmmkay. I am not too much of a fan of relying on the `uname` executable,
> as it can very well be a 32-bit `uname.exe` on Windows, which obviously
> would _not_ report the architecture of the machine, but something
> misleading.
> 
> Why not use the `uname()` function (that we even define in
> `compat/mingw.c`)?

> 
> Also, why not include the same information as `git version
> --build-options`, most importantly `GIT_HOST_CPU`?
> 
> In any case, if you are capturing the output of `uname -a`, you
> definitely will want to pass the `RUN_COMMAND_STDOUT_TO_STDERR` flag (in
> case, say, the MSYS2 `uname.exe` fails with those dreaded Cygwin error
> messages like "*** fatal error - add_item
> ("\??\D:\git\installation\Git", "/", ...) failed, errno 1").
> 
> > +
> > +	strbuf_addstr(sys_info, "uname -a: ");
> > +	strbuf_addbuf(sys_info, &std_out);
> > +	strbuf_complete_line(sys_info);
> > +
> > +	argv_array_clear(&cp.args);
> > +	strbuf_reset(&std_out);
> > +
> > +
> > +	argv_array_push(&cp.args, "curl-config");
> > +	argv_array_push(&cp.args, "--version");
> > +	capture_command(&cp, &std_out, 0);
> 
> This will be almost certainly be incorrect, as most _regular_ users
> won't have `curl-config`. I know, it is easy to forget that most Git
> users are no hard-core C developers ;-)

> 
> A better alternative would be to use `curl_version()`, guarded, of
> course, by `#ifndef NO_CURL`...
> 
> > +
> > +	strbuf_addstr(sys_info, "curl-config --version: ");
> > +	strbuf_addbuf(sys_info, &std_out);
> > +	strbuf_complete_line(sys_info);
> > +
> > +	argv_array_clear(&cp.args);
> > +	strbuf_reset(&std_out);
> > +
> > +
> > +	argv_array_push(&cp.args, "ldd");
> > +	argv_array_push(&cp.args, "--version");
> > +	capture_command(&cp, &std_out, 0);
> 
> Again, this command will only be present in few setups. I am not
> actually sure that the output of this is interesting to begin with.

> 
> > +
> > +	strbuf_addstr(sys_info, "ldd --version: ");
> > +	strbuf_addbuf(sys_info, &std_out);
> > +	strbuf_complete_line(sys_info);
> > +
> > +	argv_array_clear(&cp.args);
> > +	strbuf_reset(&std_out);
> > +}
> > diff --git a/bugreport.h b/bugreport.h
> > new file mode 100644
> > index 0000000000..ba216acf3f
> > --- /dev/null
> > +++ b/bugreport.h
> > @@ -0,0 +1,7 @@
> > +#include "strbuf.h"
> > +
> > +/**
> > + * Adds the Git version, `uname -a`, and `curl-config --version` to sys_info.
> > + * The previous contents of sys_info will be discarded.
> > + */
> > +void get_system_info(struct strbuf *sys_info);
> > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > index 2ef16440a0..7232d31be7 100644
> > --- a/builtin/bugreport.c
> > +++ b/builtin/bugreport.c
> > @@ -1,4 +1,5 @@
> >  #include "builtin.h"
> > +#include "bugreport.h"
> >  #include "stdio.h"
> >  #include "strbuf.h"
> >  #include "time.h"
> > @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template)
> >  	return 0;
> >  }
> >
> > +void add_header(FILE *report, const char *title)
> > +{
> > +	struct strbuf buffer = STRBUF_INIT;
> > +	strbuf_addf(&buffer, "\n\n[%s]\n", title);
> > +	strbuf_write(&buffer, report);
> 
> This leaks `buffer`. Why not write into `report` via `fprintf()`
> directly?

> 
> Ciao,
> Dscho
> 
> > +}
> > +
> >  int cmd_bugreport(int argc, const char **argv, const char *prefix)
> >  {
> >  	struct strbuf buffer = STRBUF_INIT;
> > @@ -43,6 +51,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> >  	get_bug_template(&buffer);
> >  	strbuf_write(&buffer, report);
> >
> > +	// add other contents
> > +	add_header(report, "System Info");
> > +	get_system_info(&buffer);
> > +	strbuf_write(&buffer, report);
> > +
> >  	fclose(report);
> >
> >  	launch_editor(report_path.buf, NULL, NULL);
> > --
> > 2.24.0.rc0.303.g954a862665-goog
> >
> >

> On Mon, Oct 28, 2019 at 02:49:29PM +0100, Johannes Schindelin wrote:
>
> > On Thu, 24 Oct 2019, Emily Shaffer wrote:
> >
> > > diff --git a/bugreport.c b/bugreport.c
> > > new file mode 100644
> > > index 0000000000..ada54fe583
> > > --- /dev/null
> > > +++ b/bugreport.c
> > > [...]
> > > +	strbuf_addstr(sys_info, "curl-config --version: ");
> > > +	strbuf_addbuf(sys_info, &std_out);
> > > +	strbuf_complete_line(sys_info);
> > > +
> > > +	argv_array_clear(&cp.args);
> > > +	strbuf_reset(&std_out);
> > > +
> > > +
> > > +	argv_array_push(&cp.args, "ldd");
> > > +	argv_array_push(&cp.args, "--version");
> > > +	capture_command(&cp, &std_out, 0);
> >
> > Again, this command will only be present in few setups. I am not
> > actually sure that the output of this is interesting to begin with.
>
> It was a suggestion, I believe, from Jonathan Nieder.

> > What I _do_ think is that a much more interesting piece of information
> > would be the exact GLIBC version, the OS name and the OS version.
>
> The glibc version is easy; I've done that. It certainly looks nicer than
> the ldd call.
>
> I guess I may be missing something, because as I start to look into how
> to the OS info, I fall down a hole of many, many preprocessor defines to
> check. If that's the approach you want me to take, just say the word,
> but it will be ugly :) I suppose I had hoped the uname info would give us
> a close enough idea that full OS info isn't necessary.

> > > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > > index 2ef16440a0..7232d31be7 100644
> > > --- a/builtin/bugreport.c
> > > +++ b/builtin/bugreport.c
> > > @@ -1,4 +1,5 @@
> > >  #include "builtin.h"
> > > +#include "bugreport.h"
> > >  #include "stdio.h"
> > >  #include "strbuf.h"
> > >  #include "time.h"
> > > @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template)
> > >  	return 0;
> > >  }
> > >
> > > +void add_header(FILE *report, const char *title)
> > > +{
> > > +	struct strbuf buffer = STRBUF_INIT;
> > > +	strbuf_addf(&buffer, "\n\n[%s]\n", title);
> > > +	strbuf_write(&buffer, report);
> >
> > This leaks `buffer`. Why not write into `report` via `fprintf()`
> > directly?
>
> Rather, to match the style of the rest of the builtin, modified
> get_header to add the header to a passed-in strbuf instead of
> modifying the file directly.

> > > > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > > > index 2ef16440a0..7232d31be7 100644
> > > > --- a/builtin/bugreport.c
> > > > +++ b/builtin/bugreport.c
> > > > @@ -1,4 +1,5 @@
> > > >  #include "builtin.h"
> > > > +#include "bugreport.h"
> > > >  #include "stdio.h"
> > > >  #include "strbuf.h"
> > > >  #include "time.h"
> > > > @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template)
> > > >  	return 0;
> > > >  }
> > > >
> > > > +void add_header(FILE *report, const char *title)
> > > > +{
> > > > +	struct strbuf buffer = STRBUF_INIT;
> > > > +	strbuf_addf(&buffer, "\n\n[%s]\n", title);
> > > > +	strbuf_write(&buffer, report);
> > >
> > > This leaks `buffer`. Why not write into `report` via `fprintf()`
> > > directly?
> >
> > Rather, to match the style of the rest of the builtin, modified
> > get_header to add the header to a passed-in strbuf instead of
> > modifying the file directly.
> 
> Hmm. It makes the code less elegant in my opinion. I would rather either
> render the entire bug report into a single `strbuf` and then write it
> via `write_in_full()`, or use `fprintf()` directly.

> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 2ef16440a0..7232d31be7 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -1,4 +1,5 @@
>  #include "builtin.h"
> +#include "bugreport.h"
>  #include "stdio.h"
>  #include "strbuf.h"
>  #include "time.h"
> @@ -27,6 +28,13 @@ int get_bug_template(struct strbuf *template)
>  	return 0;
>  }
>  
> +void add_header(FILE *report, const char *title)

> Teach bugreport to gather the values of config options which are present
> in 'git-bugreport-config-whitelist'.
>
> Many config options are sensitive, and many Git add-ons use config
> options which git-core does not know about; it is better only to gather
> config options which we know to be safe, rather than excluding options
> which we know to be unsafe.

>
> Reading the whitelist into memory and sorting it saves us time -
> since git_config_bugreport() is called for every option the user has
> configured, limiting the file IO to one open/read/close and performing
> option lookup in sublinear time is a useful optimization.

> diff --git a/bugreport.c b/bugreport.c
> index ada54fe583..afa4836ab1 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -1,10 +1,24 @@
>  #include "cache.h"
>
>  #include "bugreport.h"
> +#include "config.h"
> +#include "exec-cmd.h"
>  #include "help.h"
>  #include "run-command.h"
>  #include "version.h"
>
> +/**
> + * A sorted list of config options which we will add to the bugreport. Managed
> + * by 'gather_whitelist(...)'.
> + */
> +struct string_list whitelist = STRING_LIST_INIT_DUP;
> +struct strbuf configs_and_values = STRBUF_INIT;
> +
> +// git version --build-options
> +// uname -a
> +// curl-config --version
> +// ldd --version
> +// echo $SHELL

>  void get_system_info(struct strbuf *sys_info)
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
> @@ -53,3 +67,39 @@ void get_system_info(struct strbuf *sys_info)
>  	argv_array_clear(&cp.args);
>  	strbuf_reset(&std_out);
>  }
> +
> +void gather_whitelist(struct strbuf *path)
> +{
> +	struct strbuf tmp = STRBUF_INIT;
> +	strbuf_read_file(&tmp, path->buf, 0);
> +	string_list_init(&whitelist, 1);
> +	string_list_split(&whitelist, tmp.buf, '\n', -1);
> +	string_list_sort(&whitelist);
> +}
> +
> +int git_config_bugreport(const char *var, const char *value, void *cb)
> +{
> +	if (string_list_has_string(&whitelist, var)) {
> +		strbuf_addf(&configs_and_values,
> +			    "%s : %s\n",
> +			    var, value);

> +	}
> +
> +	return 0;
> +}
> +
> +void get_whitelisted_config(struct strbuf *config_info)
> +{
> +	struct strbuf path = STRBUF_INIT;
> +
> +	strbuf_addstr(&path, git_exec_path());
> +	strbuf_addstr(&path, "/git-bugreport-config-whitelist");

> +
> +	gather_whitelist(&path);
> +	strbuf_init(&configs_and_values, whitelist.nr);
> +
> +	git_config(git_config_bugreport, NULL);
> +
> +	strbuf_reset(config_info);
> +	strbuf_addbuf(config_info, &configs_and_values);
> +}
> diff --git a/bugreport.h b/bugreport.h
> index ba216acf3f..7413e7e1be 100644
> --- a/bugreport.h
> +++ b/bugreport.h
> @@ -5,3 +5,10 @@
>   * The previous contents of sys_info will be discarded.
>   */
>  void get_system_info(struct strbuf *sys_info);
> +
> +/**

> + * Adds the values of the config items listed in
> + * 'git-bugreport-config-whitelist' to config_info. The previous contents of
> + * config_info will be discarded.
> + */
> +void get_whitelisted_config(struct strbuf *sys_info);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 7232d31be7..70fe0d2b85 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -56,6 +56,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	get_system_info(&buffer);
>  	strbuf_write(&buffer, report);
>
> +	add_header(report, "Whitelisted Config");

> +	get_whitelisted_config(&buffer);
> +	strbuf_write(&buffer, report);
> +
>  	fclose(report);
>
>  	launch_editor(report_path.buf, NULL, NULL);
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>

> 
> On Thu, 24 Oct 2019, Emily Shaffer wrote:
> 
> > Teach bugreport to gather the values of config options which are present
> > in 'git-bugreport-config-whitelist'.
> >
> > Many config options are sensitive, and many Git add-ons use config
> > options which git-core does not know about; it is better only to gather
> > config options which we know to be safe, rather than excluding options
> > which we know to be unsafe.
> 
> Should we still have the `// bugreport-exclude` comments, then?

> 
> >
> > Reading the whitelist into memory and sorting it saves us time -
> > since git_config_bugreport() is called for every option the user has
> > configured, limiting the file IO to one open/read/close and performing
> > option lookup in sublinear time is a useful optimization.
> 
> Maybe we even want a hashmap? That would reduce the time complexity even
> further.

> 
> > diff --git a/bugreport.c b/bugreport.c
> > index ada54fe583..afa4836ab1 100644
> > --- a/bugreport.c
> > +++ b/bugreport.c
> > @@ -1,10 +1,24 @@
> >  #include "cache.h"
> >
> >  #include "bugreport.h"
> > +#include "config.h"
> > +#include "exec-cmd.h"
> >  #include "help.h"
> >  #include "run-command.h"
> >  #include "version.h"
> >
> > +/**
> > + * A sorted list of config options which we will add to the bugreport. Managed
> > + * by 'gather_whitelist(...)'.
> > + */
> > +struct string_list whitelist = STRING_LIST_INIT_DUP;
> > +struct strbuf configs_and_values = STRBUF_INIT;
> > +
> > +// git version --build-options
> > +// uname -a
> > +// curl-config --version
> > +// ldd --version
> > +// echo $SHELL
> 
> These comments probably want to move to a single, C style comment, and
> they probably want to be introduced together with `get_system_info()`.

> 
> >  void get_system_info(struct strbuf *sys_info)
> >  {
> >  	struct child_process cp = CHILD_PROCESS_INIT;
> > @@ -53,3 +67,39 @@ void get_system_info(struct strbuf *sys_info)
> >  	argv_array_clear(&cp.args);
> >  	strbuf_reset(&std_out);
> >  }
> > +
> > +void gather_whitelist(struct strbuf *path)
> > +{
> > +	struct strbuf tmp = STRBUF_INIT;
> > +	strbuf_read_file(&tmp, path->buf, 0);
> > +	string_list_init(&whitelist, 1);
> > +	string_list_split(&whitelist, tmp.buf, '\n', -1);
> > +	string_list_sort(&whitelist);
> > +}
> > +
> > +int git_config_bugreport(const char *var, const char *value, void *cb)
> > +{
> > +	if (string_list_has_string(&whitelist, var)) {
> > +		strbuf_addf(&configs_and_values,
> > +			    "%s : %s\n",
> > +			    var, value);
> 
> A quite useful piece of information would be the config source. Not sure
> whether we can do that outside of `config.c` yet...

> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void get_whitelisted_config(struct strbuf *config_info)
> > +{
> > +	struct strbuf path = STRBUF_INIT;
> > +
> > +	strbuf_addstr(&path, git_exec_path());
> > +	strbuf_addstr(&path, "/git-bugreport-config-whitelist");
> 
> Hmm. I would have expected this patch to come directly after the patch
> 2/9 that generates that white-list, and I would also have expected that
> to be pre-sorted, and compiled in.
> 
> Do you want users to _edit_ the file in the exec path? In general, that
> path will be write-protected, though. A better alternative would
> probably be to compile in a hard-coded list, and to allow including more
> values e.g. by offering command-line options to specify config setting
> patterns. But if we allow patterns, we might actually want to have those
> exclusions to prevent sensitive data from being included.

> 
> > +
> > +	gather_whitelist(&path);
> > +	strbuf_init(&configs_and_values, whitelist.nr);
> > +
> > +	git_config(git_config_bugreport, NULL);
> > +
> > +	strbuf_reset(config_info);
> > +	strbuf_addbuf(config_info, &configs_and_values);
> > +}
> > diff --git a/bugreport.h b/bugreport.h
> > index ba216acf3f..7413e7e1be 100644
> > --- a/bugreport.h
> > +++ b/bugreport.h
> > @@ -5,3 +5,10 @@
> >   * The previous contents of sys_info will be discarded.
> >   */
> >  void get_system_info(struct strbuf *sys_info);
> > +
> > +/**
> 
> I also frequently use JavaDoc-style `/**`, but I am not sure that this
> is actually desired in Git's source code ;-)
> 
> > + * Adds the values of the config items listed in
> > + * 'git-bugreport-config-whitelist' to config_info. The previous contents of
> > + * config_info will be discarded.
> > + */
> > +void get_whitelisted_config(struct strbuf *sys_info);
> > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > index 7232d31be7..70fe0d2b85 100644
> > --- a/builtin/bugreport.c
> > +++ b/builtin/bugreport.c
> > @@ -56,6 +56,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> >  	get_system_info(&buffer);
> >  	strbuf_write(&buffer, report);
> >
> > +	add_header(report, "Whitelisted Config");
> 
> Quite honestly, I would like to avoid the term "whitelist" for good. How
> about "Selected config settings" instead?

> 
> Thanks,
> Dscho
> 
> > +	get_whitelisted_config(&buffer);
> > +	strbuf_write(&buffer, report);
> > +
> >  	fclose(report);
> >
> >  	launch_editor(report_path.buf, NULL, NULL);
> > --
> > 2.24.0.rc0.303.g954a862665-goog
> >
> >

> On Mon, Oct 28, 2019 at 03:14:35PM +0100, Johannes Schindelin wrote:
>
> >
> > On Thu, 24 Oct 2019, Emily Shaffer wrote:
> >
> > > Teach bugreport to gather the values of config options which are
> > > present in 'git-bugreport-config-whitelist'.
> > >
> > > Many config options are sensitive, and many Git add-ons use config
> > > options which git-core does not know about; it is better only to
> > > gather config options which we know to be safe, rather than
> > > excluding options which we know to be unsafe.
> >
> > Should we still have the `// bugreport-exclude` comments, then?
>
> They were optional (useless) before too. I can remove them if you want;
> I suppose I like the idea of having precedent if someone wants to build
> their own internal version with opt-out configs rather than opt-in. I
> can remove them if we want; it doesn't matter very much to me either
> way.

> > > diff --git a/bugreport.c b/bugreport.c
> > > [...]
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void get_whitelisted_config(struct strbuf *config_info)
> > > +{
> > > +	struct strbuf path = STRBUF_INIT;
> > > +
> > > +	strbuf_addstr(&path, git_exec_path());
> > > +	strbuf_addstr(&path, "/git-bugreport-config-whitelist");
> >
> > Hmm. I would have expected this patch to come directly after the patch
> > 2/9 that generates that white-list, and I would also have expected that
> > to be pre-sorted, and compiled in.
> >
> > Do you want users to _edit_ the file in the exec path? In general, that
> > path will be write-protected, though. A better alternative would
> > probably be to compile in a hard-coded list, and to allow including more
> > values e.g. by offering command-line options to specify config setting
> > patterns. But if we allow patterns, we might actually want to have those
> > exclusions to prevent sensitive data from being included.
>
> Hm, interesting. Do we have precedent for compiling in a header
> generated during the build process? I think I saw one when I was adding
> this script - I'll take a look.

> diff --git a/bugreport.c b/bugreport.c
> index ada54fe583..afa4836ab1 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -1,10 +1,24 @@
>  #include "cache.h"
>  
>  #include "bugreport.h"
> +#include "config.h"
> +#include "exec-cmd.h"
>  #include "help.h"
>  #include "run-command.h"
>  #include "version.h"
>  
> +/**
> + * A sorted list of config options which we will add to the bugreport. Managed
> + * by 'gather_whitelist(...)'.
> + */
> +struct string_list whitelist = STRING_LIST_INIT_DUP;
> +struct strbuf configs_and_values = STRBUF_INIT;
> +
> +// git version --build-options
> +// uname -a
> +// curl-config --version
> +// ldd --version
> +// echo $SHELL
>  void get_system_info(struct strbuf *sys_info)
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
> @@ -53,3 +67,39 @@ void get_system_info(struct strbuf *sys_info)
>  	argv_array_clear(&cp.args);
>  	strbuf_reset(&std_out);
>  }
> +
> +void gather_whitelist(struct strbuf *path)

> Josh Steadmon <steadmon@google.com> writes:
> 
> > This and git_config_bugreport() below should both be static as well.
> > Rather than repeating advice on the later patches, I'll just note that
> > any new functions that don't show up in the corresponding .h file should
> > be marked static.
> 
> Good advice.  
> 
> More importantly, given that "git bugreport" itself has no service
> of itself to offer other existing parts of Git (it is just a "gather
> various pieces of information from different places, and then
> produce a text file output" application), I do not see much point in
> it having its own header file that others would #include (i.e. the
> include file is to define services that are offered by it).  If
> there are common enough service routines invented to support the
> need of bugreport.c (e.g. perhaps it wants to give more info than
> what is currently available via the existing API on the contents of
> in-core index), by definition of being them common enough, they
> should be added to the header that can be used by both bugreport.c
> and other existing users of the same subsystem (e.g. if it is about
> in-core index, perhaps cache.h).

> Occasionally a failure a user is seeing may be related to a specific
> hook which is being run, perhaps without the user realizing. While the
> contents of hooks can be sensitive - containing user data or process
> information specific to the user's organization - simply knowing that a
> hook is being run at a certain stage can help us to understand whether
> something is going wrong.
>
> Without a definitive list of hook names within the code, we compile our
> own list from the documentation. This is likely prone to bitrot. To
> reduce the amount of code humans need to read, we turn the list into a
> string_list and iterate over it (as we are calling the same find_hook
> operation on each string). However, since bugreport should primarily be
> called by the user, the performance loss from massaging the string
> seems acceptable.

>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  bugreport.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  bugreport.h         |  6 ++++++
>  builtin/bugreport.c |  4 ++++
>  3 files changed, 54 insertions(+)
>
> diff --git a/bugreport.c b/bugreport.c
> index afa4836ab1..9d7f44ff28 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -103,3 +103,47 @@ void get_whitelisted_config(struct strbuf *config_info)
>  	strbuf_reset(config_info);
>  	strbuf_addbuf(config_info, &configs_and_values);
>  }
> +
> +void get_populated_hooks(struct strbuf *hook_info)
> +{
> +	/*
> +	 * Doesn't look like there is a list of all possible hooks; so below is
> +	 * a transcription of `git help hook`.
> +	 */
> +	const char *hooks = "applypatch-msg,"
> +			    "pre-applypatch,"
> +			    "post-applypatch,"
> +			    "pre-commit,"
> +			    "pre-merge-commit,"
> +			    "prepare-commit-msg,"
> +			    "commit-msg,"
> +			    "post-commit,"
> +			    "pre-rebase,"
> +			    "post-checkout,"
> +			    "post-merge,"
> +			    "pre-push,"
> +			    "pre-receive,"
> +			    "update,"
> +			    "post-receive,"
> +			    "post-update,"
> +			    "push-to-checkout,"
> +			    "pre-auto-gc,"
> +			    "post-rewrite,"
> +			    "sendemail-validate,"
> +			    "fsmonitor-watchman,"
> +			    "p4-pre-submit,"
> +			    "post-index-changex";

> +	struct string_list hooks_list = STRING_LIST_INIT_DUP;
> +	struct string_list_item *iter = NULL;
> +
> +	strbuf_reset(hook_info);
> +
> +	string_list_split(&hooks_list, hooks, ',', -1);
> +
> +	for_each_string_list_item(iter, &hooks_list) {

> +		if (find_hook(iter->string)) {
> +			strbuf_addstr(hook_info, iter->string);
> +			strbuf_complete_line(hook_info);
> +		}
> +	}
> +}
> diff --git a/bugreport.h b/bugreport.h
> index 7413e7e1be..942a5436e3 100644
> --- a/bugreport.h
> +++ b/bugreport.h
> @@ -12,3 +12,9 @@ void get_system_info(struct strbuf *sys_info);
>   * config_info will be discarded.
>   */
>  void get_whitelisted_config(struct strbuf *sys_info);
> +
> +/**
> + * Adds the paths to all configured hooks (but not their contents). The previous
> + * contents of hook_info will be discarded.
> + */
> +void get_populated_hooks(struct strbuf *hook_info);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 70fe0d2b85..a0eefba498 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -60,6 +60,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	get_whitelisted_config(&buffer);
>  	strbuf_write(&buffer, report);
>
> +	add_header(report, "Populated Hooks");

> +	get_populated_hooks(&buffer);
> +	strbuf_write(&buffer, report);
> +
>  	fclose(report);
>
>  	launch_editor(report_path.buf, NULL, NULL);
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>

> Hi Emily,
> 
> On Thu, 24 Oct 2019, Emily Shaffer wrote:
> 
> > Occasionally a failure a user is seeing may be related to a specific
> > hook which is being run, perhaps without the user realizing. While the
> > contents of hooks can be sensitive - containing user data or process
> > information specific to the user's organization - simply knowing that a
> > hook is being run at a certain stage can help us to understand whether
> > something is going wrong.
> >
> > Without a definitive list of hook names within the code, we compile our
> > own list from the documentation. This is likely prone to bitrot. To
> > reduce the amount of code humans need to read, we turn the list into a
> > string_list and iterate over it (as we are calling the same find_hook
> > operation on each string). However, since bugreport should primarily be
> > called by the user, the performance loss from massaging the string
> > seems acceptable.
> 
> Good idea!
> 
> >
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> >  bugreport.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >  bugreport.h         |  6 ++++++
> >  builtin/bugreport.c |  4 ++++
> >  3 files changed, 54 insertions(+)
> >
> > diff --git a/bugreport.c b/bugreport.c
> > index afa4836ab1..9d7f44ff28 100644
> > --- a/bugreport.c
> > +++ b/bugreport.c
> > @@ -103,3 +103,47 @@ void get_whitelisted_config(struct strbuf *config_info)
> >  	strbuf_reset(config_info);
> >  	strbuf_addbuf(config_info, &configs_and_values);
> >  }
> > +
> > +void get_populated_hooks(struct strbuf *hook_info)
> > +{
> > +	/*
> > +	 * Doesn't look like there is a list of all possible hooks; so below is
> > +	 * a transcription of `git help hook`.
> > +	 */
> > +	const char *hooks = "applypatch-msg,"
> > +			    "pre-applypatch,"
> > +			    "post-applypatch,"
> > +			    "pre-commit,"
> > +			    "pre-merge-commit,"
> > +			    "prepare-commit-msg,"
> > +			    "commit-msg,"
> > +			    "post-commit,"
> > +			    "pre-rebase,"
> > +			    "post-checkout,"
> > +			    "post-merge,"
> > +			    "pre-push,"
> > +			    "pre-receive,"
> > +			    "update,"
> > +			    "post-receive,"
> > +			    "post-update,"
> > +			    "push-to-checkout,"
> > +			    "pre-auto-gc,"
> > +			    "post-rewrite,"
> > +			    "sendemail-validate,"
> > +			    "fsmonitor-watchman,"
> > +			    "p4-pre-submit,"
> > +			    "post-index-changex";
> 
> Well, this is disappointing: I tried to come up with a scripted way to
> extract the commit names from our source code, and I failed! This is
> where I gave up:
> 
> 	git grep run_hook |
> 	sed -n 's/.*run_hook[^(]*([^,]*, *\([^,]*\).*/\1/p' |
> 	sed -e '/^name$/d' -e '/^const char \*name$/d' \
> 		-e 's/push_to_checkout_hook/"push-to-checkout"/' |
> 	sort |
> 	uniq
> 
> This prints only the following hook names:
> 
> 	"applypatch-msg"
> 	"post-applypatch"
> 	"post-checkout"
> 	"post-index-change"
> 	"post-merge"
> 	"pre-applypatch"
> 	"pre-auto-gc"
> 	"pre-rebase"
> 	"prepare-commit-msg"
> 	"push-to-checkout"
> 
> But at least it was easy to script the extracting from the
> Documentation:
> 
> 	sed -n '/^[a-z]/{N;/\n~~~/{s/\n.*//;p}}' \
> 		Documentation/githooks.txt
> 

> > +	struct string_list hooks_list = STRING_LIST_INIT_DUP;
> > +	struct string_list_item *iter = NULL;
> > +
> > +	strbuf_reset(hook_info);
> > +
> > +	string_list_split(&hooks_list, hooks, ',', -1);
> > +
> > +	for_each_string_list_item(iter, &hooks_list) {
> 
> This should definitely be done at compile time, I think. We should be
> able to generate an appropriate header via something like this:
> 
> 	cat >hook-names.h <<-EOF
> 	static const char *hook_names[] = {
> 	$(sed -n '/^[a-z]/{N;/\n~~~/{s/\n.*/",/;s/^/	"/;p}}' \
> 		Documentation/githooks.txt)
> 	};
> 	EOF
> 
> Then you would use a simple `for()` loop using `ARRAY_SIZE()` to
> iterate over the hook names.
> 
> > +		if (find_hook(iter->string)) {
> > +			strbuf_addstr(hook_info, iter->string);
> > +			strbuf_complete_line(hook_info);
> > +		}
> > +	}
> > +}
> > diff --git a/bugreport.h b/bugreport.h
> > index 7413e7e1be..942a5436e3 100644
> > --- a/bugreport.h
> > +++ b/bugreport.h
> > @@ -12,3 +12,9 @@ void get_system_info(struct strbuf *sys_info);
> >   * config_info will be discarded.
> >   */
> >  void get_whitelisted_config(struct strbuf *sys_info);
> > +
> > +/**
> > + * Adds the paths to all configured hooks (but not their contents). The previous
> > + * contents of hook_info will be discarded.
> > + */
> > +void get_populated_hooks(struct strbuf *hook_info);
> > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > index 70fe0d2b85..a0eefba498 100644
> > --- a/builtin/bugreport.c
> > +++ b/builtin/bugreport.c
> > @@ -60,6 +60,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> >  	get_whitelisted_config(&buffer);
> >  	strbuf_write(&buffer, report);
> >
> > +	add_header(report, "Populated Hooks");
> 
> Wait! I should have stumbled over this in an earlier patch. The
> `add_header()` function should not take a `FILE *` parameter at all, but
> instead an `struct strbuf *` one!
> 
> Ciao,
> Dscho
> 
> > +	get_populated_hooks(&buffer);
> > +	strbuf_write(&buffer, report);
> > +
> >  	fclose(report);
> >
> >  	launch_editor(report_path.buf, NULL, NULL);
> > --
> > 2.24.0.rc0.303.g954a862665-goog
> >
> >

> On Mon, Oct 28, 2019 at 03:31:36PM +0100, Johannes Schindelin wrote:
>
> > On Thu, 24 Oct 2019, Emily Shaffer wrote:
> >
> > > diff --git a/bugreport.c b/bugreport.c
> > > index afa4836ab1..9d7f44ff28 100644
> > > --- a/bugreport.c
> > > +++ b/bugreport.c
> > > @@ -103,3 +103,47 @@ void get_whitelisted_config(struct strbuf *config_info)
> > >  	strbuf_reset(config_info);
> > >  	strbuf_addbuf(config_info, &configs_and_values);
> > >  }
> > > +
> > > +void get_populated_hooks(struct strbuf *hook_info)
> > > +{
> > > +	/*
> > > +	 * Doesn't look like there is a list of all possible hooks; so below is
> > > +	 * a transcription of `git help hook`.
> > > +	 */
> > > +	const char *hooks = "applypatch-msg,"
> > > +			    "pre-applypatch,"
> > > +			    "post-applypatch,"
> > > +			    "pre-commit,"
> > > +			    "pre-merge-commit,"
> > > +			    "prepare-commit-msg,"
> > > +			    "commit-msg,"
> > > +			    "post-commit,"
> > > +			    "pre-rebase,"
> > > +			    "post-checkout,"
> > > +			    "post-merge,"
> > > +			    "pre-push,"
> > > +			    "pre-receive,"
> > > +			    "update,"
> > > +			    "post-receive,"
> > > +			    "post-update,"
> > > +			    "push-to-checkout,"
> > > +			    "pre-auto-gc,"
> > > +			    "post-rewrite,"
> > > +			    "sendemail-validate,"
> > > +			    "fsmonitor-watchman,"
> > > +			    "p4-pre-submit,"
> > > +			    "post-index-changex";
> >
> > Well, this is disappointing: I tried to come up with a scripted way to
> > extract the commit names from our source code, and I failed! This is
> > where I gave up:
> >
> > 	git grep run_hook |
> > 	sed -n 's/.*run_hook[^(]*([^,]*, *\([^,]*\).*/\1/p' |
> > 	sed -e '/^name$/d' -e '/^const char \*name$/d' \
> > 		-e 's/push_to_checkout_hook/"push-to-checkout"/' |
> > 	sort |
> > 	uniq
> >
> > This prints only the following hook names:
> >
> > 	"applypatch-msg"
> > 	"post-applypatch"
> > 	"post-checkout"
> > 	"post-index-change"
> > 	"post-merge"
> > 	"pre-applypatch"
> > 	"pre-auto-gc"
> > 	"pre-rebase"
> > 	"prepare-commit-msg"
> > 	"push-to-checkout"
> >
> > But at least it was easy to script the extracting from the
> > Documentation:
> >
> > 	sed -n '/^[a-z]/{N;/\n~~~/{s/\n.*//;p}}' \
> > 		Documentation/githooks.txt
> >
>
> To be totally frank, I'm not keen on spending a lot of time with
> scripting this. My parallel work with config-based hooks will also
> involve an in-code source of truth for available hooknames; I'd like to
> punt on some scripting, putting it in the makefile, etc blah since I
> know I'm planning to fix this particular annoyance at the source - and
> since it looks like bugreport will stay in the review phase for at least
> a little longer.

> The number of unpacked objects in a user's repository may help us
> understand the root of the problem they're seeing, especially if a
> command is running unusually slowly.
>
> Rather than directly invoking 'git-count-objects', which may sometimes
> fail unexpectedly on Git for Windows, manually count the contents of
> .git/objects. Additionally, since we may wish to inspect other
> directories' contents for bugreport in the future, put the directory
> listing into a helper function.

>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  bugreport.c         | 72 +++++++++++++++++++++++++++++++++++++++++++++
>  bugreport.h         |  6 ++++
>  builtin/bugreport.c |  4 +++
>  3 files changed, 82 insertions(+)
>
> diff --git a/bugreport.c b/bugreport.c
> index 9d7f44ff28..54e1d47103 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -5,8 +5,11 @@
>  #include "exec-cmd.h"
>  #include "help.h"
>  #include "run-command.h"
> +#include "strbuf.h"

> +
>  /**
>   * A sorted list of config options which we will add to the bugreport. Managed
>   * by 'gather_whitelist(...)'.
> @@ -147,3 +150,72 @@ void get_populated_hooks(struct strbuf *hook_info)
>  		}
>  	}
>  }
> +
> +/**
> + * Fill 'contents' with the contents of the dir at 'dirpath'.

> + * If 'filter' is nonzero, the contents are filtered on d_type as 'type' - see
> + * 'man readdir'. opendir() doesn't take string length as an arg, so don't
> + * bother passing it in.
> + */
> +void list_contents_of_dir(struct string_list *contents, struct strbuf *dirpath,

> +			  int filter, unsigned char type)
> +{
> +	struct dirent *dir = NULL;
> +	DIR *dh = NULL;
> +
> +	dh = opendir(dirpath->buf);
> +	while (dh && (dir = readdir(dh))) {
> +		if (!filter || type == dir->d_type) {
> +			string_list_append(contents, dir->d_name);
> +		}
> +	}
> +}
> +
> +
> +void get_object_counts(struct strbuf *obj_info)

> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf std_out = STRBUF_INIT;
> +
> +	argv_array_push(&cp.args, "count-objects");
> +	argv_array_push(&cp.args, "-vH");
> +	cp.git_cmd = 1;
> +	capture_command(&cp, &std_out, 0);
> +
> +	strbuf_reset(obj_info);
> +	strbuf_addstr(obj_info, "git-count-objects -vH:\n");
> +	strbuf_addbuf(obj_info, &std_out);
> +}
> +
> +void get_loose_object_summary(struct strbuf *obj_info)
> +{
> +	struct strbuf dirpath = STRBUF_INIT;
> +	struct string_list subdirs = STRING_LIST_INIT_DUP;
> +	struct string_list_item *subdir;
> +
> +	strbuf_reset(obj_info);
> +
> +	strbuf_addstr(&dirpath, get_object_directory());
> +	strbuf_complete(&dirpath, '/');
> +
> +	list_contents_of_dir(&subdirs, &dirpath, 1, DT_DIR);
> +
> +	for_each_string_list_item(subdir, &subdirs)
> +	{
> +		struct strbuf subdir_buf = STRBUF_INIT;
> +		struct string_list objects = STRING_LIST_INIT_DUP;
> +
> +		/*
> +		 * Only interested in loose objects - so dirs named with the
> +		 * first byte of the object ID
> +		 */
> +		if (strlen(subdir->string) != 2 || !strcmp(subdir->string, ".."))
> +			continue;
> +
> +		strbuf_addbuf(&subdir_buf, &dirpath);
> +		strbuf_addstr(&subdir_buf, subdir->string);
> +		list_contents_of_dir(&objects, &subdir_buf, 0, 0);
> +		strbuf_addf(obj_info, "%s: %d objects\n", subdir->string,
> +			    objects.nr);

> +	}
> +}
> diff --git a/bugreport.h b/bugreport.h
> index 942a5436e3..09ad0c2599 100644
> --- a/bugreport.h
> +++ b/bugreport.h
> @@ -18,3 +18,9 @@ void get_whitelisted_config(struct strbuf *sys_info);
>   * contents of hook_info will be discarded.
>   */
>  void get_populated_hooks(struct strbuf *hook_info);
> +
> +/**
> + * Adds the output of `git count-object -vH`. The previous contents of hook_info
> + * will be discarded.
> + */
> +void get_loose_object_summary(struct strbuf *obj_info);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index a0eefba498..b2ab194207 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -64,6 +64,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	get_populated_hooks(&buffer);
>  	strbuf_write(&buffer, report);
>
> +	add_header(report, "Object Counts");
> +	get_loose_object_summary(&buffer);
> +	strbuf_write(&buffer, report);
> +
>  	fclose(report);
>
>  	launch_editor(report_path.buf, NULL, NULL);
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>

> Hi Emily,
> 
> On Thu, 24 Oct 2019, Emily Shaffer wrote:
> 
> > The number of unpacked objects in a user's repository may help us
> > understand the root of the problem they're seeing, especially if a
> > command is running unusually slowly.
> >
> > Rather than directly invoking 'git-count-objects', which may sometimes
> > fail unexpectedly on Git for Windows, manually count the contents of
> > .git/objects. Additionally, since we may wish to inspect other
> > directories' contents for bugreport in the future, put the directory
> > listing into a helper function.
> 
> Thank you, much appreciated!
> 
> I guess the next step is to count the number of packs, and the number of
> submodules ;-)
> 
> >
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> >  bugreport.c         | 72 +++++++++++++++++++++++++++++++++++++++++++++
> >  bugreport.h         |  6 ++++
> >  builtin/bugreport.c |  4 +++
> >  3 files changed, 82 insertions(+)
> >
> > diff --git a/bugreport.c b/bugreport.c
> > index 9d7f44ff28..54e1d47103 100644
> > --- a/bugreport.c
> > +++ b/bugreport.c
> > @@ -5,8 +5,11 @@
> >  #include "exec-cmd.h"
> >  #include "help.h"
> >  #include "run-command.h"
> > +#include "strbuf.h"
> 
> Why not append this to the end of the `#include` list, as is common in
> Git's commit history?
> 
> >  #include "version.h"
> >
> > +#include "dirent.h"
> 
> This header (although with pointy brackets instead of double quotes) is
> already included in `git-compat-util.h`
> 
> > +
> >  /**
> >   * A sorted list of config options which we will add to the bugreport. Managed
> >   * by 'gather_whitelist(...)'.
> > @@ -147,3 +150,72 @@ void get_populated_hooks(struct strbuf *hook_info)
> >  		}
> >  	}
> >  }
> > +
> > +/**
> > + * Fill 'contents' with the contents of the dir at 'dirpath'.
> 
> Since you start this comment in JavaDoc style, there should be an almost
> empty line after this one ("almost" because it still contains the
> asterisk, of course).
> 
> > + * If 'filter' is nonzero, the contents are filtered on d_type as 'type' - see
> > + * 'man readdir'. opendir() doesn't take string length as an arg, so don't
> > + * bother passing it in.
> > + */
> > +void list_contents_of_dir(struct string_list *contents, struct strbuf *dirpath,
> 
> Shouldn't this be `static`?
> 
> > +			  int filter, unsigned char type)
> > +{
> > +	struct dirent *dir = NULL;
> > +	DIR *dh = NULL;
> > +
> > +	dh = opendir(dirpath->buf);
> > +	while (dh && (dir = readdir(dh))) {
> > +		if (!filter || type == dir->d_type) {
> > +			string_list_append(contents, dir->d_name);
> > +		}
> > +	}
> > +}
> > +
> > +
> > +void get_object_counts(struct strbuf *obj_info)
> 
> Oops. This function is no longer used.
> 
> > +{
> > +	struct child_process cp = CHILD_PROCESS_INIT;
> > +	struct strbuf std_out = STRBUF_INIT;
> > +
> > +	argv_array_push(&cp.args, "count-objects");
> > +	argv_array_push(&cp.args, "-vH");
> > +	cp.git_cmd = 1;
> > +	capture_command(&cp, &std_out, 0);
> > +
> > +	strbuf_reset(obj_info);
> > +	strbuf_addstr(obj_info, "git-count-objects -vH:\n");
> > +	strbuf_addbuf(obj_info, &std_out);
> > +}
> > +
> > +void get_loose_object_summary(struct strbuf *obj_info)
> > +{
> > +	struct strbuf dirpath = STRBUF_INIT;
> > +	struct string_list subdirs = STRING_LIST_INIT_DUP;
> > +	struct string_list_item *subdir;
> > +
> > +	strbuf_reset(obj_info);
> > +
> > +	strbuf_addstr(&dirpath, get_object_directory());
> > +	strbuf_complete(&dirpath, '/');
> > +
> > +	list_contents_of_dir(&subdirs, &dirpath, 1, DT_DIR);
> > +
> > +	for_each_string_list_item(subdir, &subdirs)
> > +	{
> > +		struct strbuf subdir_buf = STRBUF_INIT;
> > +		struct string_list objects = STRING_LIST_INIT_DUP;
> > +
> > +		/*
> > +		 * Only interested in loose objects - so dirs named with the
> > +		 * first byte of the object ID
> > +		 */
> > +		if (strlen(subdir->string) != 2 || !strcmp(subdir->string, ".."))
> > +			continue;
> > +
> > +		strbuf_addbuf(&subdir_buf, &dirpath);
> > +		strbuf_addstr(&subdir_buf, subdir->string);
> > +		list_contents_of_dir(&objects, &subdir_buf, 0, 0);
> > +		strbuf_addf(obj_info, "%s: %d objects\n", subdir->string,
> > +			    objects.nr);
> 
> Hmm. Not only does this leak `objects`, it also throws away the contents
> that we so painfully constructed.
> 
> Wouldn't it make more sense to do something like this instead?
> 
> static int is_hex(const char *string, size_t count)
> {
> 	for (; count; string++, count--)
> 		if (hexval(*string) < 0)
> 			return 0;
> 	return 1;
> }

> 
> static ssize_t count_loose_objects(struct strbuf *objects_path)
> {
> 	ssize_t ret = 0;
> 	size_t len;
> 	struct dirent *d;
> 	DIR *dir, *subdir;
> 
> 	dir = opendir(objects_path->buf);
> 	if (!dir)
> 		return -1;
> 
> 	strbuf_complete(objects_path, '/');

> 		closedir(subdir);
> 	}
> 	closedir(dir);
> 	strbuf_reset(objects_path, len);
> 	return ret;
> }

> 
> Ciao,
> Dscho
> 
> > +	}
> > +}
> > diff --git a/bugreport.h b/bugreport.h
> > index 942a5436e3..09ad0c2599 100644
> > --- a/bugreport.h
> > +++ b/bugreport.h
> > @@ -18,3 +18,9 @@ void get_whitelisted_config(struct strbuf *sys_info);
> >   * contents of hook_info will be discarded.
> >   */
> >  void get_populated_hooks(struct strbuf *hook_info);
> > +
> > +/**
> > + * Adds the output of `git count-object -vH`. The previous contents of hook_info
> > + * will be discarded.
> > + */
> > +void get_loose_object_summary(struct strbuf *obj_info);
> > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > index a0eefba498..b2ab194207 100644
> > --- a/builtin/bugreport.c
> > +++ b/builtin/bugreport.c
> > @@ -64,6 +64,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> >  	get_populated_hooks(&buffer);
> >  	strbuf_write(&buffer, report);
> >
> > +	add_header(report, "Object Counts");
> > +	get_loose_object_summary(&buffer);
> > +	strbuf_write(&buffer, report);
> > +
> >  	fclose(report);
> >
> >  	launch_editor(report_path.buf, NULL, NULL);
> > --
> > 2.24.0.rc0.303.g954a862665-goog
> >
> >

> diff --git a/bugreport.h b/bugreport.h
> index 942a5436e3..09ad0c2599 100644
> --- a/bugreport.h
> +++ b/bugreport.h
> @@ -18,3 +18,9 @@ void get_whitelisted_config(struct strbuf *sys_info);
>   * contents of hook_info will be discarded.
>   */
>  void get_populated_hooks(struct strbuf *hook_info);
> +
> +/**
> + * Adds the output of `git count-object -vH`. The previous contents of hook_info
> + * will be discarded.
> + */
> +void get_loose_object_summary(struct strbuf *obj_info);

>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  bugreport.c         | 21 +++++++++++++++++++++
>  bugreport.h         |  6 ++++++
>  builtin/bugreport.c |  4 ++++
>  3 files changed, 31 insertions(+)
>
> diff --git a/bugreport.c b/bugreport.c
> index 54e1d47103..79ddb8baaa 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -219,3 +219,24 @@ void get_loose_object_summary(struct strbuf *obj_info)
>  			    objects.nr);
>  	}
>  }
> +
> +void get_packed_object_summary(struct strbuf *obj_info)
> +{
> +	struct strbuf dirpath = STRBUF_INIT;
> +	struct string_list contents = STRING_LIST_INIT_DUP;
> +	struct string_list_item *entry;
> +
> +	strbuf_reset(obj_info);
> +
> +	strbuf_addstr(&dirpath, get_object_directory());
> +	strbuf_complete(&dirpath, '/');
> +	strbuf_addstr(&dirpath, "pack/");
> +	list_contents_of_dir(&contents, &dirpath, 0, 0);
> +
> +	// list full contents of $GIT_OBJECT_DIRECTORY/pack/
> +	for_each_string_list_item(entry, &contents) {
> +		strbuf_addbuf(obj_info, &dirpath);
> +		strbuf_addstr(obj_info, entry->string);
> +		strbuf_complete_line(obj_info);
> +	}
> +}

> diff --git a/bugreport.h b/bugreport.h
> index 09ad0c2599..11ff7df41b 100644
> --- a/bugreport.h
> +++ b/bugreport.h
> @@ -24,3 +24,9 @@ void get_populated_hooks(struct strbuf *hook_info);
>   * will be discarded.
>   */
>  void get_loose_object_summary(struct strbuf *obj_info);
> +
> +/**
> + * Adds a list of the contents of '.git/objects/pack'. The previous contents of
> + * hook_info will be discarded.
> + */
> +void get_packed_object_summary(struct strbuf *obj_info);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index b2ab194207..da91a3944e 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -68,6 +68,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	get_loose_object_summary(&buffer);
>  	strbuf_write(&buffer, report);
>
> +	add_header(report, "Packed Object Summary");
> +	get_packed_object_summary(&buffer);
> +	strbuf_write(&buffer, report);
> +

>  	fclose(report);
>
>  	launch_editor(report_path.buf, NULL, NULL);
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>

> Hi Emily,
> 
> On Thu, 24 Oct 2019, Emily Shaffer wrote:
> 
> > Alongside the list of loose objects, it's useful to see the list of
> > object packs as well. It can help us to examine what Git did and did not
> > pack.
> 
> Yes, I was write! Packs are next ;-)
> 
> >
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> >  bugreport.c         | 21 +++++++++++++++++++++
> >  bugreport.h         |  6 ++++++
> >  builtin/bugreport.c |  4 ++++
> >  3 files changed, 31 insertions(+)
> >
> > diff --git a/bugreport.c b/bugreport.c
> > index 54e1d47103..79ddb8baaa 100644
> > --- a/bugreport.c
> > +++ b/bugreport.c
> > @@ -219,3 +219,24 @@ void get_loose_object_summary(struct strbuf *obj_info)
> >  			    objects.nr);
> >  	}
> >  }
> > +
> > +void get_packed_object_summary(struct strbuf *obj_info)
> > +{
> > +	struct strbuf dirpath = STRBUF_INIT;
> > +	struct string_list contents = STRING_LIST_INIT_DUP;
> > +	struct string_list_item *entry;
> > +
> > +	strbuf_reset(obj_info);
> > +
> > +	strbuf_addstr(&dirpath, get_object_directory());
> > +	strbuf_complete(&dirpath, '/');
> > +	strbuf_addstr(&dirpath, "pack/");
> > +	list_contents_of_dir(&contents, &dirpath, 0, 0);
> > +
> > +	// list full contents of $GIT_OBJECT_DIRECTORY/pack/
> > +	for_each_string_list_item(entry, &contents) {
> > +		strbuf_addbuf(obj_info, &dirpath);
> > +		strbuf_addstr(obj_info, entry->string);
> > +		strbuf_complete_line(obj_info);
> > +	}
> > +}
> 
> Okay, but I think that you will want to discern between regular `.pack`
> files, `.idx` files and `tmp_*` files.

> 
> > diff --git a/bugreport.h b/bugreport.h
> > index 09ad0c2599..11ff7df41b 100644
> > --- a/bugreport.h
> > +++ b/bugreport.h
> > @@ -24,3 +24,9 @@ void get_populated_hooks(struct strbuf *hook_info);
> >   * will be discarded.
> >   */
> >  void get_loose_object_summary(struct strbuf *obj_info);
> > +
> > +/**
> > + * Adds a list of the contents of '.git/objects/pack'. The previous contents of
> > + * hook_info will be discarded.
> > + */
> > +void get_packed_object_summary(struct strbuf *obj_info);
> > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > index b2ab194207..da91a3944e 100644
> > --- a/builtin/bugreport.c
> > +++ b/builtin/bugreport.c
> > @@ -68,6 +68,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> >  	get_loose_object_summary(&buffer);
> >  	strbuf_write(&buffer, report);
> >
> > +	add_header(report, "Packed Object Summary");
> > +	get_packed_object_summary(&buffer);
> > +	strbuf_write(&buffer, report);
> > +
> 
> Hmm. At this point, I am unclear whether you want to write into an
> `strbuf`, or directly into a `FILE *`? I would rather have only one, not
> a mix.
> 
> Ciao,
> Dscho
> 
> >  	fclose(report);
> >
> >  	launch_editor(report_path.buf, NULL, NULL);
> > --
> > 2.24.0.rc0.303.g954a862665-goog
> >
> >

> On Mon, Oct 28, 2019 at 04:43:50PM +0100, Johannes Schindelin wrote:
> > Hi Emily,
> >
> > On Thu, 24 Oct 2019, Emily Shaffer wrote:
> >
> > > Alongside the list of loose objects, it's useful to see the list of
> > > object packs as well. It can help us to examine what Git did and did not
> > > pack.
> >
> > Yes, I was write! Packs are next ;-)
> >
> > >
> > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > > ---
> > >  bugreport.c         | 21 +++++++++++++++++++++
> > >  bugreport.h         |  6 ++++++
> > >  builtin/bugreport.c |  4 ++++
> > >  3 files changed, 31 insertions(+)
> > >
> > > diff --git a/bugreport.c b/bugreport.c
> > > index 54e1d47103..79ddb8baaa 100644
> > > --- a/bugreport.c
> > > +++ b/bugreport.c
> > > @@ -219,3 +219,24 @@ void get_loose_object_summary(struct strbuf *obj_info)
> > >  			    objects.nr);
> > >  	}
> > >  }
> > > +
> > > +void get_packed_object_summary(struct strbuf *obj_info)
> > > +{
> > > +	struct strbuf dirpath = STRBUF_INIT;
> > > +	struct string_list contents = STRING_LIST_INIT_DUP;
> > > +	struct string_list_item *entry;
> > > +
> > > +	strbuf_reset(obj_info);
> > > +
> > > +	strbuf_addstr(&dirpath, get_object_directory());
> > > +	strbuf_complete(&dirpath, '/');
> > > +	strbuf_addstr(&dirpath, "pack/");
> > > +	list_contents_of_dir(&contents, &dirpath, 0, 0);
> > > +
> > > +	// list full contents of $GIT_OBJECT_DIRECTORY/pack/
> > > +	for_each_string_list_item(entry, &contents) {
> > > +		strbuf_addbuf(obj_info, &dirpath);
> > > +		strbuf_addstr(obj_info, entry->string);
> > > +		strbuf_complete_line(obj_info);
> > > +	}
> > > +}
> >
> > Okay, but I think that you will want to discern between regular `.pack`
> > files, `.idx` files and `tmp_*` files.
>
> Discern in what way? How would you like to see them treated separately?
> They're all being listed here, not just counted, so it seems to me like
> I can read the generated bugreport and see which files are index, pack,
> or temporary here.

>
> >
> > > diff --git a/bugreport.h b/bugreport.h
> > > index 09ad0c2599..11ff7df41b 100644
> > > --- a/bugreport.h
> > > +++ b/bugreport.h
> > > @@ -24,3 +24,9 @@ void get_populated_hooks(struct strbuf *hook_info);
> > >   * will be discarded.
> > >   */
> > >  void get_loose_object_summary(struct strbuf *obj_info);
> > > +
> > > +/**
> > > + * Adds a list of the contents of '.git/objects/pack'. The previous contents of
> > > + * hook_info will be discarded.
> > > + */
> > > +void get_packed_object_summary(struct strbuf *obj_info);
> > > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > > index b2ab194207..da91a3944e 100644
> > > --- a/builtin/bugreport.c
> > > +++ b/builtin/bugreport.c
> > > @@ -68,6 +68,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> > >  	get_loose_object_summary(&buffer);
> > >  	strbuf_write(&buffer, report);
> > >
> > > +	add_header(report, "Packed Object Summary");
> > > +	get_packed_object_summary(&buffer);
> > > +	strbuf_write(&buffer, report);
> > > +
> >
> > Hmm. At this point, I am unclear whether you want to write into an
> > `strbuf`, or directly into a `FILE *`? I would rather have only one, not
> > a mix.
> >
> > Ciao,
> > Dscho
> >
> > >  	fclose(report);
> > >
> > >  	launch_editor(report_path.buf, NULL, NULL);
> > > --
> > > 2.24.0.rc0.303.g954a862665-goog
> > >
> > >
>

> Hi Emily,
> 
> On Tue, 10 Dec 2019, Emily Shaffer wrote:
> 
> > On Mon, Oct 28, 2019 at 04:43:50PM +0100, Johannes Schindelin wrote:
> > > Hi Emily,
> > >
> > > On Thu, 24 Oct 2019, Emily Shaffer wrote:
> > >
> > > > Alongside the list of loose objects, it's useful to see the list of
> > > > object packs as well. It can help us to examine what Git did and did not
> > > > pack.
> > >
> > > Yes, I was write! Packs are next ;-)
> > >
> > > >
> > > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > > > ---
> > > >  bugreport.c         | 21 +++++++++++++++++++++
> > > >  bugreport.h         |  6 ++++++
> > > >  builtin/bugreport.c |  4 ++++
> > > >  3 files changed, 31 insertions(+)
> > > >
> > > > diff --git a/bugreport.c b/bugreport.c
> > > > index 54e1d47103..79ddb8baaa 100644
> > > > --- a/bugreport.c
> > > > +++ b/bugreport.c
> > > > @@ -219,3 +219,24 @@ void get_loose_object_summary(struct strbuf *obj_info)
> > > >  			    objects.nr);
> > > >  	}
> > > >  }
> > > > +
> > > > +void get_packed_object_summary(struct strbuf *obj_info)
> > > > +{
> > > > +	struct strbuf dirpath = STRBUF_INIT;
> > > > +	struct string_list contents = STRING_LIST_INIT_DUP;
> > > > +	struct string_list_item *entry;
> > > > +
> > > > +	strbuf_reset(obj_info);
> > > > +
> > > > +	strbuf_addstr(&dirpath, get_object_directory());
> > > > +	strbuf_complete(&dirpath, '/');
> > > > +	strbuf_addstr(&dirpath, "pack/");
> > > > +	list_contents_of_dir(&contents, &dirpath, 0, 0);
> > > > +
> > > > +	// list full contents of $GIT_OBJECT_DIRECTORY/pack/
> > > > +	for_each_string_list_item(entry, &contents) {
> > > > +		strbuf_addbuf(obj_info, &dirpath);
> > > > +		strbuf_addstr(obj_info, entry->string);
> > > > +		strbuf_complete_line(obj_info);
> > > > +	}
> > > > +}
> > >
> > > Okay, but I think that you will want to discern between regular `.pack`
> > > files, `.idx` files and `tmp_*` files.
> >
> > Discern in what way? How would you like to see them treated separately?
> > They're all being listed here, not just counted, so it seems to me like
> > I can read the generated bugreport and see which files are index, pack,
> > or temporary here.
> 
> I take your word for it (sorry, it's been half an eternity since I wrapped
> my head around the diff, I forgotten pretty much all about it ;-))

> 
> Ciao,
> Dscho
> 
> >
> > >
> > > > diff --git a/bugreport.h b/bugreport.h
> > > > index 09ad0c2599..11ff7df41b 100644
> > > > --- a/bugreport.h
> > > > +++ b/bugreport.h
> > > > @@ -24,3 +24,9 @@ void get_populated_hooks(struct strbuf *hook_info);
> > > >   * will be discarded.
> > > >   */
> > > >  void get_loose_object_summary(struct strbuf *obj_info);
> > > > +
> > > > +/**
> > > > + * Adds a list of the contents of '.git/objects/pack'. The previous contents of
> > > > + * hook_info will be discarded.
> > > > + */
> > > > +void get_packed_object_summary(struct strbuf *obj_info);
> > > > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > > > index b2ab194207..da91a3944e 100644
> > > > --- a/builtin/bugreport.c
> > > > +++ b/builtin/bugreport.c
> > > > @@ -68,6 +68,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> > > >  	get_loose_object_summary(&buffer);
> > > >  	strbuf_write(&buffer, report);
> > > >
> > > > +	add_header(report, "Packed Object Summary");
> > > > +	get_packed_object_summary(&buffer);
> > > > +	strbuf_write(&buffer, report);
> > > > +
> > >
> > > Hmm. At this point, I am unclear whether you want to write into an
> > > `strbuf`, or directly into a `FILE *`? I would rather have only one, not
> > > a mix.
> > >
> > > Ciao,
> > > Dscho
> > >
> > > >  	fclose(report);
> > > >
> > > >  	launch_editor(report_path.buf, NULL, NULL);
> > > > --
> > > > 2.24.0.rc0.303.g954a862665-goog
> > > >
> > > >
> >

> Miscellaneous information used about the object store can end up in
> .git/objects/info; this can help us understand what may be going on with
> the object store when the user is reporting a bug. Otherwise, it could
> be difficult to track down what is going wrong with an object which
> isn't kept locally to .git/objects/ or .git/objects/pack. Having some
> understanding of where the user's objects may be kept can save us some
> hops during the bug reporting process.

>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  bugreport.c         | 58 +++++++++++++++++++++++++++++++++++++++++++++
>  bugreport.h         |  6 +++++
>  builtin/bugreport.c |  7 ++++++
>  3 files changed, 71 insertions(+)
>
> diff --git a/bugreport.c b/bugreport.c
> index 79ddb8baaa..ce15904fec 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -6,6 +6,7 @@
>  #include "help.h"
>  #include "run-command.h"
>  #include "strbuf.h"
> +#include "string-list.h"
>  #include "version.h"
>
>  #include "dirent.h"
> @@ -171,6 +172,41 @@ void list_contents_of_dir(struct string_list *contents, struct strbuf *dirpath,
>  	}
>  }
>
> +/**
> + * Fills 'contents' with a list of all directories within the provided
> + * directory, recursing into each directory.
> + */
> +void list_contents_of_dir_recursively(struct string_list *contents,
> +				      struct strbuf *dirpath)
> +{
> +	struct string_list current_contents = STRING_LIST_INIT_DUP;
> +	struct string_list current_subdirs = STRING_LIST_INIT_DUP;

> +	struct string_list_item *it;
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	list_contents_of_dir(&current_contents, dirpath, 0, 0);
> +	for_each_string_list_item(it, &current_contents) {
> +		strbuf_reset(&buf);
> +		strbuf_addbuf(&buf, dirpath);
> +		strbuf_complete(&buf, '/');
> +		strbuf_addstr(&buf, it->string);

> +
> +		string_list_append(contents, buf.buf);
> +	}
> +
> +	list_contents_of_dir(&current_subdirs, dirpath, 1, DT_DIR);
> +	for_each_string_list_item(it, &current_subdirs) {
> +		if (strcmp(it->string, ".") == 0
> +		    || strcmp(it->string, "..") == 0)
> +			continue;
> +		strbuf_reset(&buf);
> +		strbuf_addbuf(&buf, dirpath);
> +		strbuf_complete(&buf, '/');
> +		strbuf_addstr(&buf, it->string);
> +
> +		list_contents_of_dir_recursively(contents, &buf);
> +	}
> +}
>
>  void get_object_counts(struct strbuf *obj_info)
>  {
> @@ -240,3 +276,25 @@ void get_packed_object_summary(struct strbuf *obj_info)
>  		strbuf_complete_line(obj_info);
>  	}
>  }
> +
> +void get_object_info_summary(struct strbuf *obj_info)
> +{
> +	// strbuf += GITDIR/info/:
> +	// recursively list contents of $GIT_OBJECT_DIRECTORY/info
> +	struct strbuf dirpath = STRBUF_INIT;
> +	struct string_list contents = STRING_LIST_INIT_DUP;
> +	struct string_list_item *entry;
> +
> +	strbuf_reset(obj_info);
> +
> +	strbuf_addstr(&dirpath, get_object_directory());
> +	strbuf_complete(&dirpath, '/');
> +	strbuf_addstr(&dirpath, "info/");
> +
> +	list_contents_of_dir_recursively(&contents, &dirpath);
> +
> +	for_each_string_list_item(entry, &contents) {
> +		strbuf_addstr(obj_info, entry->string);
> +		strbuf_complete_line(obj_info);
> +	}

> +}
> diff --git a/bugreport.h b/bugreport.h
> index 11ff7df41b..4f5e2d1b9a 100644
> --- a/bugreport.h
> +++ b/bugreport.h
> @@ -30,3 +30,9 @@ void get_loose_object_summary(struct strbuf *obj_info);
>   * hook_info will be discarded.
>   */
>  void get_packed_object_summary(struct strbuf *obj_info);
> +
> +/**
> + * Adds a list of all contents (recursively) of '.git/objects/info'. The
> + * previous contents of hook_info will be discarded.
> + */
> +void get_object_info_summary(struct strbuf *obj_info);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index da91a3944e..8aad33a9b0 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -72,6 +72,13 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	get_packed_object_summary(&buffer);
>  	strbuf_write(&buffer, report);
>
> +	add_header(report, "Object Info Data");
> +	get_object_info_summary(&buffer);
> +	strbuf_write(&buffer, report);
> +
> +	// Close file
> +	// open file in editor
> +	launch_editor(report_path, NULL, NULL);
>  	fclose(report);
>
>  	launch_editor(report_path.buf, NULL, NULL);
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>

>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  bugreport.c         | 14 ++++++++++++++
>  bugreport.h         |  6 ++++++
>  builtin/bugreport.c |  4 ++++
>  3 files changed, 24 insertions(+)
>
> diff --git a/bugreport.c b/bugreport.c
> index ce15904fec..a7bdc72b7f 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -298,3 +298,17 @@ void get_object_info_summary(struct strbuf *obj_info)
>  		strbuf_complete_line(obj_info);
>  	}
>  }
> +
> +void get_alternates_file(struct strbuf *alternates_info)
> +{
> +	struct strbuf alternates_path = STRBUF_INIT;
> +
> +	strbuf_addstr(&alternates_path, get_object_directory());
> +	strbuf_complete(&alternates_path, '/');
> +	strbuf_addstr(&alternates_path, "info/alternates");
> +
> +	strbuf_reset(alternates_info);
> +	strbuf_addbuf(alternates_info, &alternates_path);
> +	strbuf_complete_line(alternates_info);
> +	strbuf_read_file(alternates_info, alternates_path.buf, 0);
> +}
> diff --git a/bugreport.h b/bugreport.h
> index 4f5e2d1b9a..74d1f79960 100644
> --- a/bugreport.h
> +++ b/bugreport.h
> @@ -36,3 +36,9 @@ void get_packed_object_summary(struct strbuf *obj_info);
>   * previous contents of hook_info will be discarded.
>   */
>  void get_object_info_summary(struct strbuf *obj_info);
> +
> +/**
> + * Adds the contents of '.git/info/alternates'. The previous contents of
> + * alternates_info will be discarded.
> + */
> +void get_alternates_file(struct strbuf *alt_info);
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 8aad33a9b0..0784bdc42a 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -76,6 +76,10 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	get_object_info_summary(&buffer);
>  	strbuf_write(&buffer, report);
>
> +	add_header(report, "Alternates File");
> +	get_alternates_file(&buffer);
> +	strbuf_write(&buffer, report);
> +
>  	// Close file
>  	// open file in editor
>  	launch_editor(report_path, NULL, NULL);
> --
> 2.24.0.rc0.303.g954a862665-goog
>
>

> Hi Emily,
> 
> On Thu, 24 Oct 2019, Emily Shaffer wrote:
> 
> > In some cases, it could be that the user is having a problem with an
> > object which isn't present in their normal object directory. We can get
> > a hint that that might be the case by examining the list of alternates
> > where their object may be stored instead.
> 
> Doesn't this open the possibility of leaking project's (possibly NDA'ed) names?
> 
> I could imagine that we might rather want to count the alternates, and
> maybe separate into those alternates that actually exist and alternates
> that do not exist (which would produce a warning that the user might
> have trained themselves to ignore).

> Thanks for the patience with the long wait, all. Here's an attempt at
> the rewrite in C; I think it does verbatim what the sh version did
> except that this doesn't print the reflog - Jonathan Nieder was good
> enough to point out to me that folks probably don't want to share their
> commit subjects all willy-nilly if they're working on something
> secretive.

> Emily Shaffer <emilyshaffer@google.com> writes:
>
> > Thanks for the patience with the long wait, all. Here's an attempt at
> > the rewrite in C; I think it does verbatim what the sh version did
> > except that this doesn't print the reflog - Jonathan Nieder was good
> > enough to point out to me that folks probably don't want to share their
> > commit subjects all willy-nilly if they're working on something
> > secretive.
>
> Is the goal to give a tool the end users can type "git
> bugreport<RET>" and automatically send the result to this mailing
> list (or some bug tracker)?  Or is this only about producing a
> pre-filled bug report template to be slurped into their MUA and then
> further manually edited before sending it out?
>
> It probably is controversial if we exposed contents of hooks scripts
> (I can imagine some people may trigger external process by pinging a
> URL that includes credential material out of laziness), so the
> presence test you have is probably a good stopping point.  I do not
> know how much it helps to know which hooks exist in order to
> diagnose an anomaly without knowing what their contents are, but it
> is a start.
>
> By the way, I doubt that it is the best use of developer time to
> write these in C---taking various pieces of information from
> different places to prepare a text file sounds more suited to
> scripting languages, especially while we are still learning what
> kind of information we want to collect and how we want to present
> the final result (iow, for the first handful of years after
> deploying this command).