On Sat, Jan 30, 2021 at 07:20:22PM -0800, Jonathan Tan 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.
>
> Maybe explicitly add that we're listing them in the list with a "hookdir:"
> prefix.
Sure.
>
> > Legacy hooks should be run directly, not in shell. We know that they are
> > a path to an executable, not a oneliner script - and running them
> > directly takes care of path quoting concerns for us for free.
>
> Not sure what this paragraph is doing here.
Yep, this is an artifact of the review process (explaining why I didn't
do something weird, which I did in an earlier version, but now it
doesn't make sense to mention it at all). Deleted.
>
> > diff --git a/builtin/hook.c b/builtin/hook.c
> > index 4d36de52f8..a0013ae4d7 100644
> > --- a/builtin/hook.c
> > +++ b/builtin/hook.c
> > @@ -16,6 +16,7 @@ static int list(int argc, const char **argv, const char *prefix)
> > struct list_head *head, *pos;
> > struct hook *item;
> > struct strbuf hookname = STRBUF_INIT;
> > + struct strbuf hookdir_annotation = STRBUF_INIT;
>
> Right now this is never set? Maybe hold off on adding this until we set
> something.
Yeah, that makes sense. Will do.
>
> > @@ -110,6 +113,18 @@ struct list_head* hook_list(const struct strbuf* hookname)
> >
> > git_config(hook_config_lookup, (void*)&cb_data);
> >
> > + if (have_git_dir())
> > + legacy_hook_path = find_hook(hookname->buf);
> > +
> > + /* Unconditionally add legacy hook, but annotate it. */
> > + if (legacy_hook_path) {
> > + struct hook *legacy_hook;
> > +
> > + append_or_move_hook(hook_head, absolute_path(legacy_hook_path));
>
> Both find_hook() and absolute_path() use static buffers to hold their
> return values, which makes me a bit nervous. Perhaps put them all under
> the same "if (have_git_dir())" so that it's clearer that we're not
> supposed to insert code arbitrarily between their invocation and their
> usage.
Oh, that's a cool way to indicate that. Thanks, I did that, and learned
something new!
>
> > diff --git a/hook.h b/hook.h
> > index 8ffc4f14b6..5750634c83 100644
> > --- a/hook.h
> > +++ b/hook.h
> > @@ -12,6 +12,7 @@ struct hook
> > enum config_scope origin;
> > /* The literal command to run. */
> > struct strbuf command;
> > + int from_hookdir;
>
> unsigned from_hookdir : 1?
Sure. It doesn't make a difference now but I see that would be nice for
futureproofing.
>
> The tests look good.
Thanks.
