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

From: Derrick Stolee <stolee@gmail.com>
Date: Mar 11, 2021, 19:23 UTC
Message-ID: <33a12a7a-d19c-63b8-f21e-db7e517b0f53@gmail.com>
In-Reply-To: <CAJoAoZk0fSyPSJZ51wNv4XujPA_-hPfh8eMRQPbyeTXFvKJ9+Q@mail.gmail.com>

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

Show 21 quoted lines

> 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.

Show 10 quoted lines

> 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?

(Naturally, deprecating FS Monitor through the hook might not be a reasonable plan.)

Thanks, -Stolee