[PATCH v13 0/5] git-bugreport with fixed VS build

[PATCH v13 0/5] git-bugreport with fixed VS build

[Emily Shaffer]

Two changes from the prior set attempted for 'next':

- https://lore.kernel.org/git/xmqqh7xk45l4.fsf@gitster.c.googlers.com
  squashed into "help: move list_config_help to builtin/help"
- Danh's SOB line changed to use the correct identity in "bugreport: add
  compiler info"

When generating this series I did base it on
dd/ci-swap-azure-pipelines-with-github-actions in order to get the
vs-build CI target; the results are here:
https://github.com/nasamuffin/git/actions/runs/80163844

Thanks.

Emily Shaffer (5):
  help: move list_config_help to builtin/help
  bugreport: add tool to generate debugging info
  bugreport: gather git version and build info
  bugreport: add uname info
  bugreport: add compiler info

 .gitignore                      |   2 +
 Documentation/git-bugreport.txt |  52 ++++++++++++
 Makefile                        |  18 +++-
 bugreport.c                     | 142 ++++++++++++++++++++++++++++++++
 builtin/help.c                  |  86 +++++++++++++++++++
 command-list.txt                |   1 +
 compat/compiler.h               |  41 +++++++++
 compat/vcbuild/README           |   4 +-
 config.mak.uname                |   6 +-
 generate-cmdlist.sh             |  19 -----
 generate-configlist.sh          |  21 +++++
 help.c                          | 131 +++++++----------------------
 help.h                          |   2 +-
 strbuf.c                        |   4 +
 strbuf.h                        |   1 +
 t/t0091-bugreport.sh            |  61 ++++++++++++++
 16 files changed, 460 insertions(+), 131 deletions(-)
 create mode 100644 Documentation/git-bugreport.txt
 create mode 100644 bugreport.c
 create mode 100644 compat/compiler.h
 create mode 100755 generate-configlist.sh
 create mode 100755 t/t0091-bugreport.sh

-- 
2.26.1.301.g55bc3eb7cb9-goog

[PATCH v13 1/5] help: move list_config_help to builtin/help

[Emily Shaffer]

Starting in 3ac68a93fd2, help.o began to depend on builtin/branch.o,
builtin/clean.o, and builtin/config.o. This meant that help.o was
unusable outside of the context of the main Git executable.

To make help.o usable by other commands again, move list_config_help()
into builtin/help.c (where it makes sense to assume other builtin libraries
are present).

When command-list.h is included but a member is not used, we start to
hear a compiler warning. Since the config list is generated in a fairly
different way than the command list, and since commands and config
options are semantically different, move the config list into its own
header and move the generator into its own script and build rule.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>

msvc: the bugreport topic depends on a generated config-list.h file

For reasons explained in 976aaedc (msvc: add a Makefile target to
pre-generate the Visual Studio solution, 2019-07-29), some build
artifacts we consider non-source files cannot be generated in the
Visual Studio environment, and we already have some Makefile tweaks
to help Visual Studio to use generated command-list.h header file.

As this topic starts to depend on another such generated header file,
config-list.h, let's do the same to it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 .gitignore             |  1 +
 Makefile               | 13 +++++--
 builtin/help.c         | 86 ++++++++++++++++++++++++++++++++++++++++++
 compat/vcbuild/README  |  4 +-
 config.mak.uname       |  6 +--
 generate-cmdlist.sh    | 19 ----------
 generate-configlist.sh | 21 +++++++++++
 help.c                 | 85 -----------------------------------------
 help.h                 |  1 -
 9 files changed, 123 insertions(+), 113 deletions(-)
 create mode 100755 generate-configlist.sh

diff --git a/.gitignore b/.gitignore
index 188bd1c3de..61bf5142a9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -188,6 +188,7 @@
 /gitweb/gitweb.cgi
 /gitweb/static/gitweb.js
 /gitweb/static/gitweb.min.*
+/config-list.h
 /command-list.h
 *.tar.gz
 *.dsc
diff --git a/Makefile b/Makefile
index ef1ff2228f..d4aff7f9b5 100644
--- a/Makefile
+++ b/Makefile
@@ -815,6 +815,7 @@ LIB_FILE = libgit.a
 XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
 
+GENERATED_H += config-list.h
 GENERATED_H += command-list.h
 
 LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
@@ -2133,7 +2134,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 
 help.sp help.s help.o: command-list.h
 
-builtin/help.sp builtin/help.s builtin/help.o: command-list.h GIT-PREFIX
+builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
@@ -2153,6 +2154,12 @@ $(BUILT_INS): git$X
 	ln -s $< $@ 2>/dev/null || \
 	cp $< $@
 
+config-list.h: generate-configlist.sh
+
+config-list.h:
+	$(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \
+		>$@+ && mv $@+ $@
+
 command-list.h: generate-cmdlist.sh command-list.txt
 
 command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt
@@ -2786,7 +2793,7 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
 .PHONY: sparse $(SP_OBJ)
 sparse: $(SP_OBJ)
 
-EXCEPT_HDRS := command-list.h unicode-width.h compat/% xdiff/%
+EXCEPT_HDRS := command-list.h config-list.h unicode-width.h compat/% xdiff/%
 ifndef GCRYPT_SHA256
 	EXCEPT_HDRS += sha256/gcrypt.h
 endif
@@ -2808,7 +2815,7 @@ hdr-check: $(HCO)
 style:
 	git clang-format --style file --diff --extensions c,h
 
-check: command-list.h
+check: config-list.h command-list.h
 	@if sparse; \
 	then \
 		echo >&2 "Use 'make sparse' instead"; \
diff --git a/builtin/help.c b/builtin/help.c
index e5590d7787..1c5f2b9255 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -8,6 +8,7 @@
 #include "parse-options.h"
 #include "run-command.h"
 #include "column.h"
+#include "config-list.h"
 #include "help.h"
 #include "alias.h"
 
@@ -62,6 +63,91 @@ static const char * const builtin_help_usage[] = {
 	NULL
 };
 
+struct slot_expansion {
+	const char *prefix;
+	const char *placeholder;
+	void (*fn)(struct string_list *list, const char *prefix);
+	int found;
+};
+
+static void list_config_help(int for_human)
+{
+	struct slot_expansion slot_expansions[] = {
+		{ "advice", "*", list_config_advices },
+		{ "color.branch", "<slot>", list_config_color_branch_slots },
+		{ "color.decorate", "<slot>", list_config_color_decorate_slots },
+		{ "color.diff", "<slot>", list_config_color_diff_slots },
+		{ "color.grep", "<slot>", list_config_color_grep_slots },
+		{ "color.interactive", "<slot>", list_config_color_interactive_slots },
+		{ "color.remote", "<slot>", list_config_color_sideband_slots },
+		{ "color.status", "<slot>", list_config_color_status_slots },
+		{ "fsck", "<msg-id>", list_config_fsck_msg_ids },
+		{ "receive.fsck", "<msg-id>", list_config_fsck_msg_ids },
+		{ NULL, NULL, NULL }
+	};
+	const char **p;
+	struct slot_expansion *e;
+	struct string_list keys = STRING_LIST_INIT_DUP;
+	int i;
+
+	for (p = config_name_list; *p; p++) {
+		const char *var = *p;
+		struct strbuf sb = STRBUF_INIT;
+
+		for (e = slot_expansions; e->prefix; e++) {
+
+			strbuf_reset(&sb);
+			strbuf_addf(&sb, "%s.%s", e->prefix, e->placeholder);
+			if (!strcasecmp(var, sb.buf)) {
+				e->fn(&keys, e->prefix);
+				e->found++;
+				break;
+			}
+		}
+		strbuf_release(&sb);
+		if (!e->prefix)
+			string_list_append(&keys, var);
+	}
+
+	for (e = slot_expansions; e->prefix; e++)
+		if (!e->found)
+			BUG("slot_expansion %s.%s is not used",
+			    e->prefix, e->placeholder);
+
+	string_list_sort(&keys);
+	for (i = 0; i < keys.nr; i++) {
+		const char *var = keys.items[i].string;
+		const char *wildcard, *tag, *cut;
+
+		if (for_human) {
+			puts(var);
+			continue;
+		}
+
+		wildcard = strchr(var, '*');
+		tag = strchr(var, '<');
+
+		if (!wildcard && !tag) {
+			puts(var);
+			continue;
+		}
+
+		if (wildcard && !tag)
+			cut = wildcard;
+		else if (!wildcard && tag)
+			cut = tag;
+		else
+			cut = wildcard < tag ? wildcard : tag;
+
+		/*
+		 * We may produce duplicates, but that's up to
+		 * git-completion.bash to handle
+		 */
+		printf("%.*s\n", (int)(cut - var), var);
+	}
+	string_list_clear(&keys, 0);
+}
+
 static enum help_format parse_help_format(const char *format)
 {
 	if (!strcmp(format, "man"))
diff --git a/compat/vcbuild/README b/compat/vcbuild/README
index 1b6dabf5a2..42292e7c09 100644
--- a/compat/vcbuild/README
+++ b/compat/vcbuild/README
@@ -92,8 +92,8 @@ The Steps of Build Git with VS2008
    the git operations.
 
 3. Inside Git's directory run the command:
-       make command-list.h
-   to generate the command-list.h file needed to compile git.
+       make command-list.h config-list.h
+   to generate the header file needed to compile git.
 
 4. Then either build Git with the GNU Make Makefile in the Git projects
    root
diff --git a/config.mak.uname b/config.mak.uname
index 0ab8e00938..f880cc2792 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -721,9 +721,9 @@ vcxproj:
 	 echo '</Project>') >git-remote-http/LinkOrCopyRemoteHttp.targets
 	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
+	# Add command-list.h and config-list.h
+	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 config-list.h command-list.h
+	git add -f config-list.h command-list.h
 
 	# Add scripts
 	rm -f perl/perl.mak
diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
index 71158f7d8b..45fecf8bdf 100755
--- a/generate-cmdlist.sh
+++ b/generate-cmdlist.sh
@@ -76,23 +76,6 @@ print_command_list () {
 	echo "};"
 }
 
-print_config_list () {
-	cat <<EOF
-static const char *config_name_list[] = {
-EOF
-	grep -h '^[a-zA-Z].*\..*::$' Documentation/*config.txt Documentation/config/*.txt |
-	sed '/deprecated/d; s/::$//; s/,  */\n/g' |
-	sort |
-	while read line
-	do
-		echo "	\"$line\","
-	done
-	cat <<EOF
-	NULL,
-};
-EOF
-}
-
 exclude_programs=
 while test "--exclude-program" = "$1"
 do
@@ -113,5 +96,3 @@ echo
 define_category_names "$1"
 echo
 print_command_list "$1"
-echo
-print_config_list
diff --git a/generate-configlist.sh b/generate-configlist.sh
new file mode 100755
index 0000000000..8692fe5cf4
--- /dev/null
+++ b/generate-configlist.sh
@@ -0,0 +1,21 @@
+#!/bin/sh
+
+echo "/* Automatically generated by generate-configlist.sh */"
+echo
+
+print_config_list () {
+	cat <<EOF
+static const char *config_name_list[] = {
+EOF
+	grep -h '^[a-zA-Z].*\..*::$' Documentation/*config.txt Documentation/config/*.txt |
+	sed '/deprecated/d; s/::$//; s/,  */\n/g' |
+	sort |
+	sed 's/^.*$/	"&",/'
+	cat <<EOF
+	NULL,
+};
+EOF
+}
+
+echo
+print_config_list
diff --git a/help.c b/help.c
index cf67624a94..a21487db77 100644
--- a/help.c
+++ b/help.c
@@ -407,91 +407,6 @@ void list_common_guides_help(void)
 	putchar('\n');
 }
 
