v8, 15 partshelp: add shell-path to --build-options

> It may be useful to know which shell Git was built to try to point to,
> in the event that shell-based Git commands are failing. $SHELL_PATH is
> set during the build and used to launch the manpage viewer, as well as
> by git-compat-util.h, and it's used during tests. 'git version
> --build-options' is encouraged for use in bug reports, so it makes sense
> to include this information there.

> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  help.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/help.c b/help.c
> index a21487db77..190722fb0a 100644
> --- a/help.c
> +++ b/help.c
> @@ -654,6 +654,7 @@ int cmd_version(int argc, const char **argv, const char *prefix)
>  			printf("no commit associated with this build\n");
>  		printf("sizeof-long: %d\n", (int)sizeof(long));
>  		printf("sizeof-size_t: %d\n", (int)sizeof(size_t));
> +		printf("shell-path: %s\n", SHELL_PATH);
>  		/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
>  	}
>  	return 0;

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > It may be useful to know which shell Git was built to try to point to,
> > in the event that shell-based Git commands are failing. $SHELL_PATH is
> > set during the build and used to launch the manpage viewer, as well as
> > by git-compat-util.h, and it's used during tests. 'git version
> > --build-options' is encouraged for use in bug reports, so it makes sense
> > to include this information there.
> 
> It probably makes sense to capture PERL_PATH, PYTHON_PATH,
> TCLTK_PATH, and TCL_PATH, as they fall into exactly the same
> category as this one.

> 
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> >  help.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/help.c b/help.c
> > index a21487db77..190722fb0a 100644
> > --- a/help.c
> > +++ b/help.c
> > @@ -654,6 +654,7 @@ int cmd_version(int argc, const char **argv, const char *prefix)
> >  			printf("no commit associated with this build\n");
> >  		printf("sizeof-long: %d\n", (int)sizeof(long));
> >  		printf("sizeof-size_t: %d\n", (int)sizeof(size_t));
> > +		printf("shell-path: %s\n", SHELL_PATH);
> >  		/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
> >  	}
> >  	return 0;

> +
> +	if (option_output) {
> +		strbuf_addstr(&report_path, option_output);
> +		strbuf_complete(&report_path, '/');
> +	}
> +
> +
> +	strbuf_addstr(&report_path, "git-bugreport-");
> +	strbuf_addftime(&report_path, option_suffix, localtime(&now), 0, 0);
> +	strbuf_addstr(&report_path, ".txt");
> +
> +	if (!stat(report_path.buf, &statbuf))
> +		die("'%s' already exists", report_path.buf);

> +	switch (safe_create_leading_directories(report_path.buf)) {
> +	case SCLD_OK:
> +	case SCLD_EXISTS:
> +		break;
> +	default:
> +		die(_("could not create leading directories for '%s'"),
> +		    report_path.buf);
> +	}
> +
> +	get_bug_template(&buffer);
> +
> +	report = fopen_for_writing(report_path.buf);

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > +	argc = parse_options(argc, argv, "", bugreport_options,
> > +			     bugreport_usage, 0);
> 
> Which one between an empty string and NULL is more appropriate to be
> passed as "prefix" here?  I assume that this is *not* really a git
> program, and no repository/working-tree discovery is involved, and
> you won't be using OPT_FILENAME, so it would probably be OK.
> 
> > +
> > +	if (option_output) {
> > +		strbuf_addstr(&report_path, option_output);
> > +		strbuf_complete(&report_path, '/');
> > +	}
> > +
> > +
> > +	strbuf_addstr(&report_path, "git-bugreport-");
> > +	strbuf_addftime(&report_path, option_suffix, localtime(&now), 0, 0);
> > +	strbuf_addstr(&report_path, ".txt");
> > +
> > +	if (!stat(report_path.buf, &statbuf))
> > +		die("'%s' already exists", report_path.buf);
> 
> Missed i18n/l10n, but I do not think it is a useful thing for this
> check to be here in the first place.
> 
> > +	switch (safe_create_leading_directories(report_path.buf)) {
> > +	case SCLD_OK:
> > +	case SCLD_EXISTS:
> > +		break;
> > +	default:
> > +		die(_("could not create leading directories for '%s'"),
> > +		    report_path.buf);
> > +	}
> > +
> > +	get_bug_template(&buffer);
> > +
> > +	report = fopen_for_writing(report_path.buf);
> 
> fopen_for_writing() does not give good semantics for what you seem
> to want to do here (i.e. to make sure you do not overwrite an
> existing one).  It even has "if we cannot open it, retry after
> unlinking" logic in it, which screams "this function is designed for
> those who want to overwrite the file", and if you look at its
> callers, they are _all_ about opening a file for writing with a well
> known name like FETCH_HEAD, COMMIT_EDITMSG, etc.
> 
> Besides, a stat() that dies when a file exists would not
> help---since that check, you've spent non-zero time, creating
> leading directories and preparing boilerplate in the buffer,
> and there is no guarantee somebody else used the same filename
> in the meantime.

> 
> If you want to avoid overwriting an existing file (which I think is
> a good idea---I just do not think the earlier "if (!stat()) die()"
> is a good way to do so), the only sensible way to do so is to ask
> your open/creat to fail if there already is a file---that is how
> you'd avoid racing with another process that may be creating such a
> file.  Grep for O_EXCL to find where the flag is used with O_CREAT
> to see how it is done.

> 
> > +		strbuf_release(&report_path);
> > +		die("couldn't open '%s' for writing", report_path.buf);
> 
> Do we see a use-after-free here?  Also missed i18n/l10n.

