On Fri, Mar 12, 2021 at 09:30:04AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>
> On Thu, Mar 11 2021, Emily Shaffer wrote:
>
> > Historically, hooks are declared by placing an executable into
> > $GIT_DIR/hooks/$HOOKNAME (or $HOOKDIR/$HOOKNAME). Although hooks taken
> > from the config are more featureful than hooks placed in the $HOOKDIR,
> > those hooks should not stop working for users who already have them.
> > Let's list them to the user, but instead of displaying a config scope
> > (e.g. "global: blah") we can prefix them with "hookdir:".
> >
> > Signed-off-by: Emily Shaffer <emilyshaffer@xxxxxxxxxx>
> > ---
> >
> > Notes:
> > Since v7, fix some nits from Jonathan Tan. The largest is to move reference to
> > "hookdir annotation" from this commit to the next one which introduces the
> > hook.runHookDir option.
> >
> > builtin/hook.c | 11 +++++++++--
> > hook.c | 17 +++++++++++++++++
> > hook.h | 1 +
> > t/t1360-config-based-hooks.sh | 19 +++++++++++++++++++
> > 4 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/hook.c b/builtin/hook.c
> > index bb64cd77ca..c8fbfbb39d 100644
> > --- a/builtin/hook.c
> > +++ b/builtin/hook.c
> > @@ -40,10 +40,15 @@ static int list(int argc, const char **argv, const char *prefix)
> >
> > list_for_each(pos, head) {
> > struct hook *item = list_entry(pos, struct hook, list);
> > - if (item)
> > + item = list_entry(pos, struct hook, list);
> > + if (item) {
> > + /* Don't translate 'hookdir' - it matches the config */
>
> Let's prefix comments for translators with /* TRANSLATORS: .., see the
> coding style doc. That's what they'll see, and this is useful to them.
>
> Better yet have a note here about the first argument being 'system',
> 'local' etc., which I had to source spelunge for, and translators won't
> have any idea about unless the magic parameter is documented.
It's not a comment for translators. It's a comment for someone helpful
who comes later and says "oh, none of this is marked for translation,
I'd better fix that."
>
> > +setup_hookdir () {
> > + mkdir .git/hooks
> > + write_script .git/hooks/pre-commit <<-EOF
> > + echo \"Legacy Hook\"
>
> Nit, "'s not needed, but it also seems nothing uses this, so if it's
> just a pass-through script either "exit 0", or actually check if it's
> run or something?
The output is checked in the run tests later on. I can remove it for
this commit if you want.
>
> > [...]
> > +test_expect_success 'git hook list shows hooks from the hookdir' '
> > + setup_hookdir &&
> > +
> > + cat >expected <<-EOF &&
> > + hookdir: $(pwd)/.git/hooks/pre-commit
> > + EOF
> > +
> > + git hook list pre-commit >actual &&
> > + test_cmp expected actual
> > +'
>
> Ah, so it's just checking if it exists...