-struct slot_expansion {
-	const char *prefix;
-	const char *placeholder;
-	void (*fn)(struct string_list *list, const char *prefix);
-	int found;
-};
-
-void list_config_help(int for_human)
-{
-	struct slot_expansion slot_expansions[] = {
-		{ "advice", "*", list_config_advices },
-		{ "color.branch", "<slot>", list_config_color_branch_slots },
-		{ "color.decorate", "<slot>", list_config_color_decorate_slots },
-		{ "color.diff", "<slot>", list_config_color_diff_slots },
-		{ "color.grep", "<slot>", list_config_color_grep_slots },
-		{ "color.interactive", "<slot>", list_config_color_interactive_slots },
-		{ "color.remote", "<slot>", list_config_color_sideband_slots },
-		{ "color.status", "<slot>", list_config_color_status_slots },
-		{ "fsck", "<msg-id>", list_config_fsck_msg_ids },
-		{ "receive.fsck", "<msg-id>", list_config_fsck_msg_ids },
-		{ NULL, NULL, NULL }
-	};
-	const char **p;
-	struct slot_expansion *e;
-	struct string_list keys = STRING_LIST_INIT_DUP;
-	int i;
-
-	for (p = config_name_list; *p; p++) {
-		const char *var = *p;
-		struct strbuf sb = STRBUF_INIT;
-
-		for (e = slot_expansions; e->prefix; e++) {
-
-			strbuf_reset(&sb);
-			strbuf_addf(&sb, "%s.%s", e->prefix, e->placeholder);
-			if (!strcasecmp(var, sb.buf)) {
-				e->fn(&keys, e->prefix);
-				e->found++;
-				break;
-			}
-		}
-		strbuf_release(&sb);
-		if (!e->prefix)
-			string_list_append(&keys, var);
-	}
-
-	for (e = slot_expansions; e->prefix; e++)
-		if (!e->found)
-			BUG("slot_expansion %s.%s is not used",
-			    e->prefix, e->placeholder);
-
-	string_list_sort(&keys);
-	for (i = 0; i < keys.nr; i++) {
-		const char *var = keys.items[i].string;
-		const char *wildcard, *tag, *cut;
-
-		if (for_human) {
-			puts(var);
-			continue;
-		}
-
-		wildcard = strchr(var, '*');
-		tag = strchr(var, '<');
-
-		if (!wildcard && !tag) {
-			puts(var);
-			continue;
-		}
-
-		if (wildcard && !tag)
-			cut = wildcard;
-		else if (!wildcard && tag)
-			cut = tag;
-		else
-			cut = wildcard < tag ? wildcard : tag;
-
-		/*
-		 * We may produce duplicates, but that's up to
-		 * git-completion.bash to handle
-		 */
-		printf("%.*s\n", (int)(cut - var), var);
-	}
-	string_list_clear(&keys, 0);
-}
-
 static int get_alias(const char *var, const char *value, void *data)
 {
 	struct string_list *list = data;
diff --git a/help.h b/help.h
index 7a455beeb7..9071894e8c 100644
--- a/help.h
+++ b/help.h
@@ -22,7 +22,6 @@ static inline void mput_char(char c, unsigned int num)
 void list_common_cmds_help(void);
 void list_all_cmds_help(void);
 void list_common_guides_help(void);
-void list_config_help(int for_human);
 
 void list_all_main_cmds(struct string_list *list);
 void list_all_other_cmds(struct string_list *list);
-- 
2.26.1.301.g55bc3eb7cb9-goog

Re: [PATCH v13 1/5] help: move list_config_help to builtin/help

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> Starting in 3ac68a93fd2, help.o began to depend on builtin/branch.o,
> builtin/clean.o, and builtin/config.o. This meant that help.o was
> unusable outside of the context of the main Git executable.
>
> To make help.o usable by other commands again, move list_config_help()
> into builtin/help.c (where it makes sense to assume other builtin libraries
> are present).
>
> When command-list.h is included but a member is not used, we start to
> hear a compiler warning. Since the config list is generated in a fairly
> different way than the command list, and since commands and config
> options are semantically different, move the config list into its own
> header and move the generator into its own script and build rule.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
>
> msvc: the bugreport topic depends on a generated config-list.h file
>
> For reasons explained in 976aaedc (msvc: add a Makefile target to
> pre-generate the Visual Studio solution, 2019-07-29), some build
> artifacts we consider non-source files cannot be generated in the
> Visual Studio environment, and we already have some Makefile tweaks
> to help Visual Studio to use generated command-list.h header file.
>
> As this topic starts to depend on another such generated header file,
> config-list.h, let's do the same to it.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

Forgot to proofread and edit the log message into reasonable shape
when you squashed two patches together?

I wonder if the "squash" action of "rebase -i" can be taught to
detect a mistake like this?

Re: [PATCH v13 1/5] help: move list_config_help to builtin/help

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Forgot to proofread and edit the log message into reasonable shape
> when you squashed two patches together?
>
> I wonder if the "squash" action of "rebase -i" can be taught to
> detect a mistake like this?

Without waiting for an improvement to "rebase -i" ;-), here is what
I came up with, with a minimum edit.

Subject: help: move list_config_help to builtin/help

Starting in 3ac68a93fd2, help.o began to depend on builtin/branch.o,
builtin/clean.o, and builtin/config.o. This meant that help.o was
unusable outside of the context of the main Git executable.

To make help.o usable by other commands again, move list_config_help()
into builtin/help.c (where it makes sense to assume other builtin libraries
are present).

When command-list.h is included but a member is not used, we start to
hear a compiler warning. Since the config list is generated in a fairly
different way than the command list, and since commands and config
options are semantically different, move the config list into its own
header and move the generator into its own script and build rule.

For reasons explained in 976aaedc (msvc: add a Makefile target to
pre-generate the Visual Studio solution, 2019-07-29), some build
artifacts we consider non-source files cannot be generated in the
Visual Studio environment, and we already have some Makefile tweaks
to help Visual Studio to use generated command-list.h header file.
Do the same to a new generated file, config-list.h, introduced by
this change.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Emily Shaffer <emilyshaffer@google.com>

Re: [PATCH v13 1/5] help: move list_config_help to builtin/help

[Emily Shaffer]

On Thu, Apr 16, 2020 at 03:28:24PM -0700, Junio C Hamano wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Forgot to proofread and edit the log message into reasonable shape
> > when you squashed two patches together?

Less "forgot" and more "wanted to preserve origin", but I'll avoid it
next time. Thanks.

> >
> > I wonder if the "squash" action of "rebase -i" can be taught to
> > detect a mistake like this?
> 
> Without waiting for an improvement to "rebase -i" ;-), here is what
> I came up with, with a minimum edit.

Sorry to make more work for you. Thanks, it looks fine.

> 
> 
> Starting in 3ac68a93fd2, help.o began to depend on builtin/branch.o,
> builtin/clean.o, and builtin/config.o. This meant that help.o was
> unusable outside of the context of the main Git executable.
> 
> To make help.o usable by other commands again, move list_config_help()
> into builtin/help.c (where it makes sense to assume other builtin libraries
> are present).
> 
> When command-list.h is included but a member is not used, we start to
> hear a compiler warning. Since the config list is generated in a fairly
> different way than the command list, and since commands and config
> options are semantically different, move the config list into its own
> header and move the generator into its own script and build rule.
> 
> For reasons explained in 976aaedc (msvc: add a Makefile target to
> pre-generate the Visual Studio solution, 2019-07-29), some build
> artifacts we consider non-source files cannot be generated in the
> Visual Studio environment, and we already have some Makefile tweaks
> to help Visual Studio to use generated command-list.h header file.
> Do the same to a new generated file, config-list.h, introduced by
> this change.
> 
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>

Re: [PATCH v13 1/5] help: move list_config_help to builtin/help

[Danh Doan]

