Re: [PATCH v4 1/9] doc: propose hooks managed by the config

[Emily Shaffer <emilyshaffer@google.com>]

On Wed, Sep 23, 2020 at 03:59:10PM -0700, Jonathan Tan wrote:
> 
> For this review, I'll just concern myself with overall design and
> structure.

Thanks - the design doc is now slightly old, so it's nice to have some
fresh eyes on it.

> 
> 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.

I took a light hand when I checked for this - the topic isn't complete
yet, and there's some work in the design doc which I want to include in
this topic, but which hasn't been sent around (or written) yet.

> 
> > +[[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.

Sure.

> 
> > +[[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.

Ok. Have changed to "The order of variables in these subsections
defines..."

> 
> > ; 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".

Good idea. Done.

> 
> > +
> > +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.

Yeah, this is out of sync with what the implementation ended up looking
like; disableAll might still be a useful thing to include in the initial
feature topic, so I won't remove it, but warnHookDir is not necessary.

> 
> > +[hook "pre-commit"]
> > +  command = perl-linter
> > +  command = /usr/bin/git-secrets --pre-commit
> > +
> > +[hook "pre-applypatch"]
> > +  command = perl-linter
> > +  error = ignore
> 
> Is "error" implemented?

No, have marked it with a comment.

> 
> > +
> > +[hook]
> > +  runHookDir = interactive
> 
> Same question for "runHookDir".

It is in the reroll I'm trying to get out this week :)

> 
> > +[[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.)

Again, this is in the reroll I'm working on.

> 
> > +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?

I'm not sure yet - but I'm not quite ready to cut it from the design
doc, until I finish migrating the existing hooks and know that it's not
needed. At that point it'll make sense to move it into the future work
section.

> 
> > +[[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).

Yeah, I think this needs to be rephrased; at this point locally I've
completely removed the --porcelain patch - I'm pretty sure it needs to
be a format string instead.

> 
> > +[[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.

Yep, that's why reordering is in the future work section :)

The problem with config ordering is like such: if I want everyone in
my enterprise to run 'git-secrets --prepare-commit-msg' as the very last
prepare-commit-msg hook, but I can only ship them an /etc/gitconfig,
then the best I can do is email my users and ask them to run 'git config
hook.prepare-commit-msg.command git-secrets-prepare-commit-msg' in every
new repo and include a 'hookcmd.git-secrets-prepare-commit-msg.command'
config in the /etc/gitconfig I ship. (I mention git-secrets here because
it's possible other hooks could have introduced credential secrets into
my user's commit message after git-secrets already ran.)

 - Emily