git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH v2 0/4] propose config-based hooks
@ 2020-05-21 18:54 Emily Shaffer
  2020-05-21 18:54 ` [PATCH v2 1/4] doc: propose hooks managed by the config Emily Shaffer
                   ` (4 more replies)
  0 siblings, 5 replies; 22+ messages in thread
From: Emily Shaffer @ 2020-05-21 18:54 UTC (permalink / raw)
  To: git
  Cc: Emily Shaffer, Jeff King, Junio C Hamano, James Ramsay,
	Jonathan Nieder, brian m. carlson,
	Ævar Arnfjörð Bjarmason, Phillip Wood,
	Josh Steadmon

This series implements "Stage 1" of the config-based hooks rollout
process as proposed in the design doc. It does not touch the existing
hook implementation or change the way that Git functions - it only adds
a new, independent command.

In the design doc, I mentioned the possibility of including 'git hook
add' and 'git hook edit' in this stage. However, I'd like to get input
from our UX team internally before I get started - I know my own limits,
and coming up with good UX design is one of them ;) Unfortunately, I
won't be able to get time with them until the first week of June, so I
haven't included those commands here.

The series is listed as v2 because I included the updated design doc
with changes pointed out by Junio and brian. That's a good place to
start if you're reviewing the series for the first time. (I'm also
breaking thread with the contributor summit notes to bring the series to
the attention of more contributors who may be interested.)

One point I'd like discussion on especially is the '--porcelain'
command. The intent was to make it very easy for non-builtins to run
hooks; but I'm starting to wonder whether it makes more sense to include
a `git hook run <hookname>`, which makes parallelization possible in the
future if we decide to implement that. Even if we decide it makes sense
to keep 'list --porcelain', I'm not sure what information to include;
providing simply the line to pass to 'sh' seems a little thin.

The next stage from here is to migrate internal callers who use
'find_hook()' now to call the hook library (and teach the hook library
to call find_hook()), which will essentially turn on config-based hooks;
does it make sense to include that stage at the same time as this
series so we aren't checking in unused code?

Thanks all.
 - Emily

Emily Shaffer (4):
  doc: propose hooks managed by the config
  hook: scaffolding for git-hook subcommand
  hook: add list command
  hook: add --porcelain to list command

 .gitignore                                    |   1 +
 Documentation/Makefile                        |   1 +
 Documentation/git-hook.txt                    |  63 ++++
 .../technical/config-based-hooks.txt          | 320 ++++++++++++++++++
 Makefile                                      |   2 +
 builtin.h                                     |   1 +
 builtin/hook.c                                |  77 +++++
 git.c                                         |   1 +
 hook.c                                        |  90 +++++
 hook.h                                        |  15 +
 t/t1360-config-based-hooks.sh                 |  69 ++++
 11 files changed, 640 insertions(+)
 create mode 100644 Documentation/git-hook.txt
 create mode 100644 Documentation/technical/config-based-hooks.txt
 create mode 100644 builtin/hook.c
 create mode 100644 hook.c
 create mode 100644 hook.h
 create mode 100755 t/t1360-config-based-hooks.sh

-- 
2.27.0.rc0.183.gde8f92d652-goog


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v2 1/4] doc: propose hooks managed by the config
  2020-05-21 18:54 [PATCH v2 0/4] propose config-based hooks Emily Shaffer
@ 2020-05-21 18:54 ` Emily Shaffer
  2020-05-22 10:13   ` Phillip Wood
  2020-05-21 18:54 ` [PATCH v2 2/4] hook: scaffolding for git-hook subcommand Emily Shaffer
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 22+ messages in thread
From: Emily Shaffer @ 2020-05-21 18:54 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Begin a design document for config-based hooks, managed via git-hook.
Focus on an overview of the implementation and motivation for design
decisions. Briefly discuss the alternatives considered before this
point. Also, attempt to redefine terms to fit into a multihook world.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/Makefile                        |   1 +
 .../technical/config-based-hooks.txt          | 320 ++++++++++++++++++
 2 files changed, 321 insertions(+)
 create mode 100644 Documentation/technical/config-based-hooks.txt

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 15d9d04f31..5b21f31d31 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -80,6 +80,7 @@ SP_ARTICLES += $(API_DOCS)
 TECH_DOCS += MyFirstContribution
 TECH_DOCS += MyFirstObjectWalk
 TECH_DOCS += SubmittingPatches
+TECH_DOCS += technical/config-based-hooks
 TECH_DOCS += technical/hash-function-transition
 TECH_DOCS += technical/http-protocol
 TECH_DOCS += technical/index-format
diff --git a/Documentation/technical/config-based-hooks.txt b/Documentation/technical/config-based-hooks.txt
new file mode 100644
index 0000000000..59cdc25a47
--- /dev/null
+++ b/Documentation/technical/config-based-hooks.txt
@@ -0,0 +1,320 @@
+Configuration-based hook management
+===================================
+
+== Motivation
+
+Treat hooks as a first-class citizen by replacing the .git/hook/hookname path as
+the only source of hooks to execute, in a way which is friendly to users with
+multiple repos which have similar needs.
+
+Redefine "hook" as an event rather than a single script, allowing users to
+perform unrelated actions on a single event.
+
+Take a step closer to safety when copying zipped Git repositories from untrusted
+users.
+
+Make it easier for users to discover Git's hook feature and automate their
+workflows.
+
+== User interfaces
+
+=== Config schema
+
+Hooks can be introduced by editing the configuration manually. There are two new
+sections added, `hook` and `hookcmd`.
+
+==== `hook`
+
+Primarily contains subsections for each hook event. These subsections define
+hook command execution order; hook commands can be specified by passing the
+command directly if no additional configuration is needed, or by passing the
+name of a `hookcmd`. If Git does not find a `hookcmd` whose subsection matches
+the value of the given command string, Git will try to execute the string
+directly. Hooks are executed by passing the resolved command string to the
+shell. Hook event subsections can also contain per-hook-event settings.
+
+Also contains top-level hook execution settings, for example,
+`hook.warnHookDir`, `hook.runHookDir`, or `hook.disableAll`.
+
+----
+[hook "pre-commit"]
+  command = perl-linter
+  command = /usr/bin/git-secrets --pre-commit
+
+[hook "pre-applypatch"]
+  command = perl-linter
+  error = ignore
+
+[hook]
+  runHookDir = interactive
+----
+
+==== `hookcmd`
+
+Defines a hook command and its attributes, which will be used when a hook event
+occurs. Unqualified attributes are assumed to apply to this hook during all hook
+events, but event-specific attributes can also be supplied. The example runs
+`/usr/bin/lint-it --language=perl <args passed by Git>`, but for repos which
+include this config, the hook command will be skipped for all events to which
+it's normally subscribed _except_ `pre-commit`.
+
+----
+[hookcmd "perl-linter"]
+  command = /usr/bin/lint-it --language=perl
+  skip = true
+  pre-commit-skip = false
+----
+
+=== Command-line API
+
+Users should be able to view, reorder, and create hook commands via the command
+line. External tools should be able to view a list of hooks in the correct order
+to run.
+
+*`git hook list <hook-event>`*
+
+*`git hook list (--system|--global|--local|--worktree)`*
+
+*`git hook edit <hook-event>`*
+
+*`git hook add <hook-command> <hook-event> <options...>`*
+
+=== Hook editor
+
+The tool which is presented by `git hook edit <hook-command>`. Ideally, this
+tool should be easier to use than manually editing the config, and then produce
+a concise config afterwards. It may take a form similar to `git rebase
+--interactive`.
+
+== Implementation
+
+=== Library
+
+`hook.c` and `hook.h` are responsible for interacting with the config files. In
+the case when the code generating a hook event doesn't have special concerns
+about how to run the hooks, the hook library will provide a basic API to call
+all hooks in config order with an `argv_array` provided by the code which
+generates the hook event:
+
+*`int run_hooks(const char *hookname, struct argv_array *args)`*
+
+This call includes the hook command provided by `run-command.h:find_hook()`;
+eventually, this legacy hook will be gated by a config `hook.runHookDir`. The
+config is checked against a number of cases:
+
+- "no": the legacy hook will not be run
+- "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
+
+In case this list is expanded in the future, if a value for `hook.runHookDir` is
+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".
+
+If the caller wants to do something more complicated, the hook library can also
+provide a callback API:
+
+*`int for_each_hookcmd(const char *hookname, hookcmd_function *cb)`*
+
+Finally, to facilitate the builtin, the library will also provide the following
+APIs to interact with the config:
+
+----
+int set_hook_commands(const char *hookname, struct string_list *commands,
+	enum config_scope scope);
+int set_hookcmd(const char *hookcmd, struct hookcmd options);
+
+int list_hook_commands(const char *hookname, struct string_list *commands);
+int list_hooks_in_scope(enum config_scope scope, struct string_list *commands);
+----
+
+`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
+config schema, `struct hookcmd` should contain logical values instead of string
+pairs.
+
+----
+struct hookcmd {
+  const char *name;
+  const char *command;
+
+  /* for illustration only; not planned at present */
+  int parallelizable;
+  const char *hookcmd_before;
+  const char *hookcmd_after;
+  enum recovery_action on_fail;
+}
+----
+
+=== Builtin
+
+`builtin/hook.c` is responsible for providing the frontend. It's responsible for
+formatting user-provided data and then calling the library API to set the
+configs as appropriate. The builtin frontend is not responsible for calling the
+config directly, so that other areas of Git can rely on the hook library to
+understand the most recent config schema for hooks.
+
+=== Migration path
+
+==== Stage 0
+
+Hooks are called by running `run-command.h:find_hook()` with the hookname and
+executing the result. The hook library and builtin do not exist. Hooks only
+exist as specially named scripts within `.git/hooks/`.
+
+==== Stage 1
+
+`git hook list --porcelain <hook-event>` is implemented. Users can replace their
+`.git/hooks/<hook-event>` scripts with a trampoline based on `git hook list`'s
+output. Modifier commands like `git hook add` and `git hook edit` can be
+implemented around this time as well.
+
+==== Stage 2
+
+`hook.h:run_hooks()` is taught to include `run-command.h:find_hook()` at the
+end; calls to `find_hook()` are replaced with calls to `run_hooks()`. Users can
+opt-in to config-based hooks simply by creating some in their config; otherwise
+users should remain unaffected by the change.
+
+==== Stage 3
+
+The call to `find_hook()` inside of `run_hooks()` learns to check for a config,
+`hook.runHookDir`. Users can opt into managing their hooks completely via the
+config this way.
+
+==== Stage 4
+
+`.git/hooks` is removed from the template and the hook directory is considered
+deprecated. To avoid breaking older repos, the default of `hook.runHookDir` is
+not changed, and `find_hook()` is not removed.
+
+== Caveats
+
+=== 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
+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.
+
+=== Ease of use
+
+The config schema is nontrivial; that's why it's important for the `git hook`
+modifier commands to be usable. Contributors with UX expertise are encouraged to
+share their suggestions.
+
+== Alternative approaches
+
+A previous summary of alternatives exists in the
+archives.footnote:[https://lore.kernel.org/git/20191116011125.GG22855@google.com]
+
+=== 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
+
+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 table
+
+.Comparison of alternatives
+|===
+|Feature |Config-based hooks |Hook directories |Status quo
+
+|Supports multiple hooks
+|Natively
+|Natively
+|With user effort
+
+|Safer for zipped repos
+|A little
+|No
+|No
+
+|Previous hooks just work
+|If configured
+|Yes
+|Yes
+
+|Can install one hook to many repos
+|Yes
+|No
+|No
+
+|Discoverability
+|Better (in `git help git`)
+|Same as before
+|Same as before
+
+|Hard to run unexpected hook
+|If configured
+|No
+|No
+|===
+
+== Future work
+
+=== Execution ordering
+
+We may find that config order is insufficient for some users; for example,
+config order makes it difficult to add a new hook to the system or global config
+which runs at the end of the hook list. A new ordering schema should be:
+
+1) Specified by a `hook.order` config, so that users will not unexpectedly see
+their order change;
+
+2) Either dependency or numerically based.
+
+Dependency-based ordering is prone to classic linked-list problems, like a
+cycles and handling of missing dependencies. But, it paves the way for enabling
+parallelization if some tasks truly depend on others.
+
+Numerical ordering makes it tricky for Git to generate suggested ordering
+numbers for each command, but is easy to determine a definitive order.
+
+=== Parallelization
+
+Users with many hooks might want to run them simultaneously, if the hooks don't
+modify state; if one hook depends on another's output, then users will want to
+specify those 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.
+
+=== Securing hookdir hooks
+
+With the design as written in this doc, it's still possible for a malicious user
+to modify `.git/config` to include `hook.pre-receive.command = rm -rf /`, then
+zip their repo and send it to another user. It may be necessary to teach Git to
+only allow one-line hooks like this if they were configured outside of the local
+scope; or another approach, like a list of safe projects, might be useful. It
+may also be sufficient (or at least useful) to teach a `hook.disableAll` config
+or similar flag to the Git executable.
+
+=== Submodule inheritance
+
+It's possible some submodules may want to run the identical set of hooks that
+their superrepo runs. While a globally-configured hook set is helpful, it's not
+a great solution for users who have multiple repos-with-submodules under the
+same user. It would be useful for submodules to learn how to run hooks from
+their superrepo's config, or inherit that hook setting.
+
+== Glossary
+
+*hook event*
+
+A point during Git's execution where user scripts may be run, for example,
+_prepare-commit-msg_ or _pre-push_.
+
+*hook command*
+
+A user script or executable which will be run on one or more hook events.
-- 
2.27.0.rc0.183.gde8f92d652-goog


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v2 2/4] hook: scaffolding for git-hook subcommand
  2020-05-21 18:54 [PATCH v2 0/4] propose config-based hooks Emily Shaffer
  2020-05-21 18:54 ` [PATCH v2 1/4] doc: propose hooks managed by the config Emily Shaffer
@ 2020-05-21 18:54 ` Emily Shaffer
  2020-05-21 18:54 ` [PATCH v2 3/4] hook: add list command Emily Shaffer
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 22+ messages in thread
From: Emily Shaffer @ 2020-05-21 18:54 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Introduce infrastructure for a new subcommand, git-hook, which will be
used to ease config-based hook management. This command will handle
parsing configs to compose a list of hooks to run for a given event, as
well as adding or modifying hook configs in an interactive fashion.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 .gitignore                    |  1 +
 Documentation/git-hook.txt    | 19 +++++++++++++++++++
 Makefile                      |  1 +
 builtin.h                     |  1 +
 builtin/hook.c                | 21 +++++++++++++++++++++
 git.c                         |  1 +
 t/t1360-config-based-hooks.sh | 11 +++++++++++
 7 files changed, 55 insertions(+)
 create mode 100644 Documentation/git-hook.txt
 create mode 100644 builtin/hook.c
 create mode 100755 t/t1360-config-based-hooks.sh

diff --git a/.gitignore b/.gitignore
index ee509a2ad2..0694a34884 100644
--- a/.gitignore
+++ b/.gitignore
@@ -75,6 +75,7 @@
 /git-grep
 /git-hash-object
 /git-help
+/git-hook
 /git-http-backend
 /git-http-fetch
 /git-http-push
diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
new file mode 100644
index 0000000000..2d50c414cc
--- /dev/null
+++ b/Documentation/git-hook.txt
@@ -0,0 +1,19 @@
+git-hook(1)
+===========
+
+NAME
+----
+git-hook - Manage configured hooks
+
+SYNOPSIS
+--------
+[verse]
+'git hook'
+
+DESCRIPTION
+-----------
+You can list, add, and modify hooks with this command.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 3d3a39fc19..fce6ee154e 100644
--- a/Makefile
+++ b/Makefile
@@ -1080,6 +1080,7 @@ BUILTIN_OBJS += builtin/get-tar-commit-id.o
 BUILTIN_OBJS += builtin/grep.o
 BUILTIN_OBJS += builtin/hash-object.o
 BUILTIN_OBJS += builtin/help.o
+BUILTIN_OBJS += builtin/hook.o
 BUILTIN_OBJS += builtin/index-pack.o
 BUILTIN_OBJS += builtin/init-db.o
 BUILTIN_OBJS += builtin/interpret-trailers.o
diff --git a/builtin.h b/builtin.h
index a5ae15bfe5..4e736499c0 100644
--- a/builtin.h
+++ b/builtin.h
@@ -157,6 +157,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix);
 int cmd_grep(int argc, const char **argv, const char *prefix);
 int cmd_hash_object(int argc, const char **argv, const char *prefix);
 int cmd_help(int argc, const char **argv, const char *prefix);
+int cmd_hook(int argc, const char **argv, const char *prefix);
 int cmd_index_pack(int argc, const char **argv, const char *prefix);
 int cmd_init_db(int argc, const char **argv, const char *prefix);
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix);
diff --git a/builtin/hook.c b/builtin/hook.c
new file mode 100644
index 0000000000..b2bbc84d4d
--- /dev/null
+++ b/builtin/hook.c
@@ -0,0 +1,21 @@
+#include "cache.h"
+
+#include "builtin.h"
+#include "parse-options.h"
+
+static const char * const builtin_hook_usage[] = {
+	N_("git hook"),
+	NULL
+};
+
+int cmd_hook(int argc, const char **argv, const char *prefix)
+{
+	struct option builtin_hook_options[] = {
+		OPT_END(),
+	};
+
+	argc = parse_options(argc, argv, prefix, builtin_hook_options,
+			     builtin_hook_usage, 0);
+
+	return 0;
+}
diff --git a/git.c b/git.c
index a2d337eed7..99372529a2 100644
--- a/git.c
+++ b/git.c
@@ -517,6 +517,7 @@ static struct cmd_struct commands[] = {
 	{ "grep", cmd_grep, RUN_SETUP_GENTLY },
 	{ "hash-object", cmd_hash_object },
 	{ "help", cmd_help },
+	{ "hook", cmd_hook, RUN_SETUP },
 	{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY | NO_PARSEOPT },
 	{ "init", cmd_init_db },
 	{ "init-db", cmd_init_db },
diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
new file mode 100755
index 0000000000..34b0df5216
--- /dev/null
+++ b/t/t1360-config-based-hooks.sh
@@ -0,0 +1,11 @@
+#!/bin/bash
+
+test_description='config-managed multihooks, including git-hook command'
+
+. ./test-lib.sh
+
+test_expect_success 'git hook command does not crash' '
+	git hook
+'
+
+test_done
-- 
2.27.0.rc0.183.gde8f92d652-goog


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v2 3/4] hook: add list command
  2020-05-21 18:54 [PATCH v2 0/4] propose config-based hooks Emily Shaffer
  2020-05-21 18:54 ` [PATCH v2 1/4] doc: propose hooks managed by the config Emily Shaffer
  2020-05-21 18:54 ` [PATCH v2 2/4] hook: scaffolding for git-hook subcommand Emily Shaffer
@ 2020-05-21 18:54 ` Emily Shaffer
  2020-05-22 10:27   ` Phillip Wood
  2020-05-24 23:00   ` Johannes Schindelin
  2020-05-21 18:54 ` [PATCH v2 4/4] hook: add --porcelain to " Emily Shaffer
  2020-07-28 22:24 ` [PATCH v3 0/6] propose config-based hooks Emily Shaffer
  4 siblings, 2 replies; 22+ messages in thread
