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

On Thu, Mar 11, 2021 at 02:23:03PM -0500, Derrick Stolee wrote:

On 3/11/2021 1:42 PM, Emily Shaffer wrote:

quoted

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 (local).

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.

quoted

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 (local))
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