Fix recursive path handling for workspace/didChangeWatchedFiles

[dradtke]

Problem

LanguageClient-neovim currently fails to register filesystem notifications for paths resembling those requested by eclipse.jdt.ls:

"watchers": [
  {"globPattern":"**/*.java"},
  {"globPattern":"**/pom.xml"},
  {"globPattern":"**/*.gradle"},
  {"globPattern":"**/gradle.properties"},
  {"globPattern":"**/.project"},
  {"globPattern":"**/.classpath"},
  {"globPattern":"**/.settings/*.prefs"},
  {"globPattern":"**/src/**"}
]

Because none of the requested paths end with **, the plugin attempts to register them all as non-recursive file watches, which fails because no file named e.g. **/*.java exists.

Solution

This PR proposes a possible fix, but it's a very blunt one, and I'm open to feedback on how to make it more targeted. Specifically, if a glob pattern contains ** anywhere, it chops off it and everything after it (defaulting to . if it occurs at the beginning of the string, as seen above) and requests a recursive watch; otherwise, it adds a non-recursive file watch.

As is, the 8 watchers requested above all end up adding a recursive . watch, which is inefficient. A better solution would be to register recursive watchers on . that can still filter events by the rest of the path following **/, but I don't know where that would be implemented.

[martskins]

I think a better approach would be to use glob, which is in fact part of the dependencies already. With something like this:

    for entry in glob::glob_with(fw.glob_pattern.as_str(), MatchOptions::default()).unwrap() {
        // set watcher for entry.unwrap().display()
    }

glob_with will return a list of paths that match those patterns, and I think it should be able to handle all of the paths in your use case.

This will avoid adding watchers for the same file more than once and also adding watchers for files that the server is not interested (as I understand it, your current implementation would for example watch any extension, not only java, when given the pattern **/*.java).

Just a side note, keep in mind that there's #1054 too, which doesn't address this issue in particular but it also involves the watcher logic.

[dradtke]

Thanks for the tip, that definitely looks like a better solution. I just pushed an update that uses glob::glob() (the default options seem fine), so hopefully that results in better performance.

[martskins]

Clippy will complain about this, can we change it to

let mode = if path.is_dir() {
    notify::RecursiveMode::Recursive
} else {
    notify::RecursiveMode::NonRecursive
};

instead?

[dradtke]

Just pushed an update to change it to use if/else.

[martskins]

Oh and also, PRs should target the dev branch now, so please change the target of this PR to that branch.

[martskins]

@dradtke note that you'll need to rebase this to dev, as it has some merge conflicts now (the code base has drastically changed in the last few days 😄 ). Happy to lend a hand if you need it though.

[dradtke]

Just finished rebasing everything on dev and the diff looks pretty clean. 🧼

[dradtke]

@martskins Any other feedback? It'd be nice to have this PR merged in.

[martskins]

Looks good to me, but it'd be nice for @autozimu to take a look, as I'm not sure I've used this feature enough to test it.

[dradtke]

@autozimu Any thoughts on getting this PR merged in?

[autozimu]

Thanks for the contribution!