Allow hooks to depend on other hooks

[sds]

When writing a custom hook, it would be nice to know that certain preconditions are already met when executing the hook. These preconditions could have been verified by another hook, so having the ability to run a hook depending on the result of another would be useful.

For example, if we're going to check the contents of some .json file for certain keys, knowing that the file in question is valid JSON would be useful before we attempt further inspection.

Usage

Users can specify hook dependencies in the hook configuration:

PreCommit:
  CheckForKeyInJson:
    depend: JsonSyntax
    ...

Note that depend can be a single hook or an array of hooks.

Benefits

• This could potentially speed up hook runs in the case of failures, since we short circuit the running of other hooks if their dependencies didn't pass
• This allows hooks to incrementally build on each other without duplicating code (i.e. hooks don't need to check that a file is valid; they can rely on their dependencies to verify certain preconditions)

Costs

• Requires the introduction of a dependency resolution system (which would need to warn about cyclical dependencies)
• Execution time would increase depending on complexity of dependency graph (always need to do a traversal up front to ensure there are no cycles)
• Would limit our ability to parallelize hook runs, since some dependency graphs would force serial execution
• Requires a topological sort of hooks based on their dependencies so we execute them in the correct order. Hooks are no longer run in the order they are declared in .overcommit.yml

Questions

• What happens if you SKIP a dependency of another hook?
  • If the dependent hook is required, don't allow the skip (and display message like when trying to skip a required hook)
  • Otherwise skip all dependent hooks (make it clear to the user they were skipped due to a dependency being skipped)
• What happens if a user disables a hook that is a dependency of another hook?
  Fail loudly with a helpful error message before running any hooks
• What happens if a hook dependency returns a warning?
  Warnings are just that: warnings, not show stoppers. Thus a warning would not cause all dependent hooks to fail, only errors would.
• What happens if the user specifies a cyclical dependency?
  Detect cycles before any hooks are run and fail fast with clear error message explaining cycle(s)

Feel free to leave comments or ask questions in this issue.

[jawshooah]

Cool idea! And since it's just configuration, default dependencies can easily be overridden. Have you already started work on an implementation?

[jawshooah]

Also I really don't think performance is much of a concern here. The runtime of a topological sort should not be noticeable for the relatively small number of hooks we currently feature, especially with a sparse dependency matrix.

[jawshooah]

Parallelization is a concern, but still very doable under the stated conditions. I'm not familiar with Ruby-flavored parallelism, but especially if a decent actor-based concurrency model is available, this shouldn't be too difficult to manage.

Edit: Looks like the Celluloid framework may be worth looking into. Then again, I have no experience with multithreaded programming in Ruby and you probably do, so I'll shut up about it if you have something in the works already 😉

[sds]

No work has been done on this yet. I'm trying a different approach of developing these features more in the open.

If this is something you are interested in, I would simply ask that you make a (short) proposal on how you intend to implement a solution. I think this particular feature could be worked on without blocking other development, since most of the logic can be pulled out into a separate DependencyResolver class (or similar) which the HookRunner can ask questions of in order to determine if there are dependency cycles or the topologically sorted order of hooks.

Regarding performance: yes I agree this probably won't be an issue any time soon. I more wanted to point out that as presented it introduces a fixed startup cost that grows with the number of hooks and the dependencies between them, so if the ecosystem of hooks got very large startup time would be affected. We'll cross that bridge when it becomes a problem.

Regarding parallelization: I have experience with multithreading primitives in Ruby, but not actor-based concurrency. I don't know what an ideal solution to this problem would look like, so I'm open to suggestions. However, I think for the purposes of this feature we can safely leave parallelism out of the equation for now.

[steinybot]

This would be very handy. I have a custom hook which adds the issue key to the subject line automatically from the branch name and then I use MessageFormat to verify that the message contains an issue key. This currently doesn't work since they run in parallel (which I cannot disable due to #650).