| Message ID | 20210819033450.3382652-4-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config-based hooks restarted | expand |
On Wed, Aug 18 2021, Emily Shaffer wrote: > +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_list_usage, 0); > + > + if (argc < 1) > + usage_msg_opt(_("You must specify a hook event name to list."), > + builtin_hook_list_usage, list_options); Untested, but aren't we silently ignoring: git hook list pre-receive some extra gar bage here I.e. shouldn't this be an "argc != 1" check? > + > + hookname = argv[0]; > + > + head = hook_list(hookname); > + > + if (list_empty(head)) > + return 1; > + > + 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); Nit/suggestion: use puts(x) instead of printf("%s\n", x), but that's also a bikeshedding/style preference, so ignore if you disagree... > + } > + > + clear_hook_list(head); Nit/API suggestion: Maybe s/list_for_each/list_for_each_safe/ and remove_hook() in the loop would make more sense for this one-shot caller than iterating over the list twice? Anyway, currently remove_hook() is static, and it's probably good to not peek behind the curtain here, so on second thought clear_hook_list() is probably best... > + strbuf_release(&hookdir_annotation); This function did nothing with hookdir_annotation. Looks like leftover WIP code, but maybe it's used in (and should be moved to) a later commit, let's keep reading... > [...] > struct list_head *hook_head = xmalloc(sizeof(struct list_head)); > > + > INIT_LIST_HEAD(hook_head); ..ditto... > > if (!hookname) > @@ -103,8 +104,6 @@ struct list_head *list_hooks(const char *hookname) > > if (have_git_dir()) { > const char *hook_path = find_hook(hookname); > - ... earlier notes about whitespace churn... > - /* Add the hook from the hookdir */ > if (hook_path) { > struct hook *to_add = xmalloc(sizeof(*to_add)); > to_add->hook_path = hook_path;
On Wed, Aug 18 2021, Emily Shaffer wrote:
> + head = hook_list(hookname);
This doesn't compile, as hook_list() is added in a later
commit. Something something earlier suggestion of "git rebase -i --exec
'make test'" :)
On Tue, Aug 24, 2021 at 05:08:25PM +0200, Ævar Arnfjörð Bjarmason wrote: > > > On Wed, Aug 18 2021, Emily Shaffer wrote: > > > +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_list_usage, 0); > > + > > + if (argc < 1) > > + usage_msg_opt(_("You must specify a hook event name to list."), > > + builtin_hook_list_usage, list_options); > > Untested, but aren't we silently ignoring: > > git hook list pre-receive some extra gar bage here > > I.e. shouldn't this be an "argc != 1" check? Yeah, I think you are right. Will switch. > > > + > > + hookname = argv[0]; > > + > > + head = hook_list(hookname); > > + > > + if (list_empty(head)) > > + return 1; > > + > > + 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); > > Nit/suggestion: use puts(x) instead of printf("%s\n", x), but that's > also a bikeshedding/style preference, so ignore if you disagree... I was curious, because today I learned about puts() ;), so I checked (sorry for escape gore): $ gg puts\( | wc -l 217 $ gg "printf(\"%s\(\\\\n\)\?\"" | wc -l 96 So looks like it is indeed more idiomatic by about 2x to just use puts(). Will switch. > > > + } > > + > > + clear_hook_list(head); > > Nit/API suggestion: Maybe s/list_for_each/list_for_each_safe/ and > remove_hook() in the loop would make more sense for this one-shot caller > than iterating over the list twice? > > Anyway, currently remove_hook() is static, and it's probably good to not > peek behind the curtain here, so on second thought clear_hook_list() is > probably best... Sounds like you talked yourself out of it before I could. Noop ;) > > > + strbuf_release(&hookdir_annotation); > > This function did nothing with hookdir_annotation. Looks like leftover > WIP code, but maybe it's used in (and should be moved to) a later > commit, let's keep reading... Ah, leftover WIP code indeed. Will drop it. I do think, though, that "hook from hookdir" is an ugly thing to say in list(), so any better suggestions welcome. > > > [...] > > struct list_head *hook_head = xmalloc(sizeof(struct list_head)); > > > > + > > INIT_LIST_HEAD(hook_head); > > ..ditto... ACK > > > > if (!hookname) > > @@ -103,8 +104,6 @@ struct list_head *list_hooks(const char *hookname) > > > > if (have_git_dir()) { > > const char *hook_path = find_hook(hookname); > > - > > ... earlier notes about whitespace churn... > > > - /* Add the hook from the hookdir */ > > if (hook_path) { > > struct hook *to_add = xmalloc(sizeof(*to_add)); > > to_add->hook_path = hook_path; > Thanks. - Emily
On Tue, Aug 24, 2021 at 05:53:48PM +0200, Ævar Arnfjörð Bjarmason wrote: > > > On Wed, Aug 18 2021, Emily Shaffer wrote: > > > + head = hook_list(hookname); > > This doesn't compile, as hook_list() is added in a later > commit. Something something earlier suggestion of "git rebase -i --exec > 'make test'" :) Aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaargh. Thanks.
diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt index 8bf82b5dd4..701ada9fc0 100644 --- a/Documentation/git-hook.txt +++ b/Documentation/git-hook.txt @@ -10,6 +10,7 @@ SYNOPSIS [verse] 'git hook' run [--to-stdin=<path>] [--ignore-missing] [(-j|--jobs) <n>] <hook-name> [-- <hook-args>] +'git hook' list <hook-name> DESCRIPTION ----------- @@ -30,6 +31,10 @@ optional `--` (or `--end-of-options`, see linkgit:gitcli[7]). The arguments (if any) differ by hook name, see linkgit:githooks[5] for what those are. +list:: + Print a list of hooks which will be run on `<hook-name>` event. If no + hooks are configured for that event, print nothing and return 1. + OPTIONS ------- diff --git a/builtin/hook.c b/builtin/hook.c index 4dd3617876..d21f303eca 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -8,8 +8,11 @@ #define BUILTIN_HOOK_RUN_USAGE \ N_("git hook run [--ignore-missing] [--to-stdin=<path>] <hook-name> [-- <hook-args>]") +#define BUILTIN_HOOK_LIST_USAGE \ + N_("git hook list <hook-name>") static const char * const builtin_hook_usage[] = { + BUILTIN_HOOK_LIST_USAGE, BUILTIN_HOOK_RUN_USAGE, NULL }; @@ -19,6 +22,47 @@ static const char * const builtin_hook_run_usage[] = { NULL }; +static const char *const builtin_hook_list_usage[] = { + BUILTIN_HOOK_LIST_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_list_usage, 0); + + if (argc < 1) + usage_msg_opt(_("You must specify a hook event name to list."), + builtin_hook_list_usage, list_options); + + hookname = argv[0]; + + head = hook_list(hookname); + + if (list_empty(head)) + return 1; + + 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 +132,8 @@ int cmd_hook(int argc, const char **argv, const char *prefix) if (!argc) goto usage; + if (!strcmp(argv[0], "list")) + return list(argc, argv, prefix); if (!strcmp(argv[0], "run")) return run(argc, argv, prefix); diff --git a/hook.c b/hook.c index b8420353fa..b1ea372e15 100644 --- a/hook.c +++ b/hook.c @@ -96,6 +96,7 @@ struct list_head *list_hooks(const char *hookname) { struct list_head *hook_head = xmalloc(sizeof(struct list_head)); + INIT_LIST_HEAD(hook_head); if (!hookname) @@ -103,8 +104,6 @@ struct list_head *list_hooks(const char *hookname) 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;
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> --- Documentation/git-hook.txt | 5 +++++ builtin/hook.c | 46 ++++++++++++++++++++++++++++++++++++++ hook.c | 3 +-- 3 files changed, 52 insertions(+), 2 deletions(-)