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

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

> 
> > @@ -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?