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

"Albert Cui via GitGitGadget" [off-list ref] writes:

From: Albert Cui <redacted>

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 <redacted>
Helped-by: Emily Shaffer [off-list ref]
---

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.  

Ditto and throughout the document.

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?

This is a more honest descrition ;-)

OK.

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

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

OK.

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.

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.

On Thu, Mar 18, 2021 at 3:29 PM Junio C Hamano [off-list ref] wrote:

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

Ack, thanks for the correct terminology.

quoted

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

Good point

quoted

+User Goals / Critical User Journeys
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+* As a repository owner / project maintainer,
+
+    ** 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/?

There are two opposite intentions here and the slash is making it confusing.
Some projects have internal and external central repositories, and they want to
encourage developers to push to the external repositories when possible.

quoted

+Design Principles
+~~~~~~~~~~~~~~~~~

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

quoted

+Feature Requirements
+~~~~~~~~~~~~~~~~~~~~
+
+Minimum Feature Set
+^^^^^^^^^^^^^^^^^^^
+    * 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.

quoted

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

I don't think there's a contradiction here. Users should always be able to
decline hooks since it's their own machine, so the goal shouldn't be to prevent
uses from declining setup.

That said, most users want to do the right thing so that their patches
get accepted
e.g. adopt the right code styles via linting, so the goal should be to
make it as easy
as possible for them to do that.

In the ideal case: automation detection is great and there are few
false positives :)

If not, we can't break unattended clones, so the default clone
behavior would always have
to be no-hooks-setup... maybe that speaks to `git clone --recommended-setup`?

+1 we'd need a method for a user to trigger a set up if they change
their minds or
need to recover.

quoted

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

I don't think this is contradicting since the user is consenting, but
maybe the principle
can be clearer:

"Execution of code must require user consent, and users should clearly
understand
what they are consenting to." That is, I would imagine for a prompt
for "always" we'd

1) have clear help text saying that hooks from $REMOTE will be
automatically installed
when they change
2) give an FYI if hooks the hooks do change