[RFC] Hook management via 'git hooks' command

> Here's my suggestion.
> 
>  - Let configs handle the hook list and ordering, via 'hook.{hookname}' which
>    can be specified more than once.
>    - global config: hook.update = /path/to/firsthook
>      user config: hook.update = /path/to/secondhook
>      worktree config: hook.update = -/path/to/firsthook #eliminate the firsthook
>       call
>    - global config: hook.update = /path/to/firsthook
>      repo config: hook.update = /path/to/secondhook
>      repo config: hook.update = ^/path/to/firsthook #move firsthook execution
>        after secondhook for this project

>  - Let users decide whether they want to check core.hookDir in addition to their
>    config, instead of their config, or not at all, via a config
>    'hook.runHookDir' "hookdir-first", "hookdir-only", "config-only", etc.
>  - Let users ask to be notified if running a hook from .git/hooks via a config
>    'hook.warnHookDir'. (Mentioning .git/hooks rather than core.hookDir is
>    intentional here.) Alternatively, ask for 'hook.warn' with values "hookdir",
>    "all" depending on how trusting the user feels.
>    - If we want to phase out .git/hooks, we can make this notification opt-out
>      instead of opt-in, someday.
>    - between runHookDir and warnHookDir, users are able to phase out
>      .git/hooks scripts at their own pace.
>  - Abstract most of this via a 'git hooks' command.
>    - 'git hooks add <hookname> [--<scope>] <path>' to append another hook;
>      let the scope setting match 'git config'.
>    - 'git hooks show [<hookname>]' to indicate which hooks will be run, either
>      on a specific event or for all events, and in which order, as well as each
>      hook's config origin
>    - 'git hooks edit <hookname>' to modify the hook order, or choose not to run
>      a hook locally
>    - By managing it through a command, we can reorder the contents of config
>      files if it makes sense to do so.
>  - Add a hook library.
>    - Optionally specify all hook events via enum, so we aren't string-typing
>      calls to find_hook() anymore.
>    - Resolve configs into a list of hooks to run by concatenating them together
>      in config precedence order.
>      - Also allow configs formatted like "-<path>" to remove an
>        earlier-configured invocation of that hook, or "^<path>" to move the
>        earlier-configured invocation to this point in the execution order
>    - Move the find_hook() implementation to here, to account for the multihook
>      ordering

> On 2019-11-16 at 01:11:25, Emily Shaffer wrote:
> > Here's my suggestion.
> > 
> >  - Let configs handle the hook list and ordering, via 'hook.{hookname}' which
> >    can be specified more than once.
> >    - global config: hook.update = /path/to/firsthook
> >      user config: hook.update = /path/to/secondhook
> >      worktree config: hook.update = -/path/to/firsthook #eliminate the firsthook
> >       call
> >    - global config: hook.update = /path/to/firsthook
> >      repo config: hook.update = /path/to/secondhook
> >      repo config: hook.update = ^/path/to/firsthook #move firsthook execution
> >        after secondhook for this project
> 
> I'd like to hear more about how we handle multiple hooks that are
> repo-specific and don't live in the PATH.  This is a common situation
> for existing tools that handle multiple hooks, and it's one I think we
> should support.

> 
> >  - Let users decide whether they want to check core.hookDir in addition to their
> >    config, instead of their config, or not at all, via a config
> >    'hook.runHookDir' "hookdir-first", "hookdir-only", "config-only", etc.
> >  - Let users ask to be notified if running a hook from .git/hooks via a config
> >    'hook.warnHookDir'. (Mentioning .git/hooks rather than core.hookDir is
> >    intentional here.) Alternatively, ask for 'hook.warn' with values "hookdir",
> >    "all" depending on how trusting the user feels.
> >    - If we want to phase out .git/hooks, we can make this notification opt-out
> >      instead of opt-in, someday.
> >    - between runHookDir and warnHookDir, users are able to phase out
> >      .git/hooks scripts at their own pace.
> >  - Abstract most of this via a 'git hooks' command.
> >    - 'git hooks add <hookname> [--<scope>] <path>' to append another hook;
> >      let the scope setting match 'git config'.
> >    - 'git hooks show [<hookname>]' to indicate which hooks will be run, either
> >      on a specific event or for all events, and in which order, as well as each
> >      hook's config origin
> >    - 'git hooks edit <hookname>' to modify the hook order, or choose not to run
> >      a hook locally
> >    - By managing it through a command, we can reorder the contents of config
> >      files if it makes sense to do so.
> >  - Add a hook library.
> >    - Optionally specify all hook events via enum, so we aren't string-typing
> >      calls to find_hook() anymore.
> >    - Resolve configs into a list of hooks to run by concatenating them together
> >      in config precedence order.
> >      - Also allow configs formatted like "-<path>" to remove an
> >        earlier-configured invocation of that hook, or "^<path>" to move the
> >        earlier-configured invocation to this point in the execution order
> >    - Move the find_hook() implementation to here, to account for the multihook
> >      ordering
> 
> I think this addresses most of the concerns that I had about ordering.
> It is still a little suboptimal that we're relying on the ordering of
> the config file, since it makes things equivalent to numbered files in
> .d directories hard.

