Re: [PATCH v8 00/37] config-based hooks

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.