From: Emily Shaffer @ 2020-05-21 18:54 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Teach 'git hook list <hookname>', which checks the known configs in
order to create an ordered list of hooks to run on a given hook event.

Multiple commands can be specified for a given hook by providing
multiple "hook.<hookname>.command = <path-to-hook>" lines. Hooks will be
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.<hookcmd-name>.command = <path-to-hook>" line.

For example:

  $ git config --list | grep ^hook
  hook.pre-commit.command=baz
  hook.pre-commit.command=~/bar.sh
  hookcmd.baz.command=~/baz/from/hookcmd.sh

  $ git hook list pre-commit
  ~/baz/from/hookcmd.sh
  ~/bar.sh

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/git-hook.txt    | 37 +++++++++++++-
 Makefile                      |  1 +
 builtin/hook.c                | 55 +++++++++++++++++++--
 hook.c                        | 90 +++++++++++++++++++++++++++++++++++
 hook.h                        | 15 ++++++
 t/t1360-config-based-hooks.sh | 51 +++++++++++++++++++-
 6 files changed, 242 insertions(+), 7 deletions(-)
 create mode 100644 hook.c
 create mode 100644 hook.h

diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
index 2d50c414cc..e458586e96 100644
--- a/Documentation/git-hook.txt
+++ b/Documentation/git-hook.txt
@@ -8,12 +8,47 @@ git-hook - Manage configured hooks
 SYNOPSIS
 --------
 [verse]
-'git hook'
+'git hook' list <hook-name>
 
 DESCRIPTION
 -----------
 You can list, add, and modify hooks with this command.
 
+This command parses the default configuration files for sections "hook" and
+"hookcmd". "hook" is used to describe the commands which will be run during a
+particular hook event; commands are run in config order. "hookcmd" is used to
+describe attributes of a specific command. If additional attributes don't need
+to be specified, a command to run can be specified directly in the "hook"
+section; if a "hookcmd" by that name isn't found, Git will attempt to run the
+provided value directly. For example:
+
+Global config
+----
+  [hook "post-commit"]
+    command = "linter"
+    command = "~/typocheck.sh"
+
+  [hookcmd "linter"]
+    command = "/bin/linter --c"
+----
+
+Local config
+----
+  [hook "prepare-commit-msg"]
+    command = "linter"
+  [hook "post-commit"]
+    command = "python ~/run-test-suite.py"
+----
+
+COMMANDS
+--------
+
+list <hook-name>::
+
+List the hooks which have been configured for <hook-name>. Hooks appear
+in the order they should be run, and note the config scope where the relevant
+`hook.<hook-name>.command` was specified, not the `hookcmd` (if applicable).
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index fce6ee154e..b7bbf3be7b 100644
--- a/Makefile
+++ b/Makefile
@@ -894,6 +894,7 @@ LIB_OBJS += grep.o
 LIB_OBJS += hashmap.o
 LIB_OBJS += help.o
 LIB_OBJS += hex.o
+LIB_OBJS += hook.o
 LIB_OBJS += ident.o
 LIB_OBJS += interdiff.o
 LIB_OBJS += json-writer.o
diff --git a/builtin/hook.c b/builtin/hook.c
index b2bbc84d4d..cfd8e388bd 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -1,21 +1,68 @@
 #include "cache.h"
 
 #include "builtin.h"
+#include "config.h"
+#include "hook.h"
 #include "parse-options.h"
+#include "strbuf.h"
 
 static const char * const builtin_hook_usage[] = {
-	N_("git hook"),
+	N_("git hook list <hookname>"),
 	NULL
 };
 
