{"thread":{"id":"55294","subject":"What to do with fsmonitor-watchman hook and config-based hooks?","startedAt":"2021-03-11T18:43:18Z","lastAt":"2021-03-12T20:34:04Z","messageCount":6,"participants":["Emily Shaffer","Derrick Stolee","Jeff Hostetler","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"418894","messageId":"CAJoAoZk0fSyPSJZ51wNv4XujPA_-hPfh8eMRQPbyeTXFvKJ9+Q@mail.gmail.com","threadId":"55294","inReplyTo":null,"subject":"What to do with fsmonitor-watchman hook and config-based hooks?","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-03-11T18:42:19Z","receivedAt":"2021-03-11T18:43:18Z","isPatch":false,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Hi folks, I grabbed a bunch of CC from 'git blame fsmonitor.c' so\nsorry if you don't care about fsmonitor-watchman anymore... :) Note\nthat this whole conversation has to do with the series proposed at\nhttps://lore.kernel.org/git/20210311021037.3001235-1-emilyshaffer@google.com.\n\nWhen I was looking through the remaining hooks in\nDocumentation/githooks.txt I noticed that the fsmonitor-watchman hook\nis implemented somewhat differently than most other hooks. As I\nunderstand it, to use that hook someone needs to:\n\n1. Configure core.fsmonitor with a path to some fsmonitor-watchman\nhook. The documentation in 'Documentation/githooks.txt' claims that it\nneeds to point to '.git/hooks/fsmonitor-watchman' or\n'.git/hooks/fsmonitor-watchmanv2', but I don't see that constraint\nenforced when the config is read (config.c:git_config_get_fsmonitor()\nand fsmonitor.c:query_fsmonitor()), so it seems that core.fsmonitor\ncan point to wherever it wants. (Plus\n'templates/blt/hooks/fsmonitor-watchman.sample' suggests setting\n'core.fsmonitor' = '.git/hooks/query-watchman'...)\n2. Configure core.fsmonitorhookversion to 1 or 2, to indicate the arg\nstructure for the executable specified in core.fsmonitor.\n\nBecause the executable doesn't necessarily live in .git/hooks/,\nfsmonitor.c:query_fsmonitor() completely skips the \"API\" for running\nhooks (run-command.h:run_hook_le()) and just uses\nrun-command.h:capture_command() directly.\n\nInterestingly, this is really similar to the way config-based hooks\n(https://lore.kernel.org/git/20210311021037.3001235-1-emilyshaffer@google.com)\nwork - but different enough that I think it may be awkward to\ntransition fsmonitor-watchman to work like everything else. So, some\nquestions, plus a proposal:\n\nQ1: Does fsmonitor-watchman make sense to provide more than once\n(since config-based hooks support multiple execs per hook)? (I get the\nfeeling the answer is no here - but what if, for example, I have an\nNTFS partition with some repos and an ext4 partition with some other\nrepos, and only want to configure fsmonitor magic once at global\nscope? I don't know anything about fsmonitor so maybe that is a very\nstupid question ;) )\nQ2: Does it make sense to specify anywhere other than per-repo scope?\n(system/global scope, e.g., applying to all repos? One could imagine\nproviding it in a config which is inherited by submodules of a\nsuperproject?) It looks like it operates on CWD + timestamp-ish, so it\nseems to me like some user might want to only set it up once (e.g.\nglobally) and have it Just Work in all their repos afterwards. In\nfact, I think you could configure core.fsmonitor in ~/.gitconfig today\nand it would work for everything.\n\nIf we want to support checking vs multiple fsmonitor-watchman execs\n(that is, if Q1's answer is \"yes\"), then I propose:\n1. Check for values on 'hook.fsmonitor-watchman.command' and add them\nto the execution list in config order.\n  1a. Here we could either check for\n'hookcmd.<command-value>.fsmonitor-watchman-version' or we could just\ncheck separately for 'hook.fsmonitor-watchmanv2.command' as implied by\n'githooks.txt' to determine the arg syntax for each hook.\n2. Check for values on 'core.fsmonitor' and\n'core.fsmonitorhookversion' to add a \"legacy\" hook at the end of the\nexecution list. (This is the same thing we do for hooks that live in\n.git/hooks/<hookname>.)\n\nIf we don't want to support adding multiple fsmonitor-watchman execs,\nthen I propose:\n1. Check hook.runHookDir's value to decide whether we believe in \"legacy\" hooks.\n2. If we do, then check for core.fsmonitor and\ncore.fsmonitorhookversion, and run that one.\n3. If we don't, or if core.fsmonitor was unset, then check for\n(hook.fsmonitor-watchman +\nhookcmd.<command-value>.fsmonitor-watchman-version) or\n(hook.fsmonitor-watchman.command or\nhook.fsmonitor-watchmanv2.command), error if we find more than one\ncandidate, and run the one candidate we do find. (Or I could see us\nrunning only the most-local one, but that might be confusing for the\nuser.)\n\nThanks, all - I'm curious to learn more about this hook and the right\nway to move forward :)\n\n - Emily\n"},{"id":"418899","messageId":"33a12a7a-d19c-63b8-f21e-db7e517b0f53@gmail.com","threadId":"55294","inReplyTo":"CAJoAoZk0fSyPSJZ51wNv4XujPA_-hPfh8eMRQPbyeTXFvKJ9+Q@mail.gmail.com","subject":"Re: What to do with fsmonitor-watchman hook and config-based hooks?","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-11T19:23:03Z","receivedAt":"2021-03-11T19:30:13Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/11/2021 1:42 PM, Emily Shaffer wrote:\n> Hi folks, I grabbed a bunch of CC from 'git blame fsmonitor.c' so\n> sorry if you don't care about fsmonitor-watchman anymore... :) Note\n> that this whole conversation has to do with the series proposed at\n> https://lore.kernel.org/git/20210311021037.3001235-1-emilyshaffer@google.com.\n> \n> When I was looking through the remaining hooks in\n> Documentation/githooks.txt I noticed that the fsmonitor-watchman hook\n> is implemented somewhat differently than most other hooks. As I\n> understand it, to use that hook someone needs to:\n> \n> 1. Configure core.fsmonitor with a path to some fsmonitor-watchman\n> hook. The documentation in 'Documentation/githooks.txt' claims that it\n> needs to point to '.git/hooks/fsmonitor-watchman' or\n> '.git/hooks/fsmonitor-watchmanv2', but I don't see that constraint\n> enforced when the config is read (config.c:git_config_get_fsmonitor()\n> and fsmonitor.c:query_fsmonitor()), so it seems that core.fsmonitor\n> can point to wherever it wants. (Plus\n> 'templates/blt/hooks/fsmonitor-watchman.sample' suggests setting\n> 'core.fsmonitor' = '.git/hooks/query-watchman'...)\n> 2. Configure core.fsmonitorhookversion to 1 or 2, to indicate the arg\n> structure for the executable specified in core.fsmonitor.\n\nThis is correct.\n\n> Because the executable doesn't necessarily live in .git/hooks/,\n> fsmonitor.c:query_fsmonitor() completely skips the \"API\" for running\n> hooks (run-command.h:run_hook_le()) and just uses\n> run-command.h:capture_command() directly.\n> \n> Interestingly, this is really similar to the way config-based hooks\n> (https://lore.kernel.org/git/20210311021037.3001235-1-emilyshaffer@google.com)\n> work - but different enough that I think it may be awkward to\n> transition fsmonitor-watchman to work like everything else. So, some\n> questions, plus a proposal:\n\nYou'll want to get Jeff Hostetler's perspective first, but I'm of\nthe opinion that we'll want to stop recommending the Watchman hook\nwhen the Git-native FS Monitor feature lands, with some time to\nlet things release and simmer before we remove the core.fsmonitor\nconfig option. We would also need a Linux implementation, but that\nis planned.\n\nIf we think that the plan of \"eventually, FS Monitor won't use hooks\"\nis reasonable, then how much do you want to spend time unifying it\nwith your config-based hooks? Can they live together temporarily?\n\n(Naturally, deprecating FS Monitor through the hook might not be\na reasonable plan.)\n\nThanks,\n-Stolee\n"},{"id":"418901","messageId":"YEpxVELGCxtnNxQK@google.com","threadId":"55294","inReplyTo":"33a12a7a-d19c-63b8-f21e-db7e517b0f53@gmail.com","subject":"Re: What to do with fsmonitor-watchman hook and config-based hooks?","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-03-11T19:36:52Z","receivedAt":"2021-03-11T19:37:50Z","isPatch":false,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Mar 11, 2021 at 02:23:03PM -0500, Derrick Stolee wrote:\n> \n> On 3/11/2021 1:42 PM, Emily Shaffer wrote:\n> > Hi folks, I grabbed a bunch of CC from 'git blame fsmonitor.c' so\n> > sorry if you don't care about fsmonitor-watchman anymore... :) Note\n> > that this whole conversation has to do with the series proposed at\n> > https://lore.kernel.org/git/20210311021037.3001235-1-emilyshaffer@google.com.\n> > \n> > When I was looking through the remaining hooks in\n> > Documentation/githooks.txt I noticed that the fsmonitor-watchman hook\n> > is implemented somewhat differently than most other hooks. As I\n> > understand it, to use that hook someone needs to:\n> > \n> > 1. Configure core.fsmonitor with a path to some fsmonitor-watchman\n> > hook. The documentation in 'Documentation/githooks.txt' claims that it\n> > needs to point to '.git/hooks/fsmonitor-watchman' or\n> > '.git/hooks/fsmonitor-watchmanv2', but I don't see that constraint\n> > enforced when the config is read (config.c:git_config_get_fsmonitor()\n> > and fsmonitor.c:query_fsmonitor()), so it seems that core.fsmonitor\n> > can point to wherever it wants. (Plus\n> > 'templates/blt/hooks/fsmonitor-watchman.sample' suggests setting\n> > 'core.fsmonitor' = '.git/hooks/query-watchman'...)\n> > 2. Configure core.fsmonitorhookversion to 1 or 2, to indicate the arg\n> > structure for the executable specified in core.fsmonitor.\n> \n> This is correct.\n> \n> > Because the executable doesn't necessarily live in .git/hooks/,\n> > fsmonitor.c:query_fsmonitor() completely skips the \"API\" for running\n> > hooks (run-command.h:run_hook_le()) and just uses\n> > run-command.h:capture_command() directly.\n> > \n> > Interestingly, this is really similar to the way config-based hooks\n> > (https://lore.kernel.org/git/20210311021037.3001235-1-emilyshaffer@google.com)\n> > work - but different enough that I think it may be awkward to\n> > transition fsmonitor-watchman to work like everything else. So, some\n> > questions, plus a proposal:\n> \n> You'll want to get Jeff Hostetler's perspective first, but I'm of\n> the opinion that we'll want to stop recommending the Watchman hook\n> when the Git-native FS Monitor feature lands, with some time to\n> let things release and simmer before we remove the core.fsmonitor\n> config option. We would also need a Linux implementation, but that\n> is planned.\n> \n> If we think that the plan of \"eventually, FS Monitor won't use hooks\"\n> is reasonable, then how much do you want to spend time unifying it\n> with your config-based hooks? Can they live together temporarily?\n\nOh, that's useful context. If fsmonitor-watchman hook is going away, I\ndon't think it's necessary to convert it at all, unless someone starts\nasking for multihooks or something. There's no practical conflict between\nconfig-based hooks and the current implementation - it's just a\nsurprising inconsistency. I'll be curious to hear Jeff's opinion, of\ncourse, but given what you're describing, I'm not convinced it's worth\nspending any time on - and when we're ready to stop checking\ncore.fsmonitor then the inconsistency will just go away.\n\nThe documentation in githooks.txt could use an update, though. :)\n\n - Emily\n"},{"id":"418976","messageId":"e019cc71-ac39-44a4-0a23-b3b7decec754@jeffhostetler.com","threadId":"55294","inReplyTo":"YEpxVELGCxtnNxQK@google.com","subject":"Re: What to do with fsmonitor-watchman hook and config-based hooks?","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-03-12T16:51:38Z","receivedAt":"2021-03-12T16:52:23Z","isPatch":false,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/11/21 2:36 PM, Emily Shaffer wrote:\n> On Thu, Mar 11, 2021 at 02:23:03PM -0500, Derrick Stolee wrote:\n>>\n>> On 3/11/2021 1:42 PM, Emily Shaffer wrote:\n>>> Hi folks, I grabbed a bunch of CC from 'git blame fsmonitor.c' so\n>>> sorry if you don't care about fsmonitor-watchman anymore... :) Note\n>>> that this whole conversation has to do with the series proposed at\n>>> https://lore.kernel.org/git/20210311021037.3001235-1-emilyshaffer@google.com.\n>>>\n>>> When I was looking through the remaining hooks in\n>>> Documentation/githooks.txt I noticed that the fsmonitor-watchman hook\n>>> is implemented somewhat differently than most other hooks. As I\n>>> understand it, to use that hook someone needs to:\n>>>\n>>> 1. Configure core.fsmonitor with a path to some fsmonitor-watchman\n>>> hook. The documentation in 'Documentation/githooks.txt' claims that it\n>>> needs to point to '.git/hooks/fsmonitor-watchman' or\n>>> '.git/hooks/fsmonitor-watchmanv2', but I don't see that constraint\n>>> enforced when the config is read (config.c:git_config_get_fsmonitor()\n>>> and fsmonitor.c:query_fsmonitor()), so it seems that core.fsmonitor\n>>> can point to wherever it wants. (Plus\n>>> 'templates/blt/hooks/fsmonitor-watchman.sample' suggests setting\n>>> 'core.fsmonitor' = '.git/hooks/query-watchman'...)\n>>> 2. Configure core.fsmonitorhookversion to 1 or 2, to indicate the arg\n>>> structure for the executable specified in core.fsmonitor.\n>>\n>> This is correct.\n>>\n>>> Because the executable doesn't necessarily live in .git/hooks/,\n>>> fsmonitor.c:query_fsmonitor() completely skips the \"API\" for running\n>>> hooks (run-command.h:run_hook_le()) and just uses\n>>> run-command.h:capture_command() directly.\n>>>\n>>> Interestingly, this is really similar to the way config-based hooks\n>>> (https://lore.kernel.org/git/20210311021037.3001235-1-emilyshaffer@google.com)\n>>> work - but different enough that I think it may be awkward to\n>>> transition fsmonitor-watchman to work like everything else. So, some\n>>> questions, plus a proposal:\n>>\n>> You'll want to get Jeff Hostetler's perspective first, but I'm of\n>> the opinion that we'll want to stop recommending the Watchman hook\n>> when the Git-native FS Monitor feature lands, with some time to\n>> let things release and simmer before we remove the core.fsmonitor\n>> config option. We would also need a Linux implementation, but that\n>> is planned.\n>>\n>> If we think that the plan of \"eventually, FS Monitor won't use hooks\"\n>> is reasonable, then how much do you want to spend time unifying it\n>> with your config-based hooks? Can they live together temporarily?\n> \n> Oh, that's useful context. If fsmonitor-watchman hook is going away, I\n> don't think it's necessary to convert it at all, unless someone starts\n> asking for multihooks or something. There's no practical conflict between\n> config-based hooks and the current implementation - it's just a\n> surprising inconsistency. I'll be curious to hear Jeff's opinion, of\n> course, but given what you're describing, I'm not convinced it's worth\n> spending any time on - and when we're ready to stop checking\n> core.fsmonitor then the inconsistency will just go away.\n> \n> The documentation in githooks.txt could use an update, though. :)\n> \n>   - Emily\n> \n\nI don't think it makes sense to have multiple fsmonitors for a given\nworking directory.  They are fairly expensive to operate (listening\nto the kernel for events and building an in-memory database of changed\nfiles) and I'm not sure how two running concurrently (and listening to\nthe same kernel event stream) would come up with different answers.\n\nThe thing about the fsmonitor-watchman or query-watchman hook is that\nit is a bash/perl script that talks to a long-running service daemon.\nThe hook script itself is just a proxy to decode the JSON response from\nWatchman and emit it on stdout in a way that the foreground Git command\nexpects.  Git cannot directly talk to Watchman because it doesn't\ncurrently have the plumbing to talk to it on anything other than a fd\npair that it sets up to give to the hook child.\n\nSo your example of a watcher for NTFS and a separate watcher for ext4\nmeans you could maybe have two services running, but the foreground\nGit command would only need to spawn a single hook and maybe it would\ndecide which service to talk to based upon the filesystem type of the\nworking directory.  Or you set the repo-local config for each repo to\npoint to a hook script that knew which service to talk to.  Either way\nyou only need to invoke one hook.\n\nSetting it globally is an option, but fsmonitor is beneficial for large\nrepos and working directories.  There is an overhead to having it\nrunning and talking to it.  (Spawning a hook written in PERL, having\nthe hook talk to Watchman via some form of IPC, the hook parsing the \nmess of JSON returned, pumping that data back over stdout to Git, and\nhaving Git apply the results to the ce_flags.)  All of that has to\nhappen *before* Git actually starts to do anything useful.  For small\nrepos, all of that overhead costs more than the cost of letting the\nforeground `git status` just lstat() everything.  Of course all of this\ndepends on the speed of your filesystem and the size of your working\ndirectory (and whether you have a sparse-checkout and etc), but there\nare lots of repos that just don't need fsmonitor.\n\nSo yes, I would leave the existing fsmonitor code as is and not try\nto combine it with your current efforts (even if I wasn't working on\na replacement for the fsmonitor-watchman setup).\n\nAs Stolee mentioned I'm currently working on a builtin fsmonitor\nfeature.  It will have a native coded-in-Git-C-code daemon to watch\nthe filesystem.  Clients, such as `git status`, will directly talk\nto it via my \"Simple IPC\" patch series and completely bypass all of\nthe hook stuff.\n\nLong term both fsmonitor solutions can co-exist.  Or we can eventually\ndeprecate the hook version.  Given that, I don't see a need to change\nthe existing fsmonitor hook code.\n\nHope this helps,\nJeff\n\n"},{"id":"418978","messageId":"87h7lgfchm.fsf@evledraar.gmail.com","threadId":"55294","inReplyTo":"e019cc71-ac39-44a4-0a23-b3b7decec754@jeffhostetler.com","subject":"Re: What to do with fsmonitor-watchman hook and config-based hooks?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-12T18:33:41Z","receivedAt":"2021-03-12T18:34:29Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Mar 12 2021, Jeff Hostetler wrote:\n\n> On 3/11/21 2:36 PM, Emily Shaffer wrote:\n>> On Thu, Mar 11, 2021 at 02:23:03PM -0500, Derrick Stolee wrote:\n>>>\n>>> On 3/11/2021 1:42 PM, Emily Shaffer wrote:\n>>>> Hi folks, I grabbed a bunch of CC from 'git blame fsmonitor.c' so\n>>>> sorry if you don't care about fsmonitor-watchman anymore... :) Note\n>>>> that this whole conversation has to do with the series proposed at\n>>>> https://lore.kernel.org/git/20210311021037.3001235-1-emilyshaffer@google.com.\n>>>>\n>>>> When I was looking through the remaining hooks in\n>>>> Documentation/githooks.txt I noticed that the fsmonitor-watchman hook\n>>>> is implemented somewhat differently than most other hooks. As I\n>>>> understand it, to use that hook someone needs to:\n>>>>\n>>>> 1. Configure core.fsmonitor with a path to some fsmonitor-watchman\n>>>> hook. The documentation in 'Documentation/githooks.txt' claims that it\n>>>> needs to point to '.git/hooks/fsmonitor-watchman' or\n>>>> '.git/hooks/fsmonitor-watchmanv2', but I don't see that constraint\n>>>> enforced when the config is read (config.c:git_config_get_fsmonitor()\n>>>> and fsmonitor.c:query_fsmonitor()), so it seems that core.fsmonitor\n>>>> can point to wherever it wants. (Plus\n>>>> 'templates/blt/hooks/fsmonitor-watchman.sample' suggests setting\n>>>> 'core.fsmonitor' = '.git/hooks/query-watchman'...)\n>>>> 2. Configure core.fsmonitorhookversion to 1 or 2, to indicate the arg\n>>>> structure for the executable specified in core.fsmonitor.\n>>>\n>>> This is correct.\n>>>\n>>>> Because the executable doesn't necessarily live in .git/hooks/,\n>>>> fsmonitor.c:query_fsmonitor() completely skips the \"API\" for running\n>>>> hooks (run-command.h:run_hook_le()) and just uses\n>>>> run-command.h:capture_command() directly.\n>>>>\n>>>> Interestingly, this is really similar to the way config-based hooks\n>>>> (https://lore.kernel.org/git/20210311021037.3001235-1-emilyshaffer@google.com)\n>>>> work - but different enough that I think it may be awkward to\n>>>> transition fsmonitor-watchman to work like everything else. So, some\n>>>> questions, plus a proposal:\n>>>\n>>> You'll want to get Jeff Hostetler's perspective first, but I'm of\n>>> the opinion that we'll want to stop recommending the Watchman hook\n>>> when the Git-native FS Monitor feature lands, with some time to\n>>> let things release and simmer before we remove the core.fsmonitor\n>>> config option. We would also need a Linux implementation, but that\n>>> is planned.\n>>>\n>>> If we think that the plan of \"eventually, FS Monitor won't use hooks\"\n>>> is reasonable, then how much do you want to spend time unifying it\n>>> with your config-based hooks? Can they live together temporarily?\n>> Oh, that's useful context. If fsmonitor-watchman hook is going away,\n>> I\n>> don't think it's necessary to convert it at all, unless someone starts\n>> asking for multihooks or something. There's no practical conflict between\n>> config-based hooks and the current implementation - it's just a\n>> surprising inconsistency. I'll be curious to hear Jeff's opinion, of\n>> course, but given what you're describing, I'm not convinced it's worth\n>> spending any time on - and when we're ready to stop checking\n>> core.fsmonitor then the inconsistency will just go away.\n>> The documentation in githooks.txt could use an update, though. :)\n>>   - Emily\n>> \n>\n> I don't think it makes sense to have multiple fsmonitors for a given\n> working directory.  They are fairly expensive to operate (listening\n> to the kernel for events and building an in-memory database of changed\n> files) and I'm not sure how two running concurrently (and listening to\n> the same kernel event stream) would come up with different answers.\n>\n> The thing about the fsmonitor-watchman or query-watchman hook is that\n> it is a bash/perl script that talks to a long-running service daemon.\n> The hook script itself is just a proxy to decode the JSON response from\n> Watchman and emit it on stdout in a way that the foreground Git command\n> expects.  Git cannot directly talk to Watchman because it doesn't\n> currently have the plumbing to talk to it on anything other than a fd\n> pair that it sets up to give to the hook child.\n>\n> So your example of a watcher for NTFS and a separate watcher for ext4\n> means you could maybe have two services running, but the foreground\n> Git command would only need to spawn a single hook and maybe it would\n> decide which service to talk to based upon the filesystem type of the\n> working directory.  Or you set the repo-local config for each repo to\n> point to a hook script that knew which service to talk to.  Either way\n> you only need to invoke one hook.\n>\n> Setting it globally is an option, but fsmonitor is beneficial for large\n> repos and working directories.  There is an overhead to having it\n> running and talking to it.  (Spawning a hook written in PERL, having\n> the hook talk to Watchman via some form of IPC, the hook parsing the\n> mess of JSON returned, pumping that data back over stdout to Git, and\n> having Git apply the results to the ce_flags.)  All of that has to\n> happen *before* Git actually starts to do anything useful.  For small\n> repos, all of that overhead costs more than the cost of letting the\n> foreground `git status` just lstat() everything.  Of course all of this\n> depends on the speed of your filesystem and the size of your working\n> directory (and whether you have a sparse-checkout and etc), but there\n> are lots of repos that just don't need fsmonitor.\n>\n> So yes, I would leave the existing fsmonitor code as is and not try\n> to combine it with your current efforts (even if I wasn't working on\n> a replacement for the fsmonitor-watchman setup).\n>\n> As Stolee mentioned I'm currently working on a builtin fsmonitor\n> feature.  It will have a native coded-in-Git-C-code daemon to watch\n> the filesystem.  Clients, such as `git status`, will directly talk\n> to it via my \"Simple IPC\" patch series and completely bypass all of\n> the hook stuff.\n>\n> Long term both fsmonitor solutions can co-exist.  Or we can eventually\n> deprecate the hook version.  Given that, I don't see a need to change\n> the existing fsmonitor hook code.\n\nI'm all for built-in watchman, but this claim about Perl etc. being the\nslow part and the hook parsing etc. just doesn't add up with performance\ntesting I've done on it.\n\nSee numbers at:\nhttps://lore.kernel.org/git/CACBZZX6D8oC34qat7kdrDOWC5eYm-DRkMWG9eOPPvKKsQtgPyw@mail.gmail.com/#t\n\nI don't have this re-paged into my memory, but I remember the invoking\nof watchman etc. being trivially cheap, the cost was taking the\ninformation from the hook, updating git's view of the world (index\netc.), and spewing out info for the user.\n\nIt was something like \"git status\" taking 500ms, invoking the watchman\nbinary (talking to the daemon) would take 2-5ms, the whole Perl\netc. parsing another mfew ms, and the rest of \"git status\" 10x that time\nat least...\n\n"},{"id":"418998","messageId":"YEvP/ObHqhaLaSG1@google.com","threadId":"55294","inReplyTo":"e019cc71-ac39-44a4-0a23-b3b7decec754@jeffhostetler.com","subject":"Re: What to do with fsmonitor-watchman hook and config-based hooks?","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-03-12T20:33:00Z","receivedAt":"2021-03-12T20:34:04Z","isPatch":false,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Fri, Mar 12, 2021 at 11:51:38AM -0500, Jeff Hostetler wrote:\n> I don't think it makes sense to have multiple fsmonitors for a given\n> working directory.  They are fairly expensive to operate (listening\n> to the kernel for events and building an in-memory database of changed\n> files) and I'm not sure how two running concurrently (and listening to\n> the same kernel event stream) would come up with different answers.\n> \n> The thing about the fsmonitor-watchman or query-watchman hook is that\n> it is a bash/perl script that talks to a long-running service daemon.\n> The hook script itself is just a proxy to decode the JSON response from\n> Watchman and emit it on stdout in a way that the foreground Git command\n> expects.  Git cannot directly talk to Watchman because it doesn't\n> currently have the plumbing to talk to it on anything other than a fd\n> pair that it sets up to give to the hook child.\n> \n> So your example of a watcher for NTFS and a separate watcher for ext4\n> means you could maybe have two services running, but the foreground\n> Git command would only need to spawn a single hook and maybe it would\n> decide which service to talk to based upon the filesystem type of the\n> working directory.  Or you set the repo-local config for each repo to\n> point to a hook script that knew which service to talk to.  Either way\n> you only need to invoke one hook.\n\nThanks, that makes a lot of sense!\n\n> Setting it globally is an option, but fsmonitor is beneficial for large\n> repos and working directories.  There is an overhead to having it\n> running and talking to it.  (Spawning a hook written in PERL, having\n> the hook talk to Watchman via some form of IPC, the hook parsing the mess of\n> JSON returned, pumping that data back over stdout to Git, and\n> having Git apply the results to the ce_flags.)  All of that has to\n> happen *before* Git actually starts to do anything useful.  For small\n> repos, all of that overhead costs more than the cost of letting the\n> foreground `git status` just lstat() everything.  Of course all of this\n> depends on the speed of your filesystem and the size of your working\n> directory (and whether you have a sparse-checkout and etc), but there\n> are lots of repos that just don't need fsmonitor.\n> \n> So yes, I would leave the existing fsmonitor code as is and not try\n> to combine it with your current efforts (even if I wasn't working on\n> a replacement for the fsmonitor-watchman setup).\n> \n> As Stolee mentioned I'm currently working on a builtin fsmonitor\n> feature.  It will have a native coded-in-Git-C-code daemon to watch\n> the filesystem.  Clients, such as `git status`, will directly talk\n> to it via my \"Simple IPC\" patch series and completely bypass all of\n> the hook stuff.\n> \n> Long term both fsmonitor solutions can co-exist.  Or we can eventually\n> deprecate the hook version.  Given that, I don't see a need to change\n> the existing fsmonitor hook code.\n\nYeah, that seems like the direction everyone agrees with. Thanks very\nmuch for the detailed explanation, that helped me feel sure that doing\nnothing is the right approach (how convenient...) :)\n\nI think, then, all that's needed is a patch to the githooks.txt doc 1)\nremoving the incorrect info about core.fsmonitor's contents needing to\npoint to a specific name in .git/hooks/ and 2) explaining that because\nit uses this special config, it doesn't use the usual hook.*.command\napproach.\n\n - Emily\n"}]}