Re: [PATCH 6/9] hook: include hooks from the config

On Fri, Jul 16, 2021 at 11:01:24AM +0200, Ævar Arnfjörð Bjarmason wrote:

On Thu, Jul 15 2021, Emily Shaffer wrote:

quoted

+static struct hook * find_hook_by_command(struct list_head *head, const char *command)

nit: "*find[...]" not "* find[...]", also let's wrap the long line.

ACK

quoted

+{
+	struct list_head *pos = NULL, *tmp = NULL;
+	struct hook *found = NULL;
+
+	list_for_each_safe(pos, tmp, head) {
+		struct hook *it = list_entry(pos, struct hook, list);
+		if (!strcmp(it->command, command)) {
+		    list_del(pos);
+		    found = it;
+		    break;

Indented with spaces.

I don't know how I even did this. *facepalm*

Also is there some subtlety in the list macro here or can we just
"s/break/return it/" and skip the break/return pattern?

I guess it's probably fine, but we'd need the final return anyway
("otherwise returns NULL"). IMO one return is more readable than two
returns, so I'd rather leave this.

quoted

+static struct hook * append_or_move_hook(struct list_head *head, const char *command)

Same whitespace nits.

ACK

quoted

+	if (!to_add) {
+		/* adding a new hook, not moving an old one */
+		to_add = xmalloc(sizeof(*to_add));
+		to_add->command = command;
+		to_add->feed_pipe_cb_data = NULL;
+		/* This gets overwritten in hook_list() for hookdir hooks. */
+		to_add->from_hookdir = 0;

I commented on init verbosity elsewhere, i.e. we could do some things
via macros, but in this case just having an "init" helper make sense,
but we have at least two places copying the same init of all fields,
should just be hook_init_hook() or whatever it'll be called. Maybe with
a second "from hookdir" param?

Hm, where is the second place where we init everything? I think with
this commit we remove anywhere we're putting together a 'struct hook' manually
except during this helper? Hooks from hookdir are initted by
'append_or_move_hook()'ing them to the end of the list and modifying the
from_hookdir field, and builtin/hook.c just calls hook_list() (and some
list.h helpers to find an entry).

quoted

+	if (!strcmp(key, hook_key)) {
+		const char *command = value;
+		struct strbuf hookcmd_name = STRBUF_INIT;
+
+

Nit: 3x\n, not 2x\n

quoted

+		if (!command) {
+			strbuf_release(&hookcmd_name);

You don't need to strbuf_release() things that you haven't done anything
except init'd, but also...

quoted

+			BUG("git_config_get_value overwrote a string it shouldn't have");

...even if that were the case and it called malloc() memory diligence
when we're calling BUG() is probably going overboard, and I say that as
someone who'll tend to go overboard with it :)

:) ok.

quoted

+		}
+
+		/* TODO: implement skipping hooks */
+
+		/* TODO: immplement hook aliases */
+
+		/*
+		 * TODO: implement an option-getting callback, e.g.
+		 *   get configs by pattern hookcmd.$value.*
+		 *   for each key+value, do_callback(key, value, cb_data)
+		 */

I think we should drop the TODO and just let the commit message /
comments speak to what we actually implement, and subsequent patches can
add more features.

Ok, sure.

quoted

-
+	struct strbuf hook_key = STRBUF_INIT;
+	struct hook_config_cb cb_data = { &hook_key, hook_head };

Let's use designated initializers.

ACK

quoted

 
 	INIT_LIST_HEAD(hook_head);
 
 	if (!hookname)
 		return NULL;
 
+	/* Add the hooks from the config, e.g. hook.pre-commit.command */
+	strbuf_addf(&hook_key, "hook.%s.command", hookname);
+	git_config(hook_config_lookup, &cb_data);
+
+

Another 3x\n

ACK

quoted

+	/* to enable oneliners, let config-specified hooks run in shell */
+	cp->use_shell = !run_me->from_hookdir;

I've lost track at this point, but doesn't that mean we're going to use
a shell when we run our own do-not-need-a-shell hooks ourselves?

Isn't isatty() more appropriate here, or actually even interactively why
is the shell needed (maybe this is answered elswhere...).

use_shell means "conditionally guess whether I need to wrap this thing
in `sh -c`" - it doesn't have anything to do with TTY or not. So we need
this for something like `hook.post-commit.command = echo "made a
commit"`. In this case the entire argv[0] will be the oneliner, which
you will need use_shell set for. If we *do* just do something simple,
like `hook.post-commit.command = /bin/mail`, even though
use_shell is marked, the child_process runner will notice that there's
no reason to wrap in 'sh -c' and so will just run the /bin/mail
executable directly.

 - Emily