-int cmd_hook(int argc, const char **argv, const char *prefix)
+static int list(int argc, const char **argv, const char *prefix)
 {
-	struct option builtin_hook_options[] = {
+	struct list_head *head, *pos;
+	struct hook *item;
+	struct strbuf hookname = STRBUF_INIT;
+
+	struct option list_options[] = {
 		OPT_END(),
 	};
 
-	argc = parse_options(argc, argv, prefix, builtin_hook_options,
+	argc = parse_options(argc, argv, prefix, list_options,
 			     builtin_hook_usage, 0);
 
+	if (argc < 1) {
+		usage_msg_opt("a hookname must be provided to operate on.",
+			      builtin_hook_usage, list_options);
+	}
+
+	strbuf_addstr(&hookname, argv[0]);
+
+	head = hook_list(&hookname);
+
+	if (!head) {
+		printf(_("no commands configured for hook '%s'\n"),
+		       hookname.buf);
+		return 0;
+	}
+
+	list_for_each(pos, head) {
+		item = list_entry(pos, struct hook, list);
+		if (item)
+			printf("%s:\t%s\n",
+			       config_scope_name(item->origin),
+			       item->command.buf);
+	}
+
+	clear_hook_list();
+	strbuf_release(&hookname);
+
 	return 0;
 }
+
+int cmd_hook(int argc, const char **argv, const char *prefix)
+{
+	struct option builtin_hook_options[] = {
+		OPT_END(),
+	};
+	if (argc < 2)
+		usage_with_options(builtin_hook_usage, builtin_hook_options);
+
+	if (!strcmp(argv[1], "list"))
+		return list(argc - 1, argv + 1, prefix);
+
+	usage_with_options(builtin_hook_usage, builtin_hook_options);
+}
diff --git a/hook.c b/hook.c
new file mode 100644
index 0000000000..9dfc1a885e
--- /dev/null
+++ b/hook.c
@@ -0,0 +1,90 @@
+#include "cache.h"
+
+#include "hook.h"
+#include "config.h"
+
+static LIST_HEAD(hook_head);
+
+void free_hook(struct hook *ptr)
+{
+	if (ptr) {
+		strbuf_release(&ptr->command);
+		free(ptr);
+	}
+}
+
+static void emplace_hook(struct list_head *pos, const char *command)
+{
+	struct hook *to_add = malloc(sizeof(struct hook));
+	to_add->origin = current_config_scope();
+	strbuf_init(&to_add->command, 0);
+	strbuf_addstr(&to_add->command, command);
+
+	list_add_tail(&to_add->list, pos);
+}
+
+static void remove_hook(struct list_head *to_remove)
+{
+	struct hook *hook_to_remove = list_entry(to_remove, struct hook, list);
+	list_del(to_remove);
+	free_hook(hook_to_remove);
+}
+
+void clear_hook_list(void)
+{
+	struct list_head *pos, *tmp;
+	list_for_each_safe(pos, tmp, &hook_head)
+		remove_hook(pos);
+}
+
+static int hook_config_lookup(const char *key, const char *value, void *hook_key_cb)
+{
+	const char *hook_key = hook_key_cb;
+
+	if (!strcmp(key, hook_key)) {
+		const char *command = value;
+		struct strbuf hookcmd_name = STRBUF_INIT;
+		struct list_head *pos = NULL, *tmp = NULL;
+
+		/* Check if a hookcmd with that name exists. */
+		strbuf_addf(&hookcmd_name, "hookcmd.%s.command", command);
+		git_config_get_value(hookcmd_name.buf, &command);
+
+		if (!command)
+			BUG("git_config_get_value overwrote a string it shouldn't have");
+
+		/*
+		 * TODO: implement an option-getting callback, e.g.
+		 *   get configs by pattern hookcmd.$value.*
+		 *   for each key+value, do_callback(key, value, cb_data)
+		 */
+
+		list_for_each_safe(pos, tmp, &hook_head) {
+			struct hook *hook = list_entry(pos, struct hook, list);
+			/*
+			 * The list of hooks to run can be reordered by being redeclared
+			 * in the config. Options about hook ordering should be checked
+			 * here.
+			 */
+			if (0 == strcmp(hook->command.buf, command))
+				remove_hook(pos);
+		}
+		emplace_hook(pos, command);
+	}
+
+	return 0;
+}
+
+struct list_head* hook_list(const struct strbuf* hookname)
+{
+	struct strbuf hook_key = STRBUF_INIT;
+
+	if (!hookname)
+		return NULL;
+
+	strbuf_addf(&hook_key, "hook.%s.command", hookname->buf);
+
+	git_config(hook_config_lookup, (void*)hook_key.buf);
+
+	return &hook_head;
+}
diff --git a/hook.h b/hook.h
new file mode 100644
index 0000000000..aaf6511cff
--- /dev/null
+++ b/hook.h
@@ -0,0 +1,15 @@
+#include "config.h"
+#include "list.h"
+#include "strbuf.h"
+
+struct hook
+{
+	struct list_head list;
+	enum config_scope origin;
+	struct strbuf command;
+};
+
+struct list_head* hook_list(const struct strbuf *hookname);
+
+void free_hook(struct hook *ptr);
+void clear_hook_list(void);
diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
index 34b0df5216..4e46d7dd4e 100755
--- a/t/t1360-config-based-hooks.sh
+++ b/t/t1360-config-based-hooks.sh
@@ -4,8 +4,55 @@ test_description='config-managed multihooks, including git-hook command'
 
 . ./test-lib.sh
 
-test_expect_success 'git hook command does not crash' '
-	git hook
+test_expect_success 'git hook rejects commands without a mode' '
+	test_must_fail git hook pre-commit
+'
+
+
+test_expect_success 'git hook rejects commands without a hookname' '
+	test_must_fail git hook list
+'
+
+test_expect_success 'setup hooks in global, and local' '
+	git config --add --local hook.pre-commit.command "/path/ghi" &&
+	git config --add --global hook.pre-commit.command "/path/def"
+'
+
+test_expect_success 'git hook list orders by config order' '
+	cat >expected <<-\EOF &&
+	global:	/path/def
+	local:	/path/ghi
+	EOF
+
+	git hook list pre-commit >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git hook list dereferences a hookcmd' '
+	git config --add --local hook.pre-commit.command "abc" &&
+	git config --add --global hookcmd.abc.command "/path/abc" &&
+
+	cat >expected <<-\EOF &&
+	global:	/path/def
+	local:	/path/ghi
+	local:	/path/abc
+	EOF
+
+	git hook list pre-commit >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git hook list reorders on duplicate commands' '
+	git config --add --local hook.pre-commit.command "/path/def" &&
+
+	cat >expected <<-\EOF &&
+	local:	/path/ghi
+	local:	/path/abc
+	local:	/path/def
+	EOF
+
+	git hook list pre-commit >actual &&
+	test_cmp expected actual
 '
 
 test_done
-- 
2.27.0.rc0.183.gde8f92d652-goog


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v2 4/4] hook: add --porcelain to list command
  2020-05-21 18:54 [PATCH v2 0/4] propose config-based hooks Emily Shaffer
                   ` (2 preceding siblings ...)
  2020-05-21 18:54 ` [PATCH v2 3/4] hook: add list command Emily Shaffer
@ 2020-05-21 18:54 ` Emily Shaffer
  2020-05-24 23:00   ` Johannes Schindelin
  2020-07-28 22:24 ` [PATCH v3 0/6] propose config-based hooks Emily Shaffer
  4 siblings, 1 reply; 22+ messages in thread
From: Emily Shaffer @ 2020-05-21 18:54 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Teach 'git hook list --porcelain <hookname>', which prints simply the
commands to be run in the order suggested by the config. This option is
intended for use by user scripts, wrappers, or out-of-process Git
commands which still want to execute hooks. For example, the following
snippet might be added to git-send-email.perl to introduce a
`pre-send-email` hook:

  sub pre_send_email {
    open(my $fh, 'git hook list --porcelain pre-send-email |');
    chomp(my @hooks = <$fh>);
    close($fh);

    foreach $hook (@hooks) {
            system $hook
    }

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/git-hook.txt    | 13 +++++++++++--
 builtin/hook.c                | 17 +++++++++++++----
 t/t1360-config-based-hooks.sh | 11 +++++++++++
 3 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
index e458586e96..0854035ce2 100644
--- a/Documentation/git-hook.txt
+++ b/Documentation/git-hook.txt
@@ -8,7 +8,7 @@ git-hook - Manage configured hooks
 SYNOPSIS
 --------
 [verse]
-'git hook' list <hook-name>
+'git hook' list [--porcelain] <hook-name>
 
 DESCRIPTION
 -----------
@@ -43,11 +43,20 @@ Local config
 COMMANDS
 --------
 
-list <hook-name>::
+list [--porcelain] <hook-name>::
 
 List the hooks which have been configured for <hook-name>. Hooks appear
 in the order they should be run, and note the config scope where the relevant
 `hook.<hook-name>.command` was specified, not the `hookcmd` (if applicable).
++
+If `--porcelain` is specified, instead print the commands alone, separated by
+newlines, for easy parsing by a script.
+
+OPTIONS
+-------
+--porcelain::
+	With `list`, print the commands in the order they should be run,
+	separated by newlines, for easy parsing by a script.
 
 GIT
 ---
diff --git a/builtin/hook.c b/builtin/hook.c
index cfd8e388bd..2e51c84c81 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -16,8 +16,11 @@ static int list(int argc, const char **argv, const char *prefix)
 	struct list_head *head, *pos;
 	struct hook *item;
 	struct strbuf hookname = STRBUF_INIT;
+	int porcelain = 0;
 
 	struct option list_options[] = {
+		OPT_BOOL(0, "porcelain", &porcelain,
+			 "format for execution by a script"),
 		OPT_END(),
 	};
 
@@ -29,6 +32,8 @@ 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);
@@ -41,10 +46,14 @@ static int list(int argc, const char **argv, const char *prefix)
 
 	list_for_each(pos, head) {
 		item = list_entry(pos, struct hook, list);
-		if (item)
-			printf("%s:\t%s\n",
-			       config_scope_name(item->origin),
-			       item->command.buf);
+		if (item) {
+			if (porcelain)
+				printf("%s\n", item->command.buf);
+			else
+				printf("%s:\t%s\n",
+				       config_scope_name(item->origin),
+				       item->command.buf);
+		}
 	}
 
 	clear_hook_list();
diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
index 4e46d7dd4e..3296d8af45 100755
--- a/t/t1360-config-based-hooks.sh
+++ b/t/t1360-config-based-hooks.sh
@@ -55,4 +55,15 @@ test_expect_success 'git hook list reorders on duplicate commands' '
 	test_cmp expected actual
 '
 
+test_expect_success 'git hook list --porcelain prints just the command' '
+	cat >expected <<-\EOF &&
+	/path/ghi
+	/path/abc
+	/path/def
+	EOF
+
+	git hook list --porcelain pre-commit >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.27.0.rc0.183.gde8f92d652-goog


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/4] doc: propose hooks managed by the config
  2020-05-21 18:54 ` [PATCH v2 1/4] doc: propose hooks managed by the config Emily Shaffer
@ 2020-05-22 10:13   ` Phillip Wood
  2020-06-09 20:26     ` Emily Shaffer
  0 siblings, 1 reply; 22+ messages in thread
From: Phillip Wood @ 2020-05-22 10:13 UTC (permalink / raw)
  To: Emily Shaffer, git

Hi Emily

Thanks for working on this

On 21/05/2020 19:54, Emily Shaffer wrote:
> Begin a design document for config-based hooks, managed via git-hook.
> Focus on an overview of the implementation and motivation for design
> decisions. Briefly discuss the alternatives considered before this
> point. Also, attempt to redefine terms to fit into a multihook world.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  Documentation/Makefile                        |   1 +
>  .../technical/config-based-hooks.txt          | 320 ++++++++++++++++++
>  2 files changed, 321 insertions(+)
>  create mode 100644 Documentation/technical/config-based-hooks.txt
> 
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index 15d9d04f31..5b21f31d31 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -80,6 +80,7 @@ SP_ARTICLES += $(API_DOCS)
>  TECH_DOCS += MyFirstContribution
>  TECH_DOCS += MyFirstObjectWalk
>  TECH_DOCS += SubmittingPatches
> +TECH_DOCS += technical/config-based-hooks
>  TECH_DOCS += technical/hash-function-transition
>  TECH_DOCS += technical/http-protocol
>  TECH_DOCS += technical/index-format
> diff --git a/Documentation/technical/config-based-hooks.txt b/Documentation/technical/config-based-hooks.txt
> new file mode 100644
> index 0000000000..59cdc25a47
> --- /dev/null
> +++ b/Documentation/technical/config-based-hooks.txt
> @@ -0,0 +1,320 @@
> +Configuration-based hook management
> +===================================
> +
> +== Motivation
> +
> +Treat hooks as a first-class citizen by replacing the .git/hook/hookname path as
> +the only source of hooks to execute, in a way which is friendly to users with
> +multiple repos which have similar needs.
> +
> +Redefine "hook" as an event rather than a single script, allowing users to
> +perform unrelated actions on a single event.
> +
> +Take a step closer to safety when copying zipped Git repositories from untrusted
> +users.

Having read through this (admittedly fairly quickly) I'm not sure what
that step is

> +
> +Make it easier for users to discover Git's hook feature and automate their
> +workflows.
> +
> +== User interfaces
> +
> +=== Config schema
> +
> +Hooks can be introduced by editing the configuration manually. There are two new
> +sections added, `hook` and `hookcmd`.
> +
> +==== `hook`
> +
> +Primarily contains subsections for each hook event. These subsections define
> +hook command execution order;

May be "The order of these subsections define the hook command execution
order" ?

> hook commands can be specified by passing the
> +command directly if no additional configuration is needed, or by passing the
> +name of a `hookcmd`.

I know what you mean by "passing" but as this section is talking about
config settings perhaps it should refer to the keys and values.

> If Git does not find a `hookcmd` whose subsection matches
> +the value of the given command string, Git will try to execute the string
> +directly. Hooks are executed by passing the resolved command string to the
> +shell.

Do we really need to invoke the shell just to split a command-line and
look up the command in $PATH? If we used split_commandline() in alias.c
then we could avoid invoking this extra process for each hook command.

> Hook event subsections can also contain per-hook-event settings.
> +
> +Also contains top-level hook execution settings, for example,
> +`hook.warnHookDir`, `hook.runHookDir`, or `hook.disableAll`.

(see sections ...) ? for the forward references to these settings?

> +
> +----
> +[hook "pre-commit"]
> +  command = perl-linter
> +  command = /usr/bin/git-secrets --pre-commit
> +
> +[hook "pre-applypatch"]
> +  command = perl-linter
> +  error = ignore
> +
> +[hook]
> +  runHookDir = interactive
> +----
> +
> +==== `hookcmd`
> +
> +Defines a hook command and its attributes, which will be used when a hook event
> +occurs. Unqualified attributes are assumed to apply to this hook during all hook
> +events, but event-specific attributes can also be supplied. The example runs
> +`/usr/bin/lint-it --language=perl <args passed by Git>`, but for repos which
> +include this config, the hook command will be skipped for all events to which
> +it's normally subscribed _except_ `pre-commit`.
> +
> +----
> +[hookcmd "perl-linter"]
> +  command = /usr/bin/lint-it --language=perl
> +  skip = true
> +  pre-commit-skip = false
> +----
> +
> +=== Command-line API
> +
> +Users should be able to view, reorder, and create hook commands via the command
> +line. External tools should be able to view a list of hooks in the correct order
> +to run.
> +
> +*`git hook list <hook-event>`*
> +
> +*`git hook list (--system|--global|--local|--worktree)`*
> +
> +*`git hook edit <hook-event>`*
> +
> +*`git hook add <hook-command> <hook-event> <options...>`*
> +
> +=== Hook editor
> +
> +The tool which is presented by `git hook edit <hook-command>`. Ideally, this
> +tool should be easier to use than manually editing the config, and then produce
> +a concise config afterwards. It may take a form similar to `git rebase
> +--interactive`.

rebase -i is not necessarily an exemplar of user interface design, what
sort of thing do you have in mind?

> +
> +== Implementation
> +
> +=== Library
> +
> +`hook.c` and `hook.h` are responsible for interacting with the config files. In
> +the case when the code generating a hook event doesn't have special concerns
> +about how to run the hooks, the hook library will provide a basic API to call
> +all hooks in config order with an `argv_array` provided by the code which
> +generates the hook event:
> +
> +*`int run_hooks(const char *hookname, struct argv_array *args)`*
> +
> +This call includes the hook command provided by `run-command.h:find_hook()`;
> +eventually, this legacy hook will be gated by a config `hook.runHookDir`. The
> +config is checked against a number of cases:
> +
> +- "no": the legacy hook will not be run
> +- "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
> +
> +In case this list is expanded in the future, if a value for `hook.runHookDir` is
> +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".
> +
> +If the caller wants to do something more complicated, the hook library can also
> +provide a callback API:
> +
> +*`int for_each_hookcmd(const char *hookname, hookcmd_function *cb)`*
> +
> +Finally, to facilitate the builtin, the library will also provide the following
> +APIs to interact with the config:
> +
> +----
> +int set_hook_commands(const char *hookname, struct string_list *commands,
> +	enum config_scope scope);
> +int set_hookcmd(const char *hookcmd, struct hookcmd options);
> +
> +int list_hook_commands(const char *hookname, struct string_list *commands);
> +int list_hooks_in_scope(enum config_scope scope, struct string_list *commands);
> +----
> +
> +`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
> +config schema, `struct hookcmd` should contain logical values instead of string
> +pairs.
> +
> +----
> +struct hookcmd {
> +  const char *name;
> +  const char *command;
> +
> +  /* for illustration only; not planned at present */
> +  int parallelizable;
> +  const char *hookcmd_before;
> +  const char *hookcmd_after;
> +  enum recovery_action on_fail;
> +}
> +----
> +
> +=== Builtin
> +
> +`builtin/hook.c` is responsible for providing the frontend. It's responsible for
> +formatting user-provided data and then calling the library API to set the
> +configs as appropriate. The builtin frontend is not responsible for calling the
> +config directly, so that other areas of Git can rely on the hook library to
> +understand the most recent config schema for hooks.
> +
> +=== Migration path
> +
> +==== Stage 0
> +
> +Hooks are called by running `run-command.h:find_hook()` with the hookname and
> +executing the result. The hook library and builtin do not exist. Hooks only
> +exist as specially named scripts within `.git/hooks/`.
> +
> +==== Stage 1
> +
> +`git hook list --porcelain <hook-event>` is implemented. Users can replace their
> +`.git/hooks/<hook-event>` scripts with a trampoline based on `git hook list`'s
> +output. Modifier commands like `git hook add` and `git hook edit` can be
> +implemented around this time as well.
> +
> +==== Stage 2
> +
> +`hook.h:run_hooks()` is taught to include `run-command.h:find_hook()` at the
> +end; calls to `find_hook()` are replaced with calls to `run_hooks()`. Users can
> +opt-in to config-based hooks simply by creating some in their config; otherwise
> +users should remain unaffected by the change.
> +
> +==== Stage 3
> +
> +The call to `find_hook()` inside of `run_hooks()` learns to check for a config,
> +`hook.runHookDir`. Users can opt into managing their hooks completely via the
> +config this way.
> +
> +==== Stage 4
> +
> +`.git/hooks` is removed from the template and the hook directory is considered
> +deprecated. To avoid breaking older repos, the default of `hook.runHookDir` is
> +not changed, and `find_hook()` is not removed.
> +
> +== Caveats
> +
> +=== 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
> +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.
> +
> +=== Ease of use
> +
> +The config schema is nontrivial; that's why it's important for the `git hook`
> +modifier commands to be usable.

That's an important point

> Contributors with UX expertise are encouraged to
> +share their suggestions.
> +
> +== Alternative approaches
> +
> +A previous summary of alternatives exists in the
> +archives.footnote:[https://lore.kernel.org/git/20191116011125.GG22855@google.com]
> +
> +=== 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
> +
> +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 table
> +
> +.Comparison of alternatives
> +|===
> +|Feature |Config-based hooks |Hook directories |Status quo
> +
> +|Supports multiple hooks
> +|Natively
> +|Natively
> +|With user effort
> +
> +|Safer for zipped repos
> +|A little
> +|No
> +|No
> +
> +|Previous hooks just work
> +|If configured
> +|Yes
> +|Yes
> +
> +|Can install one hook to many repos
> +|Yes
> +|No
> +|No
> +
> +|Discoverability
> +|Better (in `git help git`)
> +|Same as before
> +|Same as before
> +
> +|Hard to run unexpected hook
> +|If configured
> +|No
> +|No
> +|===
> +
> +== Future work
> +
> +=== Execution ordering
> +
> +We may find that config order is insufficient for some users; for example,
> +config order makes it difficult to add a new hook to the system or global config
> +which runs at the end of the hook list. A new ordering schema should be:
> +
> +1) Specified by a `hook.order` config, so that users will not unexpectedly see
> +their order change;
> +
> +2) Either dependency or numerically based.
> +
> +Dependency-based ordering is prone to classic linked-list problems, like a
> +cycles and handling of missing dependencies. But, it paves the way for enabling
> +parallelization if some tasks truly depend on others.
> +
> +Numerical ordering makes it tricky for Git to generate suggested ordering
> +numbers for each command, but is easy to determine a definitive order.
> +
> +=== Parallelization
> +
> +Users with many hooks might want to run them simultaneously, if the hooks don't
> +modify state; if one hook depends on another's output, then users will want to
> +specify those 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.
> +
> +=== Securing hookdir hooks
> +
> +With the design as written in this doc, it's still possible for a malicious user
> +to modify `.git/config` to include `hook.pre-receive.command = rm -rf /`, then
> +zip their repo and send it to another user. It may be necessary to teach Git to
> +only allow one-line hooks like this if they were configured outside of the local
> +scope;

Does "disabling one-line hooks" mean "disable passing command line
arguments to the hook"? I'm not sure that gains much security - can't I
just set 'hook.pre-receive.command = ./delete-everything' and include
delete-everything in my malicious repo?

Best Wishes

Phillip

> or another approach, like a list of safe projects, might be useful. It
> +may also be sufficient (or at least useful) to teach a `hook.disableAll` config
> +or similar flag to the Git executable.
> +
> +=== Submodule inheritance
> +
> +It's possible some submodules may want to run the identical set of hooks that
> +their superrepo runs. While a globally-configured hook set is helpful, it's not
> +a great solution for users who have multiple repos-with-submodules under the
> +same user. It would be useful for submodules to learn how to run hooks from
> +their superrepo's config, or inherit that hook setting.
> +
> +== Glossary
> +
> +*hook event*
> +
> +A point during Git's execution where user scripts may be run, for example,
> +_prepare-commit-msg_ or _pre-push_.
> +
> +*hook command*
> +
> +A user script or executable which will be run on one or more hook events.
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 3/4] hook: add list command
  2020-05-21 18:54 ` [PATCH v2 3/4] hook: add list command Emily Shaffer
@ 2020-05-22 10:27   ` Phillip Wood
  2020-06-09 21:49     ` Emily Shaffer
  2020-05-24 23:00   ` Johannes Schindelin
  1 sibling, 1 reply; 22+ messages in thread
From: Phillip Wood @ 2020-05-22 10:27 UTC (permalink / raw)
  To: Emily Shaffer, git

Hi Emily

On 21/05/2020 19:54, Emily Shaffer wrote:
> Teach 'git hook list <hookname>', which checks the known configs in
> order to create an ordered list of hooks to run on a given hook event.
> 
> Multiple commands can be specified for a given hook by providing
> multiple "hook.<hookname>.command = <path-to-hook>" lines. Hooks will be
> 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.<hookcmd-name>.command = <path-to-hook>" line.
> 
> For example:
> 
>   $ git config --list | grep ^hook
>   hook.pre-commit.command=baz
>   hook.pre-commit.command=~/bar.sh
>   hookcmd.baz.command=~/baz/from/hookcmd.sh
> 
>   $ git hook list pre-commit
>   ~/baz/from/hookcmd.sh
>   ~/bar.sh
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  Documentation/git-hook.txt    | 37 +++++++++++++-
>  Makefile                      |  1 +
>  builtin/hook.c                | 55 +++++++++++++++++++--
>  hook.c                        | 90 +++++++++++++++++++++++++++++++++++
>  hook.h                        | 15 ++++++
>  t/t1360-config-based-hooks.sh | 51 +++++++++++++++++++-
>  6 files changed, 242 insertions(+), 7 deletions(-)
>  create mode 100644 hook.c
>  create mode 100644 hook.h
> 
> diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
> index 2d50c414cc..e458586e96 100644
> --- a/Documentation/git-hook.txt
> +++ b/Documentation/git-hook.txt
> @@ -8,12 +8,47 @@ git-hook - Manage configured hooks
>  SYNOPSIS
>  --------
>  [verse]
> -'git hook'
> +'git hook' list <hook-name>
>  
>  DESCRIPTION
>  -----------
>  You can list, add, and modify hooks with this command.
>  
> +This command parses the default configuration files for sections "hook" and
> +"hookcmd". "hook" is used to describe the commands which will be run during a
> +particular hook event; commands are run in config order. "hookcmd" is used to
> +describe attributes of a specific command. If additional attributes don't need
> +to be specified, a command to run can be specified directly in the "hook"
> +section; if a "hookcmd" by that name isn't found, Git will attempt to run the
> +provided value directly. For example:
> +
> +Global config
> +----
> +  [hook "post-commit"]
> +    command = "linter"
> +    command = "~/typocheck.sh"
> +
> +  [hookcmd "linter"]
> +    command = "/bin/linter --c"
> +----
> +
> +Local config
> +----
> +  [hook "prepare-commit-msg"]
> +    command = "linter"
> +  [hook "post-commit"]
> +    command = "python ~/run-test-suite.py"
> +----
> +
> +COMMANDS
> +--------
> +
> +list <hook-name>::
> +
> +List the hooks which have been configured for <hook-name>. Hooks appear
> +in the order they should be run, and note the config scope where the relevant
> +`hook.<hook-name>.command` was specified, not the `hookcmd` (if applicable).
> +
>  GIT
>  ---
>  Part of the linkgit:git[1] suite
> diff --git a/Makefile b/Makefile
> index fce6ee154e..b7bbf3be7b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -894,6 +894,7 @@ LIB_OBJS += grep.o
>  LIB_OBJS += hashmap.o
>  LIB_OBJS += help.o
>  LIB_OBJS += hex.o
> +LIB_OBJS += hook.o
>  LIB_OBJS += ident.o
>  LIB_OBJS += interdiff.o
>  LIB_OBJS += json-writer.o
> diff --git a/builtin/hook.c b/builtin/hook.c
> index b2bbc84d4d..cfd8e388bd 100644
> --- a/builtin/hook.c
> +++ b/builtin/hook.c
> @@ -1,21 +1,68 @@
>  #include "cache.h"
>  
>  #include "builtin.h"
> +#include "config.h"
> +#include "hook.h"
>  #include "parse-options.h"
> +#include "strbuf.h"
>  
>  static const char * const builtin_hook_usage[] = {
> -	N_("git hook"),
> +	N_("git hook list <hookname>"),
>  	NULL
>  };
>  
> -int cmd_hook(int argc, const char **argv, const char *prefix)
> +static int list(int argc, const char **argv, const char *prefix)
>  {
> -	struct option builtin_hook_options[] = {
> +	struct list_head *head, *pos;
> +	struct hook *item;
> +	struct strbuf hookname = STRBUF_INIT;
> +
> +	struct option list_options[] = {
>  		OPT_END(),
>  	};
>  
> -	argc = parse_options(argc, argv, prefix, builtin_hook_options,
> +	argc = parse_options(argc, argv, prefix, list_options,
>  			     builtin_hook_usage, 0);
>  
> +	if (argc < 1) {
> +		usage_msg_opt("a hookname must be provided to operate on.",
> +			      builtin_hook_usage, list_options);
> +	}
> +
> +	strbuf_addstr(&hookname, argv[0]);
> +
> +	head = hook_list(&hookname);
> +
> +	if (!head) {
> +		printf(_("no commands configured for hook '%s'\n"),
> +		       hookname.buf);
> +		return 0;
> +	}
> +
> +	list_for_each(pos, head) {
> +		item = list_entry(pos, struct hook, list);
> +		if (item)
> +			printf("%s:\t%s\n",
> +			       config_scope_name(item->origin),
> +			       item->command.buf);
> +	}
> +
> +	clear_hook_list();
> +	strbuf_release(&hookname);
> +
>  	return 0;
>  }
> +
> +int cmd_hook(int argc, const char **argv, const char *prefix)
> +{
> +	struct option builtin_hook_options[] = {
> +		OPT_END(),
> +	};
> +	if (argc < 2)
> +		usage_with_options(builtin_hook_usage, builtin_hook_options);
> +
> +	if (!strcmp(argv[1], "list"))
> +		return list(argc - 1, argv + 1, prefix);
> +
> +	usage_with_options(builtin_hook_usage, builtin_hook_options);
> +}
> diff --git a/hook.c b/hook.c
> new file mode 100644
> index 0000000000..9dfc1a885e
> --- /dev/null
> +++ b/hook.c
> @@ -0,0 +1,90 @@
> +#include "cache.h"
> +
> +#include "hook.h"
> +#include "config.h"
> +
> +static LIST_HEAD(hook_head);
> +
> +void free_hook(struct hook *ptr)
> +{
> +	if (ptr) {
> +		strbuf_release(&ptr->command);
> +		free(ptr);
> +	}
> +}
> +
> +static void emplace_hook(struct list_head *pos, const char *command)
> +{
> +	struct hook *to_add = malloc(sizeof(struct hook));
> +	to_add->origin = current_config_scope();
> +	strbuf_init(&to_add->command, 0);
> +	strbuf_addstr(&to_add->command, command);
> +
> +	list_add_tail(&to_add->list, pos);
> +}
> +
> +static void remove_hook(struct list_head *to_remove)
> +{
> +	struct hook *hook_to_remove = list_entry(to_remove, struct hook, list);
> +	list_del(to_remove);
> +	free_hook(hook_to_remove);
> +}
> +
> +void clear_hook_list(void)
> +{
> +	struct list_head *pos, *tmp;
> +	list_for_each_safe(pos, tmp, &hook_head)
> +		remove_hook(pos);
> +}
> +
> +static int hook_config_lookup(const char *key, const char *value, void *hook_key_cb)
> +{
> +	const char *hook_key = hook_key_cb;
> +
> +	if (!strcmp(key, hook_key)) {
> +		const char *command = value;
> +		struct strbuf hookcmd_name = STRBUF_INIT;
> +		struct list_head *pos = NULL, *tmp = NULL;
> +
> +		/* Check if a hookcmd with that name exists. */
> +		strbuf_addf(&hookcmd_name, "hookcmd.%s.command", command);
> +		git_config_get_value(hookcmd_name.buf, &command);

This looks dodgy to me. This code is called by git_config() as it parses
the config files, so it has not had a chance to fully populate the
config cache used by git_config_get_value(). I think the test below
passes because the hookcmd setting is set in the global file and the
hook setting is set in the local file so when we have already parsed the
hookcmd setting when we come to look it up. The same comment applies to
the hypothetical ordering config mentioned below. I think it would be
better to collect the list of hook.<event>.command settings in this
callback and then look up any hookcmd settings for those hook commands
after we've finished reading all of the config files.

> +
> +		if (!command)
> +			BUG("git_config_get_value overwrote a string it shouldn't have");
> +
> +		/*
> +		 * TODO: implement an option-getting callback, e.g.
> +		 *   get configs by pattern hookcmd.$value.*
> +		 *   for each key+value, do_callback(key, value, cb_data)
> +		 */
> +
> +		list_for_each_safe(pos, tmp, &hook_head) {
> +			struct hook *hook = list_entry(pos, struct hook, list);
> +			/*
> +			 * The list of hooks to run can be reordered by being redeclared
> +			 * in the config. Options about hook ordering should be checked
> +			 * here.
> +			 */
> +			if (0 == strcmp(hook->command.buf, command))
> +				remove_hook(pos);
> +		}
> +		emplace_hook(pos, command);
> +	}
> +
> +	return 0;
> +}
> +
> +struct list_head* hook_list(const struct strbuf* hookname)
> +{
> +	struct strbuf hook_key = STRBUF_INIT;
> +
> +	if (!hookname)
> +		return NULL;
> +
> +	strbuf_addf(&hook_key, "hook.%s.command", hookname->buf);
> +
> +	git_config(hook_config_lookup, (void*)hook_key.buf);
> +
> +	return &hook_head;
> +}
> diff --git a/hook.h b/hook.h
> new file mode 100644
> index 0000000000..aaf6511cff
> --- /dev/null
> +++ b/hook.h
> @@ -0,0 +1,15 @@
> +#include "config.h"
> +#include "list.h"
> +#include "strbuf.h"
> +
> +struct hook
> +{
> +	struct list_head list;
> +	enum config_scope origin;
> +	struct strbuf command;
> +};
> +
> +struct list_head* hook_list(const struct strbuf *hookname);
> +
> +void free_hook(struct hook *ptr);
> +void clear_hook_list(void);
> diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
> index 34b0df5216..4e46d7dd4e 100755
> --- a/t/t1360-config-based-hooks.sh
> +++ b/t/t1360-config-based-hooks.sh
> @@ -4,8 +4,55 @@ test_description='config-managed multihooks, including git-hook command'
>  
>  . ./test-lib.sh
>  
> -test_expect_success 'git hook command does not crash' '
> -	git hook
> +test_expect_success 'git hook rejects commands without a mode' '
> +	test_must_fail git hook pre-commit
> +'
> +
> +
> +test_expect_success 'git hook rejects commands without a hookname' '
> +	test_must_fail git hook list
> +'
> +
> +test_expect_success 'setup hooks in global, and local' '
> +	git config --add --local hook.pre-commit.command "/path/ghi" &&

Can I make a plea for the use of test_config please. Writing tests which
rely on previous tests for their set-up creates a chain of hidden
dependencies that make it hard to add/alter tests later or run a subset
of the tests when developing a new patch. t3404-rebase-interactive.sh is
a prime example of this and I dread touching it.

> +	git config --add --global hook.pre-commit.command "/path/def"
> +'
> +
> +test_expect_success 'git hook list orders by config order' '
> +	cat >expected <<-\EOF &&
> +	global:	/path/def
> +	local:	/path/ghi
> +	EOF
> +
> +	git hook list pre-commit >actual &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'git hook list dereferences a hookcmd' '
> +	git config --add --local hook.pre-commit.command "abc" &&
> +	git config --add --global hookcmd.abc.command "/path/abc" &&
> +
> +	cat >expected <<-\EOF &&
> +	global:	/path/def
> +	local:	/path/ghi
> +	local:	/path/abc

We should make it clear in the documentation that the config origin
applies to the hook setting, even though we display the hookcmd command
which is set globally here for the last hook.

Best Wishes

Phillip

> +	EOF
> +
> +	git hook list pre-commit >actual &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'git hook list reorders on duplicate commands' '
> +	git config --add --local hook.pre-commit.command "/path/def" &&
> +
> +	cat >expected <<-\EOF &&
> +	local:	/path/ghi
> +	local:	/path/abc
> +	local:	/path/def
> +	EOF
> +
> +	git hook list pre-commit >actual &&
> +	test_cmp expected actual
>  '
>  
>  test_done
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 3/4] hook: add list command
  2020-05-21 18:54 ` [PATCH v2 3/4] hook: add list command Emily Shaffer
  2020-05-22 10:27   ` Phillip Wood
@ 2020-05-24 23:00   ` Johannes Schindelin
  2020-05-27 23:37     ` Emily Shaffer
  1 sibling, 1 reply; 22+ messages in thread
From: Johannes Schindelin @ 2020-05-24 23:00 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

Hi Emily,

On Thu, 21 May 2020, Emily Shaffer wrote:

> diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
> index 34b0df5216..4e46d7dd4e 100755
> --- a/t/t1360-config-based-hooks.sh
> +++ b/t/t1360-config-based-hooks.sh
> @@ -4,8 +4,55 @@ test_description='config-managed multihooks, including git-hook command'
>
>  . ./test-lib.sh
>
> -test_expect_success 'git hook command does not crash' '
> -	git hook
> +test_expect_success 'git hook rejects commands without a mode' '
> +	test_must_fail git hook pre-commit
> +'
> +
> +
> +test_expect_success 'git hook rejects commands without a hookname' '
> +	test_must_fail git hook list
> +'
> +
> +test_expect_success 'setup hooks in global, and local' '
> +	git config --add --local hook.pre-commit.command "/path/ghi" &&
> +	git config --add --global hook.pre-commit.command "/path/def"
> +'
> +
> +test_expect_success 'git hook list orders by config order' '
> +	cat >expected <<-\EOF &&
> +	global:	/path/def
> +	local:	/path/ghi
> +	EOF
> +
> +	git hook list pre-commit >actual &&
> +	test_cmp expected actual

This, as well as the next two test cases, won't work on Windows, as you
almost certainly realized from looking at the failed GitHub workflow run
of your branch.

The reason is that Unix-like absolute paths like `/path/def` do _not_ do
what you think on Windows: they are relative to the MSYS2 root (because
the shell script runs in an MSYS2 Bash). The Git executable, however, has
not the slightest idea about MSYS2 and does not handle those. To remedy
that, the MSYS2 Bash prefixes those paths with the absolute
_Windows-style_ path when passing them to `git.exe` (in your case,
actually in the `setup hooks` test case above).

So you will need to squash this (or an equivalent fix) into your patch:

-- snip --
From f2568d47509130a9c35590d907797d2eb813ac0d Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mon, 25 May 2020 15:03:16 +0200
Subject: [PATCH] fixup??? hook: add list command

This is needed to make the tests pass on Windows, where Unix-like
absolute paths are not what you think they are.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1360-config-based-hooks.sh | 39 +++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
index 3296d8af4587..c862655fd4d9 100755
--- a/t/t1360-config-based-hooks.sh
+++ b/t/t1360-config-based-hooks.sh
@@ -18,10 +18,19 @@ test_expect_success 'setup hooks in global, and local' '
 	git config --add --global hook.pre-commit.command "/path/def"
 '

+ROOT=
+if test_have_prereq MINGW
+then
+	# In Git for Windows, Unix-like paths work only in shell scripts;
+	# `git.exe`, however, will prefix them with the pseudo root directory
+	# (of the Unix shell). Let's accommodate for that.
+	ROOT="$(cd / && pwd)"
+fi
+
 test_expect_success 'git hook list orders by config order' '
-	cat >expected <<-\EOF &&
-	global:	/path/def
-	local:	/path/ghi
+	cat >expected <<-EOF &&
+	global:	$ROOT/path/def
+	local:	$ROOT/path/ghi
 	EOF

 	git hook list pre-commit >actual &&
@@ -32,10 +41,10 @@ test_expect_success 'git hook list dereferences a hookcmd' '
 	git config --add --local hook.pre-commit.command "abc" &&
 	git config --add --global hookcmd.abc.command "/path/abc" &&

-	cat >expected <<-\EOF &&
-	global:	/path/def
-	local:	/path/ghi
-	local:	/path/abc
+	cat >expected <<-EOF &&
+	global:	$ROOT/path/def
+	local:	$ROOT/path/ghi
+	local:	$ROOT/path/abc
 	EOF

 	git hook list pre-commit >actual &&
@@ -45,10 +54,10 @@ test_expect_success 'git hook list dereferences a hookcmd' '
 test_expect_success 'git hook list reorders on duplicate commands' '
 	git config --add --local hook.pre-commit.command "/path/def" &&

-	cat >expected <<-\EOF &&
-	local:	/path/ghi
-	local:	/path/abc
-	local:	/path/def
+	cat >expected <<-EOF &&
+	local:	$ROOT/path/ghi
+	local:	$ROOT/path/abc
+	local:	$ROOT/path/def
 	EOF

 	git hook list pre-commit >actual &&
@@ -56,10 +65,10 @@ test_expect_success 'git hook list reorders on duplicate commands' '
 '

 test_expect_success 'git hook list --porcelain prints just the command' '
-	cat >expected <<-\EOF &&
-	/path/ghi
-	/path/abc
-	/path/def
+	cat >expected <<-EOF &&
+	$ROOT/path/ghi
+	$ROOT/path/abc
+	$ROOT/path/def
 	EOF

 	git hook list --porcelain pre-commit >actual &&
--
2.27.0.rc1.windows.1

-- snap --

Ciao,
Dscho

> +'
> +
> +test_expect_success 'git hook list dereferences a hookcmd' '
> +	git config --add --local hook.pre-commit.command "abc" &&
> +	git config --add --global hookcmd.abc.command "/path/abc" &&
> +
> +	cat >expected <<-\EOF &&
> +	global:	/path/def
> +	local:	/path/ghi
> +	local:	/path/abc
> +	EOF
> +
> +	git hook list pre-commit >actual &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'git hook list reorders on duplicate commands' '
> +	git config --add --local hook.pre-commit.command "/path/def" &&
> +
> +	cat >expected <<-\EOF &&
> +	local:	/path/ghi
> +	local:	/path/abc
> +	local:	/path/def
> +	EOF
> +
> +	git hook list pre-commit >actual &&
> +	test_cmp expected actual
>  '
>
>  test_done
> --
> 2.27.0.rc0.183.gde8f92d652-goog
>
>
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 4/4] hook: add --porcelain to list command
  2020-05-21 18:54 ` [PATCH v2 4/4] hook: add --porcelain to " Emily Shaffer
@ 2020-05-24 23:00   ` Johannes Schindelin
  2020-05-25  0:29     ` Johannes Schindelin
  0 siblings, 1 reply; 22+ messages in thread
From: Johannes Schindelin @ 2020-05-24 23:00 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

Hi Emily,

On Thu, 21 May 2020, Emily Shaffer wrote:

> diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
> index 4e46d7dd4e..3296d8af45 100755
> --- a/t/t1360-config-based-hooks.sh
> +++ b/t/t1360-config-based-hooks.sh
> @@ -55,4 +55,15 @@ test_expect_success 'git hook list reorders on duplicate commands' '
>  	test_cmp expected actual
>  '
>
> +test_expect_success 'git hook list --porcelain prints just the command' '
> +	cat >expected <<-\EOF &&
> +	/path/ghi
> +	/path/abc
> +	/path/def
> +	EOF
> +
> +	git hook list --porcelain pre-commit >actual &&
> +	test_cmp expected actual
> +'

As you surely found out from the GitHub workflow running in your fork,
this does not work on Windows. I need this (and strongly suggest you
squash that into your patch):

-- snipsnap --
From 97e3dfa6155785363c881ce2dcaf4f5ddead83ed Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mon, 25 May 2020 15:04:24 +0200
Subject: [PATCH] fixup??? hook: add --porcelain to list command

This is required to let the test pass on Windows, where Git reports
Windows-style absolute paths and has no idea about the pseudo Unix
absolute paths that the Bash knows about.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1360-config-based-hooks.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
index c862655fd4d9..fce7335e97b9 100755
--- a/t/t1360-config-based-hooks.sh
+++ b/t/t1360-config-based-hooks.sh
@@ -65,10 +65,10 @@ test_expect_success 'git hook list reorders on duplicate commands' '
 '

 test_expect_success 'git hook list --porcelain prints just the command' '
-	cat >expected <<-EOF &&
-	$ROOT/path/ghi
-	$ROOT/path/abc
-	$ROOT/path/def
+	cat >expected <<-\EOF &&
+	/path/ghi
+	/path/abc
+	/path/def
 	EOF

 	git hook list --porcelain pre-commit >actual &&
--
2.27.0.rc1.windows.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 4/4] hook: add --porcelain to list command
  2020-05-24 23:00   ` Johannes Schindelin
@ 2020-05-25  0:29     ` Johannes Schindelin
  0 siblings, 0 replies; 22+ messages in thread
