Re: [PATCH v8 03/37] hook: add list command
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 12, 2021, 08:20 UTC
- Message-ID
- <87o8fohjgq.fsf@evledraar.gmail.com>
- In-Reply-To
- <20210311021037.3001235-4-emilyshaffer@google.com>
On Thu, Mar 11 2021, Emily Shaffer wrote:
Show 25 quoted lines
> diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt > new file mode 100644 > index 0000000000..71449ecbc7 > --- /dev/null > +++ b/Documentation/config/hook.txt > @@ -0,0 +1,9 @@ > +hook.<command>.command:: > + A command to execute during the <command> hook event. This can be an > + executable on your device, a oneliner for your shell, or the name of a > + hookcmd. See linkgit:git-hook[1]. > + > +hookcmd.<name>.command:: > + A command to execute during a hook for which <name> has been specified > + as a command. This can be an executable on your device or a oneliner for > + your shell. See linkgit:git-hook[1]. > diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt > index 9eeab0009d..f19875ed68 100644 > --- a/Documentation/git-hook.txt > +++ b/Documentation/git-hook.txt > @@ -8,12 +8,65 @@ git-hook - Manage configured hooks > SYNOPSIS > -------- > [verse] > -'git hook' > +'git hook' list <hook-name>
Having just read this far (maybe this pattern is shared in the rest of the series): Let's just squash this and the 2nd patch together.
Sometimes it's worth doing the scaffolding first, but adding a new built-in is so trivial that I don't think it's worth it, and it just results in back & forth churn like the above...
> DESCRIPTION > ----------- > -A placeholder command. Later, you will be able to list, add, and modify hooks > -with this command.
...and this...
Show 77 quoted lines
> +You can list configured hooks with this command. Later, you will be able to run, > +add, and modify hooks with this command. > + > +This command parses the default configuration files for sections `hook` and > +`hookcmd`. `hook` is used to describe the commands which will be run during a > +particular hook event; commands are run in the order Git encounters them during > +the configuration parse (see linkgit:git-config[1]). `hookcmd` is used to > +describe attributes of a specific command. If additional attributes don't need > +to be specified, a command to run can be specified directly in the `hook` > +section; if a `hookcmd` by that name isn't found, Git will attempt to run the > +provided value directly. For example: > + > +Global config > +---- > + [hook "post-commit"] > + command = "linter" > + command = "~/typocheck.sh" > + > + [hookcmd "linter"] > + command = "/bin/linter --c" > +---- > + > +Local config > +---- > + [hook "prepare-commit-msg"] > + command = "linter" > + [hook "post-commit"] > + command = "python ~/run-test-suite.py" > +---- > + > +With these configs, you'd then see: > + > +---- > +$ git hook list "post-commit" > +global: /bin/linter --c > +global: ~/typocheck.sh > +local: python ~/run-test-suite.py > + > +$ git hook list "prepare-commit-msg" > +local: /bin/linter --c > +---- > + > +COMMANDS > +-------- > + > +list `<hook-name>`:: > + > +List the hooks which have been configured for `<hook-name>`. Hooks appear > +in the order they should be run, and print the config scope where the relevant > +`hook.<hook-name>.command` was specified, not the `hookcmd` (if applicable). > +This output is human-readable and the format is subject to change over time. > + > +CONFIGURATION > +------------- > +include::config/hook.txt[] > > GIT > --- > diff --git a/Makefile b/Makefile > index 8e904a1ab5..3fa51597d8 100644 > --- a/Makefile > +++ b/Makefile > @@ -891,6 +891,7 @@ LIB_OBJS += hash-lookup.o > LIB_OBJS += hashmap.o > LIB_OBJS += help.o > LIB_OBJS += hex.o > +LIB_OBJS += hook.o > LIB_OBJS += ident.o > LIB_OBJS += json-writer.o > LIB_OBJS += kwset.o > diff --git a/builtin/hook.c b/builtin/hook.c > index b2bbc84d4d..bb64cd77ca 100644 > --- a/builtin/hook.c > +++ b/builtin/hook.c > @@ -1,21 +1,67 @@ > #include "cache.h" > -
Stray back & forth whitespace churn?
Show 14 quoted lines
> #include "builtin.h"
> +#include "config.h"
> +#include "hook.h"
> #include "parse-options.h"
> +#include "strbuf.h"
>
> static const char * const builtin_hook_usage[] = {
> - N_("git hook"),
> + N_("git hook list <hookname>"),
> NULL
> };
>
> -int cmd_hook(int argc, const char **argv, const char *prefix)
> +static int list(int argc, const char **argv, const char *prefix)...and here the cmd_hook() function being replaced (not really, just moved below, but you get my drift...)
Show 22 quoted lines
> {
> - struct option builtin_hook_options[] = {
> + struct list_head *head, *pos;
> + struct strbuf hookname = STRBUF_INIT;
> +
> + struct option list_options[] = {
> OPT_END(),
> };
>
> - argc = parse_options(argc, argv, prefix, builtin_hook_options,
> + 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);
> + }
> +
> + strbuf_addstr(&hookname, argv[0]);
> +
> + head = hook_list(&hookname);
> +More on strbuf usage later in another soon-to-be-sent E-Mail.
Show 52 quoted lines
> + if (list_empty(head)) {
> + printf(_("no commands configured for hook '%s'\n"),
> + hookname.buf);
> + strbuf_release(&hookname);
> + return 0;
> + }
> +
> + list_for_each(pos, head) {
> + struct hook *item = list_entry(pos, struct hook, list);
> + if (item)
> + printf("%s: %s\n",
> + config_scope_name(item->origin),
> + item->command.buf);
> + }
> +
> + clear_hook_list(head);
> + strbuf_release(&hookname);
> +
> return 0;
> }
> +
> +int cmd_hook(int argc, const char **argv, const char *prefix)
> +{
> + struct option builtin_hook_options[] = {
> + OPT_END(),
> + };
> + if (argc < 2)
> + usage_with_options(builtin_hook_usage, builtin_hook_options);
> +
> + if (!strcmp(argv[1], "list"))
> + return list(argc - 1, argv + 1, prefix);
> +
> + usage_with_options(builtin_hook_usage, builtin_hook_options);
> +}
> diff --git a/hook.c b/hook.c
> new file mode 100644
> index 0000000000..fede40e925
> --- /dev/null
> +++ b/hook.c
> @@ -0,0 +1,120 @@
> +#include "cache.h"
> +
> +#include "hook.h"
> +#include "config.h"
> +
> +void free_hook(struct hook *ptr)
> +{
> + if (ptr) {
> + strbuf_release(&ptr->command);
> + free(ptr);
> + }
> +}Neither strbuf_release() nor free() need or should have a "if (ptr)" guard.
Show 100 quoted lines
> +
> +static void append_or_move_hook(struct list_head *head, const char *command)
> +{
> + struct list_head *pos = NULL, *tmp = NULL;
> + struct hook *to_add = NULL;
> +
> + /*
> + * remove the prior entry with this command; we'll replace it at the
> + * end.
> + */
> + list_for_each_safe(pos, tmp, head) {
> + struct hook *it = list_entry(pos, struct hook, list);
> + if (!strcmp(it->command.buf, command)) {
> + list_del(pos);
> + /* we'll simply move the hook to the end */
> + to_add = it;
> + break;
> + }
> + }
> +
> + if (!to_add) {
> + /* adding a new hook, not moving an old one */
> + to_add = xmalloc(sizeof(*to_add));
> + strbuf_init(&to_add->command, 0);
> + strbuf_addstr(&to_add->command, command);
> + }
> +
> + /* re-set the scope so we show where an override was specified */
> + to_add->origin = current_config_scope();
> +
> + list_add_tail(&to_add->list, head);
> +}
> +
> +static void remove_hook(struct list_head *to_remove)
> +{
> + struct hook *hook_to_remove = list_entry(to_remove, struct hook, list);
> + list_del(to_remove);
> + free_hook(hook_to_remove);
> +}
> +
> +void clear_hook_list(struct list_head *head)
> +{
> + struct list_head *pos, *tmp;
> + list_for_each_safe(pos, tmp, head)
> + remove_hook(pos);
> +}
> +
> +struct hook_config_cb
> +{
> + struct strbuf *hookname;
> + struct list_head *list;
> +};
> +
> +static int hook_config_lookup(const char *key, const char *value, void *cb_data)
> +{
> + struct hook_config_cb *data = cb_data;
> + const char *hook_key = data->hookname->buf;
> + struct list_head *head = data->list;
> +
> + if (!strcmp(key, hook_key)) {
> + const char *command = value;
> + struct strbuf hookcmd_name = STRBUF_INIT;
> +
> + /*
> + * Check if a hookcmd with that name exists. If it doesn't,
> + * 'git_config_get_value()' is documented not to touch &command,
> + * so we don't need to do anything.
> + */
> + strbuf_addf(&hookcmd_name, "hookcmd.%s.command", command);
> + git_config_get_value(hookcmd_name.buf, &command);
> +
> + if (!command) {
> + strbuf_release(&hookcmd_name);
> + BUG("git_config_get_value overwrote a string it shouldn't have");
> + }
> +
> + /*
> + * TODO: implement an option-getting callback, e.g.
> + * get configs by pattern hookcmd.$value.*
> + * for each key+value, do_callback(key, value, cb_data)
> + */
> +
> + append_or_move_hook(head, command);
> +
> + strbuf_release(&hookcmd_name);
> + }
> +
> + return 0;
> +}
> +
> +struct list_head* hook_list(const struct strbuf* hookname)
> +{
> + struct strbuf hook_key = STRBUF_INIT;
> + struct list_head *hook_head = xmalloc(sizeof(struct list_head));
> + struct hook_config_cb cb_data = { &hook_key, hook_head };
> +
> + INIT_LIST_HEAD(hook_head);
> +
> + if (!hookname)
> + return NULL;...if a strbuf being passed in is NULL?
Show 9 quoted lines
> [...] > +ROOT= > +if test_have_prereq MINGW > +then > + # In Git for Windows, Unix-like paths work only in shell scripts; > + # `git.exe`, however, will prefix them with the pseudo root directory > + # (of the Unix shell). Let's accommodate for that. > + ROOT="$(cd / && pwd)" > +fi
I didn't read up on previous rounds, but if we're squashing this into 02 having a seperate commit summarizing this little hack would be most welcome, or have it in this commit message.
Isn't this sort of thing generally usable, maybe we can add it under a longer variable name to test-lib.sh?