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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jun 18, 2021, 21:58 UTC
Message-ID: <20210618215848.794617-1-jonathantanmy@google.com>
In-Reply-To: <YM0IpOFH4Sy9wWaE@google.com>

Show 16 quoted lines

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

Do you mean that the server should put it in a different place, the client should put it in a different place, or both?

Show 15 quoted lines

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

Putting the hooks in a branch does mean that "git fetch" gets these scripts from the same remote (as opposed to, say, putting them in a submodule or referring to them by URL), and I think it's easier to understand (from the user point of view) that branch equals same remote (rather than say that a submodule or URL is considered to be from the same remote if $CRITERIA).

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

OK, this makes sense.

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

I didn't do this, but this is a good idea.

Show 13 quoted lines

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

How would the branch share history with the rest of the project? If you mean that we should allow the users to store hooks as part of the main codebase (within a configurable path, say, /suggested_hooks) and then point both "main" (or whatever the default branch is) and "refs/remotes/origin/suggested-hooks" to the same place, then that makes sense (although I would say that it still sounds better to have separate histories).

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

This sounds like the equivalent of "git checkout --orphan", as you suggested above. We could just write it out in the documentation. I'm not completely opposed to a special command, though.

Show 11 quoted lines

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

Yeah - that is partly why I like the branch idea (something already generally understood by people).

Show 9 quoted lines

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

Junio suggested [1] that we store more information about what's going on, so this might be practical.

[1] https://lore.kernel.org/git/xmqq35thnuqp.fsf@gitster.g/

Show 6 quoted lines

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

OK.

Show 10 quoted lines

> 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

Agreed - at the very least, we would need to write this workflow as a test, and we can just reuse the same workflow in documentation. We just need to discuss what the workflow should be :-) (as it is, one thing that is becoming obvious is that currently people prefer to have verify-before-update instead of auto-update by default).