Re: What to do with fsmonitor-watchman hook and config-based hooks?

[Emily Shaffer <emilyshaffer@google.com>]

On Thu, Mar 11, 2021 at 02:23:03PM -0500, Derrick Stolee wrote:
> 
> On 3/11/2021 1:42 PM, Emily Shaffer wrote:
> > Hi folks, I grabbed a bunch of CC from 'git blame fsmonitor.c' so
> > sorry if you don't care about fsmonitor-watchman anymore... :) Note
> > that this whole conversation has to do with the series proposed at
> > https://lore.kernel.org/git/20210311021037.3001235-1-emilyshaffer@google.com.
> > 
> > When I was looking through the remaining hooks in
> > Documentation/githooks.txt I noticed that the fsmonitor-watchman hook
> > is implemented somewhat differently than most other hooks. As I
> > understand it, to use that hook someone needs to:
> > 
> > 1. Configure core.fsmonitor with a path to some fsmonitor-watchman
> > hook. The documentation in 'Documentation/githooks.txt' claims that it
> > needs to point to '.git/hooks/fsmonitor-watchman' or
> > '.git/hooks/fsmonitor-watchmanv2', but I don't see that constraint
> > enforced when the config is read (config.c:git_config_get_fsmonitor()
> > and fsmonitor.c:query_fsmonitor()), so it seems that core.fsmonitor
> > can point to wherever it wants. (Plus
> > 'templates/blt/hooks/fsmonitor-watchman.sample' suggests setting
> > 'core.fsmonitor' = '.git/hooks/query-watchman'...)
> > 2. Configure core.fsmonitorhookversion to 1 or 2, to indicate the arg
> > structure for the executable specified in core.fsmonitor.
> 
> This is correct.
> 
> > Because the executable doesn't necessarily live in .git/hooks/,
> > fsmonitor.c:query_fsmonitor() completely skips the "API" for running
> > hooks (run-command.h:run_hook_le()) and just uses
> > run-command.h:capture_command() directly.
> > 
> > Interestingly, this is really similar to the way config-based hooks
> > (https://lore.kernel.org/git/20210311021037.3001235-1-emilyshaffer@google.com)
> > work - but different enough that I think it may be awkward to
> > transition fsmonitor-watchman to work like everything else. So, some
> > questions, plus a proposal:
> 
> You'll want to get Jeff Hostetler's perspective first, but I'm of
> the opinion that we'll want to stop recommending the Watchman hook
> when the Git-native FS Monitor feature lands, with some time to
> let things release and simmer before we remove the core.fsmonitor
> config option. We would also need a Linux implementation, but that
> is planned.
> 
> If we think that the plan of "eventually, FS Monitor won't use hooks"
> is reasonable, then how much do you want to spend time unifying it
> with your config-based hooks? Can they live together temporarily?

Oh, that's useful context. If fsmonitor-watchman hook is going away, I
don't think it's necessary to convert it at all, unless someone starts
asking for multihooks or something. There's no practical conflict between
config-based hooks and the current implementation - it's just a
surprising inconsistency. I'll be curious to hear Jeff's opinion, of
course, but given what you're describing, I'm not convinced it's worth
spending any time on - and when we're ready to stop checking
core.fsmonitor then the inconsistency will just go away.

The documentation in githooks.txt could use an update, though. :)

 - Emily