Re: [PATCH v8 04/37] hook: include hookdir hook in list

> 
> 
> On Thu, Mar 11 2021, Emily Shaffer wrote:
> 
> > Historically, hooks are declared by placing an executable into
> > $GIT_DIR/hooks/$HOOKNAME (or $HOOKDIR/$HOOKNAME). Although hooks taken
> > from the config are more featureful than hooks placed in the $HOOKDIR,
> > those hooks should not stop working for users who already have them.
> > Let's list them to the user, but instead of displaying a config scope
> > (e.g. "global: blah") we can prefix them with "hookdir:".
> >
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> >
> > Notes:
> >     Since v7, fix some nits from Jonathan Tan. The largest is to move reference to
> >     "hookdir annotation" from this commit to the next one which introduces the
> >     hook.runHookDir option.
> >
> >  builtin/hook.c                | 11 +++++++++--
> >  hook.c                        | 17 +++++++++++++++++
> >  hook.h                        |  1 +
> >  t/t1360-config-based-hooks.sh | 19 +++++++++++++++++++
> >  4 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/hook.c b/builtin/hook.c
> > index bb64cd77ca..c8fbfbb39d 100644
> > --- a/builtin/hook.c
> > +++ b/builtin/hook.c
> > @@ -40,10 +40,15 @@ static int list(int argc, const char **argv, const char *prefix)
> >  
> >  	list_for_each(pos, head) {
> >  		struct hook *item = list_entry(pos, struct hook, list);
> > -		if (item)
> > +		item = list_entry(pos, struct hook, list);
> > +		if (item) {
> > +			/* Don't translate 'hookdir' - it matches the config */
> 
> Let's prefix comments for translators with /* TRANSLATORS: .., see the
> coding style doc. That's what they'll see, and this is useful to them.
> 
> Better yet have a note here about the first argument being 'system',
> 'local' etc., which I had to source spelunge for, and translators won't
> have any idea about unless the magic parameter is documented.

> 
> > +setup_hookdir () {
> > +	mkdir .git/hooks
> > +	write_script .git/hooks/pre-commit <<-EOF
> > +	echo \"Legacy Hook\"
> 
> Nit, "'s not needed, but it also seems nothing uses this, so if it's
> just a pass-through script either "exit 0", or actually check if it's
> run or something?

> 
> > [...]
> > +test_expect_success 'git hook list shows hooks from the hookdir' '
> > +	setup_hookdir &&
> > +
> > +	cat >expected <<-EOF &&
> > +	hookdir: $(pwd)/.git/hooks/pre-commit
> > +	EOF
> > +
> > +	git hook list pre-commit >actual &&
> > +	test_cmp expected actual
> > +'
> 
> Ah, so it's just checking if it exists...