For this review, I'll just concern myself with overall design and structure. For this patch, overall I think it's better if there's a clear distinction between what we are implementing now and what we are implementing later. > +[[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. I don't understand what "first-class citizen" here means - probably better to just omit that phrase and describe the new way of doing hooks. > +[[config-schema-hook]] > +==== `hook` > + > +Primarily contains subsections for each hook event. These order of these > +subsections defines the hook command execution order The execution order is defined by the order of a multivalue config variable, I think, not the order of subsections? Besides, I believe that there's one subsection per hook event (e.g. hook."pre-commit"), not one subsection per command. > ; 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. [snip] > Hook event subsections can > +also contain per-hook-event settings. If this is not yet implemented, maybe list under "future work". > + > +Also contains top-level hook execution settings, for example, > +`hook.warnHookDir`, `hook.runHookDir`, or `hook.disableAll`. (These settings are > +described more in <<library,Library>>.) I think it's clearer if you list this under "future work" - I didn't see any implementation of this. > +[hook "pre-commit"] > + command = perl-linter > + command = /usr/bin/git-secrets --pre-commit > + > +[hook "pre-applypatch"] > + command = perl-linter > + error = ignore Is "error" implemented? > + > +[hook] > + runHookDir = interactive Same question for "runHookDir". > +[[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 > +---- And the skips. (And several more below which I will skip.) > +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)`* Is there a use case that would need such a function? > +[[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. This seems to contradict patch 8, which teaches Git to use the configs directly without any change to .git/hooks/<hook-event> (at least for certain commit-related hooks). > +[[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. With this schema, and with the "skip" behavior described above (but not implemented in this patch set), rudimentary ordering can already be done; because a hook is removed and reinserted whenever it appears in the config, even a hook X in the system config can be made to run after a hook Y in the worktree config by adding Y then X in the worktree config, and if we want to disable X instead, we can just add "skip" to X.
