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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jun 18, 2021, 21:46 UTC
Message-ID: <20210618214650.792661-1-jonathantanmy@google.com>
In-Reply-To: <xmqq35thnuqp.fsf@gitster.g>

Show 9 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> >  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).
> 
> wants to suggest?  They simply suggest ;-)

Ah yes :-)

Show 6 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.
> 
> Can be run to install?  Or can be run to first inspect?  Or both?

Right now I only have a command that installs, but I can provide the appropriate "cat-file" invocations to inspect them as well.

Show 10 quoted lines

> >  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.
> >
> >  4. To turn off autoupdate, set hook.autoupdate to false. Existing hooks
> >     will remain.
> 
> OK, so "verify even if you implicitly trust" is actively discouraged.

Yes I was thinking of the model in which we already trust upstream, but I agree that verification can be useful. I think we can print the "cat-file" commands needed to verify before installing, and add a mode in which we tell the user that the hooks have been updated (but not automatically install them).

Show 14 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).
> 
> As people mentioned in the previous discussions, "independent of the
> other branches" has advantages and disadvantages.  The most recent
> set of hooks may have some that would not work well with older
> codebase, so care must be taken to ensure any hook works on across
> versions of the main codebase.  Which may not be a huge downside,
> but something users must be aware of.

That's true - and on the flip side, I would presume that the hook-introducing admin would usually want those hooks to apply retroactively too (say, to someone updating a "maint" branch). I think it's more flexible if hooks are in an independent branch, though - (if independent branch) a hook can be written to tolerate an old codebase, but if embedded in the code branch, such a hook cannot apply retroactively to old code.

Show 18 quoted lines

> >  2. When and how should the local repo update its record of the remote's
> >     suggested hooks? If we go with storing the hooks in a branch of a
> >     remote side, this would automatically mean (with the default
> >     refspec) that it would be in refs/remotes/<remote>/<name>. This
> >     incurs network and hard disk cost even if the local repo does not
> >     want to use the suggested hooks, but I think that typically they
> >     would want to use it if they're going to do any work on the repo
> >     (they would either have to trust or inspect Makefiles etc. anyway,
> >     so they can do the same for the hooks), and even if they don't want
> >     to use the remote's hooks, they probably still want to know what the
> >     remote suggests.
> 
> A way to see what changes are made to recommendation would be
> useful, and a branch that mostly linearly progresses is a good way
> to give it to the users.
> 
> Of course, that can be done with suggested hooks inline with the
> rest of the main codebase, too.

That's true.

Show 11 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.
> 
> Meaning clobbered by malicious parties?  Then the whole thing is a
> no-go from security point of view.  Presumably you trust the main
> content enough to work on top of it, so as long as you can trust
> that refs/remotes/origin/hooks to the same degree that you would
> trust refs/remotes/origin/master, I do not think it is a problem.

I meant clobbered by the user - should have made that more clear. To elaborate...

Show 15 quoted lines

> Whatever mechanism you use to materialize an updated version of the
> hooks can and should record which commit on the suggested-hooks
> branch the .git/hooks/* file is based on.  Then when the user wants
> to update to a different version of suggested-hooks (either because
> you auto-detected, or the user issued a command to update), you have
> 
>  - The current version in .git/hooks/*
> 
>  - The old pristine version (you recorded the commit when you
>    updated the .git/hooks/* copy the last time)
> 
>  - The new pristine version (you have a remote-tracking branch).
> 
> and it is not a brain surgery to perform three-way merge to update
> the first using the difference between the second and the third.

...I was having a difficult time figuring out where to store such information (ref? config? I wouldn't be surprised if a user saw a value there and thought that they could change it). Perhaps it could be a separate file like .git/shallow.

I'm not fully convinced that maintaining the ability to retain local hook modifications by supporting three-way merges is important, but if it is, it might be brain surgery to figure out the UX when merge conflicts occur. Normally these conflicts can be written to the worktree and index, but we don't have those in the case of hooks.

Show 12 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.
> 
> I wonder if having it bound as a submodule to a known location in
> the main contents tree makes it easier to manage and more flexible.
> Just like you can update the working tree of a submodule to a
> version that is bound to the superproject tree, or to a more recent
> version of the "branch" in the submodule, you can support workflows
> that allow suggested hooks to advance independent of the main
> contents version and that uses a specific version of suggested hooks
> tied to the main contents.

This would make the hooks dependent on the commit checked out (with perhaps a bit more leeway in that our implementation could be flexible and use a commit later than what's in the gitlink), with its own pros and cons (as you said earlier).