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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Sep 23, 2020, 22:59 UTC
Message-ID: <20200923225910.1276940-1-jonathantanmy@google.com>
In-Reply-To: <20200909004939.1942347-2-emilyshaffer@google.com>

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.

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 6 quoted lines

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

Show 7 quoted lines

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

Show 16 quoted lines

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

Show 17 quoted lines

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

Show 21 quoted lines

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