On 2020-04-16 14:18:03-0700, Emily Shaffer <emilyshaffer@google.com> wrote:
> Starting in 3ac68a93fd2, help.o began to depend on builtin/branch.o,
> builtin/clean.o, and builtin/config.o. This meant that help.o was
> unusable outside of the context of the main Git executable.
> 
> To make help.o usable by other commands again, move list_config_help()
> into builtin/help.c (where it makes sense to assume other builtin libraries
> are present).
> 
> When command-list.h is included but a member is not used, we start to
> hear a compiler warning. Since the config list is generated in a fairly
> different way than the command list, and since commands and config
> options are semantically different, move the config list into its own
> header and move the generator into its own script and build rule.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> 
> msvc: the bugreport topic depends on a generated config-list.h file
> 
> For reasons explained in 976aaedc (msvc: add a Makefile target to
> pre-generate the Visual Studio solution, 2019-07-29), some build
> artifacts we consider non-source files cannot be generated in the
> Visual Studio environment, and we already have some Makefile tweaks
> to help Visual Studio to use generated command-list.h header file.
> 
> As this topic starts to depend on another such generated header file,
> config-list.h, let's do the same to it.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  .gitignore             |  1 +
>  Makefile               | 13 +++++--
>  builtin/help.c         | 86 ++++++++++++++++++++++++++++++++++++++++++
>  compat/vcbuild/README  |  4 +-
>  config.mak.uname       |  6 +--
>  generate-cmdlist.sh    | 19 ----------
>  generate-configlist.sh | 21 +++++++++++
>  help.c                 | 85 -----------------------------------------
>  help.h                 |  1 -
>  9 files changed, 123 insertions(+), 113 deletions(-)
>  create mode 100755 generate-configlist.sh
> 
> diff --git a/.gitignore b/.gitignore
> index 188bd1c3de..61bf5142a9 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -188,6 +188,7 @@
>  /gitweb/gitweb.cgi
>  /gitweb/static/gitweb.js
>  /gitweb/static/gitweb.min.*
> +/config-list.h
>  /command-list.h
>  *.tar.gz
>  *.dsc
> diff --git a/Makefile b/Makefile
> index ef1ff2228f..d4aff7f9b5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -815,6 +815,7 @@ LIB_FILE = libgit.a
>  XDIFF_LIB = xdiff/lib.a
>  VCSSVN_LIB = vcs-svn/lib.a
>  
> +GENERATED_H += config-list.h
>  GENERATED_H += command-list.h
>  
>  LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
> @@ -2133,7 +2134,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
>  
>  help.sp help.s help.o: command-list.h
>  
> -builtin/help.sp builtin/help.s builtin/help.o: command-list.h GIT-PREFIX
> +builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
>  builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
>  	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
>  	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
> @@ -2153,6 +2154,12 @@ $(BUILT_INS): git$X
>  	ln -s $< $@ 2>/dev/null || \
>  	cp $< $@
>  
> +config-list.h: generate-configlist.sh
> +
> +config-list.h:
> +	$(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \
> +		>$@+ && mv $@+ $@
> +
>  command-list.h: generate-cmdlist.sh command-list.txt
>  
>  command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt
> @@ -2786,7 +2793,7 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>  .PHONY: sparse $(SP_OBJ)
>  sparse: $(SP_OBJ)
>  
> -EXCEPT_HDRS := command-list.h unicode-width.h compat/% xdiff/%
> +EXCEPT_HDRS := command-list.h config-list.h unicode-width.h compat/% xdiff/%
>  ifndef GCRYPT_SHA256
>  	EXCEPT_HDRS += sha256/gcrypt.h
>  endif
> @@ -2808,7 +2815,7 @@ hdr-check: $(HCO)
>  style:
>  	git clang-format --style file --diff --extensions c,h
>  
> -check: command-list.h
> +check: config-list.h command-list.h
>  	@if sparse; \
>  	then \
>  		echo >&2 "Use 'make sparse' instead"; \
> diff --git a/builtin/help.c b/builtin/help.c
> index e5590d7787..1c5f2b9255 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -8,6 +8,7 @@
>  #include "parse-options.h"
>  #include "run-command.h"
>  #include "column.h"
> +#include "config-list.h"
>  #include "help.h"
>  #include "alias.h"
>  
> @@ -62,6 +63,91 @@ static const char * const builtin_help_usage[] = {
>  	NULL
>  };
>  
> +struct slot_expansion {
> +	const char *prefix;
> +	const char *placeholder;
> +	void (*fn)(struct string_list *list, const char *prefix);
> +	int found;
> +};
> +
> +static void list_config_help(int for_human)
> +{
> +	struct slot_expansion slot_expansions[] = {
> +		{ "advice", "*", list_config_advices },
> +		{ "color.branch", "<slot>", list_config_color_branch_slots },
> +		{ "color.decorate", "<slot>", list_config_color_decorate_slots },
> +		{ "color.diff", "<slot>", list_config_color_diff_slots },
> +		{ "color.grep", "<slot>", list_config_color_grep_slots },
> +		{ "color.interactive", "<slot>", list_config_color_interactive_slots },
> +		{ "color.remote", "<slot>", list_config_color_sideband_slots },
> +		{ "color.status", "<slot>", list_config_color_status_slots },
> +		{ "fsck", "<msg-id>", list_config_fsck_msg_ids },
> +		{ "receive.fsck", "<msg-id>", list_config_fsck_msg_ids },
> +		{ NULL, NULL, NULL }
> +	};
> +	const char **p;
> +	struct slot_expansion *e;
> +	struct string_list keys = STRING_LIST_INIT_DUP;
> +	int i;
> +
> +	for (p = config_name_list; *p; p++) {
> +		const char *var = *p;
> +		struct strbuf sb = STRBUF_INIT;
> +
> +		for (e = slot_expansions; e->prefix; e++) {
> +
> +			strbuf_reset(&sb);
> +			strbuf_addf(&sb, "%s.%s", e->prefix, e->placeholder);
> +			if (!strcasecmp(var, sb.buf)) {
> +				e->fn(&keys, e->prefix);
> +				e->found++;
> +				break;
> +			}
> +		}
> +		strbuf_release(&sb);
> +		if (!e->prefix)
> +			string_list_append(&keys, var);
> +	}
> +
> +	for (e = slot_expansions; e->prefix; e++)
> +		if (!e->found)
> +			BUG("slot_expansion %s.%s is not used",
> +			    e->prefix, e->placeholder);
> +
> +	string_list_sort(&keys);
> +	for (i = 0; i < keys.nr; i++) {
> +		const char *var = keys.items[i].string;
> +		const char *wildcard, *tag, *cut;
> +
> +		if (for_human) {
> +			puts(var);
> +			continue;
> +		}
> +
> +		wildcard = strchr(var, '*');
> +		tag = strchr(var, '<');
> +
> +		if (!wildcard && !tag) {
> +			puts(var);
> +			continue;
> +		}
> +
> +		if (wildcard && !tag)
> +			cut = wildcard;
> +		else if (!wildcard && tag)
> +			cut = tag;
> +		else
> +			cut = wildcard < tag ? wildcard : tag;
> +
> +		/*
> +		 * We may produce duplicates, but that's up to
> +		 * git-completion.bash to handle
> +		 */
> +		printf("%.*s\n", (int)(cut - var), var);
> +	}
> +	string_list_clear(&keys, 0);
> +}
> +
>  static enum help_format parse_help_format(const char *format)
>  {
>  	if (!strcmp(format, "man"))
> diff --git a/compat/vcbuild/README b/compat/vcbuild/README
> index 1b6dabf5a2..42292e7c09 100644
> --- a/compat/vcbuild/README
> +++ b/compat/vcbuild/README
> @@ -92,8 +92,8 @@ The Steps of Build Git with VS2008
>     the git operations.
>  
>  3. Inside Git's directory run the command:
> -       make command-list.h
> -   to generate the command-list.h file needed to compile git.
> +       make command-list.h config-list.h
> +   to generate the header file needed to compile git.
>  
>  4. Then either build Git with the GNU Make Makefile in the Git projects
>     root
> diff --git a/config.mak.uname b/config.mak.uname
> index 0ab8e00938..f880cc2792 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -721,9 +721,9 @@ vcxproj:
>  	 echo '</Project>') >git-remote-http/LinkOrCopyRemoteHttp.targets
>  	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
> +	# Add command-list.h and config-list.h
> +	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 config-list.h command-list.h

In <nycvar.QRO.7.76.6.2004161406480.46@tvgsbejvaqbjf.bet>,
Dscho suggested to squash his patch instead:

https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002261649550.46@tvgsbejvaqbjf.bet/

> +	git add -f config-list.h command-list.h
>  
>  	# Add scripts
>  	rm -f perl/perl.mak
> diff --git a/generate-cmdlist.sh b/generate-cmdlist.sh
> index 71158f7d8b..45fecf8bdf 100755
> --- a/generate-cmdlist.sh
> +++ b/generate-cmdlist.sh
> @@ -76,23 +76,6 @@ print_command_list () {
>  	echo "};"
>  }
>  
> -print_config_list () {
> -	cat <<EOF
> -static const char *config_name_list[] = {
> -EOF
> -	grep -h '^[a-zA-Z].*\..*::$' Documentation/*config.txt Documentation/config/*.txt |
> -	sed '/deprecated/d; s/::$//; s/,  */\n/g' |
> -	sort |
> -	while read line
> -	do
> -		echo "	\"$line\","
> -	done
> -	cat <<EOF
> -	NULL,
> -};
> -EOF
> -}
> -
>  exclude_programs=
>  while test "--exclude-program" = "$1"
>  do
> @@ -113,5 +96,3 @@ echo
>  define_category_names "$1"
>  echo
>  print_command_list "$1"
> -echo
> -print_config_list
> diff --git a/generate-configlist.sh b/generate-configlist.sh
> new file mode 100755
> index 0000000000..8692fe5cf4
> --- /dev/null
> +++ b/generate-configlist.sh
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +
> +echo "/* Automatically generated by generate-configlist.sh */"
> +echo
> +
> +print_config_list () {
> +	cat <<EOF
> +static const char *config_name_list[] = {
> +EOF
> +	grep -h '^[a-zA-Z].*\..*::$' Documentation/*config.txt Documentation/config/*.txt |
> +	sed '/deprecated/d; s/::$//; s/,  */\n/g' |
> +	sort |
> +	sed 's/^.*$/	"&",/'
> +	cat <<EOF
> +	NULL,
> +};
> +EOF
> +}
> +
> +echo
> +print_config_list
> diff --git a/help.c b/help.c
> index cf67624a94..a21487db77 100644
> --- a/help.c
> +++ b/help.c
> @@ -407,91 +407,6 @@ void list_common_guides_help(void)
>  	putchar('\n');
>  }
>  
> -struct slot_expansion {
> -	const char *prefix;
> -	const char *placeholder;
> -	void (*fn)(struct string_list *list, const char *prefix);
> -	int found;
> -};
> -
> -void list_config_help(int for_human)
> -{
> -	struct slot_expansion slot_expansions[] = {
> -		{ "advice", "*", list_config_advices },
> -		{ "color.branch", "<slot>", list_config_color_branch_slots },
> -		{ "color.decorate", "<slot>", list_config_color_decorate_slots },
> -		{ "color.diff", "<slot>", list_config_color_diff_slots },
> -		{ "color.grep", "<slot>", list_config_color_grep_slots },
> -		{ "color.interactive", "<slot>", list_config_color_interactive_slots },
> -		{ "color.remote", "<slot>", list_config_color_sideband_slots },
> -		{ "color.status", "<slot>", list_config_color_status_slots },
> -		{ "fsck", "<msg-id>", list_config_fsck_msg_ids },
> -		{ "receive.fsck", "<msg-id>", list_config_fsck_msg_ids },
> -		{ NULL, NULL, NULL }
> -	};
> -	const char **p;
> -	struct slot_expansion *e;
> -	struct string_list keys = STRING_LIST_INIT_DUP;
> -	int i;
> -
> -	for (p = config_name_list; *p; p++) {
> -		const char *var = *p;
> -		struct strbuf sb = STRBUF_INIT;
> -
> -		for (e = slot_expansions; e->prefix; e++) {
> -
> -			strbuf_reset(&sb);
> -			strbuf_addf(&sb, "%s.%s", e->prefix, e->placeholder);
> -			if (!strcasecmp(var, sb.buf)) {
> -				e->fn(&keys, e->prefix);
> -				e->found++;
> -				break;
> -			}
> -		}
> -		strbuf_release(&sb);
> -		if (!e->prefix)
> -			string_list_append(&keys, var);
> -	}
> -
> -	for (e = slot_expansions; e->prefix; e++)
> -		if (!e->found)
> -			BUG("slot_expansion %s.%s is not used",
> -			    e->prefix, e->placeholder);
> -
> -	string_list_sort(&keys);
> -	for (i = 0; i < keys.nr; i++) {
> -		const char *var = keys.items[i].string;
> -		const char *wildcard, *tag, *cut;
> -
> -		if (for_human) {
> -			puts(var);
> -			continue;
> -		}
> -
> -		wildcard = strchr(var, '*');
> -		tag = strchr(var, '<');
> -
> -		if (!wildcard && !tag) {
> -			puts(var);
> -			continue;
> -		}
> -
> -		if (wildcard && !tag)
> -			cut = wildcard;
> -		else if (!wildcard && tag)
> -			cut = tag;
> -		else
> -			cut = wildcard < tag ? wildcard : tag;
> -
> -		/*
> -		 * We may produce duplicates, but that's up to
> -		 * git-completion.bash to handle
> -		 */
> -		printf("%.*s\n", (int)(cut - var), var);
> -	}
> -	string_list_clear(&keys, 0);
> -}
> -
>  static int get_alias(const char *var, const char *value, void *data)
>  {
>  	struct string_list *list = data;
> diff --git a/help.h b/help.h
> index 7a455beeb7..9071894e8c 100644
> --- a/help.h
> +++ b/help.h
> @@ -22,7 +22,6 @@ static inline void mput_char(char c, unsigned int num)
>  void list_common_cmds_help(void);
>  void list_all_cmds_help(void);
>  void list_common_guides_help(void);
> -void list_config_help(int for_human);
>  
>  void list_all_main_cmds(struct string_list *list);
>  void list_all_other_cmds(struct string_list *list);
> -- 
> 2.26.1.301.g55bc3eb7cb9-goog
> 

-- 
Danh

Re: [PATCH v13 1/5] help: move list_config_help to builtin/help

[Danh Doan]

On 2020-04-17 09:04:26+0700, Danh Doan <congdanhqx@gmail.com> wrote:
> On 2020-04-16 14:18:03-0700, Emily Shaffer <emilyshaffer@google.com> wrote:
> > diff --git a/config.mak.uname b/config.mak.uname
> > index 0ab8e00938..f880cc2792 100644
> > --- a/config.mak.uname
> > +++ b/config.mak.uname
> > @@ -721,9 +721,9 @@ vcxproj:
> >  	 echo '</Project>') >git-remote-http/LinkOrCopyRemoteHttp.targets
> >  	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
> > +	# Add command-list.h and config-list.h
> > +	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 config-list.h command-list.h
> 
> In <nycvar.QRO.7.76.6.2004161406480.46@tvgsbejvaqbjf.bet>,
> Dscho suggested to squash his patch instead:
> 
> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002261649550.46@tvgsbejvaqbjf.bet/

Sorry, I'm an idiot, please ignore my reply.
His patch is used for old series.


-- 
Danh

[PATCH] Makefile: add missing dependencies of 'config-list.h'

[SZEDER Gábor]

We auto-generate the list of supported configuration variables from
'Documentation/config/*.txt', and that list used to be created by the
'generate-cmdlist.sh' helper script and stored in the 'command-list.h'
header.  Commit 709df95b78 (help: move list_config_help to
builtin/help, 2020-04-16) extracted this into a dedicated
'generate-configlist.sh' script and 'config-list.h' header, and added
a new target in the 'Makefile' as well, but while doing so it forgot
to extract the dependencies of the latter.  Consequently, since then
'config-list.h' is not re-generated when 'Documentation/config/*.txt'
is updated, while 'command-list.h' is re-generated unnecessarily:

  $ touch Documentation/config/log.txt
  $ make -j4
      GEN command-list.h
      CC help.o
      AR libgit.a

Fix this and list all config-related documentation files as
dependencies of 'config-list.h' and remove them from the dependencies
of 'command-list.h'.

  $ touch Documentation/config/log.txt
  $ make
      GEN config-list.h
      CC builtin/help.o
      LINK git

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 5a022367d4..2c41f125e0 100644
--- a/Makefile
+++ b/Makefile
@@ -2151,13 +2151,13 @@ $(BUILT_INS): git$X
 
 config-list.h: generate-configlist.sh
 
-config-list.h:
+config-list.h: Documentation/*config.txt Documentation/config/*.txt
 	$(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \
 		>$@+ && mv $@+ $@
 
 command-list.h: generate-cmdlist.sh command-list.txt
 
-command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt
+command-list.h: $(wildcard Documentation/git*.txt)
 	$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \
 		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
 		command-list.txt >$@+ && mv $@+ $@
-- 
2.31.0.346.g7485d9830f

Re: [PATCH] Makefile: add missing dependencies of 'config-list.h'

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 08 2021, SZEDER Gábor wrote:

> We auto-generate the list of supported configuration variables from
> 'Documentation/config/*.txt', and that list used to be created by the
> 'generate-cmdlist.sh' helper script and stored in the 'command-list.h'
> header.  Commit 709df95b78 (help: move list_config_help to
> builtin/help, 2020-04-16) extracted this into a dedicated
> 'generate-configlist.sh' script and 'config-list.h' header, and added
> a new target in the 'Makefile' as well, but while doing so it forgot
> to extract the dependencies of the latter.  Consequently, since then
> 'config-list.h' is not re-generated when 'Documentation/config/*.txt'
> is updated, while 'command-list.h' is re-generated unnecessarily:
>
>   $ touch Documentation/config/log.txt
>   $ make -j4
>       GEN command-list.h
>       CC help.o
>       AR libgit.a
>
> Fix this and list all config-related documentation files as
> dependencies of 'config-list.h' and remove them from the dependencies
> of 'command-list.h'.
>
>   $ touch Documentation/config/log.txt
>   $ make
>       GEN config-list.h
>       CC builtin/help.o
>       LINK git
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 5a022367d4..2c41f125e0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2151,13 +2151,13 @@ $(BUILT_INS): git$X
>  
>  config-list.h: generate-configlist.sh
>  
> -config-list.h:
> +config-list.h: Documentation/*config.txt Documentation/config/*.txt
>  	$(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \
>  		>$@+ && mv $@+ $@
>  
>  command-list.h: generate-cmdlist.sh command-list.txt
>  
> -command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt
> +command-list.h: $(wildcard Documentation/git*.txt)
>  	$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \
>  		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
>  		command-list.txt >$@+ && mv $@+ $@

This change makes sense.

I have a not-yet-submitted patch series where I added some more
config/*/*.txt that wouldn't be caught by this rule, I'd updated the
Documentation/Makefile, but missed this part in the top-level Makefile.