> 
> Possibly as an alternative to the ^ syntax, we could make the hook value
> be of the form "key program", where key is a sort key (e.g., a number)
> and program is the program to run.  We pick normal config file ordering
> if the keys are identical.  Then if the system config wants to have a
> hook that always runs at the end, it can do so easily.

> 
> In addition, we should be sure that attempting to remove a hook which
> doesn't exist isn't an error, since a user might want to set their
> ~/.gitconfig file to always unset a global hook that may or may not
> exist.

> 
> We also need to address exit codes with multiple hooks and whether we
> fail fast or not.  My series may provide some valuable options here, or
> we may choose to go with a single, simpler approach.  Whatever we do, we
> should document the behavior clearly.

> On Sat, Nov 16, 2019 at 05:45:01AM +0000, brian m. carlson wrote:
> > On 2019-11-16 at 01:11:25, Emily Shaffer wrote:
> > > Here's my suggestion.
> > > 
> > >  - Let configs handle the hook list and ordering, via 'hook.{hookname}' which
> > >    can be specified more than once.
> > >    - global config: hook.update = /path/to/firsthook
> > >      user config: hook.update = /path/to/secondhook
> > >      worktree config: hook.update = -/path/to/firsthook #eliminate the firsthook
> > >       call
> > >    - global config: hook.update = /path/to/firsthook
> > >      repo config: hook.update = /path/to/secondhook
> > >      repo config: hook.update = ^/path/to/firsthook #move firsthook execution
> > >        after secondhook for this project
> > 
> > I'd like to hear more about how we handle multiple hooks that are
> > repo-specific and don't live in the PATH.  This is a common situation
> > for existing tools that handle multiple hooks, and it's one I think we
> > should support.
> 
> I guess I'm confused about where PATH comes into play. Do you mean that
> the hook being run relies on PATH to be set appropriately? I had
> envisioned absolute paths in the config.

> > Perhaps we add a "git hook execute" subcommand that executes scripts
> > from the hook directory.
> 
> Can you give an example of when you'd use it? I'm not understanding your
> concern and I think an example use case would help me see what you mean.

> > I think this addresses most of the concerns that I had about ordering.
> > It is still a little suboptimal that we're relying on the ordering of
> > the config file, since it makes things equivalent to numbered files in
> > .d directories hard.
> 
> Hm, I suppose I don't see why, if the final ordering is determined by
> the .git/config (or future replacement for that). Can you explain what
> you mean? I want to understand where you're coming from.

> > Possibly as an alternative to the ^ syntax, we could make the hook value
> > be of the form "key program", where key is a sort key (e.g., a number)
> > and program is the program to run.  We pick normal config file ordering
> > if the keys are identical.  Then if the system config wants to have a
> > hook that always runs at the end, it can do so easily.
> 
> Interesting. This way if you decide after you've set up all your configs
> just so that you really want something to run at the end of the update
> event, you can change one place, not n=number of Git repos. (I do still
> want to be able to say "don't run that global hook in this project"
> though.)

> > In addition, we should be sure that attempting to remove a hook which
> > doesn't exist isn't an error, since a user might want to set their
> > ~/.gitconfig file to always unset a global hook that may or may not
> > exist.
> 
> I'd be comfortable with a warning when exiting 'git hook edit' mode and
> silence when actually running the hook list.

