Re: [PATCH v8 19/37] am: convert applypatch hooks to use config

On Fri, Mar 12, 2021 at 02:23:39AM -0800, Junio C Hamano wrote:

Emily Shaffer [off-list ref] writes:

quoted

@@ -1558,8 +1563,10 @@ static void do_commit(const struct am_state *state)
 	struct commit_list *parents = NULL;
 	const char *reflog_msg, *author, *committer = NULL;
 	struct strbuf sb = STRBUF_INIT;
+	struct run_hooks_opt hook_opt;
+	run_hooks_opt_init_async(&hook_opt);
 
-	if (run_hook_le(NULL, "pre-applypatch", NULL))
+	if (run_hooks("pre-applypatch", &hook_opt))
 		exit(1);
 
 	if (write_cache_as_tree(&tree, 0, NULL))

@@ -1611,8 +1618,9 @@ static void do_commit(const struct am_state *state)
 		fclose(fp);
 	}
 
-	run_hook_le(NULL, "post-applypatch", NULL);
+	run_hooks("post-applypatch", &hook_opt);
 
+	run_hooks_opt_clear(&hook_opt);
 	strbuf_release(&sb);
 }

This one does opt_init(), run_hooks(), and another run_hooks() and
then opt_clear().  If run_hooks() is a read-only operation on the
hook_opt, then that would be alright, but it just smells iffy that
it is not done as two separate opt_init(), run_hooks(), opt_clear()
sequences for two separate run_hooks() invocations.  The same worry
about future safety I meantioned elsewhere in the series also
applies.

Interesting observation. I think the only thing that could be mutated in
the run_hooks_opt struct today is the caller-provided callback data
(run_hooks_opt.feed_pipe_ctx) - which presumably is being manipulated by the
caller in a callback they wrote. But I don't think it hurts particularly
to clear/init again between the two invocations, to be safe - so I will
change the code here.

 - Emily