On Fri, Jul 16, 2021 at 11:01:24AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>
> 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.
ACK
>
> > +{
> > + 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.
>
> > +static struct hook * append_or_move_hook(struct list_head *head, const char *command)
>
> Same whitespace nits.
ACK
>
> > + 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).
>
> > + 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 :)
:) ok.
>
> > + }
> > +
> > + /* 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.
>
> > -
> > + struct strbuf hook_key = STRBUF_INIT;
> > + struct hook_config_cb cb_data = { &hook_key, hook_head };
>
> Let's use designated initializers.
ACK
>
> >
> > 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
>
> > + /* 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
