| Message ID | 20210311021037.3001235-1-emilyshaffer@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | config-based hooks | expand |
Emily Shaffer <emilyshaffer@google.com> writes: > Since v7: > - Addressed Jonathan Tan's review of part I > - Addressed Junio's review of part I and II > - Combined parts I and II > > I think the updates to patch 1 between the rest of the work I've been > doing probably have covered Ævar's comments. > > More details about per-patch changes found in the notes on each mail (I > hope). > > I know that Junio was talking about merging v7 after Josh Steadmon's > review and I asked him not to - this reroll has those changes from > Jonathan Tan's review that I was wanting to wait for. I picked it up and replaced, not necessarily because it is an urgent thing to do during the pre-release period, but primarily because I wanted to be prepared for any nasty surprises by unmanageable conflicts I may have to face once the current cycle is over. It turns out that it was a bit painful to merge to 'seen' as there are in-flight topics that touch the hooks documentation, and the changes they make must be carried forward to the new file. But it was not too bad. The merge into 'seen' is 3cdeaeab (Merge branch 'es/config-hooks' into seen, 2021-03-11) as of this writing, and the output of $ git diff 3cdeaeab3a^:Documentation/githooks.txt \ 3cdeaeab3a:Documentation/native-hooks.txt (i.e. the version of the file before the merge, where your topic being merged took material to edit to produce the new "native-hooks" document, is compared with the result) looks reasonable to me, but please double check. Thanks.
On Thu, Mar 11 2021, Emily Shaffer wrote: > Since v7: > - Addressed Jonathan Tan's review of part I > - Addressed Junio's review of part I and II > - Combined parts I and II > > I think the updates to patch 1 between the rest of the work I've been > doing probably have covered Ævar's comments. A range-diff between iterations of such a large series would be most useful. Do you have a public repo with tags or whatever the different versions, for those who'd like an easier way to follow along the differing versions than scraping the ML archive? While reading this I came up with the following fixup patches on top, for discussion, maybe not something you want as-is: Documentation/git-hook.txt | 8 +++++ builtin/bugreport.c | 8 +++-- builtin/commit.c | 3 +- builtin/hook.c | 79 ++++++++++++++++++++-------------------------- builtin/merge.c | 3 +- builtin/receive-pack.c | 11 +++---- hook.c | 21 +++++------- hook.h | 5 +-- refs.c | 4 ++- sequencer.c | 4 ++- 10 files changed, 73 insertions(+), 73 deletions(-) diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt index 4ad31ac360a..5c9af30b43e 100644 --- a/Documentation/git-hook.txt +++ b/Documentation/git-hook.txt @@ -150,10 +150,18 @@ message body and cannot be parallelized. CONFIGURATION ------------- + +The below documentation is the same as what's found in +linkgit:git-config[1]: + include::config/hook.txt[] HOOKS ----- + +The below documentation is the same as what's found in +linkgit:githooks[5]: + include::native-hooks.txt[] Noted in another reply, including it here for completeness. GIT diff --git a/builtin/bugreport.c b/builtin/bugreport.c index 04467cd1d3a..b64e53fd625 100644 --- a/builtin/bugreport.c +++ b/builtin/bugreport.c @@ -81,9 +81,13 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit) return; } - for (i = 0; i < ARRAY_SIZE(hook); i++) - if (hook_exists(hook[i], HOOKDIR_USE_CONFIG)) + for (i = 0; i < ARRAY_SIZE(hook); i++) { + struct strbuf config; + strbuf_addf(&config, "hook.%s.config", hook[i]); + if (hook_exists(hook[i], config.buf, HOOKDIR_USE_CONFIG)) strbuf_addf(hook_info, "%s\n", hook[i]); + strbuf_release(&config); + } } Less strbuf, see below. static const char * const bugreport_usage[] = { diff --git a/builtin/commit.c b/builtin/commit.c index 31df571f123..fc9f1f5ee58 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -984,7 +984,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix, return 0; } - if (!no_verify && hook_exists("pre-commit", HOOKDIR_USE_CONFIG)) { + if (!no_verify && hook_exists("pre-commit", "hook.pre-commit.command", + HOOKDIR_USE_CONFIG)) { /* * Re-read the index as pre-commit hook could have updated it, * and write it out as a tree. We must do this before we invoke ..ditto. diff --git a/builtin/hook.c b/builtin/hook.c index b4f4adb1dea..d0b56ee47f8 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -18,8 +18,6 @@ static enum hookdir_opt should_run_hookdir; static int list(int argc, const char **argv, const char *prefix) { struct list_head *head, *pos; - struct strbuf hookname = STRBUF_INIT; - struct strbuf hookdir_annotation = STRBUF_INIT; struct option list_options[] = { OPT_END(), @@ -33,67 +31,60 @@ static int list(int argc, const char **argv, const char *prefix) builtin_hook_usage, list_options); } - strbuf_addstr(&hookname, argv[0]); - - head = hook_list(&hookname); + head = hook_list(argv[0]); if (list_empty(head)) { printf(_("no commands configured for hook '%s'\n"), - hookname.buf); - strbuf_release(&hookname); + argv[0]); return 0; } - switch (should_run_hookdir) { - case HOOKDIR_NO: - strbuf_addstr(&hookdir_annotation, _(" (will not run)")); - break; - case HOOKDIR_ERROR: - strbuf_addstr(&hookdir_annotation, _(" (will error and not run)")); - break; - case HOOKDIR_INTERACTIVE: - strbuf_addstr(&hookdir_annotation, _(" (will prompt)")); - break; - case HOOKDIR_WARN: - strbuf_addstr(&hookdir_annotation, _(" (will warn but run)")); - break; - case HOOKDIR_YES: - /* - * The default behavior should agree with - * hook.c:configured_hookdir_opt(). HOOKDIR_UNKNOWN should just - * do the default behavior. - */ - case HOOKDIR_UNKNOWN: - default: - break; - } - list_for_each(pos, head) { struct hook *item = list_entry(pos, struct hook, list); item = list_entry(pos, struct hook, list); if (item) { - /* Don't translate 'hookdir' - it matches the config */ - printf("%s: %s%s\n", - (item->from_hookdir + const char *scope = item->from_hookdir ? "hookdir" - : config_scope_name(item->origin)), - item->command.buf, - (item->from_hookdir - ? hookdir_annotation.buf - : "")); + : config_scope_name(item->origin); + switch (should_run_hookdir) { + case HOOKDIR_NO: + printf(_("%s: %s (will not run)\n"), + scope, item->command.buf); + break; + case HOOKDIR_ERROR: + printf(_("%s: %s (will error and not run)\n"), + scope, item->command.buf); + break; + case HOOKDIR_INTERACTIVE: + printf(_("%s: %s (will prompt)\n"), + scope, item->command.buf); + break; + case HOOKDIR_WARN: + printf(_("%s: %s (will warn but run)\n"), + scope, item->command.buf); + break; + case HOOKDIR_YES: + /* + * The default behavior should agree with + * hook.c:configured_hookdir_opt(). HOOKDIR_UNKNOWN should just + * do the default behavior. + */ + case HOOKDIR_UNKNOWN: + default: + printf(_("%s: %s\n"), + scope, item->command.buf); + break; + } } } clear_hook_list(head); - strbuf_release(&hookdir_annotation); - strbuf_release(&hookname); return 0; } I think this is better to avoid i18n lego, as noted in another reply (but I didn't include the patch). More on strbuf below: static int run(int argc, const char **argv, const char *prefix) { - struct strbuf hookname = STRBUF_INIT; struct run_hooks_opt opt; int rc = 0; @@ -118,12 +109,10 @@ static int run(int argc, const char **argv, const char *prefix) usage_msg_opt(_("You must specify a hook event to run."), builtin_hook_usage, run_options); - strbuf_addstr(&hookname, argv[0]); opt.run_hookdir = should_run_hookdir; - rc = run_hooks(hookname.buf, &opt); + rc = run_hooks(argv[0], &opt); - strbuf_release(&hookname); run_hooks_opt_clear(&opt); return rc; diff --git a/builtin/merge.c b/builtin/merge.c index 3a2af257a6b..df4ff72fbc7 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -848,7 +848,8 @@ static void prepare_to_commit(struct commit_list *remoteheads) * and write it out as a tree. We must do this before we invoke * the editor and after we invoke run_status above. */ - if (hook_exists("pre-merge-commit", HOOKDIR_USE_CONFIG)) + if (hook_exists("pre-merge-commit", "hook.pre-merge-commit.command", + HOOKDIR_USE_CONFIG)) discard_cache(); read_cache_from(index_file); strbuf_addbuf(&msg, &merge_msg); diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index eaedeeb1e8b..a76069ea592 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1123,12 +1123,10 @@ static int run_proc_receive_hook(struct command *commands, int version = 0; int code; - struct strbuf hookname = STRBUF_INIT; struct hook *proc_receive = NULL; struct list_head *pos, *hooks; - strbuf_addstr(&hookname, "proc-receive"); - hooks = hook_list(&hookname); + hooks = hook_list("proc-receive"); list_for_each(pos, hooks) { if (proc_receive) { @@ -1460,8 +1458,6 @@ static const char *push_to_deploy(unsigned char *sha1, return NULL; } -static const char *push_to_checkout_hook = "push-to-checkout"; - static const char *push_to_checkout(unsigned char *hash, struct strvec *env, const char *work_tree) @@ -1472,7 +1468,7 @@ static const char *push_to_checkout(unsigned char *hash, strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree)); strvec_pushv(&opt.env, env->v); strvec_push(&opt.args, hash_to_hex(hash)); - if (run_hooks(push_to_checkout_hook, &opt)) { + if (run_hooks("push-to-checkout", &opt)) { run_hooks_opt_clear(&opt); return "push-to-checkout hook declined"; } else { @@ -1502,7 +1498,8 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir)); - if (!hook_exists(push_to_checkout_hook, HOOKDIR_USE_CONFIG)) + if (!hook_exists("push-to-checkout", "hook.push-to-checkout.command", + HOOKDIR_USE_CONFIG)) retval = push_to_deploy(sha1, &env, work_tree); else retval = push_to_checkout(sha1, &env, work_tree); diff --git a/hook.c b/hook.c index 7f6f3b9a616..49c3861ce00 100644 --- a/hook.c +++ b/hook.c @@ -247,7 +247,7 @@ static const char *find_legacy_hook(const char *name) } -struct list_head* hook_list(const struct strbuf* hookname) +struct list_head* hook_list(const char *hookname) { struct strbuf hook_key = STRBUF_INIT; struct list_head *hook_head = xmalloc(sizeof(struct list_head)); @@ -256,14 +256,14 @@ struct list_head* hook_list(const struct strbuf* hookname) INIT_LIST_HEAD(hook_head); if (!hookname) - return NULL; + BUG("???");; - strbuf_addf(&hook_key, "hook.%s.command", hookname->buf); + strbuf_addf(&hook_key, "hook.%s.command", hookname); git_config(hook_config_lookup, &cb_data); if (have_git_dir()) { - const char *legacy_hook_path = find_legacy_hook(hookname->buf); + const char *legacy_hook_path = find_legacy_hook(hookname); /* Unconditionally add legacy hook, but annotate it. */ if (legacy_hook_path) { @@ -300,10 +300,10 @@ void run_hooks_opt_init_async(struct run_hooks_opt *o) o->jobs = configured_hook_jobs(); } -int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir) +int hook_exists(const char *hookname, const char *hook_config, + enum hookdir_opt should_run_hookdir) { const char *value = NULL; /* throwaway */ - struct strbuf hook_key = STRBUF_INIT; int could_run_hookdir; if (should_run_hookdir == HOOKDIR_USE_CONFIG) @@ -314,9 +314,7 @@ int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir) should_run_hookdir == HOOKDIR_YES) && !!find_legacy_hook(hookname); - strbuf_addf(&hook_key, "hook.%s.command", hookname); - - return (!git_config_get_value(hook_key.buf, &value)) || could_run_hookdir; + return (!git_config_get_value(hook_config, &value)) || could_run_hookdir; } void run_hooks_opt_clear(struct run_hooks_opt *o) @@ -438,7 +436,6 @@ static int notify_hook_finished(int result, int run_hooks(const char *hookname, struct run_hooks_opt *options) { - struct strbuf hookname_str = STRBUF_INIT; struct list_head *to_run, *pos = NULL, *tmp = NULL; struct hook_cb_data cb_data = { 0, NULL, NULL, options }; @@ -448,9 +445,7 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options) if (options->path_to_stdin && options->feed_pipe) BUG("choose only one method to populate stdin"); - strbuf_addstr(&hookname_str, hookname); - - to_run = hook_list(&hookname_str); + to_run = hook_list(hookname); list_for_each_safe(pos, tmp, to_run) { struct hook *hook = list_entry(pos, struct hook, list); diff --git a/hook.h b/hook.h index 4ff9999b049..bfbbf36882d 100644 --- a/hook.h +++ b/hook.h @@ -26,7 +26,7 @@ struct hook { * Provides a linked list of 'struct hook' detailing commands which should run * in response to the 'hookname' event, in execution order. */ -struct list_head* hook_list(const struct strbuf *hookname); +struct list_head* hook_list(const char *hookname); enum hookdir_opt { @@ -123,7 +123,8 @@ void run_hooks_opt_clear(struct run_hooks_opt *o); * Like with run_hooks, if you take a --run-hookdir flag, reflect that * user-specified behavior here instead. */ -int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir); +int hook_exists(const char *hookname, const char *hook_config, + enum hookdir_opt should_run_hookdir); /* * Runs all hooks associated to the 'hookname' event in order. Each hook will be diff --git a/refs.c b/refs.c index 334fdd9103c..f01995fe64f 100644 --- a/refs.c +++ b/refs.c @@ -1966,7 +1966,9 @@ static int run_transaction_hook(struct ref_transaction *transaction, run_hooks_opt_init_async(&opt); - if (!hook_exists("reference-transaction", HOOKDIR_USE_CONFIG)) + if (!hook_exists("reference-transaction", + "hook.reference-transaction.command", + HOOKDIR_USE_CONFIG)) return ret; strvec_push(&opt.args, state); diff --git a/sequencer.c b/sequencer.c index 34ff275f0d1..52c067c1688 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1436,7 +1436,9 @@ static int try_to_commit(struct repository *r, } } - if (hook_exists("prepare-commit-msg", HOOKDIR_USE_CONFIG)) { + if (hook_exists("prepare-commit-msg", + "hook.prepare-commit-msg.command", + HOOKDIR_USE_CONFIG)) { res = run_prepare_commit_msg_hook(r, msg, hook_commit); if (res) goto out; There was another reply (from JT I believe, but didn't go back and look it up) about the over use of strbuf. I tend to agree, as much as I love the API it's really not better to write C with it if all you need is a const char* that's never modified, particularly if you get it from elsewhere. So it's really not meant for or good for "everything we need a const char*", but to avoid verbose realloc() dances all over the place, and for things like getline() loops without a hardcoded buffer size. E.g. in the first hunk here we're creating a strbuf just to copy argv[0] to it, and then throwing it away, let's just pass down argv[0]. For hook_exists I think just having the code more grep-able and having the config value inline is better, but I admit that's a matter of taste. I didn't try to find all such strbuf() occurrences, anyway, in the overall scheme of things it's a relatively small nit. I'm hoping to do some deeper diving into this series, in particular the parallelism, but just sending the shallow-ish comments I have for now. Thanks for working on this!
On Thu, Mar 11 2021, Emily Shaffer wrote: > Since v7: > - Addressed Jonathan Tan's review of part I > - Addressed Junio's review of part I and II > - Combined parts I and II > Comments on the overall design / goals (I don't just have strbuf nits): I think I mentioned this in earlier rounds, but I'm still very skeptical of the need for a "git hook" command for anything except the "run" case (which is very useful). So I tried patching it with this on top: diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt index 4f66bb35cf8..eb48da1dcf0 100644 --- a/Documentation/config/hook.txt +++ b/Documentation/config/hook.txt @@ -1,20 +1,17 @@ -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]. - -hookcmd.<name>.skip:: - Specify this boolean to remove a command from earlier in the execution - order. Useful if you want to make a single repo an exception to hook - configured at the system or global scope. If there is no hookcmd - specified for the command you want to skip, you can use the value of - `hook.<command>.command` as <name> as a shortcut. The "skip" setting - must be specified after the "hook.<command>.command" to have an effect. +hook.<name>.event:: +hook.<name>.command:: + A command to execute during a given hook event for which + <name> has been specified This can be an executable on your + device or a oneliner for your shell. See linkgit:git-hook[1]. ++ +As a convention setting this to the string `true` will clobber and +omit a command from earlier in the execution order. Similarly to the +"cat" special-case for `pager.<cmd>` we won't execute the hook at all +in that case. ++ +To have a single hook handle multiple types of events (such as +`pre-receive` and `post-receive`) specify `hook.<name>.event` multiple +times. hook.runHookDir:: Controls how hooks contained in your hookdir are executed. Can be any of I didn't finish that WIP patch, but I have yet to see any reason for why it wouldn't work. In experimenting with it further I tried just adding a "git config --show-hook" as a convenience alias for "git config --show-origin --show-scope --get-regexp '^hook\.<name>\.'", something like: diff --git a/builtin/config.c b/builtin/config.c index 963d65fd3fc..f62356b923a 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -33,6 +33,7 @@ static int end_nul; static int respect_includes_opt = -1; static struct config_options config_options; static int show_origin; +static int show_hook; static int show_scope; #define ACTION_GET (1<<0) @@ -159,6 +160,7 @@ static struct option builtin_config_options[] = { OPT_BOOL('z', "null", &end_nul, N_("terminate values with NUL byte")), OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")), OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")), + OPT_BOOL(0, "show-hook", &show_hook, N_("show configuration for a given hook (convenience alias for --show-origin --show-scope --get-regexp '^hook\\.<name>\\.')")), OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")), OPT_BOOL(0, "show-scope", &show_scope, N_("show scope of config (worktree, local, global, system, command)")), OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")), @@ -631,6 +633,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) { int nongit = !startup_info->have_repository; char *value; + struct strbuf show_hook_arg = STRBUF_INIT; given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT)); @@ -645,6 +648,14 @@ int cmd_config(int argc, const char **argv, const char *prefix) usage_builtin_config(); } + if (show_hook) { + strbuf_addf(&show_hook_arg, "^hook\\.%s\\.", argv[0]); + actions = ACTION_GET_REGEXP; + show_scope = 1; + argv[0] = show_hook_arg.buf; + } + + if (nongit) { if (use_local_config) die(_("--local can only be used inside a git repository")); @@ -915,5 +926,8 @@ int cmd_config(int argc, const char **argv, const char *prefix) return get_colorbool(argv[0], argc == 2); } + /* TODO: Memory leak on non-zero return, do we care? */ + strbuf_release(&show_hook_arg); + return 0; } So the reason that naïve approach doesn't work is that the current design has both a hook.<command>.command, *or* a hookcmd.<command>.<cfg>. So it can't be just a single --get-regexp, you need to statefully parse it, as indeed your implementation does. But this seems like a bad idea to me for at least these reasons I've thought of so far: 1. If we just change the design a bit we can make this a much simpler git-config wrapper, or point to that directly. 2. You're sticking full paths in the git config key, which is case-insensitive, and a feature of this format is being able to configure/override previously configured hooks. So the behavior of this feature depends on git's interaction with the case-sensitivity of filesystems, and not just one fs, any fs we're walking in our various config sources, and where the hook itself lives. As recent CVEs have shown that's a big can of worms, particularly for something whose goal is to address the security aspect of running hooks from other config. Arguably the case-sensitivity issue is just confusing since we canonicalize it anyway. But once you add in FS path canonicalization it becomes a real big can of worms. See the .gitmodules fsck code. Even if it wasn't for that it's relatively nastier to edit/maintain full paths and the appropriate escaping in the double-quoted key in the config file v.s. having it as an optionally quoted value. 3. We're left with this "*.command = cmd", and "*.skip = true" special-case syntax. I can't see any reason for why it's needed over simply having "*.command = true" clobber earlier hooks as noted in the proposed docs above. And that doesn't require any magic to support, just like our existing "core.pager=cat" case. I mean, I suppose it's magical in that we might otherwise error on non-consumed stdin (do we?), anyway, documenting it as a synonym for "cat >/dev/null" would get around that :) 4. It makes the common case of having the same hooks for N commands needlessly verbose, if you can just support "type" (or whatever we should call it) you can add that N times... 5. At the end of this series we're left with the start of the docs saying: You can list and run configured hooks with this command. Later, you will be able to add and modify hooks with this command. But those patches have yet to land, and looking at the design document I'm skeptical of that being a good addition v.s. just adding the same thing to "git config". As just one exmaple; surely "git config edit <name>" would need to run around and find config files to edit, then open them in a loop for you, no? Which we'd eventually want for "git config" in general with an --edit-regexp option or whatever, which brings us (well, at least me) back to "then let's just add it to git-config?". 6. The whole 'git hook' config special-casing doesn't help other commands or the security issue that seemed to have prompted (at least some of) its existence In the design doc we mention the "core.pager = rm -rf /" case for a .git/config. This series doesn't implement, but the design docs note a future want for solving that issue for the hooks. To me that's another case where we should just have general config syntax, not something hook-specific, e.g. if I could do this in my ~/.gitconfig: ;; We consider 'config.ignore' in reverse order, so e.g setting ;; it in. ~/.gitconfig will ignore any such keys for repo-level ;; config [config "ignore"] key = core.pager keyRegexp = "^hook\." We'd address both any hook security concerns, as well as core.pager etc. We could then just have e.g. some syntax sugar of: [include] path = built-in://gimme-safe-config Which would just be a thin layer of magit to include <path-to-git-prefix>/config-templates/gimme-safe-config or whatever. We'd thus address the issue for all config types without hook-specific magic. Anyway. I'm very willing to be convinced otherwise. I just think that for a first-draft implementation leaving aside 'hook.<command>.command' and the whole 'list' thing makes sense. We can consider the core code changes relatively separately from any future aspirations, particularly with a 40-some patch series, and the end-state of *this series* IMO not really justifying, that part of the implementation, and thus requiring reviewers to look ahead beyond the 40-some patches.
On Thu, Mar 11, 2021 at 02:26:10PM -0800, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > > Since v7: > > - Addressed Jonathan Tan's review of part I > > - Addressed Junio's review of part I and II > > - Combined parts I and II > > > > I think the updates to patch 1 between the rest of the work I've been > > doing probably have covered Ævar's comments. > > > > More details about per-patch changes found in the notes on each mail (I > > hope). > > > > I know that Junio was talking about merging v7 after Josh Steadmon's > > review and I asked him not to - this reroll has those changes from > > Jonathan Tan's review that I was wanting to wait for. > > I picked it up and replaced, not necessarily because it is an urgent > thing to do during the pre-release period, but primarily because I > wanted to be prepared for any nasty surprises by unmanageable > conflicts I may have to face once the current cycle is over. > > It turns out that it was a bit painful to merge to 'seen' as there > are in-flight topics that touch the hooks documentation, and the > changes they make must be carried forward to the new file. > > But it was not too bad. > > The merge into 'seen' is 3cdeaeab (Merge branch 'es/config-hooks' > into seen, 2021-03-11) as of this writing, and the output of > > $ git diff 3cdeaeab3a^:Documentation/githooks.txt \ > 3cdeaeab3a:Documentation/native-hooks.txt > > (i.e. the version of the file before the merge, where your topic > being merged took material to edit to produce the new "native-hooks" > document, is compared with the result) > > looks reasonable to me, but please double check. I had a look at that diff (but targeting 6da6893c, which is what I see for "Merge branch 'es/config-hooks' into seen" when I fetch from gitster/git today) and it looks fine to me, very reasonable. Thanks for doing that. > > Thanks.
On Fri, Mar 12, 2021 at 10:49:38AM +0100, Ævar Arnfjörð Bjarmason wrote: > > > On Thu, Mar 11 2021, Emily Shaffer wrote: > > > Since v7: > > - Addressed Jonathan Tan's review of part I > > - Addressed Junio's review of part I and II > > - Combined parts I and II > > > > I think the updates to patch 1 between the rest of the work I've been > > doing probably have covered Ævar's comments. > > A range-diff between iterations of such a large series would be most > useful. Do you have a public repo with tags or whatever the different > versions, for those who'd like an easier way to follow along the > differing versions than scraping the ML archive? I am really embarrassed to say that I don't have the branches/tags/whatever up. I have not succeeded in building that habit yet. I'll generate one from my local patches today and send it here. > > While reading this I came up with the following fixup patches on top, > for discussion, maybe not something you want as-is: I was a little bit confused reading this fixup without seeing the rest of your review, so I'll revisit this once I get through what else you wrote. > > Documentation/git-hook.txt | 8 +++++ > builtin/bugreport.c | 8 +++-- > builtin/commit.c | 3 +- > builtin/hook.c | 79 ++++++++++++++++++++-------------------------- > builtin/merge.c | 3 +- > builtin/receive-pack.c | 11 +++---- > hook.c | 21 +++++------- > hook.h | 5 +-- > refs.c | 4 ++- > sequencer.c | 4 ++- > 10 files changed, 73 insertions(+), 73 deletions(-) > > diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt > index 4ad31ac360a..5c9af30b43e 100644 > --- a/Documentation/git-hook.txt > +++ b/Documentation/git-hook.txt > @@ -150,10 +150,18 @@ message body and cannot be parallelized. > > CONFIGURATION > ------------- > + > +The below documentation is the same as what's found in > +linkgit:git-config[1]: > + > include::config/hook.txt[] > > HOOKS > ----- > + > +The below documentation is the same as what's found in > +linkgit:githooks[5]: > + > include::native-hooks.txt[] > > Noted in another reply, including it here for completeness. > > GIT > diff --git a/builtin/bugreport.c b/builtin/bugreport.c > index 04467cd1d3a..b64e53fd625 100644 > --- a/builtin/bugreport.c > +++ b/builtin/bugreport.c > @@ -81,9 +81,13 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit) > return; > } > > - for (i = 0; i < ARRAY_SIZE(hook); i++) > - if (hook_exists(hook[i], HOOKDIR_USE_CONFIG)) > + for (i = 0; i < ARRAY_SIZE(hook); i++) { > + struct strbuf config; > + strbuf_addf(&config, "hook.%s.config", hook[i]); > + if (hook_exists(hook[i], config.buf, HOOKDIR_USE_CONFIG)) > strbuf_addf(hook_info, "%s\n", hook[i]); > + strbuf_release(&config); > + } > } > > Less strbuf, see below. > > static const char * const bugreport_usage[] = { > diff --git a/builtin/commit.c b/builtin/commit.c > index 31df571f123..fc9f1f5ee58 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -984,7 +984,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > return 0; > } > > - if (!no_verify && hook_exists("pre-commit", HOOKDIR_USE_CONFIG)) { > + if (!no_verify && hook_exists("pre-commit", "hook.pre-commit.command", > + HOOKDIR_USE_CONFIG)) { > /* > * Re-read the index as pre-commit hook could have updated it, > * and write it out as a tree. We must do this before we invoke > > > ..ditto. > > diff --git a/builtin/hook.c b/builtin/hook.c > index b4f4adb1dea..d0b56ee47f8 100644 > --- a/builtin/hook.c > +++ b/builtin/hook.c > @@ -18,8 +18,6 @@ static enum hookdir_opt should_run_hookdir; > static int list(int argc, const char **argv, const char *prefix) > { > struct list_head *head, *pos; > - struct strbuf hookname = STRBUF_INIT; > - struct strbuf hookdir_annotation = STRBUF_INIT; > > struct option list_options[] = { > OPT_END(), > @@ -33,67 +31,60 @@ static int list(int argc, const char **argv, const char *prefix) > builtin_hook_usage, list_options); > } > > - strbuf_addstr(&hookname, argv[0]); > - > - head = hook_list(&hookname); > + head = hook_list(argv[0]); > > if (list_empty(head)) { > printf(_("no commands configured for hook '%s'\n"), > - hookname.buf); > - strbuf_release(&hookname); > + argv[0]); > return 0; > } > > - switch (should_run_hookdir) { > - case HOOKDIR_NO: > - strbuf_addstr(&hookdir_annotation, _(" (will not run)")); > - break; > - case HOOKDIR_ERROR: > - strbuf_addstr(&hookdir_annotation, _(" (will error and not run)")); > - break; > - case HOOKDIR_INTERACTIVE: > - strbuf_addstr(&hookdir_annotation, _(" (will prompt)")); > - break; > - case HOOKDIR_WARN: > - strbuf_addstr(&hookdir_annotation, _(" (will warn but run)")); > - break; > - case HOOKDIR_YES: > - /* > - * The default behavior should agree with > - * hook.c:configured_hookdir_opt(). HOOKDIR_UNKNOWN should just > - * do the default behavior. > - */ > - case HOOKDIR_UNKNOWN: > - default: > - break; > - } > - > list_for_each(pos, head) { > struct hook *item = list_entry(pos, struct hook, list); > item = list_entry(pos, struct hook, list); > if (item) { > - /* Don't translate 'hookdir' - it matches the config */ > - printf("%s: %s%s\n", > - (item->from_hookdir > + const char *scope = item->from_hookdir > ? "hookdir" > - : config_scope_name(item->origin)), > - item->command.buf, > - (item->from_hookdir > - ? hookdir_annotation.buf > - : "")); > + : config_scope_name(item->origin); > + switch (should_run_hookdir) { > + case HOOKDIR_NO: > + printf(_("%s: %s (will not run)\n"), > + scope, item->command.buf); > + break; > + case HOOKDIR_ERROR: > + printf(_("%s: %s (will error and not run)\n"), > + scope, item->command.buf); > + break; > + case HOOKDIR_INTERACTIVE: > + printf(_("%s: %s (will prompt)\n"), > + scope, item->command.buf); > + break; > + case HOOKDIR_WARN: > + printf(_("%s: %s (will warn but run)\n"), > + scope, item->command.buf); > + break; > + case HOOKDIR_YES: > + /* > + * The default behavior should agree with > + * hook.c:configured_hookdir_opt(). HOOKDIR_UNKNOWN should just > + * do the default behavior. > + */ > + case HOOKDIR_UNKNOWN: > + default: > + printf(_("%s: %s\n"), > + scope, item->command.buf); > + break; > + } > } > } > > clear_hook_list(head); > - strbuf_release(&hookdir_annotation); > - strbuf_release(&hookname); > > return 0; > } > > I think this is better to avoid i18n lego, as noted in another reply > (but I didn't include the patch). > > More on strbuf below: > > static int run(int argc, const char **argv, const char *prefix) > { > - struct strbuf hookname = STRBUF_INIT; > struct run_hooks_opt opt; > int rc = 0; > > @@ -118,12 +109,10 @@ static int run(int argc, const char **argv, const char *prefix) > usage_msg_opt(_("You must specify a hook event to run."), > builtin_hook_usage, run_options); > > - strbuf_addstr(&hookname, argv[0]); > opt.run_hookdir = should_run_hookdir; > > - rc = run_hooks(hookname.buf, &opt); > + rc = run_hooks(argv[0], &opt); > > - strbuf_release(&hookname); > run_hooks_opt_clear(&opt); > > return rc; > diff --git a/builtin/merge.c b/builtin/merge.c > index 3a2af257a6b..df4ff72fbc7 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -848,7 +848,8 @@ static void prepare_to_commit(struct commit_list *remoteheads) > * and write it out as a tree. We must do this before we invoke > * the editor and after we invoke run_status above. > */ > - if (hook_exists("pre-merge-commit", HOOKDIR_USE_CONFIG)) > + if (hook_exists("pre-merge-commit", "hook.pre-merge-commit.command", > + HOOKDIR_USE_CONFIG)) > discard_cache(); > read_cache_from(index_file); > strbuf_addbuf(&msg, &merge_msg); > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c > index eaedeeb1e8b..a76069ea592 100644 > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -1123,12 +1123,10 @@ static int run_proc_receive_hook(struct command *commands, > int version = 0; > int code; > > - struct strbuf hookname = STRBUF_INIT; > struct hook *proc_receive = NULL; > struct list_head *pos, *hooks; > > - strbuf_addstr(&hookname, "proc-receive"); > - hooks = hook_list(&hookname); > + hooks = hook_list("proc-receive"); > > list_for_each(pos, hooks) { > if (proc_receive) { > @@ -1460,8 +1458,6 @@ static const char *push_to_deploy(unsigned char *sha1, > return NULL; > } > > -static const char *push_to_checkout_hook = "push-to-checkout"; > - > static const char *push_to_checkout(unsigned char *hash, > struct strvec *env, > const char *work_tree) > @@ -1472,7 +1468,7 @@ static const char *push_to_checkout(unsigned char *hash, > strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree)); > strvec_pushv(&opt.env, env->v); > strvec_push(&opt.args, hash_to_hex(hash)); > - if (run_hooks(push_to_checkout_hook, &opt)) { > + if (run_hooks("push-to-checkout", &opt)) { > run_hooks_opt_clear(&opt); > return "push-to-checkout hook declined"; > } else { > @@ -1502,7 +1498,8 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w > > strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir)); > > - if (!hook_exists(push_to_checkout_hook, HOOKDIR_USE_CONFIG)) > + if (!hook_exists("push-to-checkout", "hook.push-to-checkout.command", > + HOOKDIR_USE_CONFIG)) > retval = push_to_deploy(sha1, &env, work_tree); > else > retval = push_to_checkout(sha1, &env, work_tree); > diff --git a/hook.c b/hook.c > index 7f6f3b9a616..49c3861ce00 100644 > --- a/hook.c > +++ b/hook.c > @@ -247,7 +247,7 @@ static const char *find_legacy_hook(const char *name) > } > > > -struct list_head* hook_list(const struct strbuf* hookname) > +struct list_head* hook_list(const char *hookname) > { > struct strbuf hook_key = STRBUF_INIT; > struct list_head *hook_head = xmalloc(sizeof(struct list_head)); > @@ -256,14 +256,14 @@ struct list_head* hook_list(const struct strbuf* hookname) > INIT_LIST_HEAD(hook_head); > > if (!hookname) > - return NULL; > + BUG("???");; > > - strbuf_addf(&hook_key, "hook.%s.command", hookname->buf); > + strbuf_addf(&hook_key, "hook.%s.command", hookname); > > git_config(hook_config_lookup, &cb_data); > > if (have_git_dir()) { > - const char *legacy_hook_path = find_legacy_hook(hookname->buf); > + const char *legacy_hook_path = find_legacy_hook(hookname); > > /* Unconditionally add legacy hook, but annotate it. */ > if (legacy_hook_path) { > @@ -300,10 +300,10 @@ void run_hooks_opt_init_async(struct run_hooks_opt *o) > o->jobs = configured_hook_jobs(); > } > > -int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir) > +int hook_exists(const char *hookname, const char *hook_config, > + enum hookdir_opt should_run_hookdir) > { > const char *value = NULL; /* throwaway */ > - struct strbuf hook_key = STRBUF_INIT; > int could_run_hookdir; > > if (should_run_hookdir == HOOKDIR_USE_CONFIG) > @@ -314,9 +314,7 @@ int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir) > should_run_hookdir == HOOKDIR_YES) > && !!find_legacy_hook(hookname); > > - strbuf_addf(&hook_key, "hook.%s.command", hookname); > - > - return (!git_config_get_value(hook_key.buf, &value)) || could_run_hookdir; > + return (!git_config_get_value(hook_config, &value)) || could_run_hookdir; > } > > void run_hooks_opt_clear(struct run_hooks_opt *o) > @@ -438,7 +436,6 @@ static int notify_hook_finished(int result, > > int run_hooks(const char *hookname, struct run_hooks_opt *options) > { > - struct strbuf hookname_str = STRBUF_INIT; > struct list_head *to_run, *pos = NULL, *tmp = NULL; > struct hook_cb_data cb_data = { 0, NULL, NULL, options }; > > @@ -448,9 +445,7 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options) > if (options->path_to_stdin && options->feed_pipe) > BUG("choose only one method to populate stdin"); > > - strbuf_addstr(&hookname_str, hookname); > - > - to_run = hook_list(&hookname_str); > + to_run = hook_list(hookname); > > list_for_each_safe(pos, tmp, to_run) { > struct hook *hook = list_entry(pos, struct hook, list); > diff --git a/hook.h b/hook.h > index 4ff9999b049..bfbbf36882d 100644 > --- a/hook.h > +++ b/hook.h > @@ -26,7 +26,7 @@ struct hook { > * Provides a linked list of 'struct hook' detailing commands which should run > * in response to the 'hookname' event, in execution order. > */ > -struct list_head* hook_list(const struct strbuf *hookname); > +struct list_head* hook_list(const char *hookname); > > enum hookdir_opt > { > @@ -123,7 +123,8 @@ void run_hooks_opt_clear(struct run_hooks_opt *o); > * Like with run_hooks, if you take a --run-hookdir flag, reflect that > * user-specified behavior here instead. > */ > -int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir); > +int hook_exists(const char *hookname, const char *hook_config, > + enum hookdir_opt should_run_hookdir); > > /* > * Runs all hooks associated to the 'hookname' event in order. Each hook will be > diff --git a/refs.c b/refs.c > index 334fdd9103c..f01995fe64f 100644 > --- a/refs.c > +++ b/refs.c > @@ -1966,7 +1966,9 @@ static int run_transaction_hook(struct ref_transaction *transaction, > > run_hooks_opt_init_async(&opt); > > - if (!hook_exists("reference-transaction", HOOKDIR_USE_CONFIG)) > + if (!hook_exists("reference-transaction", > + "hook.reference-transaction.command", > + HOOKDIR_USE_CONFIG)) > return ret; > > strvec_push(&opt.args, state); > diff --git a/sequencer.c b/sequencer.c > index 34ff275f0d1..52c067c1688 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -1436,7 +1436,9 @@ static int try_to_commit(struct repository *r, > } > } > > - if (hook_exists("prepare-commit-msg", HOOKDIR_USE_CONFIG)) { > + if (hook_exists("prepare-commit-msg", > + "hook.prepare-commit-msg.command", > + HOOKDIR_USE_CONFIG)) { > res = run_prepare_commit_msg_hook(r, msg, hook_commit); > if (res) > goto out; > > There was another reply (from JT I believe, but didn't go back and look > it up) about the over use of strbuf. > > I tend to agree, as much as I love the API it's really not better to > write C with it if all you need is a const char* that's never modified, > particularly if you get it from elsewhere. > > So it's really not meant for or good for "everything we need a const > char*", but to avoid verbose realloc() dances all over the place, and > for things like getline() loops without a hardcoded buffer size. > > E.g. in the first hunk here we're creating a strbuf just to copy argv[0] > to it, and then throwing it away, let's just pass down argv[0]. > > For hook_exists I think just having the code more grep-able and having > the config value inline is better, but I admit that's a matter of taste. > > I didn't try to find all such strbuf() occurrences, anyway, in the > overall scheme of things it's a relatively small nit. > > I'm hoping to do some deeper diving into this series, in particular the > parallelism, but just sending the shallow-ish comments I have for now. > > Thanks for working on this!
On Wed, Mar 17, 2021 at 11:41:59AM -0700, Emily Shaffer wrote: > > On Fri, Mar 12, 2021 at 10:49:38AM +0100, �var Arnfj�r� Bjarmason wrote: > > > > > > On Thu, Mar 11 2021, Emily Shaffer wrote: > > > > > Since v7: > > > - Addressed Jonathan Tan's review of part I > > > - Addressed Junio's review of part I and II > > > - Combined parts I and II > > > > > > I think the updates to patch 1 between the rest of the work I've been > > > doing probably have covered �var's comments. > > > > A range-diff between iterations of such a large series would be most > > useful. Do you have a public repo with tags or whatever the different > > versions, for those who'd like an easier way to follow along the > > differing versions than scraping the ML archive? > > I am really embarrassed to say that I don't have the > branches/tags/whatever up. I have not succeeded in building that habit > yet. I'll generate one from my local patches today and send it here. 1: be907f68b9 ! 1: a5e8c233c3 doc: propose hooks managed by the config @@ Documentation/technical/config-based-hooks.txt (new) +[[motivation]] +== Motivation + -+Replace the .git/hook/hookname path as the only source of hooks to execute; ++Replace the `.git/hook/hookname` path as the only source of hooks to execute; +allow users to define hooks using config files, in a way which is friendly to -+users with multiple repos which have similar needs. ++users with multiple repos which have similar needs - hooks can be easily shared ++between multiple Git repos. + +Redefine "hook" as an event rather than a single script, allowing users to +perform multiple unrelated actions on a single event. + -+Take a step closer to safety when copying zipped Git repositories from untrusted -+users by making it more apparent to users which scripts will be run during -+normal Git operations. -+ +Make it easier for users to discover Git's hook feature and automate their +workflows. + @@ Documentation/technical/config-based-hooks.txt (new) +number of cases: + +- "no": the legacy hook will not be run ++- "error": Git will print a warning to stderr before ignoring the legacy hook +- "interactive": Git will prompt the user before running the legacy hook +- "warn": Git will print a warning to stderr before running the legacy hook +- "yes" (default): Git will silently run the legacy hook @@ Documentation/technical/config-based-hooks.txt (new) +given which Git does not recognize, Git should discard that config entry. For +example, if "warn" was specified at system level and "junk" was specified at +global level, Git would resolve the value to "warn"; if the only time the config -+was set was to "junk", Git would use the default value of "yes". ++was set was to "junk", Git would use the default value of "yes" (but print a ++warning to the user first to let them know their value is wrong). + +`struct hookcmd` is expected to grow in size over time as more functionality is +added to hooks; so that other parts of the code don't need to understand the @@ Documentation/technical/config-based-hooks.txt (new) +=== Security and repo config + +Part of the motivation behind this refactor is to mitigate hooks as an attack -+vector;footnote:[https://lore.kernel.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/] -+however, as the design stands, users can still provide hooks in the repo-level -+config, which is included when a repo is zipped and sent elsewhere. The ++vector.footnote:[https://lore.kernel.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/] ++However, as the design stands, users can still provide hooks in the repo-level ++config, which is included when a repo is zipped and sent elsewhere. The +security of the repo-level config is still under discussion; this design -+generally assumes the repo-level config is secure, which is not true yet. The -+goal is to avoid an overcomplicated design to work around a problem which has -+ceased to exist. ++generally assumes the repo-level config is secure, which is not true yet. This ++assumption was made to avoid overcomplicating the design. So, this series ++doesn't particularly improve security or resistance to zip attacks. + +[[ease-of-use]] +=== Ease of use @@ Documentation/technical/config-based-hooks.txt (new) +A previous summary of alternatives exists in the +archives.footnote:[https://lore.kernel.org/git/20191116011125.GG22855@google.com] + -+[[status-quo]] -+=== Status quo -+ -+Today users can implement multihooks themselves by using a "trampoline script" -+as their hook, and pointing that script to a directory or list of other scripts -+they wish to run. -+ -+[[hook-directories]] -+=== Hook directories -+ -+Other contributors have suggested Git learn about the existence of a directory -+such as `.git/hooks/<hookname>.d` and execute those hooks in alphabetical order. -+ -+[[comparison]] -+=== Comparison table ++The table below shows a number of goals and how they might be achieved with ++config-based hooks, by implementing directory support (i.e. ++'.git/hooks/pre-commit.d'), or as hooks are run today. + +.Comparison of alternatives +|=== @@ Documentation/technical/config-based-hooks.txt (new) +|Natively +|With user effort + ++|Supports parallelization ++|Natively ++|Natively ++|No (user's multihook trampoline script would need to handle parallelism) ++ +|Safer for zipped repos +|A little +|No @@ Documentation/technical/config-based-hooks.txt (new) + +|Can install one hook to many repos +|Yes -+|No -+|No ++|With symlinks or core.hooksPath ++|With symlinks or core.hooksPath + +|Discoverability -+|Better (in `git help git`) -+|Same as before ++|Findable with 'git help git' or tab-completion via 'git hook' subcommand ++|Findable via improved documentation +|Same as before + +|Hard to run unexpected hook +|If configured -+|No ++|Could be made to warn or look for a config +|No +|=== + ++[[status-quo]] ++=== Status quo ++ ++Today users can implement multihooks themselves by using a "trampoline script" ++as their hook, and pointing that script to a directory or list of other scripts ++they wish to run. ++ ++[[hook-directories]] ++=== Hook directories ++ ++Other contributors have suggested Git learn about the existence of a directory ++such as `.git/hooks/<hookname>.d` and execute those hooks in alphabetical order. ++ +[[future-work]] +== Future work + @@ Documentation/technical/config-based-hooks.txt (new) +dependencies. If we decide to solve this problem, we may want to look to modern +build systems for inspiration on how to manage dependencies and parallel tasks. + ++[[nontrivial-hooks]] ++=== Multihooks and nontrivial output ++ ++Some hooks - like 'proc-receive' - don't lend themselves well to multihooks at ++all. In the case of 'proc-receive', for now, multiple hook definitions are ++disallowed. In the future we might be able to conceive a better approach, for ++example, running the hooks in series and using the output from one hook as the ++input to the next. ++ +[[securing-hookdir-hooks]] +=== Securing hookdir hooks + 2: b1d37c3911 = 2: a3e858d056 hook: scaffolding for git-hook subcommand 3: fea411c598 ! 3: 60b28a652b hook: add list command @@ Commit message run in config order. If more properties need to be set on a given hook in the future, commands can also be specified by providing "hook.<hookname>.command = <hookcmd-name>", as well as a "[hookcmd - <hookcmd-name>]" subsection; at minimum, this subsection must contain a + <hookcmd-name>]" subsection; this subsection should contain a "hookcmd.<hookcmd-name>.command = <path-to-hook>" line. For example: @@ Makefile: LIB_OBJS += hash-lookup.o ## builtin/hook.c ## @@ #include "cache.h" - +- #include "builtin.h" +#include "config.h" +#include "hook.h" @@ builtin/hook.c { - struct option builtin_hook_options[] = { + struct list_head *head, *pos; -+ struct hook *item; + struct strbuf hookname = STRBUF_INIT; + + struct option list_options[] = { @@ builtin/hook.c + } + + list_for_each(pos, head) { -+ item = list_entry(pos, struct hook, list); ++ struct hook *item = list_entry(pos, struct hook, list); + if (item) + printf("%s: %s\n", + config_scope_name(item->origin), @@ hook.c (new) + 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(struct hook)); ++ to_add = xmalloc(sizeof(*to_add)); + strbuf_init(&to_add->command, 0); + strbuf_addstr(&to_add->command, command); + } @@ hook.c (new) + /* re-set the scope so we show where an override was specified */ + to_add->origin = current_config_scope(); + -+ list_add_tail(&to_add->list, pos); ++ list_add_tail(&to_add->list, head); +} + +static void remove_hook(struct list_head *to_remove) @@ hook.c (new) + const char *command = value; + struct strbuf hookcmd_name = STRBUF_INIT; + -+ /* Check if a hookcmd with that name exists. */ ++ /* ++ * 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); + @@ hook.c (new) + + strbuf_addf(&hook_key, "hook.%s.command", hookname->buf); + -+ git_config(hook_config_lookup, (void*)&cb_data); ++ git_config(hook_config_lookup, &cb_data); + + strbuf_release(&hook_key); + return hook_head; @@ hook.h (new) +#include "list.h" +#include "strbuf.h" + -+struct hook -+{ ++struct hook { + struct list_head list; + /* + * Config file which holds the hook.*.command definition. 4: 89f1adf34d ! 4: d8232a8254 hook: include hookdir hook in list @@ Commit message $GIT_DIR/hooks/$HOOKNAME (or $HOOKDIR/$HOOKNAME). Although hooks taken from the config are more featureful than hooks placed in the $HOOKDIR, those hooks should not stop working for users who already have them. - - Legacy hooks should be run directly, not in shell. We know that they are - a path to an executable, not a oneliner script - and running them - directly takes care of path quoting concerns for us for free. + Let's list them to the user, but instead of displaying a config scope + (e.g. "global: blah") we can prefix them with "hookdir:". Signed-off-by: Emily Shaffer <emilyshaffer@google.com> ## builtin/hook.c ## @@ builtin/hook.c: static int list(int argc, const char **argv, const char *prefix) - struct list_head *head, *pos; - struct hook *item; - struct strbuf hookname = STRBUF_INIT; -+ struct strbuf hookdir_annotation = STRBUF_INIT; - - struct option list_options[] = { - OPT_END(), -@@ builtin/hook.c: static int list(int argc, const char **argv, const char *prefix) list_for_each(pos, head) { - item = list_entry(pos, struct hook, list); + struct hook *item = list_entry(pos, struct hook, list); - if (item) -- printf("%s: %s\n", -- config_scope_name(item->origin), -- item->command.buf); ++ item = list_entry(pos, struct hook, list); + if (item) { + /* Don't translate 'hookdir' - it matches the config */ -+ printf("%s: %s%s\n", + printf("%s: %s\n", +- config_scope_name(item->origin), + (item->from_hookdir + ? "hookdir" + : config_scope_name(item->origin)), -+ item->command.buf, -+ (item->from_hookdir -+ ? hookdir_annotation.buf -+ : "")); + item->command.buf); + } } @@ hook.c void free_hook(struct hook *ptr) { @@ hook.c: static void append_or_move_hook(struct list_head *head, const char *command) - to_add = xmalloc(sizeof(struct hook)); + to_add = xmalloc(sizeof(*to_add)); strbuf_init(&to_add->command, 0); strbuf_addstr(&to_add->command, command); + to_add->from_hookdir = 0; @@ hook.c: static void append_or_move_hook(struct list_head *head, const char *comm /* re-set the scope so we show where an override was specified */ @@ hook.c: 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 }; -+ const char *legacy_hook_path = NULL; - - INIT_LIST_HEAD(hook_head); -@@ hook.c: struct list_head* hook_list(const struct strbuf* hookname) + git_config(hook_config_lookup, &cb_data); - git_config(hook_config_lookup, (void*)&cb_data); - -+ if (have_git_dir()) -+ legacy_hook_path = find_hook(hookname->buf); ++ if (have_git_dir()) { ++ const char *legacy_hook_path = find_hook(hookname->buf); + -+ /* Unconditionally add legacy hook, but annotate it. */ -+ if (legacy_hook_path) { -+ struct hook *legacy_hook; ++ /* Unconditionally add legacy hook, but annotate it. */ ++ if (legacy_hook_path) { ++ struct hook *legacy_hook; + -+ append_or_move_hook(hook_head, absolute_path(legacy_hook_path)); -+ legacy_hook = list_entry(hook_head->prev, struct hook, list); -+ legacy_hook->from_hookdir = 1; ++ append_or_move_hook(hook_head, ++ absolute_path(legacy_hook_path)); ++ legacy_hook = list_entry(hook_head->prev, struct hook, ++ list); ++ legacy_hook->from_hookdir = 1; ++ } + } + strbuf_release(&hook_key); @@ hook.c: struct list_head* hook_list(const struct strbuf* hookname) } ## hook.h ## -@@ hook.h: struct hook +@@ hook.h: struct hook { enum config_scope origin; /* The literal command to run. */ struct strbuf command; -+ int from_hookdir; ++ unsigned from_hookdir : 1; }; /* 5: 723edcd785 ! 5: 96c0a4838f hook: respect hook.runHookDir @@ Metadata Author: Emily Shaffer <emilyshaffer@google.com> ## Commit message ## - hook: respect hook.runHookDir + hook: teach hook.runHookDir - Include hooks specified in the hook directory in the list of hooks to - run. These hooks do need to be treated differently from config-specified - ones - they do not need to run in a shell, and later on may be disabled - or warned about based on a config setting. - - Because they are at least as local as the local config, we'll run them - last - to keep the hook execution order from global to local. - - Signed-off-by: Emily Shaffer <emilyshaffer@google.com> + For now, just give a hint about how these hooks will be run in 'git hook + list'. Later on, though, we will pay attention to this enum when running + the hooks. ## Documentation/config/hook.txt ## @@ Documentation/config/hook.txt: hookcmd.<name>.command:: @@ builtin/hook.c: static const char * const builtin_hook_usage[] = { static int list(int argc, const char **argv, const char *prefix) { struct list_head *head, *pos; + struct strbuf hookname = STRBUF_INIT; ++ struct strbuf hookdir_annotation = STRBUF_INIT; + + struct option list_options[] = { + OPT_END(), @@ builtin/hook.c: static int list(int argc, const char **argv, const char *prefix) return 0; } @@ builtin/hook.c: static int list(int argc, const char **argv, const char *prefix) + case HOOKDIR_NO: + strbuf_addstr(&hookdir_annotation, _(" (will not run)")); + break; ++ case HOOKDIR_ERROR: ++ strbuf_addstr(&hookdir_annotation, _(" (will error and not run)")); ++ break; + case HOOKDIR_INTERACTIVE: + strbuf_addstr(&hookdir_annotation, _(" (will prompt)")); + break; + case HOOKDIR_WARN: -+ case HOOKDIR_UNKNOWN: -+ strbuf_addstr(&hookdir_annotation, _(" (will warn)")); ++ strbuf_addstr(&hookdir_annotation, _(" (will warn but run)")); + break; + case HOOKDIR_YES: + /* + * The default behavior should agree with -+ * hook.c:configured_hookdir_opt(). ++ * hook.c:configured_hookdir_opt(). HOOKDIR_UNKNOWN should just ++ * do the default behavior. + */ ++ case HOOKDIR_UNKNOWN: + default: + break; + } + list_for_each(pos, head) { + struct hook *item = list_entry(pos, struct hook, list); item = list_entry(pos, struct hook, list); if (item) { + /* Don't translate 'hookdir' - it matches the config */ +- printf("%s: %s\n", ++ printf("%s: %s%s\n", + (item->from_hookdir + ? "hookdir" + : config_scope_name(item->origin)), +- item->command.buf); ++ item->command.buf, ++ (item->from_hookdir ++ ? hookdir_annotation.buf ++ : "")); + } + } + + clear_hook_list(head); ++ strbuf_release(&hookdir_annotation); + strbuf_release(&hookname); + + return 0; @@ builtin/hook.c: static int list(int argc, const char **argv, const char *prefix) int cmd_hook(int argc, const char **argv, const char *prefix) @@ builtin/hook.c: static int list(int argc, const char **argv, const char *prefix) + if (run_hookdir) + if (!strcmp(run_hookdir, "no")) + should_run_hookdir = HOOKDIR_NO; ++ else if (!strcmp(run_hookdir, "error")) ++ should_run_hookdir = HOOKDIR_ERROR; + else if (!strcmp(run_hookdir, "yes")) + should_run_hookdir = HOOKDIR_YES; + else if (!strcmp(run_hookdir, "warn")) @@ hook.c: static int hook_config_lookup(const char *key, const char *value, void * + if (!strcmp(key, "no")) + return HOOKDIR_NO; + ++ if (!strcmp(key, "error")) ++ return HOOKDIR_ERROR; ++ + if (!strcmp(key, "yes")) + return HOOKDIR_YES; + @@ hook.c: static int hook_config_lookup(const char *key, const char *value, void * struct strbuf hook_key = STRBUF_INIT; ## hook.h ## -@@ hook.h: struct hook +@@ hook.h: struct hook { */ struct list_head* hook_list(const struct strbuf *hookname); +enum hookdir_opt +{ + HOOKDIR_NO, ++ HOOKDIR_ERROR, + HOOKDIR_WARN, + HOOKDIR_INTERACTIVE, + HOOKDIR_YES, @@ t/t1360-config-based-hooks.sh: test_expect_success 'git hook list shows hooks fr + test_i18ncmp expected actual +' + ++test_expect_success 'hook.runHookDir = error is respected by list' ' ++ setup_hookdir && ++ ++ test_config hook.runHookDir "error" && ++ ++ cat >expected <<-EOF && ++ hookdir: $(pwd)/.git/hooks/pre-commit (will error and not run) ++ EOF ++ ++ git hook list pre-commit >actual && ++ # the hookdir annotation is translated ++ test_i18ncmp expected actual ++' ++ +test_expect_success 'hook.runHookDir = warn is respected by list' ' + setup_hookdir && + + test_config hook.runHookDir "warn" && + + cat >expected <<-EOF && -+ hookdir: $(pwd)/.git/hooks/pre-commit (will warn) ++ hookdir: $(pwd)/.git/hooks/pre-commit (will warn but run) + EOF + + git hook list pre-commit >actual && @@ t/t1360-config-based-hooks.sh: test_expect_success 'git hook list shows hooks fr + # the hookdir annotation is translated + test_i18ncmp expected actual +' ++ ++test_expect_success 'hook.runHookDir is tolerant to unknown values' ' ++ setup_hookdir && ++ ++ test_config hook.runHookDir "junk" && ++ ++ cat >expected <<-EOF && ++ hookdir: $(pwd)/.git/hooks/pre-commit ++ EOF ++ ++ git hook list pre-commit >actual && ++ # the hookdir annotation is translated ++ test_i18ncmp expected actual ++' + test_done 6: 567f6d9d00 ! 6: 9068e11679 hook: implement hookcmd.<name>.skip @@ Commit message hook: implement hookcmd.<name>.skip If a user wants a specific repo to skip execution of a hook which is set - at a global or system level, they can now do so by specifying 'skip' in - their repo config: + at a global or system level, they will be able to do so by specifying + 'skip' in their repo config: ~/.gitconfig [hook.pre-commit] @@ Commit message Signed-off-by: Emily Shaffer <emilyshaffer@google.com> + ## Documentation/config/hook.txt ## +@@ Documentation/config/hook.txt: hookcmd.<name>.command:: + as a command. This can be an executable on your device or a oneliner for + your shell. See linkgit:git-hook[1]. + ++hookcmd.<name>.skip:: ++ Specify this boolean to remove a command from earlier in the execution ++ order. Useful if you want to make a single repo an exception to hook ++ configured at the system or global scope. If there is no hookcmd ++ specified for the command you want to skip, you can use the value of ++ `hook.<command>.command` as <name> as a shortcut. The "skip" setting ++ must be specified after the "hook.<command>.command" to have an effect. ++ + hook.runHookDir:: + Controls how hooks contained in your hookdir are executed. Can be any of + "yes", "warn", "interactive", or "no". Defaults to "yes". See + + ## Documentation/git-hook.txt ## +@@ Documentation/git-hook.txt: $ git hook list "prepare-commit-msg" + local: /bin/linter --c + ---- + ++If there is a command you wish to run in most cases but have one or two ++exceptional repos where it should be skipped, you can use specify ++`hookcmd.<name>.skip`, for example: ++ ++System config ++---- ++ [hook "pre-commit"] ++ command = check-for-secrets ++ ++ [hookcmd "check-for-secrets"] ++ command = /bin/secret-checker --aggressive ++---- ++ ++Local config ++---- ++ [hookcmd "check-for-secrets"] ++ skip = true ++ # This works for inlined hook commands, too: ++ [hookcmd "~/typocheck.sh"] ++ skip = true ++---- ++ ++After these configs are added, the hook list becomes: ++ ++---- ++$ git hook list "post-commit" ++global: /bin/linter --c ++local: python ~/run-test-suite.py ++ ++$ git hook list "pre-commit" ++no commands configured for hook 'pre-commit' ++---- ++ + COMMANDS + -------- + + ## hook.c ## @@ hook.c: void free_hook(struct hook *ptr) } } -static void append_or_move_hook(struct list_head *head, const char *command) -+static struct hook* find_hook_by_command(struct list_head *head, const char *command) ++static struct hook * find_hook_by_command(struct list_head *head, const char *command) { struct list_head *pos = NULL, *tmp = NULL; - struct hook *to_add = NULL; @@ hook.c: void free_hook(struct hook *ptr) - /* we'll simply move the hook to the end */ - to_add = it; + found = it; + break; } } + return found; @@ hook.c: void free_hook(struct hook *ptr) if (!to_add) { /* adding a new hook, not moving an old one */ -@@ hook.c: static void append_or_move_hook(struct list_head *head, const char *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, pos); -+ list_add_tail(&to_add->list, head); - } - - static void remove_hook(struct list_head *to_remove) @@ hook.c: static int hook_config_lookup(const char *key, const char *value, void *cb_data) if (!strcmp(key, hook_key)) { const char *command = value; @@ hook.c: static int hook_config_lookup(const char *key, const char *value, void * + strbuf_addf(&hookcmd_name, "hookcmd.%s.skip", command); + git_config_get_bool(hookcmd_name.buf, &skip); - /* Check if a hookcmd with that name exists. */ + /* + * 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_reset(&hookcmd_name); strbuf_addf(&hookcmd_name, "hookcmd.%s.command", command); git_config_get_value(hookcmd_name.buf, &command); @@ t/t1360-config-based-hooks.sh: test_expect_success 'hook.runHookDir = warn is re + test_i18ncmp expected actual +' + ++test_expect_success 'git hook list ignores skip referring to unused hookcmd' ' ++ test_config hookcmd.abc.command "/path/abc" --add && ++ test_config hookcmd.abc.skip "true" --add && ++ ++ cat >expected <<-EOF && ++ no commands configured for hook '\''pre-commit'\'' ++ EOF ++ ++ git hook list pre-commit >actual && ++ test_i18ncmp expected actual ++' ++ +test_expect_success 'git hook list removes skipped inlined hook' ' + setup_hooks && + test_config hookcmd."$ROOT/path/ghi".skip "true" --add && 7: a1c02b6758 ! 7: a2867ab8c0 parse-options: parse into strvec @@ Documentation/technical/api-parse-options.txt: There are some macros to easily d Use of `--no-option` will clear the list of preceding values. +`OPT_STRVEC(short, long, &struct strvec, arg_str, description)`:: -+ Introduce an option with a string argument. -+ The string argument is stored as an element in `strvec`. ++ Introduce an option with a string argument, meant to be specified ++ multiple times. ++ The string argument is stored as an element in `strvec`, and later ++ arguments are added to the same `strvec`. + Use of `--no-option` will clear the list of preceding values. + `OPT_INTEGER(short, long, &int_var, description)`:: 8: d865772ebc ! 8: 8848eeddf2 hook: add 'run' subcommand @@ Commit message supported. Legacy hooks (those present in $GITDIR/hooks) are run at the end of the - execution list. For now, there is no way to disable them. + execution list. They can be disabled, or made to print warnings, or to + prompt before running, with the 'hook.runHookDir' config. Users may wish to provide hook commands like 'git config hook.pre-commit.command "~/linter.sh --pre-commit"'. To enable this, @@ Documentation/git-hook.txt: in the order they should be run, and print the confi +run [(-e|--env)=<var>...] [(-a|--arg)=<arg>...] `<hook-name>`:: + +Runs hooks configured for `<hook-name>`, in the same order displayed by `git -+hook list`. Hooks configured this way are run prepended with `sh -c`, so paths -+containing special characters or spaces should be wrapped in single quotes: -+`command = '/my/path with spaces/script.sh' some args`. ++hook list`. Hooks configured this way may be run prepended with `sh -c`, so ++paths containing special characters or spaces should be wrapped in single ++quotes: `command = '/my/path with spaces/script.sh' some args`. + +OPTIONS +------- @@ builtin/hook.c: static int list(int argc, const char **argv, const char *prefix) +static int run(int argc, const char **argv, const char *prefix) +{ + struct strbuf hookname = STRBUF_INIT; -+ struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; ++ struct run_hooks_opt opt; + int rc = 0; + + struct option run_options[] = { @@ builtin/hook.c: static int list(int argc, const char **argv, const char *prefix) + OPT_END(), + }; + -+ /* -+ * While it makes sense to list hooks out-of-repo, it doesn't make sense -+ * to execute them. Hooks usually want to look at repository artifacts. -+ */ -+ if (!have_git_dir()) -+ usage_msg_opt(_("You must be in a Git repo to execute hooks."), -+ builtin_hook_usage, run_options); ++ run_hooks_opt_init(&opt); + + argc = parse_options(argc, argv, prefix, run_options, + builtin_hook_usage, 0); @@ hook.c: enum hookdir_opt configured_hookdir_opt(void) + + switch (cfg) + { ++ case HOOKDIR_ERROR: ++ fprintf(stderr, _("Skipping legacy hook at '%s'\n"), ++ path); ++ /* FALLTHROUGH */ + case HOOKDIR_NO: + return 0; -+ case HOOKDIR_UNKNOWN: -+ fprintf(stderr, -+ _("Unrecognized value for 'hook.runHookDir'. " -+ "Is there a typo? ")); -+ /* FALLTHROUGH */ + case HOOKDIR_WARN: + fprintf(stderr, _("Running legacy hook at '%s'\n"), + path); @@ hook.c: enum hookdir_opt configured_hookdir_opt(void) + } while (prompt.len); /* an empty reply means "Yes" */ + strbuf_release(&prompt); + return 1; ++ /* ++ * HOOKDIR_UNKNOWN should match the default behavior, but let's ++ * give a heads up to the user. ++ */ ++ case HOOKDIR_UNKNOWN: ++ fprintf(stderr, ++ _("Unrecognized value for 'hook.runHookDir'. " ++ "Is there a typo? ")); ++ /* FALLTHROUGH */ + case HOOKDIR_YES: + default: + return 1; @@ hook.c: struct list_head* hook_list(const struct strbuf* hookname) + strvec_clear(&o->args); +} + ++static void prepare_hook_cp(struct hook *hook, struct run_hooks_opt *options, ++ struct child_process *cp) ++{ ++ if (!hook) ++ return; ++ ++ cp->no_stdin = 1; ++ cp->env = options->env.v; ++ cp->stdout_to_stderr = 1; ++ cp->trace2_hook_name = hook->command.buf; ++ ++ /* ++ * Commands from the config could be oneliners, but we know ++ * for certain that hookdir commands are not. ++ */ ++ cp->use_shell = !hook->from_hookdir; ++ ++ /* add command */ ++ strvec_push(&cp->args, hook->command.buf); ++ ++ /* ++ * add passed-in argv, without expanding - let the user get back ++ * exactly what they put in ++ */ ++ strvec_pushv(&cp->args, options->args.v); ++} ++ +int run_hooks(const char *hookname, struct run_hooks_opt *options) +{ + struct strbuf hookname_str = STRBUF_INIT; @@ hook.c: struct list_head* hook_list(const struct strbuf* hookname) + struct child_process hook_proc = CHILD_PROCESS_INIT; + struct hook *hook = list_entry(pos, struct hook, list); + -+ hook_proc.env = options->env.v; -+ hook_proc.no_stdin = 1; -+ hook_proc.stdout_to_stderr = 1; -+ hook_proc.trace2_hook_name = hook->command.buf; -+ hook_proc.use_shell = 1; -+ -+ if (hook->from_hookdir) { -+ if (!should_include_hookdir(hook->command.buf, options->run_hookdir)) ++ if (hook->from_hookdir && ++ !should_include_hookdir(hook->command.buf, options->run_hookdir)) + continue; -+ /* -+ * Commands from the config could be oneliners, but we know -+ * for certain that hookdir commands are not. -+ */ -+ hook_proc.use_shell = 0; -+ } -+ -+ /* add command */ -+ strvec_push(&hook_proc.args, hook->command.buf); + -+ /* -+ * add passed-in argv, without expanding - let the user get back -+ * exactly what they put in -+ */ -+ strvec_pushv(&hook_proc.args, options->args.v); ++ prepare_hook_cp(hook, options, &hook_proc); + + rc |= run_command(&hook_proc); + } @@ hook.h #include "strbuf.h" +#include "strvec.h" - struct hook - { + struct hook { + struct list_head list; @@ hook.h: enum hookdir_opt */ enum hookdir_opt configured_hookdir_opt(void); @@ hook.h: enum hookdir_opt + enum hookdir_opt run_hookdir; +}; + -+#define RUN_HOOKS_OPT_INIT { \ -+ .env = STRVEC_INIT, \ -+ .args = STRVEC_INIT, \ -+ .run_hookdir = configured_hookdir_opt() \ -+} -+ +void run_hooks_opt_init(struct run_hooks_opt *o); +void run_hooks_opt_clear(struct run_hooks_opt *o); + @@ t/t1360-config-based-hooks.sh: test_expect_success 'hook.runHookDir = no is resp + test_must_be_empty actual ' - test_expect_success 'hook.runHookDir = warn is respected by list' ' + test_expect_success 'hook.runHookDir = error is respected by list' ' +@@ t/t1360-config-based-hooks.sh: test_expect_success 'hook.runHookDir = error is respected by list' ' + + git hook list pre-commit >actual && + # the hookdir annotation is translated ++ test_i18ncmp expected actual && ++ ++ cat >expected <<-EOF && ++ Skipping legacy hook at '\''$(pwd)/.git/hooks/pre-commit'\'' ++ EOF ++ ++ git hook run pre-commit 2>actual && + test_i18ncmp expected actual + ' + @@ t/t1360-config-based-hooks.sh: test_expect_success 'hook.runHookDir = warn is respected by list' ' git hook list pre-commit >actual && @@ t/t1360-config-based-hooks.sh: test_expect_success 'hook.runHookDir = interactiv + nongit test_must_fail git hook run pre-commit ' - test_done + test_expect_success 'hook.runHookDir is tolerant to unknown values' ' 9: 53a655ed2c ! 9: 452f7eea89 hook: replace find_hook() with hook_exists() @@ Metadata Author: Emily Shaffer <emilyshaffer@google.com> ## Commit message ## - hook: replace find_hook() with hook_exists() + hook: introduce hook_exists() Add a helper to easily determine whether any hooks exist for a given hook event. @@ Commit message hook; that check should include the config-based hooks as well. Optimize by checking the config directly. Since commands which execute hooks might want to take args to replace 'hook.runHookDir', let - 'hook_exists()' mirror the behavior of 'hook.runHookDir'. + 'hook_exists()' take a hookdir_opt to override that config. - Signed-off-by: Emily Shaffer <emilyshaffer@google.com> + In some cases, external callers today use find_hook() to discover the + location of a hook and then run it manually with run-command.h (that is, + not with run_hook_le()). Later, those cases will call hook.h:run_hook() + directly instead. - ## builtin/bugreport.c ## -@@ - #include "strbuf.h" - #include "help.h" - #include "compat/compiler.h" --#include "run-command.h" -+#include "hook.h" - - - static void get_system_info(struct strbuf *sys_info) -@@ builtin/bugreport.c: static void get_populated_hooks(struct strbuf *hook_info, int nongit) - } - - for (i = 0; i < ARRAY_SIZE(hook); i++) -- if (find_hook(hook[i])) -+ if (hook_exists(hook[i], configured_hookdir_opt())) - strbuf_addf(hook_info, "%s\n", hook[i]); - } - + Once the entire codebase is using hook_exists() instead of find_hook(), + find_hook() can be safely rolled into hook_exists() and removed from + run-command.h. + + Signed-off-by: Emily Shaffer <emilyshaffer@google.com> ## hook.c ## @@ hook.c: void run_hooks_opt_init(struct run_hooks_opt *o) @@ hook.c: void run_hooks_opt_init(struct run_hooks_opt *o) +{ + const char *value = NULL; /* throwaway */ + struct strbuf hook_key = STRBUF_INIT; ++ int could_run_hookdir; ++ ++ if (should_run_hookdir == HOOKDIR_USE_CONFIG) ++ should_run_hookdir = configured_hookdir_opt(); + -+ int could_run_hookdir = (should_run_hookdir == HOOKDIR_INTERACTIVE || ++ could_run_hookdir = (should_run_hookdir == HOOKDIR_INTERACTIVE || + should_run_hookdir == HOOKDIR_WARN || + should_run_hookdir == HOOKDIR_YES) + && !!find_hook(hookname); @@ hook.c: void run_hooks_opt_init(struct run_hooks_opt *o) strvec_clear(&o->env); ## hook.h ## +@@ hook.h: struct list_head* hook_list(const struct strbuf *hookname); + + enum hookdir_opt + { ++ HOOKDIR_USE_CONFIG, + HOOKDIR_NO, + HOOKDIR_ERROR, + HOOKDIR_WARN, @@ hook.h: struct run_hooks_opt void run_hooks_opt_init(struct run_hooks_opt *o); void run_hooks_opt_clear(struct run_hooks_opt *o); 10: 13abc6ce24 ! 10: e76507b290 hook: support passing stdin to hooks @@ Documentation/git-hook.txt: in the order they should be run, and print the confi +run [(-e|--env)=<var>...] [(-a|--arg)=<arg>...] [--to-stdin=<path>] `<hook-name>`:: Runs hooks configured for `<hook-name>`, in the same order displayed by `git - hook list`. Hooks configured this way are run prepended with `sh -c`, so paths + hook list`. Hooks configured this way may be run prepended with `sh -c`, so @@ Documentation/git-hook.txt: Specify arguments to pass to every hook that is run. + Specify environment variables to set for every hook that is run. @@ builtin/hook.c: static int run(int argc, const char **argv, const char *prefix) ## hook.c ## -@@ hook.c: int run_hooks(const char *hookname, struct run_hooks_opt *options) - struct child_process hook_proc = CHILD_PROCESS_INIT; - struct hook *hook = list_entry(pos, struct hook, list); +@@ hook.c: void run_hooks_opt_init(struct run_hooks_opt *o) + { + strvec_init(&o->env); + strvec_init(&o->args); ++ o->path_to_stdin = NULL; + o->run_hookdir = configured_hookdir_opt(); + } + +@@ hook.c: static void prepare_hook_cp(struct hook *hook, struct run_hooks_opt *options, + if (!hook) + return; -+ /* reopen the file for stdin; run_command closes it. */ -+ if (options->path_to_stdin) -+ hook_proc.in = xopen(options->path_to_stdin, O_RDONLY); -+ else -+ hook_proc.no_stdin = 1; +- cp->no_stdin = 1; ++ /* reopen the file for stdin; run_command closes it. */ ++ if (options->path_to_stdin) ++ cp->in = xopen(options->path_to_stdin, O_RDONLY); ++ else ++ cp->no_stdin = 1; + - hook_proc.env = options->env.v; -- hook_proc.no_stdin = 1; - hook_proc.stdout_to_stderr = 1; - hook_proc.trace2_hook_name = hook->command.buf; - hook_proc.use_shell = 1; + cp->env = options->env.v; + cp->stdout_to_stderr = 1; + cp->trace2_hook_name = hook->command.buf; ## hook.h ## @@ hook.h: struct run_hooks_opt @@ hook.h: struct run_hooks_opt + const char *path_to_stdin; }; - #define RUN_HOOKS_OPT_INIT { \ -- .env = STRVEC_INIT, \ -+ .env = STRVEC_INIT, \ - .args = STRVEC_INIT, \ -+ .path_to_stdin = NULL, \ - .run_hookdir = configured_hookdir_opt() \ - } - + void run_hooks_opt_init(struct run_hooks_opt *o); @@ hook.h: int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir); /* @@ hook.h: int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdi ## t/t1360-config-based-hooks.sh ## -@@ t/t1360-config-based-hooks.sh: test_expect_success 'out-of-repo runs excluded' ' - nongit test_must_fail git hook run pre-commit +@@ t/t1360-config-based-hooks.sh: test_expect_success 'hook.runHookDir is tolerant to unknown values' ' + test_i18ncmp expected actual ' +test_expect_success 'stdin to multiple hooks' ' 11: 0465a9ec94 ! 11: 5f41555e49 run-command: allow stdin for run_processes_parallel @@ run-command.c: static int pp_start_one(struct parallel_processes *pp) if (i == pp->max_processes) BUG("bookkeeping is hard"); -+ /* disallow by default, but allow users to set up stdin if they wish */ ++ /* ++ * By default, do not inherit stdin from the parent process - otherwise, ++ * all children would share stdin! Users may overwrite this to provide ++ * something to the child's stdin by having their 'get_next_task' ++ * callback assign 0 to .no_stdin and an appropriate integer to .in. ++ */ + pp->children[i].process.no_stdin = 1; + code = pp->get_next_task(&pp->children[i].process, 12: 83eb7805a4 ! 12: a3bf826304 hook: allow parallel hook execution @@ Documentation/git-hook.txt: in the order they should be run, and print the confi +run [(-e|--env)=<var>...] [(-a|--arg)=<arg>...] [--to-stdin=<path>] [(-j|--jobs)<n>] `<hook-name>`:: Runs hooks configured for `<hook-name>`, in the same order displayed by `git - hook list`. Hooks configured this way are run prepended with `sh -c`, so paths + hook list`. Hooks configured this way may be run prepended with `sh -c`, so @@ Documentation/git-hook.txt: Specify environment variables to set for every hook that is run. Specify a file which will be streamed into stdin for every hook that is run. Each hook will receive the entire file from beginning to EOF. @@ builtin/hook.c NULL }; -@@ builtin/hook.c: static int list(int argc, const char **argv, const char *prefix) - static int run(int argc, const char **argv, const char *prefix) - { - struct strbuf hookname = STRBUF_INIT; -- struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; -+ struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC; - int rc = 0; - - struct option run_options[] = { @@ builtin/hook.c: static int run(int argc, const char **argv, const char *prefix) N_("argument to pass to hook")), OPT_STRING(0, "to-stdin", &opt.path_to_stdin, N_("path"), @@ builtin/hook.c: static int run(int argc, const char **argv, const char *prefix) OPT_END(), }; +- run_hooks_opt_init(&opt); ++ run_hooks_opt_init_async(&opt); + + argc = parse_options(argc, argv, prefix, run_options, + builtin_hook_usage, 0); ## hook.c ## @@ hook.c: enum hookdir_opt configured_hookdir_opt(void) @@ hook.c: enum hookdir_opt configured_hookdir_opt(void) static int should_include_hookdir(const char *path, enum hookdir_opt cfg) { struct strbuf prompt = STRBUF_INIT; -@@ hook.c: void run_hooks_opt_init(struct run_hooks_opt *o) +@@ hook.c: struct list_head* hook_list(const struct strbuf* hookname) + return hook_head; + } + +-void run_hooks_opt_init(struct run_hooks_opt *o) ++void run_hooks_opt_init_sync(struct run_hooks_opt *o) + { strvec_init(&o->env); strvec_init(&o->args); + o->path_to_stdin = NULL; o->run_hookdir = configured_hookdir_opt(); ++ o->jobs = 1; ++} ++ ++void run_hooks_opt_init_async(struct run_hooks_opt *o) ++{ ++ run_hooks_opt_init_sync(o); + o->jobs = configured_hook_jobs(); } @@ hook.c: void run_hooks_opt_clear(struct run_hooks_opt *o) strvec_clear(&o->args); } -+ +-static void prepare_hook_cp(struct hook *hook, struct run_hooks_opt *options, +- struct child_process *cp) +static int pick_next_hook(struct child_process *cp, + struct strbuf *out, + void *pp_cb, + void **pp_task_cb) -+{ + { + struct hook_cb_data *hook_cb = pp_cb; ++ struct hook *hook = hook_cb->run_me; + -+ struct hook *hook = list_entry(hook_cb->run_me, struct hook, list); -+ -+ if (hook_cb->head == hook_cb->run_me) + if (!hook) +- return; + return 0; -+ -+ cp->env = hook_cb->options->env.v; -+ cp->stdout_to_stderr = 1; -+ cp->trace2_hook_name = hook->command.buf; -+ -+ /* reopen the file for stdin; run_command closes it. */ + + /* reopen the file for stdin; run_command closes it. */ +- if (options->path_to_stdin) +- cp->in = xopen(options->path_to_stdin, O_RDONLY); +- else + if (hook_cb->options->path_to_stdin) { + cp->no_stdin = 0; + cp->in = xopen(hook_cb->options->path_to_stdin, O_RDONLY); + } else { -+ cp->no_stdin = 1; + cp->no_stdin = 1; + } -+ -+ /* -+ * Commands from the config could be oneliners, but we know -+ * for certain that hookdir commands are not. -+ */ -+ if (hook->from_hookdir) -+ cp->use_shell = 0; -+ else -+ cp->use_shell = 1; -+ -+ /* add command */ -+ strvec_push(&cp->args, hook->command.buf); -+ -+ /* -+ * add passed-in argv, without expanding - let the user get back -+ * exactly what they put in -+ */ + +- cp->env = options->env.v; ++ cp->env = hook_cb->options->env.v; + cp->stdout_to_stderr = 1; + cp->trace2_hook_name = hook->command.buf; + +@@ hook.c: static void prepare_hook_cp(struct hook *hook, struct run_hooks_opt *options, + * add passed-in argv, without expanding - let the user get back + * exactly what they put in + */ +- strvec_pushv(&cp->args, options->args.v); + strvec_pushv(&cp->args, hook_cb->options->args.v); + + /* Provide context for errors if necessary */ + *pp_task_cb = hook; + + /* Get the next entry ready */ -+ hook_cb->run_me = hook_cb->run_me->next; ++ if (hook_cb->run_me->list.next == hook_cb->head) ++ hook_cb->run_me = NULL; ++ else ++ hook_cb->run_me = list_entry(hook_cb->run_me->list.next, ++ struct hook, list); + + return 1; +} @@ hook.c: void run_hooks_opt_clear(struct run_hooks_opt *o) + strbuf_addf(out, _("Couldn't start '%s', configured in '%s'\n"), + attempted->command.buf, + attempted->from_hookdir ? "hookdir" -+ : config_scope_name(attempted->origin)); ++ : config_scope_name(attempted->origin)); + + /* NEEDSWORK: if halt_on_error is desired, do it here. */ + return 0; @@ hook.c: void run_hooks_opt_clear(struct run_hooks_opt *o) + + /* NEEDSWORK: if halt_on_error is desired, do it here. */ + return 0; -+} -+ + } + int run_hooks(const char *hookname, struct run_hooks_opt *options) { struct strbuf hookname_str = STRBUF_INIT; @@ hook.c: int run_hooks(const char *hookname, struct run_hooks_opt *options) - struct child_process hook_proc = CHILD_PROCESS_INIT; struct hook *hook = list_entry(pos, struct hook, list); -- /* reopen the file for stdin; run_command closes it. */ -- if (options->path_to_stdin) -- hook_proc.in = xopen(options->path_to_stdin, O_RDONLY); -- else -- hook_proc.no_stdin = 1; -- -- hook_proc.env = options->env.v; -- hook_proc.stdout_to_stderr = 1; -- hook_proc.trace2_hook_name = hook->command.buf; -- hook_proc.use_shell = 1; -- -- if (hook->from_hookdir) { -- if (!should_include_hookdir(hook->command.buf, options->run_hookdir)) + if (hook->from_hookdir && + !should_include_hookdir(hook->command.buf, options->run_hookdir)) - continue; -- /* -- * Commands from the config could be oneliners, but we know -- * for certain that hookdir commands are not. -- */ -- hook_proc.use_shell = 0; -- } -- -- /* add command */ -- strvec_push(&hook_proc.args, hook->command.buf); -+ if (hook->from_hookdir && -+ !should_include_hookdir(hook->command.buf, options->run_hookdir)) + list_del(pos); + } ++ ++ if (list_empty(to_run)) ++ return 0; -- /* -- * add passed-in argv, without expanding - let the user get back -- * exactly what they put in -- */ -- strvec_pushv(&hook_proc.args, options->args.v); +- prepare_hook_cp(hook, options, &hook_proc); + cb_data.head = to_run; -+ cb_data.run_me = to_run->next; ++ cb_data.run_me = list_entry(to_run->next, struct hook, list); - rc |= run_command(&hook_proc); - } @@ hook.h: enum hookdir_opt /* Environment vars to be set for each hook */ @@ hook.h: struct run_hooks_opt + /* + * How should the hookdir be handled? +- * Leave the RUN_HOOKS_OPT_INIT default in most cases; this only needs ++ * Leave the run_hooks_opt_init_*() default in most cases; this only needs + * to be overridden if the user can override it at the command line. + */ + enum hookdir_opt run_hookdir; + /* Path to file which should be piped to stdin for each hook */ const char *path_to_stdin; + + /* Number of threads to parallelize across */ + int jobs; - }; - --#define RUN_HOOKS_OPT_INIT { \ ++}; ++ +/* + * Callback provided to feed_pipe_fn and consume_sideband_fn. + */ +struct hook_cb_data { + int rc; + struct list_head *head; -+ struct list_head *run_me; ++ struct hook *run_me; + struct run_hooks_opt *options; -+}; -+ -+#define RUN_HOOKS_OPT_INIT_SYNC { \ - .env = STRVEC_INIT, \ - .args = STRVEC_INIT, \ - .path_to_stdin = NULL, \ -+ .jobs = 1, \ - .run_hookdir = configured_hookdir_opt() \ - } + }; -+#define RUN_HOOKS_OPT_INIT_ASYNC { \ -+ .env = STRVEC_INIT, \ -+ .args = STRVEC_INIT, \ -+ .path_to_stdin = NULL, \ -+ .jobs = configured_hook_jobs(), \ -+ .run_hookdir = configured_hookdir_opt() \ -+} -+ -+ - void run_hooks_opt_init(struct run_hooks_opt *o); +-void run_hooks_opt_init(struct run_hooks_opt *o); ++void run_hooks_opt_init_sync(struct run_hooks_opt *o); ++void run_hooks_opt_init_async(struct run_hooks_opt *o); void run_hooks_opt_clear(struct run_hooks_opt *o); + /* 13: f84c879d5a < -: ---------- hook: allow specifying working directory for hooks -: ---------- > 13: 0c4add98a4 hook: allow specifying working directory for hooks 14: ac9cec6587 = 14: 1847c4c675 run-command: add stdin callback for parallelization 15: 71fca28ccf ! 15: ab781c94d7 hook: provide stdin by string_list or callback @@ hook.c: static void append_or_move_hook(struct list_head *head, const char *comm } /* re-set the scope so we show where an override was specified */ +@@ hook.c: void run_hooks_opt_init_sync(struct run_hooks_opt *o) + o->run_hookdir = configured_hookdir_opt(); + o->jobs = 1; + o->dir = NULL; ++ o->feed_pipe = NULL; ++ o->feed_pipe_ctx = NULL; + } + + void run_hooks_opt_init_async(struct run_hooks_opt *o) @@ hook.c: void run_hooks_opt_clear(struct run_hooks_opt *o) - { - strvec_clear(&o->env); strvec_clear(&o->args); -+ string_list_clear(&o->str_stdin, 0); } - -+static int pipe_from_string_list(struct strbuf *pipe, void *pp_cb, void *pp_task_cb) ++int pipe_from_string_list(struct strbuf *pipe, void *pp_cb, void *pp_task_cb) +{ + int *item_idx; + struct hook *ctx = pp_task_cb; -+ struct string_list *to_pipe = &((struct hook_cb_data*)pp_cb)->options->str_stdin; ++ struct string_list *to_pipe = ((struct hook_cb_data*)pp_cb)->options->feed_pipe_ctx; + + /* Bootstrap the state manager if necessary. */ + if (!ctx->feed_pipe_cb_data) { @@ hook.c: int run_hooks(const char *hookname, struct run_hooks_opt *options) if (!options) BUG("a struct run_hooks_opt must be provided to run_hooks"); -+ if ((options->path_to_stdin && options->str_stdin.nr) || -+ (options->path_to_stdin && options->feed_pipe) || -+ (options->str_stdin.nr && options->feed_pipe)) ++ if (options->path_to_stdin && options->feed_pipe) + BUG("choose only one method to populate stdin"); -+ -+ if (options->str_stdin.nr) -+ options->feed_pipe = &pipe_from_string_list; + strbuf_addstr(&hookname_str, hookname); @@ hook.h #include "strvec.h" +#include "run-command.h" - struct hook - { -@@ hook.h: struct hook + struct hook { + struct list_head list; +@@ hook.h: struct hook { /* The literal command to run. */ struct strbuf command; - int from_hookdir; + unsigned from_hookdir : 1; + + /* + * Use this to keep state for your feed_pipe_fn if you are using @@ hook.h: struct run_hooks_opt /* Path to file which should be piped to stdin for each hook */ const char *path_to_stdin; -+ /* Pipe each string to stdin, separated by newlines */ -+ struct string_list str_stdin; + /* + * Callback and state pointer to ask for more content to pipe to stdin. + * Will be called repeatedly, for each hook. See + * hook.c:pipe_from_stdin() for an example. Keep per-hook state in + * hook.feed_pipe_cb_data (per process). Keep initialization context in + * feed_pipe_ctx (shared by all processes). ++ * ++ * See 'pipe_from_string_list()' for info about how to specify a ++ * string_list as the stdin input instead of writing your own handler. + */ + feed_pipe_fn feed_pipe; + void *feed_pipe_ctx; @@ hook.h: struct run_hooks_opt + }; ++/* ++ * To specify a 'struct string_list', set 'run_hooks_opt.feed_pipe_ctx' to the ++ * string_list and set 'run_hooks_opt.feed_pipe' to 'pipe_from_string_list()'. ++ * This will pipe each string in the list to stdin, separated by newlines. (Do ++ * not inject your own newlines.) ++ */ ++int pipe_from_string_list(struct strbuf *pipe, void *pp_cb, void *pp_task_cb); ++ /* -@@ hook.h: struct hook_cb_data { - .path_to_stdin = NULL, \ - .jobs = 1, \ - .dir = NULL, \ -+ .str_stdin = STRING_LIST_INIT_DUP, \ -+ .feed_pipe = NULL, \ -+ .feed_pipe_ctx = NULL, \ - .run_hookdir = configured_hookdir_opt() \ - } - -@@ hook.h: struct hook_cb_data { - .path_to_stdin = NULL, \ - .jobs = configured_hook_jobs(), \ - .dir = NULL, \ -+ .str_stdin = STRING_LIST_INIT_DUP, \ -+ .feed_pipe = NULL, \ -+ .feed_pipe_ctx = NULL, \ - .run_hookdir = configured_hookdir_opt() \ - } - + * Callback provided to feed_pipe_fn and consume_sideband_fn. + */ 16: 98253fa8fd = 16: c51bf46e8d run-command: allow capturing of collated output 17: 9505812b74 ! 17: b90a4ee79b hooks: allow callers to capture output @@ Commit message Signed-off-by: Emily Shaffer <emilyshaffer@google.com> ## hook.c ## +@@ hook.c: void run_hooks_opt_init_sync(struct run_hooks_opt *o) + o->dir = NULL; + o->feed_pipe = NULL; + o->feed_pipe_ctx = NULL; ++ o->consume_sideband = NULL; + } + + void run_hooks_opt_init_async(struct run_hooks_opt *o) @@ hook.c: int run_hooks(const char *hookname, struct run_hooks_opt *options) pick_next_hook, notify_start_failure, @@ hook.h: struct run_hooks_opt /* Number of threads to parallelize across */ int jobs; -@@ hook.h: struct hook_cb_data { - .str_stdin = STRING_LIST_INIT_DUP, \ - .feed_pipe = NULL, \ - .feed_pipe_ctx = NULL, \ -+ .consume_sideband = NULL, \ - .run_hookdir = configured_hookdir_opt() \ - } - -@@ hook.h: struct hook_cb_data { - .str_stdin = STRING_LIST_INIT_DUP, \ - .feed_pipe = NULL, \ - .feed_pipe_ctx = NULL, \ -+ .consume_sideband = NULL, \ - .run_hookdir = configured_hookdir_opt() \ - } -
On Fri, Mar 12 2021, Ævar Arnfjörð Bjarmason wrote: A small correction to one of my comments: > On Thu, Mar 11 2021, Emily Shaffer wrote: > 2. You're sticking full paths in the git config key, which is > case-insensitive, and a feature of this format is being able to > configure/override previously configured hooks. > > So the behavior of this feature depends on git's interaction with > the case-sensitivity of filesystems, and not just one fs, any fs > we're walking in our various config sources, and where the hook > itself lives. > > As recent CVEs have shown that's a big can of worms, particularly > for something whose goal is to address the security aspect of > running hooks from other config. > > Arguably the case-sensitivity issue is just confusing since we > canonicalize it anyway. But once you add in FS path canonicalization > it becomes a real big can of worms. See the .gitmodules fsck code. > > Even if it wasn't for that it's relatively nastier to edit/maintain > full paths and the appropriate escaping in the double-quoted key in > the config file v.s. having it as an optionally quoted value. So the "case-insensitive" part of that *mostly* doesn't apply. I'd forgotten that we don't consider the "LeVeL" part of "ThReE.LeVeL.KeY" to be case-insensitive, but the other two components are, as discussed in git-config(1)'s docs. I say "mostly" because that's tolower()'s idea of case normalization, which may or may not match the FS's, but anyway, I think that's probably splitting hairs, but I worry more about the path normalization aspect noted in the last two paragraphs there. > 3. We're left with this "*.command = cmd", and "*.skip = true" > special-case syntax. I can't see any reason for why it's needed over > simply having "*.command = true" clobber earlier hooks as noted in > the proposed docs above. > > And that doesn't require any magic to support, just like our > existing "core.pager=cat" case. > > I mean, I suppose it's magical in that we might otherwise error on > non-consumed stdin (do we?), anyway, documenting it as a synonym for > "cat >/dev/null" would get around that :) > > 4. It makes the common case of having the same hooks for N commands > needlessly verbose, if you can just support "type" (or whatever we > should call it) you can add that N times... > > 5. At the end of this series we're left with the start of the docs > saying: > > You can list and run configured hooks with this command. Later, > you will be able to add and modify hooks with this command. > > But those patches have yet to land, and looking at the design > document I'm skeptical of that being a good addition v.s. just > adding the same thing to "git config". > > As just one exmaple; surely "git config edit <name>" would need to > run around and find config files to edit, then open them in a loop > for you, no? > > Which we'd eventually want for "git config" in general with an > --edit-regexp option or whatever, which brings us (well, at least > me) back to "then let's just add it to git-config?". > > 6. The whole 'git hook' config special-casing doesn't help other > commands or the security issue that seemed to have prompted (at > least some of) its existence > > In the design doc we mention the "core.pager = rm -rf /" case for a > .git/config. > > This series doesn't implement, but the design docs note a future > want for solving that issue for the hooks. > > To me that's another case where we should just have general config > syntax, not something hook-specific, e.g. if I could do this in my > ~/.gitconfig: > > ;; We consider 'config.ignore' in reverse order, so e.g setting > ;; it in. ~/.gitconfig will ignore any such keys for repo-level > ;; config > [config "ignore"] > key = core.pager > keyRegexp = "^hook\." > > We'd address both any hook security concerns, as well as core.pager > etc. We could then just have e.g. some syntax sugar of: > > [include] > path = built-in://gimme-safe-config > > Which would just be a thin layer of magit to include > <path-to-git-prefix>/config-templates/gimme-safe-config or whatever. > > We'd thus address the issue for all config types without > hook-specific magic. > > Anyway. I'm very willing to be convinced otherwise. I just think that > for a first-draft implementation leaving aside 'hook.<command>.command' > and the whole 'list' thing makes sense. > > We can consider the core code changes relatively separately from any > future aspirations, particularly with a 40-some patch series, and the > end-state of *this series* IMO not really justifying, that part of the > implementation, and thus requiring reviewers to look ahead beyond the > 40-some patches. Emily: *Bump* on being interesed in what you think about the rest of this though.