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@xxxxxxxxxx. > > > > 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@xxxxxxxxxx) > > 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
