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

From
Jonathan Tan <hidden>
Date
2021-06-03 22:10:21
Reply to
Albert Cui, 2021-06-03
quoted
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.)
quoted
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.)
quoted
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.)
quoted
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.
quoted
quoted
+* 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).
Thread (26 messages) 26 messages, 8 authors, last reply 2021-06-21
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help