On Thu, Jul 15 2021, Emily Shaffer wrote:
> +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.
> +{
> + 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.
Also is there some subtlety in the list macro here or can we just
"s/break/return it/" and skip the break/return pattern?
> +static struct hook * append_or_move_hook(struct list_head *head, const char *command)
Same whitespace nits.
> + 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?
> + if (!strcmp(key, hook_key)) {
> + const char *command = value;
> + struct strbuf hookcmd_name = STRBUF_INIT;
> +
> +
Nit: 3x\n, not 2x\n
> + 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...
> + 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 :)
> + }
> +
> + /* 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.
> -
> + struct strbuf hook_key = STRBUF_INIT;
> + struct hook_config_cb cb_data = { &hook_key, hook_head };
Let's use designated initializers.
>
> 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
> + /* 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...).
