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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 25, 2020, 00:30 UTC
Message-ID: <20200425003002.GC17217@google.com>
In-Reply-To: <20200424233800.200439-1-emilyshaffer@google.com>

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