threads / patch / 53313

patchbugreport: collect list of populated hooks

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

## tl;dr

33 messages between Apr 24, 2020 and May 11, 2020. Diffs are folded; open one to read it.

replies: 32people: 4as markdown or json

Emily Shaffer· Apr 24, 2020, 23:38 UTC · lore

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(+)
Show changes to 2 files +53 −0

Documentation/git-bugreport.txt, bugreport.c

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
Jonathan Nieder· Apr 25, 2020, 00:30 UTC · re: Emily Shaffer · lore

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

Emily Shaffer wrote:
Show 6 quoted lines
> 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?
Show 18 quoted lines
> 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).

[...]
Show 32 quoted lines
> +	/*
> +	 * 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

Emily Shaffer· Apr 27, 2020, 20:48 UTC · re: Jonathan Nieder · lore

[PATCH] bugreport: drop time.h include

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(-)
Show changes to bugreport.c +0 −1
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
Jonathan Nieder· Apr 27, 2020, 21:03 UTC · re: Emily Shaffer · lore

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

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.

Show 22 quoted lines
>                                                                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

Junio C Hamano· Apr 27, 2020, 21:25 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 8 quoted lines
> 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.
Junio C Hamano· Apr 27, 2020, 21:41 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> 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(-)
Show changes to bugreport.c +0 −1
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
Emily Shaffer· Apr 27, 2020, 21:56 UTC · re: Junio C Hamano · lore

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

On Mon, Apr 27, 2020 at 02:41:12PM -0700, Junio C Hamano wrote:
Show 20 quoted lines
> 
> 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.

Show 33 quoted lines
> 
> 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
> 
Junio C Hamano· Apr 27, 2020, 23:27 UTC · re: Emily Shaffer · lore

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

Emily Shaffer <emilyshaffer@google.com> writes:
Show 26 quoted lines
> 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.
Emily Shaffer· Apr 27, 2020, 23:42 UTC · re: Emily Shaffer · lore

[PATCH v2] bugreport: drop extraneous includes

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(-)
Show changes to bugreport.c +0 −2
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
Jonathan Nieder· Apr 27, 2020, 23:46 UTC · re: Emily Shaffer · lore

Re: [PATCH v2] bugreport: drop extraneous includes

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/
Show 9 quoted lines
> "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.
Junio C Hamano· Apr 25, 2020, 04:52 UTC · re: Emily Shaffer · lore

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

Emily Shaffer <emilyshaffer@google.com> writes:
Show 7 quoted lines
> 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?

Show 13 quoted lines
> @@ -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"?
Show 7 quoted lines
> +	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.
Emily Shaffer· Apr 27, 2020, 19:02 UTC · re: Junio C Hamano · lore

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

On Fri, Apr 24, 2020 at 09:52:07PM -0700, Junio C Hamano wrote:
Show 13 quoted lines
> 
> 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.
Show 17 quoted lines
> 
> > @@ -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
Junio C Hamano· Apr 27, 2020, 20:46 UTC · re: Emily Shaffer · lore

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

Emily Shaffer <emilyshaffer@google.com> writes:
Show 15 quoted lines
>> > +	/*
>> > +	 * 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.
Emily Shaffer· Apr 27, 2020, 20:49 UTC · re: Junio C Hamano · lore

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

On Mon, Apr 27, 2020 at 01:46:38PM -0700, Junio C Hamano wrote:
Show 24 quoted lines
> 
> 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.
Emily Shaffer· Apr 27, 2020, 23:38 UTC · re: Emily Shaffer · lore

[PATCH v2] bugreport: collect list of populated hooks

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(+)
Show changes to 3 files +63 −0

Documentation/git-bugreport.txt, bugreport.c, t/t0091-bugreport.sh

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
Jonathan Nieder· Apr 27, 2020, 23:45 UTC · re: Emily Shaffer · lore

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

Emily Shaffer wrote:
Show 12 quoted lines
> --- 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.
Junio C Hamano· Apr 28, 2020, 00:04 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 16 quoted lines
> 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.

Show 5 quoted lines
>> +	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.
Emily Shaffer· Apr 30, 2020, 00:01 UTC · re: Jonathan Nieder · lore

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

On Mon, Apr 27, 2020 at 04:45:10PM -0700, Jonathan Nieder wrote:
Show 17 quoted lines
> 
> 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.
Show 10 quoted lines
> 
> > +	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
Emily Shaffer· Apr 30, 2020, 01:24 UTC · re: Emily Shaffer · lore

[PATCH v3] bugreport: collect list of populated hooks

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(+)
Show changes to 3 files +63 −0

Documentation/git-bugreport.txt, bugreport.c, t/t0091-bugreport.sh

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
Jonathan Nieder· Apr 30, 2020, 01:50 UTC · re: Emily Shaffer · lore

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

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.
[...]
Show 10 quoted lines
> --- 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

Jonathan Nieder· Apr 30, 2020, 01:53 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder wrote:
> Emily Shaffer wrote:
Show 28 quoted lines
>> +++ 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/
Junio C Hamano· Apr 30, 2020, 17:44 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 5 quoted lines
> 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.
Show 10 quoted lines
>> +	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?

Show 6 quoted lines
>> +	git bugreport -s hooks &&
>> +	grep applypatch-msg git-bugreport-hooks.txt &&
>> +	! grep prepare-commit-msg git-bugreport-hooks.txt
>> +'
>
> Thanks,
Thanks.
Junio C Hamano· Apr 30, 2020, 22:09 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
>> 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(-)
Show changes to t/t0091-bugreport.sh +7 −4
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
Emily Shaffer· May 7, 2020, 21:08 UTC · re: Junio C Hamano · lore

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

On Thu, Apr 30, 2020 at 03:09:55PM -0700, Junio C Hamano wrote:
Show 25 quoted lines
> 
> 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
Show 14 quoted lines
> 
>  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.

Show 10 quoted lines
> +	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.
Show 6 quoted lines
>  	git bugreport -s hooks &&
>  	grep applypatch-msg git-bugreport-hooks.txt &&
>  	! grep prepare-commit-msg git-bugreport-hooks.txt
> -- 
> 2.26.2-447-gd61d20c9b4
> 
Junio C Hamano· May 7, 2020, 23:06 UTC · re: Emily Shaffer · lore

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

Emily Shaffer <emilyshaffer@google.com> writes:
Show 6 quoted lines
>>  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.

Show 12 quoted lines
>> +	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).

Show 6 quoted lines
>>  	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.
Emily Shaffer· May 11, 2020, 21:26 UTC · re: Junio C Hamano · lore

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

On Thu, May 07, 2020 at 04:06:14PM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> 
> 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).

Show 16 quoted lines
> 
> >> +	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
Emily Shaffer· May 8, 2020, 00:53 UTC · re: Emily Shaffer · lore

[PATCH v4] bugreport: collect list of populated hooks

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(+)
Show changes to 3 files +68 −0

Documentation/git-bugreport.txt, bugreport.c, t/t0091-bugreport.sh

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
Junio C Hamano· May 8, 2020, 01:20 UTC · re: Emily Shaffer · lore

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

Emily Shaffer <emilyshaffer@google.com> writes:
Show 18 quoted lines
> 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.
Đoàn Trần Công Danh· May 8, 2020, 01:34 UTC · re: Emily Shaffer · lore

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

On 2020-05-07 17:53:57-0700, Emily Shaffer <emilyshaffer@google.com> wrote:
Show 14 quoted lines
> +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(-)
Show changes to t/t0091-bugreport.sh +7 −2
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
Emily Shaffer· May 11, 2020, 21:22 UTC · re: Đoàn Trần Công Danh · lore

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

On Fri, May 08, 2020 at 08:34:05AM +0700, Đoàn Trần Công Danh wrote:
Show 48 quoted lines
> 
> 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
Emily Shaffer· May 11, 2020, 22:14 UTC · re: Emily Shaffer · lore

[PATCH v5] bugreport: collect list of populated hooks

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(+)
Show changes to 3 files +68 −0

Documentation/git-bugreport.txt, bugreport.c, t/t0091-bugreport.sh

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
Jonathan Nieder· May 11, 2020, 23:26 UTC · re: Emily Shaffer · lore

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

Hi,
Emily Shaffer wrote:
Show 18 quoted lines
> 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.
Junio C Hamano· May 11, 2020, 23:45 UTC · re: Emily Shaffer · lore

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

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.

← back to recent threads