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

On Thu, Jul 22 2021, Emily Shaffer wrote:

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

quoted

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

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*

quoted

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.

Sure makes sense. I'd tend to go for two returns, but let's not split
hairs on personal style.

quoted

quoted

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

Same whitespace nits.

ACK

quoted

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).

Looking again I think I just misread this then, thanks.

quoted

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.

Ah, I missed that. Makes sense.