git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 3/3] hook-list.h: add a generated list of hooks, like config-list.h

From
Emily Shaffer <emilyshaffer@google.com>
Date
Jul 9, 2021, 20:29 UTC
Message-ID
<YOixxZQDVDKIM8bo@google.com>
In-Reply-To
<patch-3.3-ba7f01f4f6-20210629T183325Z-avarab@gmail.com>
On Tue, Jun 29, 2021 at 08:54:02PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 236 quoted lines
> 
> Make githooks(5) the source of truth for what hooks git supports, and
> die hooks we don't know about in find_hook(). This ensures that the
> documentation and the C code's idea about existing hooks doesn't
> diverge.
> 
> We still have Perl and Python code running its own hooks, but that'll
> be addressed by Emily Shaffer's upcoming "git hook run" command.
> 
> This resolves a long-standing TODO item in bugreport.c of there being
> no centralized listing of hooks, and fixes a bug with the bugreport
> listing only knowing about 1/4 of the p4 hooks. It didn't know about
> the recent "reference-transaction" hook either.
> 
> I have not been able to directly test the CMake change being made
> here. Since 4c2c38e800 (ci: modification of main.yml to use cmake for
> vs-build job, 2020-06-26) some of the Windows CI has a hard dependency
> on CMake, this change works there, and is to my eyes an obviously
> correct use of a pattern established in previous CMake changes,
> namely:
> 
>  - 061c2240b1 (Introduce CMake support for configuring Git,
>     2020-06-12)
>  - 709df95b78 (help: move list_config_help to builtin/help,
>     2020-04-16)
>  - 976aaedca0 (msvc: add a Makefile target to pre-generate the Visual
>    Studio solution, 2019-07-29)
> 
> The LC_ALL=C is needed because at least in my locale the dash ("-") is
> ignored for the purposes of sorting, which results in a different
> order. I'm not aware of anything in git that has a hard dependency on
> the order, but e.g. the bugreport output would end up using whatever
> locale was in effect when git was compiled.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  .gitignore                          |  1 +
>  Makefile                            | 10 ++++++-
>  builtin/bugreport.c                 | 44 ++++++-----------------------
>  contrib/buildsystems/CMakeLists.txt |  7 +++++
>  generate-hooklist.sh                | 18 ++++++++++++
>  hook.c                              | 19 +++++++++++++
>  6 files changed, 62 insertions(+), 37 deletions(-)
>  create mode 100755 generate-hooklist.sh
> 
> diff --git a/.gitignore b/.gitignore
> index 311841f9be..6be9de41ae 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -190,6 +190,7 @@
>  /gitweb/static/gitweb.min.*
>  /config-list.h
>  /command-list.h
> +/hook-list.h
>  *.tar.gz
>  *.dsc
>  *.deb
> diff --git a/Makefile b/Makefile
> index d155b7be21..9b811d3548 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -817,6 +817,8 @@ XDIFF_LIB = xdiff/lib.a
>  
>  GENERATED_H += command-list.h
>  GENERATED_H += config-list.h
> +GENERATED_H += hook-list.h
> +
>  .PHONY: generated-hdrs
>  generated-hdrs: $(GENERATED_H)
>  
> @@ -2208,7 +2210,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
>  
>  help.sp help.s help.o: command-list.h
>  
> -builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
> +hook.sp hook.s hook.o: hook-list.h
> +
> +builtin/help.sp builtin/help.s builtin/help.o: config-list.h hook-list.h GIT-PREFIX
>  builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
>  	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
>  	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
> @@ -2241,6 +2245,10 @@ command-list.h: $(wildcard Documentation/git*.txt)
>  		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
>  		command-list.txt >$@+ && mv $@+ $@
>  
> +hook-list.h: generate-hooklist.sh Documentation/githooks.txt
> +	$(QUIET_GEN)$(SHELL_PATH) ./generate-hooklist.sh \
> +		>$@+ && mv $@+ $@
> +
>  SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
>  	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
>  	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 941c8d5e27..a7a1fcb8a7 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -4,6 +4,7 @@
>  #include "help.h"
>  #include "compat/compiler.h"
>  #include "hook.h"
> +#include "hook-list.h"
>  
>  
>  static void get_system_info(struct strbuf *sys_info)
> @@ -41,39 +42,7 @@ static void get_system_info(struct strbuf *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;
> +	const char **p;
>  
>  	if (nongit) {
>  		strbuf_addstr(hook_info,
> @@ -81,9 +50,12 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
>  		return;
>  	}
>  
> -	for (i = 0; i < ARRAY_SIZE(hook); i++)
> -		if (hook_exists(hook[i]))
> -			strbuf_addf(hook_info, "%s\n", hook[i]);
> +	for (p = hook_name_list; *p; p++) {
> +		const char *hook = *p;
> +
> +		if (hook_exists(hook))
> +			strbuf_addf(hook_info, "%s\n", hook);
> +	}
>  }
>  
>  static const char * const bugreport_usage[] = {
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index a87841340e..c216940945 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -602,6 +602,13 @@ if(NOT EXISTS ${CMAKE_BINARY_DIR}/config-list.h)
>  			OUTPUT_FILE ${CMAKE_BINARY_DIR}/config-list.h)
>  endif()
>  
> +if(NOT EXISTS ${CMAKE_BINARY_DIR}/hook-list.h)
> +	message("Generating hook-list.h")
> +	execute_process(COMMAND ${SH_EXE} ${CMAKE_SOURCE_DIR}/generate-hooklist.sh
> +			WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
> +			OUTPUT_FILE ${CMAKE_BINARY_DIR}/hook-list.h)
> +endif()
> +
>  include_directories(${CMAKE_BINARY_DIR})
>  
>  #build
> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
> new file mode 100755
> index 0000000000..6d4e56d1a3
> --- /dev/null
> +++ b/generate-hooklist.sh
> @@ -0,0 +1,18 @@
> +#!/bin/sh
> +#
> +# Usage: ./generate-hooklist.sh >hook-list.h
> +
> +cat <<EOF
> +/* Automatically generated by generate-hooklist.sh */
> +
> +static const char *hook_name_list[] = {
> +EOF
> +
> +sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
> +	<Documentation/githooks.txt |
> +	LC_ALL=C sort
> +
> +cat <<EOF
> +	NULL,
> +};
> +EOF
> diff --git a/hook.c b/hook.c
> index 97cd799a32..1f1db1ec9b 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -1,11 +1,30 @@
>  #include "cache.h"
>  #include "hook.h"
>  #include "run-command.h"
> +#include "hook-list.h"
> +
> +static int known_hook(const char *name)
> +{
> +	const char **p;
> +	size_t len = strlen(name);
> +	for (p = hook_name_list; *p; p++) {
> +		const char *hook = *p;
> +
> +		if (!strncmp(name, hook, len) && hook[len] == '\0')
> +			return 1;
> +	}
> +
> +	return 0;
> +}
>  
>  const char *find_hook(const char *name)
>  {
>  	static struct strbuf path = STRBUF_INIT;
>  
> +	if (!known_hook(name))
> +		die(_("the hook '%s' is not known to git, should be in hook-list.h via githooks(5)"),
> +		    name);
> +

I'm not sure that it's necessary to require this, to be honest. I see a use case for wrappers to want to store and run hooks in an idiomatic way, and doing so by instructing their users to stick in .git/hooks/wrapper-clone (for example) and then calling 'git hook run wrapper-clone'. That's doubly compelling in a later config-based-hooks world where 'git hook run' gets you free multihook features like ordering and parallelism. I will likely want to remove this when rebasing my config-based hooks work on top of your restart.

Show 6 quoted lines
>  	strbuf_reset(&path);
>  	strbuf_git_path(&path, "hooks/%s", name);
>  	if (access(path.buf, X_OK) < 0) {
> -- 
> 2.32.0.615.g90fb4d7369
> 
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 18 of 21 in “Add a generated list of hooks in hook-list.h”
  1. 0/3 Add a generated list of hooks in hook-list.hÆvar Arnfjörð Bjarmason, Jun 17, 2021
  2. 1/3 hook.[ch]: move find_hook() to this new libraryÆvar Arnfjörð Bjarmason, Jun 17, 2021
  3. 2/3 hook.c: add a hook_exists() wrapper and use it in bugreport.cÆvar Arnfjörð Bjarmason, Jun 17, 2021
  4. 3/3 hook-list.h: add a generated list of hooks, like config-list.hÆvar Arnfjörð Bjarmason, Jun 17, 2021
  5. SZEDER GáborJun 18, 2021
  6. Eric SunshineJun 18, 2021
  7. Junio C HamanoJun 19, 2021
  8. Ævar Arnfjörð BjarmasonJun 20, 2021
  9. René ScharfeJun 20, 2021
  10. Johannes SchindelinJun 22, 2021
  11. Junio C HamanoJun 29, 2021
  12. René ScharfeJun 29, 2021
  13. 0/3 Add a generated list of hooks in hook-list.hÆvar Arnfjörð Bjarmason, Jun 29, 2021
  14. 1/3 hook.[ch]: move find_hook() to this new libraryÆvar Arnfjörð Bjarmason, Jun 29, 2021
  15. 3/3 hook-list.h: add a generated list of hooks, like config-list.hÆvar Arnfjörð Bjarmason, Jun 29, 2021
  16. René ScharfeJun 29, 2021
  17. Ævar Arnfjörð BjarmasonJun 29, 2021
  18. Emily ShafferJul 9, 2021
  19. Ævar Arnfjörð BjarmasonJul 10, 2021
  20. Emily ShafferJul 12, 2021
  21. 2/3 hook.c: add a hook_exists() wrapper and use it in bugreport.cÆvar Arnfjörð Bjarmason, Jun 29, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.