Subject: Re: [PATCH] hooks: propose repository owner configured hooks

From: Junio C Hamano <[email-masked]>
Date: Thu, 18 Mar 2021 15:29:51 -0700

"Albert Cui via GitGitGadget" <[email-masked]> writes:

> From: Albert Cui <[email-masked]>
>
> Hooks today are configured at the repository level, making it difficult to
> share hooks across repositories. Configuration-based hook management, by
> moving hooks configuration to the config, makes this much easier. However,
> there is still no good way for project maintainers to encourage or enforce
> adoption of specific hook commands on specific hook events in a repository.
> As such, there are many tools that provide this functionality on top of Git.
>
> We propose adding native Git functionality to allow project maintainers to
> specify hooks that a user ought to install and utilize in their development
> workflows. This patch documents the requirements we propose for this feature
> as well as a design sketch for implementation.
>
> Signed-off-by: Albert Cui <[email-masked]>
> Helped-by: Emily Shaffer <[email-masked]>
> ---

A copy of Android or Chromium project I have on my disk is owned by
me in the distributed world---open projects do not and do not have
to care who makes and has copies, even though they may care who can
push to the project's own repository.

So avoid "repository owner configured", when you mean "project
configured" or "project controlled".

On the other side of the coin is that this document should avoid
reference to a "repository" in an ambiguous way, as it can refer to
the central meeting place the project controls, lets developers to
clone and fetch from, and push into, and it can also refer to the
copy of that central meeting place individual contributors work in.

In our own documentation, we often refer to the former as "the
central repository", and the latter as "a clone" (as in "you start
working in your own clone").

>     hooks: propose repository owner configured hooks
> ...
>     for this feature as well as a design sketch for implementation.

No need to duplicate this text twice.  

> @@ -0,0 +1,294 @@
> +Repository Owner Hooks Administration
> +-------------------------------------

Ditto and throughout the document.

> +Background
> +~~~~~~~~~~
> +
> +Context
> +^^^^^^^
> +
> +Git has https://git-scm.com/docs/githooks[hooks] functionality to allow users to
> +execute commands or scripts when certain Git events occur. Some use cases
> +include:
> +
> +* The `pre-commit` hook event: before committing, a developer may want to lint
> +their changes to enforce certain code style and quality. If there are style
> +issues, the developer may want the commit to fail so that they can fix the
> +issues.

This is irrelevant in the context of this proposal, no?  It is not
that "the developer may want".

Rather, it is "the project may want the commit to fail so that they
won't upload commits that violate the house style", no?

> +* The `commit-msg` hook event: after committing, repository owners may want to
> +enforce a certain commit message style. This may be out of necessity:
> +https://www.gerritcodereview.com/[Gerrit Code Review], for example, requires
> +each commit to have a Change-Id in the footer.

This is a more honest descrition ;-)

> +We propose adding native Git functionality to allow project maintainers to
> +specify hooks that a user ought to install and utilize in their development
> +workflows.

OK.

> +User Goals / Critical User Journeys
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +* As a repository owner / project maintainer,
> +
> +    ** I want to enforce code style so I require developers to use a
> +    standardized linter.
> +
> +    ** I want to prevent leaks / share code widely so I check that developers
> +    are uploading code to the right place.

I understand "You want to prevent leaks", but not "I want to share
code widely".  Perhaps you meant s/widely/narrowly/?

> +    ** I want this to just work for all the developers in my repository, without
> +    needing to support them through configuration.
> +
> +* As a developer developing in a repository,
> +
> +    ** I want to set up my workspace
> +
> +    ** I want control over what code runs on my machine
> +
> +    ** I want to skip hooks from running (for various reasons)

Also I want to run hooks other people may not run.  And that is one
thing Emily's config based stuff makes easier.

> +Design Principles
> +~~~~~~~~~~~~~~~~~
> +
> +* *Make it easy:* Developers just want to get their work done. Introducing
> +friction is bad, especially when it prevents development from even starting e.g.
> +workspace set up.
> +
> +* *Treat hooks as software, not configuration:* We take seriously the
> +responsibility that comes with causing arbitrary code to run on a user's
> +machine. Such code needs to come from a reputable source, be automatically
> +updated, and run with user consent.

OK.

> +Feature Requirements
> +~~~~~~~~~~~~~~~~~~~~
> +
> +Minimum Feature Set
> +^^^^^^^^^^^^^^^^^^^
> +
> +* A repository owner can specify a configuration for what hook commands are
> +enabled for what hook events
> +
> +* A repository owner can specify, in this configuration, where the hook
> +commands reside
> +
> +    ** This could be a path to a script/binary within the repository
> +
> +    ** This could be a path to a script/binary contained within submodules of
> +    the repository
> +
> +    ** This could be a user installed command or script/binary that exists
> +    outside of the repository and is present in `$PATH`
> +
> +* Users must explicitly approve hook execution at least once (yes/no)
> +
> +    ** This could happen during setup or at execution time
> +
> +    ** When a hook command changes, a user should re-approve execution (note:
> +    implementation should not interfere with requirement listed in “Fast
> +    Follows")
> +
> +    * Users do not need to run setup scripts to install hooks --- the setup flow
> +    happens automatically at clone time

This one is probably unacceptable, as it makes it impossible to
perform unattended cloning.  A better alternative may be to make it
part of the build procedure.

> +* Automation is able to continue to use clone and other commands
> +non-interactively

This directly contradicts with the "setup flow happens
automatically", doesn't it?  The user can pretend to be (or the
"automation" detection may incorrectly misidentify the users to be)
an automated client when cloning the project, and would not trigger
any setup.  A separate setup procedure needs to be there to salvage
such users anyway.

> +* Works across Windows/Linux/macOS
> +
> +Fast Follows
> +^^^^^^^^^^^^
> +
> +* When prompted to execute a hook, users can specify always or never, even if
> +the hook updates

This contradicts the earlier claim to take the responsibility
seriously, doesn't it?  I think the convenience feature is useful,
but then we should tone down the claim---we allow users to be loose
and blindly trust their own project, instead of taking it always
seriously.