> On 2019-11-18 at 22:38:19, Emily Shaffer wrote:
> > On Sat, Nov 16, 2019 at 05:45:01AM +0000, brian m. carlson wrote:
> > > On 2019-11-16 at 01:11:25, Emily Shaffer wrote:
> > > > Here's my suggestion.
> > > > 
> > > >  - Let configs handle the hook list and ordering, via 'hook.{hookname}' which
> > > >    can be specified more than once.
> > > >    - global config: hook.update = /path/to/firsthook
> > > >      user config: hook.update = /path/to/secondhook
> > > >      worktree config: hook.update = -/path/to/firsthook #eliminate the firsthook
> > > >       call
> > > >    - global config: hook.update = /path/to/firsthook
> > > >      repo config: hook.update = /path/to/secondhook
> > > >      repo config: hook.update = ^/path/to/firsthook #move firsthook execution
> > > >        after secondhook for this project
> > > 
> > > I'd like to hear more about how we handle multiple hooks that are
> > > repo-specific and don't live in the PATH.  This is a common situation
> > > for existing tools that handle multiple hooks, and it's one I think we
> > > should support.
> > 
> > I guess I'm confused about where PATH comes into play. Do you mean that
> > the hook being run relies on PATH to be set appropriately? I had
> > envisioned absolute paths in the config.
> 
> In past discussions, there's been an assumption that hooks in the config
> will be found in PATH if they're not specified explicitly, and I assumed
> (apparently incorrectly) that the same would be true here.

> 
> It's also common for folks to use something like "bundle exec" in a hook
> to run a linter that's installed by the local package manager, and in
> order to do that, you have to honor PATH to find the package manager's
> binary.  That could be in a variety of places, and it could end up
> changing dynamically in a session due to a tool like RVM.

> 
> > > Perhaps we add a "git hook execute" subcommand that executes scripts
> > > from the hook directory.
> > 
> > Can you give an example of when you'd use it? I'm not understanding your
> > concern and I think an example use case would help me see what you mean.
> 
> Sure.  Currently, if I have pre-push hook, it lives in
> .git/hooks/pre-push.  Now, I want to have multiple hooks for that which
> are specific to my repo.  Maybe I've stuffed them into
> .git/hooks/pre-push.d/hook1 and .git/hooks/pre-push.d/hook2, since
> that's where my previous hook management system put them, but I now want
> to use those same hooks with the config style and drop the old tool.
> 
> I'd like to use "git hook execute pre-push.d/hook1" and "git hook
> execute pre-push.d/hook2" to automatically find the right hooks and
> invoke them.  Similarly, I could use "git hook execute pre-push" to
> execute the old pre-push hook.

> 
> I suppose if we continue to keep the existing behavior of changing the
> directory and we pass the config options to the shell, then we could
> just write "$(git config core.hooksPath || echo
> .git/hooks)/pre-push.d/hook1" instead, which, while ugly, gets the job
> done.  Then we wouldn't need such a command.

> > > I think this addresses most of the concerns that I had about ordering.
> > > It is still a little suboptimal that we're relying on the ordering of
> > > the config file, since it makes things equivalent to numbered files in
> > > .d directories hard.
> > 
> > Hm, I suppose I don't see why, if the final ordering is determined by
> > the .git/config (or future replacement for that). Can you explain what
> > you mean? I want to understand where you're coming from.
> 
> One of the benefits to using numbered files in a .d directory is that
> you can explicitly control ordering of operations.  For example, maybe I
> have a per-repo pre-push hook that performs some checks and rejects a
> push if something is off.  I also have a pre-push hook for Git LFS that
> pushes the Git LFS objects to the remote server if Git LFS is in use.
> 
> In this case, I'd always want my sanity-check hook to run first, and so
> I'd number it first.  This is fine if both are per-repo, but if the LFS
> hook is global, then it's in the wrong order and my LFS objects are
> pushed even though my sanity check failed.

