Re: [PATCH v4 2/9] hook: scaffolding for git-hook subcommand

On Mon, Oct 05, 2020 at 04:24:18PM -0700, Jonathan Nieder wrote:

Hi,

Emily Shaffer wrote:

quoted

Introduce infrastructure for a new subcommand, git-hook, which will be
used to ease config-based hook management. This command will handle
parsing configs to compose a list of hooks to run for a given event, as
well as adding or modifying hook configs in an interactive fashion.

Signed-off-by: Emily Shaffer <redacted>
---
 .gitignore                    |  1 +
 Documentation/git-hook.txt    | 19 +++++++++++++++++++
 Makefile                      |  1 +
 builtin.h                     |  1 +
 builtin/hook.c                | 21 +++++++++++++++++++++
 git.c                         |  1 +
 t/t1360-config-based-hooks.sh | 11 +++++++++++
 7 files changed, 55 insertions(+)
 create mode 100644 Documentation/git-hook.txt
 create mode 100644 builtin/hook.c
 create mode 100755 t/t1360-config-based-hooks.sh

optional: I could imagine this being squashed into patch 3 --- that way,
the command has functionality as soon as it exists.  Alternatively:

I would prefer to leave it on its own. Managing changes like
builtin<->standalone or even the one you mentioned below about
RUN_SETUP_GENTLY is somewhat easier to manage when they aren't in the
same patch as the business logic, IMO.

[...]

quoted

--- /dev/null
+++ b/Documentation/git-hook.txt

@@ -0,0 +1,19 @@
+git-hook(1)
+===========
+
+NAME
+----
+git-hook - Manage configured hooks
+
+SYNOPSIS
+--------
+[verse]
+'git hook'
+
+DESCRIPTION
+-----------
+You can list, add, and modify hooks with this command.

This could say something like "This is a placeholder command that will
gain functionality in subsequent patches" to make the current state
clear.

Done.

[...]

quoted

--- a/git.c
+++ b/git.c

@@ -519,6 +519,7 @@ static struct cmd_struct commands[] = {
 	{ "grep", cmd_grep, RUN_SETUP_GENTLY },
 	{ "hash-object", cmd_hash_object },
 	{ "help", cmd_help },
+	{ "hook", cmd_hook, RUN_SETUP },

This makes the command require that it run within a git repository,
but I can imagine wanting to list hooks outside of any.  Should it use
RUN_SETUP_GENTLY instead?

Nice catch. I'll add a test to the list patch to that effect also.