Re: [PATCH 8/9] hook: teach 'hookcmd' config to alias hook scripts

[Emily Shaffer <emilyshaffer@google.com>]

On Fri, Aug 06, 2021 at 12:26:02AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> 
> On Thu, Aug 05 2021, Emily Shaffer wrote:
> 
> > On Thu, Aug 05, 2021 at 02:17:29AM +0200, Ævar Arnfjörð Bjarmason wrote:
> >> [...]
> >> > No, that's in fact as designed, with my model B. The user configured
> >> > "echo hi" to run on "verify-commit" events; if those events are
> >> > initially used by some wrapper, but later we decide they're a great idea
> >> > and absorb the verify-commit event into native Git, then this is working
> >> > as intended. I think your argument is based on a misunderstanding of the
> >> > design.
> >> >
> >> > Could it be that the way I provided the examples (my schema after A: and
> >> > your schema after B:) made it hard to parse? Sorry about that if so.
> >> 
> >> Aren't you assuming that users who specify a verify-commit will be happy
> >> because git's usurping of that will 1=1 match what they were using
> >> "verify-commit" for.
> >> 
> >> I'm pointing out that we can't know that, and since you want to make
> >> "git hook run" a general thing that runs any <name> of script you've
> >> configured, and not just what's in githooks(5) that it becomes very
> >> likely that if we add a new hook with some obvious name that we'll
> >> either break things for users, or subtly change behavior.
> >> 
> >> Which isn't just theoretical, e.g. I tend to run something like a "git
> >> log --check @{u}.." before I run git-send-email, with this configurable
> >> hook mechanism having a "git hook run sendemail-check" would be a way I
> >> might expose that in my own ~/.gitconfig.
> >> 
> >> But if git-send-email learns a "sendemail-check" and the behavior
> >> doesn't exactly match mine; E.g. maybe it similar to pre-auto-gc expects
> >> me to return a status code to ask me if I want to abort on a failed
> >> --check, but mine expects a revision range to run "log --check".
> >> 
> >> In practice that's a non-issue with the current hook mechanism,
> >> i.e. nobody's sticking a script into .git/hooks/my-custom-name and
> >> expecting it to do anything useful (and if they are, they have only
> >> themselves to blame).
> >> 
> >> Whereas we'd now be actively inviting users to squat on the same
> >> namespace we ourselves will need for future hooks.
> >
> > Yeah, this is a good point. Seems worth a note in the 'git hook run'
> > doc, making a point that "you can use this for your wrapper to run
> > specific hooks, but be careful about namespace collisions". We're a lot
> > less likely to add a hook named "repotool-verify-commit" than we are to
> > add a hook named "verify-commit", for example.
> >
> > I think it's enough to warn about future namespace collisions and make
> > an "at your own risk" note.
> 
> I might have lost track at this point, but later examples in this E-Mail
> you show don't seem to require such a note.
> 
> I.e. it's only an issue if we conflate a semantically meaningful slot
> like "pre-commit" in the config with one that can also have the meaning
> of simply defining an arbitrary user-decided name.

No, I don't think that's the case. The examples I used later in the mail
don't have anything to do with this "wrapper wants to use 'git hooks
run'" case.

Let's imagine a current commonly-used wrapper, 'repo'
(https://gerrit.googlesource.com/git-repo/+/refs/heads/master/README.md).
This wrapper is used to manage multiple Git repos into something
semantically similar to submodules.

Let's say that 'repo' decides that instead of rolling its own hook
system, it would like to use Git's native hook system. Let's say that it
wants to call user-defined hooks any time someone has just completed
running 'repo sync'. It could ask users to define a config like so:

  [hook "myrepohook"]
    event = post-sync
    command = ~/some/downloaded/myrepohook --with --args

Then 'repo' itself can, at the end of its 'repo sync' implementation,
call 'git hook run post-sync' (instead of using some other hook
specification schema).

But Git itself has a 'git submodule sync' command, and let's imagine
that we decided we want to run some user-defined hook at the end of 'git
submodule sync'. Naturally, we can call it 'post-sync', and then we will
accidentally invoke the hooks which someone configured for 'repo' - as
you pointed out would be a concern with my design, this will *also* be
a concern with your design.

This specific use case is what I think we can get around with some "at
your own risk" documentation, because we are much less likely to collide
with someone asking their users to configure like so:

  [hook "myrepohook"]
    event = repo-tool-post-sync
    command = ~/some/downloaded/myrepohook --with-args

> >> > No, but it's something I'm interested in passing as an environment
> >> > variable. I didn't add it to this series because it seemed to me to
> >> > distract from the core feature. We would like to add it to simplify our
> >> > invocations of https://github.com/awslabs/git-secrets, so it's on my
> >> > radar.
> >> 
> >> Having such an env var as part of the initial series seems like a
> >> sensible thing to have.
> >
> > Eh. To me, it feels like feature creep. It also is something we could
> > add today to the existing hook mechanism (even if it's a little
> > pointless since you can basename, like you say), so it feels orthogonal.
> > I would prefer not to add it in this series.
> 
> Sure, I guess you can add two hook sections to replace e.g. your
> {pre,post}-receive hook (which are commonly routed to the same script
> with file-based hooks). Having a single setenv() seems easy enough, and
> I'd bet a way more common use-case than wanting to skip earlier defined
> hooks...

More than anything else, I think this comment convinced me that the skip
config also falls under YAGNI and I can drop it (and we can add it later
if "hook.myhook.command = true" isn't clear enough).

I snipped the rest because I don't disagree or have anything further to
say. Look for a reroll this weekend, we hope.

 - Emily