From: Johannes Schindelin @ 2020-05-25  0:29 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

Hi Emily,

On Mon, 25 May 2020, Johannes Schindelin wrote:

> Hi Emily,
>
> On Thu, 21 May 2020, Emily Shaffer wrote:
>
> > diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
> > index 4e46d7dd4e..3296d8af45 100755
> > --- a/t/t1360-config-based-hooks.sh
> > +++ b/t/t1360-config-based-hooks.sh
> > @@ -55,4 +55,15 @@ test_expect_success 'git hook list reorders on duplicate commands' '
> >  	test_cmp expected actual
> >  '
> >
> > +test_expect_success 'git hook list --porcelain prints just the command' '
> > +	cat >expected <<-\EOF &&
> > +	/path/ghi
> > +	/path/abc
> > +	/path/def
> > +	EOF
> > +
> > +	git hook list --porcelain pre-commit >actual &&
> > +	test_cmp expected actual
> > +'
>
> As you surely found out from the GitHub workflow running in your fork,
> this does not work on Windows. I need this (and strongly suggest you
> squash that into your patch):
>
> -- snipsnap --
> From 97e3dfa6155785363c881ce2dcaf4f5ddead83ed Mon Sep 17 00:00:00 2001
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Date: Mon, 25 May 2020 15:04:24 +0200
> Subject: [PATCH] fixup??? hook: add --porcelain to list command
>
> This is required to let the test pass on Windows, where Git reports
> Windows-style absolute paths and has no idea about the pseudo Unix
> absolute paths that the Bash knows about.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t1360-config-based-hooks.sh | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
> index c862655fd4d9..fce7335e97b9 100755
> --- a/t/t1360-config-based-hooks.sh
> +++ b/t/t1360-config-based-hooks.sh
> @@ -65,10 +65,10 @@ test_expect_success 'git hook list reorders on duplicate commands' '
>  '
>
>  test_expect_success 'git hook list --porcelain prints just the command' '
> -	cat >expected <<-EOF &&
> -	$ROOT/path/ghi
> -	$ROOT/path/abc
> -	$ROOT/path/def
> +	cat >expected <<-\EOF &&
> +	/path/ghi
> +	/path/abc
> +	/path/def

Due to an oversight on my part, this is actually the _reverse_ diff, and
the corresponding part in my mail answering your PATCH 3/4 should be
skipped from that fixup. Sorry for that.

Ciao,
Dscho

>  	EOF
>
>  	git hook list --porcelain pre-commit >actual &&
> --
> 2.27.0.rc1.windows.1
>
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 3/4] hook: add list command
  2020-05-24 23:00   ` Johannes Schindelin
@ 2020-05-27 23:37     ` Emily Shaffer
  0 siblings, 0 replies; 22+ messages in thread
From: Emily Shaffer @ 2020-05-27 23:37 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git

On Mon, May 25, 2020 at 01:00:03AM +0200, Johannes Schindelin wrote:
> cc: git@vger.kernel.org
> 
> Hi Emily,
> 
> On Thu, 21 May 2020, Emily Shaffer wrote:
> 
> > diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
> > index 34b0df5216..4e46d7dd4e 100755
> > --- a/t/t1360-config-based-hooks.sh
> > +++ b/t/t1360-config-based-hooks.sh
> > @@ -4,8 +4,55 @@ test_description='config-managed multihooks, including git-hook command'
> >
> >  . ./test-lib.sh
> >
> > -test_expect_success 'git hook command does not crash' '
> > -	git hook
> > +test_expect_success 'git hook rejects commands without a mode' '
> > +	test_must_fail git hook pre-commit
> > +'
> > +
> > +
> > +test_expect_success 'git hook rejects commands without a hookname' '
> > +	test_must_fail git hook list
> > +'
> > +
> > +test_expect_success 'setup hooks in global, and local' '
> > +	git config --add --local hook.pre-commit.command "/path/ghi" &&
> > +	git config --add --global hook.pre-commit.command "/path/def"
> > +'
> > +
> > +test_expect_success 'git hook list orders by config order' '
> > +	cat >expected <<-\EOF &&
> > +	global:	/path/def
> > +	local:	/path/ghi
> > +	EOF
> > +
> > +	git hook list pre-commit >actual &&
> > +	test_cmp expected actual
> 
> This, as well as the next two test cases, won't work on Windows, as you
> almost certainly realized from looking at the failed GitHub workflow run
> of your branch.

Thanks very much for sending this - to be honest, the failed workflow
run appeared to be because of the earlier SDK download issue, which I
have not rebased on top of a fix for yet, so I missed any actionable
failures when I ran the CI locally. I'll take it into account, much
appreciated.

 - Emily

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/4] doc: propose hooks managed by the config
  2020-05-22 10:13   ` Phillip Wood
@ 2020-06-09 20:26     ` Emily Shaffer
  0 siblings, 0 replies; 22+ messages in thread
From: Emily Shaffer @ 2020-06-09 20:26 UTC (permalink / raw)
  To: Phillip Wood; +Cc: git

On Fri, May 22, 2020 at 11:13:07AM +0100, Phillip Wood wrote:
> 
> Hi Emily
> 
> Thanks for working on this
> 
> On 21/05/2020 19:54, Emily Shaffer wrote:
> > Begin a design document for config-based hooks, managed via git-hook.
> > Focus on an overview of the implementation and motivation for design
> > decisions. Briefly discuss the alternatives considered before this
> > point. Also, attempt to redefine terms to fit into a multihook world.
> > 
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> >  Documentation/Makefile                        |   1 +
> >  .../technical/config-based-hooks.txt          | 320 ++++++++++++++++++
> >  2 files changed, 321 insertions(+)
> >  create mode 100644 Documentation/technical/config-based-hooks.txt
> > 
> > diff --git a/Documentation/Makefile b/Documentation/Makefile
> > index 15d9d04f31..5b21f31d31 100644
> > --- a/Documentation/Makefile
> > +++ b/Documentation/Makefile
> > @@ -80,6 +80,7 @@ SP_ARTICLES += $(API_DOCS)
> >  TECH_DOCS += MyFirstContribution
> >  TECH_DOCS += MyFirstObjectWalk
> >  TECH_DOCS += SubmittingPatches
> > +TECH_DOCS += technical/config-based-hooks
> >  TECH_DOCS += technical/hash-function-transition
> >  TECH_DOCS += technical/http-protocol
> >  TECH_DOCS += technical/index-format
> > diff --git a/Documentation/technical/config-based-hooks.txt b/Documentation/technical/config-based-hooks.txt
> > new file mode 100644
> > index 0000000000..59cdc25a47
> > --- /dev/null
> > +++ b/Documentation/technical/config-based-hooks.txt
> > @@ -0,0 +1,320 @@
> > +Configuration-based hook management
> > +===================================
> > +
> > +== Motivation
> > +
> > +Treat hooks as a first-class citizen by replacing the .git/hook/hookname path as
> > +the only source of hooks to execute, in a way which is friendly to users with
> > +multiple repos which have similar needs.
> > +
> > +Redefine "hook" as an event rather than a single script, allowing users to
> > +perform unrelated actions on a single event.
> > +
> > +Take a step closer to safety when copying zipped Git repositories from untrusted
> > +users.
> 
> Having read through this (admittedly fairly quickly) I'm not sure what
> that step is

Ok, I'll try to clarify it a little here.

> 
> > +
> > +Make it easier for users to discover Git's hook feature and automate their
> > +workflows.
> > +
> > +== User interfaces
> > +
> > +=== Config schema
> > +
> > +Hooks can be introduced by editing the configuration manually. There are two new
> > +sections added, `hook` and `hookcmd`.
> > +
> > +==== `hook`
> > +
> > +Primarily contains subsections for each hook event. These subsections define
> > +hook command execution order;
> 
> May be "The order of these subsections define the hook command execution
> order" ?

Nice. Took it verbatim.

> 
> > hook commands can be specified by passing the
> > +command directly if no additional configuration is needed, or by passing the
> > +name of a `hookcmd`.
> 
> I know what you mean by "passing" but as this section is talking about
> config settings perhaps it should refer to the keys and values.

Sure.

> 
> > If Git does not find a `hookcmd` whose subsection matches
> > +the value of the given command string, Git will try to execute the string
> > +directly. Hooks are executed by passing the resolved command string to the
> > +shell.
> 
> Do we really need to invoke the shell just to split a command-line and
> look up the command in $PATH? If we used split_commandline() in alias.c
> then we could avoid invoking this extra process for each hook command.

I'll want to experiment a little bit with this and figure out what works
best - you may be right, and I could also be wrong about platform
compatibility doing it the way I described. I haven't written this bit
yet - so I'd like to update this section of the design doc when I get to
the implementation, so that it matches.

