| Message ID | 20210715232603.3415111-4-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | config-based hooks restarted | expand |
On Thu, Jul 15 2021, Emily Shaffer wrote: > static const char * const builtin_hook_usage[] = { > N_("git hook <command> [...]"), > + N_("git hook list <hookname>"), > N_("git hook run [<args>] <hook-name> [-- <hook-args>]"), > NULL Uses <hook-name> already, let's use that too. I can't remember if it's something I changed myself, or if your original version used both and I picked one for consistency, or... Anyway, I can re-roll the base topic or whatever, but let's have the end result use one or the other. > + if (argc < 1) { > + usage_msg_opt(_("You must specify a hook event name to list."), > + builtin_hook_usage, list_options); > + } {} braces not needed. > + if (!strcmp(argv[0], "list")) > + return list(argc, argv, prefix); > if (!strcmp(argv[0], "run")) This should be "else if" now. (Doesn't matter for code execution, just IMO readability, but I'll leave that to you ... :) > return run(argc, argv, prefix); > else > diff --git a/hook.c b/hook.c > index 935751fa6c..19138a8290 100644 > --- a/hook.c > +++ b/hook.c > @@ -104,22 +104,20 @@ int hook_exists(const char *name) > struct list_head* hook_list(const char* hookname) > { > struct list_head *hook_head = xmalloc(sizeof(struct list_head)); > + const char *hook_path = find_hook(hookname); > + > > INIT_LIST_HEAD(hook_head); > > if (!hookname) > return NULL; > > - if (have_git_dir()) { > - const char *hook_path = find_hook(hookname); > - > - /* Add the hook from the hookdir */ > - if (hook_path) { > - struct hook *to_add = xmalloc(sizeof(*to_add)); > - to_add->hook_path = hook_path; > - to_add->feed_pipe_cb_data = NULL; > - list_add_tail(&to_add->list, hook_head); > - } > + /* Add the hook from the hookdir */ > + if (hook_path) { > + struct hook *to_add = xmalloc(sizeof(*to_add)); > + to_add->hook_path = hook_path; > + to_add->feed_pipe_cb_data = NULL; > + list_add_tail(&to_add->list, hook_head); Maybe we should have a INIT for "struct hook" too? This also needlessly leaves behind an un-free'd hook struct in a way that it wouldn't if we just had this on the stack, no?
On Fri, Jul 16, 2021 at 10:52:27AM +0200, Ævar Arnfjörð Bjarmason wrote: > > > On Thu, Jul 15 2021, Emily Shaffer wrote: > > > static const char * const builtin_hook_usage[] = { > > N_("git hook <command> [...]"), > > + N_("git hook list <hookname>"), > > N_("git hook run [<args>] <hook-name> [-- <hook-args>]"), > > NULL > > Uses <hook-name> already, let's use that too. I can't remember if it's > something I changed myself, or if your original version used both and I > picked one for consistency, or... > > Anyway, I can re-roll the base topic or whatever, but let's have the end > result use one or the other. 'hook-name' is fine, I'll use that. Thanks for pointing it out. > > > + if (argc < 1) { > > + usage_msg_opt(_("You must specify a hook event name to list."), > > + builtin_hook_usage, list_options); > > + } > > {} braces not needed. ACK > > > > + if (!strcmp(argv[0], "list")) > > + return list(argc, argv, prefix); > > if (!strcmp(argv[0], "run")) > > This should be "else if" now. > > (Doesn't matter for code execution, just IMO readability, but I'll leave > that to you ... :) Eh, I think it's easier to read in the strcmps line up, so I will leave it this way ;) > > > return run(argc, argv, prefix); > > else > > diff --git a/hook.c b/hook.c > > index 935751fa6c..19138a8290 100644 > > --- a/hook.c > > +++ b/hook.c > > @@ -104,22 +104,20 @@ int hook_exists(const char *name) > > struct list_head* hook_list(const char* hookname) > > { > > struct list_head *hook_head = xmalloc(sizeof(struct list_head)); > > + const char *hook_path = find_hook(hookname); > > + > > > > INIT_LIST_HEAD(hook_head); > > > > if (!hookname) > > return NULL; > > > > - if (have_git_dir()) { > > - const char *hook_path = find_hook(hookname); > > - > > - /* Add the hook from the hookdir */ > > - if (hook_path) { > > - struct hook *to_add = xmalloc(sizeof(*to_add)); > > - to_add->hook_path = hook_path; > > - to_add->feed_pipe_cb_data = NULL; > > - list_add_tail(&to_add->list, hook_head); > > - } > > + /* Add the hook from the hookdir */ > > + if (hook_path) { > > + struct hook *to_add = xmalloc(sizeof(*to_add)); > > + to_add->hook_path = hook_path; > > + to_add->feed_pipe_cb_data = NULL; > > + list_add_tail(&to_add->list, hook_head); > > Maybe we should have a INIT for "struct hook" too? This also needlessly > leaves behind an un-free'd hook struct in a way that it wouldn't if we > just had this on the stack, no? I can clean it up here, but I don't think we need an initializer for struct hook. This code chunk gets moved into one of the list manipulators (append_or_move() or something) after the config is introduced. I don't think it leaves an unfreed hook laying around, does it? list_add_tail() uses the malloc'd pointer, and we free() in clear_hook_list(). What am I missing? - Emily
On Thu, Jul 22 2021, Emily Shaffer wrote: > On Fri, Jul 16, 2021 at 10:52:27AM +0200, Ævar Arnfjörð Bjarmason wrote: >> >> >> On Thu, Jul 15 2021, Emily Shaffer wrote: >> >> > static const char * const builtin_hook_usage[] = { >> > N_("git hook <command> [...]"), >> > + N_("git hook list <hookname>"), >> > N_("git hook run [<args>] <hook-name> [-- <hook-args>]"), >> > NULL >> >> Uses <hook-name> already, let's use that too. I can't remember if it's >> something I changed myself, or if your original version used both and I >> picked one for consistency, or... >> >> Anyway, I can re-roll the base topic or whatever, but let's have the end >> result use one or the other. > > 'hook-name' is fine, I'll use that. Thanks for pointing it out. > >> >> > + if (argc < 1) { >> > + usage_msg_opt(_("You must specify a hook event name to list."), >> > + builtin_hook_usage, list_options); >> > + } >> >> {} braces not needed. > ACK >> >> >> > + if (!strcmp(argv[0], "list")) >> > + return list(argc, argv, prefix); >> > if (!strcmp(argv[0], "run")) >> >> This should be "else if" now. >> >> (Doesn't matter for code execution, just IMO readability, but I'll leave >> that to you ... :) > > Eh, I think it's easier to read in the strcmps line up, so I will leave > it this way ;) *nod* >> >> > return run(argc, argv, prefix); >> > else >> > diff --git a/hook.c b/hook.c >> > index 935751fa6c..19138a8290 100644 >> > --- a/hook.c >> > +++ b/hook.c >> > @@ -104,22 +104,20 @@ int hook_exists(const char *name) >> > struct list_head* hook_list(const char* hookname) >> > { >> > struct list_head *hook_head = xmalloc(sizeof(struct list_head)); >> > + const char *hook_path = find_hook(hookname); >> > + >> > >> > INIT_LIST_HEAD(hook_head); >> > >> > if (!hookname) >> > return NULL; >> > >> > - if (have_git_dir()) { >> > - const char *hook_path = find_hook(hookname); >> > - >> > - /* Add the hook from the hookdir */ >> > - if (hook_path) { >> > - struct hook *to_add = xmalloc(sizeof(*to_add)); >> > - to_add->hook_path = hook_path; >> > - to_add->feed_pipe_cb_data = NULL; >> > - list_add_tail(&to_add->list, hook_head); >> > - } >> > + /* Add the hook from the hookdir */ >> > + if (hook_path) { >> > + struct hook *to_add = xmalloc(sizeof(*to_add)); >> > + to_add->hook_path = hook_path; >> > + to_add->feed_pipe_cb_data = NULL; >> > + list_add_tail(&to_add->list, hook_head); >> >> Maybe we should have a INIT for "struct hook" too? This also needlessly >> leaves behind an un-free'd hook struct in a way that it wouldn't if we >> just had this on the stack, no? > > I can clean it up here, but I don't think we need an initializer for > struct hook. This code chunk gets moved into one of the list > manipulators (append_or_move() or something) after the config is > introduced. > > I don't think it leaves an unfreed hook laying around, does it? > list_add_tail() uses the malloc'd pointer, and we free() in > clear_hook_list(). What am I missing? I don't think you're missing anything. I replied on that "struct hook" in another E-Mail, i.e. I think I just misread parts of this. Thanks.
diff --git a/builtin/hook.c b/builtin/hook.c index d196d8498c..8340c8c9a8 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -8,6 +8,7 @@ static const char * const builtin_hook_usage[] = { N_("git hook <command> [...]"), + N_("git hook list <hookname>"), N_("git hook run [<args>] <hook-name> [-- <hook-args>]"), NULL }; @@ -18,6 +19,46 @@ static const char * const builtin_hook_run_usage[] = { NULL }; +static int list(int argc, const char **argv, const char *prefix) +{ + struct list_head *head, *pos; + const char *hookname = NULL; + struct strbuf hookdir_annotation = STRBUF_INIT; + + struct option list_options[] = { + OPT_END(), + }; + + argc = parse_options(argc, argv, prefix, list_options, + builtin_hook_usage, 0); + + if (argc < 1) { + usage_msg_opt(_("You must specify a hook event name to list."), + builtin_hook_usage, list_options); + } + + hookname = argv[0]; + + head = hook_list(hookname); + + if (list_empty(head)) { + printf(_("no commands configured for hook '%s'\n"), + hookname); + return 0; + } + + list_for_each(pos, head) { + struct hook *item = list_entry(pos, struct hook, list); + item = list_entry(pos, struct hook, list); + if (item) + printf("%s\n", item->hook_path); + } + + clear_hook_list(head); + strbuf_release(&hookdir_annotation); + + return 0; +} static int run(int argc, const char **argv, const char *prefix) { int i; @@ -88,6 +129,8 @@ int cmd_hook(int argc, const char **argv, const char *prefix) if (!argc) usage_with_options(builtin_hook_usage, builtin_hook_options); + if (!strcmp(argv[0], "list")) + return list(argc, argv, prefix); if (!strcmp(argv[0], "run")) return run(argc, argv, prefix); else diff --git a/hook.c b/hook.c index 935751fa6c..19138a8290 100644 --- a/hook.c +++ b/hook.c @@ -104,22 +104,20 @@ int hook_exists(const char *name) struct list_head* hook_list(const char* hookname) { struct list_head *hook_head = xmalloc(sizeof(struct list_head)); + const char *hook_path = find_hook(hookname); + INIT_LIST_HEAD(hook_head); if (!hookname) return NULL; - if (have_git_dir()) { - const char *hook_path = find_hook(hookname); - - /* Add the hook from the hookdir */ - if (hook_path) { - struct hook *to_add = xmalloc(sizeof(*to_add)); - to_add->hook_path = hook_path; - to_add->feed_pipe_cb_data = NULL; - list_add_tail(&to_add->list, hook_head); - } + /* Add the hook from the hookdir */ + if (hook_path) { + struct hook *to_add = xmalloc(sizeof(*to_add)); + to_add->hook_path = hook_path; + to_add->feed_pipe_cb_data = NULL; + list_add_tail(&to_add->list, hook_head); } return hook_head;
If more than one hook will be run, it may be useful to see a list of which hooks should be run. At very least, it will be useful for us to test the semantics of multihooks ourselves. For now, only list the hooks which will run in the order they will run in; later, it might be useful to include more information like where the hooks were configured and whether or not they will run. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- builtin/hook.c | 43 +++++++++++++++++++++++++++++++++++++++++++ hook.c | 18 ++++++++---------- 2 files changed, 51 insertions(+), 10 deletions(-)