So a relation question: Does anyone actually prefer this state of
affairs of having a Makefile, Documentation/Makefile, t/Makefile
t/perf/Makefile and template/Makefile?

It seems to me with ever-closer coupling between them that it's getting
to be more of a hassle to manage state between them than it would be to
just move them all into one big Makefile.

Re: [PATCH] Makefile: add missing dependencies of 'config-list.h'

[Jeff King]

On Fri, Apr 09, 2021 at 12:08:23AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > -config-list.h:
> > +config-list.h: Documentation/*config.txt Documentation/config/*.txt
> >  	$(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \
> >  		>$@+ && mv $@+ $@
> >  
> >  command-list.h: generate-cmdlist.sh command-list.txt
> >  
> > -command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt
> > +command-list.h: $(wildcard Documentation/git*.txt)
> >  	$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \
> >  		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
> >  		command-list.txt >$@+ && mv $@+ $@
> 
> This change makes sense.

I agree it looks like it's moving in the right direction, but I am
slightly puzzled by the existing code. Why do we need to use $(wildcard)
for git*.txt, but not for the others?

> I have a not-yet-submitted patch series where I added some more
> config/*/*.txt that wouldn't be caught by this rule, I'd updated the
> Documentation/Makefile, but missed this part in the top-level Makefile.
> 
> So a relation question: Does anyone actually prefer this state of
> affairs of having a Makefile, Documentation/Makefile, t/Makefile
> t/perf/Makefile and template/Makefile?
> 
> It seems to me with ever-closer coupling between them that it's getting
> to be more of a hassle to manage state between them than it would be to
> just move them all into one big Makefile.

Yes, I'm generally a fan of avoiding recursive make when we can. I think
the caveats are:

  - it would be nice to continue to have stub Makefiles in
    sub-directories that trigger the main one (so "cd t && make"
    continues to work, for example).

  - we may need some cleanup of parts of the top-level Makefile which
    are triggered without dependencies (e.g., I think we unconditionally
    run some scripts to compute GIT_VERSION in the top-level; this is
    already a bit wasteful, but may get even more so as we add more
    rules from sub-directories).

