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

Commit Message

Emily Shaffer Dec. 13, 2019, 12:43 a.m. UTC

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 | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Comments

Junio C Hamano Dec. 13, 2019, 9:47 p.m. UTC | #1

Emily Shaffer <[e-mail]> writes:

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

We probably would want to employ technique similar to what you used
for the list of safe configuration we saw in an earlier patch in the
series.  What's not even documented is not worth reporting ;-)

> +	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;
> +	int nongit_ok;
> +
> +	setup_git_directory_gently(&nongit_ok);
> +
> +	if (nongit_ok) {
> +		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
> @@ -188,6 +240,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);
> +
>  	report = fopen_for_writing(report_path.buf);
>  	strbuf_write(&buffer, report);
>  	fclose(report);

Emily Shaffer Dec. 16, 2019, 11:51 p.m. UTC | #2

On Fri, Dec 13, 2019 at 01:47:27PM -0800, Junio C Hamano wrote:
> Emily Shaffer <[e-mail]> writes:
> 
> > +	/*
> > +	 * Doesn't look like there is a list of all possible hooks; so below is
> > +	 * a transcription of `git help hook`.
> > +	 */
> 
> We probably would want to employ technique similar to what you used
> for the list of safe configuration we saw in an earlier patch in the
> series.  What's not even documented is not worth reporting ;-)

I somewhat slightly would rather not, since I hope to include a list of
the available hooks in my work with git-hook:
https://lore.kernel.org/[e-mail]

It's true that the way this patch is written now is prone to bitrot.
It's also true that I could win the lottery tomorrow and never finish my
hopes of git-hook implementation. And it's finally true that I don't
want to write a fourth build-time-generated header which scrapes docs,
especially if I also plan to delete it two months from now.

I wonder whether a better approach might be to drop hook support
entirely here, and add it later on when this particular bit of tech debt
(no canonical list of hook names) is solved, one way or another.

Another approach may be to just list all contents of $(core.hookdir)/
which doesn't end in ".sample", which is what the bash first attempt of
bugreport did. This does mean that those who have something like:

  .git/hooks/
    pre-commit
    pre-commit.d/

  pre-commit:
    sh pre-commit.d/a
    sh pre-commit.d/b

will have their multihook config exposed, although the contents of those
hooks won't be.

I'm interested to know everyone else's opinion, because mine is biased
by my hope that I can solve all the world's problems with git-hook ;)

 - Emily

Patch

diff --git a/bugreport.c b/bugreport.c
index 1fca28f0b9..f89cb8d754 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_http_version_info(struct strbuf *http_info)
 {
@@ -121,6 +122,57 @@  static void get_safelisted_config(struct strbuf *config_info)
 	cfgset_clear(&safelist);
 }
 
+static void get_populated_hooks(struct strbuf *hook_info)
+{
+	/*
+	 * 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;
+	int nongit_ok;
+
+	setup_git_directory_gently(&nongit_ok);
+
+	if (nongit_ok) {
+		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
@@ -188,6 +240,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);
+
 	report = fopen_for_writing(report_path.buf);
 	strbuf_write(&buffer, report);
 	fclose(report);