Re: [PATCH v9 07/37] hook: add 'run' subcommand
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jun 3, 2021, 09:07 UTC
- Message-ID
- <875yyv71lz.fsf@evledraar.gmail.com>
- In-Reply-To
- <20210527000856.695702-8-emilyshaffer@google.com>
On Wed, May 26 2021, Emily Shaffer wrote:
Show 6 quoted lines
> +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@evledraar.gmail.com/ 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@google.com/
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,