[v5,11/15] bugreport: collect list of populated hooks

Commit Message

Emily Shaffer Jan. 24, 2020, 3:34 a.m. UTC

From: Emily Shaffer <[e-mail]>

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 <[e-mail]>
---
 bugreport.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

Comments

Junio C Hamano Feb. 4, 2020, 6:44 p.m. UTC | #1

[e-mail] writes:

> +	/*
> +	 * Doesn't look like there is a list of all possible hooks; so below is
> +	 * a transcription of `git help hook`.

That's "git help hooks", if I tried my reproduction correctly.  A
straight-forward (in the sense of "what we want in the outcome is
quite clear" and not in the sense of "anybody can design and
implement it with a single 30-line patch") follow-up we can make
after this series lands is to rethink how Documentation/githooks.txt
is maintained and the list we have here is synchronized with it.

The design could me just the matter of running "grep" of some sort,
with appropriate markups that are no-op to AsciiDoctor/AsciiDoc
added to the documentation source, to produce this list.

> +	 */
> +	const char *hooks = "applypatch-msg,"
> +			    "pre-applypatch,"
> +			    "post-applypatch,"
> +			    "pre-commit,"
> +			    "pre-merge-commit,"
> +			    "prepare-commit-msg,"
> +			    "commit-msg,"
> +			    "post-commit,"
> +			    "pre-rebase,"
> +			    "post-checkout,"
> +			    "post-merge,"
> +			    "pre-push,"
> +			    "pre-receive,"
> +			    "update,"
> +			    "post-receive,"
> +			    "post-update,"
> +			    "push-to-checkout,"
> +			    "pre-auto-gc,"
> +			    "post-rewrite,"
> +			    "sendemail-validate,"
> +			    "fsmonitor-watchman,"
> +			    "p4-pre-submit,"
> +			    "post-index-changex";
> +	struct string_list hooks_list = STRING_LIST_INIT_DUP;
> +	struct string_list_item *iter = NULL;
> +
> +
> +	if (nongit) {
> +		strbuf_addstr(hook_info,
> +			"not run from a git repository - no hooks to show\n");
> +		return;
> +	}
> +
> +	string_list_split(&hooks_list, hooks, ',', -1);
> +
> +	for_each_string_list_item(iter, &hooks_list) {
> +		if (find_hook(iter->string)) {
> +			strbuf_addstr(hook_info, iter->string);
> +			strbuf_complete_line(hook_info);
> +		}
> +	}
> +}
> +
>  static const char * const bugreport_usage[] = {
>  	N_("git bugreport [-o|--output <file>]"),
>  	NULL
> @@ -193,6 +243,9 @@ int cmd_main(int argc, const char **argv)
>  	get_header(&buffer, "Safelisted Config Info");
>  	get_safelisted_config(&buffer);
>  
> +	get_header(&buffer, "Configured Hooks");

Phrase nit.  There may be many people who just enabled hooks without
configuring, so "Enabled Hooks" may be more appropriate.  We do not
have to inspect what is in the hook to determine if it is enabled,
but we do need to if we want to tell if a hook is "configured".

> +	get_populated_hooks(&buffer, nongit_ok);
> +
>  	report = fopen_for_writing(report_path.buf);
>  	strbuf_write(&buffer, report);
>  	fclose(report);

Emily Shaffer Feb. 5, 2020, 2:48 a.m. UTC | #2

On Tue, Feb 04, 2020 at 10:44:39AM -0800, Junio C Hamano wrote:
> [e-mail] writes:
> 
> > +	/*
> > +	 * Doesn't look like there is a list of all possible hooks; so below is
> > +	 * a transcription of `git help hook`.
> 
> That's "git help hooks", if I tried my reproduction correctly.

Yep. I'll fix it.

> A straight-forward (in the sense of "what we want in the outcome is
> quite clear" and not in the sense of "anybody can design and implement
> it with a single 30-line patch") follow-up we can make after this
> series lands is to rethink how Documentation/githooks.txt is
> maintained and the list we have here is synchronized with it.
> 
> The design could me just the matter of running "grep" of some sort,
> with appropriate markups that are no-op to AsciiDoctor/AsciiDoc
> added to the documentation source, to produce this list.

Well, with Martin's suggestion[1] to use annotate:bugreport[include]
instead of bugreport:include[x], this kind of change becomes a matter of
course to just use "annotate:hook[x]" and write a script nearly
identical to generate-bugreport-config-safelist.sh, and add it to the
Makefile.

Like I said up-thread, I don't like the idea just because it's awfully
temporary (pending work that's right below this on my list for the
coming weeks).

It sounds like you're saying you don't mind any work of this nature
being done as a follow-up - and that's fine with me, too. I'd just
rather do the follow-up with enums and a library than with grep and
sh. :)

> >  static const char * const bugreport_usage[] = {
> >  	N_("git bugreport [-o|--output <file>]"),
> >  	NULL
> > @@ -193,6 +243,9 @@ int cmd_main(int argc, const char **argv)
> >  	get_header(&buffer, "Safelisted Config Info");
> >  	get_safelisted_config(&buffer);
> >  
> > +	get_header(&buffer, "Configured Hooks");
> 
> Phrase nit.  There may be many people who just enabled hooks without
> configuring, so "Enabled Hooks" may be more appropriate.  We do not
> have to inspect what is in the hook to determine if it is enabled,
> but we do need to if we want to tell if a hook is "configured".

Ah, that's a good point. OK, I'll change the wording as you suggested.

 - Emily

Emily Shaffer Feb. 5, 2020, 3 a.m. UTC | #3

On Tue, Feb 04, 2020 at 06:48:59PM -0800, Emily Shaffer wrote:
> Well, with Martin's suggestion[1] to use annotate:bugreport[include]
> instead of bugreport:include[x], this kind of change becomes a matter of
> course to just use "annotate:hook[x]" and write a script nearly
> identical to generate-bugreport-config-safelist.sh, and add it to the
> Makefile.

Bah.

[1]: https://lore.kernel.org/git/CAN0heSq_i4EitqYH-qrZyXBU+=[e-mail]

Patch

diff --git a/bugreport.c b/bugreport.c
index 7a9fd36b60..4c77009f1b 100644
--- a/bugreport.c
+++ b/bugreport.c
@@ -9,6 +9,7 @@ 
 #include "config.h"
 #include "bugreport-config-safelist.h"
 #include "khash.h"
+#include "run-command.h"
 
 static void get_curl_version_info(struct strbuf *curl_info)
 {
@@ -121,6 +122,55 @@  static void get_safelisted_config(struct strbuf *config_info)
 	cfgset_clear(&safelist);
 }
 
+static void get_populated_hooks(struct strbuf *hook_info, int nongit)
+{
+	/*
+	 * Doesn't look like there is a list of all possible hooks; so below is
+	 * a transcription of `git help hook`.
+	 */
+	const char *hooks = "applypatch-msg,"
+			    "pre-applypatch,"
+			    "post-applypatch,"
+			    "pre-commit,"
+			    "pre-merge-commit,"
+			    "prepare-commit-msg,"
+			    "commit-msg,"
+			    "post-commit,"
+			    "pre-rebase,"
+			    "post-checkout,"
+			    "post-merge,"
+			    "pre-push,"
+			    "pre-receive,"
+			    "update,"
+			    "post-receive,"
+			    "post-update,"
+			    "push-to-checkout,"
+			    "pre-auto-gc,"
+			    "post-rewrite,"
+			    "sendemail-validate,"
+			    "fsmonitor-watchman,"
+			    "p4-pre-submit,"
+			    "post-index-changex";
+	struct string_list hooks_list = STRING_LIST_INIT_DUP;
+	struct string_list_item *iter = NULL;
+
+
+	if (nongit) {
+		strbuf_addstr(hook_info,
+			"not run from a git repository - no hooks to show\n");
+		return;
+	}
+
+	string_list_split(&hooks_list, hooks, ',', -1);
+
+	for_each_string_list_item(iter, &hooks_list) {
+		if (find_hook(iter->string)) {
+			strbuf_addstr(hook_info, iter->string);
+			strbuf_complete_line(hook_info);
+		}
+	}
+}
+
 static const char * const bugreport_usage[] = {
 	N_("git bugreport [-o|--output <file>]"),
 	NULL
@@ -193,6 +243,9 @@  int cmd_main(int argc, const char **argv)
 	get_header(&buffer, "Safelisted Config Info");
 	get_safelisted_config(&buffer);
 
+	get_header(&buffer, "Configured Hooks");
+	get_populated_hooks(&buffer, nongit_ok);
+
 	report = fopen_for_writing(report_path.buf);
 	strbuf_write(&buffer, report);
 	fclose(report);