Subject: Re: [PATCH v8 00/37] config-based hooks

From: Emily Shaffer <[email-masked]>
Date: Wed, 17 Mar 2021 11:41:59 -0700

On Fri, Mar 12, 2021 at 10:49:38AM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> 
> On Thu, Mar 11 2021, Emily Shaffer wrote:
> 
> > Since v7:
> > - Addressed Jonathan Tan's review of part I
> > - Addressed Junio's review of part I and II
> > - Combined parts I and II
> >
> > I think the updates to patch 1 between the rest of the work I've been
> > doing probably have covered Ævar's comments.
> 
> A range-diff between iterations of such a large series would be most
> useful. Do you have a public repo with tags or whatever the different
> versions, for those who'd like an easier way to follow along the
> differing versions than scraping the ML archive?

I am really embarrassed to say that I don't have the
branches/tags/whatever up. I have not succeeded in building that habit
yet. I'll generate one from my local patches today and send it here.

> 
> While reading this I came up with the following fixup patches on top,
> for discussion, maybe not something you want as-is:

I was a little bit confused reading this fixup without seeing the rest of your
review, so I'll revisit this once I get through what else you wrote.

> 	
> 	 Documentation/git-hook.txt |  8 +++++
> 	 builtin/bugreport.c        |  8 +++--
> 	 builtin/commit.c           |  3 +-
> 	 builtin/hook.c             | 79 ++++++++++++++++++++--------------------------
> 	 builtin/merge.c            |  3 +-
> 	 builtin/receive-pack.c     | 11 +++----
> 	 hook.c                     | 21 +++++-------
> 	 hook.h                     |  5 +--
> 	 refs.c                     |  4 ++-
> 	 sequencer.c                |  4 ++-
> 	 10 files changed, 73 insertions(+), 73 deletions(-)
> 	
> 	diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
> 	index 4ad31ac360a..5c9af30b43e 100644
> 	--- a/Documentation/git-hook.txt
> 	+++ b/Documentation/git-hook.txt
> 	@@ -150,10 +150,18 @@ message body and cannot be parallelized.
> 	 
> 	 CONFIGURATION
> 	 -------------
> 	+
> 	+The below documentation is the same as what's found in
> 	+linkgit:git-config[1]:
> 	+
> 	 include::config/hook.txt[]
> 	 
> 	 HOOKS
> 	 -----
> 	+
> 	+The below documentation is the same as what's found in
> 	+linkgit:githooks[5]:
> 	+
> 	 include::native-hooks.txt[]
> 	 
> Noted in another reply, including it here for completeness.
> 
> 	 GIT
> 	diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> 	index 04467cd1d3a..b64e53fd625 100644
> 	--- a/builtin/bugreport.c
> 	+++ b/builtin/bugreport.c
> 	@@ -81,9 +81,13 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
> 	 		return;
> 	 	}
> 	 
> 	-	for (i = 0; i < ARRAY_SIZE(hook); i++)
> 	-		if (hook_exists(hook[i], HOOKDIR_USE_CONFIG))
> 	+	for (i = 0; i < ARRAY_SIZE(hook); i++) {
> 	+		struct strbuf config;
> 	+		strbuf_addf(&config, "hook.%s.config", hook[i]);
> 	+		if (hook_exists(hook[i], config.buf, HOOKDIR_USE_CONFIG))
> 	 			strbuf_addf(hook_info, "%s\n", hook[i]);
> 	+		strbuf_release(&config);
> 	+	}
> 	 }
> 
> Less strbuf, see below.
> 	 
> 	 static const char * const bugreport_usage[] = {
> 	diff --git a/builtin/commit.c b/builtin/commit.c
> 	index 31df571f123..fc9f1f5ee58 100644
> 	--- a/builtin/commit.c
> 	+++ b/builtin/commit.c
> 	@@ -984,7 +984,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> 	 		return 0;
> 	 	}
> 	 
> 	-	if (!no_verify && hook_exists("pre-commit", HOOKDIR_USE_CONFIG)) {
> 	+	if (!no_verify && hook_exists("pre-commit", "hook.pre-commit.command",
> 	+				      HOOKDIR_USE_CONFIG)) {
> 	 		/*
> 	 		 * Re-read the index as pre-commit hook could have updated it,
> 	 		 * and write it out as a tree.  We must do this before we invoke
> 
> 
> ..ditto.
> 
> 	diff --git a/builtin/hook.c b/builtin/hook.c
> 	index b4f4adb1dea..d0b56ee47f8 100644
> 	--- a/builtin/hook.c
> 	+++ b/builtin/hook.c
> 	@@ -18,8 +18,6 @@ static enum hookdir_opt should_run_hookdir;
> 	 static int list(int argc, const char **argv, const char *prefix)
> 	 {
> 	 	struct list_head *head, *pos;
> 	-	struct strbuf hookname = STRBUF_INIT;
> 	-	struct strbuf hookdir_annotation = STRBUF_INIT;
> 	 
> 	 	struct option list_options[] = {
> 	 		OPT_END(),
> 	@@ -33,67 +31,60 @@ static int list(int argc, const char **argv, const char *prefix)
> 	 			      builtin_hook_usage, list_options);
> 	 	}
> 	 
> 	-	strbuf_addstr(&hookname, argv[0]);
> 	-
> 	-	head = hook_list(&hookname);
> 	+	head = hook_list(argv[0]);
> 	 
> 	 	if (list_empty(head)) {
> 	 		printf(_("no commands configured for hook '%s'\n"),
> 	-		       hookname.buf);
> 	-		strbuf_release(&hookname);
> 	+		       argv[0]);
> 	 		return 0;
> 	 	}
> 	 
> 	-	switch (should_run_hookdir) {
> 	-		case HOOKDIR_NO:
> 	-			strbuf_addstr(&hookdir_annotation, _(" (will not run)"));
> 	-			break;
> 	-		case HOOKDIR_ERROR:
> 	-			strbuf_addstr(&hookdir_annotation, _(" (will error and not run)"));
> 	-			break;
> 	-		case HOOKDIR_INTERACTIVE:
> 	-			strbuf_addstr(&hookdir_annotation, _(" (will prompt)"));
> 	-			break;
> 	-		case HOOKDIR_WARN:
> 	-			strbuf_addstr(&hookdir_annotation, _(" (will warn but run)"));
> 	-			break;
> 	-		case HOOKDIR_YES:
> 	-		/*
> 	-		 * The default behavior should agree with
> 	-		 * hook.c:configured_hookdir_opt(). HOOKDIR_UNKNOWN should just
> 	-		 * do the default behavior.
> 	-		 */
> 	-		case HOOKDIR_UNKNOWN:
> 	-		default:
> 	-			break;
> 	-	}
> 	-
> 	 	list_for_each(pos, head) {
> 	 		struct hook *item = list_entry(pos, struct hook, list);
> 	 		item = list_entry(pos, struct hook, list);
> 	 		if (item) {
> 	-			/* Don't translate 'hookdir' - it matches the config */
> 	-			printf("%s: %s%s\n",
> 	-			       (item->from_hookdir
> 	+			const char *scope = item->from_hookdir
> 	 				? "hookdir"
> 	-				: config_scope_name(item->origin)),
> 	-			       item->command.buf,
> 	-			       (item->from_hookdir
> 	-				? hookdir_annotation.buf
> 	-				: ""));
> 	+				: config_scope_name(item->origin);
> 	+			switch (should_run_hookdir) {
> 	+			case HOOKDIR_NO:
> 	+				printf(_("%s: %s (will not run)\n"),
> 	+				       scope, item->command.buf);
> 	+				break;
> 	+			case HOOKDIR_ERROR:
> 	+				printf(_("%s: %s (will error and not run)\n"),
> 	+				       scope, item->command.buf);
> 	+				break;
> 	+			case HOOKDIR_INTERACTIVE:
> 	+				printf(_("%s: %s (will prompt)\n"),
> 	+				       scope, item->command.buf);
> 	+				break;
> 	+			case HOOKDIR_WARN:
> 	+				printf(_("%s: %s (will warn but run)\n"),
> 	+				       scope, item->command.buf);
> 	+				break;
> 	+			case HOOKDIR_YES:
> 	+				/*
> 	+				 * The default behavior should agree with
> 	+				 * hook.c:configured_hookdir_opt(). HOOKDIR_UNKNOWN should just
> 	+				 * do the default behavior.
> 	+				 */
> 	+			case HOOKDIR_UNKNOWN:
> 	+			default:
> 	+				printf(_("%s: %s\n"),
> 	+				       scope, item->command.buf);
> 	+				break;
> 	+			}
> 	 		}
> 	 	}
> 	 
> 	 	clear_hook_list(head);
> 	-	strbuf_release(&hookdir_annotation);
> 	-	strbuf_release(&hookname);
> 	 
> 	 	return 0;
> 	 }
> 
> I think this is better to avoid i18n lego, as noted in another reply
> (but I didn't include the patch).
> 
> More on strbuf below:
> 	 
> 	 static int run(int argc, const char **argv, const char *prefix)
> 	 {
> 	-	struct strbuf hookname = STRBUF_INIT;
> 	 	struct run_hooks_opt opt;
> 	 	int rc = 0;
> 	 
> 	@@ -118,12 +109,10 @@ static int run(int argc, const char **argv, const char *prefix)
> 	 		usage_msg_opt(_("You must specify a hook event to run."),
> 	 			      builtin_hook_usage, run_options);
> 	 
> 	-	strbuf_addstr(&hookname, argv[0]);
> 	 	opt.run_hookdir = should_run_hookdir;
> 	 
> 	-	rc = run_hooks(hookname.buf, &opt);
> 	+	rc = run_hooks(argv[0], &opt);
> 	 
> 	-	strbuf_release(&hookname);
> 	 	run_hooks_opt_clear(&opt);
> 	 
> 	 	return rc;
> 	diff --git a/builtin/merge.c b/builtin/merge.c
> 	index 3a2af257a6b..df4ff72fbc7 100644
> 	--- a/builtin/merge.c
> 	+++ b/builtin/merge.c
> 	@@ -848,7 +848,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
> 	 	 * and write it out as a tree.  We must do this before we invoke
> 	 	 * the editor and after we invoke run_status above.
> 	 	 */
> 	-	if (hook_exists("pre-merge-commit", HOOKDIR_USE_CONFIG))
> 	+	if (hook_exists("pre-merge-commit", "hook.pre-merge-commit.command",
> 	+			HOOKDIR_USE_CONFIG))
> 	 		discard_cache();
> 	 	read_cache_from(index_file);
> 	 	strbuf_addbuf(&msg, &merge_msg);
> 	diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> 	index eaedeeb1e8b..a76069ea592 100644
> 	--- a/builtin/receive-pack.c
> 	+++ b/builtin/receive-pack.c
> 	@@ -1123,12 +1123,10 @@ static int run_proc_receive_hook(struct command *commands,
> 	 	int version = 0;
> 	 	int code;
> 	 
> 	-	struct strbuf hookname = STRBUF_INIT;
> 	 	struct hook *proc_receive = NULL;
> 	 	struct list_head *pos, *hooks;
> 	 
> 	-	strbuf_addstr(&hookname, "proc-receive");
> 	-	hooks = hook_list(&hookname);
> 	+	hooks = hook_list("proc-receive");
> 	 
> 	 	list_for_each(pos, hooks) {
> 	 		if (proc_receive) {
> 	@@ -1460,8 +1458,6 @@ static const char *push_to_deploy(unsigned char *sha1,
> 	 	return NULL;
> 	 }
> 	 
> 	-static const char *push_to_checkout_hook = "push-to-checkout";
> 	-
> 	 static const char *push_to_checkout(unsigned char *hash,
> 	 				    struct strvec *env,
> 	 				    const char *work_tree)
> 	@@ -1472,7 +1468,7 @@ static const char *push_to_checkout(unsigned char *hash,
> 	 	strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree));
> 	 	strvec_pushv(&opt.env, env->v);
> 	 	strvec_push(&opt.args, hash_to_hex(hash));
> 	-	if (run_hooks(push_to_checkout_hook, &opt)) {
> 	+	if (run_hooks("push-to-checkout", &opt)) {
> 	 		run_hooks_opt_clear(&opt);
> 	 		return "push-to-checkout hook declined";
> 	 	} else {
> 	@@ -1502,7 +1498,8 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
> 	 
> 	 	strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
> 	 
> 	-	if (!hook_exists(push_to_checkout_hook, HOOKDIR_USE_CONFIG))
> 	+	if (!hook_exists("push-to-checkout", "hook.push-to-checkout.command",
> 	+			 HOOKDIR_USE_CONFIG))
> 	 		retval = push_to_deploy(sha1, &env, work_tree);
> 	 	else
> 	 		retval = push_to_checkout(sha1, &env, work_tree);
> 	diff --git a/hook.c b/hook.c
> 	index 7f6f3b9a616..49c3861ce00 100644
> 	--- a/hook.c
> 	+++ b/hook.c
> 	@@ -247,7 +247,7 @@ static const char *find_legacy_hook(const char *name)
> 	 }
> 	 
> 	 
> 	-struct list_head* hook_list(const struct strbuf* hookname)
> 	+struct list_head* hook_list(const char *hookname)
> 	 {
> 	 	struct strbuf hook_key = STRBUF_INIT;
> 	 	struct list_head *hook_head = xmalloc(sizeof(struct list_head));
> 	@@ -256,14 +256,14 @@ struct list_head* hook_list(const struct strbuf* hookname)
> 	 	INIT_LIST_HEAD(hook_head);
> 	 
> 	 	if (!hookname)
> 	-		return NULL;
> 	+		BUG("???");;
> 	 
> 	-	strbuf_addf(&hook_key, "hook.%s.command", hookname->buf);
> 	+	strbuf_addf(&hook_key, "hook.%s.command", hookname);
> 	 
> 	 	git_config(hook_config_lookup, &cb_data);
> 	 
> 	 	if (have_git_dir()) {
> 	-		const char *legacy_hook_path = find_legacy_hook(hookname->buf);
> 	+		const char *legacy_hook_path = find_legacy_hook(hookname);
> 	 
> 	 		/* Unconditionally add legacy hook, but annotate it. */
> 	 		if (legacy_hook_path) {
> 	@@ -300,10 +300,10 @@ void run_hooks_opt_init_async(struct run_hooks_opt *o)
> 	 	o->jobs = configured_hook_jobs();
> 	 }
> 	 
> 	-int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir)
> 	+int hook_exists(const char *hookname, const char *hook_config,
> 	+		enum hookdir_opt should_run_hookdir)
> 	 {
> 	 	const char *value = NULL; /* throwaway */
> 	-	struct strbuf hook_key = STRBUF_INIT;
> 	 	int could_run_hookdir;
> 	 
> 	 	if (should_run_hookdir == HOOKDIR_USE_CONFIG)
> 	@@ -314,9 +314,7 @@ int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir)
> 	 				should_run_hookdir == HOOKDIR_YES)
> 	 				&& !!find_legacy_hook(hookname);
> 	 
> 	-	strbuf_addf(&hook_key, "hook.%s.command", hookname);
> 	-
> 	-	return (!git_config_get_value(hook_key.buf, &value)) || could_run_hookdir;
> 	+	return (!git_config_get_value(hook_config, &value)) || could_run_hookdir;
> 	 }
> 	 
> 	 void run_hooks_opt_clear(struct run_hooks_opt *o)
> 	@@ -438,7 +436,6 @@ static int notify_hook_finished(int result,
> 	 
> 	 int run_hooks(const char *hookname, struct run_hooks_opt *options)
> 	 {
> 	-	struct strbuf hookname_str = STRBUF_INIT;
> 	 	struct list_head *to_run, *pos = NULL, *tmp = NULL;
> 	 	struct hook_cb_data cb_data = { 0, NULL, NULL, options };
> 	 
> 	@@ -448,9 +445,7 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options)
> 	 	if (options->path_to_stdin && options->feed_pipe)
> 	 		BUG("choose only one method to populate stdin");
> 	 
> 	-	strbuf_addstr(&hookname_str, hookname);
> 	-
> 	-	to_run = hook_list(&hookname_str);
> 	+	to_run = hook_list(hookname);
> 	 
> 	 	list_for_each_safe(pos, tmp, to_run) {
> 	 		struct hook *hook = list_entry(pos, struct hook, list);
> 	diff --git a/hook.h b/hook.h
> 	index 4ff9999b049..bfbbf36882d 100644
> 	--- a/hook.h
> 	+++ b/hook.h
> 	@@ -26,7 +26,7 @@ struct hook {
> 	  * Provides a linked list of 'struct hook' detailing commands which should run
> 	  * in response to the 'hookname' event, in execution order.
> 	  */
> 	-struct list_head* hook_list(const struct strbuf *hookname);
> 	+struct list_head* hook_list(const char *hookname);
> 	 
> 	 enum hookdir_opt
> 	 {
> 	@@ -123,7 +123,8 @@ void run_hooks_opt_clear(struct run_hooks_opt *o);
> 	  * Like with run_hooks, if you take a --run-hookdir flag, reflect that
> 	  * user-specified behavior here instead.
> 	  */
> 	-int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir);
> 	+int hook_exists(const char *hookname, const char *hook_config,
> 	+		enum hookdir_opt should_run_hookdir);
> 	 
> 	 /*
> 	  * Runs all hooks associated to the 'hookname' event in order. Each hook will be
> 	diff --git a/refs.c b/refs.c
> 	index 334fdd9103c..f01995fe64f 100644
> 	--- a/refs.c
> 	+++ b/refs.c
> 	@@ -1966,7 +1966,9 @@ static int run_transaction_hook(struct ref_transaction *transaction,
> 	 
> 	 	run_hooks_opt_init_async(&opt);
> 	 
> 	-	if (!hook_exists("reference-transaction", HOOKDIR_USE_CONFIG))
> 	+	if (!hook_exists("reference-transaction",
> 	+			 "hook.reference-transaction.command",
> 	+			 HOOKDIR_USE_CONFIG))
> 	 		return ret;
> 	 
> 	 	strvec_push(&opt.args, state);
> 	diff --git a/sequencer.c b/sequencer.c
> 	index 34ff275f0d1..52c067c1688 100644
> 	--- a/sequencer.c
> 	+++ b/sequencer.c
> 	@@ -1436,7 +1436,9 @@ static int try_to_commit(struct repository *r,
> 	 		}
> 	 	}
> 	 
> 	-	if (hook_exists("prepare-commit-msg", HOOKDIR_USE_CONFIG)) {
> 	+	if (hook_exists("prepare-commit-msg",
> 	+			"hook.prepare-commit-msg.command",
> 	+			HOOKDIR_USE_CONFIG)) {
> 	 		res = run_prepare_commit_msg_hook(r, msg, hook_commit);
> 	 		if (res)
> 	 			goto out;
> 
> There was another reply (from JT I believe, but didn't go back and look
> it up) about the over use of strbuf.
> 
> I tend to agree, as much as I love the API it's really not better to
> write C with it if all you need is a const char* that's never modified,
> particularly if you get it from elsewhere.
> 
> So it's really not meant for or good for "everything we need a const
> char*", but to avoid verbose realloc() dances all over the place, and
> for things like getline() loops without a hardcoded buffer size.
> 
> E.g. in the first hunk here we're creating a strbuf just to copy argv[0]
> to it, and then throwing it away, let's just pass down argv[0].
> 
> For hook_exists I think just having the code more grep-able and having
> the config value inline is better, but I admit that's a matter of taste.
> 
> I didn't try to find all such strbuf() occurrences, anyway, in the
> overall scheme of things it's a relatively small nit.
> 
> I'm hoping to do some deeper diving into this series, in particular the
> parallelism, but just sending the shallow-ish comments I have for now.
> 
> Thanks for working on this!