Re: [PATCH v9 07/37] hook: add 'run' subcommand

[Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>]

On Wed, May 26 2021, Emily Shaffer wrote:

> +void run_hooks_opt_init(struct run_hooks_opt *o)
> +{
> +	strvec_init(&o->env);
> +	strvec_init(&o->args);
> +	o->run_hookdir = configured_hookdir_opt();
> +}

I suggested in
https://lore.kernel.org/git/87y2bs7gyc.fsf@xxxxxxxxxxxxxxxxxxx/ that
this could and should be a RUN_HOOKS_OPT_INIT

After some digging I see that was the case in an earlier version of your
series, i.e. before:
https://lore.kernel.org/git/20210131042254.1032233-1-jonathantanmy@xxxxxxxxxx/

You came up with this current pattern because of
configured_hookdir_opt().

But a better option here is to use a RUN_HOOKS_OPT_INIT still and just
defer the initialization of this "run_hookdir" member. I.e. set it to
"we have not asked the config yet" in the initializer. I.e. the diff on
top your series at the end of this E-Mail[1].

That along with doing the same for the "jobs" member means we can move
back to a RUN_HOOKS_OPT_INIT, and also that the final version of this
function in this series, i.e.:
	
	void run_hooks_opt_init_sync(struct run_hooks_opt *o)
	{
		strvec_init(&o->env);
		strvec_init(&o->args);
		o->path_to_stdin = NULL;
		o->run_hookdir = HOOKDIR_UNINITIALIZED;
		o->jobs = 1;
		o->dir = NULL;
		o->feed_pipe = NULL;
		o->feed_pipe_ctx = NULL;
		o->consume_sideband = NULL;
	}

Is now mostly redundant to a designated initializer. I.e. you don't need
to NULL any of these out anymore. Then either don't set "jobs" and have
"0" mean "ask config" or set it to "-1" or whatever for "uninitialized".

1.

diff --git a/hook.c b/hook.c
index ff80e52eddd..daf3ddcc188 100644
--- a/hook.c
+++ b/hook.c
@@ -154,7 +154,7 @@ int configured_hook_jobs(void)
 	return n;
 }
 
-static int should_include_hookdir(const char *path, enum hookdir_opt cfg)
+static int should_include_hookdir(struct run_hooks_opt *options, const char *path)
 {
 	struct strbuf prompt = STRBUF_INIT;
 	/*
@@ -164,7 +164,10 @@ static int should_include_hookdir(const char *path, enum hookdir_opt cfg)
 	if (!path)
 		return 0;
 
-	switch (cfg)
+	if (options->run_hookdir == HOOKDIR_UNINITIALIZED)
+		options->run_hookdir = configured_hookdir_opt();
+
+	switch (options->run_hookdir)
 	{
 	case HOOKDIR_ERROR:
 		fprintf(stderr, _("Skipping legacy hook at '%s'\n"),
@@ -292,7 +295,7 @@ void run_hooks_opt_init_sync(struct run_hooks_opt *o)
 	strvec_init(&o->env);
 	strvec_init(&o->args);
 	o->path_to_stdin = NULL;
-	o->run_hookdir = configured_hookdir_opt();
+	o->run_hookdir = HOOKDIR_UNINITIALIZED;
 	o->jobs = 1;
 	o->dir = NULL;
 	o->feed_pipe = NULL;
@@ -312,6 +315,9 @@ int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir)
 	struct strbuf hook_key = STRBUF_INIT;
 	int could_run_hookdir;
 
+	if (should_run_hookdir != HOOKDIR_USE_CONFIG)
+		BUG("no callers want !HOOKDIR_USE_CONFIG?");
+
 	if (should_run_hookdir == HOOKDIR_USE_CONFIG)
 		should_run_hookdir = configured_hookdir_opt();
 
@@ -459,7 +465,7 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options)
 		struct hook *hook = list_entry(pos, struct hook, list);
 
 		if (hook->from_hookdir &&
-		    !should_include_hookdir(hook->command.buf, options->run_hookdir))
+		    !should_include_hookdir(options, hook->command.buf))
 			    list_del(pos);
 	}
 
diff --git a/hook.h b/hook.h
index f32189380a8..3c4491a74e7 100644
--- a/hook.h
+++ b/hook.h
@@ -30,6 +30,7 @@ struct list_head* hook_list(const char *hookname);
 
 enum hookdir_opt
 {
+	HOOKDIR_UNINITIALIZED,
 	HOOKDIR_USE_CONFIG,
 	HOOKDIR_NO,
 	HOOKDIR_ERROR,