Mostly my argument against it (and why I haven't purused it) would be:
it sounds like a lot of work and risk of regression, and the current
system seems pretty fine in practice.

-Peff

Re: [PATCH] Makefile: add missing dependencies of 'config-list.h'

[SZEDER Gábor]

On Thu, Apr 08, 2021 at 07:40:41PM -0400, Jeff King wrote:
> On Fri, Apr 09, 2021 at 12:08:23AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > > -config-list.h:
> > > +config-list.h: Documentation/*config.txt Documentation/config/*.txt
> > >  	$(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \
> > >  		>$@+ && mv $@+ $@
> > >  
> > >  command-list.h: generate-cmdlist.sh command-list.txt
> > >  
> > > -command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt
> > > +command-list.h: $(wildcard Documentation/git*.txt)
> > >  	$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \
> > >  		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
> > >  		command-list.txt >$@+ && mv $@+ $@
> > 
> > This change makes sense.
> 
> I agree it looks like it's moving in the right direction, but I am
> slightly puzzled by the existing code. Why do we need to use $(wildcard)
> for git*.txt, but not for the others?

We don't need $(wildcard) for git*.txt either, because 'make' expands
wildcards in prerequisites, see e.g.:

  https://www.gnu.org/software/make/manual/html_node/Wildcard-Examples.html


On a related note: all config variables are now listed in
Documentation/config/*.txt; Documentation/*config.txt doesn't contain
any, so that could be removed.

Re: [PATCH] Makefile: add missing dependencies of 'config-list.h'

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Thu, Apr 08, 2021 at 07:40:41PM -0400, Jeff King wrote:
>> On Fri, Apr 09, 2021 at 12:08:23AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> 
>> > > -config-list.h:
>> > > +config-list.h: Documentation/*config.txt Documentation/config/*.txt
>> > >  	$(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \
>> > >  		>$@+ && mv $@+ $@
>> > >  
>> > >  command-list.h: generate-cmdlist.sh command-list.txt
>> > >  
>> > > -command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt
>> > > +command-list.h: $(wildcard Documentation/git*.txt)
>> > >  	$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \
>> > >  		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
>> > >  		command-list.txt >$@+ && mv $@+ $@
>> > 
>> > This change makes sense.
>> 
>> I agree it looks like it's moving in the right direction, but I am
>> slightly puzzled by the existing code. Why do we need to use $(wildcard)
>> for git*.txt, but not for the others?
>
> We don't need $(wildcard) for git*.txt either, because 'make' expands
> wildcards in prerequisites, see e.g.:
>
>   https://www.gnu.org/software/make/manual/html_node/Wildcard-Examples.html
>
>
> On a related note: all config variables are now listed in
> Documentation/config/*.txt; Documentation/*config.txt doesn't contain
> any, so that could be removed.

Is it OK for me to keep expecting an update to the patch happen soon?

Thanks.

Re: [PATCH] Makefile: add missing dependencies of 'config-list.h'

[SZEDER Gábor]

On Fri, Apr 16, 2021 at 12:03:54PM -0700, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > On Thu, Apr 08, 2021 at 07:40:41PM -0400, Jeff King wrote:
> >> On Fri, Apr 09, 2021 at 12:08:23AM +0200, Ævar Arnfjörð Bjarmason wrote:
> >> 
> >> > > -config-list.h:
> >> > > +config-list.h: Documentation/*config.txt Documentation/config/*.txt
> >> > >  	$(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \
> >> > >  		>$@+ && mv $@+ $@
> >> > >  
> >> > >  command-list.h: generate-cmdlist.sh command-list.txt
> >> > >  
> >> > > -command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt
> >> > > +command-list.h: $(wildcard Documentation/git*.txt)
> >> > >  	$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \
> >> > >  		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
> >> > >  		command-list.txt >$@+ && mv $@+ $@
> >> > 
> >> > This change makes sense.
> >> 
> >> I agree it looks like it's moving in the right direction, but I am
> >> slightly puzzled by the existing code. Why do we need to use $(wildcard)
> >> for git*.txt, but not for the others?
> >
> > We don't need $(wildcard) for git*.txt either, because 'make' expands
> > wildcards in prerequisites, see e.g.:
> >
> >   https://www.gnu.org/software/make/manual/html_node/Wildcard-Examples.html
> >
> >
> > On a related note: all config variables are now listed in
> > Documentation/config/*.txt; Documentation/*config.txt doesn't contain
> > any, so that could be removed.
> 
> Is it OK for me to keep expecting an update to the patch happen soon?

No, I think this is a good bugfix patch that stands on its own, and
further cleanups could be done independently on top and should not
block this patch from being merged to master.

I'm inclined to think that this should be merged and then
'so/log-diff-merge' should be queued on top because of the subtle
interaction between this bug, the new config variable and the
completion tests.

Re: [PATCH] Makefile: add missing dependencies of 'config-list.h'

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Fri, Apr 16, 2021 at 12:03:54PM -0700, Junio C Hamano wrote:
>> SZEDER Gábor <szeder.dev@gmail.com> writes:
>> 
>> > On Thu, Apr 08, 2021 at 07:40:41PM -0400, Jeff King wrote:
>> ...
>> >> I agree it looks like it's moving in the right direction, but I am
>> >> slightly puzzled by the existing code. Why do we need to use $(wildcard)
>> >> for git*.txt, but not for the others?
>> >
>> > We don't need $(wildcard) for git*.txt either, because 'make' expands
>> > wildcards in prerequisites, see e.g.:
>> >
>> >   https://www.gnu.org/software/make/manual/html_node/Wildcard-Examples.html
>> >
>> >
>> > On a related note: all config variables are now listed in
>> > Documentation/config/*.txt; Documentation/*config.txt doesn't contain
>> > any, so that could be removed.
>> 
>> Is it OK for me to keep expecting an update to the patch happen soon?
>
> No, I think this is a good bugfix patch that stands on its own, and
> further cleanups could be done independently on top and should not
> block this patch from being merged to master.

That's fair.

Thanks.

Re: [PATCH] Makefile: add missing dependencies of 'config-list.h'

[Ævar Arnfjörð Bjarmason]

On Fri, Apr 09 2021, Jeff King wrote:

> On Fri, Apr 09, 2021 at 12:08:23AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > -config-list.h:
>> > +config-list.h: Documentation/*config.txt Documentation/config/*.txt
>> >  	$(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \
>> >  		>$@+ && mv $@+ $@
>> >  
>> >  command-list.h: generate-cmdlist.sh command-list.txt
>> >  
>> > -command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Documentation/config/*.txt
>> > +command-list.h: $(wildcard Documentation/git*.txt)
>> >  	$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \
>> >  		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
>> >  		command-list.txt >$@+ && mv $@+ $@
>> 
>> This change makes sense.
>
> I agree it looks like it's moving in the right direction, but I am
> slightly puzzled by the existing code. Why do we need to use $(wildcard)
> for git*.txt, but not for the others?
>
>> I have a not-yet-submitted patch series where I added some more
>> config/*/*.txt that wouldn't be caught by this rule, I'd updated the
>> Documentation/Makefile, but missed this part in the top-level Makefile.
>> 
>> So a relation question: Does anyone actually prefer this state of
>> affairs of having a Makefile, Documentation/Makefile, t/Makefile
>> t/perf/Makefile and template/Makefile?
>> 
>> It seems to me with ever-closer coupling between them that it's getting
>> to be more of a hassle to manage state between them than it would be to
>> just move them all into one big Makefile.
>
> Yes, I'm generally a fan of avoiding recursive make when we can. I think
> the caveats are:
>
>   - it would be nice to continue to have stub Makefiles in
>     sub-directories that trigger the main one (so "cd t && make"
>     continues to work, for example).

Yeah, we should definitely keep those in place. I also wonder if various
rules for local wildcards will be more complex when they need to reach
into subdirectories.

>   - we may need some cleanup of parts of the top-level Makefile which
>     are triggered without dependencies (e.g., I think we unconditionally
>     run some scripts to compute GIT_VERSION in the top-level; this is
>     already a bit wasteful, but may get even more so as we add more
>     rules from sub-directories).
>
> Mostly my argument against it (and why I haven't purused it) would be:
> it sounds like a lot of work and risk of regression, and the current
> system seems pretty fine in practice.

One edge case I discovered the other day but didn't bother debugging
much was make at the top-level failing because "doc.dep" in
Documentation/Makefile uses this pattern:

    rm x &&
    script >x

Which would normally work in one Makefile, but in this case two rules in
the top-level called unrelated "make -C Documentation [...]", so both of
those sub-processes end up needing to generate the doc.dep, and they
race each other.

Another thing fixed (or, worked around) with a wider application of [1].

1. https://lore.kernel.org/git/patch-3.6-96e2338ed8e-20210329T161723Z-avarab@gmail.com/

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

[Emily Shaffer]

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>
---
 .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 61bf5142a9..ee509a2ad2 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 d4aff7f9b5..b0a9073788 100644
--- a/Makefile
+++ b/Makefile
@@ -680,6 +680,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
@@ -2462,6 +2463,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..f6f53a5e8e
--- /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 = "%Y-%m-%d-%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 bb0065ccaf..3bfcaababb 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -554,6 +554,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 ce8e49c0b2..801a3694ec 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -468,6 +468,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..2e73658a5c
--- /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' '
+	test_when_finished rm git-bugreport-check-headers.txt &&
+	git bugreport -s check-headers &&
+	check_all_headers_populated <git-bugreport-check-headers.txt
+'
+
+test_expect_success 'dies if file with same name as report already exists' '
+	test_when_finished rm git-bugreport-duplicate.txt &&
+	>>git-bugreport-duplicate.txt &&
+	test_must_fail git bugreport --suffix duplicate
+'
+
+test_expect_success '--output-directory puts the report in the provided dir' '
+	test_when_finished rm -fr foo/ &&
+	git bugreport -o foo/ &&
+	test_path_is_file foo/git-bugreport-*
+'
+
+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' '
+	test_when_finished rm non-repo/git-bugreport-* &&
+	nongit git bugreport
+'
+
+test_expect_success 'can create leading directories outside of a git dir' '
+	test_when_finished rm -fr foo/bar/baz &&
+	nongit git bugreport -o foo/bar/baz
+'
+
+
+test_done
-- 
2.26.1.301.g55bc3eb7cb9-goog

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

[SZEDER Gábor]

On Thu, Apr 16, 2020 at 02:18:04PM -0700, Emily Shaffer wrote:
> 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>
> ---

> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> new file mode 100755
> index 0000000000..2e73658a5c
> --- /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 () {

I'm afraid that this helper function doesn't do what it was supposed
to.

> +	while read -r line

It iterates through each line of stdin, which is a file written by
'git bugreport'.

> +	do
> +		if test "$(grep "$HEADER_PATTERN" "$line")"

This first tries to find a match in the _file_ called "$line", which never
exists, resulting in trace output:

  + check_all_headers_populated
  + read -r line
  + grep ^\[.*\]$ Thank you for filling out a Git bug report!
  grep: Thank you for filling out a Git bug report!: No such file or directory
  + test 
  + read -r line
  + grep ^\[.*\]$ Please answer the following questions to help us understand your issue.
  grep: Please answer the following questions to help us understand your issue.: No such file or directory
  + test
  + read -r line
  + grep ^\[.*\]$
  grep: : No such file or directory
  [...]

Then, since 'grep' doesn't print any matches to its stdout, it invokes

  test ""

which always returns non-zero, so that if condition is never fulfilled.

On first sight I thought that simply changing that 'grep' invocation
to something like:

  $(printf "%s\n" "$line" | grep "$HEADER_PATTERN")

would be sufficient to fix it, but then the first test failed... and
I'm not sure that I understand what this was supposed to check in the
first place.

> +		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' '
> +	test_when_finished rm git-bugreport-check-headers.txt &&
> +	git bugreport -s check-headers &&
> +	check_all_headers_populated <git-bugreport-check-headers.txt
> +'

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

[SZEDER Gábor]

On Wed, Aug 12, 2020 at 05:53:06PM +0200, SZEDER Gábor wrote:
> On Thu, Apr 16, 2020 at 02:18:04PM -0700, Emily Shaffer wrote:
> > 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>
> > ---
> 
> > diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> > new file mode 100755
> > index 0000000000..2e73658a5c
> > --- /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 () {
> 
> I'm afraid that this helper function doesn't do what it was supposed
> to.
> 
> > +	while read -r line
> 
> It iterates through each line of stdin, which is a file written by
> 'git bugreport'.
> 
> > +	do
> > +		if test "$(grep "$HEADER_PATTERN" "$line")"
> 
> This first tries to find a match in the _file_ called "$line", which never
> exists, resulting in trace output:
> 
>   + check_all_headers_populated
>   + read -r line
>   + grep ^\[.*\]$ Thank you for filling out a Git bug report!
>   grep: Thank you for filling out a Git bug report!: No such file or directory
>   + test 
>   + read -r line
>   + grep ^\[.*\]$ Please answer the following questions to help us understand your issue.
>   grep: Please answer the following questions to help us understand your issue.: No such file or directory
>   + test
>   + read -r line
>   + grep ^\[.*\]$
>   grep: : No such file or directory
>   [...]
> 
> Then, since 'grep' doesn't print any matches to its stdout, it invokes
> 
>   test ""
> 
> which always returns non-zero, so that if condition is never fulfilled.

Just a reminder that this test is still broken...

> On first sight I thought that simply changing that 'grep' invocation
> to something like:
> 
>   $(printf "%s\n" "$line" | grep "$HEADER_PATTERN")
> 
> would be sufficient to fix it, but then the first test failed... and
> I'm not sure that I understand what this was supposed to check in the
> first place.
> 
> > +		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' '
> > +	test_when_finished rm git-bugreport-check-headers.txt &&
> > +	git bugreport -s check-headers &&
> > +	check_all_headers_populated <git-bugreport-check-headers.txt
> > +'

[PATCH v13 3/5] bugreport: gather git version and build info

[Emily Shaffer]

Knowing which version of Git a user has and how it was built allows us
to more precisely pin down the circumstances when a certain issue
occurs, so teach bugreport how to tell us the same output as 'git
version --build-options'.

It's not ideal to directly call 'git version --build-options' because
that output goes to stdout. Instead, wrap the version string in a helper
within help.[ch] library, and call that helper from within the bugreport
library.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/git-bugreport.txt |  4 +++
 bugreport.c                     | 19 +++++++++++++-
 help.c                          | 46 ++++++++++++++++++++-------------
 help.h                          |  1 +
 4 files changed, 51 insertions(+), 19 deletions(-)

diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
index 1f9fde5cde..f44ae8cbe7 100644
--- a/Documentation/git-bugreport.txt
+++ b/Documentation/git-bugreport.txt
@@ -23,6 +23,10 @@ The following information is requested from the user:
  - Expected behavior
  - Actual behavior
 
+The following information is captured automatically:
+
+ - 'git version --build-options'
+
 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
diff --git a/bugreport.c b/bugreport.c
index f6f53a5e8e..4cdb58bbaa 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -1,8 +1,17 @@
-#include "builtin.h"
+#include "cache.h"
 #include "parse-options.h"
 #include "stdio.h"
 #include "strbuf.h"
 #include "time.h"
+#include "help.h"
+
+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);
+}
 
 static const char * const bugreport_usage[] = {
 	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
@@ -32,6 +41,11 @@ static int get_bug_template(struct strbuf *template)
 	return 0;
 }
 
+static void get_header(struct strbuf *buf, const char *title)
+{
+	strbuf_addf(buf, "\n\n[%s]\n", title);
+}
+
 int cmd_main(int argc, const char **argv)
 {
 	struct strbuf buffer = STRBUF_INIT;
@@ -79,6 +93,9 @@ int cmd_main(int argc, const char **argv)
 	/* Prepare the report contents */
 	get_bug_template(&buffer);
 
+	get_header(&buffer, _("System Info"));
+	get_system_info(&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);
 
diff --git a/help.c b/help.c
index a21487db77..1de9c0d589 100644
--- a/help.c
+++ b/help.c
@@ -622,8 +622,32 @@ 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));
+		/* 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,25 +661,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));
-		/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
-	}
 	return 0;
 }
 
diff --git a/help.h b/help.h
index 9071894e8c..500521b908 100644
--- a/help.h
+++ b/help.h
@@ -37,6 +37,7 @@ void add_cmdname(struct cmdnames *cmds, const char *name, int len);
 void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes);
 int is_in_cmdlist(struct cmdnames *cmds, const char *name);
 void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdnames *other_cmds);
+void get_version_info(struct strbuf *buf, int show_build_options);
 
 /*
  * call this to die(), when it is suspected that the user mistyped a
-- 
2.26.1.301.g55bc3eb7cb9-goog

[PATCH v13 4/5] bugreport: add uname info

[Emily Shaffer]

The contents of uname() can give us some insight into what sort of
system the user is running on, and help us replicate their setup if need
be. The domainname field is not guaranteed to be available, so don't
collect it.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/git-bugreport.txt |  1 +
 bugreport.c                     | 16 +++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
index f44ae8cbe7..17b0d14e8d 100644
--- a/Documentation/git-bugreport.txt
+++ b/Documentation/git-bugreport.txt
@@ -26,6 +26,7 @@ The following information is requested from the user:
 The following information is captured automatically:
 
  - 'git version --build-options'
+ - uname sysname, release, version, and machine strings
 
 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 4cdb58bbaa..1a3172bcec 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -7,10 +7,24 @@
 
 static void get_system_info(struct strbuf *sys_info)
 {
+	struct utsname uname_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);
+
+	/* system call for other version info */
+	strbuf_addstr(sys_info, "uname: ");
+	if (uname(&uname_info))
+		strbuf_addf(sys_info, _("uname() failed with error '%s' (%d)\n"),
+			    strerror(errno),
+			    errno);
+	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[] = {
-- 
2.26.1.301.g55bc3eb7cb9-goog

Re: [PATCH v13 4/5] bugreport: add uname info

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 16 2020, Emily Shaffer wrote:

> The contents of uname() can give us some insight into what sort of
> system the user is running on, and help us replicate their setup if need
> be. The domainname field is not guaranteed to be available, so don't
> collect it.

Even with _GNU_SOURCE would anyone care about the domainname (the NIS/YP
name, not DNS) these days, as opposed to the portable POSIX "nodename"
field you're not including?

In any case, I'd think it's a good idea to omit both. People tend not to
want to want to include their FQDN (e.g. their employer), and I can't
think of a reason we'd care about it for debugging git.

> [...]
> +		strbuf_addf(sys_info, "%s %s %s %s\n",
> +			    uname_info.sysname,
> +			    uname_info.release,
> +			    uname_info.version,
> +			    uname_info.machine);

Since this is completely free-form I'd think:

    "sysname: %s\nrelease: %s\nversion: %s\nmachine: %s\nnodename: %s\ndomainname: %s\n",

Or something like that would be better (after pruning out the fields we
don't care about).

Re: [PATCH v13 4/5] bugreport: add uname info

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Thu, Apr 16 2020, Emily Shaffer wrote:
>
>> The contents of uname() can give us some insight into what sort of
>> system the user is running on, and help us replicate their setup if need
>> be. The domainname field is not guaranteed to be available, so don't
>> collect it.
>
> Even with _GNU_SOURCE would anyone care about the domainname (the NIS/YP
> name, not DNS) these days, as opposed to the portable POSIX "nodename"
> field you're not including?
>
> In any case, I'd think it's a good idea to omit both. People tend not to
> want to want to include their FQDN (e.g. their employer), and I can't
> think of a reason we'd care about it for debugging git.
>
>> [...]
>> +		strbuf_addf(sys_info, "%s %s %s %s\n",
>> +			    uname_info.sysname,
>> +			    uname_info.release,
>> +			    uname_info.version,
>> +			    uname_info.machine);
>
> Since this is completely free-form I'd think:
>
>     "sysname: %s\nrelease: %s\nversion: %s\nmachine: %s\nnodename: %s\ndomainname: %s\n",
>
> Or something like that would be better (after pruning out the fields we
> don't care about).

All true.

By the way, what's this sudden interest in re-reviewing an age old
topic?

Re: [PATCH v13 4/5] bugreport: add uname info

[Ævar Arnfjörð Bjarmason]

On Fri, Apr 09 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Thu, Apr 16 2020, Emily Shaffer wrote:
>>
>>> The contents of uname() can give us some insight into what sort of
>>> system the user is running on, and help us replicate their setup if need
>>> be. The domainname field is not guaranteed to be available, so don't
>>> collect it.
>>
>> Even with _GNU_SOURCE would anyone care about the domainname (the NIS/YP
>> name, not DNS) these days, as opposed to the portable POSIX "nodename"
>> field you're not including?
>>
>> In any case, I'd think it's a good idea to omit both. People tend not to
>> want to want to include their FQDN (e.g. their employer), and I can't
>> think of a reason we'd care about it for debugging git.
>>
>>> [...]
>>> +		strbuf_addf(sys_info, "%s %s %s %s\n",
>>> +			    uname_info.sysname,
>>> +			    uname_info.release,
>>> +			    uname_info.version,
>>> +			    uname_info.machine);
>>
>> Since this is completely free-form I'd think:
>>
>>     "sysname: %s\nrelease: %s\nversion: %s\nmachine: %s\nnodename: %s\ndomainname: %s\n",
>>
>> Or something like that would be better (after pruning out the fields we
>> don't care about).
>
> All true.
>
> By the way, what's this sudden interest in re-reviewing an age old
> topic?

The thread got bumped by SZEDER in [1] and I'd read an April date
without noticing the year, so I see this has long-since landed,
nevermind :)

1. https://lore.kernel.org/git/20200416211807.60811-2-emilyshaffer@google.com/

Re: [PATCH v13 4/5] bugreport: add uname info

[Emily Shaffer]

On Fri, Apr 09, 2021 at 12:33:42AM +0200, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Apr 09 2021, Junio C Hamano wrote:
> > By the way, what's this sudden interest in re-reviewing an age old
> > topic?
> 
> The thread got bumped by SZEDER in [1] and I'd read an April date
> without noticing the year, so I see this has long-since landed,
> nevermind :)
> 
> 1. https://lore.kernel.org/git/20200416211807.60811-2-emilyshaffer@google.com/

Phew, you scared me :)

 - Emily

Re: [PATCH v13 4/5] bugreport: add uname info

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> On Fri, Apr 09, 2021 at 12:33:42AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> On Fri, Apr 09 2021, Junio C Hamano wrote:
>> > By the way, what's this sudden interest in re-reviewing an age old
>> > topic?
>> 
>> The thread got bumped by SZEDER in [1] and I'd read an April date
>> without noticing the year, so I see this has long-since landed,
>> nevermind :)
>> 
>> 1. https://lore.kernel.org/git/20200416211807.60811-2-emilyshaffer@google.com/
>
> Phew, you scared me :)

