Re: [RFC PATCH 0/2] MVP implementation of remote-suggested hooks

[Emily Shaffer <emilyshaffer@google.com>]

On Wed, Jun 16, 2021 at 04:31:47PM -0700, Jonathan Tan wrote:
> 
> I have had to make several design choices (which I will discuss later),
> but now with this implementation, the following workflow is possible:
> 
>  1. The remote repo administrator creates a new branch
>     "refs/heads/suggested-hooks" pointing to a commit that has all the
>     hooks that the administrator wants to suggest. The hooks are
>     directly referenced by the commit tree (i.e. they are in the "/"
>     directory).

I don't really like that this is in the same namespace as branches users
could create themselves. Hm, I think for 'git maintenance' prefetching
we put those refs in some special namespace, right? Can we do something
similar in this case? Would that prevent us from treating that ref like
a normal branch?

> 
>  2. When a user clones, Git notices that
>     "refs/remotes/origin/suggested-hooks" is present and prints out a
>     message about a command that can be run.
> 
>  3. If the user runs that command, Git will install the hooks pointed to
>     by that ref, and set hook.autoupdate to true. This config variable
>     is checked whenever "git fetch" is run: whenever it notices that
>     "refs/remotes/origin/suggested-hooks" changes, it will reinstall the
>     hooks.

I think this is where most people will have a lot to say from security
perspective. It will be important to make sure that:
 - "git fetch" only automatically updates these scripts when fetching
   from the same remote they were initially fetched from, over some
   secure protocol
 - hook.autoupdate setting maybe should not be default, or should at
   least be able to override during the install command. (One suggestion
   was to also set some experimental.suggestedHooks setting, or
   something, to turn on "automatically set up autoupdate" behavior?)
 - we should notify the user that their hooks were updated/reinstalled
   when that happens later on. (Maybe you did this, I didn't look at the
   diff quite yet)

>  4. To turn off autoupdate, set hook.autoupdate to false. Existing hooks
>     will remain.
> 
> Design choices:
> 
>  1. Where should the suggested hooks be in the remote repo? A branch,
>     a non-branch ref, a config? I think that a branch is best - it is
>     relatively well-understood and any hooks there can be
>     version-controlled (and its history is independent of the other
>     branches).

I agree - a branch without any prior parent sounds ideal to me, that is,
the repo owner setting up the hooks can say 'git checkout --orphan
suggested-hooks'. I guess if they want the branch to have history
shared with the rest of the project there's no reason not to do that,
either.

Do we want to provide helpful tooling for the administrator to create
this magic branch? At the very least I guess we want some documentation,
but I wonder if it's possible to make a helper (maybe a 'git hook'
subcommand) without being more prescriptive than Git project usually is.

>  3. How should the local repo detect when the remote has updated its
>     suggested hooks? I'm thinking when a certain local ref is updated.
>     Right now it's hardcoded, but perhaps "git clone" could detect what
>     "refs/heads/suggested-hooks" would correspond to, and then set it in
>     a config accordingly. Other options include remembering what the
>     remote's "refs/heads/suggested-hooks" used to be and always
>     comparing it upon every "ls-refs" call, but I think that the local
>     ref method is more straightforward.

Hm, I like that idea as it's analogous to remote tracking branch + local
tracking branch (which is a pretty common pattern for people to use).

> 
>  4. Should the local repo try to notice if the hooks have been changed
>     locally before overwriting upon autoupdate? This would be nice to
>     have, but I don't know how practical it would be. In particular, I
>     don't know if we can trust that
>     "refs/remotes/origin/suggested-hooks" has not been clobbered.

I think it would be nice to have, yeah. It seems important to notify the
users that they are executing something different from yesterday, so
noticing when we're making changes to the hook sounds useful.

> 
>  5. Should we have a command that manually updates the hooks with what's
>     in "refs/heads/suggested-hooks"? This is not in this patch set, but
>     it sounds like a good idea.

Yeah, I think so - that lets a user say "no, I don't need those hooks
(or this update)" at first, but later change their mind.

One thing that sounds useful to me, even at this RFC stage, is
documentation showing what this feature looks like to use - for both
the administrator setting up the magic hook branch, and the developer
cloning and using hooks. I think we want that in some Git manpage
eventually anyways, and it would help to see the workflow you're trying
to achieve.

Thanks for sending - will review the diffs today too.

 - Emily

> 
> There are perhaps other points that I haven't thought of, of course.
> 
> [1] https://lore.kernel.org/git/pull.908.git.1616105016055.gitgitgadget@gmail.com/
> 
> Jonathan Tan (2):
>   hook: move list of hooks
>   clone,fetch: remote-suggested auto-updating hooks
> 
>  builtin/bugreport.c |  38 ++------------
>  builtin/clone.c     |  10 ++++
>  builtin/fetch.c     |  21 ++++++++
>  builtin/hook.c      |  13 +++--
>  hook.c              | 118 ++++++++++++++++++++++++++++++++++++++++++++
>  hook.h              |   5 ++
>  t/t5601-clone.sh    |  36 ++++++++++++++
>  7 files changed, 202 insertions(+), 39 deletions(-)
> 
> -- 
> 2.32.0.272.g935e593368-goog
>