[PATCH 0/8] Makefile: generate a hook-list.h, prep for config-based-hooks

[PATCH 0/8] Makefile: generate a hook-list.h, prep for config-based-hooks

[Ævar Arnfjörð Bjarmason]

This series is an incremental restart of the es/config-based-hooks and
ab/config-based-hooks-base topics, which should both be ejected in
favor of this.

Those topics have been lingering for a long time. The v5 of
ab/config-based-hooks-base[1] was re-rolled on September 2nd, and then
on September 9th I re-rolled a v4 of Emily's on-top topic[2] to solve
conflicts with the "base" one as other things in "seen" were colliding
with its v4 + Emily's older version.

But in the end the sum total of master..es/config-based-hooks is 43
commits and a rather daunting "44 files changed, 1870 insertions(+),
611 deletions(-)" diffstat. I think it's fair to say that anything
approaching a full picture of these patches only exists in our
respective wetwares, and wider reviewer attention is clearly needed.

That much larger topic also has a conflict Junio's been solving, and
by its presence has surely been holding up the progression of various
other submitted and unsubmitted topics.

So this is step #1 in an (at least) 5 step plan outlined in [3] (start
reading at "It seems due to") to get this greater series into git in a
more piecemeal fashion.

Here we're only setting up some build system changes needed for the
eventual master..ab/config-based-hooks-base topic, the range-diff is
against the relevant part of[1]. These changes are able to stand on
their own.

The only caveat to that is that we end up with a hook.[ch] with just
two functions, which is usually not the point at which we split out
new headers. As noted in the commit messages we expect those headers
to get much larger with the rest of ab/config-based-hooks-base.

I intentionally split this off from the much larger thread starting at
[4] and pruned the CC list. If anyone feels left out or is interested
a review of this is a most welcome way to be re-added :)

Changes since [1]:

 * Resolved a conflict with advice.[ch]-related changes
 * A new commit to skip the "mv $@+ $@" dance for
   $(GENERATED_H). These patches originally pre-dated our use of
   ".DELETE_ON_ERROR" in the Makefile.
 * There was a "while we're at it" change to a comment in
   run-command.h as it was being moved, now this is a purely move-only
   change. I'll either re-incorporate that minor change in re-rolls of
   ab/config-based-hooks-base, or just drop it.

1. https://lore.kernel.org/git/cover-v5-00.36-00000000000-20210902T125110Z-avarab@gmail.com/#t
2. https://lore.kernel.org/git/cover-v4-0.5-00000000000-20210909T122802Z-avarab@gmail.com/
3. https://lore.kernel.org/git/875yut8nns.fsf@evledraar.gmail.com/
4. https://lore.kernel.org/git/20210311021037.3001235-1-emilyshaffer@google.com/#t

Emily Shaffer (1):
  hook.c: add a hook_exists() wrapper and use it in bugreport.c

Ævar Arnfjörð Bjarmason (7):
  Makefile: mark "check" target as .PHONY
  Makefile: stop hardcoding {command,config}-list.h
  Makefile: don't perform "mv $@+ $@" dance for $(GENERATED_H)
  Makefile: remove an out-of-date comment
  hook.[ch]: move find_hook() from run-command.c to hook.c
  hook.c users: use "hook_exists()" instead of "find_hook()"
  hook-list.h: add a generated list of hooks, like config-list.h

 .gitignore                          |  1 +
 Makefile                            | 29 ++++++++++--------
 builtin/am.c                        |  1 +
 builtin/bugreport.c                 | 46 ++++++-----------------------
 builtin/commit.c                    |  3 +-
 builtin/merge.c                     |  3 +-
 builtin/receive-pack.c              |  3 +-
 builtin/worktree.c                  |  1 +
 compat/vcbuild/README               |  2 +-
 config.mak.uname                    |  6 ++--
 contrib/buildsystems/CMakeLists.txt |  7 +++++
 generate-hooklist.sh                | 18 +++++++++++
 hook.c                              | 42 ++++++++++++++++++++++++++
 hook.h                              | 16 ++++++++++
 refs.c                              |  1 +
 run-command.c                       | 35 +---------------------
 run-command.h                       |  7 -----
 sequencer.c                         |  3 +-
 transport.c                         |  1 +
 19 files changed, 126 insertions(+), 99 deletions(-)
 create mode 100755 generate-hooklist.sh
 create mode 100644 hook.c
 create mode 100644 hook.h