> 
> > > Possibly as an alternative to the ^ syntax, we could make the hook value
> > > be of the form "key program", where key is a sort key (e.g., a number)
> > > and program is the program to run.  We pick normal config file ordering
> > > if the keys are identical.  Then if the system config wants to have a
> > > hook that always runs at the end, it can do so easily.
> > 
> > Interesting. This way if you decide after you've set up all your configs
> > just so that you really want something to run at the end of the update
> > event, you can change one place, not n=number of Git repos. (I do still
> > want to be able to say "don't run that global hook in this project"
> > though.)
> 
> Exactly.  A global or per-user commit-msg hook may want to see the final
> message before approving or rejecting it, and that wouldn't be possible
> without some sort of ordering.
> 
> I strongly agree that we should still allow removing higher-level hooks.
> 
> > > In addition, we should be sure that attempting to remove a hook which
> > > doesn't exist isn't an error, since a user might want to set their
> > > ~/.gitconfig file to always unset a global hook that may or may not
> > > exist.
> > 
> > I'd be comfortable with a warning when exiting 'git hook edit' mode and
> > silence when actually running the hook list.
> 
> Yeah, that's what I'm going for.
> -- 
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204

> Ah, I think I see what you mean.
> 
> hook.update = security-heuristic-runner
> 
> where "security-heuristic-runner" is some compiled binary your employer
> purchased from some vendor and distributed directly to your `/bin/`.
> 
> No, I had imagined users would achieve that by writing:
> 
> hook.update = /bin/security-heuristic-runner

> Hm. Do you mean:
> 
> hook.update = git grep "something"
> 
> Or do you mean:
> 
> hook.update = ~/grephook.sh
> 
> grephook.sh:
>   #!/bin/bash
> 
>   git grep "something" >output
>   ... do something with output ...

> I suppose I need to understand better how $PATH works with the latter
> scenario, but my gut says "if you didn't worry about where to find the
> Git binary from your script before, why are you going to start caring
> now".
> 
> This led me to wonder: "Should we allow someone to pass arguments via
> the hook config?" Or to put it another way, "Should we allow 'hook.update
> = grep -Rin blahblah >audit.log'?" I think the answer is no - some hooks
> do expect to be given arguments, for example
> sequencer.c:run_prepare_commit_msg_hook().

> > I suppose if we continue to keep the existing behavior of changing the
> > directory and we pass the config options to the shell, then we could
> > just write "$(git config core.hooksPath || echo
> > .git/hooks)/pre-push.d/hook1" instead, which, while ugly, gets the job
> > done.  Then we wouldn't need such a command.
> 
> Yeah, I am wondering about when you want to run a hook generically (i.e.
> from a noninteractive script) but outside of the context of something in
> the Git binary invoking a hook. Are you thinking of Git commands
> implemented as scripts?

> > One of the benefits to using numbered files in a .d directory is that
> > you can explicitly control ordering of operations.  For example, maybe I
> > have a per-repo pre-push hook that performs some checks and rejects a
> > push if something is off.  I also have a pre-push hook for Git LFS that
> > pushes the Git LFS objects to the remote server if Git LFS is in use.
> > 
> > In this case, I'd always want my sanity-check hook to run first, and so
> > I'd number it first.  This is fine if both are per-repo, but if the LFS
> > hook is global, then it's in the wrong order and my LFS objects are
> > pushed even though my sanity check failed.
> 
> Yeah, this is really compelling, and also removes the somewhat wonky ^
> proposed just below here. I like this idea quite a lot:
> 
> hook.pre-push = 001:/path/to/sanity-checker

> On 2019-11-23 at 01:19:24, Emily Shaffer wrote:
> > Ah, I think I see what you mean.
> > 
> > hook.update = security-heuristic-runner
> > 
> > where "security-heuristic-runner" is some compiled binary your employer
> > purchased from some vendor and distributed directly to your `/bin/`.
> > 
> > No, I had imagined users would achieve that by writing:
> > 
> > hook.update = /bin/security-heuristic-runner
> 
> Yeah, that's what I'm looking for.  The problem is that, for example,
> Debian does not guarantee where in PATH a file is.  Having a newer Git
> on RHEL or CentOS systems often involves hacking PATH and
> LD_LIBRARY_PATH.
> 
> > Hm. Do you mean:
> > 
> > hook.update = git grep "something"
> > 
> > Or do you mean:
> > 
> > hook.update = ~/grephook.sh
> > 
> > grephook.sh:
> >   #!/bin/bash
> > 
> >   git grep "something" >output
> >   ... do something with output ...
> 
> I had intended to include the latter case, but also allow valid hooks
> with multiple argument support.  For example, you could invoke "git lfs
> pre-push" directly in your hook, and that is a fully functioning
> pre-push hook, and would require a suitable PATH lookup to find your Git
> binary.  It accepts the additional arguments that pre-push hooks accept;
> right now we basically do the following instead:
> 
> ----
> #!/bin/sh
> 
> exec git lfs pre-push "$@"
> ----
> 
> > I suppose I need to understand better how $PATH works with the latter
> > scenario, but my gut says "if you didn't worry about where to find the
> > Git binary from your script before, why are you going to start caring
> > now".
> > 
> > This led me to wonder: "Should we allow someone to pass arguments via
> > the hook config?" Or to put it another way, "Should we allow 'hook.update
> > = grep -Rin blahblah >audit.log'?" I think the answer is no - some hooks
> > do expect to be given arguments, for example
> > sequencer.c:run_prepare_commit_msg_hook().
> 
> I think what we want to do in this case is just invoke things in the
> shell with extra arguments, like we do with editors.  This means we
> don't have to handle PATH or anything else; we just invoke the shell and
> let it handle it.  That lets people provide multi-call binaries (like
> git lfs) that include hooks inside them.