Even if it has been in the releases, the room for improvement is
still there, no?

Re: [PATCH v13 4/5] bugreport: add uname info

[SZEDER Gábor]

On Thu, Apr 08, 2021 at 04:41:59PM -0700, Emily Shaffer wrote:
> On Fri, Apr 09, 2021 at 12:33:42AM +0200, Ævar Arnfjörð Bjarmason wrote:
> > On Fri, Apr 09 2021, Junio C Hamano wrote:
> > > By the way, what's this sudden interest in re-reviewing an age old
> > > topic?
> > 
> > The thread got bumped by SZEDER in [1] and I'd read an April date
> > without noticing the year, so I see this has long-since landed,
> > nevermind :)
> > 
> > 1. https://lore.kernel.org/git/20200416211807.60811-2-emilyshaffer@google.com/
> 
> Phew, you scared me :)

Doesn't the output of your bugreport tests scares you?

  Initialized empty Git repository in /home/szeder/src/git/t/trash directory.t0091-bugreport/.git/
  expecting success of 0091.1 'creates a report with content in the right places': 
  	test_when_finished rm git-bugreport-check-headers.txt &&
  	git bugreport -s check-headers &&
  	check_all_headers_populated <git-bugreport-check-headers.txt
  
  Created new report at 'git-bugreport-check-headers.txt'.
  grep: Thank you for filling out a Git bug report!: No such file or directory
  grep: Please answer the following questions to help us understand your issue.: No such file or directory
  grep: : No such file or directory
  grep: What did you do before the bug happened? (Steps to reproduce your issue): No such file or directory
  grep: : No such file or directory
  grep: What did you expect to happen? (Expected behavior): No such file or directory
  grep: : No such file or directory
  grep: What happened instead? (Actual behavior): No such file or directory
  grep: : No such file or directory
  grep: What's different between what you expected and what actually happened?: No such file or directory
  grep: : No such file or directory
  grep: Anything else you want to add:: No such file or directory
  grep: : No such file or directory
  grep: Please review the rest of the bug report below.: No such file or directory
  grep: You can delete any lines you don't wish to share.: No such file or directory
  grep: : No such file or directory
  grep: : No such file or directory
  grep: [System Info]: No such file or directory
  grep: git version:: No such file or directory
  grep: git version 2.31.0.7.ga9ff022d9b: No such file or directory
  grep: cpu: x86_64: No such file or directory
  grep: built from commit: a9ff022d9b49e64336612f89100eb5220ed793bd: No such file or directory
  grep: sizeof-long: 8: No such file or directory
  grep: sizeof-size_t: 8: No such file or directory
  grep: shell-path: /bin/sh: No such file or directory
  grep: uname: Linux 5.10.17-051017-generic #202102170631 SMP Wed Feb 17 11:37:41 UTC 2021 x86_64: No such file or directory
  grep: compiler info: gnuc: 9.3: No such file or directory
  grep: libc info: glibc: 2.31: No such file or directory
  grep: $SHELL (typically, interactive shell): /bin/bash: No such file or directory
  grep: : No such file or directory
  grep: : No such file or directory
  grep: [Enabled Hooks]: No such file or directory
  
  ok 1 - creates a report with content in the right places

It does scare me...

[PATCH] t0091-bugreport.sh: actually verify some content of report

[Martin Ågren]

In the first test in this script, 'creates a report with content in the
right places', we generate a report and pipe it into our helper
`check_all_headers_populated()`. The idea of the helper is to find all
lines that look like headers ("[Some Header Here]") and to check that
the next line is non-empty. This is supposed to catch erroneous outputs
such as the following:

  [A Header]
  something
  more here

  [Another Header]

  [Too Early Header]
  contents

However, we provide the lines of the bug report as filenames to grep,
meaning we mostly end up spewing errors:

  grep: : No such file or directory
  grep: [System Info]: No such file or directory
  grep: git version:: No such file or directory
  grep: git version 2.31.1.164.g984c2561cd: No such file

This doesn't disturb the test, which tugs along and reports success, not
really having verified the contents of the report at all.

