Re: [PATCH v7 01/17] doc: propose hooks managed by the config

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 1, 2021, 22:11 UTC
Message-ID: <xmqqa6snjv4m.fsf@gitster.c.googlers.com>
In-Reply-To: <20201222000220.1491091-2-emilyshaffer@google.com>

Emily Shaffer <emilyshaffer@google.com> writes:

> +ways, providing an avenue to deprecate these "legacy" hooks if desired. The
> +handling is based on a config `hook.runHookDir`, which is checked against a
> +number of cases:

Don't we want to also warn when the setting "no" or something similar prevents the legacy hook from running, to help users who wonder why their hook scripts are not running? I.e.

> +- "no": the legacy hook will not be run

+- "warn-no": Git will print a warning to stderr before ignoring the
+  legacy hook

> +- "interactive": Git will prompt the user before running the legacy hook
> +- "warn": Git will print a warning to stderr before running the legacy hook
> +- "yes" (default): Git will silently run the legacy hook

Show 5 quoted lines

> +In case this list is expanded in the future, if a value for `hook.runHookDir` is
> +given which Git does not recognize, Git should discard that config entry. For
> +example, if "warn" was specified at system level and "junk" was specified at
> +global level, Git would resolve the value to "warn"; if the only time the config
> +was set was to "junk", Git would use the default value of "yes".

Hmph, instead of complaining "value 'junk' is not recognized" and erroring out? Why?

Show 6 quoted lines

> +[[stage-3]]
> +==== Stage 3
> +
> +`.git/hooks` is removed from the template and the hook directory is considered
> +deprecated. To avoid breaking older repos, the default of `hook.runHookDir` is
> +not changed, and `find_hook()` is not removed.

Presumably, we'll have documentation somewhere that instructs users (who were taught by slashdot and other site to add certain scripts under their .git/hooks/) how to do the equivalent without adding scripts in .git/hooks/ directory and instead using the config mechanism (e.g. "when told to add script X in .git/hooks/, read such an instruction as if telling you to do Y instead") by the time this happens? It probably makes sense to do so as part of stage-2, at which point the users are _ready_ to migrate.

Show 11 quoted lines

> +[[security]]
> +=== Security and repo config
> +
> +Part of the motivation behind this refactor is to mitigate hooks as an attack
> +vector;footnote:[https://lore.kernel.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/]
> +however, as the design stands, users can still provide hooks in the repo-level
> +config, which is included when a repo is zipped and sent elsewhere.  The
> +security of the repo-level config is still under discussion; this design
> +generally assumes the repo-level config is secure, which is not true yet. The
> +goal is to avoid an overcomplicated design to work around a problem which has
> +ceased to exist.

I doubt we want to claim anything about security as part of this series. As you say in the paragraph, .git/config and .git/hooks/ are equally (un)protected and if we decide to punt .git/config security, then not moving away from .git/hooks would not hurt security-wise, either (in other words, security is not a viable motivation behind this series).

And if we stop advertising 'security merit' that does not exist, what remains? Isn't the biggest selling point that an identical set of hook configuration can be shared among multiple repositories, and it allows more than one hook scripts to be triggered by a single "hook event"? There may be other good things we should be able to sell the new mechanism to our users, and we do stress on them, which is done in the motivation section. So...

> +.Comparison of alternatives
> +|===
> +|Feature |Config-based hooks |Hook directories |Status quo

Sorry, but I did not find this table particularly convincing.

The only thing I sense is a hand-wavy desire that "we could make it better than everybody else if we work on it in this area", which can apply equally for other approaches---they could enhance what they already have (e.g. "discoverability & documentation").

As a list of "these are the points we aspire to do better than other people", I think it is an excellent idea to have a table like this here in the documentation. But that is not a "comparison".

Show 18 quoted lines

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

OK.

Have we decided what we do for hooks whose interface is to feed their input from their standard input? The current system, I think, just feeds the single hook by writing into a pipe to it, but if we were to drive multiple hooks, we'd need to write the same thing to each of these hook programs?

Do we have a plan to deal with hooks whose outcome is not just "yes/no", e.g. "proc-receive" hook that munges the list of refs to be updated and the new values for them, or "applypatch-msg" that munges the incoming proposed commit log message? Does the second hook work on the result of the first hook? Do the two hooks work on the vanilla state and their output have to agree with each other?

Show 6 quoted lines

> +[[parallelization]]
> +=== Parallelization with dependencies
> +
> +Currently hooks use a naive parallelization scheme or are run in series.  But if
> +one hook depends on another's output, then users will want to specify those
> +dependencies.

An untold assumption here is that the questions I asked earlier on having more than one hooks that is not just yes/no is something readers know the same answer, and that answer is "the outcome of the first hook is passed along (as if it were the input given by Git directly, if the first hook did not exist), to the second hook. It should be spelled out somewhere before the execution ordering section, I think.