> 
> > > I suppose if we continue to keep the existing behavior of changing the
> > > directory and we pass the config options to the shell, then we could
> > > just write "$(git config core.hooksPath || echo
> > > .git/hooks)/pre-push.d/hook1" instead, which, while ugly, gets the job
> > > done.  Then we wouldn't need such a command.
> > 
> > Yeah, I am wondering about when you want to run a hook generically (i.e.
> > from a noninteractive script) but outside of the context of something in
> > the Git binary invoking a hook. Are you thinking of Git commands
> > implemented as scripts?
> 
> I'm just thinking about existing hook wrappers that invoke multiple
> scripts at the moment, something like how
> https://gist.github.com/mjackson/7e602a7aa357cfe37dadcc016710931b works
> at the moment and how we'd replace that with a config-based model.
> 
> I think using the shell avoids the entire proposal, because it then
> becomes trivial to script that in the command and port it over, since we
> can use my ugly hack above.

> I think I like that better than "git hook
> execute" because it's a little more flexible.
> 
> > > One of the benefits to using numbered files in a .d directory is that
> > > you can explicitly control ordering of operations.  For example, maybe I
> > > have a per-repo pre-push hook that performs some checks and rejects a
> > > push if something is off.  I also have a pre-push hook for Git LFS that
> > > pushes the Git LFS objects to the remote server if Git LFS is in use.
> > > 
> > > In this case, I'd always want my sanity-check hook to run first, and so
> > > I'd number it first.  This is fine if both are per-repo, but if the LFS
> > > hook is global, then it's in the wrong order and my LFS objects are
> > > pushed even though my sanity check failed.
> > 
> > Yeah, this is really compelling, and also removes the somewhat wonky ^
> > proposed just below here. I like this idea quite a lot:
> > 
> > hook.pre-push = 001:/path/to/sanity-checker
> 
> I think a colon is actually better than my proposal for a space in this
> regard, but I'm not picky: anything unambiguous is fine.

> 
> > I'll have to ponder on the UX of a 'git hook'-facilitated interactive
> > edit of the hook numbering, though. UX is not my strong point :)
> 
> I also find I'm not great at UX, so I can't be of much help.

> I think that you are saying we should do the nice equivalent of:
> 
>   system("git lfs pre-push");
> 
> and tack the args onto the end. (I suppose that's run-command.h, but I'm
> trying to use a shorthand that is easy to understand.)

> It seems like this would solve the PATH issue, yes. However, I feel
> tentative because of pushback on that approach in the bugreport review.
> Hmmm. I think this is different, because the user understands that the
> hook they configure themselves will be invoked on the shell of their
> choosing. That is, I think run-command.h with "C:\myhook.exe" would
> still work, no?

> > I do, however, think we should require folks to have a suitable hook
> > that accepts the right arguments.  So "git grep blahblah" isn't a valid
> > hook in most cases, because it doesn't take the right arguments and read
> > the right data from stdin if necessary.
> 
> I'm not sure how we would guarantee that. Are you suggesting we should
> try dry running a hook somehow when it's added with 'git hook add'? Even
> doing that won't stop someone from popping open ~/.gitconfig with nano
> and adding their hook that doesn't take the right args that way.

