Re: [PATCH v7 04/17] hook: include hookdir hook in list

From: Emily Shaffer <emilyshaffer@google.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v7 04/17] hook: include hookdir hook in list
Date: Tue, 9 Feb 2021 14:05:45 -0800	[thread overview]
Message-ID: <YCMHOeLqESHokPZL@google.com> (raw)
In-Reply-To: <20210131032022.1018820-1-jonathantanmy@google.com>

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.