[PATCH] bugreport: collect list of populated hooks

[PATCH] bugreport: collect list of populated hooks

[Emily Shaffer]

Occasionally a failure a user is seeing may be related to a specific
hook which is being run, perhaps without the user realizing. While the
contents of hooks can be sensitive - containing user data or process
information specific to the user's organization - simply knowing that a
hook is being run at a certain stage can help us to understand whether
something is going wrong.

Without a definitive list of hook names within the code, we compile our
own list from the documentation. This is likely prone to bitrot. To
reduce the amount of code humans need to read, we turn the list into a
string_list and iterate over it (as we are calling the same find_hook
operation on each string). However, since bugreport should primarily be
called by the user, the performance loss from massaging the string
seems acceptable.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
Based on es/bugreport.

 Documentation/git-bugreport.txt |  1 +
 bugreport.c                     | 52 +++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
index 643d1b2884..7fe9aef34e 100644
--- a/Documentation/git-bugreport.txt
+++ b/Documentation/git-bugreport.txt
@@ -28,6 +28,7 @@ The following information is captured automatically:
  - 'git version --build-options'
  - uname sysname, release, version, and machine strings
  - Compiler-specific info string
+ - A list of enabled hooks
 
 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 089b939a87..ce32145bce 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -5,6 +5,8 @@
 #include "time.h"
 #include "help.h"
 #include "compat/compiler.h"
+#include "run-command.h"
+
 
 static void get_system_info(struct strbuf *sys_info)
 {
@@ -33,6 +35,53 @@ static void get_system_info(struct strbuf *sys_info)
 	get_libc_info(sys_info);
 }
 
+static void get_populated_hooks(struct strbuf *hook_info, int nongit)
+{
+	/*
+	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
+	 * so below is a transcription of `git help hooks`. Later, this should
+	 * be replaced with some programmatically generated list (generated from
+	 * doc or else taken from some library which tells us about all the
+	 * hooks)
+	 */
+	const char *hook[] = {
+		"applypatch-msg",
+		"pre-applypatch",
+		"post-applypatch",
+		"pre-commit",
+		"pre-merge-commit",
+		"prepare-commit-msg",
+		"commit-msg",
+		"post-commit",
+		"pre-rebase",
+		"post-checkout",
+		"post-merge",
+		"pre-push",
+		"pre-receive",
+		"update",
+		"post-receive",
+		"post-update",
+		"push-to-checkout",
+		"pre-auto-gc",
+		"post-rewrite",
+		"sendemail-validate",
+		"fsmonitor-watchman",
+		"p4-pre-submit",
+		"post-index-change",
+	};
+	int i;
+
+	if (nongit) {
+		strbuf_addstr(hook_info,
+			"not run from a git repository - no hooks to show\n");
+		return;
+	}
+
+	for (i=0; i < ARRAY_SIZE(hook); i++)
+		if (find_hook(hook[i]))
+			strbuf_addf(hook_info, "%s\n", hook[i]);
+}
+
 static const char * const bugreport_usage[] = {
 	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
 	NULL
@@ -116,6 +165,9 @@ int cmd_main(int argc, const char **argv)
 	get_header(&buffer, _("System Info"));
 	get_system_info(&buffer);
 
+	get_header(&buffer, "Enabled Hooks");
+	get_populated_hooks(&buffer, nongit_ok);
+
 	/* fopen doesn't offer us an O_EXCL alternative, except with glibc. */
 	report = open(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);
 
-- 
2.26.2.303.gf8c07b1a785-goog

Re: [PATCH] bugreport: collect list of populated hooks

[Jonathan Nieder]

Emily Shaffer wrote:

> Occasionally a failure a user is seeing may be related to a specific
> hook which is being run, perhaps without the user realizing. While the
> contents of hooks can be sensitive - containing user data or process
> information specific to the user's organization - simply knowing that a
> hook is being run at a certain stage can help us to understand whether
> something is going wrong.

Nice.

[...]
>  Documentation/git-bugreport.txt |  1 +
>  bugreport.c                     | 52 +++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)

Can this functionality be demonstrated in a test?

> diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
> index 643d1b2884..7fe9aef34e 100644
> --- a/Documentation/git-bugreport.txt
> +++ b/Documentation/git-bugreport.txt
> @@ -28,6 +28,7 @@ The following information is captured automatically:
>   - 'git version --build-options'
>   - uname sysname, release, version, and machine strings
>   - Compiler-specific info string
> + - A list of enabled hooks
>  
>  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 089b939a87..ce32145bce 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -5,6 +5,8 @@
>  #include "time.h"

Not about this patch: this is for the system header <time.h>, right?
Git includes the same system headers in (almost) all source files, via
cache.h or git-compat-util.h, so it should be possible to leave this
#include out.  (Handling it in one place gives us a chance to get
portability gotchas around names of headers, macros like _POSIX_SOURCE,
and so on right).

[...]
> +	/*
> +	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
> +	 * so below is a transcription of `git help hooks`. Later, this should
> +	 * be replaced with some programmatically generated list (generated from
> +	 * doc or else taken from some library which tells us about all the
> +	 * hooks)
> +	 */
> +	const char *hook[] = {
> +		"applypatch-msg",
> +		"pre-applypatch",
> +		"post-applypatch",
> +		"pre-commit",
> +		"pre-merge-commit",
> +		"prepare-commit-msg",
> +		"commit-msg",
> +		"post-commit",
> +		"pre-rebase",
> +		"post-checkout",
> +		"post-merge",
> +		"pre-push",
> +		"pre-receive",
> +		"update",
> +		"post-receive",
> +		"post-update",
> +		"push-to-checkout",
> +		"pre-auto-gc",
> +		"post-rewrite",
> +		"sendemail-validate",
> +		"fsmonitor-watchman",
> +		"p4-pre-submit",
> +		"post-index-change",
> +	};

Interesting.   It would be possible to do some gettext-style trickery
involving scanning for run_hook calls, but converting to an enum as
you've suggested previously sounds simpler.

Thanks,
Jonathan

Re: [PATCH] bugreport: collect list of populated hooks

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> Without a definitive list of hook names within the code, we compile our
> own list from the documentation. This is likely prone to bitrot. To
> reduce the amount of code humans need to read, we turn the list into a
> string_list and iterate over it (as we are calling the same find_hook
> operation on each string). However, since bugreport should primarily be
> called by the user, the performance loss from massaging the string
> seems acceptable.

In this iteration we no longer are collecting the hook names into
string list, but just formating the findings in a strbuf, no?