Range-diff:
1:  ac419613fdc = 1:  91d0cd09c8e Makefile: mark "check" target as .PHONY
2:  a161b7f0a5c = 2:  804795771c6 Makefile: stop hardcoding {command,config}-list.h
-:  ----------- > 3:  010701fd784 Makefile: don't perform "mv $@+ $@" dance for $(GENERATED_H)
3:  ffef1d3257e = 4:  b3cce74d259 Makefile: remove an out-of-date comment
4:  545e16c6f04 ! 5:  7dd874d50ec hook.[ch]: move find_hook() from run-command.c to hook.c
    @@ hook.c (new)
     +			err = errno;
     +#endif
     +
    -+		if (err == EACCES && advice_ignored_hook) {
    ++		if (err == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) {
     +			static struct string_list advise_given = STRING_LIST_INIT_DUP;
     +
     +			if (!string_list_lookup(&advise_given, name)) {
    @@ hook.h (new)
     +#ifndef HOOK_H
     +#define HOOK_H
     +
    -+/**
    ++/*
     + * Returns the path to the hook file, or NULL if the hook is missing
     + * or disabled. Note that this points to static storage that will be
    -+ * overwritten by further calls to find_hook().
    ++ * overwritten by further calls to find_hook and run_hook_*.
     + */
     +const char *find_hook(const char *name);
     +
    @@ refs.c
     
      ## run-command.c ##
     @@
    - #include "string-list.h"
      #include "quote.h"
      #include "config.h"
    + #include "packfile.h"
     +#include "hook.h"
      
      void child_process_init(struct child_process *child)
    @@ run-command.c: int async_with_fork(void)
     -			err = errno;
     -#endif
     -
    --		if (err == EACCES && advice_ignored_hook) {
    +-		if (err == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) {
     -			static struct string_list advise_given = STRING_LIST_INIT_DUP;
     -
     -			if (!string_list_lookup(&advise_given, name)) {
5:  a9bc4519e9a = 6:  db8893afee6 hook.c: add a hook_exists() wrapper and use it in bugreport.c
6:  e99ec2e6f8f = 7:  b61130dee5b hook.c users: use "hook_exists()" instead of "find_hook()"
7:  2ffb2332c8a ! 8:  80aae4d5c13 hook-list.h: add a generated list of hooks, like config-list.h
    @@ Makefile: git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
      	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
     @@ Makefile: command-list.h: $(wildcard Documentation/git*.txt)
      		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
    - 		command-list.txt >$@+ && mv $@+ $@
    + 		command-list.txt >$@
      
     +hook-list.h: generate-hooklist.sh Documentation/githooks.txt
    -+	$(QUIET_GEN)$(SHELL_PATH) ./generate-hooklist.sh \
    -+		>$@+ && mv $@+ $@
    ++	$(QUIET_GEN)$(SHELL_PATH) ./generate-hooklist.sh >$@
     +
      SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
      	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
-- 
2.33.0.1229.g0a86d28df49

[PATCH 1/8] Makefile: mark "check" target as .PHONY

[Ævar Arnfjörð Bjarmason]

Fix a bug in 44c9e8594e (Fix up header file dependencies and add
sparse checking rules, 2005-07-03), we never marked the phony "check"
target as such.

Perhaps we should just remove it, since as of a combination of
912f9980d2 (Makefile: help people who run 'make check' by mistake,
2008-11-11) 0bcd9ae85d (sparse: Fix errors due to missing
target-specific variables, 2011-04-21) we've been suggesting the user
run "make sparse" directly.

But under that mode it still does something, as well as directing the
user to run "make test" under non-sparse. So let's punt that and
narrowly fix the PHONY bug.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 9df565f27bb..e85a8e07d3e 100644
--- a/Makefile
+++ b/Makefile
@@ -2932,6 +2932,7 @@ hdr-check: $(HCO)
 style:
 	git clang-format --style file --diff --extensions c,h
 
+.PHONY: check
 check: config-list.h command-list.h
 	@if sparse; \
 	then \
-- 
2.33.0.1229.g0a86d28df49

[PATCH 2/8] Makefile: stop hardcoding {command,config}-list.h

[Ævar Arnfjörð Bjarmason]

Change various places that hardcode the names of these two files to
refer to either $(GENERATED_H), or to a new generated-hdrs
target. That target is consistent with the *-objs targets I recently
added in 029bac01a8 (Makefile: add {program,xdiff,test,git,fuzz}-objs
& objects targets, 2021-02-23).

A subsequent commit will add a new generated hook-list.h. By doing
this refactoring we'll only need to add the new file to the
GENERATED_H variable, not EXCEPT_HDRS, the vcbuild/README etc.

Hardcoding command-list.h there seems to have been a case of
copy/paste programming in 976aaedca0 (msvc: add a Makefile target to
pre-generate the Visual Studio solution, 2019-07-29). The
config-list.h was added later in 709df95b78 (help: move
list_config_help to builtin/help, 2020-04-16).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile              | 6 ++++--
 compat/vcbuild/README | 2 +-
 config.mak.uname      | 6 +++---
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index e85a8e07d3e..eb5780264a4 100644
--- a/Makefile
+++ b/Makefile
@@ -817,6 +817,8 @@ XDIFF_LIB = xdiff/lib.a
 
 GENERATED_H += command-list.h
 GENERATED_H += config-list.h
+.PHONY: generated-hdrs
+generated-hdrs: $(GENERATED_H)
 
 LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
 	$(FIND) . \
@@ -2910,7 +2912,7 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
 .PHONY: sparse $(SP_OBJ)
 sparse: $(SP_OBJ)
 
-EXCEPT_HDRS := command-list.h config-list.h unicode-width.h compat/% xdiff/%
+EXCEPT_HDRS := $(GENERATED_H) unicode-width.h compat/% xdiff/%
 ifndef GCRYPT_SHA256
 	EXCEPT_HDRS += sha256/gcrypt.h
 endif
@@ -2933,7 +2935,7 @@ style:
 	git clang-format --style file --diff --extensions c,h
 
 .PHONY: check
-check: config-list.h command-list.h
+check: $(GENERATED_H)
 	@if sparse; \
 	then \
 		echo >&2 "Use 'make sparse' instead"; \
diff --git a/compat/vcbuild/README b/compat/vcbuild/README
index 51fb083dbbe..29ec1d0f104 100644
--- a/compat/vcbuild/README
+++ b/compat/vcbuild/README
@@ -92,7 +92,7 @@ The Steps of Build Git with VS2008
    the git operations.
 
 3. Inside Git's directory run the command:
-       make command-list.h config-list.h
+       make generated-hdrs
    to generate the header file needed to compile git.
 
 4. Then either build Git with the GNU Make Makefile in the Git projects
diff --git a/config.mak.uname b/config.mak.uname
index 76516aaa9a5..8aac06eb094 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -735,9 +735,9 @@ vcxproj:
 	 echo '</Project>') >git-remote-http/LinkOrCopyRemoteHttp.targets
 	git add -f git/LinkOrCopyBuiltins.targets git-remote-http/LinkOrCopyRemoteHttp.targets
 
-	# Add command-list.h and config-list.h
-	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 config-list.h command-list.h
-	git add -f config-list.h command-list.h
+	# Add generated headers
+	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 $(GENERATED_H)
+	git add -f $(GENERATED_H)
 
 	# Add scripts
 	rm -f perl/perl.mak
-- 
2.33.0.1229.g0a86d28df49

[PATCH 3/8] Makefile: don't perform "mv $@+ $@" dance for $(GENERATED_H)

[Ævar Arnfjörð Bjarmason]

Change the "cmd.sh > $@+ && mv $@+ $@" pattern used for generating the
config-list.h and command-list.h to just "cmd.sh >$@". This was needed
as a guard to ensure that we don't have an empty file if the script
failed, but since 7b76d6bf221 (Makefile: add and use the
".DELETE_ON_ERROR" flag, 2021-06-29) GNU make ensures that doesn't
happen.

There's still a lot of other places in the Makefile where we
needlessly use this pattern, but I'm just changing these because I'm
about to add a new $(GENERATED_H) target, let's have them all look and
act the same way.

Even with ".DELETE_ON_ERROR" there is still a point to using the "mv
$@+ $@" pattern in some cases, e.g. to ensure that you have a working
binary during recompilation (see [1] for the start of a long
discussion about that), but that doesn't apply here. Nothing external
uses $(GENERATED_H) directly, it's only ever used in the context of
the Makefile's own dependency (re-)generation.

1. https://lore.kernel.org/git/8735t93h0u.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index eb5780264a4..c529f283dcc 100644
--- a/Makefile
+++ b/Makefile
@@ -2238,15 +2238,14 @@ $(BUILT_INS): git$X
 config-list.h: generate-configlist.sh
 
 config-list.h: Documentation/*config.txt Documentation/config/*.txt
-	$(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \
-		>$@+ && mv $@+ $@
+	$(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh >$@
 
 command-list.h: generate-cmdlist.sh command-list.txt
 
 command-list.h: $(wildcard Documentation/git*.txt)
 	$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \
 		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
-		command-list.txt >$@+ && mv $@+ $@
+		command-list.txt >$@
 
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
 	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
-- 
2.33.0.1229.g0a86d28df49

[PATCH 4/8] Makefile: remove an out-of-date comment

[Ævar Arnfjörð Bjarmason]

This comment added in dfea575017 (Makefile: lazily compute header
dependencies, 2010-01-26) has been out of date since
92b88eba9f (Makefile: use `git ls-files` to list header files, if
possible, 2019-03-04), when we did exactly what it tells us not to do
and added $(GENERATED_H) to $(OBJECTS) dependencies.

The rest of it was also somewhere between inaccurate and outdated,
since as of b8ba629264 (Makefile: fold MISC_H into LIB_H, 2012-06-20)
it's not followed by a list of header files, that got moved earlier in
the file into LIB_H in 60d24dd255 (Makefile: fold XDIFF_H and VCSSVN_H
into LIB_H, 2012-07-06).

Let's just remove it entirely, to the extent that we have anything
useful to say here the comment on the
"USE_COMPUTED_HEADER_DEPENDENCIES" variable a few lines above this
change does the job for us.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/Makefile b/Makefile
index c529f283dcc..c50115cd565 100644
--- a/Makefile
+++ b/Makefile
@@ -2508,13 +2508,6 @@ ifneq ($(dep_files_present),)
 include $(dep_files_present)
 endif
 else
-# Dependencies on header files, for platforms that do not support
-# the gcc -MMD option.
-#
-# Dependencies on automatically generated headers such as command-list.h
-# should _not_ be included here, since they are necessary even when
-# building an object for the first time.
-
 $(OBJECTS): $(LIB_H) $(GENERATED_H)
 endif
 
-- 
2.33.0.1229.g0a86d28df49

[PATCH 5/8] hook.[ch]: move find_hook() from run-command.c to hook.c

[Ævar Arnfjörð Bjarmason]

Move the find_hook() function from run-command.c to a new hook.c
library. This change establishes a stub library that's pretty
pointless right now, but will see much wider use with Emily Shaffer's
upcoming "configuration-based hooks" series.

Eventually all the hook related code will live in hook.[ch]. Let's
start that process by moving the simple find_hook() function over
as-is.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile               |  1 +
 builtin/am.c           |  1 +
 builtin/bugreport.c    |  2 +-
 builtin/commit.c       |  1 +
 builtin/merge.c        |  1 +
 builtin/receive-pack.c |  1 +
 builtin/worktree.c     |  1 +
 hook.c                 | 37 +++++++++++++++++++++++++++++++++++++
 hook.h                 | 11 +++++++++++
 refs.c                 |  1 +
 run-command.c          | 35 +----------------------------------
 run-command.h          |  7 -------
 sequencer.c            |  1 +
 transport.c            |  1 +
 14 files changed, 59 insertions(+), 42 deletions(-)
 create mode 100644 hook.c
 create mode 100644 hook.h

diff --git a/Makefile b/Makefile
index c50115cd565..ad2aeff68f0 100644
--- a/Makefile
+++ b/Makefile
@@ -904,6 +904,7 @@ LIB_OBJS += hash-lookup.o
 LIB_OBJS += hashmap.o
 LIB_OBJS += help.o
 LIB_OBJS += hex.o
+LIB_OBJS += hook.o
 LIB_OBJS += ident.o
 LIB_OBJS += json-writer.o
 LIB_OBJS += kwset.o
diff --git a/builtin/am.c b/builtin/am.c
index 60f3737f99c..85e3d92376b 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -11,6 +11,7 @@
 #include "parse-options.h"
 #include "dir.h"
 #include "run-command.h"
+#include "hook.h"
 #include "quote.h"
 #include "tempfile.h"
 #include "lockfile.h"
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 06ed10dc92d..c30a360d695 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -3,7 +3,7 @@
 #include "strbuf.h"
 #include "help.h"
 #include "compat/compiler.h"
-#include "run-command.h"
+#include "hook.h"
 
 
 static void get_system_info(struct strbuf *sys_info)
diff --git a/builtin/commit.c b/builtin/commit.c
index e7320f66f95..5359d961d22 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -19,6 +19,7 @@
 #include "revision.h"
 #include "wt-status.h"
 #include "run-command.h"
+#include "hook.h"
 #include "refs.h"
 #include "log-tree.h"
 #include "strbuf.h"
diff --git a/builtin/merge.c b/builtin/merge.c
index 3fbdacc7db4..fe664f6a863 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -13,6 +13,7 @@
 #include "builtin.h"
 #include "lockfile.h"
 #include "run-command.h"
+#include "hook.h"
 #include "diff.h"
 #include "diff-merges.h"
 #include "refs.h"
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 48960a9575b..e3895aec622 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -7,6 +7,7 @@
 #include "pkt-line.h"
 #include "sideband.h"
 #include "run-command.h"
+#include "hook.h"
 #include "exec-cmd.h"
 #include "commit.h"
 #include "object.h"
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 0d0a80da61f..d22ece93e1a 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -8,6 +8,7 @@
 #include "branch.h"
 #include "refs.h"
 #include "run-command.h"
+#include "hook.h"
 #include "sigchain.h"
 #include "submodule.h"
 #include "utf8.h"
diff --git a/hook.c b/hook.c
new file mode 100644
index 00000000000..ba70b314718
--- /dev/null
+++ b/hook.c
@@ -0,0 +1,37 @@
+#include "cache.h"
+#include "hook.h"
+#include "run-command.h"
+
+const char *find_hook(const char *name)
+{
+	static struct strbuf path = STRBUF_INIT;
+
+	strbuf_reset(&path);
+	strbuf_git_path(&path, "hooks/%s", name);
+	if (access(path.buf, X_OK) < 0) {
+		int err = errno;
+
+#ifdef STRIP_EXTENSION
+		strbuf_addstr(&path, STRIP_EXTENSION);
+		if (access(path.buf, X_OK) >= 0)
+			return path.buf;
+		if (errno == EACCES)
+			err = errno;
+#endif
+
+		if (err == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) {
+			static struct string_list advise_given = STRING_LIST_INIT_DUP;
+
+			if (!string_list_lookup(&advise_given, name)) {
+				string_list_insert(&advise_given, name);
+				advise(_("The '%s' hook was ignored because "
+					 "it's not set as executable.\n"
+					 "You can disable this warning with "
+					 "`git config advice.ignoredHook false`."),
+				       path.buf);
+			}
+		}
+		return NULL;
+	}
+	return path.buf;
+}
diff --git a/hook.h b/hook.h
new file mode 100644
index 00000000000..68624f16059
--- /dev/null
+++ b/hook.h
@@ -0,0 +1,11 @@
+#ifndef HOOK_H
+#define HOOK_H
+
+/*
+ * Returns the path to the hook file, or NULL if the hook is missing
+ * or disabled. Note that this points to static storage that will be
+ * overwritten by further calls to find_hook and run_hook_*.
+ */
+const char *find_hook(const char *name);
+
+#endif
diff --git a/refs.c b/refs.c
index 8b9f7c3a80a..6211692eaae 100644
--- a/refs.c
+++ b/refs.c
@@ -10,6 +10,7 @@
 #include "refs.h"
 #include "refs/refs-internal.h"
 #include "run-command.h"
+#include "hook.h"
 #include "object-store.h"
 #include "object.h"
 #include "tag.h"
diff --git a/run-command.c b/run-command.c
index 821245672d6..222a48e1400 100644
--- a/run-command.c
+++ b/run-command.c
@@ -9,6 +9,7 @@
 #include "quote.h"
 #include "config.h"
 #include "packfile.h"
+#include "hook.h"
 
 void child_process_init(struct child_process *child)
 {
@@ -1322,40 +1323,6 @@ int async_with_fork(void)
 #endif
 }
 
-const char *find_hook(const char *name)
-{
-	static struct strbuf path = STRBUF_INIT;
-
-	strbuf_reset(&path);
-	strbuf_git_path(&path, "hooks/%s", name);
-	if (access(path.buf, X_OK) < 0) {
-		int err = errno;
-
-#ifdef STRIP_EXTENSION
-		strbuf_addstr(&path, STRIP_EXTENSION);
-		if (access(path.buf, X_OK) >= 0)
-			return path.buf;
-		if (errno == EACCES)
-			err = errno;
-#endif
-
-		if (err == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) {
-			static struct string_list advise_given = STRING_LIST_INIT_DUP;
-
-			if (!string_list_lookup(&advise_given, name)) {
-				string_list_insert(&advise_given, name);
-				advise(_("The '%s' hook was ignored because "
-					 "it's not set as executable.\n"
-					 "You can disable this warning with "
-					 "`git config advice.ignoredHook false`."),
-				       path.buf);
-			}
-		}
-		return NULL;
-	}
-	return path.buf;
-}
-
 int run_hook_ve(const char *const *env, const char *name, va_list args)
 {
 	struct child_process hook = CHILD_PROCESS_INIT;
diff --git a/run-command.h b/run-command.h
index ad207daced7..a9fd2d24f00 100644
--- a/run-command.h
+++ b/run-command.h
@@ -212,13 +212,6 @@ int finish_command_in_signal(struct child_process *);
  */
 int run_command(struct child_process *);
 
-/*
- * Returns the path to the hook file, or NULL if the hook is missing
- * or disabled. Note that this points to static storage that will be
- * overwritten by further calls to find_hook and run_hook_*.
- */
-const char *find_hook(const char *name);
-
 /**
  * Run a hook.
  * The first argument is a pathname to an index file, or NULL
diff --git a/sequencer.c b/sequencer.c
index 614d56f5e21..8ee6c4ac240 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -8,6 +8,7 @@
 #include "sequencer.h"
 #include "tag.h"
 #include "run-command.h"
+#include "hook.h"
 #include "exec-cmd.h"
 #include "utf8.h"
 #include "cache-tree.h"
diff --git a/transport.c b/transport.c
index b37664ba870..5d31f7cc131 100644
--- a/transport.c
+++ b/transport.c
@@ -2,6 +2,7 @@
 #include "config.h"
 #include "transport.h"
 #include "run-command.h"
+#include "hook.h"
 #include "pkt-line.h"
 #include "fetch-pack.h"
 #include "remote.h"
-- 
2.33.0.1229.g0a86d28df49

[PATCH 6/8] hook.c: add a hook_exists() wrapper and use it in bugreport.c

[Ævar Arnfjörð Bjarmason]

From: Emily Shaffer <emilyshaffer@google.com>

Add a boolean version of the find_hook() function for those callers
who are only interested in checking whether the hook exists, not what
the path to it is.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bugreport.c | 2 +-
 hook.c              | 5 +++++
 hook.h              | 5 +++++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index c30a360d695..a02c2540bb1 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -82,7 +82,7 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 	}
 
 	for (i = 0; i < ARRAY_SIZE(hook); i++)
-		if (find_hook(hook[i]))
+		if (hook_exists(hook[i]))
 			strbuf_addf(hook_info, "%s\n", hook[i]);
 }
 
diff --git a/hook.c b/hook.c
index ba70b314718..55e1145a4b7 100644
--- a/hook.c
+++ b/hook.c
@@ -35,3 +35,8 @@ const char *find_hook(const char *name)
 	}
 	return path.buf;
 }
+
+int hook_exists(const char *name)
+{
+	return !!find_hook(name);
+}
diff --git a/hook.h b/hook.h
index 68624f16059..6aa36fc7ff9 100644
--- a/hook.h
+++ b/hook.h
@@ -8,4 +8,9 @@
  */
 const char *find_hook(const char *name);
 
+/**
+ * A boolean version of find_hook()
+ */
+int hook_exists(const char *hookname);
+
 #endif
-- 
2.33.0.1229.g0a86d28df49

[PATCH 7/8] hook.c users: use "hook_exists()" instead of "find_hook()"

[Ævar Arnfjörð Bjarmason]

Use the new hook_exists() function instead of find_hook() where the
latter was called in boolean contexts. This make subsequent changes in
a series where we further refactor the hook API clearer, as we won't
conflate wanting to get the path of the hook with checking for its
existence.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit.c       | 2 +-
 builtin/merge.c        | 2 +-
 builtin/receive-pack.c | 2 +-
 sequencer.c            | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 5359d961d22..883c16256c8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1052,7 +1052,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (!no_verify && find_hook("pre-commit")) {
+	if (!no_verify && hook_exists("pre-commit")) {
 		/*
 		 * Re-read the index as pre-commit hook could have updated it,
 		 * and write it out as a tree.  We must do this before we invoke
diff --git a/builtin/merge.c b/builtin/merge.c
index fe664f6a863..956b6259f21 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -849,7 +849,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	 * and write it out as a tree.  We must do this before we invoke
 	 * the editor and after we invoke run_status above.
 	 */
-	if (find_hook("pre-merge-commit"))
+	if (hook_exists("pre-merge-commit"))
 		discard_cache();
 	read_cache_from(index_file);
 	strbuf_addbuf(&msg, &merge_msg);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e3895aec622..25cc0c907e1 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1464,7 +1464,7 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
 
 	strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
 
-	if (!find_hook(push_to_checkout_hook))
+	if (!hook_exists(push_to_checkout_hook))
 		retval = push_to_deploy(sha1, &env, work_tree);
 	else
 		retval = push_to_checkout(sha1, &env, work_tree);
diff --git a/sequencer.c b/sequencer.c
index 8ee6c4ac240..e501945796d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1460,7 +1460,7 @@ static int try_to_commit(struct repository *r,
 		}
 	}
 
-	if (find_hook("prepare-commit-msg")) {
+	if (hook_exists("prepare-commit-msg")) {
 		res = run_prepare_commit_msg_hook(r, msg, hook_commit);
 		if (res)
 			goto out;
-- 
2.33.0.1229.g0a86d28df49

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

[Ævar Arnfjörð Bjarmason]

Make githooks(5) the source of truth for what hooks git supports, and
punt out early on 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.

We could make the find_hook() function die() or BUG() out if the new
known_hook() returned 0, but let's make it return NULL just as it does
when it can't find a hook of a known type. Making it die() is overly
anal, and unlikely to be what we need in catching stupid typos in the
name of some new hook hardcoded in git.git's sources. By making this
be tolerant of unknown hook names, changes in a later series to make
"git hook run" run arbitrary user-configured hook names will be easier
to implement.

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>
Helped-by: René Scharfe <l.s.r@web.de>
---
 .gitignore                          |  1 +
 Makefile                            |  9 +++++-
 builtin/bugreport.c                 | 44 ++++++-----------------------
 contrib/buildsystems/CMakeLists.txt |  7 +++++
 generate-hooklist.sh                | 18 ++++++++++++
 5 files changed, 42 insertions(+), 37 deletions(-)
 create mode 100755 generate-hooklist.sh

diff --git a/.gitignore b/.gitignore
index 311841f9bed..6be9de41ae8 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 ad2aeff68f0..f883657fa26 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)
 
@@ -2216,7 +2218,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)"' \
@@ -2248,6 +2252,9 @@ command-list.h: $(wildcard Documentation/git*.txt)
 		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
 		command-list.txt >$@
 
+hook-list.h: generate-hooklist.sh Documentation/githooks.txt
+	$(QUIET_GEN)$(SHELL_PATH) ./generate-hooklist.sh >$@
+
 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 a02c2540bb1..9de32bc96e7 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 171b4124afe..fd1399c440f 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -624,6 +624,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 00000000000..6d4e56d1a31
--- /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
-- 
2.33.0.1229.g0a86d28df49

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

[Phillip Wood]

Hi Ævar
On 23/09/2021 11:30, Ævar Arnfjörð Bjarmason wrote:
> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
> new file mode 100755
> index 00000000000..6d4e56d1a31
> --- /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' \

POSIX does not support using a semicolon after a closing brace [1], 
grepping our code base with
	git grep 'sed .*};' '*.sh'
does not give any matches so I don't think we're using that pattern any 
where else. Replacing the semicolon with ' -e' would fix it.

Best Wishes

Phillip

[1] "Editing commands other that {...}, a, b, c, i, r, t, w, : and # can 
be followed by a <semicolon>" from 
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>> +sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
>
> POSIX does not support using a semicolon after a closing brace [1],
> ...
> [1] "Editing commands other that {...}, a, b, c, i, r, t, w, : and #
> can be followed by a <semicolon>" from 
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html

Would sed implementation of BSD ancestry fail reliably to be a good
coalmine canary?

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

[René Scharfe]

Am 24.09.21 um 17:51 schrieb Junio C Hamano:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>>> +sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
>>
>> POSIX does not support using a semicolon after a closing brace [1],
>> ...
>> [1] "Editing commands other that {...}, a, b, c, i, r, t, w, : and #
>> can be followed by a <semicolon>" from
>> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html
>
> Would sed implementation of BSD ancestry fail reliably to be a good
> coalmine canary?

I doubt it.  https://man.openbsd.org/sed says:

   "Multiple commands may be specified separated by newlines or
    semicolons"

   "The a, c, i, r, and w functions cannot be followed by another
    command separated with a semicolon."

   "Following the b, t, or : commands with a semicolon and another
    command is an extension to the specification."

   "The use of newlines to separate multiple commands on the command
    line is non-portable"

https://www.freebsd.org/cgi/man.cgi?query=sed doesn't mention semicolons
at all.

On macOS 11.6 (FreeBSD-based userland) the semicolon is accepted:

   $ echo a | sed -n -e '{p;}; p'
   a
   a

René

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

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 24 2021, Phillip Wood wrote:

> Hi Ævar
> On 23/09/2021 11:30, Ævar Arnfjörð Bjarmason wrote:
>> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
>> new file mode 100755
>> index 00000000000..6d4e56d1a31
>> --- /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' \
>
> POSIX does not support using a semicolon after a closing brace [1],
> grepping our code base with
> 	git grep 'sed .*};' '*.sh'
> does not give any matches so I don't think we're using that pattern
> any where else. Replacing the semicolon with ' -e' would fix it.
>
> Best Wishes
>
> Phillip

Does this fail on any system you're aware of? If so what OS/version (and
preferably version of "sed").

René's downthread <d5f297d4-9c64-1ff9-8422-054979bf8cfa@web.de> seems to
suggest that this is fine.

Both beforehand and just now I've tested this on AIX, Solaris,
{Open,Net,Free}BSD, HP/UX, OSX and Linux (a few distros/versions).

All of them are able to generate the same hook-list.h using this version
of the patch.

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

[René Scharfe]

Am 24.09.21 um 21:30 schrieb Ævar Arnfjörð Bjarmason:
>
> On Fri, Sep 24 2021, Phillip Wood wrote:
>
>> Hi Ævar
>> On 23/09/2021 11:30, Ævar Arnfjörð Bjarmason wrote:
>>> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
>>> new file mode 100755
>>> index 00000000000..6d4e56d1a31
>>> --- /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' \
>>
>> POSIX does not support using a semicolon after a closing brace [1],
>> grepping our code base with
>> 	git grep 'sed .*};' '*.sh'
>> does not give any matches so I don't think we're using that pattern
>> any where else. Replacing the semicolon with ' -e' would fix it.
>>
>> Best Wishes
>>
>> Phillip
>
> Does this fail on any system you're aware of? If so what OS/version (and
> preferably version of "sed").

I wasn't aware of that restriction and my gut feeling is that enforcing
it in a sed implementation would be slightly harder than allowing "};".

> René's downthread <d5f297d4-9c64-1ff9-8422-054979bf8cfa@web.de> seems to
> suggest that this is fine.

That said, I didn't mean to suggest that we keep using this construct,
just that I couldn't find an implementation which would reject it.

> Both beforehand and just now I've tested this on AIX, Solaris,
> {Open,Net,Free}BSD, HP/UX, OSX and Linux (a few distros/versions).
>
> All of them are able to generate the same hook-list.h using this version
> of the patch.

Impressive list, but still it's probably better to move the last "x" to
its own -e, to appease the POSIX spirits.  Small change..

René

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

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 24 2021, René Scharfe wrote:

> Am 24.09.21 um 21:30 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Fri, Sep 24 2021, Phillip Wood wrote:
>>
>>> Hi Ævar
>>> On 23/09/2021 11:30, Ævar Arnfjörð Bjarmason wrote:
>>>> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
>>>> new file mode 100755
>>>> index 00000000000..6d4e56d1a31
>>>> --- /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' \
>>>
>>> POSIX does not support using a semicolon after a closing brace [1],
>>> grepping our code base with
>>> 	git grep 'sed .*};' '*.sh'
>>> does not give any matches so I don't think we're using that pattern
>>> any where else. Replacing the semicolon with ' -e' would fix it.
>>>
>>> Best Wishes
>>>
>>> Phillip
>>
>> Does this fail on any system you're aware of? If so what OS/version (and
>> preferably version of "sed").
>
> I wasn't aware of that restriction and my gut feeling is that enforcing
> it in a sed implementation would be slightly harder than allowing "};".
>
>> René's downthread <d5f297d4-9c64-1ff9-8422-054979bf8cfa@web.de> seems to
>> suggest that this is fine.
>
> That said, I didn't mean to suggest that we keep using this construct,
> just that I couldn't find an implementation which would reject it.

I'll re-test without "};", my (seemingly incorrect) quick read of your
E-Mail + my own testing was that semicolons were fine there, but maybe
they're just accepted by most...

>> Both beforehand and just now I've tested this on AIX, Solaris,
>> {Open,Net,Free}BSD, HP/UX, OSX and Linux (a few distros/versions).
>>
>> All of them are able to generate the same hook-list.h using this version
>> of the patch.
>
> Impressive list.

FWIW that's just the GCC Farm which is fairly easy to get access to for
free software devlopment: https://cfarm.tetaneutral.net/machines/list/

The HP/UX box is a similar "private" setup mainly for Perl5 core
development, I'm getting to use it to get git working there.

> but still it's probably better to move the last "x" to its own -e, to
> appease the POSIX spirits.  Small change..

*nod*, will test with some version of that.

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

[Phillip Wood]

On 24/09/2021 20:30, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Sep 24 2021, Phillip Wood wrote:
> 
>> Hi Ævar
>> On 23/09/2021 11:30, Ævar Arnfjörð Bjarmason wrote:
>>> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
>>> new file mode 100755
>>> index 00000000000..6d4e56d1a31
>>> --- /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' \
>>
>> POSIX does not support using a semicolon after a closing brace [1],
>> grepping our code base with
>> 	git grep 'sed .*};' '*.sh'
>> does not give any matches so I don't think we're using that pattern
>> any where else. Replacing the semicolon with ' -e' would fix it.
>>
>> Best Wishes
>>
>> Phillip
> 
> Does this fail on any system you're aware of? If so what OS/version (and
> preferably version of "sed").

I'm not aware of any such system but I rarely use anything other than 
linux. As this departure from POSIX is not already in the code base I 
thought it was worth flagging it. I did wonder if it would be supported 
by the various BSDs but you testing shows it is actually quite widely 
supported.

Best Wishes

Phillip

> René's downthread <d5f297d4-9c64-1ff9-8422-054979bf8cfa@web.de> seems to
> suggest that this is fine.
> 
> Both beforehand and just now I've tested this on AIX, Solaris,
> {Open,Net,Free}BSD, HP/UX, OSX and Linux (a few distros/versions).
> 
> All of them are able to generate the same hook-list.h using this version
> of the patch.
> 

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

[Ævar Arnfjörð Bjarmason]

On Mon, Sep 27 2021, Phillip Wood wrote:

> On 24/09/2021 20:30, Ævar Arnfjörð Bjarmason wrote:
>> On Fri, Sep 24 2021, Phillip Wood wrote:
>> 
>>> Hi Ævar
>>> On 23/09/2021 11:30, Ævar Arnfjörð Bjarmason wrote:
>>>> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
>>>> new file mode 100755
>>>> index 00000000000..6d4e56d1a31
>>>> --- /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' \
>>>
>>> POSIX does not support using a semicolon after a closing brace [1],
>>> grepping our code base with
>>> 	git grep 'sed .*};' '*.sh'
>>> does not give any matches so I don't think we're using that pattern
>>> any where else. Replacing the semicolon with ' -e' would fix it.
>>>
>>> Best Wishes
>>>
>>> Phillip
>> Does this fail on any system you're aware of? If so what OS/version
>> (and
>> preferably version of "sed").
>
> I'm not aware of any such system but I rarely use anything other than
> linux. As this departure from POSIX is not already in the code base I 
> thought it was worth flagging it. I did wonder if it would be
> supported by the various BSDs but you testing shows it is actually
> quite widely supported.
>
> Best Wishes
>
> Phillip

Thanks, I completely agree that it's worth changing either way (as I did
in my v2). I just wondered if it was a careful reading of the standard
or having run into it on a system in the wild, in that case I'd try to
test on that system in the future.

Good eyes :)

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

[Mike Hommey]

On Thu, Sep 23, 2021 at 12:30:03PM +0200, Ævar Arnfjörð Bjarmason wrote:
> -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

hook-list.h is only included from buitin/bugreport.c, so
builtin/bugreport.o should be the one with the hook-list.h dependency,
shouldn't it?

Mike

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

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 16 2021, Mike Hommey wrote:

> On Thu, Sep 23, 2021 at 12:30:03PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> -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
>
> hook-list.h is only included from buitin/bugreport.c, so
> builtin/bugreport.o should be the one with the hook-list.h dependency,
> shouldn't it?

Well spotted, yes. This is a mistake. I think from some earlier WIP
version of the series.

In practice we don't really miss dependencies due to these sorts of
mistakes since we use the .depends files, i.e. GCC & Clang figure this
out for us:

    $ grep hook-list .depend/* */.depend/*
    builtin/.depend/bugreport.o.d: compat/compiler.h git-compat-util.h hook.h hook-list.h
    builtin/.depend/bugreport.o.d:hook-list.h:

But we do over-depend a bit, if you touch hook-list.h and make
builtin/help.o we'll re-generate it due to this line.

If you or anyone else is more generally interested in the Makefile I'd
really like to get some reviews over at:
https://lore.kernel.org/git/cover-v2-00.18-00000000000-20211112T214150Z-avarab@gmail.com/

I've got some follow-up work that solve these dependencies more
generally, e.g. in this case we should really not need to slavishly
maintain these fallback dependency lists by hand, or have automated ways
of validating their correctness.

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

[Mike Hommey]

On Mon, Nov 15, 2021 at 11:26:36PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Nov 16 2021, Mike Hommey wrote:
> 
> > On Thu, Sep 23, 2021 at 12:30:03PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >> -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
> >
> > hook-list.h is only included from buitin/bugreport.c, so
> > builtin/bugreport.o should be the one with the hook-list.h dependency,
> > shouldn't it?
> 
> Well spotted, yes. This is a mistake. I think from some earlier WIP
> version of the series.
> 
> In practice we don't really miss dependencies due to these sorts of
> mistakes since we use the .depends files, i.e. GCC & Clang figure this
> out for us:
> 
>     $ grep hook-list .depend/* */.depend/*
>     builtin/.depend/bugreport.o.d: compat/compiler.h git-compat-util.h hook.h hook-list.h
>     builtin/.depend/bugreport.o.d:hook-list.h:

But aren't those .depends files are only created when compiling object
files, such that builtin/.depend/bugreport.o.d wouldn't exist until
bugreport.c is compiled, which would fail if hook-list.h wasn't created
before that?

Mike

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

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 16 2021, Mike Hommey wrote:

> On Mon, Nov 15, 2021 at 11:26:36PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Tue, Nov 16 2021, Mike Hommey wrote:
>> 
>> > On Thu, Sep 23, 2021 at 12:30:03PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> >> -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
>> >
>> > hook-list.h is only included from buitin/bugreport.c, so
>> > builtin/bugreport.o should be the one with the hook-list.h dependency,
>> > shouldn't it?
>> 
>> Well spotted, yes. This is a mistake. I think from some earlier WIP
>> version of the series.
>> 
>> In practice we don't really miss dependencies due to these sorts of
>> mistakes since we use the .depends files, i.e. GCC & Clang figure this
>> out for us:
>> 
>>     $ grep hook-list .depend/* */.depend/*
>>     builtin/.depend/bugreport.o.d: compat/compiler.h git-compat-util.h hook.h hook-list.h
>>     builtin/.depend/bugreport.o.d:hook-list.h:
>
> But aren't those .depends files are only created when compiling object
> files, such that builtin/.depend/bugreport.o.d wouldn't exist until
> bugreport.c is compiled, which would fail if hook-list.h wasn't created
> before that?

Fail how?

I don't think it could fail, because the purpose of these dependency
relationships is to avoid needless *re*builds. So if you're building for
the first time it doesn't matter, your compiler will find the relevant
things to include for you. It doesn't need what's in the Makefile to do
that.

See [1], what I said about LIB_H there applies more generally for the
.depends files.

It will only fail in the sense that it over-depends, i.e. if you do:

    git clean -dxf; make builtin/help.o

We'll generate the hook list, as opposed to for buildin/add.o or
whatever, but I'd say that generally doesn't matter all that much, what
matters is that we don't miss dependency relationships.

Anyway, I'll try to loop around to fixing this, just wanted to gather
some interest in the more general set of fixes I've got. I've got some
fixes queued up after that that fix/improve the *.sp and *.s parts of
the line you quoted (and other similar lines). I'd prefer just to fix
this along with those so the two don't textually conflict.

1. https://lore.kernel.org/git/211110.86h7cki0uo.gmgdl@evledraar.gmail.com/

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

[Mike Hommey]

On Mon, Nov 15, 2021 at 11:49:31PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Nov 16 2021, Mike Hommey wrote:
> 
> > On Mon, Nov 15, 2021 at 11:26:36PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> 
> >> On Tue, Nov 16 2021, Mike Hommey wrote:
> >> 
> >> > On Thu, Sep 23, 2021 at 12:30:03PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >> >> -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
> >> >
> >> > hook-list.h is only included from buitin/bugreport.c, so
> >> > builtin/bugreport.o should be the one with the hook-list.h dependency,
> >> > shouldn't it?
> >> 
> >> Well spotted, yes. This is a mistake. I think from some earlier WIP
> >> version of the series.
> >> 
> >> In practice we don't really miss dependencies due to these sorts of
> >> mistakes since we use the .depends files, i.e. GCC & Clang figure this
> >> out for us:
> >> 
> >>     $ grep hook-list .depend/* */.depend/*
> >>     builtin/.depend/bugreport.o.d: compat/compiler.h git-compat-util.h hook.h hook-list.h
> >>     builtin/.depend/bugreport.o.d:hook-list.h:
> >
> > But aren't those .depends files are only created when compiling object
> > files, such that builtin/.depend/bugreport.o.d wouldn't exist until
> > bugreport.c is compiled, which would fail if hook-list.h wasn't created
> > before that?
> 
> Fail how?
> 
> I don't think it could fail, because the purpose of these dependency
> relationships is to avoid needless *re*builds. So if you're building for
> the first time it doesn't matter, your compiler will find the relevant
> things to include for you. It doesn't need what's in the Makefile to do
> that.
> 
> See [1], what I said about LIB_H there applies more generally for the
> .depends files.
> 
> It will only fail in the sense that it over-depends, i.e. if you do:
> 
>     git clean -dxf; make builtin/help.o

Try

    git clean -dxf; make builtin/bugreport.o

It fails with:

      CC builtin/bugreport.o
  builtin/bugreport.c:7:10: fatal error: hook-list.h: そのようなファイルやディレクトリはありません
      7 | #include "hook-list.h"
        |          ^~~~~~~~~~~~~
  compilation terminated.
  make: *** [Makefile:2500: builtin/bugreport.o] エラー 1

The only reason I can see why it builds at all normally is that hook.o
is built soon enough that by the time builtin/bugreport.o is built
hook-list.h has already been generated.

Mike

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

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 16 2021, Mike Hommey wrote:

> On Mon, Nov 15, 2021 at 11:49:31PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Tue, Nov 16 2021, Mike Hommey wrote:
>> 
>> > On Mon, Nov 15, 2021 at 11:26:36PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> >> 
>> >> On Tue, Nov 16 2021, Mike Hommey wrote:
>> >> 
>> >> > On Thu, Sep 23, 2021 at 12:30:03PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> >> >> -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
>> >> >
>> >> > hook-list.h is only included from buitin/bugreport.c, so
>> >> > builtin/bugreport.o should be the one with the hook-list.h dependency,
>> >> > shouldn't it?
>> >> 
>> >> Well spotted, yes. This is a mistake. I think from some earlier WIP
>> >> version of the series.
>> >> 
>> >> In practice we don't really miss dependencies due to these sorts of
>> >> mistakes since we use the .depends files, i.e. GCC & Clang figure this
>> >> out for us:
>> >> 
>> >>     $ grep hook-list .depend/* */.depend/*
>> >>     builtin/.depend/bugreport.o.d: compat/compiler.h git-compat-util.h hook.h hook-list.h
>> >>     builtin/.depend/bugreport.o.d:hook-list.h:
>> >
>> > But aren't those .depends files are only created when compiling object
>> > files, such that builtin/.depend/bugreport.o.d wouldn't exist until
>> > bugreport.c is compiled, which would fail if hook-list.h wasn't created
>> > before that?
>> 
>> Fail how?
>> 
>> I don't think it could fail, because the purpose of these dependency
>> relationships is to avoid needless *re*builds. So if you're building for
>> the first time it doesn't matter, your compiler will find the relevant
>> things to include for you. It doesn't need what's in the Makefile to do
>> that.
>> 
>> See [1], what I said about LIB_H there applies more generally for the
>> .depends files.
>> 
>> It will only fail in the sense that it over-depends, i.e. if you do:
>> 
>>     git clean -dxf; make builtin/help.o
>
> Try
>
>     git clean -dxf; make builtin/bugreport.o
>
> It fails with:
>
>       CC builtin/bugreport.o
>   builtin/bugreport.c:7:10: fatal error: hook-list.h: そのようなファイルやディレクトリはありません
>       7 | #include "hook-list.h"
>         |          ^~~~~~~~~~~~~
>   compilation terminated.
>   make: *** [Makefile:2500: builtin/bugreport.o] エラー 1
>
> The only reason I can see why it builds at all normally is that hook.o
> is built soon enough that by the time builtin/bugreport.o is built
> hook-list.h has already been generated.

Ah, you're obviously right. I don't know what I was thinking yesterday.

I submitted a re-roll of the greater dependency fix-up & optimization
series I've got kicking around, which includes a fix for this
issue. Thank you for the report:
https://lore.kernel.org/git/cover-v3-00.23-00000000000-20211116T114334Z-avarab@gmail.com

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I submitted a re-roll of the greater dependency fix-up & optimization
> series I've got kicking around, which includes a fix for this
> issue. Thank you for the report:

Makes me wonder what other breakages are introduced in such a big
series, in exchange for correcting this single issue X-<.  We'll see
soon enough, I guess ;-)

[PATCH v2 0/8] Makefile: generate a hook-list.h, prep for config-based-hooks

[Ævar Arnfjörð Bjarmason]

This series is an incremental restart of the now-ejected
es/config-based-hooks and ab/config-based-hooks-base topics. See [1]
for a summary of the plan and progression.

In v2 the "sed" invocation that generates the new hook-list.h has been
changed to be portable under POSIX. See the thread starting at
https://lore.kernel.org/git/92471ff9-7573-c3e4-e9fd-63a5cbf5738f@gmail.com/;

The portability issue is AFAICT theoretical in that any "sed" command
I've tried accepts the old version (I tried the large list of OS's
listed in [2]), but better safe than sorry.

Other changes:

 * I noticed that the run-command.h inclusion in transport.c become
   redundant, I removed that and validated the other ones that have
   the new hook.h, they all still need run-command.h.

 * A whitespace change in v1 in a change to the Makefile makes the
   diff for 8/8 easier to read.

1. http://lore.kernel.org/git/cover-0.8-00000000000-20210923T095326Z-avarab@gmail.com
2. https://lore.kernel.org/git/87fstt3gzd.fsf@evledraar.gmail.com/

Emily Shaffer (1):
  hook.c: add a hook_exists() wrapper and use it in bugreport.c

Ævar Arnfjörð Bjarmason (7):
  Makefile: mark "check" target as .PHONY
  Makefile: stop hardcoding {command,config}-list.h
  Makefile: don't perform "mv $@+ $@" dance for $(GENERATED_H)
  Makefile: remove an out-of-date comment
  hook.[ch]: move find_hook() from run-command.c to hook.c
  hook.c users: use "hook_exists()" instead of "find_hook()"
  hook-list.h: add a generated list of hooks, like config-list.h

 .gitignore                          |  1 +
 Makefile                            | 28 ++++++++++--------
 builtin/am.c                        |  1 +
 builtin/bugreport.c                 | 46 ++++++-----------------------
 builtin/commit.c                    |  3 +-
 builtin/merge.c                     |  3 +-
 builtin/receive-pack.c              |  3 +-
 builtin/worktree.c                  |  1 +
 compat/vcbuild/README               |  2 +-
 config.mak.uname                    |  6 ++--
 contrib/buildsystems/CMakeLists.txt |  7 +++++
 generate-hooklist.sh                | 20 +++++++++++++
 hook.c                              | 42 ++++++++++++++++++++++++++
 hook.h                              | 16 ++++++++++
 refs.c                              |  1 +
 run-command.c                       | 35 +---------------------
 run-command.h                       |  7 -----
 sequencer.c                         |  3 +-
 transport.c                         |  2 +-
 19 files changed, 127 insertions(+), 100 deletions(-)
 create mode 100755 generate-hooklist.sh
 create mode 100644 hook.c
 create mode 100644 hook.h

Range-diff against v1:
1:  91d0cd09c8e = 1:  095ef66c29e Makefile: mark "check" target as .PHONY
2:  804795771c6 = 2:  520e4baede6 Makefile: stop hardcoding {command,config}-list.h
3:  010701fd784 = 3:  312b353c651 Makefile: don't perform "mv $@+ $@" dance for $(GENERATED_H)
4:  b3cce74d259 = 4:  aea4868f988 Makefile: remove an out-of-date comment
5:  7dd874d50ec ! 5:  64f3af94161 hook.[ch]: move find_hook() from run-command.c to hook.c
    @@ sequencer.c
     
      ## transport.c ##
     @@
    + #include "cache.h"
      #include "config.h"
      #include "transport.h"
    - #include "run-command.h"
    +-#include "run-command.h"
     +#include "hook.h"
      #include "pkt-line.h"
      #include "fetch-pack.h"
6:  db8893afee6 = 6:  4eabe7ca39f hook.c: add a hook_exists() wrapper and use it in bugreport.c
7:  b61130dee5b = 7:  6237a1a5549 hook.c users: use "hook_exists()" instead of "find_hook()"
8:  80aae4d5c13 ! 8:  7420267ce09 hook-list.h: add a generated list of hooks, like config-list.h
    @@ Makefile: XDIFF_LIB = xdiff/lib.a
      generated-hdrs: $(GENERATED_H)
      
     @@ Makefile: git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
    + 		$(filter %.o,$^) $(LIBS)
      
      help.sp help.s help.o: command-list.h
    ++hook.sp hook.s hook.o: hook-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)"' \
    @@ generate-hooklist.sh (new)
     +static const char *hook_name_list[] = {
     +EOF
     +
    -+sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
    ++sed -n \
    ++	-e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}' \
    ++	-e 'x' \
     +	<Documentation/githooks.txt |
     +	LC_ALL=C sort
     +
-- 
2.33.0.1291.g8857a6a91ac

[PATCH v2 1/8] Makefile: mark "check" target as .PHONY

[Ævar Arnfjörð Bjarmason]

Fix a bug in 44c9e8594e (Fix up header file dependencies and add
sparse checking rules, 2005-07-03), we never marked the phony "check"
target as such.

Perhaps we should just remove it, since as of a combination of
912f9980d2 (Makefile: help people who run 'make check' by mistake,
2008-11-11) 0bcd9ae85d (sparse: Fix errors due to missing
target-specific variables, 2011-04-21) we've been suggesting the user
run "make sparse" directly.

But under that mode it still does something, as well as directing the
user to run "make test" under non-sparse. So let's punt that and
narrowly fix the PHONY bug.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index a9f9b689f0c..2777284b0f1 100644
--- a/Makefile
+++ b/Makefile
@@ -2925,6 +2925,7 @@ hdr-check: $(HCO)
 style:
 	git clang-format --style file --diff --extensions c,h
 
+.PHONY: check
 check: config-list.h command-list.h
 	@if sparse; \
 	then \
-- 
2.33.0.1291.g8857a6a91ac

[PATCH v2 2/8] Makefile: stop hardcoding {command,config}-list.h

[Ævar Arnfjörð Bjarmason]

Change various places that hardcode the names of these two files to
refer to either $(GENERATED_H), or to a new generated-hdrs
target. That target is consistent with the *-objs targets I recently
added in 029bac01a8 (Makefile: add {program,xdiff,test,git,fuzz}-objs
& objects targets, 2021-02-23).

A subsequent commit will add a new generated hook-list.h. By doing
this refactoring we'll only need to add the new file to the
GENERATED_H variable, not EXCEPT_HDRS, the vcbuild/README etc.

Hardcoding command-list.h there seems to have been a case of
copy/paste programming in 976aaedca0 (msvc: add a Makefile target to
pre-generate the Visual Studio solution, 2019-07-29). The
config-list.h was added later in 709df95b78 (help: move
list_config_help to builtin/help, 2020-04-16).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile              | 6 ++++--
 compat/vcbuild/README | 2 +-
 config.mak.uname      | 6 +++---
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/Makefile b/Makefile
index 2777284b0f1..47e79a6252a 100644
--- a/Makefile
+++ b/Makefile
@@ -817,6 +817,8 @@ XDIFF_LIB = xdiff/lib.a
 
 GENERATED_H += command-list.h
 GENERATED_H += config-list.h
+.PHONY: generated-hdrs
+generated-hdrs: $(GENERATED_H)
 
 LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \
 	$(FIND) . \
@@ -2903,7 +2905,7 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
 .PHONY: sparse $(SP_OBJ)
 sparse: $(SP_OBJ)
 
-EXCEPT_HDRS := command-list.h config-list.h unicode-width.h compat/% xdiff/%
+EXCEPT_HDRS := $(GENERATED_H) unicode-width.h compat/% xdiff/%
 ifndef GCRYPT_SHA256
 	EXCEPT_HDRS += sha256/gcrypt.h
 endif
@@ -2926,7 +2928,7 @@ style:
 	git clang-format --style file --diff --extensions c,h
 
 .PHONY: check
-check: config-list.h command-list.h
+check: $(GENERATED_H)
 	@if sparse; \
 	then \
 		echo >&2 "Use 'make sparse' instead"; \
diff --git a/compat/vcbuild/README b/compat/vcbuild/README
index 51fb083dbbe..29ec1d0f104 100644
--- a/compat/vcbuild/README
+++ b/compat/vcbuild/README
@@ -92,7 +92,7 @@ The Steps of Build Git with VS2008
    the git operations.
 
 3. Inside Git's directory run the command:
-       make command-list.h config-list.h
+       make generated-hdrs
    to generate the header file needed to compile git.
 
 4. Then either build Git with the GNU Make Makefile in the Git projects
diff --git a/config.mak.uname b/config.mak.uname
index 76516aaa9a5..8aac06eb094 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -735,9 +735,9 @@ vcxproj:
 	 echo '</Project>') >git-remote-http/LinkOrCopyRemoteHttp.targets
 	git add -f git/LinkOrCopyBuiltins.targets git-remote-http/LinkOrCopyRemoteHttp.targets
 
-	# Add command-list.h and config-list.h
-	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 config-list.h command-list.h
-	git add -f config-list.h command-list.h
+	# Add generated headers
+	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 $(GENERATED_H)
+	git add -f $(GENERATED_H)
 
 	# Add scripts
 	rm -f perl/perl.mak
-- 
2.33.0.1291.g8857a6a91ac

[PATCH v2 3/8] Makefile: don't perform "mv $@+ $@" dance for $(GENERATED_H)

[Ævar Arnfjörð Bjarmason]

Change the "cmd.sh > $@+ && mv $@+ $@" pattern used for generating the
config-list.h and command-list.h to just "cmd.sh >$@". This was needed
as a guard to ensure that we don't have an empty file if the script
failed, but since 7b76d6bf221 (Makefile: add and use the
".DELETE_ON_ERROR" flag, 2021-06-29) GNU make ensures that doesn't
happen.

There's still a lot of other places in the Makefile where we
needlessly use this pattern, but I'm just changing these because I'm
about to add a new $(GENERATED_H) target, let's have them all look and
act the same way.

Even with ".DELETE_ON_ERROR" there is still a point to using the "mv
$@+ $@" pattern in some cases, e.g. to ensure that you have a working
binary during recompilation (see [1] for the start of a long
discussion about that), but that doesn't apply here. Nothing external
uses $(GENERATED_H) directly, it's only ever used in the context of
the Makefile's own dependency (re-)generation.

1. https://lore.kernel.org/git/8735t93h0u.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 47e79a6252a..e6b8d00f221 100644
--- a/Makefile
+++ b/Makefile
@@ -2231,15 +2231,14 @@ $(BUILT_INS): git$X
 config-list.h: generate-configlist.sh
 
 config-list.h: Documentation/*config.txt Documentation/config/*.txt
-	$(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh \
-		>$@+ && mv $@+ $@
+	$(QUIET_GEN)$(SHELL_PATH) ./generate-configlist.sh >$@
 
 command-list.h: generate-cmdlist.sh command-list.txt
 
 command-list.h: $(wildcard Documentation/git*.txt)
 	$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh \
 		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
-		command-list.txt >$@+ && mv $@+ $@
+		command-list.txt >$@
 
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
 	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
-- 
2.33.0.1291.g8857a6a91ac

[PATCH v2 4/8] Makefile: remove an out-of-date comment

[Ævar Arnfjörð Bjarmason]

This comment added in dfea575017 (Makefile: lazily compute header
dependencies, 2010-01-26) has been out of date since
92b88eba9f (Makefile: use `git ls-files` to list header files, if
possible, 2019-03-04), when we did exactly what it tells us not to do
and added $(GENERATED_H) to $(OBJECTS) dependencies.

The rest of it was also somewhere between inaccurate and outdated,
since as of b8ba629264 (Makefile: fold MISC_H into LIB_H, 2012-06-20)
it's not followed by a list of header files, that got moved earlier in
the file into LIB_H in 60d24dd255 (Makefile: fold XDIFF_H and VCSSVN_H
into LIB_H, 2012-07-06).

Let's just remove it entirely, to the extent that we have anything
useful to say here the comment on the
"USE_COMPUTED_HEADER_DEPENDENCIES" variable a few lines above this
change does the job for us.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/Makefile b/Makefile
index e6b8d00f221..b9bf13239d0 100644
--- a/Makefile
+++ b/Makefile
@@ -2501,13 +2501,6 @@ ifneq ($(dep_files_present),)
 include $(dep_files_present)
 endif
 else
-# Dependencies on header files, for platforms that do not support
-# the gcc -MMD option.
-#
-# Dependencies on automatically generated headers such as command-list.h
-# should _not_ be included here, since they are necessary even when
-# building an object for the first time.
-
 $(OBJECTS): $(LIB_H) $(GENERATED_H)
 endif
 
-- 
2.33.0.1291.g8857a6a91ac

[PATCH v2 5/8] hook.[ch]: move find_hook() from run-command.c to hook.c

[Ævar Arnfjörð Bjarmason]

Move the find_hook() function from run-command.c to a new hook.c
library. This change establishes a stub library that's pretty
pointless right now, but will see much wider use with Emily Shaffer's
upcoming "configuration-based hooks" series.

Eventually all the hook related code will live in hook.[ch]. Let's
start that process by moving the simple find_hook() function over
as-is.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile               |  1 +
 builtin/am.c           |  1 +
 builtin/bugreport.c    |  2 +-
 builtin/commit.c       |  1 +
 builtin/merge.c        |  1 +
 builtin/receive-pack.c |  1 +
 builtin/worktree.c     |  1 +
 hook.c                 | 37 +++++++++++++++++++++++++++++++++++++
 hook.h                 | 11 +++++++++++
 refs.c                 |  1 +
 run-command.c          | 35 +----------------------------------
 run-command.h          |  7 -------
 sequencer.c            |  1 +
 transport.c            |  2 +-
 14 files changed, 59 insertions(+), 43 deletions(-)
 create mode 100644 hook.c
 create mode 100644 hook.h

diff --git a/Makefile b/Makefile
index b9bf13239d0..1f2b0205da5 100644
--- a/Makefile
+++ b/Makefile
@@ -904,6 +904,7 @@ LIB_OBJS += hash-lookup.o
 LIB_OBJS += hashmap.o
 LIB_OBJS += help.o
 LIB_OBJS += hex.o
+LIB_OBJS += hook.o
 LIB_OBJS += ident.o
 LIB_OBJS += json-writer.o
 LIB_OBJS += kwset.o
diff --git a/builtin/am.c b/builtin/am.c
index e4a0ff9cd7c..3527945a467 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -11,6 +11,7 @@
 #include "parse-options.h"
 #include "dir.h"
 #include "run-command.h"
+#include "hook.h"
 #include "quote.h"
 #include "tempfile.h"
 #include "lockfile.h"
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 06ed10dc92d..c30a360d695 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -3,7 +3,7 @@
 #include "strbuf.h"
 #include "help.h"
 #include "compat/compiler.h"
-#include "run-command.h"
+#include "hook.h"
 
 
 static void get_system_info(struct strbuf *sys_info)
diff --git a/builtin/commit.c b/builtin/commit.c
index e7320f66f95..5359d961d22 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -19,6 +19,7 @@
 #include "revision.h"
 #include "wt-status.h"
 #include "run-command.h"
+#include "hook.h"
 #include "refs.h"
 #include "log-tree.h"
 #include "strbuf.h"
diff --git a/builtin/merge.c b/builtin/merge.c
index 3fbdacc7db4..fe664f6a863 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -13,6 +13,7 @@
 #include "builtin.h"
 #include "lockfile.h"
 #include "run-command.h"
+#include "hook.h"
 #include "diff.h"
 #include "diff-merges.h"
 #include "refs.h"
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 48960a9575b..e3895aec622 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -7,6 +7,7 @@
 #include "pkt-line.h"
 #include "sideband.h"
 #include "run-command.h"
+#include "hook.h"
 #include "exec-cmd.h"
 #include "commit.h"
 #include "object.h"
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 0d0a80da61f..d22ece93e1a 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -8,6 +8,7 @@
 #include "branch.h"
 #include "refs.h"
 #include "run-command.h"
+#include "hook.h"
 #include "sigchain.h"
 #include "submodule.h"
 #include "utf8.h"
diff --git a/hook.c b/hook.c
new file mode 100644
index 00000000000..ba70b314718
--- /dev/null
+++ b/hook.c
@@ -0,0 +1,37 @@
+#include "cache.h"
+#include "hook.h"
+#include "run-command.h"
+
+const char *find_hook(const char *name)
+{
+	static struct strbuf path = STRBUF_INIT;
+
+	strbuf_reset(&path);
+	strbuf_git_path(&path, "hooks/%s", name);
+	if (access(path.buf, X_OK) < 0) {
+		int err = errno;
+
+#ifdef STRIP_EXTENSION
+		strbuf_addstr(&path, STRIP_EXTENSION);
+		if (access(path.buf, X_OK) >= 0)
+			return path.buf;
+		if (errno == EACCES)
+			err = errno;
+#endif
+
+		if (err == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) {
+			static struct string_list advise_given = STRING_LIST_INIT_DUP;
+
+			if (!string_list_lookup(&advise_given, name)) {
+				string_list_insert(&advise_given, name);
+				advise(_("The '%s' hook was ignored because "
+					 "it's not set as executable.\n"
+					 "You can disable this warning with "
+					 "`git config advice.ignoredHook false`."),
+				       path.buf);
+			}
+		}
+		return NULL;
+	}
+	return path.buf;
+}
diff --git a/hook.h b/hook.h
new file mode 100644
index 00000000000..68624f16059
--- /dev/null
+++ b/hook.h
@@ -0,0 +1,11 @@
+#ifndef HOOK_H
+#define HOOK_H
+
+/*
+ * Returns the path to the hook file, or NULL if the hook is missing
+ * or disabled. Note that this points to static storage that will be
+ * overwritten by further calls to find_hook and run_hook_*.
+ */
+const char *find_hook(const char *name);
+
+#endif
diff --git a/refs.c b/refs.c
index 8b9f7c3a80a..6211692eaae 100644
--- a/refs.c
+++ b/refs.c
@@ -10,6 +10,7 @@
 #include "refs.h"
 #include "refs/refs-internal.h"
 #include "run-command.h"
+#include "hook.h"
 #include "object-store.h"
 #include "object.h"
 #include "tag.h"
diff --git a/run-command.c b/run-command.c
index 04a07e6366e..e4862b8f46d 100644
--- a/run-command.c
+++ b/run-command.c
@@ -9,6 +9,7 @@
 #include "quote.h"
 #include "config.h"
 #include "packfile.h"
+#include "hook.h"
 
 void child_process_init(struct child_process *child)
 {
@@ -1322,40 +1323,6 @@ int async_with_fork(void)
 #endif
 }
 
-const char *find_hook(const char *name)
-{
-	static struct strbuf path = STRBUF_INIT;
-
-	strbuf_reset(&path);
-	strbuf_git_path(&path, "hooks/%s", name);
-	if (access(path.buf, X_OK) < 0) {
-		int err = errno;
-
-#ifdef STRIP_EXTENSION
-		strbuf_addstr(&path, STRIP_EXTENSION);
-		if (access(path.buf, X_OK) >= 0)
-			return path.buf;
-		if (errno == EACCES)
-			err = errno;
-#endif
-
-		if (err == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) {
-			static struct string_list advise_given = STRING_LIST_INIT_DUP;
-
-			if (!string_list_lookup(&advise_given, name)) {
-				string_list_insert(&advise_given, name);
-				advise(_("The '%s' hook was ignored because "
-					 "it's not set as executable.\n"
-					 "You can disable this warning with "
-					 "`git config advice.ignoredHook false`."),
-				       path.buf);
-			}
-		}
-		return NULL;
-	}
-	return path.buf;
-}
-
 int run_hook_ve(const char *const *env, const char *name, va_list args)
 {
 	struct child_process hook = CHILD_PROCESS_INIT;
diff --git a/run-command.h b/run-command.h
index b9aff74914d..5e544acf4bc 100644
--- a/run-command.h
+++ b/run-command.h
@@ -224,13 +224,6 @@ int finish_command_in_signal(struct child_process *);
  */
 int run_command(struct child_process *);
 
-/*
- * Returns the path to the hook file, or NULL if the hook is missing
- * or disabled. Note that this points to static storage that will be
- * overwritten by further calls to find_hook and run_hook_*.
- */
-const char *find_hook(const char *name);
-
 /**
  * Run a hook.
  * The first argument is a pathname to an index file, or NULL
diff --git a/sequencer.c b/sequencer.c
index 614d56f5e21..8ee6c4ac240 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -8,6 +8,7 @@
 #include "sequencer.h"
 #include "tag.h"
 #include "run-command.h"
+#include "hook.h"
 #include "exec-cmd.h"
 #include "utf8.h"
 #include "cache-tree.h"
diff --git a/transport.c b/transport.c
index b37664ba870..e4f1decae20 100644
--- a/transport.c
+++ b/transport.c
@@ -1,7 +1,7 @@
 #include "cache.h"
 #include "config.h"
 #include "transport.h"
-#include "run-command.h"
+#include "hook.h"
 #include "pkt-line.h"
 #include "fetch-pack.h"
 #include "remote.h"
-- 
2.33.0.1291.g8857a6a91ac

[PATCH v2 6/8] hook.c: add a hook_exists() wrapper and use it in bugreport.c

[Ævar Arnfjörð Bjarmason]

From: Emily Shaffer <emilyshaffer@google.com>

Add a boolean version of the find_hook() function for those callers
who are only interested in checking whether the hook exists, not what
the path to it is.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bugreport.c | 2 +-
 hook.c              | 5 +++++
 hook.h              | 5 +++++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index c30a360d695..a02c2540bb1 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -82,7 +82,7 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 	}
 
 	for (i = 0; i < ARRAY_SIZE(hook); i++)
-		if (find_hook(hook[i]))
+		if (hook_exists(hook[i]))
 			strbuf_addf(hook_info, "%s\n", hook[i]);
 }
 
diff --git a/hook.c b/hook.c
index ba70b314718..55e1145a4b7 100644
--- a/hook.c
+++ b/hook.c
@@ -35,3 +35,8 @@ const char *find_hook(const char *name)
 	}
 	return path.buf;
 }
+
+int hook_exists(const char *name)
+{
+	return !!find_hook(name);
+}
diff --git a/hook.h b/hook.h
index 68624f16059..6aa36fc7ff9 100644
--- a/hook.h
+++ b/hook.h
@@ -8,4 +8,9 @@
  */
 const char *find_hook(const char *name);
 
+/**
+ * A boolean version of find_hook()
+ */
+int hook_exists(const char *hookname);
+
 #endif
-- 
2.33.0.1291.g8857a6a91ac

[PATCH v2 7/8] hook.c users: use "hook_exists()" instead of "find_hook()"

[Ævar Arnfjörð Bjarmason]

Use the new hook_exists() function instead of find_hook() where the
latter was called in boolean contexts. This make subsequent changes in
a series where we further refactor the hook API clearer, as we won't
conflate wanting to get the path of the hook with checking for its
existence.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit.c       | 2 +-
 builtin/merge.c        | 2 +-
 builtin/receive-pack.c | 2 +-
 sequencer.c            | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 5359d961d22..883c16256c8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1052,7 +1052,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (!no_verify && find_hook("pre-commit")) {
+	if (!no_verify && hook_exists("pre-commit")) {
 		/*
 		 * Re-read the index as pre-commit hook could have updated it,
 		 * and write it out as a tree.  We must do this before we invoke
diff --git a/builtin/merge.c b/builtin/merge.c
index fe664f6a863..956b6259f21 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -849,7 +849,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	 * and write it out as a tree.  We must do this before we invoke
 	 * the editor and after we invoke run_status above.
 	 */
-	if (find_hook("pre-merge-commit"))
+	if (hook_exists("pre-merge-commit"))
 		discard_cache();
 	read_cache_from(index_file);
 	strbuf_addbuf(&msg, &merge_msg);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e3895aec622..25cc0c907e1 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1464,7 +1464,7 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
 
 	strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
 
-	if (!find_hook(push_to_checkout_hook))
+	if (!hook_exists(push_to_checkout_hook))
 		retval = push_to_deploy(sha1, &env, work_tree);
 	else
 		retval = push_to_checkout(sha1, &env, work_tree);
diff --git a/sequencer.c b/sequencer.c
index 8ee6c4ac240..e501945796d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1460,7 +1460,7 @@ static int try_to_commit(struct repository *r,
 		}
 	}
 
-	if (find_hook("prepare-commit-msg")) {
+	if (hook_exists("prepare-commit-msg")) {
 		res = run_prepare_commit_msg_hook(r, msg, hook_commit);
 		if (res)
 			goto out;
-- 
2.33.0.1291.g8857a6a91ac

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

[Ævar Arnfjörð Bjarmason]

Make githooks(5) the source of truth for what hooks git supports, and
punt out early on 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.

We could make the find_hook() function die() or BUG() out if the new
known_hook() returned 0, but let's make it return NULL just as it does
when it can't find a hook of a known type. Making it die() is overly
anal, and unlikely to be what we need in catching stupid typos in the
name of some new hook hardcoded in git.git's sources. By making this
be tolerant of unknown hook names, changes in a later series to make
"git hook run" run arbitrary user-configured hook names will be easier
to implement.

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>
Helped-by: René Scharfe <l.s.r@web.de>
---
 .gitignore                          |  1 +
 Makefile                            |  8 +++++-
 builtin/bugreport.c                 | 44 ++++++-----------------------
 contrib/buildsystems/CMakeLists.txt |  7 +++++
 generate-hooklist.sh                | 20 +++++++++++++
 5 files changed, 43 insertions(+), 37 deletions(-)
 create mode 100755 generate-hooklist.sh

diff --git a/.gitignore b/.gitignore
index 311841f9bed..6be9de41ae8 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 1f2b0205da5..21e2bcc21e3 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,8 +2210,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 		$(filter %.o,$^) $(LIBS)
 
 help.sp help.s help.o: command-list.h
+hook.sp hook.s hook.o: hook-list.h
 
-builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
+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 +2244,9 @@ command-list.h: $(wildcard Documentation/git*.txt)
 		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
 		command-list.txt >$@
 
+hook-list.h: generate-hooklist.sh Documentation/githooks.txt
+	$(QUIET_GEN)$(SHELL_PATH) ./generate-hooklist.sh >$@
+
 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 a02c2540bb1..9de32bc96e7 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 171b4124afe..fd1399c440f 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -624,6 +624,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 00000000000..2f9f54eb545
--- /dev/null
+++ b/generate-hooklist.sh
@@ -0,0 +1,20 @@
+#!/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;}' \
+	-e 'x' \
+	<Documentation/githooks.txt |
+	LC_ALL=C sort
+
+cat <<EOF
+	NULL,
+};
+EOF
-- 
2.33.0.1291.g8857a6a91ac

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> +sed -n \
> +	-e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}' \
> +	-e 'x' \
> +	<Documentation/githooks.txt |
> +	LC_ALL=C sort

Thanks.  It is not wrong per-se, but if you are willing to do a
multi-line script for readability, wouldn't a much more readable
"single long and multi-line string" approach, i.e.

    sed -ne '
	/^~~~~*$/ {
		x
		s/^.*$/ "&",/
		p
	}
	x
    ' Documentation/githooks.txt |
    LC_ALL=C sort

work better?

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

[René Scharfe]

Am 27.09.21 um 18:48 schrieb Junio C Hamano:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> +sed -n \
>> +	-e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}' \
>> +	-e 'x' \
>> +	<Documentation/githooks.txt |
>> +	LC_ALL=C sort
>
> Thanks.  It is not wrong per-se, but if you are willing to do a
> multi-line script for readability, wouldn't a much more readable
> "single long and multi-line string" approach, i.e.
>
>     sed -ne '
> 	/^~~~~*$/ {
> 		x
> 		s/^.*$/ "&",/
> 		p
> 	}
> 	x
>     ' Documentation/githooks.txt |
>     LC_ALL=C sort
>
> work better?
>

It is more readable, but according to OpenBSD's sed(1) manpage it would
also be non-portable (https://man.openbsd.org/sed#STANDARDS).  That note
was added in 2006 (https://github.com/openbsd/src/commit/24ce9718),
though, so perhaps it needs an update.

FWIW, generate-cmdlist.sh uses such a multi-line sed script in its
function get_synopsis.

René

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 27.09.21 um 18:48 schrieb Junio C Hamano:
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> +sed -n \
>>> +	-e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}' \
>>> +	-e 'x' \
>>> +	<Documentation/githooks.txt |
>>> +	LC_ALL=C sort
>>
>> Thanks.  It is not wrong per-se, but if you are willing to do a
>> multi-line script for readability, wouldn't a much more readable
>> "single long and multi-line string" approach, i.e.
>>
>>     sed -ne '
>> 	/^~~~~*$/ {
>> 		x
>> 		s/^.*$/ "&",/
>> 		p
>> 	}
>> 	x
>>     ' Documentation/githooks.txt |
>>     LC_ALL=C sort
>>
>> work better?
>>
>
> It is more readable, but according to OpenBSD's sed(1) manpage it would
> also be non-portable (https://man.openbsd.org/sed#STANDARDS).  That note
> was added in 2006 (https://github.com/openbsd/src/commit/24ce9718),
> though, so perhaps it needs an update.
>
> FWIW, generate-cmdlist.sh uses such a multi-line sed script in its
> function get_synopsis.

I wonder who wrote the invocation in generate-cmdlist.sh ;-)

FWIW, the informative part of POSIX.1 sed manpage

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html#tag_20_116_17

has such a single long and multi-line string script given on the
command line, but "non-portable" can refer to "some implementations
may not implement all of what POSIX expects", so ... ;-)

Re: [PATCH v2 0/8] Makefile: generate a hook-list.h, prep for config-based-hooks

[Phillip Wood]

On 26/09/2021 20:03, Ævar Arnfjörð Bjarmason wrote:
> This series is an incremental restart of the now-ejected
> es/config-based-hooks and ab/config-based-hooks-base topics. See [1]
> for a summary of the plan and progression.
> 
> In v2 the "sed" invocation that generates the new hook-list.h has been
> changed to be portable under POSIX. See the thread starting at
> https://lore.kernel.org/git/92471ff9-7573-c3e4-e9fd-63a5cbf5738f@gmail.com/;
> 
> The portability issue is AFAICT theoretical in that any "sed" command
> I've tried accepts the old version (I tried the large list of OS's
> listed in [2]), but better safe than sorry.
> 
> Other changes:
> 
>   * I noticed that the run-command.h inclusion in transport.c become
>     redundant, I removed that and validated the other ones that have
>     the new hook.h, they all still need run-command.h.
> 
>   * A whitespace change in v1 in a change to the Makefile makes the
>     diff for 8/8 easier to read.
> 
> 1. http://lore.kernel.org/git/cover-0.8-00000000000-20210923T095326Z-avarab@gmail.com
> 2. https://lore.kernel.org/git/87fstt3gzd.fsf@evledraar.gmail.com/
 > [...]
> 8:  80aae4d5c13 ! 8:  7420267ce09 hook-list.h: add a generated list of hooks, like config-list.h
>      @@ Makefile: XDIFF_LIB = xdiff/lib.a
>        generated-hdrs: $(GENERATED_H)
>        
>       @@ Makefile: git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
>      + 		$(filter %.o,$^) $(LIBS)
>        
>        help.sp help.s help.o: command-list.h
>      ++hook.sp hook.s hook.o: hook-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

This is billed as a whitespace change above but this line has actually 
changed since the last version - was that intentional?

>        builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
>        	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
>      @@ generate-hooklist.sh (new)
>       +static const char *hook_name_list[] = {
>       +EOF
>       +
>      -+sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
>      ++sed -n \
>      ++	-e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}' \
>      ++	-e 'x' \
>       +	<Documentation/githooks.txt |
>       +	LC_ALL=C sort
>       +

The sed change looks good

Thanks

Phillip

Re: [PATCH v2 0/8] Makefile: generate a hook-list.h, prep for config-based-hooks

[Ævar Arnfjörð Bjarmason]

On Mon, Sep 27 2021, Phillip Wood wrote:

> On 26/09/2021 20:03, Ævar Arnfjörð Bjarmason wrote:
>> This series is an incremental restart of the now-ejected
>> es/config-based-hooks and ab/config-based-hooks-base topics. See [1]
>> for a summary of the plan and progression.
>> In v2 the "sed" invocation that generates the new hook-list.h has
>> been
>> changed to be portable under POSIX. See the thread starting at
>> https://lore.kernel.org/git/92471ff9-7573-c3e4-e9fd-63a5cbf5738f@gmail.com/;
>> The portability issue is AFAICT theoretical in that any "sed"
>> command
>> I've tried accepts the old version (I tried the large list of OS's
>> listed in [2]), but better safe than sorry.
>> Other changes:
>>   * I noticed that the run-command.h inclusion in transport.c become
>>     redundant, I removed that and validated the other ones that have
>>     the new hook.h, they all still need run-command.h.
>>   * A whitespace change in v1 in a change to the Makefile makes the
>>     diff for 8/8 easier to read.
>> 1. http://lore.kernel.org/git/cover-0.8-00000000000-20210923T095326Z-avarab@gmail.com
>> 2. https://lore.kernel.org/git/87fstt3gzd.fsf@evledraar.gmail.com/
>> [...]
>> 8:  80aae4d5c13 ! 8:  7420267ce09 hook-list.h: add a generated list of hooks, like config-list.h
>>      @@ Makefile: XDIFF_LIB = xdiff/lib.a
>>        generated-hdrs: $(GENERATED_H)
>>              @@ Makefile: git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS)
>> $(GITLIBS)
>>      + 		$(filter %.o,$^) $(LIBS)
>>               help.sp help.s help.o: command-list.h
>>      ++hook.sp hook.s hook.o: hook-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
>
> This is billed as a whitespace change above but this line has actually
> changed since the last version - was that intentional?

I think you're mistaken here, it is a whitespace-only change to the
end-state, but the diff and range-diff are confusing. If I diff the two
Makefiles I end up with when applying v1 and v2 I get:

@@ -2217,7 +2210,6 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 		$(filter %.o,$^) $(LIBS)
 
 help.sp help.s help.o: command-list.h
-
 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

I.e. only the line between the old command-list.h and new hook-list.h
line is gone.

But the diff for v1 is D/A/A/A and for v2 A/D/A (D = Deletion, A =
Addition).

I.e. it's one of those times when "git diff" produces a valid diff, and
one that's actually smaller than couldu have been produced with a
v2-like diff given the change in v1.

As an aside I've sometimes wished we had a --diff-algorithm=maximal or
something, i.e. there's a lot of cases where by just adding one line to
the diff you can produce a bigger but IMO less confusing one.

In any case, the end state looks better & the diff for v2 is more
intuitive to look at.

>>        builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
>>        	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
>>      @@ generate-hooklist.sh (new)
>>       +static const char *hook_name_list[] = {
>>       +EOF
>>       +
>>      -+sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
>>      ++sed -n \
>>      ++	-e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}' \
>>      ++	-e 'x' \
>>       +	<Documentation/githooks.txt |
>>       +	LC_ALL=C sort
>>       +
>
> The sed change looks good

Thanks for confirming.

Re: [PATCH v2 0/8] Makefile: generate a hook-list.h, prep for config-based-hooks

[Phillip Wood]

On 27/09/2021 11:38, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Sep 27 2021, Phillip Wood wrote:
> 
>> On 26/09/2021 20:03, Ævar Arnfjörð Bjarmason wrote:
>>> This series is an incremental restart of the now-ejected
>>> es/config-based-hooks and ab/config-based-hooks-base topics. See [1]
>>> for a summary of the plan and progression.
>>> In v2 the "sed" invocation that generates the new hook-list.h has
>>> been
>>> changed to be portable under POSIX. See the thread starting at
>>> https://lore.kernel.org/git/92471ff9-7573-c3e4-e9fd-63a5cbf5738f@gmail.com/;
>>> The portability issue is AFAICT theoretical in that any "sed"
>>> command
>>> I've tried accepts the old version (I tried the large list of OS's
>>> listed in [2]), but better safe than sorry.
>>> Other changes:
>>>    * I noticed that the run-command.h inclusion in transport.c become
>>>      redundant, I removed that and validated the other ones that have
>>>      the new hook.h, they all still need run-command.h.
>>>    * A whitespace change in v1 in a change to the Makefile makes the
>>>      diff for 8/8 easier to read.
>>> 1. http://lore.kernel.org/git/cover-0.8-00000000000-20210923T095326Z-avarab@gmail.com
>>> 2. https://lore.kernel.org/git/87fstt3gzd.fsf@evledraar.gmail.com/
>>> [...]
>>> 8:  80aae4d5c13 ! 8:  7420267ce09 hook-list.h: add a generated list of hooks, like config-list.h
>>>       @@ Makefile: XDIFF_LIB = xdiff/lib.a
>>>         generated-hdrs: $(GENERATED_H)
>>>               @@ Makefile: git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS)
>>> $(GITLIBS)
>>>       + 		$(filter %.o,$^) $(LIBS)
>>>                help.sp help.s help.o: command-list.h
>>>       ++hook.sp hook.s hook.o: hook-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
>>
>> This is billed as a whitespace change above but this line has actually
>> changed since the last version - was that intentional?
> 
> I think you're mistaken here,

