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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 24, 2021, 14:56 UTC
Message-ID: <87zgt6evkf.fsf@evledraar.gmail.com>
In-Reply-To: <20210819033450.3382652-2-emilyshaffer@google.com>

On Wed, Aug 18 2021, Emily Shaffer wrote:

Show 10 quoted lines

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

Show 21 quoted lines

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

Show 39 quoted lines

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

Show 32 quoted lines

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