Re: [PATCH] doc: propose hooks managed by the config

[Emily Shaffer <emilyshaffer@google.com>]

On Wed, May 06, 2020 at 02:33:54PM -0700, Emily Shaffer wrote:
> 
> On Sat, Apr 25, 2020 at 08:57:27PM +0000, brian m. carlson wrote:
> > 
> > On 2020-04-20 at 23:53:10, Emily Shaffer wrote:
> > > +=== Config schema
> > > +
> > > +Hooks can be introduced by editing the configuration manually. There are two new
> > > +sections added, `hook` and `hookcmd`.
> > > +
> > > +==== `hook`
> > > +
> > > +Primarily contains subsections for each hook event. These subsections define
> > > +hook command execution order; hook commands can be specified by passing the
> > > +command directly if no additional configuration is needed, or by passing the
> > > +name of a `hookcmd`. If Git does not find a `hookcmd` whose subsection matches
> > > +the value of the given command string, Git will try to execute the string
> > > +directly. Hook event subsections can also contain per-hook-event settings.
> > 
> > Can we say explicitly that the commands are invoked by the shell?  Or is
> > the plan to try to parse them without passing to the shell?
> 
> Sure. If I didn't make it clear it was by mistake, not by intent.
> 
> > 
> > > +Also contains top-level hook execution settings, for example,
> > > +`hook.warnHookDir`, `hook.runHookDir`, or `hook.disableAll`.
> > > +
> > > +----
> > > +[hook "pre-commit"]
> > > +  command = perl-linter
> > > +  command = /usr/bin/git-secrets --pre-commit
> > > +
> > > +[hook "pre-applypatch"]
> > > +  command = perl-linter
> > > +  error = ignore
> > > +
> > > +[hook]
> > > +  warnHookDir = true
> > > +  runHookDir = prompt
> > > +----
> > > +
> > > +==== `hookcmd`
> > > +
> > > +Defines a hook command and its attributes, which will be used when a hook event
> > > +occurs. Unqualified attributes are assumed to apply to this hook during all hook
> > > +events, but event-specific attributes can also be supplied. The example runs
> > > +`/usr/bin/lint-it --language=perl <args passed by Git>`, but for repos which
> > > +include this config, the hook command will be skipped for all events to which
> > > +it's normally subscribed _except_ `pre-commit`.
> > > +
> > > +----
> > > +[hookcmd "perl-linter"]
> > > +  command = /usr/bin/lint-it --language=perl
> > > +  skip = true
> > > +  pre-commit-skip = false
> > > +----
> > 
> > This seems fine to me.  I like this design and it seems sane.
> > 
> > > +== Implementation
> > > +
> > > +=== Library
> > > +
> > > +`hook.c` and `hook.h` are responsible for interacting with the config files. In
> > > +the case when the code generating a hook event doesn't have special concerns
> > > +about how to run the hooks, the hook library will provide a basic API to call
> > > +all hooks in config order with an `argv_array` provided by the code which
> > > +generates the hook event:
> > > +
> > > +*`int run_hooks(const char *hookname, struct argv_array *args)`*
> > > +
> > > +This call includes the hook command provided by `run-command.h:find_hook()`;
> > > +eventually, this legacy hook will be gated by a config `hook.runHookDir`. The
> > > +config is checked against a number of cases:
> > > +
> > > +- "no": the legacy hook will not be run
> > > +- "interactive": Git will prompt the user before running the legacy hook
> > > +- "warn": Git will print a warning to stderr before running the legacy hook
> > > +- "yes" (default): Git will silently run the legacy hook
> > > +
> > > +If `hook.runHookDir` is provided more than once, Git will use the most
> > > +restrictive setting provided, for security reasons.
> > 
> > I don't think this is consistent with the way the rest of our options
> > work.  What if someone generally wants to disable legacy hooks but then
> > works with a program in a repository that requires them?
> 
> Unfortunately this is something I think my end will want to hold firm
> on. In general we disagree with your statement later about not wanting
> to make the .git/config secure. I see your use case, and I anticipate
> two possible workarounds I'd present:
> 
> 1) If working in that repo for the short term, run `git -c
> hook.runHookDir=yes <command> <arg...>` (and therefore allow the config
> from command line scope, which I'm happy with in general). Maybe
> someone would want to use an alias, hookgit or hg? Just kidding.. ;P
> 
> 2) If you're stuck with that repo for the long term, add
> `hook.<hookname>.command = /path/.git/hooks/<hookname>` lines to the local
> config.
> 
> Yes, those are both somewhat user-unfriendly, and I think we can do
> better... I'll have to think more and see what I can come up with.
> Suggestions welcome.

I thought more about this and today I'm revisiting this work (and
starting on patches!) so I figured I'd close the loop, since it'll be
buried in the next round of the design doc.

Refusing to trust the local config is actually contrary to one of the
tenets I was trying to use when designing this - that we should assume
the .git/config is safe, so that we don't end up with bloat later if
.git/config does become safe. The suggestion I made here to disallow
overrides doesn't fit, so I'll drop it. The implementation will allow a
more local config to turn hookdir hooks back on.

Thanks, all. By way of status update, I think I'll be able to start
working on this more actively starting this week.

 - Emily