> > I'm just thinking about existing hook wrappers that invoke multiple
> > scripts at the moment, something like how
> > https://gist.github.com/mjackson/7e602a7aa357cfe37dadcc016710931b works
> > at the moment and how we'd replace that with a config-based model.
> > 
> > I think using the shell avoids the entire proposal, because it then
> > becomes trivial to script that in the command and port it over, since we
> > can use my ugly hack above.
> 
> Still not sure I understand what the issue was before. Is it that the
> trampoline script needs to know $PATH to be able to invoke the child
> hooks in hookname.d? Or it's because the current working directory
> isn't clear, so a relative path in the trampoline script may not be
> resolved well?

> On 2019-11-25 at 22:21:13, Emily Shaffer wrote:
> > I think that you are saying we should do the nice equivalent of:
> > 
> >   system("git lfs pre-push");
> > 
> > and tack the args onto the end. (I suppose that's run-command.h, but I'm
> > trying to use a shorthand that is easy to understand.)
> 
> Yeah, I'm proposing we run the command using run-command.c with the
> use_shell flag, which does something very much like that, except a
> little bit saner.
> 
> > It seems like this would solve the PATH issue, yes. However, I feel
> > tentative because of pushback on that approach in the bugreport review.
> > Hmmm. I think this is different, because the user understands that the
> > hook they configure themselves will be invoked on the shell of their
> > choosing. That is, I think run-command.h with "C:\myhook.exe" would
> > still work, no?
> 
> This will always use /bin/sh, as we do for editors.
> 
> bash on Windows does understand the Windows paths and works correctly in
> this case, AIUI, so that should be fine.
> 
> > Will this approach "just work" for Windows, et al.?
> 
> Yes.  Windows ships with bash (or in Portable Git, busybox sh), which is
> not only required to run the testsuite, but required to invoke editors.
> 
> > > I do, however, think we should require folks to have a suitable hook
> > > that accepts the right arguments.  So "git grep blahblah" isn't a valid
> > > hook in most cases, because it doesn't take the right arguments and read
> > > the right data from stdin if necessary.
> > 
> > I'm not sure how we would guarantee that. Are you suggesting we should
> > try dry running a hook somehow when it's added with 'git hook add'? Even
> > doing that won't stop someone from popping open ~/.gitconfig with nano
> > and adding their hook that doesn't take the right args that way.
> 
> We don't need to guarantee that.  We just need to document it, so that
> (a) people write hooks in the expected way and (b) if people don't, we
> can point them to the documentation and explain why their hooks don't
> work.  I can see people thinking of this as a set of commands that just
> checks exit statuses, and there's likely to be confusion.
> 
> > > I'm just thinking about existing hook wrappers that invoke multiple
> > > scripts at the moment, something like how
> > > https://gist.github.com/mjackson/7e602a7aa357cfe37dadcc016710931b works
> > > at the moment and how we'd replace that with a config-based model.
> > > 
> > > I think using the shell avoids the entire proposal, because it then
> > > becomes trivial to script that in the command and port it over, since we
> > > can use my ugly hack above.
> > 
> > Still not sure I understand what the issue was before. Is it that the
> > trampoline script needs to know $PATH to be able to invoke the child
> > hooks in hookname.d? Or it's because the current working directory
> > isn't clear, so a relative path in the trampoline script may not be
> > resolved well?
> 
> I think we need to have either (a) a way to explicitly invoke hooks
> underneath the hook directory or (b) a shell invocation to allow looking
> up the hook directory.  People want to have per-repository hooks that
> are not a part of the project, and they need a place to store them,
> which is logically the hook directory.
> 
> If we want to allow people to have multiple hooks under something like
> .git/hooks/pre-push.d, then we need to have a way to wire them up in the
> configuration using the correct location of the hook directory instead
> of using a helper script like the one I linked above.
> 
> We don't know that the hook directory will necessarily be under
> .git/hooks, so if the user has moved it elsewhere, we'd want to follow
> that.  A relative path would be wrong if the user changed the hook
> directory to a different location.

> Hopefully I am not beating a dead horse here but I really want to
> understand. Let me take another guess with examples at what you mean;
> please correct me!
> 
> For our purposes, let's assume:
> 
> .git/hooks/
>   update
>   update.d/
>     update-1
>     update-2
> 
> update:
>   #!/bin/bash
> 
>   ./git/hooks/update.d/update-1 &&
>   ./git/hooks/update.d/update-2
> 
> The goal is to make sure update-1 and update-2 are run when other update
> hooks happen.