> diff --git a/bugreport.c b/bugreport.c
> new file mode 100644
> index 0000000000..8d4a76fdac
> --- /dev/null
> +++ b/bugreport.c
> @@ -0,0 +1,94 @@
> +#include "builtin.h"
> +#include "parse-options.h"
> +#include "stdio.h"
> +#include "strbuf.h"
> +#include "time.h"
> +
> +static const char * const bugreport_usage[] = {
> +	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
> +	NULL
> +};
> +
> +static int get_bug_template(struct strbuf *template)
> +{
> +	const char template_text[] = N_(
> +"Thank you for filling out a Git bug report!\n"
> +"Please answer the following questions to help us understand your issue.\n"
> +"\n"
> +"What did you do before the bug happened? (Steps to reproduce your issue)\n"
> +"\n"
> +"What did you expect to happen? (Expected behavior)\n"
> +"\n"
> +"What happened instead? (Actual behavior)\n"
> +"\n"
> +"What's different between what you expected and what actually happened?\n"
> +"\n"
> +"Anything else you want to add:\n"
> +"\n"
> +"Please review the rest of the bug report below.\n"
> +"You can delete any lines you don't wish to share.\n");
> +
> +	strbuf_addstr(template, template_text);
> +	return 0;
> +}
> +
> +int cmd_main(int argc, const char **argv)
> +{
> +	struct strbuf buffer = STRBUF_INIT;
> +	struct strbuf report_path = STRBUF_INIT;
> +	FILE *report;
> +	time_t now = time(NULL);
> +	char *option_output = NULL;
> +	char *option_suffix = "%F-%H%M";

> +static void get_system_info(struct strbuf *sys_info)
> +{
> +	/* get git version from native cmd */
> +	strbuf_addstr(sys_info, "git version:\n");
> +	get_version_info(sys_info, 1);
> +	strbuf_complete_line(sys_info);

> +}
> ...
> diff --git a/help.c b/help.c
> index 190722fb0a..44cee69c11 100644
> --- a/help.c
> +++ b/help.c
> @@ -622,8 +622,33 @@ const char *help_unknown_cmd(const char *cmd)
>  	exit(1);
>  }
>  
> +void get_version_info(struct strbuf *buf, int show_build_options)
> +{
> +	/*
> +	 * The format of this string should be kept stable for compatibility
> +	 * with external projects that rely on the output of "git version".
> +	 *
> +	 * Always show the version, even if other options are given.
> +	 */
> +	strbuf_addf(buf, "git version %s\n", git_version_string);

> +	if (show_build_options) {
> +		strbuf_addf(buf, "cpu: %s\n", GIT_HOST_CPU);
> +		if (git_built_from_commit_string[0])
> +			strbuf_addf(buf, "built from commit: %s\n",
> +			       git_built_from_commit_string);
> +		else
> +			strbuf_addstr(buf, "no commit associated with this build\n");
> +		strbuf_addf(buf, "sizeof-long: %d\n", (int)sizeof(long));
> +		strbuf_addf(buf, "sizeof-size_t: %d\n", (int)sizeof(size_t));
> +		strbuf_addf(buf, "shell-path: %s\n", SHELL_PATH);
> +		/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */

>  int cmd_version(int argc, const char **argv, const char *prefix)
>  {
> +	struct strbuf buf = STRBUF_INIT;
>  	int build_options = 0;
>  	const char * const usage[] = {
>  		N_("git version [<options>]"),
> @@ -637,26 +662,11 @@ int cmd_version(int argc, const char **argv, const char *prefix)
>  
>  	argc = parse_options(argc, argv, prefix, options, usage, 0);
>  
> -	/*
> -	 * The format of this string should be kept stable for compatibility
> -	 * with external projects that rely on the output of "git version".
> -	 *
> -	 * Always show the version, even if other options are given.
> -	 */
> -	printf("git version %s\n", git_version_string);
> +	get_version_info(&buf, build_options);
> +	printf("%s", buf.buf);
> +
> +	strbuf_release(&buf);
>  
> -	if (build_options) {
> -		printf("cpu: %s\n", GIT_HOST_CPU);
> -		if (git_built_from_commit_string[0])
> -			printf("built from commit: %s\n",
> -			       git_built_from_commit_string);
> -		else
> -			printf("no commit associated with this build\n");
> -		printf("sizeof-long: %d\n", (int)sizeof(long));
> -		printf("sizeof-size_t: %d\n", (int)sizeof(size_t));
> -		printf("shell-path: %s\n", SHELL_PATH);
> -		/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
> -	}
>  	return 0;

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > +static void get_system_info(struct strbuf *sys_info)
> > +{
> > +	/* get git version from native cmd */
> > +	strbuf_addstr(sys_info, "git version:\n");
> > +	get_version_info(sys_info, 1);
> > +	strbuf_complete_line(sys_info);
> 
> It is a bit curious use of "don't do anything if sys_info ends with
> a complete line, but complete it if it ends with an imcomplete
> line".  That tells the readers that we do not know what
> get_version_info() will do (now or in the future) to its output
> buffer.
> 
> > +}
> > ...
> > diff --git a/help.c b/help.c
> > index 190722fb0a..44cee69c11 100644
> > --- a/help.c
> > +++ b/help.c
> > @@ -622,8 +622,33 @@ const char *help_unknown_cmd(const char *cmd)
> >  	exit(1);
> >  }
> >  
> > +void get_version_info(struct strbuf *buf, int show_build_options)
> > +{
> > +	/*
> > +	 * The format of this string should be kept stable for compatibility
> > +	 * with external projects that rely on the output of "git version".
> > +	 *
> > +	 * Always show the version, even if other options are given.
> > +	 */
> > +	strbuf_addf(buf, "git version %s\n", git_version_string);
> 
> This ends the output with a complete line when !show_build_options ...
> 
> > +	if (show_build_options) {
> > +		strbuf_addf(buf, "cpu: %s\n", GIT_HOST_CPU);
> > +		if (git_built_from_commit_string[0])
> > +			strbuf_addf(buf, "built from commit: %s\n",
> > +			       git_built_from_commit_string);
> > +		else
> > +			strbuf_addstr(buf, "no commit associated with this build\n");
> > +		strbuf_addf(buf, "sizeof-long: %d\n", (int)sizeof(long));
> > +		strbuf_addf(buf, "sizeof-size_t: %d\n", (int)sizeof(size_t));
> > +		strbuf_addf(buf, "shell-path: %s\n", SHELL_PATH);
> > +		/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
> 
> ... and the pattern indicates the output will end with a complete
> line when !!show_build_options, too.
> 
> > +	}
> > +}
> 
> So, was the strbuf_complete_line() merely defensive programming?  It
> may deserve a comment if it will stay there.

> On Thu, Feb 20, 2020 at 12:07:46PM -0800, Junio C Hamano wrote:
>> Emily Shaffer <emilyshaffer@google.com> writes:
>> 
>> > +static void get_system_info(struct strbuf *sys_info)
>> > +{
>> > +	/* get git version from native cmd */
>> > +	strbuf_addstr(sys_info, "git version:\n");
>> > +	get_version_info(sys_info, 1);
>> > +	strbuf_complete_line(sys_info);
>> 
>> It is a bit curious use of "don't do anything if sys_info ends with
>> a complete line, but complete it if it ends with an imcomplete
>> line".  That tells the readers that we do not know what
>> get_version_info() will do (now or in the future) to its output
>> buffer.
>>  ...
>> So, was the strbuf_complete_line() merely defensive programming?  It
>> may deserve a comment if it will stay there.
>
> It was meant defensively, here and elsewhere in the series. I figured
> that for something like this, which is mostly bounded by human writing
> in an editor and then by file IO, spurious string-checking was not such
> a big deal.
>
> Are you suggesting to comment around the strbuf_complete_line() calls,
> or to comment around get_version_info() that it should end in newline?

> +	else
> +		strbuf_addf(sys_info, "%s %s %s %s\n",
> +			    uname_info.sysname,
> +			    uname_info.release,
> +			    uname_info.version,
> +			    uname_info.machine);
>  }
>  
>  static const char * const bugreport_usage[] = {

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > +	/* system call for other version info */
> > +	strbuf_addstr(sys_info, "uname -a: ");
> 
> Is that "-a" portable across systems?  That is, when given "-a",
> would "uname" on all platforms show sysname, release, version and
> machine in that order and nothing else?
> 
> If we are merely saying "in this section, we are showing a selected
> subset from what we learned about the system with uname(2)", perhaps
> just stop at saying "uname: "?

> 
> > +	if (uname(&uname_info))
> > +		strbuf_addf(sys_info, "uname() failed with code %d\n", errno);
> 
> i18n/l10n?  Don't we want to use strerror() here?

> diff --git a/compat/compiler.h b/compat/compiler.h
> new file mode 100644
> index 0000000000..ef41177233
> --- /dev/null
> +++ b/compat/compiler.h
> @@ -0,0 +1,27 @@
> +#ifndef COMPILER_H
> +#define COMPILER_H
> +
> +#include "git-compat-util.h"
> +#include "strbuf.h"
> +
> +#ifdef __GLIBC__
> +#include <gnu/libc-version.h>
> +
> +static inline void get_compiler_info(struct strbuf *info)
> +{
> +	strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version());
> +#ifdef __GNUC__
> +	strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__);
> +#endif

> On 2020-02-19 17:58:49-0800, Emily Shaffer <emilyshaffer@google.com> wrote:
> > diff --git a/compat/compiler.h b/compat/compiler.h
> > new file mode 100644
> > index 0000000000..ef41177233
> > --- /dev/null
> > +++ b/compat/compiler.h
> > @@ -0,0 +1,27 @@
> > +#ifndef COMPILER_H
> > +#define COMPILER_H
> > +
> > +#include "git-compat-util.h"
> > +#include "strbuf.h"
> > +
> > +#ifdef __GLIBC__
> > +#include <gnu/libc-version.h>
> > +
> > +static inline void get_compiler_info(struct strbuf *info)
> > +{
> > +	strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version());
> > +#ifdef __GNUC__
> > +	strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__);
> > +#endif
> 
> I think we're better having
> 
> - get_compiler_info placed under #ifdef __GNUC__ gate,
> - get_libc_info under #ifdef __GLIBC
> 
> Then have a function to merge those information together.
> Correct me if I were wrong, as it is now,
> this is only useful for Linux people with glibc.
> 
> Anyway, clang also defines __GNUC__ and __GNUC_MINOR__,
> IIRC, FreeBSD people started to move to use clang instead of gcc
> as their default compiler.
> 
> Gentoo forks are also known to use clang to compile their ports.

> @@ -24,6 +25,10 @@ static void get_system_info(struct strbuf *sys_info)
>  			    uname_info.release,
>  			    uname_info.version,
>  			    uname_info.machine);
> +
> +	strbuf_addstr(sys_info, "compiler info: ");
> +	get_compiler_info(sys_info);
> +	strbuf_complete_line(sys_info);
>  }
>  
>  static const char * const bugreport_usage[] = {
> diff --git a/compat/compiler.h b/compat/compiler.h
> new file mode 100644
> index 0000000000..ef41177233
> --- /dev/null
> +++ b/compat/compiler.h
> @@ -0,0 +1,27 @@
> +#ifndef COMPILER_H
> +#define COMPILER_H
> +
> +#include "git-compat-util.h"
> +#include "strbuf.h"
> +
> +#ifdef __GLIBC__
> +#include <gnu/libc-version.h>
> +
> +static inline void get_compiler_info(struct strbuf *info)
> +{
> +	strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version());
> +#ifdef __GNUC__
> +	strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__);
> +#endif
> +}
> +
> +#else
> +
> +static inline void get_compiler_info(struct strbuf *info)
> +{
> +	strbuf_addstr(info, "get_compiler_info() not implemented");

> +static void get_git_remote_https_version_info(struct strbuf *version_info)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +
> +	cp.git_cmd = 1;
> +	argv_array_push(&cp.args, "remote-https");
> +	argv_array_push(&cp.args, "--build-info");
> +	if (capture_command(&cp, version_info, 0))
> +	    strbuf_addstr(version_info, "'git-remote-https --build-info' not supported\n");
> +}
>  
>  static void get_system_info(struct strbuf *sys_info)
>  {
> @@ -29,6 +41,10 @@ static void get_system_info(struct strbuf *sys_info)
>  	strbuf_addstr(sys_info, "compiler info: ");
>  	get_compiler_info(sys_info);
>  	strbuf_complete_line(sys_info);
> +
> +	strbuf_addstr(sys_info, "git-remote-https --build-info:\n");
> +	get_git_remote_https_version_info(sys_info);
> +	strbuf_complete_line(sys_info);
>  }
>  
>  static const char * const bugreport_usage[] = {
> diff --git a/remote-curl.c b/remote-curl.c
> index 8eb96152f5..73e52175c0 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -17,6 +17,7 @@
>  #include "protocol.h"
>  #include "quote.h"
>  #include "transport.h"
> +#include "version.h"
>  
>  static struct remote *remote;
>  /* always ends with a trailing slash */
> @@ -1375,6 +1376,13 @@ int cmd_main(int argc, const char **argv)
>  	string_list_init(&options.deepen_not, 1);
>  	string_list_init(&options.push_options, 1);
>  
> +	if (!strcmp("--build-info", argv[1])) {
> +		printf("git-http-fetch version: %s\n", git_version_string);

> +		printf("built from commit: %s\n", git_built_from_commit_string);
> +		printf("curl version: %s\n", curl_version());
> +		return 0;
> +	}
> +
>  	/*
>  	 * Just report "remote-curl" here (folding all the various aliases
>  	 * ("git-remote-http", "git-remote-https", and etc.) here since they

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > +static void get_git_remote_https_version_info(struct strbuf *version_info)
> > +{
> > +	struct child_process cp = CHILD_PROCESS_INIT;
> > +
> > +	cp.git_cmd = 1;
> > +	argv_array_push(&cp.args, "remote-https");
> > +	argv_array_push(&cp.args, "--build-info");
> > +	if (capture_command(&cp, version_info, 0))
> > +	    strbuf_addstr(version_info, "'git-remote-https --build-info' not supported\n");
> > +}
> >  
> >  static void get_system_info(struct strbuf *sys_info)
> >  {
> > @@ -29,6 +41,10 @@ static void get_system_info(struct strbuf *sys_info)
> >  	strbuf_addstr(sys_info, "compiler info: ");
> >  	get_compiler_info(sys_info);
> >  	strbuf_complete_line(sys_info);
> > +
> > +	strbuf_addstr(sys_info, "git-remote-https --build-info:\n");
> > +	get_git_remote_https_version_info(sys_info);
> > +	strbuf_complete_line(sys_info);
> >  }
> >  
> >  static const char * const bugreport_usage[] = {
> > diff --git a/remote-curl.c b/remote-curl.c
> > index 8eb96152f5..73e52175c0 100644
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> > @@ -17,6 +17,7 @@
> >  #include "protocol.h"
> >  #include "quote.h"
> >  #include "transport.h"
> > +#include "version.h"
> >  
> >  static struct remote *remote;
> >  /* always ends with a trailing slash */
> > @@ -1375,6 +1376,13 @@ int cmd_main(int argc, const char **argv)
> >  	string_list_init(&options.deepen_not, 1);
> >  	string_list_init(&options.push_options, 1);
> >  
> > +	if (!strcmp("--build-info", argv[1])) {
> > +		printf("git-http-fetch version: %s\n", git_version_string);
> 
> We are letting bugreport grab this information from remote-curl, and
> they are different binaries, so git_version_string we see here is
> that of the remote-curl.  Good.
> 
> > +		printf("built from commit: %s\n", git_built_from_commit_string);
> > +		printf("curl version: %s\n", curl_version());
> > +		return 0;
> > +	}
> > +
> >  	/*
> >  	 * Just report "remote-curl" here (folding all the various aliases
> >  	 * ("git-remote-http", "git-remote-https", and etc.) here since they
> 
> Makes sense, except that it is not clear what our overall stance on
> i18n/l10n of the bugreport output.
> 
> I think there are two schools of thought.  
> 
>  - You can stick to C local, with an expectation that developers can
>    read reports in C locale.  This will allow developers to avoid
>    having to read reports written in a language they do not
>    understand, and also makes it easier to mechanically process
>    reports.
> 
>  - You can make sure what matters in the report is localized to the
>    end-users' locale.  This will avoid forcing end-users to send out
>    a report that they may not even able to read (which is what
>    happens if we did not do i18n/l10n).
> 
> I am not sure which way you are aiming at.  The boilerplate text
> we saw in a very early part of the series were marked N_() but some
> of the messages in later parts of the series are not.

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > ... Unfortunately I agree that we wouldn't be equipped to handle
> > reports in other languages.
> 
> I actually was anticipating a far better world ;-) 
> 
> There is no reason to limit the recipient of reports only to *us*.
> Use of Git and population proficient in Git would become wide enough
> that we should be able to expect that users who speak language X can
> be helped by experts who speak the same language X with their
> issues.

> How about if I localize the bugreport template, headers, and formatted
> comments (e.g. "3745 local loose objects"), and include a tip in 'git
> help bugreport' suggesting that if it doesn't look right, maybe the user
> wants to run it with 'LANG= LANGUAGE= git bugreport' to ensure it
> actually gets generated?

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > How about if I localize the bugreport template, headers, and formatted
> > comments (e.g. "3745 local loose objects"), and include a tip in 'git
> > help bugreport' suggesting that if it doesn't look right, maybe the user
> > wants to run it with 'LANG= LANGUAGE= git bugreport' to ensure it
> > actually gets generated?
> 
> I think that will be what is going to happen anyway in the real
> world.  We'd spend a reasonable effort for localization (and I
> personally am perfectly OK if the effort for the initial round is
> "almost zero"), but make sure C-locale is left as an escape hatch.

> 
> > I had another thought, actually, that this is maybe semantically similar
> > problem to the malformed config we discussed earlier in the review. Does
> > it make sense to include some kind of --safemode flag to 'git' which
> > asks it to not perform localization and not read configs?
> 
> I suspect that "git --safemode" would always end up being buggy than
> running "git" under LANG=C LC_ALL=C GIT_CONFIG=/dev/null or something
> like that ;-)
> 
> Wouldn't it defeat more than 30% of the value of the tool if we
> do not read and report the contents of the configuration file(s)?

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > Hm. I guess I got the opposite impression from you way back in v1. I do
> > wish it had been communicated a little more clearly; it's frustrating to
> > perceive a reversal after seven months of review. But that's probably on
> > my own reading comprehension :)
> 
> Well, seven months is a long time for anybody to learn from repeated
> reading and gained experience to form an opinion and get affected by
> others' opinions.  In any case, this is one of the reasons why I try
> to discourage people to have too many topics in flight before moving
> on to a new and different topic---the risk of ending up with fliping
> and flopping is just too high when you give people chance to forget.
> Rather, I'd prefer to see something simple to land first and then
> later refined.

> On this particular issue, I actually do not have a preference.  As
> long as the topic has a coherent position/stance, any one of
> 
>  (a) we do as much i18n as possible to help end-users, or
> 
>  (b) we stay away from i18n to ensure the reports are machine
>      readable, or
> 
>  (c) somewhere in between with a clear criterion where you are
>      drawing the line (e.g. "the introductory text is what we want
>      the end-user to read, so it is i18ned, but the report about
>      their environment are primarily for our use and we avoid
>      localizing so that we can process mechanically"),
> 
> is fine.  The important point is that we choose what we do with a
> solid guiding principle behind the decision.
> 
> In practice, every string in bugreport.c you have control over the
> use (or non-use) of _() around it, but codepaths that you call from
> existing parts of the system are likely to have their messages
> localized if they are meant for Porcelain use.  So from that point
> of view, (a) would be easier to arrange than (b), I suspect.

> --- /dev/null
> +++ b/generate-bugreport-config-safelist.sh
> @@ -0,0 +1,18 @@
> +#!/bin/sh
> +
> +cat <<EOF

> +/* Automatically generated by bugreport-generate-config-safelist.sh */
> +
> +
> +static const char *bugreport_config_safelist[] = {
> +EOF
> +
> +# cat all regular files in Documentation/config
> +find Documentation/config -type f -exec cat {} \; |
> +# print the command name which matches the annotate-bugreport macro
> +sed -n 's/^\([^ ]*\)  *annotate:bugreport\[include\].* ::$/  "\1",/p' \
> +	| sort
> +

> diff --git a/Makefile b/Makefile
> index 9e6705061d..6bdd3b9337 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -818,6 +818,7 @@ VCSSVN_LIB = vcs-svn/lib.a
>
>  GENERATED_H += config-list.h
>  GENERATED_H += command-list.h
> +GENERATED_H += bugreport-config-safelist.h
>
>  LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
>  	$(FIND) . \

> Subject: [PATCH] fixup??? bugreport: generate config safelist based on docs
>
> The Visual Studio build is a special beast: as we cannot assume the
> presence of any Unix tools on Windows, we have to commit all of the
> files generated via shell scripts.
>
> These two generated header files are no exception.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  config.mak.uname | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/config.mak.uname b/config.mak.uname
> index 6d58d22cd5a..f1f36e43e47 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -788,8 +788,10 @@ vcxproj:
>  	git add -f git/LinkOrCopyBuiltins.targets git-remote-http/LinkOrCopyRemoteHttp.targets
>
>  	# Add command-list.h
> -	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 command-list.h
> -	git add -f command-list.h
> +	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 command-list.h \
> +		config-list.h bugreport-config-safelist.h
> +	git add -f command-list.h \
> +	 	config-list.h bugreport-config-safelist.h

> @@ -94,6 +120,7 @@ int cmd_main(int argc, const char **argv)
>  	char *option_output = NULL;
>  	char *option_suffix = "%F-%H%M";
>  	struct stat statbuf;
> +	int nongit_ok = 0;
>  
>  	const struct option bugreport_options[] = {
>  		OPT_STRING('o', "output-directory", &option_output, N_("path"),
> @@ -102,6 +129,10 @@ int cmd_main(int argc, const char **argv)
>  			   N_("specify a strftime format suffix for the filename")),
>  		OPT_END()
>  	};
> +
> +	/* Prerequisite for hooks and config checks */
> +	setup_git_directory_gently(&nongit_ok);

> +static void get_populated_hooks(struct strbuf *hook_info, int nongit)
> +{
> +	/*
> +	 * Doesn't look like there is a list of all possible hooks; so below is
> +	 * a transcription of `git help hooks`.
> +	 */

> +	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;
> +
> +
> +	if (nongit) {
> +		strbuf_addstr(hook_info,
> +			"not run from a git repository - no hooks to show\n");
> +		return;
> +	}
> +
> +	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);
> +		}
> +	}
> +}
> +
>  static const char * const bugreport_usage[] = {
>  	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
>  	NULL
> @@ -166,6 +216,9 @@ int cmd_main(int argc, const char **argv)
>  	get_header(&buffer, "Safelisted Config Info");
>  	get_safelisted_config(&buffer);
>  
> +	get_header(&buffer, "Enabled Hooks");
> +	get_populated_hooks(&buffer, nongit_ok);
> +
>  	report = fopen_for_writing(report_path.buf);
>  
>  	if (report == NULL) {

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > +static void get_populated_hooks(struct strbuf *hook_info, int nongit)
> > +{
> > +	/*
> > +	 * Doesn't look like there is a list of all possible hooks; so below is
> > +	 * a transcription of `git help hooks`.
> > +	 */
> 
> It may want to become a NEEDSWORK comment.

> 
> > +	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";
> 
> Typo here?

> 
> > +	struct string_list hooks_list = STRING_LIST_INIT_DUP;
> > +	struct string_list_item *iter = NULL;
> > +
> > +
> > +	if (nongit) {
> > +		strbuf_addstr(hook_info,
> > +			"not run from a git repository - no hooks to show\n");
> > +		return;
> > +	}
> > +
> > +	string_list_split(&hooks_list, hooks, ',', -1);
> > +
> > +	for_each_string_list_item(iter, &hooks_list) {
> 
> I do not get why you want to use string_list for this, especially if
> you need to use string_list_split.
> 
> To me,
> 
> 	int i;
> 	const char *hook[] = {
> 		"applypatch-msg",
> 	        "pre-applypatch",
> 		...
> 		"post-index-change",
> 	};
> 
> 	for (i = 0; i < ARRAY_SIZE(hook); i++) {
> 		if (hook[i] is enabled)
> 			strbuf_addf(hook_info, "%s\n", hook[i]);
> 	}
> 
> would be far easier to understand.  Do you have an external source
> that can feed you a single long string of comma separated hook names
> in mind, so that the initialization of *hooks will become simpler
> that way, or something?

> 
> > +		if (find_hook(iter->string)) {
> > +			strbuf_addstr(hook_info, iter->string);
> > +			strbuf_complete_line(hook_info);
> > +		}
> > +	}
> > +}
> > +
> >  static const char * const bugreport_usage[] = {
> >  	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
> >  	NULL
> > @@ -166,6 +216,9 @@ int cmd_main(int argc, const char **argv)
> >  	get_header(&buffer, "Safelisted Config Info");
> >  	get_safelisted_config(&buffer);
> >  
> > +	get_header(&buffer, "Enabled Hooks");
> > +	get_populated_hooks(&buffer, nongit_ok);
> > +
> >  	report = fopen_for_writing(report_path.buf);
> >  
> >  	if (report == NULL) {

> 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.
>
> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  Documentation/git-bugreport.txt |  1 +
>  bugreport.c                     | 52 +++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
>
> diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
> index 4d01528540..4fe1c60506 100644
> --- a/Documentation/git-bugreport.txt
> +++ b/Documentation/git-bugreport.txt
> @@ -32,6 +32,7 @@ The following information is captured automatically:
>   - $SHELL
>   - Selected config values
>   - A list of enabled hooks
> + - The number of loose objects in the repository
>  
>  This tool is invoked via the typical Git setup process, which means that in some
>  cases, it might not be able to launch - for example, if a relevant config file
> diff --git a/bugreport.c b/bugreport.c
> index b5a0714a7f..fb7bc72723 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -10,6 +10,7 @@
>  #include "bugreport-config-safelist.h"
>  #include "khash.h"
>  #include "run-command.h"
> +#include "object-store.h"
>  
>  static void get_git_remote_https_version_info(struct strbuf *version_info)
>  {
> @@ -128,6 +129,54 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
>  	}
>  }
>  
> +static int loose_object_cb(const struct object_id *oid, const char *path,
> +			   void *data) {
> +	int *loose_object_count = data;
> +
> +	if (loose_object_count) {
> +		(*loose_object_count)++;
> +		return 0;
> +	}
> +
> +	return 1;

> +}
> +
> +static void get_loose_object_summary(struct strbuf *obj_info, int nongit) {
> +
> +	int local_loose_object_count = 0, total_loose_object_count = 0;
> +	int local_count_questionable = 0, total_count_questionable = 0;
> +
> +	if (nongit) {
> +		strbuf_addstr(obj_info,
> +			"not run from a git repository - no objects to show\n");
> +		return;
> +	}
> +
> +	local_count_questionable = for_each_loose_object(
> +					loose_object_cb,
> +					&local_loose_object_count,
> +					FOR_EACH_OBJECT_LOCAL_ONLY);
> +
> +	total_count_questionable = for_each_loose_object(
> +					loose_object_cb,
> +					&total_loose_object_count,
> +					0);
> +	strbuf_addf(obj_info, "%d local loose objects%s\n",
> +		    local_loose_object_count,
> +		    local_count_questionable ? " (problem during count)" : "");
> +
> +	strbuf_addf(obj_info, "%d alternate loose objects%s\n",
> +		    total_loose_object_count - local_loose_object_count,
> +		    (local_count_questionable || total_count_questionable)
> +			? " (problem during count)"
> +			: "");
> +
> +	strbuf_addf(obj_info, "%d total loose objects%s\n",
> +		    total_loose_object_count,
> +		    total_count_questionable ? " (problem during count)" : "");
> +}
> +
>  static const char * const bugreport_usage[] = {
>  	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
>  	NULL
> @@ -219,6 +268,9 @@ int cmd_main(int argc, const char **argv)
>  	get_header(&buffer, "Enabled Hooks");
>  	get_populated_hooks(&buffer, nongit_ok);
>  
> +	get_header(&buffer, "Loose Object Counts");
> +	get_loose_object_summary(&buffer, nongit_ok);
> +
>  	report = fopen_for_writing(report_path.buf);
>  
>  	if (report == NULL) {

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > 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.
> >
> > Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> >  Documentation/git-bugreport.txt |  1 +
> >  bugreport.c                     | 52 +++++++++++++++++++++++++++++++++
> >  2 files changed, 53 insertions(+)
> >
> > diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
> > index 4d01528540..4fe1c60506 100644
> > --- a/Documentation/git-bugreport.txt
> > +++ b/Documentation/git-bugreport.txt
> > @@ -32,6 +32,7 @@ The following information is captured automatically:
> >   - $SHELL
> >   - Selected config values
> >   - A list of enabled hooks
> > + - The number of loose objects in the repository
> >  
> >  This tool is invoked via the typical Git setup process, which means that in some
> >  cases, it might not be able to launch - for example, if a relevant config file
> > diff --git a/bugreport.c b/bugreport.c
> > index b5a0714a7f..fb7bc72723 100644
> > --- a/bugreport.c
> > +++ b/bugreport.c
> > @@ -10,6 +10,7 @@
> >  #include "bugreport-config-safelist.h"
> >  #include "khash.h"
> >  #include "run-command.h"
> > +#include "object-store.h"
> >  
> >  static void get_git_remote_https_version_info(struct strbuf *version_info)
> >  {
> > @@ -128,6 +129,54 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
> >  	}
> >  }
> >  
> > +static int loose_object_cb(const struct object_id *oid, const char *path,
> > +			   void *data) {
> > +	int *loose_object_count = data;
> > +
> > +	if (loose_object_count) {
> > +		(*loose_object_count)++;
> > +		return 0;
> > +	}
> > +
> > +	return 1;
> 
> What is the point of returning 1 here to abort the iteration early?
> Wouldn't it be a BUG() if this callback ends up gettting called with
> NULL in data?

> On Thu, Feb 20, 2020 at 01:04:33PM -0800, Junio C Hamano wrote:
> > Emily Shaffer <emilyshaffer@google.com> writes:
> > 
> > > 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.
> > >
> > > Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > > ---
> > >  Documentation/git-bugreport.txt |  1 +
> > >  bugreport.c                     | 52 +++++++++++++++++++++++++++++++++
> > >  2 files changed, 53 insertions(+)
> > >
> > > diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
> > > index 4d01528540..4fe1c60506 100644
> > > --- a/Documentation/git-bugreport.txt
> > > +++ b/Documentation/git-bugreport.txt
> > > @@ -32,6 +32,7 @@ The following information is captured automatically:
> > >   - $SHELL
> > >   - Selected config values
> > >   - A list of enabled hooks
> > > + - The number of loose objects in the repository
> > >  
> > >  This tool is invoked via the typical Git setup process, which means that in some
> > >  cases, it might not be able to launch - for example, if a relevant config file
> > > diff --git a/bugreport.c b/bugreport.c
> > > index b5a0714a7f..fb7bc72723 100644
> > > --- a/bugreport.c
> > > +++ b/bugreport.c
> > > @@ -10,6 +10,7 @@
> > >  #include "bugreport-config-safelist.h"
> > >  #include "khash.h"
> > >  #include "run-command.h"
> > > +#include "object-store.h"
> > >  
> > >  static void get_git_remote_https_version_info(struct strbuf *version_info)
> > >  {
> > > @@ -128,6 +129,54 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
> > >  	}
> > >  }
> > >  
> > > +static int loose_object_cb(const struct object_id *oid, const char *path,
> > > +			   void *data) {
> > > +	int *loose_object_count = data;
> > > +
> > > +	if (loose_object_count) {
> > > +		(*loose_object_count)++;
> > > +		return 0;
> > > +	}
> > > +
> > > +	return 1;
> > 
> > What is the point of returning 1 here to abort the iteration early?
> > Wouldn't it be a BUG() if this callback ends up gettting called with
> > NULL in data?
> 
> Hrm. Wouldn't BUG() throw away the rest of the generated report?
> 
> Maybe it's better, in NULL case, to indicate an error occurred there and
> write that information into the report, and continue gathering the rest
> of the info.

> +static void get_packed_object_summary(struct strbuf *obj_info, int nongit)
> +{
> +	struct packed_git *pack = NULL;
> +	int pack_count = 0;
> +	int object_count = 0;
> +
> +	if (nongit) {
> +		strbuf_addstr(obj_info,
> +			"not run from a git repository - no objects to show\n");
> +		return;
> +	}
> +
> +	for_each_pack(the_repository, pack) {
> +		pack_count++;
> +		/*
> +		 * To accurately count how many objects are packed, look inside
> +		 * the packfile's index.
> +		 */
> +		open_pack_index(pack);
> +		object_count += pack->num_objects;
> +	}
> +
> +	strbuf_addf(obj_info, "%d total packs (%d objects)\n", pack_count,
> +		    object_count);
> +
> +}

> @@ -447,4 +448,9 @@ int for_each_object_in_pack(struct packed_git *p,
>  int for_each_packed_object(each_packed_object_fn, void *,
>  			   enum for_each_object_flags flags);
>  
> +#define for_each_pack(repo, pack) 		\
> +		for (pack = get_all_packs(repo);\
> +		     pack;			\
> +		     pack = pack->next)

>  #endif /* OBJECT_STORE_H */
> diff --git a/packfile.c b/packfile.c
> index 99dd1a7d09..95afcc1187 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -2095,8 +2095,7 @@ int for_each_packed_object(each_packed_object_fn cb, void *data,
>  	int r = 0;
>  	int pack_errors = 0;
>  
> -	prepare_packed_git(the_repository);
> -	for (p = get_all_packs(the_repository); p; p = p->next) {
> +	for_each_pack(the_repository, p) {
>  		if ((flags & FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
>  			continue;
>  		if ((flags & FOR_EACH_OBJECT_PROMISOR_ONLY) &&

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > +static void get_packed_object_summary(struct strbuf *obj_info, int nongit)
> > +{
> > +	struct packed_git *pack = NULL;
> > +	int pack_count = 0;
> > +	int object_count = 0;
> > +
> > +	if (nongit) {
> > +		strbuf_addstr(obj_info,
> > +			"not run from a git repository - no objects to show\n");
> > +		return;
> > +	}
> > +
> > +	for_each_pack(the_repository, pack) {
> > +		pack_count++;
> > +		/*
> > +		 * To accurately count how many objects are packed, look inside
> > +		 * the packfile's index.
> > +		 */
> > +		open_pack_index(pack);
> > +		object_count += pack->num_objects;
> > +	}
> > +
> > +	strbuf_addf(obj_info, "%d total packs (%d objects)\n", pack_count,
> > +		    object_count);
> > +
> > +}
> 
> Makes sense.
> 
> > @@ -447,4 +448,9 @@ int for_each_object_in_pack(struct packed_git *p,
> >  int for_each_packed_object(each_packed_object_fn, void *,
> >  			   enum for_each_object_flags flags);
> >  
> > +#define for_each_pack(repo, pack) 		\
> > +		for (pack = get_all_packs(repo);\
> > +		     pack;			\
> > +		     pack = pack->next)
> 
> I generally avoid #define'ing a control loop pseudo-syntax unless it
> makes the resulting code hide overly ugly implementation detail.
> 
> for_each_string_list() is there to hide the fact that items are
> stored in an embedded array whose name is .items and size is .nr
> that is sufficiently ugnly to expose, but iterating over packs
> does not look so bad.
> 
> If you MUST have this as a pseudo-syntax macro, we need
> 
>  - to match for_each_string_list_item(), have iterating variable
>    'pack' as the first parameter, and the scope of what's iterated
>    'repo' as the second.
> 
>  - to make sure the syntax works correctly even if a parameter is
>    *not* a simple identifier (I think the above is OK, but there may
>    be cases that it does not work well).
> 
> Regarding the latter, the way 'item' is incremented at the end of
> iteration in for_each_string_list_item() is subtle and correct.
> 
> #define for_each_string_list_item(item,list)            \
> 	for (item = (list)->items;                      \
> 	     item && item < (list)->items + (list)->nr; \
> 	     ++item)
> 
> You would break it if you changed pre-increment to post-increment
> for a user like this:
> 
> 	struct string_list *list;
> 	struct string_list_item *i, **p;
> 	p = &i;
> 
> 	for_each_string_list_item(*p, list) {
> 		...
> 	}
> 
> because ++*p is ++(*p), while *p++ is (*p)++, and we do want the
> former (i.e. increment the memory cell pointed at by pointer p).
> 
> Personally, I would prefer not to introduce this macro if I were
> working on this topic.

> 
> >  #endif /* OBJECT_STORE_H */
> > diff --git a/packfile.c b/packfile.c
> > index 99dd1a7d09..95afcc1187 100644
> > --- a/packfile.c
> > +++ b/packfile.c
> > @@ -2095,8 +2095,7 @@ int for_each_packed_object(each_packed_object_fn cb, void *data,
> >  	int r = 0;
> >  	int pack_errors = 0;
> >  
> > -	prepare_packed_git(the_repository);
> > -	for (p = get_all_packs(the_repository); p; p = p->next) {
> > +	for_each_pack(the_repository, p) {
> >  		if ((flags & FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
> >  			continue;
> >  		if ((flags & FOR_EACH_OBJECT_PROMISOR_ONLY) &&

> +static void list_contents_of_dir_recursively(struct strbuf *contents,
> +					     struct strbuf *dirpath)
> +{
> +	struct dirent *d;
> +	DIR *dir;
> +	size_t path_len;
> +
> +	dir = opendir(dirpath->buf);
> +	if (!dir)
> +		return;

> +	strbuf_complete(dirpath, '/');
> +	path_len = dirpath->len;
> +
> +	while ((d = readdir(dir))) {
> +		if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
> +			continue;
> +
> +		strbuf_addbuf(contents, dirpath);
> +		strbuf_addstr(contents, d->d_name);
> +		strbuf_complete_line(contents);
> +
> +		if (d->d_type == DT_DIR) {
> +			strbuf_addstr(dirpath, d->d_name);
> +			list_contents_of_dir_recursively(contents, dirpath);
> +		}
> +		strbuf_setlen(dirpath, path_len);

> +static void get_object_info_summary(struct strbuf *obj_info, int nongit)
> +{
> +	struct strbuf dirpath = STRBUF_INIT;
> +
> +	if (nongit) {
> +		strbuf_addstr(obj_info,
> +			"not run from a git repository - object info unavailable\n");
> +		return;
> +	}
> +
> +	strbuf_addstr(&dirpath, get_object_directory());
> +	strbuf_complete(&dirpath, '/');
> +	strbuf_addstr(&dirpath, "info/");

> +	list_contents_of_dir_recursively(obj_info, &dirpath);
> +	strbuf_release(&dirpath);
> +}
> +
>  static const char * const bugreport_usage[] = {
>  	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
>  	NULL
> @@ -301,6 +352,9 @@ int cmd_main(int argc, const char **argv)
>  	get_header(&buffer, "Packed Object Summary");
>  	get_packed_object_summary(&buffer, nongit_ok);
>  
> +	get_header(&buffer, "Object Info Summary");
> +	get_object_info_summary(&buffer, nongit_ok);
> +
>  	report = fopen_for_writing(report_path.buf);
>  
>  	if (report == NULL) {

> 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. Since paths to alternates may
> be sensitive, we'll instead count how many alternates have been
> specified and note how many of them exist or are broken.
>
> While object-cache.h describes a function "foreach_alt_odb()", this
> function does not provide information on broken alternates, which are
> skipped over in "link_alt_odb_entry()". Since the goal is to identify
> missing alternates, we can gather the contents of
> .git/objects/info/alternates manually.

> diff --git a/bugreport.c b/bugreport.c
> index 1d61e0f642..1640a71086 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -255,6 +255,48 @@ static void get_object_info_summary(struct strbuf *obj_info, int nongit)
>  	strbuf_release(&dirpath);
>  }
>
> +static void get_alternates_summary(struct strbuf *alternates_info, int nongit)
> +{
> +	struct strbuf alternates_path = STRBUF_INIT;
> +	struct strbuf alternate = STRBUF_INIT;

> +	FILE *file;
> +	size_t exists = 0, broken = 0;
> +
> +	if (nongit) {
> +		strbuf_addstr(alternates_info,
> +			"not run from a git repository - alternates unavailable\n");
> +		return;
> +	}
> +
> +	strbuf_addstr(&alternates_path, get_object_directory());
> +	strbuf_complete(&alternates_path, '/');
> +	strbuf_addstr(&alternates_path, "info/alternates");
> +
> +	file = fopen(alternates_path.buf, "r");
> +	if (!file) {
> +		strbuf_addstr(alternates_info, "No alternates file found.\n");
> +		strbuf_release(&alternates_path);
> +		return;
> +	}
> +
> +	while (strbuf_getline(&alternate, file) != EOF) {
> +		if (!access(alternate.buf, F_OK))

> +			exists++;
> +		else
> +			broken++;
> +	}
> +
> +	strbuf_addf(alternates_info,
> +		    "%zd alternates found (%zd working, %zd broken)\n",

> +static void get_alternates_summary(struct strbuf *alternates_info, int nongit)
> +{
> +	struct strbuf alternates_path = STRBUF_INIT;
> +	struct strbuf alternate = STRBUF_INIT;
> +	FILE *file;
> +	size_t exists = 0, broken = 0;
> +
> +	if (nongit) {
> +		strbuf_addstr(alternates_info,
> +			"not run from a git repository - alternates unavailable\n");
> +		return;
> +	}
> +
> +	strbuf_addstr(&alternates_path, get_object_directory());
> +	strbuf_complete(&alternates_path, '/');
> +	strbuf_addstr(&alternates_path, "info/alternates");

> +	file = fopen(alternates_path.buf, "r");
> +	if (!file) {
> +		strbuf_addstr(alternates_info, "No alternates file found.\n");
> +		strbuf_release(&alternates_path);
> +		return;
> +	}
> +
> +	while (strbuf_getline(&alternate, file) != EOF) {
> +		if (!access(alternate.buf, F_OK))
> +			exists++;

> +	}
> +
> +	strbuf_addf(alternates_info,
> +		    "%zd alternates found (%zd working, %zd broken)\n",
> +		    exists + broken,
> +		    exists,
> +		    broken);

> +	fclose(file);
> +	strbuf_release(&alternate);
> +	strbuf_release(&alternates_path);
> +}
> +
>  static const char * const bugreport_usage[] = {
>  	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
>  	NULL
> @@ -355,6 +397,9 @@ int cmd_main(int argc, const char **argv)
>  	get_header(&buffer, "Object Info Summary");
>  	get_object_info_summary(&buffer, nongit_ok);
>  
> +	get_header(&buffer, "Alternates");
> +	get_alternates_summary(&buffer, nongit_ok);
> +
>  	report = fopen_for_writing(report_path.buf);
>  
>  	if (report == NULL) {

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

>
> diff --git a/.gitignore b/.gitignore
> index ea97de83f3..d89bf9e11e 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -25,6 +25,7 @@
>  /git-bisect--helper
>  /git-blame
>  /git-branch
> +/git-bugreport
>  /git-bundle
>  /git-cat-file
>  /git-check-attr
> diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
> new file mode 100644
> index 0000000000..1f9fde5cde
> --- /dev/null
> +++ b/Documentation/git-bugreport.txt
> @@ -0,0 +1,46 @@
> +git-bugreport(1)
> +================
> +
> +NAME
> +----
> +git-bugreport - Collect information for user to file a bug report
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'git bugreport' [(-o | --output-directory) <path>] [(-s | --suffix) <format>]
> +
> +DESCRIPTION
> +-----------
> +Captures information about the user's machine, Git client, and repository state,
> +as well as a form requesting information about the behavior the user observed,
> +into a single text file which the user can then share, for example to the Git
> +mailing list, in order to report an observed bug.
> +
> +The following information is requested from the user:
> +
> + - Reproduction steps
> + - Expected behavior
> + - Actual behavior
> +
> +This tool is invoked via the typical Git setup process, which means that in some
> +cases, it might not be able to launch - for example, if a relevant config file
> +is unreadable. In this kind of scenario, it may be helpful to manually gather
> +the kind of information listed above when manually asking for help.
> +
> +OPTIONS
> +-------
> +-o <path>::
> +--output-directory <path>::
> +	Place the resulting bug report file in `<path>` instead of the root of
> +	the Git repository.
> +
> +-s <format>::
> +--suffix <format>::
> +	Specify an alternate suffix for the bugreport name, to create a file
> +	named 'git-bugreport-<formatted suffix>'. This should take the form of a
> +	link:strftime[3] format string; the current local time will be used.
> +
> +GIT
> +---
> +Part of the linkgit:git[1] suite
> diff --git a/Makefile b/Makefile
> index c552312d3f..9e6705061d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -681,6 +681,7 @@ EXTRA_PROGRAMS =
>  # ... and all the rest that could be moved out of bindir to gitexecdir
>  PROGRAMS += $(EXTRA_PROGRAMS)
>
> +PROGRAM_OBJS += bugreport.o
>  PROGRAM_OBJS += credential-store.o
>  PROGRAM_OBJS += daemon.o
>  PROGRAM_OBJS += fast-import.o
> @@ -2461,6 +2462,10 @@ endif
>  git-%$X: %.o GIT-LDFLAGS $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
>
> +git-bugreport$X: bugreport.o GIT-LDFLAGS $(GITLIBS)
> +	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> +		$(LIBS)
> +
>  git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
>  		$(IMAP_SEND_LDFLAGS) $(LIBS)
> diff --git a/bugreport.c b/bugreport.c
> new file mode 100644
> index 0000000000..f473d606f2
> --- /dev/null
> +++ b/bugreport.c
> @@ -0,0 +1,105 @@
> +#include "builtin.h"
> +#include "parse-options.h"
> +#include "stdio.h"
> +#include "strbuf.h"
> +#include "time.h"
> +
> +static const char * const bugreport_usage[] = {
> +	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
> +	NULL
> +};
> +
> +static int get_bug_template(struct strbuf *template)
> +{
> +	const char template_text[] = N_(
> +"Thank you for filling out a Git bug report!\n"
> +"Please answer the following questions to help us understand your issue.\n"
> +"\n"
> +"What did you do before the bug happened? (Steps to reproduce your issue)\n"
> +"\n"
> +"What did you expect to happen? (Expected behavior)\n"
> +"\n"
> +"What happened instead? (Actual behavior)\n"
> +"\n"
> +"What's different between what you expected and what actually happened?\n"
> +"\n"
> +"Anything else you want to add:\n"
> +"\n"
> +"Please review the rest of the bug report below.\n"
> +"You can delete any lines you don't wish to share.\n");
> +
> +	strbuf_addstr(template, _(template_text));
> +	return 0;
> +}
> +
> +int cmd_main(int argc, const char **argv)
> +{
> +	struct strbuf buffer = STRBUF_INIT;
> +	struct strbuf report_path = STRBUF_INIT;
> +	int report = -1;
> +	time_t now = time(NULL);
> +	char *option_output = NULL;
> +	char *option_suffix = "%F-%H%M";
> +	int nongit_ok = 0;
> +	const char *prefix = NULL;
> +	const char *user_relative_path = NULL;
> +
> +	const struct option bugreport_options[] = {
> +		OPT_STRING('o', "output-directory", &option_output, N_("path"),
> +			   N_("specify a destination for the bugreport file")),
> +		OPT_STRING('s', "suffix", &option_suffix, N_("format"),
> +			   N_("specify a strftime format suffix for the filename")),
> +		OPT_END()
> +	};
> +
> +	prefix = setup_git_directory_gently(&nongit_ok);
> +
> +	argc = parse_options(argc, argv, prefix, bugreport_options,
> +			     bugreport_usage, 0);
> +
> +	/* Prepare the path to put the result */
> +	strbuf_addstr(&report_path,
> +		      prefix_filename(prefix,
> +				      option_output ? option_output : ""));
> +	strbuf_complete(&report_path, '/');
> +
> +	strbuf_addstr(&report_path, "git-bugreport-");
> +	strbuf_addftime(&report_path, option_suffix, localtime(&now), 0, 0);
> +	strbuf_addstr(&report_path, ".txt");
> +
> +	switch (safe_create_leading_directories(report_path.buf)) {
> +	case SCLD_OK:
> +	case SCLD_EXISTS:
> +		break;
> +	default:
> +		die(_("could not create leading directories for '%s'"),
> +		    report_path.buf);
> +	}
> +
> +	/* Prepare the report contents */
> +	get_bug_template(&buffer);
> +
> +	/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */
> +	report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);
> +
> +	if (report < 0) {
> +		UNLEAK(report_path);
> +		die(_("couldn't create a new file at '%s'"), report_path.buf);
> +	}
> +
> +	strbuf_write_fd(&buffer, report);
> +	close(report);
> +
> +	/*
> +	 * We want to print the path relative to the user, but we still need the
> +	 * path relative to us to give to the editor.
> +	 */
> +	if (!(prefix && skip_prefix(report_path.buf, prefix, &user_relative_path)))
> +		user_relative_path = report_path.buf;
> +	fprintf(stderr, _("Created new report at '%s'.\n"),
> +		user_relative_path);
> +
> +	UNLEAK(buffer);
> +	UNLEAK(report_path);
> +	return !!launch_editor(report_path.buf, NULL, NULL);
> +}
> diff --git a/command-list.txt b/command-list.txt
> index 2087894655..185e5e3f05 100644
> --- a/command-list.txt
> +++ b/command-list.txt
> @@ -54,6 +54,7 @@ git-archive                             mainporcelain
>  git-bisect                              mainporcelain           info
>  git-blame                               ancillaryinterrogators          complete
>  git-branch                              mainporcelain           history
> +git-bugreport                           ancillaryinterrogators
>  git-bundle                              mainporcelain
>  git-cat-file                            plumbinginterrogators
>  git-check-attr                          purehelpers
> diff --git a/strbuf.c b/strbuf.c
> index f19da55b07..f1d66c7848 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -539,6 +539,10 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f)
>  	return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
>  }
>
> +ssize_t strbuf_write_fd(struct strbuf *sb, int fd)
> +{
> +	return sb->len ? write(fd, sb->buf, sb->len) : 0;
> +}
>
>  #define STRBUF_MAXLINK (2*PATH_MAX)
>
> diff --git a/strbuf.h b/strbuf.h
> index aae7ac3a82..bbf6204de7 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -462,6 +462,7 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
>   * NUL bytes.
>   */
>  ssize_t strbuf_write(struct strbuf *sb, FILE *stream);
> +ssize_t strbuf_write_fd(struct strbuf *sb, int fd);
>
>  /**
>   * Read a line from a FILE *, overwriting the existing contents of
> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> new file mode 100755
> index 0000000000..65f664fdac
> --- /dev/null
> +++ b/t/t0091-bugreport.sh
> @@ -0,0 +1,61 @@
> +#!/bin/sh
> +
> +test_description='git bugreport'
> +
> +. ./test-lib.sh
> +
> +# Headers "[System Info]" will be followed by a non-empty line if we put some
> +# information there; we can make sure all our headers were followed by some
> +# information to check if the command was successful.
> +HEADER_PATTERN="^\[.*\]$"
> +
> +check_all_headers_populated () {
> +	while read -r line
> +	do
> +		if test "$(grep "$HEADER_PATTERN" "$line")"
> +		then
> +			echo "$line"
> +			read -r nextline
> +			if test -z "$nextline"; then
> +				return 1;
> +			fi
> +		fi
> +	done
> +}
> +
> +test_expect_success 'creates a report with content in the right places' '
> +	git bugreport -s check-headers &&
> +	check_all_headers_populated <git-bugreport-check-headers.txt &&
> +	test_when_finished rm git-bugreport-check-headers.txt
> +'
> +
> +test_expect_success 'dies if file with same name as report already exists' '
> +	>>git-bugreport-duplicate.txt &&
> +	test_must_fail git bugreport --suffix duplicate &&
> +	test_when_finished rm git-bugreport-duplicate.txt
> +'
> +
> +test_expect_success '--output-directory puts the report in the provided dir' '
> +	git bugreport -o foo/ &&
> +	test_path_is_file foo/git-bugreport-* &&
> +	test_when_finished rm -fr foo/
> +'
> +
> +test_expect_success 'incorrect arguments abort with usage' '
> +	test_must_fail git bugreport --false 2>output &&
> +	test_i18ngrep usage output &&
> +	test_path_is_missing git-bugreport-*
> +'
> +
> +test_expect_success 'runs outside of a git dir' '
> +	nongit git bugreport &&
> +	test_when_finished rm non-repo/git-bugreport-*
> +'
> +
> +test_expect_success 'can create leading directories outside of a git dir' '
> +	nongit git bugreport -o foo/bar/baz &&
> +	test_when_finished rm -fr foo/bar/baz
> +'
> +
> +
> +test_done
> --
> 2.25.0.265.gbab2e86ba0-goog
>
>

> Hi,
> 
> On Mon, 2 Mar 2020, Emily Shaffer wrote:
> 
>>   .gitignore                      |   1 +
>>   Documentation/git-bugreport.txt |  46 ++++++++++++++
>>   Makefile                        |   5 ++
>>   bugreport.c                     | 105 ++++++++++++++++++++++++++++++++
>>   command-list.txt                |   1 +
>>   strbuf.c                        |   4 ++
>>   strbuf.h                        |   1 +
>>   t/t0091-bugreport.sh            |  61 +++++++++++++++++++
>>   8 files changed, 224 insertions(+)
>>   create mode 100644 Documentation/git-bugreport.txt
>>   create mode 100644 bugreport.c
>>   create mode 100755 t/t0091-bugreport.sh
> 
> Hmm. I am still _quite_ convinced that this would be much better as a
> built-in. Remember, non-built-ins come with a footprint, and I do not
> necessarily think that you will want to spend 3MB on a `git-bugreport`
> executable when you could have it for a couple dozen kilobytes inside
> `git` instead.
> 
> Ciao,
> Dscho

> On 3/4/2020 4:35 PM, Johannes Schindelin wrote:
> >
> > On Mon, 2 Mar 2020, Emily Shaffer wrote:
> >
> > >   .gitignore                      |   1 +
> > >   Documentation/git-bugreport.txt |  46 ++++++++++++++
> > >   Makefile                        |   5 ++
> > >   bugreport.c                     | 105 ++++++++++++++++++++++++++++++++
> > >   command-list.txt                |   1 +
> > >   strbuf.c                        |   4 ++
> > >   strbuf.h                        |   1 +
> > >   t/t0091-bugreport.sh            |  61 +++++++++++++++++++
> > >   8 files changed, 224 insertions(+)
> > >   create mode 100644 Documentation/git-bugreport.txt
> > >   create mode 100644 bugreport.c
> > >   create mode 100755 t/t0091-bugreport.sh
> >
> > Hmm. I am still _quite_ convinced that this would be much better as a
> > built-in. Remember, non-built-ins come with a footprint, and I do not
> > necessarily think that you will want to spend 3MB on a `git-bugreport`
> > executable when you could have it for a couple dozen kilobytes inside
> > `git` instead.
> >
> > Ciao,
> > Dscho
>
> Having this command be a stand-alone exe rather than a builtin allows
> it to have a different linkage.  For example, you could include the
> libcurl and other libraries that are only linked into the transports.
> And then report version numbers for them if you wanted.

> Jeff Hostetler <git@jeffhostetler.com> writes:
> 
>> Having this command be a stand-alone exe rather than a builtin allows
>> it to have a different linkage.  For example, you could include the
>> libcurl and other libraries that are only linked into the transports.
>> And then report version numbers for them if you wanted.
> 
> I actually do not think that is a good rationale.  Unless your
> version of "git bugreport" links into the same binary as the
> "transports", it still is possible that the version of cURL (for
> example) "git bugreport" can learn internally from may not have
> anything to do with the version of the library used by the
> transports.
> 
> Of course, making "bugreport" a built-in, i.e. the same binary as
> the non-transport part of Git, is not a solution for that issue,
> either.  As Dscho suggested and recent rounds of "git bugreport"
> implements, teaching the transport binaries an option to report
> relevant pieces of information regarding the libraries they use, and
> making "git bugreport" ask them, is a very good solution for that.
> 
> What makes it possible by making "git bugreport" stand-alone is for
> it to link with libraries that the remainder of Git, including the
> transports that link with libcurl, has no business linking with (a
> library to obtain system details for diagnostic purposes, for
> example).  Early versions of "git bugreport" may not link with
> anything special, but making sure it starts and stays standalone
> leaves it easier to do so _without_ having to worry about splitting
> the code that started as a built-in out to make it independent later.
> 
> 

> What makes it possible by making "git bugreport" stand-alone is for
> it to link with libraries that the remainder of Git, including the
> transports that link with libcurl, has no business linking with (a
> library to obtain system details for diagnostic purposes, for
> example).

> Hi Junio,
>
> On Fri, 6 Mar 2020, Junio C Hamano wrote:
>
>> What makes it possible by making "git bugreport" stand-alone is for
>> it to link with libraries that the remainder of Git, including the
>> transports that link with libcurl, has no business linking with (a
>> library to obtain system details for diagnostic purposes, for
>> example).
>
> That would, however, make `git-bugreport` more fragile than `git`, i.e.
> the former might fail to launch under more circumstances than the latter.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Fri, 6 Mar 2020, Junio C Hamano wrote:
> >
> >> What makes it possible by making "git bugreport" stand-alone is for
> >> it to link with libraries that the remainder of Git, including the
> >> transports that link with libcurl, has no business linking with (a
> >> library to obtain system details for diagnostic purposes, for
> >> example).
> >
> > That would, however, make `git-bugreport` more fragile than `git`, i.e.
> > the former might fail to launch under more circumstances than the latter.
>
> That's a bug.  You can go fix it when it happens.

> On the other hand, if we make `git-bugreport` a built-in, I cannot see any
> downsides.
>
> For me, therefore, having it as a built-in is a clear win. What am I
> missing?

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On the other hand, if we make `git-bugreport` a built-in, I cannot see any
> > downsides.
> >
> > For me, therefore, having it as a built-in is a clear win. What am I
> > missing?
>
> The right sense of relative importance between efficiently running
> the rest of Git by not bloting the main binary and making sure not
> to ship Git that does not run unless "git bugreport" runs (which
> makes sure that "bugreport" runs) is what you are missing, I
> suspect.

> Another thing is that you are giving "git bugreport" too much weight
> and too little credit to inexperienced users, by assuming that we
> will never hear from them when "bugreport" is incapable to run for
> them.  They will report, with or without "git bugreport", and the
> more important thing you seem to be missing is that after the
> initial contact it would become an interactive process---there is
> no reason to prioritize to ensure that the initial contact has
> everything that is needed to diagnose any issue without further
> interaction.  "With my build, even 'bugreport' dumps core." is
> perfectly good place to start.
>
> Besides, wouldn't the ones on platforms, on which "git bugreport"
> may have trouble running, i.e. the ones on minority and exotic
> platforms, tend to be self sufficient and capable (both diagnosing
> the issue for themselves, and reaching out to us as appropriate)
> ones in practice (e.g. I have NonStop folks in mind)?

>> The right sense of relative importance between efficiently running
>> the rest of Git by not bloting the main binary and making sure not
>> to ship Git that does not run unless "git bugreport" runs (which
>> makes sure that "bugreport" runs) is what you are missing, I
>> suspect.
>
> By that reasoning, `git bugreport` should not be included in core Git.

>> Another thing is that you are giving "git bugreport" too much weight
>> and too little credit to inexperienced users, by assuming that we
>> will never hear from them when "bugreport" is incapable to run for
>> them.  They will report, with or without "git bugreport", and the
>> more important thing you seem to be missing is that after the
>> initial contact it would become an interactive process---there is
>> no reason to prioritize to ensure that the initial contact has
>> everything that is needed to diagnose any issue without further
>> interaction.  "With my build, even 'bugreport' dumps core." is
>> perfectly good place to start.
>>
>> Besides, wouldn't the ones on platforms, on which "git bugreport"
>> may have trouble running, i.e. the ones on minority and exotic
>> platforms, tend to be self sufficient and capable (both diagnosing
>> the issue for themselves, and reaching out to us as appropriate)
>> ones in practice (e.g. I have NonStop folks in mind)?
>
> Yes, I can agree that inexperienced users will not give up and keep
> up the conversation until they see their problems fixed.

> Hi,
> 
> On Mon, 2 Mar 2020, Emily Shaffer wrote:
> 
> >  .gitignore                      |   1 +
> >  Documentation/git-bugreport.txt |  46 ++++++++++++++
> >  Makefile                        |   5 ++
> >  bugreport.c                     | 105 ++++++++++++++++++++++++++++++++
> >  command-list.txt                |   1 +
> >  strbuf.c                        |   4 ++
> >  strbuf.h                        |   1 +
> >  t/t0091-bugreport.sh            |  61 +++++++++++++++++++
> >  8 files changed, 224 insertions(+)
> >  create mode 100644 Documentation/git-bugreport.txt
> >  create mode 100644 bugreport.c
> >  create mode 100755 t/t0091-bugreport.sh
> 
> Hmm. I am still _quite_ convinced that this would be much better as a
> built-in. Remember, non-built-ins come with a footprint, and I do not
> necessarily think that you will want to spend 3MB on a `git-bugreport`
> executable when you could have it for a couple dozen kilobytes inside
> `git` instead.

> Builtin:
> + Don't have to call out-of-process to identify 'git version --build-options'
> + Better assurance that we aren't shipping a broken bugreport alongside a new
>   version
> - Binary bloat, possible startup time hit
> ? Libraries will behave identically to where the user is seeing issues
>   (This point is a possible pro but also a possible con; see similar point in
>   standalone list)

> Emily Shaffer <emilyshaffer@google.com> writes:
>
> > Builtin:
> > + Don't have to call out-of-process to identify 'git version --build-options'
> > + Better assurance that we aren't shipping a broken bugreport alongside a new
> >   version
> > - Binary bloat, possible startup time hit
> > ? Libraries will behave identically to where the user is seeing issues
> >   (This point is a possible pro but also a possible con; see similar point in
> >   standalone list)
>
>  - Makes it hard to pull in libraries that "bugreport" alone will
>    want to use in the future without negatively impacting the rest
>    of Git.

> >  - Time to editor for 'git bugreport' with the same setup as previous
>
> This is something we absolutely should totally ignore.  Even if the
> bulitin version takes 1 sec to spawn an editor while a standalone
> one took only 0.1 sec, if other criteria you listed that are more
> important favours the builtin solution, we should pick it.
>
> In any case, I think we've wasted enough time on this.  Let's see a
> minimum working version that won't break/affect the rest of Git and
> ship it standalone.

> On Thu, 19 Mar 2020, Junio C Hamano wrote:
> 
> > In any case, I think we've wasted enough time on this.  Let's see a
> > minimum working version that won't break/affect the rest of Git and
> > ship it standalone.
> 
> I still disagree with that, but it seems that no amount of arguments will
> convince Junio, and he's dead set on the standalone version.

> On Wed, Mar 04, 2020 at 10:35:04PM +0100, Johannes Schindelin wrote:
>
> > On Mon, 2 Mar 2020, Emily Shaffer wrote:
> >
> > >  .gitignore                      |   1 +
> > >  Documentation/git-bugreport.txt |  46 ++++++++++++++
> > >  Makefile                        |   5 ++
> > >  bugreport.c                     | 105 ++++++++++++++++++++++++++++++++
> > >  command-list.txt                |   1 +
> > >  strbuf.c                        |   4 ++
> > >  strbuf.h                        |   1 +
> > >  t/t0091-bugreport.sh            |  61 +++++++++++++++++++
> > >  8 files changed, 224 insertions(+)
> > >  create mode 100644 Documentation/git-bugreport.txt
> > >  create mode 100644 bugreport.c
> > >  create mode 100755 t/t0091-bugreport.sh
> >
> > Hmm. I am still _quite_ convinced that this would be much better as a
> > built-in. Remember, non-built-ins come with a footprint, and I do not
> > necessarily think that you will want to spend 3MB on a `git-bugreport`
> > executable when you could have it for a couple dozen kilobytes inside
> > `git` instead.
>
> This is the kind of stuff I really wanted to get straightened out by
> sending the smaller changeset, so I'm glad to be having this
> conversation (again, and hopefully for the last time). I read the
> replies to this mail, which I'm truncating as I think many of them are
> distracting rather than discussion-worthy, and have tried to summarize:
>
> Builtin:
> + Don't have to call out-of-process to identify 'git version --build-options'
> + Better assurance that we aren't shipping a broken bugreport alongside a new
>   version
> - Binary bloat, possible startup time hit
> ? Libraries will behave identically to where the user is seeing issues
>   (This point is a possible pro but also a possible con; see similar point in
>   standalone list)
>
> Standalone:
> + Could investigate libraries who aren't behaving the way they should.
>   (This seems like it'd be better addressed by regression tests, and if we're so
>   sure that git-bugreport is working with the info at hand correctly, why don't
>   we just write the git binary that way too?)

> Of course if I missed capturing something, please add/correct. Some of
> these concerns are quantifiable - binary size and overhead, for example
> - so I'm planning on doing some experiments in the coming days. I plan
> put together the handful of patches in the latest version of the topic
> in both standalone and builtin mode, and then gather the following info:
>
>  - Binary size of 'git'
>  - Binary size of 'git-bugreport' when applicable
>  - Time for "make" following clean
>  - Time for 'git status' on an identical real-world repo (I'll use ours,
>    without touching the repo state e.g. changing branch or fetching in
>    between)
>  - Time to editor for 'git bugreport' with the same setup as previous
>  - Peak memory footprint during 'git status'

> And of course, if there's more to compare that I can quantify, please
> let me know that too. I expect I'll be ready to run the experiments by
> Monday (taking some time off tomorrow) so there's time for me to hear
> about other concerns.
>
> I'll hold off on sharing my own preference until after we've got some
> benchmarking to look at, so I can understand the whole picture.

> Hi Emily,
> 
> On Thu, 19 Mar 2020, Emily Shaffer wrote:
> > I'll hold off on sharing my own preference until after we've got some
> > benchmarking to look at, so I can understand the whole picture.
> 
> Why? Why not strong-arm your preference? Junio and I are not shy doing the
> same, and those are _your_ patches. Junio is clearly not interested in the
> command at all, but you are clearly interested in it [*1*]. You should
> have more than just the final say over this.

> Emily Shaffer <emilyshaffer@google.com> writes:
>
> > This is the kind of stuff I really wanted to get straightened out by
> > sending the smaller changeset, so I'm glad to be having this
> > conversation (again, and hopefully for the last time).
>
> I actually have a suspicion that "git bugreport" that is spawned via
> "git" wrapper is a bad idea (in other words, /usr/bin/git-bug that
> is totally standalone may be better).

>
> The thing is, anything launched by "git" as its subcommand (be it
> standalone or builtin) sees an environment already modified by
> "git", so inspecting say "$PATH" from "git bugreport" (be it
> standalone or builtin) does not show what the end-user, who may be
> having trouble, actually has.  The mangling of $PATH alone happens
> to be simple and (we may think) we can easily reverse without losing
> bits, but there would probably other differences that we think is so
> subtle and insignificant, right now in this discussion without
> having actual end-user who is having trouble in front of us, that
> having "git" layer in between may hide from us.
>
> Having "git-bug" a totally separate tool, that does not even go
> through git.c::cmd_main(), would also allow (and force) us to treat
> "git" just like any system component on the end-users' system whose
> versions and configurations may affect the use of "git" by the end
> user.  The tool, instead of relying on git_version_string[] that is
> determined at the compile time, would ask "git version" just like
> the end-users would when asked by us "what version of git do you
> run?"  It also means that "git-bug" can be updated at different
> cadence and the mismatch of versions between it and "git" does not
> matter at all.
>
> Having said all that, after suggesting to make the tool even more
> distant from the remainder of the binary, quite honestly, I do not
> even want to see us wasting any more time on builtin/standalone at
> this point, and instead would like to see reports from the end-users
> produced by a lets-start-small version of the tool and measure how
> having the tool actually helps.
>
> Thanks.
>

>> I actually have a suspicion that "git bugreport" that is spawned via
>> "git" wrapper is a bad idea (in other words, /usr/bin/git-bug that
>> is totally standalone may be better).
>
> The obvious downside of `/usr/bin/git-bug`, of course, is that it has no
> way to provide accurate data regarding, say, the cURL version in use.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> I actually have a suspicion that "git bugreport" that is spawned via
> >> "git" wrapper is a bad idea (in other words, /usr/bin/git-bug that
> >> is totally standalone may be better).
> >
> > The obvious downside of `/usr/bin/git-bug`, of course, is that it has no
> > way to provide accurate data regarding, say, the cURL version in use.
>
> Sorry, but I do not see what's new in your argument this time.
>
> I thought we've already established that the best solution for the
> "accurate data regarding, say, the cURL version in use" is to use
> your earlier idea, i.e. give an interface to git-remote-curl so that
> "git bugreport" can ask it such details, because "git bugreport"
> that is known by git.c::cmd_main(), whether it is builtin or
> standalone, is *NOT* linked to the transport anyway.
>
> And the same interface can be used by an independent "git-bug", or
> even by the end user who is sitting at a terminal when asked by git
> developers "what version of curl library does your build link
> with?".

> To help pinpoint the source of a regression, it is useful to know some
> info about the compiler which the user's Git client was built with. By
> adding a generic get_compiler_info() in 'compat/' we can choose which
> relevant information to share per compiler; to get started, let's
> demonstrate the version of glibc if the user built with 'gcc'.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  Documentation/git-bugreport.txt |  1 +
>  bugreport.c                     |  6 ++++++
>  compat/compiler.h               | 38 +++++++++++++++++++++++++++++++++
>  3 files changed, 45 insertions(+)
>  create mode 100644 compat/compiler.h
> 
> diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
> index 17b0d14e8d..643d1b2884 100644
> --- a/Documentation/git-bugreport.txt
> +++ b/Documentation/git-bugreport.txt
> @@ -27,6 +27,7 @@ The following information is captured automatically:
>  
>   - 'git version --build-options'
>   - uname sysname, release, version, and machine strings
> + - Compiler-specific info string
>  
>  This tool is invoked via the typical Git setup process, which means that in some
>  cases, it might not be able to launch - for example, if a relevant config file
> diff --git a/bugreport.c b/bugreport.c
> index 857b4f712c..5073c901cd 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -4,6 +4,7 @@
>  #include "strbuf.h"
>  #include "time.h"
>  #include "help.h"
> +#include "compat/compiler.h"
>  
>  static void get_system_info(struct strbuf *sys_info)
>  {
> @@ -25,6 +26,11 @@ static void get_system_info(struct strbuf *sys_info)
>  			    uname_info.release,
>  			    uname_info.version,
>  			    uname_info.machine);
> +
> +	strbuf_addstr(sys_info, _("compiler info: "));
> +	get_compiler_info(sys_info);
> +	strbuf_addstr(sys_info, _("libc info: "));
> +	get_libc_info(sys_info);
>  }
>  
>  static const char * const bugreport_usage[] = {
> diff --git a/compat/compiler.h b/compat/compiler.h
> new file mode 100644
> index 0000000000..21f098e6a4
> --- /dev/null
> +++ b/compat/compiler.h
> @@ -0,0 +1,38 @@
> +#ifndef COMPILER_H
> +#define COMPILER_H
> +
> +#include "git-compat-util.h"
> +#include "strbuf.h"
> +
> +#ifdef __GLIBC__
> +#include <gnu/libc-version.h>
> +#endif
> +
> +static inline void get_compiler_info(struct strbuf *info)
> +{
> +	int len = info->len;
> +#ifdef __GNUC__
> +	strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__);
> +#endif

> +static inline void get_compiler_info(struct strbuf *info)
> +{
> + ...
> +	if (len == info->len)
> +		strbuf_addf(info, _("no compiler information available\n"));

> +}
> +
> +static inline void get_libc_info(struct strbuf *info)
> +{
> + ...
> +	if (len == info->len)
> +		strbuf_addf(info, _("no libc information available\n"));

> diff --git a/compat/compiler.h b/compat/compiler.h
> new file mode 100644
> index 0000000000..21f098e6a4
> --- /dev/null
> +++ b/compat/compiler.h
> @@ -0,0 +1,38 @@
> +#ifndef COMPILER_H
> +#define COMPILER_H
> +
> +#include "git-compat-util.h"
> +#include "strbuf.h"
> +
> +#ifdef __GLIBC__
> +#include <gnu/libc-version.h>
> +#endif
> +
> +static inline void get_compiler_info(struct strbuf *info)
> +{
> +	int len = info->len;
> +#ifdef __GNUC__
> +	strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__);
> +#endif
> +
> +#ifdef _MSC_VER
> +	strbuf_addf(info, "MSVC version: %s\n", _MSC_FULL_VER);

> Hi Emily,
> 
> On Mon, 2 Mar 2020, Emily Shaffer wrote:
> 
> > diff --git a/compat/compiler.h b/compat/compiler.h
> > new file mode 100644
> > index 0000000000..21f098e6a4
> > --- /dev/null
> > +++ b/compat/compiler.h
> > @@ -0,0 +1,38 @@
> > +#ifndef COMPILER_H
> > +#define COMPILER_H
> > +
> > +#include "git-compat-util.h"
> > +#include "strbuf.h"
> > +
> > +#ifdef __GLIBC__
> > +#include <gnu/libc-version.h>
> > +#endif
> > +
> > +static inline void get_compiler_info(struct strbuf *info)
> > +{
> > +	int len = info->len;
> > +#ifdef __GNUC__
> > +	strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__);
> > +#endif
> > +
> > +#ifdef _MSC_VER
> > +	strbuf_addf(info, "MSVC version: %s\n", _MSC_FULL_VER);
> 
> I need this to fix a crash in the Visual Studio build's test run:

> 
> -- snip --
> Subject: [PATCH] fixup??? bugreport: add compiler info
> 
> As documented at
> https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=vs-2019,
> `_MSC_FULL_VER` is an integer, not a string. Therefore this fix is
> needed to prevent a segmentation fault in the test t0091.1.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  compat/compiler.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/compat/compiler.h b/compat/compiler.h
> index ce6a7f6de9a..ac90fa051dd 100644
> --- a/compat/compiler.h
> +++ b/compat/compiler.h
> @@ -16,7 +16,8 @@ static inline void get_compiler_info(struct strbuf *info)
>  #endif
> 
>  #ifdef _MSC_VER
> -	strbuf_addf(info, "MSVC version: %s\n", _MSC_FULL_VER);
> +	strbuf_addf(info, "MSVC version: %02d.%02d.%05d\n",
> +		    _MSC_VER / 100, _MSC_VER % 100, _MSC_FULL_VER % 100000);
>  #endif
> 
>  	if (len == info->len)
> -- snap --
> 
> Could you squash that in, please?
> 
> BTW the two issues I reported in the earlier round are still problems.
> Could you squash my fixes, please? For your convenience, you can pluck all
> four of them right off of
> https://github.com/git-for-windows/git/commits/shears/pu

> 
> Just replace the `???` by `!` and you can even use `git rebase
> --autosquash`.
> 
> Ciao,
> Dscho
> 
> 
> > +#endif
> > +
> > +	if (len == info->len)
> > +		strbuf_addf(info, _("no compiler information available\n"));
> > +}
> > +
> > +static inline void get_libc_info(struct strbuf *info)
> > +{
> > +	int len = info->len;
> > +
> > +#ifdef __GLIBC__
> > +	strbuf_addf(info, "glibc: %s\n", gnu_get_libc_version());
> > +#endif
> > +
> > +	if (len == info->len)
> > +		strbuf_addf(info, _("no libc information available\n"));
> > +}
> > +
> > +#endif /* COMPILER_H */
> > --
> > 2.25.0.265.gbab2e86ba0-goog
> >
> >