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

[Emily Shaffer <emilyshaffer@google.com>]

On Mon, Oct 05, 2020 at 04:24:18PM -0700, Jonathan Nieder wrote:
> 
> Hi,
> 
> Emily Shaffer wrote:
> 
> > 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 <emilyshaffer@google.com>
> > ---
> >  .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.

> 
> [...]
> > --- /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.

> 
> [...]
> > --- 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.