> With my proposal as is, I see two options:
> 
> 1)
> .git/config:
>   hook.runHookDir = true
> 
> This will run whatever else is in hook.update, and then it will run
> .git/hooks/update. This is not ideal because hookDir could change, and
> then the content of update will be incorrect:
> 
>   git config --add core.hookdir=~/hooks/
>   mv .git/hooks/update* ~/hooks/
>   # call something which invokes update hooks
>   # ~/hooks/update fails, .git/hooks/update-1 is gone :(
> 

> .git/config:
>   hook.update = 001:/project/path/.git/hooks/update.d/update-1
>   hook.update = 002:/project/path/.git/hooks/update.d/update-2
> 
> This will run each update hook individually and have no notion of
> whether they're in a "hook dir" or not. It sees a path, it hands it to
> 'sh' to run, it checks the return code.

> It sounds like you might be asking for something like:
> 
> .git/config:
>   hook.update = 001:__HOOKDIR__/update.d/update-1
> 
> I'm not sure that I like the idea of this. My own dream is to eliminate
> the need for a hookdir entirely, so it's logically easy for my Gerrit
> hook to live in ~/gerrit-tools/ and my linter to live in ~/clang-tidy/
> and so on.

> I could see option 1 being alleviated by writing it as something like:
> 
> update:
>   $GIT_HOOKDIR/update.d/update-1 &&
>   $GIT_HOOKDIR/update.d/update-2
> 
> or
> update:
>   $(git config core.hookdir)/update.d/update-1 &&
>   $(git config core.hookdir)/update.d/update-2

> To quote you out-of-order now:
> 
> > If we want to allow people to have multiple hooks under something like
> > .git/hooks/pre-push.d, then we need to have a way to wire them up in the
> > configuration using the correct location of the hook directory instead
> > of using a helper script like the one I linked above.
> 
> I think I may spot the misunderstanding. It sounds like you hope someone
> can provide "hook.update=001:~/update.d/" and have all the contents of
> update.d executed. I'll be clear and say that I didn't have the
> intention to support that at all; instead I was hoping to support a case
> like 2. above. Recursing through directories like that sounds scary to
> order.

> On 2019-11-26 at 00:56:14, Emily Shaffer wrote:
> > Hopefully I am not beating a dead horse here but I really want to
> > understand. Let me take another guess with examples at what you mean;
> > please correct me!
> > 
> > For our purposes, let's assume:
> > 
> > .git/hooks/
> >   update
> >   update.d/
> >     update-1
> >     update-2
> > 
> > update:
> >   #!/bin/bash
> > 
> >   ./git/hooks/update.d/update-1 &&
> >   ./git/hooks/update.d/update-2
> > 
> > The goal is to make sure update-1 and update-2 are run when other update
> > hooks happen.
> 
> 
> 
> > With my proposal as is, I see two options:
> > 
> > 1)
> > .git/config:
> >   hook.runHookDir = true
> > 
> > This will run whatever else is in hook.update, and then it will run
> > .git/hooks/update. This is not ideal because hookDir could change, and
> > then the content of update will be incorrect:
> > 
> >   git config --add core.hookdir=~/hooks/
> >   mv .git/hooks/update* ~/hooks/
> >   # call something which invokes update hooks
> >   # ~/hooks/update fails, .git/hooks/update-1 is gone :(
> > 
> 
> This is actually fine.  We assume the user knows where they want to
> store their hooks.  If they change that, then that's intentional.
> 
> > .git/config:
> >   hook.update = 001:/project/path/.git/hooks/update.d/update-1
> >   hook.update = 002:/project/path/.git/hooks/update.d/update-2
> > 
> > This will run each update hook individually and have no notion of
> > whether they're in a "hook dir" or not. It sees a path, it hands it to
> > 'sh' to run, it checks the return code.
> 
> Correct.  This is the logical porting of the above shell script to the
> config syntax if you use an absolute path.
> 
> > Now I run 'mv .git/hooks/update* ~/hooks/'. Next time I invoke something
> > which would run the 'update' hook, it fails, because the path in the
> > config isn't pointing to anything. But this way is unrelated to hookdir.
> 
> Yes, that's correct.
> 
> It sounds like you're thinking of the config approach as completely
> orthogonal to the hook directory.  However, I want to have multiple
> per-repository hooks that do not live within the repository but move
> with it.  The logical place to store those hooks is in the hook
> directory, even if it's not being invoked by Git explicitly.  To use
> that with the config approach so I can have multiple hooks in a useful
> way that's compatible with other tools, I need some way to refer to
> whatever the hook directory is at a given point in time.

