Re: [PATCH v4] hooks: propose project configured hooks

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jun 3, 2021, 22:10 UTC
Message-ID: <20210603221006.458323-1-jonathantanmy@google.com>
In-Reply-To: <CAMbkP-Qkd0EzNvhKLeOU3LCdTDiYKpTmZJqMN5rFDg-WkVMrAw@mail.gmail.com>

Show 18 quoted lines

> > From the implementation point of view, would it be sufficient to just
> > advertise that hooks are available? Assuming that the hooks will be
> > available from a specially named ref (as stated below), then we would
> > only need to inform the user that this ref exists and hooks can be
> > inspected using a special command. Likewise for when we fetch and notice
> > that the ref now points to a different object. Then, we wouldn't need to
> > do any extra fetching upon clone/fetch, saving time and bandwidth, but
> > just do so if the user requests it.
> >
> 
> From a user perspective, I think it's better to not just tell users
> that hooks are available but also /what/ hooks are available.
> 
> That said, that doesn't require getting everything from the ref as the
> hook command itself might be stored in this ref. So something like
> this seems sufficient to me: "Origin suggests setting up a `pre-push`
> hook which runs `pre_push.sh`. To view the contents of `pre_push.sh`,
> run {special command}."

I agree that on its own, more information upfront is better than less. But we can't store anything more than an object ID (a SHA-1 hash) in a ref, though. Knowing anything beyond the object ID would require fetching some objects, which incurs time and bandwidth (perhaps comparatively a tiny amount in well-behaved repos, but we still need to acknowledge that). (Well, there is also the possibility of using multiple refs to communicate which hooks are present and which are not, but I think that's too cumbersome.)

Show 7 quoted lines

> > Right now hooks are fixed files (well, not counting Emily Shaffer's work
> > on config hooks). Would it be sufficient to just provide replacements
> > for those files?
> >
> 
> My thought was we'd leverage config hooks and basically write to the
> config for setting up hooks.

The config hooks work is still in progress, so I think we need to support both the legacy way and the new way. I think this shouldn't be a problem since both ways involve writing something somewhere.

In any case, if the aim is to support config hooks, I think it's clearer if this section just says "write a config" without explaining what the config can do, since that is the responsibility of the config hooks feature. (But then there remains the question of whether there will be a whitelist of acceptable configs to write.)

Show 9 quoted lines

> > Hmm...what would be a use case of this? And how would, say, a pre-commit
> > hook know which remote it is for?
> >
> 
> For a concrete example, we use Gerrit for internal reviews and need
> the Change-Id hook, but we don't want that when upstreaming.
> 
> Good question for specifying remotes. This might imply the need for
> something like `git commit --hooks-from=origin`.

I think passing a parameter to choose which hook to invoke is beyond the scope of upstream-supplied hooks. (Also, I don't think that this situation is resolved by having 2 remotes providing 2 different sets of hooks. Perhaps one of the remote's set knows to set the Change-Id trailer, but the other set would not know that it needs to remove a specific trailer.)

Show 11 quoted lines

> > This seems contradictory to a point above where we only inform the user
> > upon clone (when the user is in the setup mood).
> >
> 
> Good catch, I should clarify that previous point. The main concern is
> prompting before a hook will execute which would get in the way of
> existing workflows, making users susceptible to blindly agreeing.
> Showing advice after the fact doesn't get in the way, and this is one
> reason why "advice" felt like the right terminology to use (more
> below): it's merely a helpful message that a user can optionally
> choose to follow.

Ah, I see. I'm still not sure that it is a good idea. Firstly, the user should have already made this decision when they read the message at clone time, and secondly, this advice might come too late (a pre-commit hook might be "salvageable" but a push hook might not). Having said that, the presence or absence of this doesn't affect the overall design and implementation, so we can leave this in if you want.

Show 18 quoted lines

> > > +* If, after fetch, the central repository suggests new or updated hooks, the
> > > +user should receive advice to install these new hooks (note: implementation
> > > +should not interfere with requirement listed in "Fast Follows")
> >
> > In Git, the term "advice" seems to be used more for extra
> > explanations that you can turn off once you're experienced with Git.
> > Here, these seem like things that we would want to notify users about
> > regardless of experience level, so maybe the word "notification" is more
> > appropriate.
> >
> 
> Another reason to use "advice" here is because the existing system
> allows users to turn off the advice when it's not needed for the
> requirement: "The user should be able to use configuration to turn off
> this advice."
> 
> Do advice settings work on a per-clone basis? If not, I agree "advice"
> is probably not the right term.

Those settings don't work per-clone. My main point, though, is that the user probably would always need to know if there are new hooks.

> No, that's what I meant by "Fast Follows" -- not needed for the initial feature.

Ah, maybe call it "future work" then. "Fast follows" is not a term I'm familiar with (and searching online, it doesn't seem like a common term either).