> 
> > Hook event subsections can also contain per-hook-event settings.
> > +
> > +Also contains top-level hook execution settings, for example,
> > +`hook.warnHookDir`, `hook.runHookDir`, or `hook.disableAll`.
> 
> (see sections ...) ? for the forward references to these settings?

Sure. I think the best way to do this is if I use anchors for all the
sections; this works without me specifying it in Asciidoctor but needs
to be explicitly specified in Asciidoc. So I'll make sure to include
that with the next iteration.

> 
> > +
> > +----
> > +[hook "pre-commit"]
> > +  command = perl-linter
> > +  command = /usr/bin/git-secrets --pre-commit
> > +
> > +[hook "pre-applypatch"]
> > +  command = perl-linter
> > +  error = ignore
> > +
> > +[hook]
> > +  runHookDir = interactive
> > +----
> > +
> > +==== `hookcmd`
> > +
> > +Defines a hook command and its attributes, which will be used when a hook event
> > +occurs. Unqualified attributes are assumed to apply to this hook during all hook
> > +events, but event-specific attributes can also be supplied. The example runs
> > +`/usr/bin/lint-it --language=perl <args passed by Git>`, but for repos which
> > +include this config, the hook command will be skipped for all events to which
> > +it's normally subscribed _except_ `pre-commit`.
> > +
> > +----
> > +[hookcmd "perl-linter"]
> > +  command = /usr/bin/lint-it --language=perl
> > +  skip = true
> > +  pre-commit-skip = false
> > +----
> > +
> > +=== Command-line API
> > +
> > +Users should be able to view, reorder, and create hook commands via the command
> > +line. External tools should be able to view a list of hooks in the correct order
> > +to run.
> > +
> > +*`git hook list <hook-event>`*
> > +
> > +*`git hook list (--system|--global|--local|--worktree)`*
> > +
> > +*`git hook edit <hook-event>`*
> > +
> > +*`git hook add <hook-command> <hook-event> <options...>`*
> > +
> > +=== Hook editor
> > +
> > +The tool which is presented by `git hook edit <hook-command>`. Ideally, this
> > +tool should be easier to use than manually editing the config, and then produce
> > +a concise config afterwards. It may take a form similar to `git rebase
> > +--interactive`.
> 
> rebase -i is not necessarily an exemplar of user interface design, what
> sort of thing do you have in mind?

Thanks for patience on this - I didn't really have a clear idea before
when I wrote the doc because I don't have much expertise in user
interfaces. However, since then I worked with some UX experts here, so
I'll make a better writeup in the next iteration - I've got a much
clearer idea of how that should look, now.

> 
> > +
> > +== Implementation
> > +
> > +=== Library
> > +
> > +`hook.c` and `hook.h` are responsible for interacting with the config files. In
> > +the case when the code generating a hook event doesn't have special concerns
> > +about how to run the hooks, the hook library will provide a basic API to call
> > +all hooks in config order with an `argv_array` provided by the code which
> > +generates the hook event:
> > +
> > +*`int run_hooks(const char *hookname, struct argv_array *args)`*
> > +
> > +This call includes the hook command provided by `run-command.h:find_hook()`;
> > +eventually, this legacy hook will be gated by a config `hook.runHookDir`. The
> > +config is checked against a number of cases:
> > +
> > +- "no": the legacy hook will not be run
> > +- "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
> > +
> > +In case this list is expanded in the future, if a value for `hook.runHookDir` is
> > +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".
> > +
> > +If the caller wants to do something more complicated, the hook library can also
> > +provide a callback API:
> > +
> > +*`int for_each_hookcmd(const char *hookname, hookcmd_function *cb)`*
> > +
> > +Finally, to facilitate the builtin, the library will also provide the following
> > +APIs to interact with the config:
> > +
> > +----
> > +int set_hook_commands(const char *hookname, struct string_list *commands,
> > +	enum config_scope scope);
> > +int set_hookcmd(const char *hookcmd, struct hookcmd options);
> > +
> > +int list_hook_commands(const char *hookname, struct string_list *commands);
> > +int list_hooks_in_scope(enum config_scope scope, struct string_list *commands);
> > +----
> > +
> > +`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
> > +config schema, `struct hookcmd` should contain logical values instead of string
> > +pairs.
> > +
> > +----
> > +struct hookcmd {
> > +  const char *name;
> > +  const char *command;
> > +
> > +  /* for illustration only; not planned at present */
> > +  int parallelizable;
> > +  const char *hookcmd_before;
> > +  const char *hookcmd_after;
> > +  enum recovery_action on_fail;
> > +}
> > +----
> > +
> > +=== Builtin
> > +
> > +`builtin/hook.c` is responsible for providing the frontend. It's responsible for
> > +formatting user-provided data and then calling the library API to set the
> > +configs as appropriate. The builtin frontend is not responsible for calling the
> > +config directly, so that other areas of Git can rely on the hook library to
> > +understand the most recent config schema for hooks.
> > +
> > +=== Migration path
> > +
> > +==== Stage 0
> > +
> > +Hooks are called by running `run-command.h:find_hook()` with the hookname and
> > +executing the result. The hook library and builtin do not exist. Hooks only
> > +exist as specially named scripts within `.git/hooks/`.
> > +
> > +==== Stage 1
> > +
> > +`git hook list --porcelain <hook-event>` is implemented. Users can replace their
> > +`.git/hooks/<hook-event>` scripts with a trampoline based on `git hook list`'s
> > +output. Modifier commands like `git hook add` and `git hook edit` can be
> > +implemented around this time as well.
> > +
> > +==== Stage 2
> > +
> > +`hook.h:run_hooks()` is taught to include `run-command.h:find_hook()` at the
> > +end; calls to `find_hook()` are replaced with calls to `run_hooks()`. Users can
> > +opt-in to config-based hooks simply by creating some in their config; otherwise
> > +users should remain unaffected by the change.
> > +
> > +==== Stage 3
> > +
> > +The call to `find_hook()` inside of `run_hooks()` learns to check for a config,
> > +`hook.runHookDir`. Users can opt into managing their hooks completely via the
> > +config this way.
> > +
> > +==== Stage 4
> > +
> > +`.git/hooks` is removed from the template and the hook directory is considered
> > +deprecated. To avoid breaking older repos, the default of `hook.runHookDir` is
> > +not changed, and `find_hook()` is not removed.
> > +
> > +== Caveats
> > +
> > +=== 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
> > +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.
> > +
> > +=== Ease of use
> > +
> > +The config schema is nontrivial; that's why it's important for the `git hook`
> > +modifier commands to be usable.
> 
> That's an important point
> 
> > Contributors with UX expertise are encouraged to
> > +share their suggestions.
> > +
> > +== Alternative approaches
> > +
> > +A previous summary of alternatives exists in the
> > +archives.footnote:[https://lore.kernel.org/git/20191116011125.GG22855@google.com]
> > +
> > +=== 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
> > +
> > +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 table
> > +
> > +.Comparison of alternatives
> > +|===
> > +|Feature |Config-based hooks |Hook directories |Status quo
> > +
> > +|Supports multiple hooks
> > +|Natively
> > +|Natively
> > +|With user effort
> > +
> > +|Safer for zipped repos
> > +|A little
> > +|No
> > +|No
> > +
> > +|Previous hooks just work
> > +|If configured
> > +|Yes
> > +|Yes
> > +
> > +|Can install one hook to many repos
> > +|Yes
> > +|No
> > +|No
> > +
> > +|Discoverability
> > +|Better (in `git help git`)
> > +|Same as before
> > +|Same as before
> > +
> > +|Hard to run unexpected hook
> > +|If configured
> > +|No
> > +|No
> > +|===
> > +
> > +== Future work
> > +
> > +=== Execution ordering
> > +
> > +We may find that config order is insufficient for some users; for example,
> > +config order makes it difficult to add a new hook to the system or global config
> > +which runs at the end of the hook list. A new ordering schema should be:
> > +
> > +1) Specified by a `hook.order` config, so that users will not unexpectedly see
> > +their order change;
> > +
> > +2) Either dependency or numerically based.
> > +
> > +Dependency-based ordering is prone to classic linked-list problems, like a
> > +cycles and handling of missing dependencies. But, it paves the way for enabling
> > +parallelization if some tasks truly depend on others.
> > +
> > +Numerical ordering makes it tricky for Git to generate suggested ordering
> > +numbers for each command, but is easy to determine a definitive order.
> > +
> > +=== Parallelization
> > +
> > +Users with many hooks might want to run them simultaneously, if the hooks don't
> > +modify state; if one hook depends on another's output, then users will want to
> > +specify those 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.
> > +
> > +=== Securing hookdir hooks
> > +
> > +With the design as written in this doc, it's still possible for a malicious user
> > +to modify `.git/config` to include `hook.pre-receive.command = rm -rf /`, then
> > +zip their repo and send it to another user. It may be necessary to teach Git to
> > +only allow one-line hooks like this if they were configured outside of the local
> > +scope;
> 
> Does "disabling one-line hooks" mean "disable passing command line
> arguments to the hook"? I'm not sure that gains much security - can't I
> just set 'hook.pre-receive.command = ./delete-everything' and include
> delete-everything in my malicious repo?

No, I meant something more along the lines of:

- hookcmds cannot be specified at the repo/worktree level
- hook.pre-receive.command's value *must* be a hookcmd name

I'll try to make that more clear next round.

Thanks for reading.
 - Emily

> > or another approach, like a list of safe projects, might be useful. It
> > +may also be sufficient (or at least useful) to teach a `hook.disableAll` config
> > +or similar flag to the Git executable.
> > +
> > +=== Submodule inheritance
> > +
> > +It's possible some submodules may want to run the identical set of hooks that
> > +their superrepo runs. While a globally-configured hook set is helpful, it's not
> > +a great solution for users who have multiple repos-with-submodules under the
> > +same user. It would be useful for submodules to learn how to run hooks from
> > +their superrepo's config, or inherit that hook setting.
> > +
> > +== Glossary
> > +
> > +*hook event*
> > +
> > +A point during Git's execution where user scripts may be run, for example,
> > +_prepare-commit-msg_ or _pre-push_.
> > +
> > +*hook command*
> > +
> > +A user script or executable which will be run on one or more hook events.
> > 
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 3/4] hook: add list command
  2020-05-22 10:27   ` Phillip Wood
@ 2020-06-09 21:49     ` Emily Shaffer
  0 siblings, 0 replies; 22+ messages in thread
From: Emily Shaffer @ 2020-06-09 21:49 UTC (permalink / raw)
  To: Phillip Wood; +Cc: git

On Fri, May 22, 2020 at 11:27:44AM +0100, Phillip Wood wrote:
> 
> Hi Emily
> 
> On 21/05/2020 19:54, Emily Shaffer wrote:
> > Teach 'git hook list <hookname>', which checks the known configs in
> > order to create an ordered list of hooks to run on a given hook event.
> > 
> > Multiple commands can be specified for a given hook by providing
> > multiple "hook.<hookname>.command = <path-to-hook>" lines. Hooks will be
> > 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.<hookcmd-name>.command = <path-to-hook>" line.
> > 
> > For example:
> > 
> >   $ git config --list | grep ^hook
> >   hook.pre-commit.command=baz
> >   hook.pre-commit.command=~/bar.sh
> >   hookcmd.baz.command=~/baz/from/hookcmd.sh
> > 
> >   $ git hook list pre-commit
> >   ~/baz/from/hookcmd.sh
> >   ~/bar.sh
> > 
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> >  Documentation/git-hook.txt    | 37 +++++++++++++-
> >  Makefile                      |  1 +
> >  builtin/hook.c                | 55 +++++++++++++++++++--
> >  hook.c                        | 90 +++++++++++++++++++++++++++++++++++
> >  hook.h                        | 15 ++++++
> >  t/t1360-config-based-hooks.sh | 51 +++++++++++++++++++-
> >  6 files changed, 242 insertions(+), 7 deletions(-)
> >  create mode 100644 hook.c
> >  create mode 100644 hook.h
> > 
> > diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
> > index 2d50c414cc..e458586e96 100644
> > --- a/Documentation/git-hook.txt
> > +++ b/Documentation/git-hook.txt
> > @@ -8,12 +8,47 @@ git-hook - Manage configured hooks
> >  SYNOPSIS
> >  --------
> >  [verse]
> > -'git hook'
> > +'git hook' list <hook-name>
> >  
> >  DESCRIPTION
> >  -----------
> >  You can list, add, and modify hooks with this command.
> >  
> > +This command parses the default configuration files for sections "hook" and
> > +"hookcmd". "hook" is used to describe the commands which will be run during a
> > +particular hook event; commands are run in config order. "hookcmd" is used to
> > +describe attributes of a specific command. If additional attributes don't need
> > +to be specified, a command to run can be specified directly in the "hook"
> > +section; if a "hookcmd" by that name isn't found, Git will attempt to run the
> > +provided value directly. For example:
> > +
> > +Global config
> > +----
> > +  [hook "post-commit"]
> > +    command = "linter"
> > +    command = "~/typocheck.sh"
> > +
> > +  [hookcmd "linter"]
> > +    command = "/bin/linter --c"
> > +----
> > +
> > +Local config
> > +----
> > +  [hook "prepare-commit-msg"]
> > +    command = "linter"
> > +  [hook "post-commit"]
> > +    command = "python ~/run-test-suite.py"
> > +----
> > +
> > +COMMANDS
> > +--------
> > +
> > +list <hook-name>::
> > +
> > +List the hooks which have been configured for <hook-name>. Hooks appear
> > +in the order they should be run, and note the config scope where the relevant
> > +`hook.<hook-name>.command` was specified, not the `hookcmd` (if applicable).
> > +
> >  GIT
> >  ---
> >  Part of the linkgit:git[1] suite
> > diff --git a/Makefile b/Makefile
> > index fce6ee154e..b7bbf3be7b 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -894,6 +894,7 @@ LIB_OBJS += grep.o
> >  LIB_OBJS += hashmap.o
> >  LIB_OBJS += help.o
> >  LIB_OBJS += hex.o
> > +LIB_OBJS += hook.o
> >  LIB_OBJS += ident.o
> >  LIB_OBJS += interdiff.o
> >  LIB_OBJS += json-writer.o
> > diff --git a/builtin/hook.c b/builtin/hook.c
> > index b2bbc84d4d..cfd8e388bd 100644
> > --- a/builtin/hook.c
> > +++ b/builtin/hook.c
> > @@ -1,21 +1,68 @@
> >  #include "cache.h"
> >  
> >  #include "builtin.h"
> > +#include "config.h"
> > +#include "hook.h"
> >  #include "parse-options.h"
> > +#include "strbuf.h"
> >  
> >  static const char * const builtin_hook_usage[] = {
> > -	N_("git hook"),
> > +	N_("git hook list <hookname>"),
> >  	NULL
> >  };
> >  
> > -int cmd_hook(int argc, const char **argv, const char *prefix)
> > +static int list(int argc, const char **argv, const char *prefix)
> >  {
> > -	struct option builtin_hook_options[] = {
> > +	struct list_head *head, *pos;
> > +	struct hook *item;
> > +	struct strbuf hookname = STRBUF_INIT;
> > +
> > +	struct option list_options[] = {
> >  		OPT_END(),
> >  	};
> >  
> > -	argc = parse_options(argc, argv, prefix, builtin_hook_options,
> > +	argc = parse_options(argc, argv, prefix, list_options,
> >  			     builtin_hook_usage, 0);
> >  
> > +	if (argc < 1) {
> > +		usage_msg_opt("a hookname must be provided to operate on.",
> > +			      builtin_hook_usage, list_options);
> > +	}
> > +
> > +	strbuf_addstr(&hookname, argv[0]);
> > +
> > +	head = hook_list(&hookname);
> > +
> > +	if (!head) {
> > +		printf(_("no commands configured for hook '%s'\n"),
> > +		       hookname.buf);
> > +		return 0;
> > +	}
> > +
> > +	list_for_each(pos, head) {
> > +		item = list_entry(pos, struct hook, list);
> > +		if (item)
> > +			printf("%s:\t%s\n",
> > +			       config_scope_name(item->origin),
> > +			       item->command.buf);
> > +	}
> > +
> > +	clear_hook_list();
> > +	strbuf_release(&hookname);
> > +
> >  	return 0;
> >  }
> > +
> > +int cmd_hook(int argc, const char **argv, const char *prefix)
> > +{
> > +	struct option builtin_hook_options[] = {
> > +		OPT_END(),
> > +	};
> > +	if (argc < 2)
> > +		usage_with_options(builtin_hook_usage, builtin_hook_options);
> > +
> > +	if (!strcmp(argv[1], "list"))
> > +		return list(argc - 1, argv + 1, prefix);
> > +
> > +	usage_with_options(builtin_hook_usage, builtin_hook_options);
> > +}
> > diff --git a/hook.c b/hook.c
> > new file mode 100644
> > index 0000000000..9dfc1a885e
> > --- /dev/null
> > +++ b/hook.c
> > @@ -0,0 +1,90 @@
> > +#include "cache.h"
> > +
> > +#include "hook.h"
> > +#include "config.h"
> > +
> > +static LIST_HEAD(hook_head);
> > +
> > +void free_hook(struct hook *ptr)
> > +{
> > +	if (ptr) {
> > +		strbuf_release(&ptr->command);
> > +		free(ptr);
> > +	}
> > +}
> > +
> > +static void emplace_hook(struct list_head *pos, const char *command)
> > +{
> > +	struct hook *to_add = malloc(sizeof(struct hook));
> > +	to_add->origin = current_config_scope();
> > +	strbuf_init(&to_add->command, 0);
> > +	strbuf_addstr(&to_add->command, command);
> > +
> > +	list_add_tail(&to_add->list, pos);
> > +}
> > +
> > +static void remove_hook(struct list_head *to_remove)
> > +{
> > +	struct hook *hook_to_remove = list_entry(to_remove, struct hook, list);
> > +	list_del(to_remove);
> > +	free_hook(hook_to_remove);
> > +}
> > +
> > +void clear_hook_list(void)
> > +{
> > +	struct list_head *pos, *tmp;
> > +	list_for_each_safe(pos, tmp, &hook_head)
> > +		remove_hook(pos);
> > +}
> > +
> > +static int hook_config_lookup(const char *key, const char *value, void *hook_key_cb)
> > +{
> > +	const char *hook_key = hook_key_cb;
> > +
> > +	if (!strcmp(key, hook_key)) {
> > +		const char *command = value;
> > +		struct strbuf hookcmd_name = STRBUF_INIT;
> > +		struct list_head *pos = NULL, *tmp = NULL;
> > +
> > +		/* Check if a hookcmd with that name exists. */
> > +		strbuf_addf(&hookcmd_name, "hookcmd.%s.command", command);
> > +		git_config_get_value(hookcmd_name.buf, &command);
> 
> This looks dodgy to me. This code is called by git_config() as it parses
> the config files, so it has not had a chance to fully populate the
> config cache used by git_config_get_value(). I think the test below
> passes because the hookcmd setting is set in the global file and the
> hook setting is set in the local file so when we have already parsed the
> hookcmd setting when we come to look it up. The same comment applies to
> the hypothetical ordering config mentioned below. I think it would be
> better to collect the list of hook.<event>.command settings in this
> callback and then look up any hookcmd settings for those hook commands
> after we've finished reading all of the config files.

git_config_get_value() calls repo_read_config(the_repository) if the
config hasn't been fully parsed yet, so I think what you're worrying
about is not an issue. It's ugly, I agree, but since the new hotness
(git_config_get_value() and friends) doesn't offer the same
functionality as the old solution (config origin) this seemed like an
okay approach. As I understand it, moving this hookcmd lookup section
outside of the config callback will save us up to one additional pass
through the configs, at the expense of a more convoluted code path.

> 
> > +
> > +		if (!command)
> > +			BUG("git_config_get_value overwrote a string it shouldn't have");
> > +
> > +		/*
> > +		 * TODO: implement an option-getting callback, e.g.
> > +		 *   get configs by pattern hookcmd.$value.*
> > +		 *   for each key+value, do_callback(key, value, cb_data)
> > +		 */
> > +
> > +		list_for_each_safe(pos, tmp, &hook_head) {
> > +			struct hook *hook = list_entry(pos, struct hook, list);
> > +			/*
> > +			 * The list of hooks to run can be reordered by being redeclared
> > +			 * in the config. Options about hook ordering should be checked
> > +			 * here.
> > +			 */
> > +			if (0 == strcmp(hook->command.buf, command))
> > +				remove_hook(pos);
> > +		}
> > +		emplace_hook(pos, command);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +struct list_head* hook_list(const struct strbuf* hookname)
> > +{
> > +	struct strbuf hook_key = STRBUF_INIT;
> > +
> > +	if (!hookname)
> > +		return NULL;
> > +
> > +	strbuf_addf(&hook_key, "hook.%s.command", hookname->buf);
> > +
> > +	git_config(hook_config_lookup, (void*)hook_key.buf);
> > +
> > +	return &hook_head;
> > +}
> > diff --git a/hook.h b/hook.h
> > new file mode 100644
> > index 0000000000..aaf6511cff
> > --- /dev/null
> > +++ b/hook.h
> > @@ -0,0 +1,15 @@
> > +#include "config.h"
> > +#include "list.h"
> > +#include "strbuf.h"
> > +
> > +struct hook
> > +{
> > +	struct list_head list;
> > +	enum config_scope origin;
> > +	struct strbuf command;
> > +};
> > +
> > +struct list_head* hook_list(const struct strbuf *hookname);
> > +
> > +void free_hook(struct hook *ptr);
> > +void clear_hook_list(void);
> > diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
> > index 34b0df5216..4e46d7dd4e 100755
> > --- a/t/t1360-config-based-hooks.sh
> > +++ b/t/t1360-config-based-hooks.sh
> > @@ -4,8 +4,55 @@ test_description='config-managed multihooks, including git-hook command'
> >  
> >  . ./test-lib.sh
> >  
> > -test_expect_success 'git hook command does not crash' '
> > -	git hook
> > +test_expect_success 'git hook rejects commands without a mode' '
> > +	test_must_fail git hook pre-commit
> > +'
> > +
> > +
> > +test_expect_success 'git hook rejects commands without a hookname' '
> > +	test_must_fail git hook list
> > +'
> > +
> > +test_expect_success 'setup hooks in global, and local' '
> > +	git config --add --local hook.pre-commit.command "/path/ghi" &&
> 
> Can I make a plea for the use of test_config please. Writing tests which
> rely on previous tests for their set-up creates a chain of hidden
> dependencies that make it hard to add/alter tests later or run a subset
> of the tests when developing a new patch. t3404-rebase-interactive.sh is
> a prime example of this and I dread touching it.

Sure. I'll redo them.

> 
> > +	git config --add --global hook.pre-commit.command "/path/def"
> > +'
> > +
> > +test_expect_success 'git hook list orders by config order' '
> > +	cat >expected <<-\EOF &&
> > +	global:	/path/def
> > +	local:	/path/ghi
> > +	EOF
> > +
> > +	git hook list pre-commit >actual &&
> > +	test_cmp expected actual
> > +'
> > +
> > +test_expect_success 'git hook list dereferences a hookcmd' '
> > +	git config --add --local hook.pre-commit.command "abc" &&
> > +	git config --add --global hookcmd.abc.command "/path/abc" &&
> > +
> > +	cat >expected <<-\EOF &&
> > +	global:	/path/def
> > +	local:	/path/ghi
> > +	local:	/path/abc
> 
> We should make it clear in the documentation that the config origin
> applies to the hook setting, even though we display the hookcmd command
> which is set globally here for the last hook.

One of the suggestions from our UX team last week was to make this list
output clearer to indicate the origin of the command plus the origin of
the hookcmd object; I'll try to straighten this out and make sure the
doc agrees.

 - Emily

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 0/6] propose config-based hooks
  2020-05-21 18:54 [PATCH v2 0/4] propose config-based hooks Emily Shaffer
                   ` (3 preceding siblings ...)
  2020-05-21 18:54 ` [PATCH v2 4/4] hook: add --porcelain to " Emily Shaffer
@ 2020-07-28 22:24 ` Emily Shaffer
  2020-07-28 22:24   ` [PATCH v3 1/6] doc: propose hooks managed by the config Emily Shaffer
                     ` (5 more replies)
  4 siblings, 6 replies; 22+ messages in thread
