| Message ID | 20210812004258.74318-4-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | config-based hooks restarted | expand |
Emily Shaffer <emilyshaffer@google.com> writes: > +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)) { The same "can't hook_list() signal an error by returning NULL?" comment applies here. head = hook_list(hookname); if (!head) die(_("no such hook '%s'"), hookname); or something? > + printf(_("no commands configured for hook '%s'\n"), > + hookname); > + return 0; If it is a normally expected state that there is no hook for the given name, signalling success by returning 0 here may be sensible, but then the message should at least go to the standard error stream to leave the standard output empty, so that a caller can reasonably do something like for path in $(git hooks list "$1") do ls -l "$path" done If we really want to show such a message, perhaps if (list_empty(head)) { if (!quiet) warning(_("no commands configured")); return 0; } The normal display just shows the path without saying "command %s will run for hook %s"; the warning probably should do the same. Having said that, if it truly is a normal and expected state that no hook is defined for the given name, I actually think there should be no message. > + } > + > + 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); > + } > diff --git a/hook.c b/hook.c > index 37f682c6d8..2714b63473 100644 > --- a/hook.c > +++ b/hook.c > @@ -96,22 +96,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); > } I do not think this belongs to the step to add "list" command. The log message does not explain or justify why have-git-dir goes away, either.
On Thu, Aug 12, 2021 at 11:59:51AM -0700, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > > +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)) { > > The same "can't hook_list() signal an error by returning NULL?" > comment applies here. > > head = hook_list(hookname); > if (!head) > die(_("no such hook '%s'"), hookname); > > or something? > > > + printf(_("no commands configured for hook '%s'\n"), > > + hookname); > > + return 0; > > If it is a normally expected state that there is no hook for the > given name, signalling success by returning 0 here may be sensible, > but then the message should at least go to the standard error stream > to leave the standard output empty, so that a caller can reasonably > do something like > > for path in $(git hooks list "$1") > do > ls -l "$path" > done > > If we really want to show such a message, perhaps > > if (list_empty(head)) { > if (!quiet) > warning(_("no commands configured")); > return 0; > } > > The normal display just shows the path without saying "command %s > will run for hook %s"; the warning probably should do the same. > > Having said that, if it truly is a normal and expected state that no > hook is defined for the given name, I actually think there should be > no message. Ah, I think you are saying "either return an error code and be chatty if you want, or return an empty list and a success code, but pick one". Makes sense to me. No message + well-defined return code sounds fine. I'll do that. > > > + } > > + > > + 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); > > + } > > > diff --git a/hook.c b/hook.c > > index 37f682c6d8..2714b63473 100644 > > --- a/hook.c > > +++ b/hook.c > > @@ -96,22 +96,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); > > } > > I do not think this belongs to the step to add "list" command. The > log message does not explain or justify why have-git-dir goes away, > either. Ah, sure. It seems like I also didn't update the documentation for 'git hook' command during this commit. Will fix that as well.
diff --git a/builtin/hook.c b/builtin/hook.c index 12c9126032..c36b05376c 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -8,8 +8,11 @@ #define BUILTIN_HOOK_RUN_USAGE \ N_("git hook run [--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,50 @@ 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)) { + 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 +135,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 37f682c6d8..2714b63473 100644 --- a/hook.c +++ b/hook.c @@ -96,22 +96,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 | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ hook.c | 18 ++++++++---------- 2 files changed, 57 insertions(+), 10 deletions(-)