Emily Shaffer <emilyshaffer@xxxxxxxxxx> writes:
> @@ -3070,6 +3071,8 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
> unsigned flags)
> {
> int ret;
> + struct run_hooks_opt hook_opt;
> + run_hooks_opt_init_async(&hook_opt);
>
Nit. blank line between the last of decls and the first stmt (many
identical nits exist everywhere in this series).
> /*
> * TODO trace2: replace "the_repository" with the actual repo instance
> @@ -3088,9 +3091,13 @@ static int do_write_locked_index(s
> else
> ret = close_lock_file_gently(lock);
>
> - run_hook_le(NULL, "post-index-change",
> - istate->updated_workdir ? "1" : "0",
> - istate->updated_skipworktree ? "1" : "0", NULL);
> + strvec_pushl(&hook_opt.args,
> + istate->updated_workdir ? "1" : "0",
> + istate->updated_skipworktree ? "1" : "0",
> + NULL);
> + run_hooks("post-index-change", &hook_opt);
> + run_hooks_opt_clear(&hook_opt);
There is one early return before the precontext of this hunk that
bypasses this opt_clear() call. It is before any member of hook_opt
structure that was opt_init()'ed gets touched, so with the current
code, there is no leak, but it probably is laying a landmine for the
future, where opt_init() may allocate some resource to its member,
with the expectation that all users of the API would call
opt_clear() to release. Or the caller of the API (like this one) may
start mucking with the opt structure before the existing early return,
at which point the current assumption that it is safe to return from
that point without opt_clear() would be broken.
I saw that there are other early returns in the series that are safe
right now but may become unsafe when the API implementation gets
extended that way. If it does not involve too much code churning,
we may want to restructure the code to make these early returns into
"goto"s that jump to a single exit point, so that we can always
match opt_init() with opt_clear(), like the structure of the
existing code allowed cmd_rebase() to use the hooks API cleanly in
[v8 22/37].
Thanks.
