wincent / command-t

⌨️ Fast file navigation for Neovim and Vim
BSD 2-Clause "Simplified" License
2.74k stars 317 forks source link

use `watch-project` rather than deprecated `watch` #389

Closed mr-salty closed 2 years ago

mr-salty commented 2 years ago

watch is deprecated per the watchman docs: https://facebook.github.io/watchman/docs/cmd/watch.html it should be replaced with watch-project: https://facebook.github.io/watchman/docs/cmd/watch-project.html

The latter has the big advantage of not creating redundant watches for subdirectories. When I looked at watchman watch-list on my machine I had ~20 watches, all of which were subdirectories of my home directory. The daemon was using ~1.6GB of memory (I have about 1.5M files in my homedir).

I did watchman watch-del-all, killed the daemon, then did watchman watch-project $HOME, the current memory usage is 571MB.

The downside is you have to change how you do queries, you have to supply the "real" root returned from watch-project and put the subdirectory in the expression. It seems to me that watchman could easily split the path itself to allow existing queries to continue to work.

So, for instance:

$ watchman watch-project $HOME
{
    "version": "2022.06.13.00",
    "watch": "/Users/todd",
    "watcher": "fsevents"
}

$ watchman watch-project $HOME/subdir
{
    "version": "2022.06.13.00",
    "relative_path": "subdir",
    "watch": "/Users/todd",
    "watcher": "fsevents"
}

then, instead of using this query: ["query", "/Users/todd/subdir", { "expression": ["type", "f"], "fields": ["name"]}] we have to do ["query", "/Users/todd", { "expression": ["allof", ["dirname", "subdir"], ["type", "f"]], "fields": ["name"]}] and, annoyingly, strip subdir/ from the results.

mr-salty commented 2 years ago

Here's a proof of concept I did, it's hacky but it works - I can clean it up and submit it as a PR: Do we have to worry about supporting old versions of watchman that don't support watch-project?

diff --git a/ruby/command-t/lib/command-t/scanner/file_scanner/watchman_file_scanner.rb b/ruby/command-t/lib/command-t/scanner/file_scanner/watchman_file_scanner.rb
index 60f412a..7bddbf1 100644
--- a/ruby/command-t/lib/command-t/scanner/file_scanner/watchman_file_scanner.rb
+++ b/ruby/command-t/lib/command-t/scanner/file_scanner/watchman_file_scanner.rb
@@ -25,24 +25,27 @@ module CommandT

           UNIXSocket.open(sockname) do |socket|
             root = Pathname.new(@path).realpath.to_s
-            roots = Watchman::Utils.query(['watch-list'], socket)['roots']
-            if !roots.include?(root)
+#            roots = Watchman::Utils.query(['watch-list'], socket)['roots']
+
+            if true # !roots.include?(root)
               # this path isn't being watched yet; try to set up watch
-              result = Watchman::Utils.query(['watch', root], socket)
+              result = Watchman::Utils.query(['watch-project', root], socket)

               # root_restrict_files setting may prevent Watchman from working
               # or enforce_root_files/root_files (>= version 3.1)
-              extract_value(result)
+              watch_root = extract_value(result, 'watch')
+              relative_path = extract_value(result, 'relative_path') if result['relative_path']
             end

-            query = ['query', root, {
-              'expression' => ['type', 'f'],
+            query = ['query', watch_root, {
+              'expression' => relative_path ? ['allof', ['type', 'f'], ['dirname', relative_path]] : ['type', 'f'],
               'fields'     => ['name'],
             }]
             paths = Watchman::Utils.query(query, socket)

             # could return error if watch is removed
             extracted = extract_value(paths, 'files')
+            extracted.each { |p| p.delete_prefix!("#{relative_path}/") } if relative_path
             if @wildignore
               extracted.select { |path| path !~ @wildignore }
             else
wincent commented 2 years ago

Do we have to worry about supporting old versions of watchman that don't support watch-project?

We probably should, so as not to break people's set-ups... One hacky way to do it that springs to mind is to scan the output of watchman --help to see if watch-project is a substring. Even though it's a hack, it's probably better (less annoying to users) than providing a configuration option.

The other question that comes to mind is whether there is any performance impact from the delete_prefix! calls... if it's negligible, great; if it's not, then continuing to use the deprecated command may be desirable for some users (ie. those who want to trade memory for speed).

mr-salty commented 2 years ago

yeah, I was worried about all the extra string data, plus the cost of stripping it, but I found a better option that takes care of that inside watchman - relative_root: https://facebook.github.io/watchman/docs/file-query.html#relative-roots . I updated my script and confirmed the paths you get back are relative to relative_root so no prefix stripping is needed.

one issue is that says it was introduced in 3.3 whereas watch-project was 3.1 so checking for watch-project won't work but maybe we can check watchman --version. FWIW, the oldest release even shown on https://facebook.github.io/watchman/docs/release-notes.html is 3.3.0 and that's from 2015-06-22 (so i'll have to try to find an old version and confirm --version even exists).