Re: [RFC] Hook management via 'git hooks' command

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 4834 bytes --]

On 2019-11-23 at 01:19:24, Emily Shaffer wrote:
> Ah, I think I see what you mean.
> 
> hook.update = security-heuristic-runner
> 
> where "security-heuristic-runner" is some compiled binary your employer
> purchased from some vendor and distributed directly to your `/bin/`.
> 
> No, I had imagined users would achieve that by writing:
> 
> hook.update = /bin/security-heuristic-runner

Yeah, that's what I'm looking for.  The problem is that, for example,
Debian does not guarantee where in PATH a file is.  Having a newer Git
on RHEL or CentOS systems often involves hacking PATH and
LD_LIBRARY_PATH.

> Hm. Do you mean:
> 
> hook.update = git grep "something"
> 
> Or do you mean:
> 
> hook.update = ~/grephook.sh
> 
> grephook.sh:
>   #!/bin/bash
> 
>   git grep "something" >output
>   ... do something with output ...

I had intended to include the latter case, but also allow valid hooks
with multiple argument support.  For example, you could invoke "git lfs
pre-push" directly in your hook, and that is a fully functioning
pre-push hook, and would require a suitable PATH lookup to find your Git
binary.  It accepts the additional arguments that pre-push hooks accept;
right now we basically do the following instead:

----
#!/bin/sh

exec git lfs pre-push "$@"
----

> I suppose I need to understand better how $PATH works with the latter
> scenario, but my gut says "if you didn't worry about where to find the
> Git binary from your script before, why are you going to start caring
> now".
> 
> This led me to wonder: "Should we allow someone to pass arguments via
> the hook config?" Or to put it another way, "Should we allow 'hook.update
> = grep -Rin blahblah >audit.log'?" I think the answer is no - some hooks
> do expect to be given arguments, for example
> sequencer.c:run_prepare_commit_msg_hook().

I think what we want to do in this case is just invoke things in the
shell with extra arguments, like we do with editors.  This means we
don't have to handle PATH or anything else; we just invoke the shell and
let it handle it.  That lets people provide multi-call binaries (like
git lfs) that include hooks inside them.

I do, however, think we should require folks to have a suitable hook
that accepts the right arguments.  So "git grep blahblah" isn't a valid
hook in most cases, because it doesn't take the right arguments and read
the right data from stdin if necessary.

> > I suppose if we continue to keep the existing behavior of changing the
> > directory and we pass the config options to the shell, then we could
> > just write "$(git config core.hooksPath || echo
> > .git/hooks)/pre-push.d/hook1" instead, which, while ugly, gets the job
> > done.  Then we wouldn't need such a command.
> 
> Yeah, I am wondering about when you want to run a hook generically (i.e.
> from a noninteractive script) but outside of the context of something in
> the Git binary invoking a hook. Are you thinking of Git commands
> implemented as scripts?

I'm just thinking about existing hook wrappers that invoke multiple
scripts at the moment, something like how
https://gist.github.com/mjackson/7e602a7aa357cfe37dadcc016710931b works
at the moment and how we'd replace that with a config-based model.

I think using the shell avoids the entire proposal, because it then
becomes trivial to script that in the command and port it over, since we
can use my ugly hack above.  I think I like that better than "git hook
execute" because it's a little more flexible.

> > One of the benefits to using numbered files in a .d directory is that
> > you can explicitly control ordering of operations.  For example, maybe I
> > have a per-repo pre-push hook that performs some checks and rejects a
> > push if something is off.  I also have a pre-push hook for Git LFS that
> > pushes the Git LFS objects to the remote server if Git LFS is in use.
> > 
> > In this case, I'd always want my sanity-check hook to run first, and so
> > I'd number it first.  This is fine if both are per-repo, but if the LFS
> > hook is global, then it's in the wrong order and my LFS objects are
> > pushed even though my sanity check failed.
> 
> Yeah, this is really compelling, and also removes the somewhat wonky ^
> proposed just below here. I like this idea quite a lot:
> 
> hook.pre-push = 001:/path/to/sanity-checker

I think a colon is actually better than my proposal for a space in this
regard, but I'm not picky: anything unambiguous is fine.

> I'll have to ponder on the UX of a 'git hook'-facilitated interactive
> edit of the hook numbering, though. UX is not my strong point :)

I also find I'm not great at UX, so I can't be of much help.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]