From: Emily Shaffer @ 2020-07-28 22:24 UTC (permalink / raw)
  To: git
  Cc: Emily Shaffer, Jeff King, Junio C Hamano, James Ramsay,
	Jonathan Nieder, brian m. carlson,
	Ævar Arnfjörð Bjarmason, Phillip Wood,
	Josh Steadmon, Johannes Schindelin

Hi all,

After taking a few weeks to work on other items, I've got another update
to the config-based hook series. Patches 5 and 6 are RFC - a sketch of
how the hook library could run the appropriate set of hooks. There's
more work to do, which I'll outline later in the cover letter.

Since last time, I took into account review comments, including Dscho's
fixups to make the tests work in Windows. It seems those tests are
passing now, according to the GH Actions run:
https://github.com/nasamuffin/git/actions/runs/186242637

One thing I didn't decide on was the benefit of moving the hookcmd
resolution outside of the hook config pass; that code is unchanged. I
still haven't decided quite which approach I like better, but it's still
on my mind.

In the 'run_hook()' implementation I flipped the 'use_shell' bit, which
by my understanding only uses a shell if it can't find the command in
PATH; this seems like a reasonable approach especially because the code
is so brief, but I'm interested in hearing why I'm wrong or it won't
work well :)

There is still some work I've got locally which isn't quite ready:
 - support for hook.runHookDir. This is turning into a yak shave about
   who decides where and when to display or run the hookdir hook. I
   think I've got it mostly figured out and there's a patch locally, but
   it's not polished.
 - Drafts for 'git hook add' and 'git hook edit'. These features are
   probably the most complicated part of the series, but it's possible
   to use config-based hooks without them. In the interest of getting
   something out for people to try on their own, I'll probably leave
   these for later.
 - Support for stdin redirection to hooks. Since this means we want to
   point the same stdin to multiple processes, I'm thinking it will be
   slightly complicated. Maybe someone has a hint for me? :) Without
   having looked at what's available or not yet, I'm planning to do this
   by reading the whole stdin to memory and then streaming it to each
   process in turn, as I can't seek back to the beginning of the stream
   when I start each new process.
 - Conversion of codebase to use the hook library instead. Partly, this
   is gated on the previous point - there are plenty of callers who,
   instead of using run-command's run_hook_*(), just use find_hook() and
   roll their own struct child_process so they can use stdin/stdout. I
   do plan to consider the hook lib's run_hooks() implementation as
   non-final until I start this process - I'm expecting to learn more
   about what I do and don't have to support when I do this.

Thanks, all. Hopefully I can do better than a 2-month wait for the
series after this one... although I imagine I cursed myself by saying
that. :)

 - Emily


Emily Shaffer (6):
  doc: propose hooks managed by the config
  hook: scaffolding for git-hook subcommand
  hook: add list command
  hook: add --porcelain to list command
  parse-options: parse into argv_array
  hook: add 'run' subcommand

 .gitignore                                    |   1 +
 Documentation/Makefile                        |   1 +
 Documentation/git-hook.txt                    |  63 ++++
 Documentation/technical/api-parse-options.txt |   5 +
 .../technical/config-based-hooks.txt          | 354 ++++++++++++++++++
 Makefile                                      |   2 +
 builtin.h                                     |   1 +
 builtin/hook.c                                | 107 ++++++
 git.c                                         |   1 +
 hook.c                                        | 132 +++++++
 hook.h                                        |  18 +
 parse-options-cb.c                            |  16 +
 parse-options.h                               |   4 +
 t/t1360-config-based-hooks.sh                 | 115 ++++++
 14 files changed, 820 insertions(+)
 create mode 100644 Documentation/git-hook.txt
 create mode 100644 Documentation/technical/config-based-hooks.txt
 create mode 100644 builtin/hook.c
 create mode 100644 hook.c
 create mode 100644 hook.h
 create mode 100755 t/t1360-config-based-hooks.sh

-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 1/6] doc: propose hooks managed by the config
  2020-07-28 22:24 ` [PATCH v3 0/6] propose config-based hooks Emily Shaffer
@ 2020-07-28 22:24   ` Emily Shaffer
  2020-07-28 22:24   ` [PATCH v3 2/6] hook: scaffolding for git-hook subcommand Emily Shaffer
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Emily Shaffer @ 2020-07-28 22:24 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Begin a design document for config-based hooks, managed via git-hook.
Focus on an overview of the implementation and motivation for design
decisions. Briefly discuss the alternatives considered before this
point. Also, attempt to redefine terms to fit into a multihook world.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/Makefile                        |   1 +
 .../technical/config-based-hooks.txt          | 354 ++++++++++++++++++
 2 files changed, 355 insertions(+)
 create mode 100644 Documentation/technical/config-based-hooks.txt

diff --git a/Documentation/Makefile b/Documentation/Makefile
index ecd0b340b1..5483995113 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -80,6 +80,7 @@ SP_ARTICLES += $(API_DOCS)
 TECH_DOCS += MyFirstContribution
 TECH_DOCS += MyFirstObjectWalk
 TECH_DOCS += SubmittingPatches
+TECH_DOCS += technical/config-based-hooks
 TECH_DOCS += technical/hash-function-transition
 TECH_DOCS += technical/http-protocol
 TECH_DOCS += technical/index-format
diff --git a/Documentation/technical/config-based-hooks.txt b/Documentation/technical/config-based-hooks.txt
new file mode 100644
index 0000000000..c6e762b192
--- /dev/null
+++ b/Documentation/technical/config-based-hooks.txt
@@ -0,0 +1,354 @@
+Configuration-based hook management
+===================================
+:sectanchors:
+
+[[motivation]]
+== Motivation
+
+Treat hooks as a first-class citizen by replacing the .git/hook/hookname path as
+the only source of hooks to execute, in a way which is friendly to users with
+multiple repos which have similar needs.
+
+Redefine "hook" as an event rather than a single script, allowing users to
+perform 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.
+
+[[user-interfaces]]
+== User interfaces
+
+[[config-schema]]
+=== Config schema
+
+Hooks can be introduced by editing the configuration manually. There are two new
+sections added, `hook` and `hookcmd`.
+
+[[config-schema-hook]]
+==== `hook`
+
+Primarily contains subsections for each hook event. These order of these
+subsections defines the hook command execution order; hook commands can be
+specified by setting the value directly to the command if no additional
+configuration is needed, or by setting the value as the name of a `hookcmd`. If
+Git does not find a `hookcmd` whose subsection matches the value of the given
+command string, Git will try to execute the string directly. Hooks are executed
+by passing the resolved command string to the shell. Hook event subsections can
+also contain per-hook-event settings.
+
+Also contains top-level hook execution settings, for example,
+`hook.warnHookDir`, `hook.runHookDir`, or `hook.disableAll`. (These settings are
+described more in <<library,Library>>.)
+
+----
+[hook "pre-commit"]
+  command = perl-linter
+  command = /usr/bin/git-secrets --pre-commit
+
+[hook "pre-applypatch"]
+  command = perl-linter
+  error = ignore
+
+[hook]
+  runHookDir = interactive
+----
+
+[[config-schema-hookcmd]]
+==== `hookcmd`
+
+Defines a hook command and its attributes, which will be used when a hook event
+occurs. Unqualified attributes are assumed to apply to this hook during all hook
+events, but event-specific attributes can also be supplied. The example runs
+`/usr/bin/lint-it --language=perl <args passed by Git>`, but for repos which
+include this config, the hook command will be skipped for all events to which
+it's normally subscribed _except_ `pre-commit`.
+
+----
+[hookcmd "perl-linter"]
+  command = /usr/bin/lint-it --language=perl
+  skip = true
+  pre-commit-skip = false
+----
+
+[[command-line-api]]
+=== Command-line API
+
+Users should be able to view, reorder, and create hook commands via the command
+line. External tools should be able to view a list of hooks in the correct order
+to run.
+
+*`git hook list <hook-event>`*
+
+*`git hook list (--system|--global|--local|--worktree)`*
+
+*`git hook edit <hook-event>`*
+
+*`git hook add <hook-command> <hook-event> <options...>`*
+
+[[hook-editor]]
+=== Hook editor
+
+The tool which is presented by `git hook edit <hook-command>`. Ideally, this
+tool should be easier to use than manually editing the config, and then produce
+a concise config afterwards. It may take a form similar to `git rebase
+--interactive`.
+
+[[implementation]]
+== Implementation
+
+[[library]]
+=== Library
+
+`hook.c` and `hook.h` are responsible for interacting with the config files. In
+the case when the code generating a hook event doesn't have special concerns
+about how to run the hooks, the hook library will provide a basic API to call
+all hooks in config order with an `argv_array` provided by the code which
+generates the hook event:
+
+*`int run_hooks(const char *hookname, struct argv_array *args)`*
+
+This call includes the hook command provided by `run-command.h:find_hook()`;
+eventually, this legacy hook will be gated by a config `hook.runHookDir`. The
+config is checked against a number of cases:
+
+- "no": the legacy hook will not be run
+- "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
+
+In case this list is expanded in the future, if a value for `hook.runHookDir` is
+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".
+
+If the caller wants to do something more complicated, the hook library can also
+provide a callback API:
+
+*`int for_each_hookcmd(const char *hookname, hookcmd_function *cb)`*
+
+Finally, to facilitate the builtin, the library will also provide the following
+APIs to interact with the config:
+
+----
+int set_hook_commands(const char *hookname, struct string_list *commands,
+	enum config_scope scope);
+int set_hookcmd(const char *hookcmd, struct hookcmd options);
+
+int list_hook_commands(const char *hookname, struct string_list *commands);
+int list_hooks_in_scope(enum config_scope scope, struct string_list *commands);
+----
+
+`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
+config schema, `struct hookcmd` should contain logical values instead of string
+pairs.
+
+----
+struct hookcmd {
+  const char *name;
+  const char *command;
+
+  /* for illustration only; not planned at present */
+  int parallelizable;
+  const char *hookcmd_before;
+  const char *hookcmd_after;
+  enum recovery_action on_fail;
+}
+----
+
+[[builtin]]
+=== Builtin
+
+`builtin/hook.c` is responsible for providing the frontend. It's responsible for
+formatting user-provided data and then calling the library API to set the
+configs as appropriate. The builtin frontend is not responsible for calling the
+config directly, so that other areas of Git can rely on the hook library to
+understand the most recent config schema for hooks.
+
+[[migration]]
+=== Migration path
+
+[[stage-0]]
+==== Stage 0
+
+Hooks are called by running `run-command.h:find_hook()` with the hookname and
+executing the result. The hook library and builtin do not exist. Hooks only
+exist as specially named scripts within `.git/hooks/`.
+
+[[stage-1]]
+==== Stage 1
+
+`git hook list --porcelain <hook-event>` is implemented. Users can replace their
+`.git/hooks/<hook-event>` scripts with a trampoline based on `git hook list`'s
+output. Modifier commands like `git hook add` and `git hook edit` can be
+implemented around this time as well.
+
+[[stage-2]]
+==== Stage 2
+
+`hook.h:run_hooks()` is taught to include `run-command.h:find_hook()` at the
+end; calls to `find_hook()` are replaced with calls to `run_hooks()`. Users can
+opt-in to config-based hooks simply by creating some in their config; otherwise
+users should remain unaffected by the change.
+
+[[stage-3]]
+==== Stage 3
+
+The call to `find_hook()` inside of `run_hooks()` learns to check for a config,
+`hook.runHookDir`. Users can opt into managing their hooks completely via the
+config this way.
+
+[[stage-4]]
+==== Stage 4
+
+`.git/hooks` is removed from the template and the hook directory is considered
+deprecated. To avoid breaking older repos, the default of `hook.runHookDir` is
+not changed, and `find_hook()` is not removed.
+
+[[caveats]]
+== Caveats
+
+[[security]]
+=== 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
+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.
+
+[[ease-of-use]]
+=== Ease of use
+
+The config schema is nontrivial; that's why it's important for the `git hook`
+modifier commands to be usable. Contributors with UX expertise are encouraged to
+share their suggestions.
+
+[[alternatives]]
+== Alternative approaches
+
+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
+
+.Comparison of alternatives
+|===
+|Feature |Config-based hooks |Hook directories |Status quo
+
+|Supports multiple hooks
+|Natively
+|Natively
+|With user effort
+
+|Safer for zipped repos
+|A little
+|No
+|No
+
+|Previous hooks just work
+|If configured
+|Yes
+|Yes
+
+|Can install one hook to many repos
+|Yes
+|No
+|No
+
+|Discoverability
+|Better (in `git help git`)
+|Same as before
+|Same as before
+
+|Hard to run unexpected hook
+|If configured
+|No
+|No
+|===
+
+[[future-work]]
+== Future work
+
+[[execution-ordering]]
+=== Execution ordering
+
+We may find that config order is insufficient for some users; for example,
+config order makes it difficult to add a new hook to the system or global config
+which runs at the end of the hook list. A new ordering schema should be:
+
+1) Specified by a `hook.order` config, so that users will not unexpectedly see
+their order change;
+
+2) Either dependency or numerically based.
+
+Dependency-based ordering is prone to classic linked-list problems, like a
+cycles and handling of missing dependencies. But, it paves the way for enabling
+parallelization if some tasks truly depend on others.
+
+Numerical ordering makes it tricky for Git to generate suggested ordering
+numbers for each command, but is easy to determine a definitive order.
+
+[[parallelization]]
+=== Parallelization
+
+Users with many hooks might want to run them simultaneously, if the hooks don't
+modify state; if one hook depends on another's output, then users will want to
+specify those 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.
+
+[[securing-hookdir-hooks]]
+=== Securing hookdir hooks
+
+With the design as written in this doc, it's still possible for a malicious user
+to modify `.git/config` to include `hook.pre-receive.command = rm -rf /`, then
+zip their repo and send it to another user. It may be necessary to teach Git to
+only allow inlined hooks like this if they were configured outside of the local
+scope (in other words, only run hookcmds, and only allow hookcmds to be
+configured in global or system scope); or another approach, like a list of safe
+projects, might be useful. It may also be sufficient (or at least useful) to
+teach a `hook.disableAll` config or similar flag to the Git executable.
+
+[[submodule-inheritance]]
+=== Submodule inheritance
+
+It's possible some submodules may want to run the identical set of hooks that
+their superrepo runs. While a globally-configured hook set is helpful, it's not
+a great solution for users who have multiple repos-with-submodules under the
+same user. It would be useful for submodules to learn how to run hooks from
+their superrepo's config, or inherit that hook setting.
+
+[[glossary]]
+== Glossary
+
+*hook event*
+
+A point during Git's execution where user scripts may be run, for example,
+_prepare-commit-msg_ or _pre-push_.
+
+*hook command*
+
+A user script or executable which will be run on one or more hook events.
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 2/6] hook: scaffolding for git-hook subcommand
  2020-07-28 22:24 ` [PATCH v3 0/6] propose config-based hooks Emily Shaffer
  2020-07-28 22:24   ` [PATCH v3 1/6] doc: propose hooks managed by the config Emily Shaffer
