On Tue, Jun 29, 2021 at 08:54:02PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> 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@xxxxxxxxx>
> Signed-off-by: René Scharfe <l.s.r@xxxxxx>
> ---
> .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.
> strbuf_reset(&path);
> strbuf_git_path(&path, "hooks/%s", name);
> if (access(path.buf, X_OK) < 0) {
> --
> 2.32.0.615.g90fb4d7369
>
