Hi Emily
Thanks for working on this, having a way to manage multiple commands per
hook without using an external framework would be really useful
On 14/04/2020 01:54, Emily Shaffer wrote:
Not much to look at compared to the original RFC I sent some months ago.
This implements Peff's suggestion of using the "hookcmd" section as a
layer of indirection.
I'm not really clear what the advantage of this indirection is. It seems
unlikely to me that different hooks will share exactly the same command
line or other options. In the 'git secrets' example earlier in this
thread each hook needs to use a different command line. In general a
command cannot tell which hook it is being invoked as without a flag of
some kind. (In some cases it can use the number of arguments if that is
different for each hook that it handles but that is not true in general)
Without the redirection one could have
hook.pre-commit.linter.command = my-command
hook.pre-commit.check-whitespace.command = 'git diff --check --cached'
and other keys can be added for ordering etc. e.g.
hook.pre-commit.linter.before = check-whitespace
With the indirection one needs to set
hook.pre-commit.command = linter
hook.pre-commit.check-whitespace = 'git diff --check --cached'
hookcmd.linter.command = my-command
hookcmd.linter.pre-commit-before = check-whitespace
which involves setting an extra key and checking it each time the hook
is invoked without any benefit that I can see. I suspect which one seems
more logical depends on how one thinks of setting hooks - I tend to
think "I want to set a pre-commit hook" not "I want to set a git-secrets
hook". If you've got an example where this indirection is helpful or
necessary that would be really useful to see.
Best Wishes
Phillip
The scope is a little smaller than the original
RFC as it doesn't have a way to remove hooks from downstream (yet), and
ordering numbers are dropped (for now).
One thing that's missing, as evidenced by the TODO, is a way to handle
arbitrary options given within a "hookcmd" unit. I think this can be
achieved with a callback, since it seems plausible that "pre-receive"
might want a different set of options than "post-commit" or so on. To
me, it sounds achievable with a callback; I imagine a follow-on teaching
git-hook how to remove a hook with something like "hookcmd.foo.skip =
true" will give an OK indication of how that might look.
Overall though, I think this is simpler than the first version of the
RFC because I was reminded by wiser folks than I to "keep it simple,
stupid." ;)
I think it's feasible that with these couple patches applied, someone
who wanted to jump in early could replace their
.git/hook/whatever-hookname with some boilerplate like
xargs -n 1 'sh -c' <<<"$(git hook --list whatever-hookname)"
and give it a shot. Untested snippet. :)
CI run: https://github.com/gitgitgadget/git/pull/611/checks
- Emily
Emily Shaffer (2):
hook: scaffolding for git-hook subcommand
hook: add --list mode
.gitignore | 1 +
Documentation/git-hook.txt | 53 ++++++++++++++++++++
Makefile | 2 +
builtin.h | 1 +
builtin/hook.c | 77 +++++++++++++++++++++++++++++
git.c | 1 +
hook.c | 92 +++++++++++++++++++++++++++++++++++
hook.h | 13 +++++
t/t1360-config-based-hooks.sh | 58 ++++++++++++++++++++++
9 files changed, 298 insertions(+)
create mode 100644 Documentation/git-hook.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