@ 2020-07-28 22:24   ` Emily Shaffer
  2020-07-28 22:24   ` [PATCH v3 3/6] hook: add list command Emily Shaffer
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Emily Shaffer @ 2020-07-28 22:24 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Introduce infrastructure for a new subcommand, git-hook, which will be
used to ease config-based hook management. This command will handle
parsing configs to compose a list of hooks to run for a given event, as
well as adding or modifying hook configs in an interactive fashion.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 .gitignore                    |  1 +
 Documentation/git-hook.txt    | 19 +++++++++++++++++++
 Makefile                      |  1 +
 builtin.h                     |  1 +
 builtin/hook.c                | 21 +++++++++++++++++++++
 git.c                         |  1 +
 t/t1360-config-based-hooks.sh | 11 +++++++++++
 7 files changed, 55 insertions(+)
 create mode 100644 Documentation/git-hook.txt
 create mode 100644 builtin/hook.c
 create mode 100755 t/t1360-config-based-hooks.sh

diff --git a/.gitignore b/.gitignore
index ee509a2ad2..0694a34884 100644
--- a/.gitignore
+++ b/.gitignore
@@ -75,6 +75,7 @@
 /git-grep
 /git-hash-object
 /git-help
+/git-hook
 /git-http-backend
 /git-http-fetch
 /git-http-push
diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
new file mode 100644
index 0000000000..2d50c414cc
--- /dev/null
+++ b/Documentation/git-hook.txt
@@ -0,0 +1,19 @@
+git-hook(1)
+===========
+
+NAME
+----
+git-hook - Manage configured hooks
+
+SYNOPSIS
+--------
+[verse]
+'git hook'
+
+DESCRIPTION
+-----------
+You can list, add, and modify hooks with this command.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 372139f1f2..e13e58e23f 100644
--- a/Makefile
+++ b/Makefile
@@ -1077,6 +1077,7 @@ BUILTIN_OBJS += builtin/get-tar-commit-id.o
 BUILTIN_OBJS += builtin/grep.o
 BUILTIN_OBJS += builtin/hash-object.o
 BUILTIN_OBJS += builtin/help.o
+BUILTIN_OBJS += builtin/hook.o
 BUILTIN_OBJS += builtin/index-pack.o
 BUILTIN_OBJS += builtin/init-db.o
 BUILTIN_OBJS += builtin/interpret-trailers.o
diff --git a/builtin.h b/builtin.h
index a5ae15bfe5..4e736499c0 100644
--- a/builtin.h
+++ b/builtin.h
@@ -157,6 +157,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix);
 int cmd_grep(int argc, const char **argv, const char *prefix);
 int cmd_hash_object(int argc, const char **argv, const char *prefix);
 int cmd_help(int argc, const char **argv, const char *prefix);
+int cmd_hook(int argc, const char **argv, const char *prefix);
 int cmd_index_pack(int argc, const char **argv, const char *prefix);
 int cmd_init_db(int argc, const char **argv, const char *prefix);
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix);
diff --git a/builtin/hook.c b/builtin/hook.c
new file mode 100644
index 0000000000..b2bbc84d4d
--- /dev/null
+++ b/builtin/hook.c
@@ -0,0 +1,21 @@
+#include "cache.h"
+
+#include "builtin.h"
+#include "parse-options.h"
+
+static const char * const builtin_hook_usage[] = {
+	N_("git hook"),
+	NULL
+};
+
+int cmd_hook(int argc, const char **argv, const char *prefix)
+{
+	struct option builtin_hook_options[] = {
+		OPT_END(),
+	};
+
+	argc = parse_options(argc, argv, prefix, builtin_hook_options,
+			     builtin_hook_usage, 0);
+
+	return 0;
+}
diff --git a/git.c b/git.c
index 2f021b97f3..7f3328c63f 100644
--- a/git.c
+++ b/git.c
@@ -517,6 +517,7 @@ static struct cmd_struct commands[] = {
 	{ "grep", cmd_grep, RUN_SETUP_GENTLY },
 	{ "hash-object", cmd_hash_object },
 	{ "help", cmd_help },
+	{ "hook", cmd_hook, RUN_SETUP },
 	{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY | NO_PARSEOPT },
 	{ "init", cmd_init_db },
 	{ "init-db", cmd_init_db },
diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
new file mode 100755
index 0000000000..34b0df5216
--- /dev/null
+++ b/t/t1360-config-based-hooks.sh
@@ -0,0 +1,11 @@
+#!/bin/bash
+
+test_description='config-managed multihooks, including git-hook command'
+
+. ./test-lib.sh
+
+test_expect_success 'git hook command does not crash' '
+	git hook
+'
+
+test_done
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 3/6] hook: add list command
  2020-07-28 22:24 ` [PATCH v3 0/6] propose config-based hooks Emily Shaffer
  2020-07-28 22:24   ` [PATCH v3 1/6] doc: propose hooks managed by the config Emily Shaffer
  2020-07-28 22:24   ` [PATCH v3 2/6] hook: scaffolding for git-hook subcommand Emily Shaffer
@ 2020-07-28 22:24   ` Emily Shaffer
  2020-07-28 22:24   ` [PATCH v3 4/6] hook: add --porcelain to " Emily Shaffer
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Emily Shaffer @ 2020-07-28 22:24 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Teach 'git hook list <hookname>', which checks the known configs in
order to create an ordered list of hooks to run on a given hook event.

Multiple commands can be specified for a given hook by providing
multiple "hook.<hookname>.command = <path-to-hook>" lines. Hooks will be
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.<hookcmd-name>.command = <path-to-hook>" line.

For example:

  $ git config --list | grep ^hook
  hook.pre-commit.command=baz
  hook.pre-commit.command=~/bar.sh
  hookcmd.baz.command=~/baz/from/hookcmd.sh

  $ git hook list pre-commit
  ~/baz/from/hookcmd.sh
  ~/bar.sh

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/git-hook.txt    | 37 +++++++++++++-
 Makefile                      |  1 +
 builtin/hook.c                | 55 +++++++++++++++++++--
 hook.c                        | 90 +++++++++++++++++++++++++++++++++++
 hook.h                        | 15 ++++++
 t/t1360-config-based-hooks.sh | 68 +++++++++++++++++++++++++-
 6 files changed, 259 insertions(+), 7 deletions(-)
 create mode 100644 hook.c
 create mode 100644 hook.h

diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
index 2d50c414cc..e458586e96 100644
--- a/Documentation/git-hook.txt
+++ b/Documentation/git-hook.txt
@@ -8,12 +8,47 @@ git-hook - Manage configured hooks
 SYNOPSIS
 --------
 [verse]
-'git hook'
+'git hook' list <hook-name>
 
 DESCRIPTION
 -----------
 You can list, add, and modify hooks with this command.
 
+This command parses the default configuration files for sections "hook" and
+"hookcmd". "hook" is used to describe the commands which will be run during a
+particular hook event; commands are run in config order. "hookcmd" is used to
+describe attributes of a specific command. If additional attributes don't need
+to be specified, a command to run can be specified directly in the "hook"
+section; if a "hookcmd" by that name isn't found, Git will attempt to run the
+provided value directly. For example:
+
+Global config
+----
+  [hook "post-commit"]
+    command = "linter"
+    command = "~/typocheck.sh"
+
+  [hookcmd "linter"]
+    command = "/bin/linter --c"
+----
+
+Local config
+----
+  [hook "prepare-commit-msg"]
+    command = "linter"
+  [hook "post-commit"]
+    command = "python ~/run-test-suite.py"
+----
+
+COMMANDS
+--------
+
+list <hook-name>::
+
+List the hooks which have been configured for <hook-name>. Hooks appear
+in the order they should be run, and note the config scope where the relevant
+`hook.<hook-name>.command` was specified, not the `hookcmd` (if applicable).
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index e13e58e23f..50e7c911d1 100644
--- a/Makefile
+++ b/Makefile
@@ -891,6 +891,7 @@ LIB_OBJS += grep.o
 LIB_OBJS += hashmap.o
 LIB_OBJS += help.o
 LIB_OBJS += hex.o
+LIB_OBJS += hook.o
 LIB_OBJS += ident.o
 LIB_OBJS += interdiff.o
 LIB_OBJS += json-writer.o
diff --git a/builtin/hook.c b/builtin/hook.c
index b2bbc84d4d..a0759a4c26 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -1,21 +1,68 @@
 #include "cache.h"
 
 #include "builtin.h"
+#include "config.h"
+#include "hook.h"
 #include "parse-options.h"
+#include "strbuf.h"
 
 static const char * const builtin_hook_usage[] = {
-	N_("git hook"),
+	N_("git hook list <hookname>"),
 	NULL
 };
 
-int cmd_hook(int argc, const char **argv, const char *prefix)
+static int list(int argc, const char **argv, const char *prefix)
 {
-	struct option builtin_hook_options[] = {
+	struct list_head *head, *pos;
+	struct hook *item;
+	struct strbuf hookname = STRBUF_INIT;
+
+	struct option list_options[] = {
 		OPT_END(),
 	};
 
-	argc = parse_options(argc, argv, prefix, builtin_hook_options,
+	argc = parse_options(argc, argv, prefix, list_options,
 			     builtin_hook_usage, 0);
 
+	if (argc < 1) {
+		usage_msg_opt("a hookname must be provided to operate on.",
+			      builtin_hook_usage, list_options);
+	}
+
+	strbuf_addstr(&hookname, argv[0]);
+
+	head = hook_list(&hookname);
+
+	if (list_empty(head)) {
+		printf(_("no commands configured for hook '%s'\n"),
+		       hookname.buf);
+		return 0;
+	}
+
+	list_for_each(pos, head) {
+		item = list_entry(pos, struct hook, list);
+		if (item)
+			printf("%s:\t%s\n",
+			       config_scope_name(item->origin),
+			       item->command.buf);
+	}
+
+	clear_hook_list();
+	strbuf_release(&hookname);
+
 	return 0;
 }
+
+int cmd_hook(int argc, const char **argv, const char *prefix)
+{
+	struct option builtin_hook_options[] = {
+		OPT_END(),
+	};
+	if (argc < 2)
+		usage_with_options(builtin_hook_usage, builtin_hook_options);
+
+	if (!strcmp(argv[1], "list"))
+		return list(argc - 1, argv + 1, prefix);
+
+	usage_with_options(builtin_hook_usage, builtin_hook_options);
+}
diff --git a/hook.c b/hook.c
new file mode 100644
index 0000000000..9dfc1a885e
--- /dev/null
+++ b/hook.c
@@ -0,0 +1,90 @@
+#include "cache.h"
+
+#include "hook.h"
+#include "config.h"
+
+static LIST_HEAD(hook_head);
+
+void free_hook(struct hook *ptr)
+{
+	if (ptr) {
+		strbuf_release(&ptr->command);
+		free(ptr);
+	}
+}
+
+static void emplace_hook(struct list_head *pos, const char *command)
+{
+	struct hook *to_add = malloc(sizeof(struct hook));
+	to_add->origin = current_config_scope();
+	strbuf_init(&to_add->command, 0);
+	strbuf_addstr(&to_add->command, command);
+
+	list_add_tail(&to_add->list, pos);
+}
+
+static void remove_hook(struct list_head *to_remove)
+{
+	struct hook *hook_to_remove = list_entry(to_remove, struct hook, list);
+	list_del(to_remove);
+	free_hook(hook_to_remove);
+}
+
+void clear_hook_list(void)
+{
+	struct list_head *pos, *tmp;
+	list_for_each_safe(pos, tmp, &hook_head)
+		remove_hook(pos);
+}
+
+static int hook_config_lookup(const char *key, const char *value, void *hook_key_cb)
+{
+	const char *hook_key = hook_key_cb;
+
+	if (!strcmp(key, hook_key)) {
+		const char *command = value;
+		struct strbuf hookcmd_name = STRBUF_INIT;
+		struct list_head *pos = NULL, *tmp = NULL;
+
+		/* Check if a hookcmd with that name exists. */
+		strbuf_addf(&hookcmd_name, "hookcmd.%s.command", command);
+		git_config_get_value(hookcmd_name.buf, &command);
+
+		if (!command)
+			BUG("git_config_get_value overwrote a string it shouldn't have");
+
+		/*
+		 * TODO: implement an option-getting callback, e.g.
+		 *   get configs by pattern hookcmd.$value.*
+		 *   for each key+value, do_callback(key, value, cb_data)
+		 */
+
+		list_for_each_safe(pos, tmp, &hook_head) {
+			struct hook *hook = list_entry(pos, struct hook, list);
+			/*
+			 * The list of hooks to run can be reordered by being redeclared
+			 * in the config. Options about hook ordering should be checked
+			 * here.
+			 */
+			if (0 == strcmp(hook->command.buf, command))
+				remove_hook(pos);
+		}
+		emplace_hook(pos, command);
+	}
+
+	return 0;
+}
+
+struct list_head* hook_list(const struct strbuf* hookname)
+{
+	struct strbuf hook_key = STRBUF_INIT;
+
+	if (!hookname)
+		return NULL;
+
+	strbuf_addf(&hook_key, "hook.%s.command", hookname->buf);
+
+	git_config(hook_config_lookup, (void*)hook_key.buf);
+
+	return &hook_head;
+}
diff --git a/hook.h b/hook.h
new file mode 100644
index 0000000000..aaf6511cff
--- /dev/null
+++ b/hook.h
@@ -0,0 +1,15 @@
+#include "config.h"
+#include "list.h"
+#include "strbuf.h"
+
+struct hook
+{
+	struct list_head list;
+	enum config_scope origin;
+	struct strbuf command;
+};
+
+struct list_head* hook_list(const struct strbuf *hookname);
+
+void free_hook(struct hook *ptr);
+void clear_hook_list(void);
diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
index 34b0df5216..46d1ed354a 100755
--- a/t/t1360-config-based-hooks.sh
+++ b/t/t1360-config-based-hooks.sh
@@ -4,8 +4,72 @@ test_description='config-managed multihooks, including git-hook command'
 
 . ./test-lib.sh
 
-test_expect_success 'git hook command does not crash' '
-	git hook
+ROOT=
+if test_have_prereq MINGW
+then
+	# In Git for Windows, Unix-like paths work only in shell scripts;
+	# `git.exe`, however, will prefix them with the pseudo root directory
+	# (of the Unix shell). Let's accommodate for that.
+	ROOT="$(cd / && pwd)"
+fi
+
+setup_hooks () {
+	test_config hook.pre-commit.command "/path/ghi" --add
+	test_config_global hook.pre-commit.command "/path/def" --add
+}
+
+setup_hookcmd () {
+	test_config hook.pre-commit.command "abc" --add
+	test_config_global hookcmd.abc.command "/path/abc" --add
+}
+
+test_expect_success 'git hook rejects commands without a mode' '
+	test_must_fail git hook pre-commit
+'
+
+
+test_expect_success 'git hook rejects commands without a hookname' '
+	test_must_fail git hook list
+'
+
+test_expect_success 'git hook list orders by config order' '
+	setup_hooks &&
+
+	cat >expected <<-EOF &&
+	global:	$ROOT/path/def
+	local:	$ROOT/path/ghi
+	EOF
+
+	git hook list pre-commit >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git hook list dereferences a hookcmd' '
+	setup_hooks &&
+	setup_hookcmd &&
+
+	cat >expected <<-EOF &&
+	global:	$ROOT/path/def
+	local:	$ROOT/path/ghi
+	local:	$ROOT/path/abc
+	EOF
+
+	git hook list pre-commit >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git hook list reorders on duplicate commands' '
+	setup_hooks &&
+
+	test_config hook.pre-commit.command "/path/def" --add &&
+
+	cat >expected <<-EOF &&
+	local:	$ROOT/path/ghi
+	local:	$ROOT/path/def
+	EOF
+
+	git hook list pre-commit >actual &&
+	test_cmp expected actual
 '
 
 test_done
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 4/6] hook: add --porcelain to list command
  2020-07-28 22:24 ` [PATCH v3 0/6] propose config-based hooks Emily Shaffer
                     ` (2 preceding siblings ...)
  2020-07-28 22:24   ` [PATCH v3 3/6] hook: add list command Emily Shaffer
