| Message ID | 20200414005457.3505-1-emilyshaffer@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | configuration-based hook management (was: [TOPIC 2/17] Hooks in the future) | expand |
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 >
On Tue, Apr 14, 2020 at 04:15:11PM +0100, Phillip Wood wrote: > 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' I think this isn't supported by the config semantics. Have a look at config.h:parse_config_key: /* * Match and parse a config key of the form: * * section.(subsection.)?key * * (i.e., what gets handed to a config_fn_t). The caller provides the section; * we return -1 if it does not match, 0 otherwise. The subsection and key * out-parameters are filled by the function (and *subsection is NULL if it is * missing). * * If the subsection pointer-to-pointer passed in is NULL, returns 0 only if * there is no subsection at all. */ int parse_config_key(const char *var, const char *section, const char **subsection, int *subsection_len, const char **key); We'd need to fudge one of these fields to include the extra section, I think. Unfortunate, because I find your example very tidy, but in practice maybe not very neat. The closest thing I can find to a nice way of writing it might be: [hook.pre-commit "linter"] command = my-command before = check-whitespace [hook.pre-commit "check-whitespace"] command = 'git diff --check --cached' But this is kind of a lie; the sections aren't "hook", "pre-commit", and "linter" as you'd expect. Whether it's OK to lie like this, though, I don't know - I suspect it might make it awkward for others trying to parse the config. (my Vim syntax highlighter had kind of a hard time.) > > 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. Thanks for sharing your workflow; as always, it's hard to understand the ways others work differently from yourself, so I'm glad to hear from you. Let me think some more on it and reply back again. - Emily
On 2020.04.14 16:15, Phillip Wood wrote: > 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 Indexing repo content (see [1] for a detailed discussion) is one use case where you have a single command that runs identically from post-commit, post-merge, and post-checkout. Also, I suspect that many users don't have a firm enough grasp on the various git hooks options to know ahead of time which ones they want to set to accomplish a given task (without diving into the docs first). I'm not trying to say that your workflow is incorrect, but my gut feeling is that most Git users would work in the opposite direction. Every time I have needed to automate something, I generally had a rough script in place first, and then looked up which hook(s) would be appropriate triggers for the script. [1]: https://tbaggery.com/2011/08/08/effortless-ctags-with-git.html
On Tue, Apr 14, 2020 at 12:24:18PM -0700, Emily Shaffer wrote: > > Without the redirection one could have > > hook.pre-commit.linter.command = my-command > > hook.pre-commit.check-whitespace.command = 'git diff --check --cached' > [...] > We'd need to fudge one of these fields to include the extra section, I > think. Unfortunate, because I find your example very tidy, but in > practice maybe not very neat. The closest thing I can find to a nice way > of writing it might be: > > [hook.pre-commit "linter"] > command = my-command > before = check-whitespace > [hook.pre-commit "check-whitespace"] > command = 'git diff --check --cached' Syntactically the whole section between the outer dots is the subsection. So it's: [hook "pre-commit.check-whitespace"] command = ... And I don't think we want to change the config syntax at this point. Even in the neater dotted notation, we must keep that whole thing as a subsection, because existing subsections may contain dots, too. > But this is kind of a lie; the sections aren't "hook", "pre-commit", and > "linter" as you'd expect. Whether it's OK to lie like this, though, I > don't know - I suspect it might make it awkward for others trying to > parse the config. (my Vim syntax highlighter had kind of a hard time.) I think we should avoid it if possible. There are some subtleties there, like the fact that subsections are case-sensitive, but sections and keys are not. -Peff
On Tue, Apr 14, 2020 at 04:15:11PM +0100, Phillip Wood wrote: > 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 In the proposal I gave, you could do: hook.pre-commit.command = my-command hook.pre-commit.command = git diff --check --cached If you want to refer to commands in ordering options (like your "before"), then you'd have to refer to their names. For "my-command" that's not too bad. For the longer one, it's a bit awkward. You _could_ do: hookcmd.my-command.before = git diff --check --cached which is the same number of lines as yours. But I'd probably give it a name, like: hookcmd.check-whitespace.command = git diff --check --cached hookcmd.my-command.before = check-whitespace That's one more line than yours, but I think it separates the concerns more clearly. And it extends naturally to more options specific to check-whitespace. -Peff
On 14/04/2020 21:27, Jeff King wrote: > On Tue, Apr 14, 2020 at 12:24:18PM -0700, Emily Shaffer wrote: > >>> Without the redirection one could have >>> hook.pre-commit.linter.command = my-command >>> hook.pre-commit.check-whitespace.command = 'git diff --check --cached' >> [...] >> We'd need to fudge one of these fields to include the extra section, I >> think. Unfortunate, because I find your example very tidy, but in >> practice maybe not very neat. The closest thing I can find to a nice way >> of writing it might be: >> >> [hook.pre-commit "linter"] >> command = my-command >> before = check-whitespace >> [hook.pre-commit "check-whitespace"] >> command = 'git diff --check --cached' > > Syntactically the whole section between the outer dots is the > subsection. So it's: > > [hook "pre-commit.check-whitespace"] > command = ... > > And I don't think we want to change the config syntax at this point. > Even in the neater dotted notation, we must keep that whole thing as a > subsection, because existing subsections may contain dots, too. Thanks for clarifying that, I agree we don't want to change the config syntax and break existing subsections Best Wishes Phillip >> But this is kind of a lie; the sections aren't "hook", "pre-commit", and >> "linter" as you'd expect. Whether it's OK to lie like this, though, I >> don't know - I suspect it might make it awkward for others trying to >> parse the config. (my Vim syntax highlighter had kind of a hard time.) > > I think we should avoid it if possible. There are some subtleties there, > like the fact that subsections are case-sensitive, but sections and keys > are not.> -Peff >
On 14/04/2020 21:32, Jeff King wrote: > On Tue, Apr 14, 2020 at 04:15:11PM +0100, Phillip Wood wrote: > >> 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 > > In the proposal I gave, you could do: > > hook.pre-commit.command = my-command > hook.pre-commit.command = git diff --check --cached > > If you want to refer to commands in ordering options (like your > "before"), then you'd have to refer to their names. For "my-command" > that's not too bad. For the longer one, it's a bit awkward. You _could_ > do: > > hookcmd.my-command.before = git diff --check --cached > > which is the same number of lines as yours. But I'd probably give it a > name, like: > > hookcmd.check-whitespace.command = git diff --check --cached > hookcmd.my-command.before = check-whitespace > > That's one more line than yours, but I think it separates the concerns > more clearly. And it extends naturally to more options specific to > check-whitespace. I agree that using a name rather than the command line makes things clearer here Best Wishes Phillip > -Peff >
On 14/04/2020 21:03, Josh Steadmon wrote: > On 2020.04.14 16:15, Phillip Wood wrote: >> 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 > > Indexing repo content (see [1] for a detailed discussion) is one use > case where you have a single command that runs identically from > post-commit, post-merge, and post-checkout. Thanks for sharing that, it is a useful reference point > Also, I suspect that many users don't have a firm enough grasp on the > various git hooks options to know ahead of time which ones they want to > set to accomplish a given task (without diving into the docs first). I agree with this, especially as setting up a hook is probably an infrequent task for most people > I'm > not trying to say that your workflow is incorrect, but my gut feeling is > that most Git users would work in the opposite direction. Every time I > have needed to automate something, I generally had a rough script in > place first, and then looked up which hook(s) would be appropriate > triggers for the script. As you say once they have a script they still have to look up which hooks they want to hook it up to, the indirection does not avoid that, it just means they have to lookup how to set up a hookcmd as well as which hooks they want to use. Best Wishes Phillip > [1]: https://tbaggery.com/2011/08/08/effortless-ctags-with-git.html >
Phillip Wood <phillip.wood123@gmail.com> writes: >> If you want to refer to commands in ordering options (like your >> "before"), then you'd have to refer to their names. For "my-command" >> that's not too bad. For the longer one, it's a bit awkward. You _could_ >> do: >> >> hookcmd.my-command.before = git diff --check --cached >> >> which is the same number of lines as yours. But I'd probably give it a >> name, like: >> >> hookcmd.check-whitespace.command = git diff --check --cached >> hookcmd.my-command.before = check-whitespace >> >> That's one more line than yours, but I think it separates the concerns >> more clearly. And it extends naturally to more options specific to >> check-whitespace. > > I agree that using a name rather than the command line makes things > clearer here True. These ways call for a different attitude to deal with errors compared to the approach to order them with numbers, though. If your approach is to order by number attached to each hook, only possible errors you'd need to worry about are (1) what to do when the user forgets to give a number to a hook and (2) what to do when the user gives the same number by accident to multiple hooks, and both can even be made non-errors by declaring that an unnumbered hook has a default number, and that two hooks with the same number execute in an unspecified and unstable order. On the other hand, the approach to specify relative ordering among hooks can break more easily. E.g. when a hook that used to be before "my-command" got removed. It is harder to find a "sensible" default behaviour for such situations. I am perfectly fine with having more possible error cases than allowing misconfigured system to silently do a wrong thing, so...
On Wed, Apr 15, 2020 at 07:51:14AM -0700, Junio C Hamano wrote: > > Phillip Wood <phillip.wood123@gmail.com> writes: > > >> If you want to refer to commands in ordering options (like your > >> "before"), then you'd have to refer to their names. For "my-command" > >> that's not too bad. For the longer one, it's a bit awkward. You _could_ > >> do: > >> > >> hookcmd.my-command.before = git diff --check --cached > >> > >> which is the same number of lines as yours. But I'd probably give it a > >> name, like: > >> > >> hookcmd.check-whitespace.command = git diff --check --cached > >> hookcmd.my-command.before = check-whitespace > >> > >> That's one more line than yours, but I think it separates the concerns > >> more clearly. And it extends naturally to more options specific to > >> check-whitespace. > > > > I agree that using a name rather than the command line makes things > > clearer here > > True. > > These ways call for a different attitude to deal with errors > compared to the approach to order them with numbers, though. > > If your approach is to order by number attached to each hook, only > possible errors you'd need to worry about are (1) what to do when > the user forgets to give a number to a hook and (2) what to do when > the user gives the same number by accident to multiple hooks, and > both can even be made non-errors by declaring that an unnumbered > hook has a default number, and that two hooks with the same number > execute in an unspecified and unstable order. > > On the other hand, the approach to specify relative ordering among > hooks can break more easily. E.g. when a hook that used to be > before "my-command" got removed. It is harder to find a "sensible" > default behaviour for such situations. To be clear, the examples listed (both numbered order and relational order) were more for illustration purposes. At the contributor summit, I think Peff's suggestion was to stick with config ordering until we discover something more robust is needed, which is fine by me. At that time, I don't see a problem with doing something like: [hook] ordering = numerical [hookcmd "my-command"] command = ~/my-command.sh order = 001 (which means others can still rely on config ordering if they want.) Or, to put it another way, I don't think we need to solve the config ordering problem today - as long as we don't make it impossible for us to change tomorrow :) - Emily
Emily Shaffer <emilyshaffer@google.com> writes: > Or, to put it another way, I don't think we need to solve the config > ordering problem today - as long as we don't make it impossible for us > to change tomorrow :) OK.