Note that after 788a776069 ("bugreport: collect list of populated
hooks", 2020-05-07), the bug report, which is created in our hook-less
test repo, contains an empty section with the enabled hooks. Thus, even
the intention of our helper is a bit misguided: there is nothing
inherently wrong with having an empty section in the bug report.

Let's instead grep for some contents that we expect to find in a bug
report. We won't verify that they appear in the right order, but at
least we end up verifying the contents more than before this commit.

Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 > It does scare me..

 Maybe something like this?

 t/t0091-bugreport.sh | 26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index 526304ff95..9111c4c26f 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -4,29 +4,13 @@ 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' '
+test_expect_success 'creates a report with content' '
 	test_when_finished rm git-bugreport-check-headers.txt &&
 	git bugreport -s check-headers &&
-	check_all_headers_populated <git-bugreport-check-headers.txt
+	grep "^Please answer " git-bugreport-check-headers.txt &&
+	grep "^\[System Info\]$" git-bugreport-check-headers.txt &&
+	grep "^git version:$" git-bugreport-check-headers.txt &&
+	grep "^\[Enabled Hooks\]$" git-bugreport-check-headers.txt
 '
 
 test_expect_success 'dies if file with same name as report already exists' '
-- 
2.31.1.163.ga65ce7f831

Re: [PATCH] t0091-bugreport.sh: actually verify some content of report

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> In the first test in this script, 'creates a report with content in the
> right places', we generate a report and pipe it into our helper
> `check_all_headers_populated()`. The idea of the helper is to find all
> lines that look like headers ("[Some Header Here]") and to check that
> the next line is non-empty. This is supposed to catch erroneous outputs
> such as the following:
>
>   [A Header]
>   something
>   more here
>
>   [Another Header]
>
>   [Too Early Header]
>   contents
>
> However, we provide the lines of the bug report as filenames to grep,
> meaning we mostly end up spewing errors:
>
>   grep: : No such file or directory
>   grep: [System Info]: No such file or directory
>   grep: git version:: No such file or directory
>   grep: git version 2.31.1.164.g984c2561cd: No such file
>
> This doesn't disturb the test, which tugs along and reports success, not
> really having verified the contents of the report at all.
>
> Note that after 788a776069 ("bugreport: collect list of populated
> hooks", 2020-05-07), the bug report, which is created in our hook-less
> test repo, contains an empty section with the enabled hooks. Thus, even
> the intention of our helper is a bit misguided: there is nothing
> inherently wrong with having an empty section in the bug report.
>
> Let's instead grep for some contents that we expect to find in a bug
> report. We won't verify that they appear in the right order, but at
> least we end up verifying the contents more than before this commit.

Nicely described.  I agree that the original intent (let alone the
implementation) is misguided and we should allow an empty section as
a perfectly normal thing.

> Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  > It does scare me..
>
>  Maybe something like this?
> ...
> +test_expect_success 'creates a report with content' '
>  	test_when_finished rm git-bugreport-check-headers.txt &&
>  	git bugreport -s check-headers &&
> -	check_all_headers_populated <git-bugreport-check-headers.txt
> +	grep "^Please answer " git-bugreport-check-headers.txt &&
> +	grep "^\[System Info\]$" git-bugreport-check-headers.txt &&
> +	grep "^git version:$" git-bugreport-check-headers.txt &&
> +	grep "^\[Enabled Hooks\]$" git-bugreport-check-headers.txt
>  '

It is a different matter if it is sufficient to ensure only certain
selected lines appear in the report, though.  As all the lines lost
by this fix comes from 238b439d (bugreport: add tool to generate
debugging info, 2020-04-16), it would be nice to hear from Emily.

Thanks.

Re: [PATCH] t0091-bugreport.sh: actually verify some content of report

[Martin Ågren]

On Mon, 12 Apr 2021 at 19:17, Junio C Hamano <gitster@pobox.com> wrote:
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > In the first test in this script, 'creates a report with content in the
> > right places', we generate a report and pipe it into our helper
> > `check_all_headers_populated()`. The idea of the helper is to find all
> > lines that look like headers ("[Some Header Here]") and to check that
> > the next line is non-empty. This is supposed to catch erroneous outputs
> > such as the following:
...
> > Let's instead grep for some contents that we expect to find in a bug
> > report. We won't verify that they appear in the right order, but at
> > least we end up verifying the contents more than before this commit.
>
> Nicely described.  I agree that the original intent (let alone the
> implementation) is misguided and we should allow an empty section as
> a perfectly normal thing.

> > +test_expect_success 'creates a report with content' '
> >       test_when_finished rm git-bugreport-check-headers.txt &&
> >       git bugreport -s check-headers &&
> > -     check_all_headers_populated <git-bugreport-check-headers.txt
> > +     grep "^Please answer " git-bugreport-check-headers.txt &&
> > +     grep "^\[System Info\]$" git-bugreport-check-headers.txt &&
> > +     grep "^git version:$" git-bugreport-check-headers.txt &&
> > +     grep "^\[Enabled Hooks\]$" git-bugreport-check-headers.txt
> >  '
>
> It is a different matter if it is sufficient to ensure only certain
> selected lines appear in the report, though.  As all the lines lost
> by this fix comes from 238b439d (bugreport: add tool to generate
> debugging info, 2020-04-16), it would be nice to hear from Emily.

Maybe something like

       awk '\''BEGIN { sect="" }
               /^\[.*]$/ { sect=$0 }
               /./ { print sect, $0 }'\'' \
           git-bugreport-check-headers.txt >prefixed &&
       grep "^ Thank you for filling out a Git bug report" prefixed &&
       grep "^ Please review the rest of the bug report below" prefixed &&
       grep "^ You can delete any lines you don.t wish to share" prefixed &&
       grep "\[System Info\] git version ..." prefixed

Something like that could be used to verify that a line goes into the
right section, as opposed to just seeing that it appears *somewhere*. Or
maybe

  grep -e Thank.you -e Please.review -e You.can.delete -e "^\[" \
       -e git.version git-bugreport-check-headers.txt >actual

then setting up an "expect" and comparing. That would help us verify the
order, including which section things appear in. Slightly less friendly
for comparing loosely, compared to the awk-then-grep above.

Let's see what Emily thinks about the various alternatives. Maybe she can
think of something else.

Martin

Re: [PATCH] t0091-bugreport.sh: actually verify some content of report

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 13 2021, Martin Ågren wrote:

> On Mon, 12 Apr 2021 at 19:17, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Martin Ågren <martin.agren@gmail.com> writes:
>>
>> > In the first test in this script, 'creates a report with content in the
>> > right places', we generate a report and pipe it into our helper
>> > `check_all_headers_populated()`. The idea of the helper is to find all
>> > lines that look like headers ("[Some Header Here]") and to check that
>> > the next line is non-empty. This is supposed to catch erroneous outputs
>> > such as the following:
> ...
>> > Let's instead grep for some contents that we expect to find in a bug
>> > report. We won't verify that they appear in the right order, but at
>> > least we end up verifying the contents more than before this commit.
>>
>> Nicely described.  I agree that the original intent (let alone the
>> implementation) is misguided and we should allow an empty section as
>> a perfectly normal thing.
>
>> > +test_expect_success 'creates a report with content' '
>> >       test_when_finished rm git-bugreport-check-headers.txt &&
>> >       git bugreport -s check-headers &&
>> > -     check_all_headers_populated <git-bugreport-check-headers.txt
>> > +     grep "^Please answer " git-bugreport-check-headers.txt &&
>> > +     grep "^\[System Info\]$" git-bugreport-check-headers.txt &&
>> > +     grep "^git version:$" git-bugreport-check-headers.txt &&
>> > +     grep "^\[Enabled Hooks\]$" git-bugreport-check-headers.txt
>> >  '
>>
>> It is a different matter if it is sufficient to ensure only certain
>> selected lines appear in the report, though.  As all the lines lost
>> by this fix comes from 238b439d (bugreport: add tool to generate
>> debugging info, 2020-04-16), it would be nice to hear from Emily.
>
> Maybe something like
>
>        awk '\''BEGIN { sect="" }
>                /^\[.*]$/ { sect=$0 }
>                /./ { print sect, $0 }'\'' \
>            git-bugreport-check-headers.txt >prefixed &&
>        grep "^ Thank you for filling out a Git bug report" prefixed &&
>        grep "^ Please review the rest of the bug report below" prefixed &&
>        grep "^ You can delete any lines you don.t wish to share" prefixed &&
>        grep "\[System Info\] git version ..." prefixed
>
> Something like that could be used to verify that a line goes into the
> right section, as opposed to just seeing that it appears *somewhere*. Or
> maybe
>
>   grep -e Thank.you -e Please.review -e You.can.delete -e "^\[" \
>        -e git.version git-bugreport-check-headers.txt >actual
>
> then setting up an "expect" and comparing. That would help us verify the
> order, including which section things appear in. Slightly less friendly
> for comparing loosely, compared to the awk-then-grep above.
>
> Let's see what Emily thinks about the various alternatives. Maybe she can
> think of something else.

I think a straight-up test_cmp is preferrable, both for correctness and
also as self-documentation, you can see from the test what the full
expected output is like.

Obviously in this case we can't do a test_cmp on the raw output, as it
contains various things from uname.

But it looks like we could do that if we do some light awk/perl/sed
munging of the "[System Info]" and "[Enabled Hooks]" section(s).

Or, since we also control the generator we could pass a --no-system-info
and/or --no-hooks-info, which may be something some people want for
privacy/reporting reasons anyway (e.g. I've often used perlbug and
deleted that whole info, because info there has no relevance to the
specific issue I'm reporting).

Re: [PATCH] t0091-bugreport.sh: actually verify some content of report

[Emily Shaffer]

On Tue, Apr 13, 2021 at 09:27:33PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> 
> On Tue, Apr 13 2021, Martin Ågren wrote:
> 
> > On Mon, 12 Apr 2021 at 19:17, Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> Martin Ågren <martin.agren@gmail.com> writes:
> >>
> >> > In the first test in this script, 'creates a report with content in the
> >> > right places', we generate a report and pipe it into our helper
> >> > `check_all_headers_populated()`. The idea of the helper is to find all
> >> > lines that look like headers ("[Some Header Here]") and to check that
> >> > the next line is non-empty. This is supposed to catch erroneous outputs
> >> > such as the following:
> > ...
> >> > Let's instead grep for some contents that we expect to find in a bug
> >> > report. We won't verify that they appear in the right order, but at
> >> > least we end up verifying the contents more than before this commit.
> >>
> >> Nicely described.  I agree that the original intent (let alone the
> >> implementation) is misguided and we should allow an empty section as
> >> a perfectly normal thing.
> >
> >> > +test_expect_success 'creates a report with content' '
> >> >       test_when_finished rm git-bugreport-check-headers.txt &&
> >> >       git bugreport -s check-headers &&
> >> > -     check_all_headers_populated <git-bugreport-check-headers.txt
> >> > +     grep "^Please answer " git-bugreport-check-headers.txt &&
> >> > +     grep "^\[System Info\]$" git-bugreport-check-headers.txt &&
> >> > +     grep "^git version:$" git-bugreport-check-headers.txt &&
> >> > +     grep "^\[Enabled Hooks\]$" git-bugreport-check-headers.txt
> >> >  '
> >>
> >> It is a different matter if it is sufficient to ensure only certain
> >> selected lines appear in the report, though.  As all the lines lost
> >> by this fix comes from 238b439d (bugreport: add tool to generate
> >> debugging info, 2020-04-16), it would be nice to hear from Emily.
> >
> > Maybe something like
> >
> >        awk '\''BEGIN { sect="" }
> >                /^\[.*]$/ { sect=$0 }
> >                /./ { print sect, $0 }'\'' \
> >            git-bugreport-check-headers.txt >prefixed &&
> >        grep "^ Thank you for filling out a Git bug report" prefixed &&
> >        grep "^ Please review the rest of the bug report below" prefixed &&
> >        grep "^ You can delete any lines you don.t wish to share" prefixed &&
> >        grep "\[System Info\] git version ..." prefixed
> >
> > Something like that could be used to verify that a line goes into the
> > right section, as opposed to just seeing that it appears *somewhere*. Or
> > maybe
> >
> >   grep -e Thank.you -e Please.review -e You.can.delete -e "^\[" \
> >        -e git.version git-bugreport-check-headers.txt >actual
> >
> > then setting up an "expect" and comparing. That would help us verify the
> > order, including which section things appear in. Slightly less friendly
> > for comparing loosely, compared to the awk-then-grep above.
> >
> > Let's see what Emily thinks about the various alternatives. Maybe she can
> > think of something else.

My apologies for the slow reply :)

> I think a straight-up test_cmp is preferrable, both for correctness and
> also as self-documentation, you can see from the test what the full
> expected output is like.

Yeah, I like the sound of this.

> 
> Obviously in this case we can't do a test_cmp on the raw output, as it
> contains various things from uname.
> 
> But it looks like we could do that if we do some light awk/perl/sed
> munging of the "[System Info]" and "[Enabled Hooks]" section(s).
> 
> Or, since we also control the generator we could pass a --no-system-info
> and/or --no-hooks-info, which may be something some people want for
> privacy/reporting reasons anyway (e.g. I've often used perlbug and
> deleted that whole info, because info there has no relevance to the
> specific issue I'm reporting).

This approach sounds more appealing to me than awk munging. I think
you're right that folks may not want to share it in some cases.

Thanks for noticing.
 - Emily

[PATCH v2] t0091-bugreport.sh: actually verify some content of report

[Martin Ågren]

In the first test in this script, 'creates a report with content in the
right places', we generate a report and pipe it into our helper
`check_all_headers_populated()`. The idea of the helper is to find all
lines that look like headers ("[Some Header Here]") and to check that
the next line is non-empty. This is supposed to catch erroneous outputs
such as the following:

  [A Header]
  something
  more here

  [Another Header]

  [Too Early Header]
  contents

However, we provide the lines of the bug report as filenames to grep,
meaning we mostly end up spewing errors:

  grep: : No such file or directory
  grep: [System Info]: No such file or directory
  grep: git version:: No such file or directory
  grep: git version 2.41.0.2.gfb7d80edca: No such file or directory

This doesn't disturb the test, which tugs along and reports success, not
really having verified the contents of the report at all.

Note that after 788a776069 ("bugreport: collect list of populated
hooks", 2020-05-07), the bug report, which is created in our hook-less
test repo, contains an empty section with the enabled hooks. Thus, even
the intention of our helper is a bit misguided: there is nothing
inherently wrong with having an empty section in the bug report.

Let's instead split this test into three: first verify that we generate
a report at all, then check that the introductory blurb looks the way it
should, then verify that the "[System Info]" seems to contain the right
things. (The "[Enabled Hooks]" section is tested later in the script.)

Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 This is a much belated follow-up to v1 [1]. Feedback on that patch
 ranged from not bothering checking the generated report at all to
 implementing `--no-system-info` and `--no-hooks-info` so that we could
 easily test the introductory blurb verbatim.

 I hope I'm hitting a reasonable middle ground here. Verifying the
 contents at least somewhat is in line with the original intention of
 the test. Those `--no-...-info` could probably be useful to bug
 reporters, but it feels wrong to me to implement them just to be able
 to use them in the tests.

 [1] https://lore.kernel.org/git/20210411143354.25134-1-martin.agren@gmail.com/

 t/t0091-bugreport.sh | 67 +++++++++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 23 deletions(-)

diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index b6d2f591ac..9390631b17 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -5,29 +5,50 @@ test_description='git bugreport'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./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' '
-	test_when_finished rm git-bugreport-check-headers.txt &&
-	git bugreport -s check-headers &&
-	check_all_headers_populated <git-bugreport-check-headers.txt
+test_expect_success 'create a report' '
+	git bugreport -s format &&
+	test_file_not_empty git-bugreport-format.txt
+'
+
+test_expect_success 'report contains wanted template (before first section)' '
+	awk "/^\[/ { exit } { print }" git-bugreport-format.txt >actual &&
+	cat >expect <<-\EOF &&
+	Thank you for filling out a Git bug report!
+	Please answer the following questions to help us understand your issue.
+
+	What did you do before the bug happened? (Steps to reproduce your issue)
+
+	What did you expect to happen? (Expected behavior)
+
+	What happened instead? (Actual behavior)
+
+	What'\''s different between what you expected and what actually happened?
+
+	Anything else you want to add:
+
+	Please review the rest of the bug report below.
+	You can delete any lines you don'\''t wish to share.
+
+
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'sanity check "System Info" section' '
+	test_when_finished rm -f git-bugreport-format.txt &&
+
+	sed -ne "/^\[System Info\]$/,/^$/p" <git-bugreport-format.txt >system &&
+
+	# The beginning should match "git version --build-info" verbatim,
+	# but rather than checking bit-for-bit equality, just test some basics.
+	grep "git version [0-9]." system &&
+	grep "shell-path: ." system &&
+
+	# After the version, there should be some more info.
+	# This is bound to differ from environment to environment,
+	# so we just do some rather high-level checks.
+	grep "uname: ." system &&
+	grep "compiler info: ." system
 '
 
 test_expect_success 'dies if file with same name as report already exists' '
-- 
2.41.0.404.g5b50783d6b

Re: [PATCH v2] t0091-bugreport.sh: actually verify some content of report

[Phillip Wood]

Hi Martin

On 01/07/2023 20:26, Martin Ågren wrote:
> In the first test in this script, 'creates a report with content in the
> right places', we generate a report and pipe it into our helper
> `check_all_headers_populated()`. The idea of the helper is to find all
> lines that look like headers ("[Some Header Here]") and to check that
> the next line is non-empty. This is supposed to catch erroneous outputs
> such as the following:
> 
>    [A Header]
>    something
>    more here
> 
>    [Another Header]
> 
>    [Too Early Header]
>    contents
> 
> However, we provide the lines of the bug report as filenames to grep,
> meaning we mostly end up spewing errors:
> 
>    grep: : No such file or directory
>    grep: [System Info]: No such file or directory
>    grep: git version:: No such file or directory
>    grep: git version 2.41.0.2.gfb7d80edca: No such file or directory
> 
> This doesn't disturb the test, which tugs along and reports success, not
> really having verified the contents of the report at all.

Thanks for the clear description of the problem and for fixing it

> Note that after 788a776069 ("bugreport: collect list of populated
> hooks", 2020-05-07), the bug report, which is created in our hook-less
> test repo, contains an empty section with the enabled hooks. Thus, even
> the intention of our helper is a bit misguided: there is nothing
> inherently wrong with having an empty section in the bug report.
> 
> Let's instead split this test into three: first verify that we generate
> a report at all, then check that the introductory blurb looks the way it
> should, then verify that the "[System Info]" seems to contain the right
> things. (The "[Enabled Hooks]" section is tested later in the script.)

That sounds like a good plan

> +test_expect_success 'report contains wanted template (before first section)' '
> +	awk "/^\[/ { exit } { print }" git-bugreport-format.txt >actual &&

Personally I'd find

	sed -n -e '/^\[/q;p' git-bugreport-format.txt >actual

easier to understand but that's probably because I don't use awk very 
much. I'm not sure it is worth a re-roll though as I see we do use awk 
in a few of the other test scripts.

Best Wishes

Phillip

Re: [PATCH v2] t0091-bugreport.sh: actually verify some content of report

[Martin Ågren]

Hi Phillip,

On Mon, 3 Jul 2023 at 17:47, Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> > +test_expect_success 'report contains wanted template (before first section)' '
> > +     awk "/^\[/ { exit } { print }" git-bugreport-format.txt >actual &&
>
> Personally I'd find
>
>         sed -n -e '/^\[/q;p' git-bugreport-format.txt >actual
>
> easier to understand but that's probably because I don't use awk very
> much. I'm not sure it is worth a re-roll though as I see we do use awk
> in a few of the other test scripts.

I do see we use sed quite a bit more than awk in the tests. This script
doesn't yet use awk and if most feel like you we're probably better off
introducing another sed use rather than another awk use.

I'll be happy to post a v3 with only this change. (This script uses "sed
-ne" already and I even do so elsewhere in my patch, so I'll be applying
s/-n -e/-ne/ to your suggestion.)

Thanks for reviewing!

Martin

[PATCH v3] t0091-bugreport.sh: actually verify some content of report

[Martin Ågren]

In the first test in this script, 'creates a report with content in the
right places', we generate a report and pipe it into our helper
`check_all_headers_populated()`. The idea of the helper is to find all
lines that look like headers ("[Some Header Here]") and to check that
the next line is non-empty. This is supposed to catch erroneous outputs
such as the following:

  [A Header]
  something
  more here

  [Another Header]

  [Too Early Header]
  contents

However, we provide the lines of the bug report as filenames to grep,
meaning we mostly end up spewing errors:

  grep: : No such file or directory
  grep: [System Info]: No such file or directory
  grep: git version:: No such file or directory
  grep: git version 2.41.0.2.gfb7d80edca: No such file or directory

This doesn't disturb the test, which tugs along and reports success, not
really having verified the contents of the report at all.

Note that after 788a776069 ("bugreport: collect list of populated
hooks", 2020-05-07), the bug report, which is created in our hook-less
test repo, contains an empty section with the enabled hooks. Thus, even
the intention of our helper is a bit misguided: there is nothing
inherently wrong with having an empty section in the bug report.

Let's instead split this test into three: first verify that we generate
a report at all, then check that the introductory blurb looks the way it
should, then verify that the "[System Info]" seems to contain the right
things. (The "[Enabled Hooks]" section is tested later in the script.)

Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 (Resend of v3, now with correct In-Reply-To.)

 t/t0091-bugreport.sh | 67 +++++++++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 23 deletions(-)

diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index b6d2f591ac..f6998269be 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -5,29 +5,50 @@ test_description='git bugreport'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./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' '
-	test_when_finished rm git-bugreport-check-headers.txt &&
-	git bugreport -s check-headers &&
-	check_all_headers_populated <git-bugreport-check-headers.txt
+test_expect_success 'create a report' '
+	git bugreport -s format &&
+	test_file_not_empty git-bugreport-format.txt
+'
+
+test_expect_success 'report contains wanted template (before first section)' '
+	sed -ne "/^\[/q;p" git-bugreport-format.txt >actual &&
+	cat >expect <<-\EOF &&
+	Thank you for filling out a Git bug report!
+	Please answer the following questions to help us understand your issue.
+
+	What did you do before the bug happened? (Steps to reproduce your issue)
+
+	What did you expect to happen? (Expected behavior)
+
+	What happened instead? (Actual behavior)
+
+	What'\''s different between what you expected and what actually happened?
+
+	Anything else you want to add:
+
+	Please review the rest of the bug report below.
+	You can delete any lines you don'\''t wish to share.
+
+
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'sanity check "System Info" section' '
+	test_when_finished rm -f git-bugreport-format.txt &&
+
+	sed -ne "/^\[System Info\]$/,/^$/p" <git-bugreport-format.txt >system &&
+
+	# The beginning should match "git version --build-info" verbatim,
+	# but rather than checking bit-for-bit equality, just test some basics.
+	grep "git version [0-9]." system &&
+	grep "shell-path: ." system &&
+
+	# After the version, there should be some more info.
+	# This is bound to differ from environment to environment,
+	# so we just do some rather high-level checks.
+	grep "uname: ." system &&
+	grep "compiler info: ." system
 '
 
 test_expect_success 'dies if file with same name as report already exists' '
-- 
2.41.0.404.g5b50783d6b

Re: [PATCH v3] t0091-bugreport.sh: actually verify some content of report

[Phillip Wood]

Hi Martin

This version looks good to me, thanks for re-rolling

Phillip

On 05/07/2023 19:40, Martin Ågren wrote:
> In the first test in this script, 'creates a report with content in the
> right places', we generate a report and pipe it into our helper
> `check_all_headers_populated()`. The idea of the helper is to find all
> lines that look like headers ("[Some Header Here]") and to check that
> the next line is non-empty. This is supposed to catch erroneous outputs
> such as the following:
> 
>    [A Header]
>    something
>    more here
> 
>    [Another Header]
> 
>    [Too Early Header]
>    contents
> 
> However, we provide the lines of the bug report as filenames to grep,
> meaning we mostly end up spewing errors:
> 
>    grep: : No such file or directory
>    grep: [System Info]: No such file or directory
>    grep: git version:: No such file or directory
>    grep: git version 2.41.0.2.gfb7d80edca: No such file or directory
> 
> This doesn't disturb the test, which tugs along and reports success, not
> really having verified the contents of the report at all.
> 
> Note that after 788a776069 ("bugreport: collect list of populated
> hooks", 2020-05-07), the bug report, which is created in our hook-less
> test repo, contains an empty section with the enabled hooks. Thus, even
> the intention of our helper is a bit misguided: there is nothing
> inherently wrong with having an empty section in the bug report.
> 
> Let's instead split this test into three: first verify that we generate
> a report at all, then check that the introductory blurb looks the way it
> should, then verify that the "[System Info]" seems to contain the right
> things. (The "[Enabled Hooks]" section is tested later in the script.)
> 
> Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>   (Resend of v3, now with correct In-Reply-To.)
> 
>   t/t0091-bugreport.sh | 67 +++++++++++++++++++++++++++++---------------
>   1 file changed, 44 insertions(+), 23 deletions(-)
> 
> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> index b6d2f591ac..f6998269be 100755
> --- a/t/t0091-bugreport.sh
> +++ b/t/t0091-bugreport.sh
> @@ -5,29 +5,50 @@ test_description='git bugreport'
>   TEST_PASSES_SANITIZE_LEAK=true
>   . ./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' '
> -	test_when_finished rm git-bugreport-check-headers.txt &&
> -	git bugreport -s check-headers &&
> -	check_all_headers_populated <git-bugreport-check-headers.txt
> +test_expect_success 'create a report' '
> +	git bugreport -s format &&
> +	test_file_not_empty git-bugreport-format.txt
> +'
> +
> +test_expect_success 'report contains wanted template (before first section)' '
> +	sed -ne "/^\[/q;p" git-bugreport-format.txt >actual &&
> +	cat >expect <<-\EOF &&
> +	Thank you for filling out a Git bug report!
> +	Please answer the following questions to help us understand your issue.
> +
> +	What did you do before the bug happened? (Steps to reproduce your issue)
> +
> +	What did you expect to happen? (Expected behavior)
> +
> +	What happened instead? (Actual behavior)
> +
> +	What'\''s different between what you expected and what actually happened?
> +
> +	Anything else you want to add:
> +
> +	Please review the rest of the bug report below.
> +	You can delete any lines you don'\''t wish to share.
> +
> +
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'sanity check "System Info" section' '
> +	test_when_finished rm -f git-bugreport-format.txt &&
> +
> +	sed -ne "/^\[System Info\]$/,/^$/p" <git-bugreport-format.txt >system &&
> +
> +	# The beginning should match "git version --build-info" verbatim,
> +	# but rather than checking bit-for-bit equality, just test some basics.
> +	grep "git version [0-9]." system &&
> +	grep "shell-path: ." system &&
> +
> +	# After the version, there should be some more info.
> +	# This is bound to differ from environment to environment,
> +	# so we just do some rather high-level checks.
> +	grep "uname: ." system &&
> +	grep "compiler info: ." system
>   '
>   
>   test_expect_success 'dies if file with same name as report already exists' '

Re: [PATCH] t0091-bugreport.sh: actually verify some content of report

[SZEDER Gábor]

On Sun, Apr 11, 2021 at 04:33:54PM +0200, Martin Ågren wrote:
> In the first test in this script, 'creates a report with content in the
> right places', we generate a report and pipe it into our helper
> `check_all_headers_populated()`. The idea of the helper is to find all
> lines that look like headers ("[Some Header Here]") and to check that
> the next line is non-empty. This is supposed to catch erroneous outputs
> such as the following:
> 
>   [A Header]
>   something
>   more here
> 
>   [Another Header]
> 
>   [Too Early Header]
>   contents
> 
> However, we provide the lines of the bug report as filenames to grep,
> meaning we mostly end up spewing errors:
> 
>   grep: : No such file or directory
>   grep: [System Info]: No such file or directory
>   grep: git version:: No such file or directory
>   grep: git version 2.31.1.164.g984c2561cd: No such file
> 
> This doesn't disturb the test, which tugs along and reports success, not
> really having verified the contents of the report at all.
> 
> Note that after 788a776069 ("bugreport: collect list of populated
> hooks", 2020-05-07), the bug report, which is created in our hook-less
> test repo, contains an empty section with the enabled hooks. Thus, even
> the intention of our helper is a bit misguided: there is nothing
> inherently wrong with having an empty section in the bug report.
> 
> Let's instead grep for some contents that we expect to find in a bug
> report. We won't verify that they appear in the right order, but at
> least we end up verifying the contents more than before this commit.
> 
> Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  > It does scare me..
> 
>  Maybe something like this?

Thanks!

>  t/t0091-bugreport.sh | 26 +++++---------------------
>  1 file changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> index 526304ff95..9111c4c26f 100755
> --- a/t/t0091-bugreport.sh
> +++ b/t/t0091-bugreport.sh
> @@ -4,29 +4,13 @@ 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' '
> +test_expect_success 'creates a report with content' '
>  	test_when_finished rm git-bugreport-check-headers.txt &&
>  	git bugreport -s check-headers &&
> -	check_all_headers_populated <git-bugreport-check-headers.txt
> +	grep "^Please answer " git-bugreport-check-headers.txt &&

This "Please answer" is translated and you look for it with plain
'grep' instead of 'test_i18ngrep', which is fine nowadays...  However,
Junio queued this patch on top of v2.29.3, which is old enough to
still have the GETTEXT_POISON CI job, and fails because of this.

> +	grep "^\[System Info\]$" git-bugreport-check-headers.txt &&
> +	grep "^git version:$" git-bugreport-check-headers.txt &&
> +	grep "^\[Enabled Hooks\]$" git-bugreport-check-headers.txt
>  '

I have to wonder, however, whether this is worth testing at all.

>  
>  test_expect_success 'dies if file with same name as report already exists' '
> -- 
> 2.31.1.163.ga65ce7f831
> 

[PATCH v13 5/5] bugreport: add compiler info

[Emily Shaffer]

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>
Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 Documentation/git-bugreport.txt |  1 +
 bugreport.c                     |  6 +++++
 compat/compiler.h               | 41 +++++++++++++++++++++++++++++++++
 3 files changed, 48 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 1a3172bcec..089b939a87 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..10dbb65937
--- /dev/null
+++ b/compat/compiler.h
@@ -0,0 +1,41 @@
+#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 __clang__
+	strbuf_addf(info, "clang: %s\n", __clang_version__);
+#elif defined(__GNUC__)
+	strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__);
+#endif
+
+#ifdef _MSC_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)
+		strbuf_addstr(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_addstr(info, _("no libc information available\n"));
+}
+
+#endif /* COMPILER_H */
-- 
2.26.1.301.g55bc3eb7cb9-goog

Re: [PATCH v13 5/5] bugreport: add compiler info

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 16 2020, Emily Shaffer wrote:

> 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>
> Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> ---
>  Documentation/git-bugreport.txt |  1 +
>  bugreport.c                     |  6 +++++
>  compat/compiler.h               | 41 +++++++++++++++++++++++++++++++++
>  3 files changed, 48 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 1a3172bcec..089b939a87 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: "));

These are marked with _() but not the "clang" etc. below. I'd think that
for a git-bugreport tool we'd be better off without any i18n.

> +	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..10dbb65937
> --- /dev/null
> +++ b/compat/compiler.h
> @@ -0,0 +1,41 @@
> +#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 __clang__
> +	strbuf_addf(info, "clang: %s\n", __clang_version__);
> +#elif defined(__GNUC__)
> +	strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__);
> +#endif
> +
> +#ifdef _MSC_VER
> +	strbuf_addf(info, "MSVC version: %02d.%02d.%05d\n",
> +		    _MSC_VER / 100, _MSC_VER % 100, _MSC_FULL_VER % 100000);
> +#endif

Why the ifdef/elif/ifdef instead of ifdef/elif/elif? Isn't _MSC_VER
mutually exclusive with __clang__ and __GNUC__?

Re: [PATCH v13 5/5] bugreport: add compiler info

[Đoàn Trần Công Danh]

On 2021-04-09 00:23:52+0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> > +static inline void get_compiler_info(struct strbuf *info)
> > +{
> > +	int len = info->len;
> > +#ifdef __clang__
> > +	strbuf_addf(info, "clang: %s\n", __clang_version__);
> > +#elif defined(__GNUC__)
> > +	strbuf_addf(info, "gnuc: %d.%d\n", __GNUC__, __GNUC_MINOR__);
> > +#endif
> > +
> > +#ifdef _MSC_VER
> > +	strbuf_addf(info, "MSVC version: %02d.%02d.%05d\n",
> > +		    _MSC_VER / 100, _MSC_VER % 100, _MSC_FULL_VER % 100000);
> > +#endif
> 
> Why the ifdef/elif/ifdef instead of ifdef/elif/elif? Isn't _MSC_VER
> mutually exclusive with __clang__ and __GNUC__?

No, clang-cl is a MSVC emulation from clang, and clang-cl defines both
__clang__ and _MSC_VER. See: [mail].

AFAICT, clang-cl is production-usable, Google uses clang-cl to build
Google Chrome for Windows fro 2018 [blog]

mail: https://lists.llvm.org/pipermail/cfe-dev/2016-March/048147.html
blog: https://blog.llvm.org/2018/03/clang-is-now-used-to-build-chrome-for.html

-- 
Danh