@ 2020-07-28 22:24   ` Emily Shaffer
  2020-07-28 22:24   ` [RFC PATCH v3 5/6] parse-options: parse into argv_array Emily Shaffer
  2020-07-28 22:24   ` [RFC PATCH v3 6/6] hook: add 'run' subcommand Emily Shaffer
  5 siblings, 0 replies; 22+ messages in thread
From: Emily Shaffer @ 2020-07-28 22:24 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Teach 'git hook list --porcelain <hookname>', which prints simply the
commands to be run in the order suggested by the config. This option is
intended for use by user scripts, wrappers, or out-of-process Git
commands which still want to execute hooks. For example, the following
snippet might be added to git-send-email.perl to introduce a
`pre-send-email` hook:

  sub pre_send_email {
    open(my $fh, 'git hook list --porcelain pre-send-email |');
    chomp(my @hooks = <$fh>);
    close($fh);

    foreach $hook (@hooks) {
            system $hook
    }

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/git-hook.txt    | 13 +++++++++++--
 builtin/hook.c                | 17 +++++++++++++----
 t/t1360-config-based-hooks.sh | 12 ++++++++++++
 3 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
index e458586e96..0854035ce2 100644
--- a/Documentation/git-hook.txt
+++ b/Documentation/git-hook.txt
@@ -8,7 +8,7 @@ git-hook - Manage configured hooks
 SYNOPSIS
 --------
 [verse]
-'git hook' list <hook-name>
+'git hook' list [--porcelain] <hook-name>
 
 DESCRIPTION
 -----------
@@ -43,11 +43,20 @@ Local config
 COMMANDS
 --------
 
-list <hook-name>::
+list [--porcelain] <hook-name>::
 
 List the hooks which have been configured for <hook-name>. Hooks appear
 in the order they should be run, and note the config scope where the relevant
 `hook.<hook-name>.command` was specified, not the `hookcmd` (if applicable).
++
+If `--porcelain` is specified, instead print the commands alone, separated by
+newlines, for easy parsing by a script.
+
+OPTIONS
+-------
+--porcelain::
+	With `list`, print the commands in the order they should be run,
+	separated by newlines, for easy parsing by a script.
 
 GIT
 ---
diff --git a/builtin/hook.c b/builtin/hook.c
index a0759a4c26..0d92124ca6 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -16,8 +16,11 @@ static int list(int argc, const char **argv, const char *prefix)
 	struct list_head *head, *pos;
 	struct hook *item;
 	struct strbuf hookname = STRBUF_INIT;
+	int porcelain = 0;
 
 	struct option list_options[] = {
+		OPT_BOOL(0, "porcelain", &porcelain,
+			 "format for execution by a script"),
 		OPT_END(),
 	};
 
@@ -29,6 +32,8 @@ 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);
@@ -41,10 +46,14 @@ static int list(int argc, const char **argv, const char *prefix)
 
 	list_for_each(pos, head) {
 		item = list_entry(pos, struct hook, list);
-		if (item)
-			printf("%s:\t%s\n",
-			       config_scope_name(item->origin),
-			       item->command.buf);
+		if (item) {
+			if (porcelain)
+				printf("%s\n", item->command.buf);
+			else
+				printf("%s:\t%s\n",
+				       config_scope_name(item->origin),
+				       item->command.buf);
+		}
 	}
 
 	clear_hook_list();
diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
index 46d1ed354a..ebf8f38d68 100755
--- a/t/t1360-config-based-hooks.sh
+++ b/t/t1360-config-based-hooks.sh
@@ -72,4 +72,16 @@ test_expect_success 'git hook list reorders on duplicate commands' '
 	test_cmp expected actual
 '
 
+test_expect_success 'git hook list --porcelain prints just the command' '
+	setup_hooks &&
+
+	cat >expected <<-EOF &&
+	$ROOT/path/def
+	$ROOT/path/ghi
+	EOF
+
+	git hook list --porcelain pre-commit >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [RFC PATCH v3 5/6] parse-options: parse into argv_array
  2020-07-28 22:24 ` [PATCH v3 0/6] propose config-based hooks Emily Shaffer
                     ` (3 preceding siblings ...)
  2020-07-28 22:24   ` [PATCH v3 4/6] hook: add --porcelain to " Emily Shaffer
@ 2020-07-28 22:24   ` Emily Shaffer
  2020-07-29 19:33     ` Junio C Hamano
  2020-07-28 22:24   ` [RFC PATCH v3 6/6] hook: add 'run' subcommand Emily Shaffer
  5 siblings, 1 reply; 22+ messages in thread
From: Emily Shaffer @ 2020-07-28 22:24 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

parse-options already knows how to read into a string_list, and it knows
how to read into an argv_array as a passthrough (that is, including the
argument as well as its value). string_list and argv_array serve similar
purposes but are somewhat painful to convert between; so, let's teach
parse-options to read values of string arguments directly into an
argv_array without preserving the argument name.

This is useful if collecting generic arguments to pass through to
another command, for example, 'git hook run --arg "--quiet" --arg
"--format=pretty" some-hook'. The resulting argv_array would contain
{ "--quiet", "--format=pretty" }.

The implementation is based on that of OPT_STRING_LIST.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/technical/api-parse-options.txt |  5 +++++
 parse-options-cb.c                            | 16 ++++++++++++++++
 parse-options.h                               |  4 ++++
 3 files changed, 25 insertions(+)

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 2e2e7c10c6..1e97343338 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -173,6 +173,11 @@ There are some macros to easily define options:
 	The string argument is stored as an element in `string_list`.
 	Use of `--no-option` will clear the list of preceding values.
 
+`OPT_ARGV_ARRAY(short, long, &struct argv_array, arg_str, description)`::
+	Introduce an option with a string argument.
+	The string argument is stored as an element in `argv_array`.
+	Use of `--no-option` will clear the list of preceding values.
+
 `OPT_INTEGER(short, long, &int_var, description)`::
 	Introduce an option with integer argument.
 	The integer is put into `int_var`.
diff --git a/parse-options-cb.c b/parse-options-cb.c
index 86cd393013..94c2dd397a 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -205,6 +205,22 @@ int parse_opt_string_list(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+int parse_opt_argv_array(const struct option *opt, const char *arg, int unset)
+{
+	struct argv_array *v = opt->value;
+
+	if (unset) {
+		argv_array_clear(v);
+		return 0;
+	}
+
+	if (!arg)
+		return -1;
+
+	argv_array_push(v, arg);
+	return 0;
+}
+
 int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset)
 {
 	return 0;
diff --git a/parse-options.h b/parse-options.h
index 46af942093..e2e2de75c8 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -177,6 +177,9 @@ struct option {
 #define OPT_STRING_LIST(s, l, v, a, h) \
 				    { OPTION_CALLBACK, (s), (l), (v), (a), \
 				      (h), 0, &parse_opt_string_list }
+#define OPT_ARGV_ARRAY(s, l, v, a, h) \
+				    { OPTION_CALLBACK, (s), (l), (v), (a), \
+				      (h), 0, &parse_opt_argv_array }
 #define OPT_UYN(s, l, v, h)         { OPTION_CALLBACK, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, &parse_opt_tertiary }
 #define OPT_EXPIRY_DATE(s, l, v, h) \
@@ -296,6 +299,7 @@ int parse_opt_commits(const struct option *, const char *, int);
 int parse_opt_commit(const struct option *, const char *, int);
 int parse_opt_tertiary(const struct option *, const char *, int);
 int parse_opt_string_list(const struct option *, const char *, int);
+int parse_opt_argv_array(const struct option *, const char *, int);
 int parse_opt_noop_cb(const struct option *, const char *, int);
 enum parse_opt_result parse_opt_unknown_cb(struct parse_opt_ctx_t *ctx,
 					   const struct option *,
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [RFC PATCH v3 6/6] hook: add 'run' subcommand
  2020-07-28 22:24 ` [PATCH v3 0/6] propose config-based hooks Emily Shaffer
                     ` (4 preceding siblings ...)
  2020-07-28 22:24   ` [RFC PATCH v3 5/6] parse-options: parse into argv_array Emily Shaffer
@ 2020-07-28 22:24   ` Emily Shaffer
  5 siblings, 0 replies; 22+ messages in thread
From: Emily Shaffer @ 2020-07-28 22:24 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

In order to enable hooks to be run as an external process, by a
standalone Git command, or by tools which wrap Git, provide an external
means to run all configured hook commands for a given hook event.

For now, the hook commands will in config order, in series. As alternate
ordering or parallelism is supported in the future, we should add knobs
to use those to the command line as well.

As with the legacy hook implementation, all stdout generated by hook
commands is redirected to stderr. Piping from stdin is not yet
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.

Users may wish to provide hook commands like 'git config
hook.pre-commit.command "~/linter.sh --pre-commit"'. To enable this, the
contents of the 'hook.*.command' and 'hookcmd.*.command' strings are
first split by space or quotes into an argv_array, then expanded with
'expand_user_path()'.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 builtin/hook.c                | 30 +++++++++++++++++++++++++
 hook.c                        | 42 +++++++++++++++++++++++++++++++++++
 hook.h                        |  3 +++
 t/t1360-config-based-hooks.sh | 28 +++++++++++++++++++++++
 4 files changed, 103 insertions(+)

diff --git a/builtin/hook.c b/builtin/hook.c
index 0d92124ca6..cd61fad5fb 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -5,9 +5,11 @@
 #include "hook.h"
 #include "parse-options.h"
 #include "strbuf.h"
+#include "argv-array.h"
 
 static const char * const builtin_hook_usage[] = {
 	N_("git hook list <hookname>"),
+	N_("git hook run [(-e|--env)=<var>...] [(-a|--arg)=<arg>...] <hookname>"),
 	NULL
 };
 
@@ -62,6 +64,32 @@ static int list(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static int run(int argc, const char **argv, const char *prefix)
+{
+	struct strbuf hookname = STRBUF_INIT;
+	struct argv_array env_argv = ARGV_ARRAY_INIT;
+	struct argv_array arg_argv = ARGV_ARRAY_INIT;
+
+	struct option run_options[] = {
+		OPT_ARGV_ARRAY('e', "env", &env_argv, N_("var"),
+			       N_("environment variables for hook to use")),
+		OPT_ARGV_ARRAY('a', "arg", &arg_argv, N_("args"),
+			       N_("argument to pass to hook")),
+		OPT_END(),
+	};
+
+	argc = parse_options(argc, argv, prefix, run_options,
+			     builtin_hook_usage, 0);
+
+	if (argc < 1)
+		usage_msg_opt(_("a hookname must be provided to operate on."),
+			      builtin_hook_usage, run_options);
+
+	strbuf_addstr(&hookname, argv[0]);
+
+	return run_hooks(env_argv.argv, &hookname, &arg_argv);
+}
+
 int cmd_hook(int argc, const char **argv, const char *prefix)
 {
 	struct option builtin_hook_options[] = {
@@ -72,6 +100,8 @@ int cmd_hook(int argc, const char **argv, const char *prefix)
 
 	if (!strcmp(argv[1], "list"))
 		return list(argc - 1, argv + 1, prefix);
+	if (!strcmp(argv[1], "run"))
+		return run(argc - 1, argv + 1, prefix);
 
 	usage_with_options(builtin_hook_usage, builtin_hook_options);
 }
diff --git a/hook.c b/hook.c
index 9dfc1a885e..902e213173 100644
--- a/hook.c
+++ b/hook.c
@@ -2,6 +2,7 @@
 
 #include "hook.h"
 #include "config.h"
+#include "run-command.h"
 
 static LIST_HEAD(hook_head);
 
@@ -78,6 +79,7 @@ static int hook_config_lookup(const char *key, const char *value, void *hook_key
 struct list_head* hook_list(const struct strbuf* hookname)
 {
 	struct strbuf hook_key = STRBUF_INIT;
+	const char *legacy_hook_path = NULL;
 
 	if (!hookname)
 		return NULL;
@@ -86,5 +88,45 @@ struct list_head* hook_list(const struct strbuf* hookname)
 
 	git_config(hook_config_lookup, (void*)hook_key.buf);
 
+	legacy_hook_path = find_hook(hookname->buf);
+
+	/* TODO: check hook.runHookDir */
+	if (legacy_hook_path)
+		emplace_hook(&hook_head, legacy_hook_path);
+
 	return &hook_head;
 }
+
+int run_hooks(const char *const *env, const struct strbuf *hookname,
+	      const struct argv_array *args)
+{
+	struct list_head *to_run, *pos = NULL, *tmp = NULL;
+	int rc = 0;
+
+	to_run = hook_list(hookname);
+
+	list_for_each_safe(pos, tmp, to_run) {
+		struct child_process hook_proc = CHILD_PROCESS_INIT;
+		struct hook *hook = list_entry(pos, struct hook, list);
+
+		/* add command */
+		argv_array_push(&hook_proc.args, hook->command.buf);
+
+		/*
+		 * add passed-in argv, without expanding - let the user get back
+		 * exactly what they put in
+		 */
+		if (args)
+			argv_array_pushv(&hook_proc.args, args->argv);
+
+		hook_proc.env = env;
+		hook_proc.no_stdin = 1;
+		hook_proc.stdout_to_stderr = 1;
+		hook_proc.trace2_hook_name = hook->command.buf;
+		hook_proc.use_shell = 1;
+
+		rc |= run_command(&hook_proc);
+	}
+
+	return rc;
+}
diff --git a/hook.h b/hook.h
index aaf6511cff..cf598d6ccb 100644
--- a/hook.h
+++ b/hook.h
@@ -1,6 +1,7 @@
 #include "config.h"
 #include "list.h"
 #include "strbuf.h"
+#include "argv-array.h"
 
 struct hook
 {
@@ -10,6 +11,8 @@ struct hook
 };
 
 struct list_head* hook_list(const struct strbuf *hookname);
+int run_hooks(const char *const *env, const struct strbuf *hookname,
+	      const struct argv_array *args);
 
 void free_hook(struct hook *ptr);
 void clear_hook_list(void);
diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
index ebf8f38d68..ee8114250d 100755
--- a/t/t1360-config-based-hooks.sh
+++ b/t/t1360-config-based-hooks.sh
@@ -84,4 +84,32 @@ test_expect_success 'git hook list --porcelain prints just the command' '
 	test_cmp expected actual
 '
 
+test_expect_success 'inline hook definitions execute oneliners' '
+	test_config hook.pre-commit.command "echo \"Hello World\"" &&
+
+	echo "Hello World" >expected &&
+
+	# hooks are run with stdout_to_stderr = 1
+	git hook run pre-commit 2>actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'inline hook definitions resolve paths' '
+	cat >~/sample-hook.sh <<-EOF &&
+	echo \"Sample Hook\"
+	EOF
+
+	test_when_finished "rm ~/sample-hook.sh" &&
+
+	chmod +x ~/sample-hook.sh &&
+
+	test_config hook.pre-commit.command "~/sample-hook.sh" &&
+
+	echo \"Sample Hook\" >expected &&
+
+	# hooks are run with stdout_to_stderr = 1
+	git hook run pre-commit 2>actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC PATCH v3 5/6] parse-options: parse into argv_array
  2020-07-28 22:24   ` [RFC PATCH v3 5/6] parse-options: parse into argv_array Emily Shaffer
@ 2020-07-29 19:33     ` Junio C Hamano
  2020-07-30 23:41       ` Junio C Hamano
  0 siblings, 1 reply; 22+ messages in thread
From: Junio C Hamano @ 2020-07-29 19:33 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

Emily Shaffer <emilyshaffer@google.com> writes:

> parse-options already knows how to read into a string_list, and it knows
> how to read into an argv_array as a passthrough (that is, including the
> argument as well as its value). string_list and argv_array serve similar
> purposes but are somewhat painful to convert between; so, let's teach
> parse-options to read values of string arguments directly into an
> argv_array without preserving the argument name.
>
> This is useful if collecting generic arguments to pass through to
> another command, for example, 'git hook run --arg "--quiet" --arg
> "--format=pretty" some-hook'. The resulting argv_array would contain
> { "--quiet", "--format=pretty" }.
>
> The implementation is based on that of OPT_STRING_LIST.

Be it argv_array or strvec, I think this is a useful thing to do.

I grepped for the users of OPT_STRING_LIST() to see if some of them
are better served by this, but none of them stood out as candidates
that are particularly good match.

> +int parse_opt_argv_array(const struct option *opt, const char *arg, int unset)
> +{
> +	struct argv_array *v = opt->value;
> +
> +	if (unset) {
> +		argv_array_clear(v);
> +		return 0;
> +	}
> +
> +	if (!arg)
> +		return -1;

I think the calling parse_options() loop would catch this negative
return and raise an error, but is it better for this code to stay
silent or would it be better to say that opt->long_name/short_name 
is not a boolean?

> +	argv_array_push(v, arg);
> +	return 0;
> +}

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC PATCH v3 5/6] parse-options: parse into argv_array
  2020-07-29 19:33     ` Junio C Hamano
@ 2020-07-30 23:41       ` Junio C Hamano
  0 siblings, 0 replies; 22+ messages in thread
From: Junio C Hamano @ 2020-07-30 23:41 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git, Jeff King

Junio C Hamano <gitster@pobox.com> writes:

> Be it argv_array or strvec, I think this is a useful thing to do.
>
> I grepped for the users of OPT_STRING_LIST() to see if some of them
> are better served by this, but none of them stood out as candidates
> that are particularly good match.
>
>> +int parse_opt_argv_array(const struct option *opt, const char *arg, int unset)
>> +{
>> +	struct argv_array *v = opt->value;
>> +
>> +	if (unset) {
>> +		argv_array_clear(v);
>> +		return 0;
>> +	}
>> +
>> +	if (!arg)
>> +		return -1;
>
> I think the calling parse_options() loop would catch this negative
> return and raise an error, but is it better for this code to stay
> silent or would it be better to say that opt->long_name/short_name 
> is not a boolean?

I am still waiting for this to be answered, but I queued the whole
topic, these last two steps included, just to see how bad adjusting
to the strvec API migration would be.  It wasn't too bad.

I would not recommend you, or other contributors who use argv-array
API in their topics, to build on top of jk/strvec, not just yet, as
I expect it to go through at least one more reroll to update the
details.

Thanks.


^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, back to index

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-21 18:54 [PATCH v2 0/4] propose config-based hooks Emily Shaffer
2020-05-21 18:54 ` [PATCH v2 1/4] doc: propose hooks managed by the config Emily Shaffer
2020-05-22 10:13   ` Phillip Wood
2020-06-09 20:26     ` Emily Shaffer
2020-05-21 18:54 ` [PATCH v2 2/4] hook: scaffolding for git-hook subcommand Emily Shaffer
2020-05-21 18:54 ` [PATCH v2 3/4] hook: add list command Emily Shaffer
2020-05-22 10:27   ` Phillip Wood
2020-06-09 21:49     ` Emily Shaffer
2020-05-24 23:00   ` Johannes Schindelin
2020-05-27 23:37     ` Emily Shaffer
2020-05-21 18:54 ` [PATCH v2 4/4] hook: add --porcelain to " Emily Shaffer
2020-05-24 23:00   ` Johannes Schindelin
2020-05-25  0:29     ` Johannes Schindelin
2020-07-28 22:24 ` [PATCH v3 0/6] propose config-based hooks Emily Shaffer
2020-07-28 22:24   ` [PATCH v3 1/6] doc: propose hooks managed by the config Emily Shaffer
2020-07-28 22:24   ` [PATCH v3 2/6] hook: scaffolding for git-hook subcommand Emily Shaffer
2020-07-28 22:24   ` [PATCH v3 3/6] hook: add list command Emily Shaffer
2020-07-28 22:24   ` [PATCH v3 4/6] hook: add --porcelain to " Emily Shaffer
2020-07-28 22:24   ` [RFC PATCH v3 5/6] parse-options: parse into argv_array Emily Shaffer
2020-07-29 19:33     ` Junio C Hamano
2020-07-30 23:41       ` Junio C Hamano
2020-07-28 22:24   ` [RFC PATCH v3 6/6] hook: add 'run' subcommand Emily Shaffer

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git