> @@ -33,6 +35,53 @@ static void get_system_info(struct strbuf *sys_info)
>  	get_libc_info(sys_info);
>  }
>  
> +static void get_populated_hooks(struct strbuf *hook_info, int nongit)
> +{
> +	/*
> +	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
> +	 * so below is a transcription of `git help hooks`. Later, this should
> +	 * be replaced with some programmatically generated list (generated from
> +	 * doc or else taken from some library which tells us about all the
> +	 * hooks)
> +	 */

Yes, I recall that we discussed adding some annotation to
documentation and extracting this automatically.

> +	const char *hook[] = {

Is it worth making this "static const"?

> +	if (nongit) {
> +		strbuf_addstr(hook_info,
> +			"not run from a git repository - no hooks to show\n");
> +		return;
> +	}
> +
> +	for (i=0; i < ARRAY_SIZE(hook); i++)

Style: SP around = in "i=0".

> +	get_header(&buffer, "Enabled Hooks");
> +	get_populated_hooks(&buffer, nongit_ok);
> +

Sounds good, otherwise.

Re: [PATCH] bugreport: collect list of populated hooks

[Emily Shaffer]

On Fri, Apr 24, 2020 at 09:52:07PM -0700, Junio C Hamano wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > Without a definitive list of hook names within the code, we compile our
> > own list from the documentation. This is likely prone to bitrot. To
> > reduce the amount of code humans need to read, we turn the list into a
> > string_list and iterate over it (as we are calling the same find_hook
> > operation on each string). However, since bugreport should primarily be
> > called by the user, the performance loss from massaging the string
> > seems acceptable.
> 
> In this iteration we no longer are collecting the hook names into
> string list, but just formating the findings in a strbuf, no?

Right - I'll fix the commit message for round 2.

> 
> > @@ -33,6 +35,53 @@ static void get_system_info(struct strbuf *sys_info)
> >  	get_libc_info(sys_info);
> >  }
> >  
> > +static void get_populated_hooks(struct strbuf *hook_info, int nongit)
> > +{
> > +	/*
> > +	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
> > +	 * so below is a transcription of `git help hooks`. Later, this should
> > +	 * be replaced with some programmatically generated list (generated from
> > +	 * doc or else taken from some library which tells us about all the
> > +	 * hooks)
> > +	 */
> 
> Yes, I recall that we discussed adding some annotation to
> documentation and extracting this automatically.

Right, we did. I think I was hesitant to move on it because I had
https://lore.kernel.org/git/20200420235310.94493-1-emilyshaffer@google.com
on my back burner and wasn't sure how the hook architecture would look
afterwards.

I'm not sure I know the right way to move forward (which is why I left a
NEEDSWORK) - my strongest preference is to leave it as is and wait for
the linked RFC and related work to land, at which point this code will
need a rework anyways. But if we're very interested in generating an
enum or something from the docs rather than letting this gross char**
land, I can do that too.

 - Emily

Re: [PATCH] bugreport: collect list of populated hooks

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

>> > +	/*
>> > +	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
>> > +	 * so below is a transcription of `git help hooks`. Later, this should
>> > +	 * be replaced with some programmatically generated list (generated from
>> > +	 * doc or else taken from some library which tells us about all the
>> > +	 * hooks)
>> > +	 */
>> 
>> Yes, I recall that we discussed adding some annotation to
>> documentation and extracting this automatically.
>
> Right, we did. I think I was hesitant to move on it because I had
> https://lore.kernel.org/git/20200420235310.94493-1-emilyshaffer@google.com
> on my back burner and wasn't sure how the hook architecture would look
> afterwards.

I thought we already agreed back then during that discussion I
recalled that we'd leave that "single source of truth" unification
out of the current topic.  Unless anything has changed recently to
bring us closer to be ready to start designing, I think it still is
fine to keep the needs-work comment above and punting.

Thanks.

[PATCH] bugreport: drop time.h include

[Emily Shaffer]

As pointed out in
https://lore.kernel.org/git/20200425003002.GC17217@google.com, we don't
need to include time.h explicitly because it is included (and possibly
managed for portability) by cache.h. Drop the include.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
Thanks for the observation, Jonathan.

 - Emily

 bugreport.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/bugreport.c b/bugreport.c
index 089b939a87..e4a7ed3a23 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -2,7 +2,6 @@
 #include "parse-options.h"
 #include "stdio.h"
 #include "strbuf.h"
-#include "time.h"
 #include "help.h"
 #include "compat/compiler.h"
 
-- 
2.26.2.303.gf8c07b1a785-goog

Re: [PATCH] bugreport: collect list of populated hooks

[Emily Shaffer]

On Mon, Apr 27, 2020 at 01:46:38PM -0700, Junio C Hamano wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> >> > +	/*
> >> > +	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
> >> > +	 * so below is a transcription of `git help hooks`. Later, this should
> >> > +	 * be replaced with some programmatically generated list (generated from
> >> > +	 * doc or else taken from some library which tells us about all the
> >> > +	 * hooks)
> >> > +	 */
> >> 
> >> Yes, I recall that we discussed adding some annotation to
> >> documentation and extracting this automatically.
> >
> > Right, we did. I think I was hesitant to move on it because I had
> > https://lore.kernel.org/git/20200420235310.94493-1-emilyshaffer@google.com
> > on my back burner and wasn't sure how the hook architecture would look
> > afterwards.
> 
> I thought we already agreed back then during that discussion I
> recalled that we'd leave that "single source of truth" unification
> out of the current topic.  Unless anything has changed recently to
> bring us closer to be ready to start designing, I think it still is
> fine to keep the needs-work comment above and punting.

Sure. Thanks.

Re: [PATCH] bugreport: drop time.h include

[Jonathan Nieder]

Emily Shaffer wrote:

> As pointed out in
> https://lore.kernel.org/git/20200425003002.GC17217@google.com,

This breadcrumb shouldn't be needed, since the rest of the commit
message already speaks for itself.  We can save the future "git log"
reader some time by leaing it out.

>                                                                we don't
> need to include time.h explicitly because it is included (and possibly
> managed for portability) by cache.h. Drop the include.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> Thanks for the observation, Jonathan.
>
>  - Emily
>
>  bugreport.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/bugreport.c b/bugreport.c
> index 089b939a87..e4a7ed3a23 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -2,7 +2,6 @@
>  #include "parse-options.h"
>  #include "stdio.h"
>  #include "strbuf.h"
> -#include "time.h"

Same applies to stdio.h, I believe.  Forgive my laziness in not being
exhaustive before.

Thanks,
Jonathan

Re: [PATCH] bugreport: drop time.h include

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Emily Shaffer wrote:
>
>> As pointed out in
>> https://lore.kernel.org/git/20200425003002.GC17217@google.com,
>
> This breadcrumb shouldn't be needed, since the rest of the commit
> message already speaks for itself.  We can save the future "git log"
> reader some time by leaing it out.

True.

Thanks.

Re: [PATCH] bugreport: drop time.h include

[Junio C Hamano]

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

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Emily Shaffer wrote:
>>
>>> As pointed out in
>>> https://lore.kernel.org/git/20200425003002.GC17217@google.com,
>>
>> This breadcrumb shouldn't be needed, since the rest of the commit
>> message already speaks for itself.  We can save the future "git log"
>> reader some time by leaing it out.
>
> True.

Well, removing these two lines made the rest non-sentence, so I had
to rewrite the thing.  I am not sure if the educational value warrants
the mention of compat/ exemption, but it people find it too noisy,
it can certainly be dropped.

Thanks.

-- >8 --
From: Emily Shaffer <emilyshaffer@google.com>
Date: Mon, 27 Apr 2020 13:48:59 -0700
Subject: [PATCH] bugreport: do not include <time.h>

In the generic parts of the source files, system headers like
<time.h> are supposed to be included indirectly by including
"git-compat-util.h", which manages portability issues (platform
specific compat/ sources are generally exempt from this rule).

Drop the inclusion.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 bugreport.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/bugreport.c b/bugreport.c
index 089b939a87..e4a7ed3a23 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -2,7 +2,6 @@
 #include "parse-options.h"
 #include "stdio.h"
 #include "strbuf.h"
-#include "time.h"
 #include "help.h"
 #include "compat/compiler.h"
 
-- 
2.26.2-266-ge870325ee8

Re: [PATCH] bugreport: drop time.h include

[Emily Shaffer]

On Mon, Apr 27, 2020 at 02:41:12PM -0700, Junio C Hamano wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Jonathan Nieder <jrnieder@gmail.com> writes:
> >
> >> Emily Shaffer wrote:
> >>
> >>> As pointed out in
> >>> https://lore.kernel.org/git/20200425003002.GC17217@google.com,
> >>
> >> This breadcrumb shouldn't be needed, since the rest of the commit
> >> message already speaks for itself.  We can save the future "git log"
> >> reader some time by leaing it out.
> >
> > True.
> 
> Well, removing these two lines made the rest non-sentence, so I had
> to rewrite the thing.  I am not sure if the educational value warrants
> the mention of compat/ exemption, but it people find it too noisy,
> it can certainly be dropped.

I've got a reroll to drop the "stdio.h" include too - do you want me to
send it? Your commit message is much nicer than what I came up with on
my end dropping the breadcrumb and generalizing to include stdio.h, so I
can adapt it if you're interested in the reroll.

> 
> Thanks.
> 
> -- >8 --
> 
> In the generic parts of the source files, system headers like
> <time.h> are supposed to be included indirectly by including
> "git-compat-util.h", which manages portability issues (platform
> specific compat/ sources are generally exempt from this rule).
> 
> Drop the inclusion.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  bugreport.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/bugreport.c b/bugreport.c
> index 089b939a87..e4a7ed3a23 100644
> --- a/bugreport.c
> +++ b/bugreport.c
> @@ -2,7 +2,6 @@
>  #include "parse-options.h"
>  #include "stdio.h"
>  #include "strbuf.h"
> -#include "time.h"
>  #include "help.h"
>  #include "compat/compiler.h"
>  
> -- 
> 2.26.2-266-ge870325ee8
> 

Re: [PATCH] bugreport: drop time.h include

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> On Mon, Apr 27, 2020 at 02:41:12PM -0700, Junio C Hamano wrote:
>> 
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> > Jonathan Nieder <jrnieder@gmail.com> writes:
>> >
>> >> Emily Shaffer wrote:
>> >>
>> >>> As pointed out in
>> >>> https://lore.kernel.org/git/20200425003002.GC17217@google.com,
>> >>
>> >> This breadcrumb shouldn't be needed, since the rest of the commit
>> >> message already speaks for itself.  We can save the future "git log"
>> >> reader some time by leaing it out.
>> >
>> > True.
>> 
>> Well, removing these two lines made the rest non-sentence, so I had
>> to rewrite the thing.  I am not sure if the educational value warrants
>> the mention of compat/ exemption, but it people find it too noisy,
>> it can certainly be dropped.
>
> I've got a reroll to drop the "stdio.h" include too - do you want me to
> send it? Your commit message is much nicer than what I came up with on
> my end dropping the breadcrumb and generalizing to include stdio.h, so I
> can adapt it if you're interested in the reroll.

Sure.  I already queued it and merged it to 'next', but the result
hasn't been pushed out and I am bisecting a test failure in some
newish topics that are merged to 'jch' recently, so I do not mind
taking a reroll and redoing 'next' before I can push it out today.

Thanks.

[PATCH v2] bugreport: collect list of populated hooks

[Emily Shaffer]

Occasionally a failure a user is seeing may be related to a specific
hook which is being run, perhaps without the user realizing. While the
contents of hooks can be sensitive - containing user data or process
information specific to the user's organization - simply knowing that a
hook is being run at a certain stage can help us to understand whether
something is going wrong.

Without a definitive list of hook names within the code, we compile our
own list from the documentation. This is likely prone to bitrot, but
designing a single source of truth for acceptable hooks is too much
overhead for this small change to the bugreport tool.

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

 - turned array of hooknames into a static const
 - added a test
 - i18n-ified the outputs added in this section
 - style changes

Thanks, all.
 - Emily

 Documentation/git-bugreport.txt |  1 +
 bugreport.c                     | 52 +++++++++++++++++++++++++++++++++
 t/t0091-bugreport.sh            | 10 +++++++
 3 files changed, 63 insertions(+)

diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
index 643d1b2884..7fe9aef34e 100644
--- a/Documentation/git-bugreport.txt
+++ b/Documentation/git-bugreport.txt
@@ -28,6 +28,7 @@ The following information is captured automatically:
  - 'git version --build-options'
  - uname sysname, release, version, and machine strings
  - Compiler-specific info string
+ - A list of enabled hooks
 
 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 089b939a87..81904b508e 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -5,6 +5,8 @@
 #include "time.h"
 #include "help.h"
 #include "compat/compiler.h"
+#include "run-command.h"
+
 
 static void get_system_info(struct strbuf *sys_info)
 {
@@ -33,6 +35,53 @@ static void get_system_info(struct strbuf *sys_info)
 	get_libc_info(sys_info);
 }
 
+static void get_populated_hooks(struct strbuf *hook_info, int nongit)
+{
+	/*
+	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
+	 * so below is a transcription of `git help hooks`. Later, this should
+	 * be replaced with some programmatically generated list (generated from
+	 * doc or else taken from some library which tells us about all the
+	 * hooks)
+	 */
+	static const char *hook[] = {
+		"applypatch-msg",
+		"pre-applypatch",
+		"post-applypatch",
+		"pre-commit",
+		"pre-merge-commit",
+		"prepare-commit-msg",
+		"commit-msg",
+		"post-commit",
+		"pre-rebase",
+		"post-checkout",
+		"post-merge",
+		"pre-push",
+		"pre-receive",
+		"update",
+		"post-receive",
+		"post-update",
+		"push-to-checkout",
+		"pre-auto-gc",
+		"post-rewrite",
+		"sendemail-validate",
+		"fsmonitor-watchman",
+		"p4-pre-submit",
+		"post-index-change",
+	};
+	int i;
+
+	if (nongit) {
+		strbuf_addstr(hook_info,
+			_("not run from a git repository - no hooks to show\n"));
+		return;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(hook); i++)
+		if (find_hook(hook[i]))
+			strbuf_addf(hook_info, "%s\n", hook[i]);
+}
+
 static const char * const bugreport_usage[] = {
 	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
 	NULL
@@ -116,6 +165,9 @@ int cmd_main(int argc, const char **argv)
 	get_header(&buffer, _("System Info"));
 	get_system_info(&buffer);
 
+	get_header(&buffer, _("Enabled Hooks"));
+	get_populated_hooks(&buffer, nongit_ok);
+
 	/* 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/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index 2e73658a5c..387fa46c3f 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' '
 	nongit git bugreport -o foo/bar/baz
 '
 
+test_expect_success 'indicates populated hooks' '
+	test_when_finished rm git-bugreport-hooks.txt &&
+	test_when_finished rm -fr .git/hooks &&
+	mkdir .git/hooks &&
+	touch .git/hooks/applypatch-msg &&
+	chmod +x .git/hooks/applypatch-msg &&
+	git bugreport -s hooks &&
+	test_i18ngrep applypatch-msg git-bugreport-hooks.txt
+'
+
 
 test_done
-- 
2.26.2.303.gf8c07b1a785-goog

[PATCH v2] bugreport: drop extraneous includes

[Emily Shaffer]

In the generic parts of the source files, system headers like <time.h>
and <stdio.h> are supposed to be included indirectly bt including
"git-compat-util.h", which manages portability issues.

Drop our explicit inclusions and rely on "cache.h", which includes
"git-compat-util.h".

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 bugreport.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/bugreport.c b/bugreport.c
index 089b939a87..acacca8fef 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -1,8 +1,6 @@
 #include "cache.h"
 #include "parse-options.h"
-#include "stdio.h"
 #include "strbuf.h"
-#include "time.h"
 #include "help.h"
 #include "compat/compiler.h"
 
-- 
2.26.2.303.gf8c07b1a785-goog

Re: [PATCH v2] bugreport: collect list of populated hooks

[Jonathan Nieder]

Emily Shaffer wrote:

> --- a/t/t0091-bugreport.sh
> +++ b/t/t0091-bugreport.sh
> @@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' '
>  	nongit git bugreport -o foo/bar/baz
>  '
>  
> +test_expect_success 'indicates populated hooks' '
> +	test_when_finished rm git-bugreport-hooks.txt &&
> +	test_when_finished rm -fr .git/hooks &&
> +	mkdir .git/hooks &&
> +	touch .git/hooks/applypatch-msg &&
> +	chmod +x .git/hooks/applypatch-msg &&

optional: could use write_script for this

> +	git bugreport -s hooks &&
> +	test_i18ngrep applypatch-msg git-bugreport-hooks.txt

This should work even when translated, so it can use "grep" instead of
test_i18ngrep.

A few more things to test:
- that it doesn't include hooks we *haven't* installed. :)
- that it isn't confused by the default *.sample hooks
- what happens when outside a git repository?

Even without such tests,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH v2] bugreport: drop extraneous includes

[Jonathan Nieder]

Emily Shaffer wrote:

> In the generic parts of the source files, system headers like <time.h>
> and <stdio.h> are supposed to be included indirectly bt including

s/bt/by/

> "git-compat-util.h", which manages portability issues.
>
> Drop our explicit inclusions and rely on "cache.h", which includes
> "git-compat-util.h".
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  bugreport.c | 2 --
>  1 file changed, 2 deletions(-)

With that tweak,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH v2] bugreport: collect list of populated hooks

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Emily Shaffer wrote:
>
>> --- a/t/t0091-bugreport.sh
>> +++ b/t/t0091-bugreport.sh
>> @@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' '
>>  	nongit git bugreport -o foo/bar/baz
>>  '
>>  
>> +test_expect_success 'indicates populated hooks' '
>> +	test_when_finished rm git-bugreport-hooks.txt &&
>> +	test_when_finished rm -fr .git/hooks &&
>> +	mkdir .git/hooks &&
>> +	touch .git/hooks/applypatch-msg &&
>> +	chmod +x .git/hooks/applypatch-msg &&
>
> optional: could use write_script for this

Yup, it is a good practice to do so.  Use of touch is especially bad
here, as it is quite plausible for us to later change the "is the
hook there?" check to exclude a completely empty file.

>> +	git bugreport -s hooks &&
>> +	test_i18ngrep applypatch-msg git-bugreport-hooks.txt
>
> This should work even when translated, so it can use "grep" instead of
> test_i18ngrep.

Nicely spotted.

Thanks.

Re: [PATCH v2] bugreport: collect list of populated hooks

[Emily Shaffer]

On Mon, Apr 27, 2020 at 04:45:10PM -0700, Jonathan Nieder wrote:
> 
> Emily Shaffer wrote:
> 
> > --- a/t/t0091-bugreport.sh
> > +++ b/t/t0091-bugreport.sh
> > @@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' '
> >  	nongit git bugreport -o foo/bar/baz
> >  '
> >  
> > +test_expect_success 'indicates populated hooks' '
> > +	test_when_finished rm git-bugreport-hooks.txt &&
> > +	test_when_finished rm -fr .git/hooks &&
> > +	mkdir .git/hooks &&
> > +	touch .git/hooks/applypatch-msg &&
> > +	chmod +x .git/hooks/applypatch-msg &&
> 
> optional: could use write_script for this

ACK, especially given Junio's comment in reply here.

> 
> > +	git bugreport -s hooks &&
> > +	test_i18ngrep applypatch-msg git-bugreport-hooks.txt
> 
> This should work even when translated, so it can use "grep" instead of
> test_i18ngrep.
> 
> A few more things to test:
> - that it doesn't include hooks we *haven't* installed. :)
> - that it isn't confused by the default *.sample hooks

I'll lump these together by adding a .sample hook and ensuring that hook
was excluded.

> - what happens when outside a git repository?

The earlier test to make sure it doesn't crash seems satisfactory to me
- is there some other behavior you'd like to ensure? I'm not keen to
check the lines below the header are empty, since the spacing there
could change and break an unrelated test.

Reroll coming today. These changes are straightforward and passing for
me locally.

 - Emily

[PATCH v3] bugreport: collect list of populated hooks

[Emily Shaffer]

Occasionally a failure a user is seeing may be related to a specific
hook which is being run, perhaps without the user realizing. While the
contents of hooks can be sensitive - containing user data or process
information specific to the user's organization - simply knowing that a
hook is being run at a certain stage can help us to understand whether
something is going wrong.

Without a definitive list of hook names within the code, we compile our
own list from the documentation. This is likely prone to bitrot, but
designing a single source of truth for acceptable hooks is too much
overhead for this small change to the bugreport tool.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
Since v2, switched to write_script, and modified the test to check for
uninstalled hooks and sample hooks having correct behavior.

 Documentation/git-bugreport.txt |  1 +
 bugreport.c                     | 52 +++++++++++++++++++++++++++++++++
 t/t0091-bugreport.sh            | 10 +++++++
 3 files changed, 63 insertions(+)

diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
index 643d1b2884..7fe9aef34e 100644
--- a/Documentation/git-bugreport.txt
+++ b/Documentation/git-bugreport.txt
@@ -28,6 +28,7 @@ The following information is captured automatically:
  - 'git version --build-options'
  - uname sysname, release, version, and machine strings
  - Compiler-specific info string
+ - A list of enabled hooks
 
 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 acacca8fef..aa8a489c35 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -3,6 +3,8 @@
 #include "strbuf.h"
 #include "help.h"
 #include "compat/compiler.h"
+#include "run-command.h"
+
 
 static void get_system_info(struct strbuf *sys_info)
 {
@@ -31,6 +33,53 @@ static void get_system_info(struct strbuf *sys_info)
 	get_libc_info(sys_info);
 }
 
+static void get_populated_hooks(struct strbuf *hook_info, int nongit)
+{
+	/*
+	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
+	 * so below is a transcription of `git help hooks`. Later, this should
+	 * be replaced with some programmatically generated list (generated from
+	 * doc or else taken from some library which tells us about all the
+	 * hooks)
+	 */
+	static const char *hook[] = {
+		"applypatch-msg",
+		"pre-applypatch",
+		"post-applypatch",
+		"pre-commit",
+		"pre-merge-commit",
+		"prepare-commit-msg",
+		"commit-msg",
+		"post-commit",
+		"pre-rebase",
+		"post-checkout",
+		"post-merge",
+		"pre-push",
+		"pre-receive",
+		"update",
+		"post-receive",
+		"post-update",
+		"push-to-checkout",
+		"pre-auto-gc",
+		"post-rewrite",
+		"sendemail-validate",
+		"fsmonitor-watchman",
+		"p4-pre-submit",
+		"post-index-change",
+	};
+	int i;
+
+	if (nongit) {
+		strbuf_addstr(hook_info,
+			_("not run from a git repository - no hooks to show\n"));
+		return;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(hook); i++)
+		if (find_hook(hook[i]))
+			strbuf_addf(hook_info, "%s\n", hook[i]);
+}
+
 static const char * const bugreport_usage[] = {
 	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
 	NULL
@@ -114,6 +163,9 @@ int cmd_main(int argc, const char **argv)
 	get_header(&buffer, _("System Info"));
 	get_system_info(&buffer);
 
+	get_header(&buffer, _("Enabled Hooks"));
+	get_populated_hooks(&buffer, nongit_ok);
+
 	/* 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/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index 2e73658a5c..612c12a918 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' '
 	nongit git bugreport -o foo/bar/baz
 '
 
+test_expect_success 'indicates populated hooks' '
+	test_when_finished rm git-bugreport-hooks.txt &&
+	test_when_finished rm -fr .git/hooks &&
+	mkdir .git/hooks &&
+	write_script .git/hooks/applypatch-msg &&
+	write_script .git/hooks/prepare-commit-msg.sample &&
+	git bugreport -s hooks &&
+	grep applypatch-msg git-bugreport-hooks.txt &&
+	! grep prepare-commit-msg git-bugreport-hooks.txt
+'
 
 test_done
-- 
2.26.2.303.gf8c07b1a785-goog

Re: [PATCH v3] bugreport: collect list of populated hooks

[Jonathan Nieder]

Emily Shaffer wrote:

> Since v2, switched to write_script, and modified the test to check for
> uninstalled hooks and sample hooks having correct behavior.

Looks almost ready.  Two nits.

[...]
> --- a/t/t0091-bugreport.sh
> +++ b/t/t0091-bugreport.sh
> @@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' '
>  	nongit git bugreport -o foo/bar/baz
>  '
>  
> +test_expect_success 'indicates populated hooks' '
> +	test_when_finished rm git-bugreport-hooks.txt &&
> +	test_when_finished rm -fr .git/hooks &&
> +	mkdir .git/hooks &&

This is subtle.  Since c09a69a83e3 (Disable hooks during tests,
2005-10-16), the repository in which the test runs has its "hooks"
directory renamed to "hooks-disabled", with rationale

    Individual tests for hooks would want to have their own tests when
    written.  Also we should not pick up from random templates the user
    happens to have.

That rationale is a bit strange because we explicitly passed
--template to "git init" even then, so we could be confident that the
built-in templates that do not enable any hooks would be in use.

Mailing list diving finds [1].  We didn't have f98f8cbac01 (Ship
sample hooks with .sample suffix, 2008-06-24) yet, which means that on
filesystems that do not record an executable bit, the sample hooks
were all enabled by default.  Nowadays that has been fixed and we
should be able to get rid of the "mv .git/hooks .git/hooks-disabled"
in the test setup.

When we do that, this "mkdir .git/hooks" will fail because the
directory already exists.  Ideas:

 A. Include a preparatory patch in this series that removes that "mv"
    command.  That way, this test can do

	test_when_finished "
		rm -fr .git/hooks &&
		mv .git/hooks-saved .git/hooks
	" &&
	mv .git/hooks .git/hooks-saved &&
	mkdir .git/hooks &&
	...

    or even better (because of increased realism)

	test_when_finished rm .git/hooks/applypatch-msg &&
	write_script .git/hooks/applypatch-msg <<-\EOF &&
	...

  B. Run "git init" ourselves so we know what we're getting:

  	test_when_finished "rm -fr .git && mv .git-saved .git" &&
	mv .git .git-saved &&
	git init &&
	...

> +	write_script .git/hooks/applypatch-msg &&

write_script looks for a script on its stdin.  test_eval_ redirects
stdin to come from /dev/null, so we happen to get an empty script, but
this is subtle.  How about something like

	write_script .git/hooks/applypatch-msg <<-\EOF &&
	echo >&2 "rejecting message in $1"
	exit 1
	EOF

or

	write_script .git/hooks/applypatch-msg </dev/null &&

?

> +	write_script .git/hooks/prepare-commit-msg.sample &&

Likewise.

> +	git bugreport -s hooks &&
> +	grep applypatch-msg git-bugreport-hooks.txt &&
> +	! grep prepare-commit-msg git-bugreport-hooks.txt
> +'

Thanks,
Jonathan

Re: [PATCH v3] bugreport: collect list of populated hooks

[Jonathan Nieder]

Jonathan Nieder wrote:
> Emily Shaffer wrote:

>> +++ b/t/t0091-bugreport.sh
>> @@ -57,5 +57,15 @@ test_expect_success 'can create leading directories outside of a git dir' '
>>  	nongit git bugreport -o foo/bar/baz
>>  '
>>  
>> +test_expect_success 'indicates populated hooks' '
>> +	test_when_finished rm git-bugreport-hooks.txt &&
>> +	test_when_finished rm -fr .git/hooks &&
>> +	mkdir .git/hooks &&
>
> This is subtle.  Since c09a69a83e3 (Disable hooks during tests,
> 2005-10-16), the repository in which the test runs has its "hooks"
> directory renamed to "hooks-disabled", with rationale
>
>     Individual tests for hooks would want to have their own tests when
>     written.  Also we should not pick up from random templates the user
>     happens to have.
>
> That rationale is a bit strange because we explicitly passed
> --template to "git init" even then, so we could be confident that the
> built-in templates that do not enable any hooks would be in use.
>
> Mailing list diving finds [1].  We didn't have f98f8cbac01 (Ship
> sample hooks with .sample suffix, 2008-06-24) yet, which means that on
> filesystems that do not record an executable bit, the sample hooks
> were all enabled by default.  Nowadays that has been fixed and we
> should be able to get rid of the "mv .git/hooks .git/hooks-disabled"
> in the test setup.

And of course I forget to include the footnote.  Sorry for the noise.

Jonathan

[1] https://lore.kernel.org/git/81b0412b0510140546ya10bc8fg3dd5eaab429eba6f@mail.gmail.com/

Re: [PATCH v3] bugreport: collect list of populated hooks

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> When we do that, this "mkdir .git/hooks" will fail because the
> directory already exists.  Ideas:
>
>  A. Include a preparatory patch in this series that removes that "mv"
>     command.  That way, this test can do

While I do not think it is realistic to anticipate that the "test"
repository may someday come with a hooks/ directory, even if we did
so, we would not enable any hook by default in there.  So "move away
and restore" feels way overkill.

>   B. Run "git init" ourselves so we know what we're getting:

That is certainly safer, and simpler.  But perhaps the simplest
would be

    C. Use "mkdir -p .git/hooks" so we won't get affected.

>> +	write_script .git/hooks/applypatch-msg &&
>
> write_script looks for a script on its stdin.  test_eval_ redirects
> stdin to come from /dev/null, so we happen to get an empty script, but
> this is subtle.  How about something like
>
> 	write_script .git/hooks/applypatch-msg <<-\EOF &&
> 	echo >&2 "rejecting message in $1"
> 	exit 1
> 	EOF

Yes, that is good.

> or
>
> 	write_script .git/hooks/applypatch-msg </dev/null &&

This takes us back to the resuling "empty" hook we wanted to avoid
by switching from "use touch to create something" to "write some
meaningful contents" approach, no?

>> +	git bugreport -s hooks &&
>> +	grep applypatch-msg git-bugreport-hooks.txt &&
>> +	! grep prepare-commit-msg git-bugreport-hooks.txt
>> +'
>
> Thanks,

Thanks.

Re: [PATCH v3] bugreport: collect list of populated hooks

[Junio C Hamano]

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

>> When we do that, this "mkdir .git/hooks" will fail because the
>> directory already exists.  Ideas:
>>
>>  A. Include a preparatory patch in this series that removes that "mv"
>>     command.  That way, this test can do
>
> While I do not think it is realistic to anticipate that the "test"
> repository may someday come with a hooks/ directory, even if we did
> so, we would not enable any hook by default in there.  So "move away
> and restore" feels way overkill.
>
>>   B. Run "git init" ourselves so we know what we're getting:
>
> That is certainly safer, and simpler.  But perhaps the simplest
> would be
>
>     C. Use "mkdir -p .git/hooks" so we won't get affected.

In the meantime, I added this SQUASH on top.  I do not claim that
this is the best solution, but the idea is to refuse to be affected
by what is left in .git/hooks either by the test framework or
earlier tests in the same test script file.

 t/t0091-bugreport.sh | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index 612c12a918..9450cc02e3 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -58,11 +58,14 @@ test_expect_success 'can create leading directories outside of a git dir' '
 '
 
 test_expect_success 'indicates populated hooks' '
-	test_when_finished rm git-bugreport-hooks.txt &&
-	test_when_finished rm -fr .git/hooks &&
+	rm -fr .git/hooks &&
 	mkdir .git/hooks &&
-	write_script .git/hooks/applypatch-msg &&
-	write_script .git/hooks/prepare-commit-msg.sample &&
+	for hook in applypatch-msg prepare-commit-msg.sample
+	do
+		write_script ".git/hooks/$hook" <<-\EOF || return 1
+		echo "hook $hook exists"
+		EOF
+	done &&
 	git bugreport -s hooks &&
 	grep applypatch-msg git-bugreport-hooks.txt &&
 	! grep prepare-commit-msg git-bugreport-hooks.txt
-- 
2.26.2-447-gd61d20c9b4

Re: [PATCH v3] bugreport: collect list of populated hooks

[Emily Shaffer]

On Thu, Apr 30, 2020 at 03:09:55PM -0700, Junio C Hamano wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> When we do that, this "mkdir .git/hooks" will fail because the
> >> directory already exists.  Ideas:
> >>
> >>  A. Include a preparatory patch in this series that removes that "mv"
> >>     command.  That way, this test can do
> >
> > While I do not think it is realistic to anticipate that the "test"
> > repository may someday come with a hooks/ directory, even if we did
> > so, we would not enable any hook by default in there.  So "move away
> > and restore" feels way overkill.
> >
> >>   B. Run "git init" ourselves so we know what we're getting:
> >
> > That is certainly safer, and simpler.  But perhaps the simplest
> > would be
> >
> >     C. Use "mkdir -p .git/hooks" so we won't get affected.
> 
> In the meantime, I added this SQUASH on top.  I do not claim that
> this is the best solution, but the idea is to refuse to be affected
> by what is left in .git/hooks either by the test framework or
> earlier tests in the same test script file.

Thanks for the patience with the reply - like many of us, my attention
is being pulled many directions right now :)

This solution looks very neat to me, minus one comment. I can send a
squash with the change I mention below.

 - Emily

> 
>  t/t0091-bugreport.sh | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> index 612c12a918..9450cc02e3 100755
> --- a/t/t0091-bugreport.sh
> +++ b/t/t0091-bugreport.sh
> @@ -58,11 +58,14 @@ test_expect_success 'can create leading directories outside of a git dir' '
>  '
>  
>  test_expect_success 'indicates populated hooks' '
> -	test_when_finished rm git-bugreport-hooks.txt &&
> -	test_when_finished rm -fr .git/hooks &&

I'm not sure it's necessary to lose these two lines. Especially the
generated bugreport I'd like to clean up.

> +	rm -fr .git/hooks &&
>  	mkdir .git/hooks &&
> -	write_script .git/hooks/applypatch-msg &&
> -	write_script .git/hooks/prepare-commit-msg.sample &&
> +	for hook in applypatch-msg prepare-commit-msg.sample
> +	do
> +		write_script ".git/hooks/$hook" <<-\EOF || return 1
> +		echo "hook $hook exists"
> +		EOF
> +	done &&

I like this placeholder script a lot.

>  	git bugreport -s hooks &&
>  	grep applypatch-msg git-bugreport-hooks.txt &&
>  	! grep prepare-commit-msg git-bugreport-hooks.txt
> -- 
> 2.26.2-447-gd61d20c9b4
> 

Re: [PATCH v3] bugreport: collect list of populated hooks

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

>>  test_expect_success 'indicates populated hooks' '
>> -	test_when_finished rm git-bugreport-hooks.txt &&
>> -	test_when_finished rm -fr .git/hooks &&
>
> I'm not sure it's necessary to lose these two lines. Especially the
> generated bugreport I'd like to clean up.

I do not care either way, actually.  I left it so that it would be
easier to debug the test by looking at the output.

>> +	rm -fr .git/hooks &&
>>  	mkdir .git/hooks &&
>> -	write_script .git/hooks/applypatch-msg &&
>> -	write_script .git/hooks/prepare-commit-msg.sample &&
>> +	for hook in applypatch-msg prepare-commit-msg.sample
>> +	do
>> +		write_script ".git/hooks/$hook" <<-\EOF || return 1
>> +		echo "hook $hook exists"
>> +		EOF
>> +	done &&
>
> I like this placeholder script a lot.

I actually don't.  At least the final version should not quote EOF
(otherwise $hook will appear verbatim).

>>  	git bugreport -s hooks &&
>>  	grep applypatch-msg git-bugreport-hooks.txt &&
>>  	! grep prepare-commit-msg git-bugreport-hooks.txt
>> -- 
>> 2.26.2-447-gd61d20c9b4
>> 

Thanks.

[PATCH v4] bugreport: collect list of populated hooks

[Emily Shaffer]

Occasionally a failure a user is seeing may be related to a specific
hook which is being run, perhaps without the user realizing. While the
contents of hooks can be sensitive - containing user data or process
information specific to the user's organization - simply knowing that a
hook is being run at a certain stage can help us to understand whether
something is going wrong.

Without a definitive list of hook names within the code, we compile our
own list from the documentation. This is likely prone to bitrot, but
designing a single source of truth for acceptable hooks is too much
overhead for this small change to the bugreport tool.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
Since v3, squashed Junio's suggestion
(https://lore.kernel.org/git/xmqqwo5wpqvg.fsf@gitster.c.googlers.com)
but kept the test_when_finished cleanup lines to try to avoid leaving
junk for later tests.

CI: https://github.com/nasamuffin/git/actions/runs/98568890, which seems
like it might be flaky. Have failed a couple because of downloads timing
out... but I'm on top of next from a while ago, so I might be missing
some extra Actions topics?

 - Emily

 Documentation/git-bugreport.txt |  1 +
 bugreport.c                     | 52 +++++++++++++++++++++++++++++++++
 t/t0091-bugreport.sh            | 15 ++++++++++
 3 files changed, 68 insertions(+)

diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
index 643d1b2884..7fe9aef34e 100644
--- a/Documentation/git-bugreport.txt
+++ b/Documentation/git-bugreport.txt
@@ -28,6 +28,7 @@ The following information is captured automatically:
  - 'git version --build-options'
  - uname sysname, release, version, and machine strings
  - Compiler-specific info string
+ - A list of enabled hooks
 
 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 acacca8fef..aa8a489c35 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -3,6 +3,8 @@
 #include "strbuf.h"
 #include "help.h"
 #include "compat/compiler.h"
+#include "run-command.h"
+
 
 static void get_system_info(struct strbuf *sys_info)
 {
@@ -31,6 +33,53 @@ static void get_system_info(struct strbuf *sys_info)
 	get_libc_info(sys_info);
 }
 
+static void get_populated_hooks(struct strbuf *hook_info, int nongit)
+{
+	/*
+	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
+	 * so below is a transcription of `git help hooks`. Later, this should
+	 * be replaced with some programmatically generated list (generated from
+	 * doc or else taken from some library which tells us about all the
+	 * hooks)
+	 */
+	static const char *hook[] = {
+		"applypatch-msg",
+		"pre-applypatch",
+		"post-applypatch",
+		"pre-commit",
+		"pre-merge-commit",
+		"prepare-commit-msg",
+		"commit-msg",
+		"post-commit",
+		"pre-rebase",
+		"post-checkout",
+		"post-merge",
+		"pre-push",
+		"pre-receive",
+		"update",
+		"post-receive",
+		"post-update",
+		"push-to-checkout",
+		"pre-auto-gc",
+		"post-rewrite",
+		"sendemail-validate",
+		"fsmonitor-watchman",
+		"p4-pre-submit",
+		"post-index-change",
+	};
+	int i;
+
+	if (nongit) {
+		strbuf_addstr(hook_info,
+			_("not run from a git repository - no hooks to show\n"));
+		return;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(hook); i++)
+		if (find_hook(hook[i]))
+			strbuf_addf(hook_info, "%s\n", hook[i]);
+}
+
 static const char * const bugreport_usage[] = {
 	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
 	NULL
@@ -114,6 +163,9 @@ int cmd_main(int argc, const char **argv)
 	get_header(&buffer, _("System Info"));
 	get_system_info(&buffer);
 
+	get_header(&buffer, _("Enabled Hooks"));
+	get_populated_hooks(&buffer, nongit_ok);
+
 	/* 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/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index 2e73658a5c..ff317cce68 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -57,5 +57,20 @@ test_expect_success 'can create leading directories outside of a git dir' '
 	nongit git bugreport -o foo/bar/baz
 '
 
+test_expect_success 'indicates populated hooks' '
+	test_when_finished rm git-bugreport-hooks.txt &&
+	test_when_finished rm -fr .git/hooks &&
+	rm -fr .git/hooks &&
+	mkdir .git/hooks &&
+	for hook in applypatch-msg prepare-commit-msg.sample
+	do
+		write_script ".git/hooks/$hook" <<-\EOF || return 1
+		echo "hook $hook exists"
+		EOF
+	done &&
+	git bugreport -s hooks &&
+	grep applypatch-msg git-bugreport-hooks.txt &&
+	! grep prepare-commit-msg git-bugreport-hooks.txt
+'
 
 test_done
-- 
2.26.2.645.ge9eca65c58-goog

Re: [PATCH v4] bugreport: collect list of populated hooks

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> index 2e73658a5c..ff317cce68 100755
> --- a/t/t0091-bugreport.sh
> +++ b/t/t0091-bugreport.sh
> @@ -57,5 +57,20 @@ test_expect_success 'can create leading directories outside of a git dir' '
>  	nongit git bugreport -o foo/bar/baz
>  '
>  
> +test_expect_success 'indicates populated hooks' '
> +	test_when_finished rm git-bugreport-hooks.txt &&
> +	test_when_finished rm -fr .git/hooks &&
> +	rm -fr .git/hooks &&
> +	mkdir .git/hooks &&
> +	for hook in applypatch-msg prepare-commit-msg.sample
> +	do
> +		write_script ".git/hooks/$hook" <<-\EOF || return 1
> +		echo "hook $hook exists"
> +		EOF

Probably our mails crossed, but as I said in my response, this will
make the hook say "hook  exists" because $hook will be literally in
the script file.  EOF needs to be left unquoted, i.e.

		write_script ".git/hooks/$hook" <<-EOF || return 1
		...

> +	done &&
> +	...

Thanks.

Re: [PATCH v4] bugreport: collect list of populated hooks

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

On 2020-05-07 17:53:57-0700, Emily Shaffer <emilyshaffer@google.com> wrote:
> +test_expect_success 'indicates populated hooks' '
> +	test_when_finished rm git-bugreport-hooks.txt &&
> +	test_when_finished rm -fr .git/hooks &&
> +	rm -fr .git/hooks &&
> +	mkdir .git/hooks &&
> +	for hook in applypatch-msg prepare-commit-msg.sample
> +	do
> +		write_script ".git/hooks/$hook" <<-\EOF || return 1
> +		echo "hook $hook exists"
> +		EOF
> +	done &&
> +	git bugreport -s hooks &&
> +	grep applypatch-msg git-bugreport-hooks.txt &&
> +	! grep prepare-commit-msg git-bugreport-hooks.txt

Hi Emily,

I think this is a bit more correct test.
---------8<----------
From: =?UTF-8?q?=C4=90o=C3=A0n=20Tr=E1=BA=A7n=20C=C3=B4ng=20Danh?=
 <congdanhqx@gmail.com>
Date: Fri, 8 May 2020 08:26:53 +0700
Subject: [PATCH] fixup! bugreport: collect list of populated hooks
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Use an exact match to check for populated hooks

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t0091-bugreport.sh | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index 9450cc02e3..789e8f1ac7 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -67,8 +67,13 @@ test_expect_success 'indicates populated hooks' '
 		EOF
 	done &&
 	git bugreport -s hooks &&
-	grep applypatch-msg git-bugreport-hooks.txt &&
-	! grep prepare-commit-msg git-bugreport-hooks.txt
+	cat <<-\EOF >expected &&
+	applypatch-msg
+
+	EOF
+	awk -F "]\\n" -v RS="[" "/applypatch-msg/{print \$2}" \
+		git-bugreport-hooks.txt >actual &&
+	test_cmp expected actual
 '
 
 test_done
-- 
2.26.2.672.g232c24e857


-- 
Danh

Re: [PATCH v4] bugreport: collect list of populated hooks

[Emily Shaffer]

On Fri, May 08, 2020 at 08:34:05AM +0700, Đoàn Trần Công Danh wrote:
> 
> On 2020-05-07 17:53:57-0700, Emily Shaffer <emilyshaffer@google.com> wrote:
> > +test_expect_success 'indicates populated hooks' '
> > +	test_when_finished rm git-bugreport-hooks.txt &&
> > +	test_when_finished rm -fr .git/hooks &&
> > +	rm -fr .git/hooks &&
> > +	mkdir .git/hooks &&
> > +	for hook in applypatch-msg prepare-commit-msg.sample
> > +	do
> > +		write_script ".git/hooks/$hook" <<-\EOF || return 1
> > +		echo "hook $hook exists"
> > +		EOF
> > +	done &&
> > +	git bugreport -s hooks &&
> > +	grep applypatch-msg git-bugreport-hooks.txt &&
> > +	! grep prepare-commit-msg git-bugreport-hooks.txt
> 
> Hi Emily,
> 
> I think this is a bit more correct test.
> ---------8<----------
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Use an exact match to check for populated hooks
> 
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  t/t0091-bugreport.sh | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> index 9450cc02e3..789e8f1ac7 100755
> --- a/t/t0091-bugreport.sh
> +++ b/t/t0091-bugreport.sh
> @@ -67,8 +67,13 @@ test_expect_success 'indicates populated hooks' '
>  		EOF
>  	done &&
>  	git bugreport -s hooks &&
> -	grep applypatch-msg git-bugreport-hooks.txt &&
> -	! grep prepare-commit-msg git-bugreport-hooks.txt
> +	cat <<-\EOF >expected &&
> +	applypatch-msg
> +
> +	EOF
> +	awk -F "]\\n" -v RS="[" "/applypatch-msg/{print \$2}" \
> +		git-bugreport-hooks.txt >actual &&

If I understand correctly, you are saying "look for a line like [...]
which is followed by a line that says 'applypatch-msg'" - that is,
making sure that you don't see some false positive should
"applypatch-msg" exist in the rest of the bugreport.

Could we compromise and grep for "^applypatch-msg$", e.g. "there is a
line calling out applypatch-msg in some way that isn't in the context of
the report template"?

- Even though regex magic is used, ^$ are beginner regex that many
  people can understand easily.
- Using grep for a single line means we are not allergic to header
  format changes later on

It doesn't search for false positives, true, but I found your awk
suggestion hard to understand - my impression is that awk is less
commonly understood, even among Git contributors.

In sed, it's a little bit more readable:

  cat <<-\EOF >expected &&
  [Enabled Hooks]
  applypatch-msg

  EOF

  sed -n '/\[Enabled Hooks\]/, /^$/ p' git-bugreport-hooks.txt >actual
  test_cmp expected actual

But, I don't like this because it relies on the name of the header, and
the newline spacing between sections - and would be nontrivial to change
if we decided to underline headers instead, or add a newline between a
header and its contents. So I think it may be overkill.

Thanks for your suggestion, though.

 - Emily

Re: [PATCH v3] bugreport: collect list of populated hooks

[Emily Shaffer]

On Thu, May 07, 2020 at 04:06:14PM -0700, Junio C Hamano wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> >>  test_expect_success 'indicates populated hooks' '
> >> -	test_when_finished rm git-bugreport-hooks.txt &&
> >> -	test_when_finished rm -fr .git/hooks &&
> >
> > I'm not sure it's necessary to lose these two lines. Especially the
> > generated bugreport I'd like to clean up.
> 
> I do not care either way, actually.  I left it so that it would be
> easier to debug the test by looking at the output.

That's a good point, although I was worried about the junk left behind
for other tests. I think I'd rather delete the junk, as people should
still be able to debug the test with "test_pause &&" and see the
contents of the temporary hook directory (verified locally).

> 
> >> +	rm -fr .git/hooks &&
> >>  	mkdir .git/hooks &&
> >> -	write_script .git/hooks/applypatch-msg &&
> >> -	write_script .git/hooks/prepare-commit-msg.sample &&
> >> +	for hook in applypatch-msg prepare-commit-msg.sample
> >> +	do
> >> +		write_script ".git/hooks/$hook" <<-\EOF || return 1
> >> +		echo "hook $hook exists"
> >> +		EOF
> >> +	done &&
> >
> > I like this placeholder script a lot.
> 
> I actually don't.  At least the final version should not quote EOF
> (otherwise $hook will appear verbatim).

ACK.

New patch inbound as discussed in standup.
 - Emily

[PATCH v5] bugreport: collect list of populated hooks

[Emily Shaffer]

Occasionally a failure a user is seeing may be related to a specific
hook which is being run, perhaps without the user realizing. While the
contents of hooks can be sensitive - containing user data or process
information specific to the user's organization - simply knowing that a
hook is being run at a certain stage can help us to understand whether
something is going wrong.

Without a definitive list of hook names within the code, we compile our
own list from the documentation. This is likely prone to bitrot, but
designing a single source of truth for acceptable hooks is too much
overhead for this small change to the bugreport tool.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
The only change since v4 is to uncomment the heredoc, meaning the hook
names will be correctly injected into the test hooks.

I chose not to implement Danh's suggestion of multiline grep:
https://lore.kernel.org/git/20200511212205.GI77802@google.com

Tests passed:
https://github.com/nasamuffin/git/actions/runs/101874479

Thanks, all.
 - Emily

 Documentation/git-bugreport.txt |  1 +
 bugreport.c                     | 52 +++++++++++++++++++++++++++++++++
 t/t0091-bugreport.sh            | 15 ++++++++++
 3 files changed, 68 insertions(+)

diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
index 643d1b2884..7fe9aef34e 100644
--- a/Documentation/git-bugreport.txt
+++ b/Documentation/git-bugreport.txt
@@ -28,6 +28,7 @@ The following information is captured automatically:
  - 'git version --build-options'
  - uname sysname, release, version, and machine strings
  - Compiler-specific info string
+ - A list of enabled hooks
 
 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 acacca8fef..aa8a489c35 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -3,6 +3,8 @@
 #include "strbuf.h"
 #include "help.h"
 #include "compat/compiler.h"
+#include "run-command.h"
+
 
 static void get_system_info(struct strbuf *sys_info)
 {
@@ -31,6 +33,53 @@ static void get_system_info(struct strbuf *sys_info)
 	get_libc_info(sys_info);
 }
 
+static void get_populated_hooks(struct strbuf *hook_info, int nongit)
+{
+	/*
+	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
+	 * so below is a transcription of `git help hooks`. Later, this should
+	 * be replaced with some programmatically generated list (generated from
+	 * doc or else taken from some library which tells us about all the
+	 * hooks)
+	 */
+	static const char *hook[] = {
+		"applypatch-msg",
+		"pre-applypatch",
+		"post-applypatch",
+		"pre-commit",
+		"pre-merge-commit",
+		"prepare-commit-msg",
+		"commit-msg",
+		"post-commit",
+		"pre-rebase",
+		"post-checkout",
+		"post-merge",
+		"pre-push",
+		"pre-receive",
+		"update",
+		"post-receive",
+		"post-update",
+		"push-to-checkout",
+		"pre-auto-gc",
+		"post-rewrite",
+		"sendemail-validate",
+		"fsmonitor-watchman",
+		"p4-pre-submit",
+		"post-index-change",
+	};
+	int i;
+
+	if (nongit) {
+		strbuf_addstr(hook_info,
+			_("not run from a git repository - no hooks to show\n"));
+		return;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(hook); i++)
+		if (find_hook(hook[i]))
+			strbuf_addf(hook_info, "%s\n", hook[i]);
+}
+
 static const char * const bugreport_usage[] = {
 	N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"),
 	NULL
@@ -114,6 +163,9 @@ int cmd_main(int argc, const char **argv)
 	get_header(&buffer, _("System Info"));
 	get_system_info(&buffer);
 
+	get_header(&buffer, _("Enabled Hooks"));
+	get_populated_hooks(&buffer, nongit_ok);
+
 	/* 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/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index 2e73658a5c..526304ff95 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -57,5 +57,20 @@ test_expect_success 'can create leading directories outside of a git dir' '
 	nongit git bugreport -o foo/bar/baz
 '
 
+test_expect_success 'indicates populated hooks' '
+	test_when_finished rm git-bugreport-hooks.txt &&
+	test_when_finished rm -fr .git/hooks &&
+	rm -fr .git/hooks &&
+	mkdir .git/hooks &&
+	for hook in applypatch-msg prepare-commit-msg.sample
+	do
+		write_script ".git/hooks/$hook" <<-EOF || return 1
+		echo "hook $hook exists"
+		EOF
+	done &&
+	git bugreport -s hooks &&
+	grep applypatch-msg git-bugreport-hooks.txt &&
+	! grep prepare-commit-msg git-bugreport-hooks.txt
+'
 
 test_done
-- 
2.26.2.645.ge9eca65c58-goog

Re: [PATCH v5] bugreport: collect list of populated hooks

[Jonathan Nieder]

Hi,

Emily Shaffer wrote:

> Occasionally a failure a user is seeing may be related to a specific
> hook which is being run, perhaps without the user realizing. While the
> contents of hooks can be sensitive - containing user data or process
> information specific to the user's organization - simply knowing that a
> hook is being run at a certain stage can help us to understand whether
> something is going wrong.
>
> Without a definitive list of hook names within the code, we compile our
> own list from the documentation. This is likely prone to bitrot, but
> designing a single source of truth for acceptable hooks is too much
> overhead for this small change to the bugreport tool.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  Documentation/git-bugreport.txt |  1 +
>  bugreport.c                     | 52 +++++++++++++++++++++++++++++++++
>  t/t0091-bugreport.sh            | 15 ++++++++++
>  3 files changed, 68 insertions(+)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH v5] bugreport: collect list of populated hooks

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> The only change since v4 is to uncomment the heredoc, meaning the hook
> names will be correctly injected into the test hooks.

Which means that this is identical to what we had in 'pu' for a
couple of days already ;-)

Let's mark it as ready to be merged to 'next' and move on.

Thanks.