Subject: Re: [PATCH v3 1/6] hook: run a list of hooks instead

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Fri, 27 Aug 2021 13:15:31 +0200

On Thu, Aug 26 2021, Emily Shaffer wrote:

> On Tue, Aug 24, 2021 at 04:56:10PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> 
>> 
>> On Wed, Aug 18 2021, Emily Shaffer wrote:
>> 
>> > @@ -25,7 +25,8 @@ static int run(int argc, const char **argv, const char *prefix)
>> >  	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
>> >  	int ignore_missing = 0;
>> >  	const char *hook_name;
>> > -	const char *hook_path;
>> > +	struct list_head *hooks;
>> > +
>> >  	struct option run_options[] = {
>> >  		OPT_BOOL(0, "ignore-missing", &ignore_missing,
>> >  			 N_("exit quietly with a zero exit code if the requested hook cannot be found")),
>> 
>> In general in this patch series there's a bunch of little whitespace
>> changes like that along with other changes. I think it's probably best
>> if I just absorb that in the "base" topic instead of doing them
>> here. E.g. in this case we could also add a line between "struct option"
>> and the rest.
>> 
>> I don't mind either way, but the whitespace churn makes for distracting
>> reading...
>
> Ah, hm. I don't know if in this specific case it's necessary for me to
> even have this whitespace change, since 'run_options' is still a struct
> declaration. I'll just drop this one, but in general whichever
> whitespace bits you like from this topic, feel free to absorb. Will make
> a note to scan through the diff when I rebase onto your next reroll
> checking for spurious whitespace changes.
>
>> 
>> > @@ -58,15 +59,16 @@ static int run(int argc, const char **argv, const char *prefix)
>> >  	git_config(git_default_config, NULL);
>> >  
>> >  	hook_name = argv[0];
>> > -	if (ignore_missing)
>> > -		return run_hooks_oneshot(hook_name, &opt);
>> > -	hook_path = find_hook(hook_name);
>> > -	if (!hook_path) {
>> > +	hooks = list_hooks(hook_name);
>> > +	if (list_empty(hooks)) {
>> > +		/* ... act like run_hooks_oneshot() under --ignore-missing */
>> > +		if (ignore_missing)
>> > +			return 0;
>> >  		error("cannot find a hook named %s", hook_name);
>> >  		return 1;
>> >  	}
>> >  
>> > -	ret = run_hooks(hook_name, hook_path, &opt);
>> > +	ret = run_hooks(hook_name, hooks, &opt);
>> >  	run_hooks_opt_clear(&opt);
>> >  	return ret;
>> 
>> This memory management is a bit inconsistent. So here we list_hooks(),
>> pass it to run_hooks(), which clears it for us, OK...
>> 
>> > -int run_hooks(const char *hook_name, const char *hook_path,
>> > -	      struct run_hooks_opt *options)
>> > +int run_hooks(const char *hook_name, struct list_head *hooks,
>> > +		    struct run_hooks_opt *options)
>> >  {
>> > -	struct strbuf abs_path = STRBUF_INIT;
>> > -	struct hook my_hook = {
>> > -		.hook_path = hook_path,
>> > -	};
>> >  	struct hook_cb_data cb_data = {
>> >  		.rc = 0,
>> >  		.hook_name = hook_name,
>> > @@ -197,11 +241,9 @@ int run_hooks(const char *hook_name, const char *hook_path,
>> >  	if (!options)
>> >  		BUG("a struct run_hooks_opt must be provided to run_hooks");
>> >  
>> > -	if (options->absolute_path) {
>> > -		strbuf_add_absolute_path(&abs_path, hook_path);
>> > -		my_hook.hook_path = abs_path.buf;
>> > -	}
>> > -	cb_data.run_me = &my_hook;
>> > +
>> > +	cb_data.head = hooks;
>> > +	cb_data.run_me = list_first_entry(hooks, struct hook, list);
>> >  
>> >  	run_processes_parallel_tr2(jobs,
>> >  				   pick_next_hook,
>> > @@ -213,18 +255,15 @@ int run_hooks(const char *hook_name, const char *hook_path,
>> >  				   "hook",
>> >  				   hook_name);
>> >  
>> > -
>> > -	if (options->absolute_path)
>> > -		strbuf_release(&abs_path);
>> > -	free(my_hook.feed_pipe_cb_data);
>> > +	clear_hook_list(hooks);
>> >  
>> >  	return cb_data.rc;
>> >  }
>> 
>> Which we can see here will call clear_hook_list(), so far so good, but then...
>> 
>> >  int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options)
>> >  {
>> > -	const char *hook_path;
>> > -	int ret;
>> > +	struct list_head *hooks;
>> > +	int ret = 0;
>> >  	struct run_hooks_opt hook_opt_scratch = RUN_HOOKS_OPT_INIT;
>> >  
>> >  	if (!options)
>> > @@ -233,14 +272,19 @@ int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options)
>> >  	if (options->path_to_stdin && options->feed_pipe)
>> >  		BUG("choose only one method to populate stdin");
>> >  
>> > -	hook_path = find_hook(hook_name);
>> > -	if (!hook_path) {
>> > -		ret = 0;
>> > +	hooks = list_hooks(hook_name);
>> > +
>> > +	/*
>> > +	 * If you need to act on a missing hook, use run_found_hooks()
>> > +	 * instead
>> > +	 */
>> > +	if (list_empty(hooks))
>> >  		goto cleanup;
>> > -	}
>> >  
>> > -	ret = run_hooks(hook_name, hook_path, options);
>> > +	ret = run_hooks(hook_name, hooks, options);
>> > +
>> >  cleanup:
>> >  	run_hooks_opt_clear(options);
>> > +	clear_hook_list(hooks);
>> 
>> ...the oneshot command also does clear_hook_list(), after calling
>> run_hooks() which cleared it already.  That looks like a mistake,
>> i.e. we should always trust run_hooks() to clear it, no?
>
> Ah, good catch. I will update the comment on run_hooks() and fix
> _oneshot() :)
>
>  - Emily

I found a further memory issue with this, on "seen" running e.g. t0001
when compiled with SANITIZE=leak is broken by this series.

It's because in clear_hook_list() you clear the list of hooks, but
forget to clear the malloc'd container, so a missing free() fixes it. As
in the POC patch at the end of this mail.

But e.g. "git hook list <name>" is still broken, easy enough to fix,
just also needs fixing of the list_hooks_gently() callsites to e.g. this:
    
    diff --git a/builtin/hook.c b/builtin/hook.c
    index 50233366a8..2cd1831075 100644
    --- a/builtin/hook.c
    +++ b/builtin/hook.c
    @@ -48,8 +48,10 @@ static int list(int argc, const char **argv, const char *prefix)
     
     	head = list_hooks_gently(hookname);
     
    -	if (list_empty(head))
    +	if (list_empty(head)) {
    +		clear_hook_list(head);
     		return 1;
    +	}
     
     	list_for_each(pos, head) {
     		struct hook *item = list_entry(pos, struct hook, list);

Although going to that length to make the SANITIZE=leak run clean is
arguably overdoing it...

diff --git a/hook.c b/hook.c
index 23af86b9ea..e6e1e4173a 100644
--- a/hook.c
+++ b/hook.c
@@ -67,6 +67,7 @@ void clear_hook_list(struct list_head *head)
 	struct list_head *pos, *tmp;
 	list_for_each_safe(pos, tmp, head)
 		remove_hook(pos);
+	free(head);
 }
 
 static int known_hook(const char *name)
@@ -142,7 +143,16 @@ const char *find_hook_gently(const char *name)
 
 int hook_exists(const char *name)
 {
-	return !list_empty(list_hooks(name));
+	struct list_head *hooks;
+	int exists;
+
+	hooks = list_hooks(name);
+
+	exists = !list_empty(hooks);
+
+	clear_hook_list(hooks);
+
+	return exists;
 }
 
 struct hook_config_cb