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

[Junio C Hamano <gitster@pobox.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

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

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

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

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

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