Yes you're right, looking more closely it is a context line in the range 
diff that has changed, sorry.

> it is a whitespace-only change to the
> end-state, but the diff and range-diff are confusing. If I diff the two
> Makefiles I end up with when applying v1 and v2 I get:
> 
> @@ -2217,7 +2210,6 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
>   		$(filter %.o,$^) $(LIBS)
>   
>   help.sp help.s help.o: command-list.h
> -
>   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
> 
> I.e. only the line between the old command-list.h and new hook-list.h
> line is gone.
> 
> But the diff for v1 is D/A/A/A and for v2 A/D/A (D = Deletion, A =
> Addition).
> 
> I.e. it's one of those times when "git diff" produces a valid diff, and
> one that's actually smaller than couldu have been produced with a
> v2-like diff given the change in v1.
> 
> As an aside I've sometimes wished we had a --diff-algorithm=maximal or
> something, i.e. there's a lot of cases where by just adding one line to
> the diff you can produce a bigger but IMO less confusing one.

Yes especially when it splits the diff just to keep an empty line or 
closing brace as context even though the line itself is not really 
interesting. Python's diff-lib has an option to ignore "junk" when 
determining which lines are unchanged, I keep meaning to see how it 
affects the diff but have never got round to it (the library uses a 
different slower algorithm for calculating the diff as well).

Best Wishes

Phillip

> 
> In any case, the end state looks better & the diff for v2 is more
> intuitive to look at.
> 
>>>         builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
>>>         	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
>>>       @@ generate-hooklist.sh (new)
>>>        +static const char *hook_name_list[] = {
>>>        +EOF
>>>        +
>>>       -+sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
>>>       ++sed -n \
>>>       ++	-e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}' \
>>>       ++	-e 'x' \
>>>        +	<Documentation/githooks.txt |
>>>        +	LC_ALL=C sort
>>>        +
>>
>> The sed change looks good
> 
> Thanks for confirming.
>