> 
> > It sounds like you might be asking for something like:
> > 
> > .git/config:
> >   hook.update = 001:__HOOKDIR__/update.d/update-1
> > 
> > I'm not sure that I like the idea of this. My own dream is to eliminate
> > the need for a hookdir entirely, so it's logically easy for my Gerrit
> > hook to live in ~/gerrit-tools/ and my linter to live in ~/clang-tidy/
> > and so on.
> 
> Using the config syntax eliminates per-repository storage for hooks.  I,
> personally, want to store multiple one-off hooks in my hooks directory.
> I want to use tools that install one-off hooks into my repository
> without needing to depend on the location of those tools in the future.
> I don't want those hooks to live elsewhere on my file system, since that
> makes my repository no longer self contained.
> 
> I want to store those hooks in the hook directory, wherever I've
> configured that, and whatever that happens to be at this point in time.
> 
> I may additionally have tools that live in other locations as well and
> may use other syntaxes to invoke them.  For example, I may install a
> hook that's provided by a Debian package and refer to it using a bare
> program name.
> 
> If your goal is to eliminate the hook directory entirely, then I can't
> say that I'm in support of that.  A design which does that won't meet my
> needs, and it likely won't meet the needs of other users.

> 
> The benefit of your proposed config syntax for me is that it provides a
> standard way to configure multiple hooks.  I still want many of those
> hooks to live in the hook directory, although others may live elsewhere.
> 
> > I could see option 1 being alleviated by writing it as something like:
> > 
> > update:
> >   $GIT_HOOKDIR/update.d/update-1 &&
> >   $GIT_HOOKDIR/update.d/update-2
> > 
> > or
> > update:
> >   $(git config core.hookdir)/update.d/update-1 &&
> >   $(git config core.hookdir)/update.d/update-2
> 
> This is similar to what I want, and why I want to pass it to the shell.
> I can write the following, and everything just works:
> 
> .git/config:
>   hook.update = 001:$(git config core.hookdir || echo .git/hooks)/update.d/update-1
>   hook.update = 002:$(git config core.hookdir || echo .git/hooks)/update.d/update-2
> 
> Wherever I have placed my hooks for this repository, I can refer to
> them with a shell script.  I can also add the following line as well:
> 
> .git/config:
>   hook.update = debian-package-hook update
> 
> …and everything just works.
> 
> > But, I think once you "buy in" to the idea of providing a full path to
> > the final target - not to a trampoline script - in your config, you
> > should forget about the idea of "core.hookdir" having anything to do
> > with it.
> 
> I have no intention of providing a full path to any hook, since that's
> quite brittle.  There are very few paths on my system which can be
> guaranteed not to change, and all of them are things like /bin/sh or
> /usr/bin/env.  If my hooks are in the hook directory (or even in the
> working tree) with a full path and I move that repository, they become
> broken.  If they're shipped by Debian and Debian decides to move the
> command, they're broken.

> If the goal is to support other syntaxes in the future, then
> let's use a prefix character (e.g. !) to denote commands vs.
> non-commands or something like that.  If the goal is security, then I'd
> like to hear more about the security model you're trying to achieve with
> this design.

> 
> > To quote you out-of-order now:
> > 
> > > If we want to allow people to have multiple hooks under something like
> > > .git/hooks/pre-push.d, then we need to have a way to wire them up in the
> > > configuration using the correct location of the hook directory instead
> > > of using a helper script like the one I linked above.
> > 
> > I think I may spot the misunderstanding. It sounds like you hope someone
> > can provide "hook.update=001:~/update.d/" and have all the contents of
> > update.d executed. I'll be clear and say that I didn't have the
> > intention to support that at all; instead I was hoping to support a case
> > like 2. above. Recursing through directories like that sounds scary to
> > order.
> 
> I don't need a particular way to do that, since I can do it already, but
> I do need a way to wire up multiple hooks that are per-repository and
> move with the repository but aren't in the repository.  In other words,
> I need a functional replacement for that approach if we're not going to
> use that approach itself.