Watch subdirectories

[dpsutton]

Create a defcustom lsp-watch-folders.

At work our project has thousands of directories and many directories at the top level that are not interesting as watch roots. There is a way to ignore files and these are checked at every level. It would be far easier to create an allow list. However, this allow-list should be checked only at the top level rather than everywhere like the ignore list.

This PR creates this functionality. It can be easily set in dir-locals like:

((nil
  (lsp-watch-folders . ("src" "shared-src" "test" "dashboard/src"))
  ))

I added some tests but I've found a few relating to the file watchers a) don't run in CI due to the presence of the :tags '(no-win) metadata and b) when i specify to run them explicitly, they fail.

# on my branch
cask exec ert-runner test/lsp-file-watch-test.el

Ran 11 tests in 2.088 seconds
5 unexpected results:
   FAILED  lsp-file-notifications-sub-folders-test # new test mimicing the existing
   FAILED  lsp-file-notifications-test
   FAILED  lsp-file-watch--adding-watches
   FAILED  lsp-file-watch--recursive
   FAILED  lsp-file-watch--relative-path-glob-patterns

and on master:

4 unexpected results:
   FAILED  lsp-file-notifications-test
   FAILED  lsp-file-watch--adding-watches
   FAILED  lsp-file-watch--recursive
   FAILED  lsp-file-watch--relative-path-glob-patterns

[dpsutton]

sorry about the git noise. I was accidentally on an old version of master. should be updated and now in a single clean commit.

[yyoncho]

Thank you for contributing.

I was thinking about the implementation - can we have a list of regexes which are used to override the current list of all folders. E. g. you can set ignored filters to .* (ignore all) and then set the force watch folders variable to ("foo" "bar")?

[yyoncho]

can we have a regexp as well?

[yyoncho]

we are not allowed to do that.

[dpsutton]

@kiennq had the same comment inline but i think the UI will obscure it some so i'll paste it here as well:

I'm not sure why its needed but I'll explain the problem it seems to solve. When I would start up emacs fresh and enter a project, the var lsp-watch-folders wasn't picked up. I would then lsp-restart-workspace and it was seen. I did that as a kludge to get it to pick them up but not sure why they weren't in the first place. I'll try again though.

[kiennq]

Shouldn't this to use lsp-f-same??
I don't think we need to follow symlink when compare the workspace-root here

[dpsutton]

didn't know about that one. looks good to me.

[kiennq]

Why this is needed?

[dpsutton]

I'm not sure why its needed but I'll explain the problem it seems to solve. When I would start up emacs fresh and enter a project, the var lsp-watch-folders wasn't picked up. I would then lsp-restart-workspace and it was seen. I did that as a kludge to get it to pick them up but not sure why they weren't in the first place. I'll try again though.

[dpsutton]

I was thinking about the implementation - can we have a list of regexes which are used to override the current list of all folders. E. g. you can set ignored filters to .* (ignore all) and then set the force watch folders variable to ("foo" "bar")?

I thought about doing regexes similar to the ignore list and i chose this path for two reasons:

1. i was worried about the interaction of ignore and allow lists. with the current version they interact nicely.
2. The ignore list is checked on each file in the tree of watches and this wasn't necessary for the allow-list type behavior. It seemed unnecessary and error prone to allow to directories at arbitrary depths when i really want to just say watch the src/ and test/ directories and not the vendored files, bin, data, resources, etc.

I'm not married to this implementation by any means so if the regex version sounds better to you as a maintainer this can be adapted or scrapped as you see fit. My usage would end up being "src|test" anyways. It feels a bit wrong to investigate at arbitrary depth which folders match this grant regex when i just want to set up watches of root source directories rather than the whole project.

As an aside, it is quite nice to have both of your eyes on the changes so quickly and makes it quite pleasant to discuss and work on this great project. So thanks much for that!

[dpsutton]

I don't think anyone is happy with the (hack-local-variables) but the dir-local value isn't picked up on the first start of the lsp client. I followed a similar defcustom to add the autoload to mark it as safe but i don't know how to generate the autoloads for this. I wonder if that would help.

update:

i ran (package-generate-autoloads "lsp-mode" "/Users/dan/projects/elisp/lsp-mode") and it didn't help. I see i have them accidentally marked as booleanp rather than whatever predicate accepts a list of strings so i'll need to fix that. But not sure why its not there on startup. Perhaps the current buffer at the time isn't the project file that has that variable set?

[yyoncho]

2. It seemed unnecessary and error prone to allow to directories at arbitrary depths when i really want to just say watch the src/ and test/ directories and not the vendored files, bin, data, resources, etc.

In multi-module project you would like to watch subproject1/src, subproject2/src/ etc. So the need to explicitly mention each subproject path will be limiting for this use-case.

i was worried about the interaction of ignore and allow lists. with the current version they interact nicely.

Can you elaborate? It seems to me that all will narrow down to

(or (not (lsp--string-match-any <PATH> ignore-list))
    (lsp--string-match-any <PATH> allow-list))

[yyoncho]

I don't think anyone is happy with the (hack-local-variables) but the dir-local value isn't picked up on the first start of the lsp client. I followed a similar defcustom to add the autoload to mark it as safe but i don't know how to generate the autoloads for this. I wonder if that would help.

At the beginning of the project, we decided not to call hack-.. in favour of recommending users to use 'hack-local-variables-hook . I guess this decision was favouring purism over being practical.

https://emacs-lsp.github.io/lsp-mode/page/faq/

[dpsutton]

In multi-module project you would like to watch subproject1/src, subproject2/src/ etc. So the need to explicitly mention each subproject path will be limiting for this use-case.

I never had a good mental model of what multi module projects looked like so i hadn't considered this. The code as it is now considers a project to have a single root and passes the subdirectories along when the root is found but not for other project roots because i didn't really know what they were.

(or (not (lsp--string-match-any <PATH> ignore-list))
    (lsp--string-match-any <PATH> allow-list))

Doing this brings up strange semantics in my opinion. If a file matches both the ignore and allow regexes, what should happen? If a file matches neither the ignore or allow regexes, what should happen. Its clear from the code that it will be watching in both cases but just reading documentation or the var names might have very different expectations. I like the current implementation because its more clear: rather than watching the root directory, watch these sub directories, with the same ability to use the ignore regex as it currently exists.

Having both an ignore and allow regex that don't necessarily partition the set of all files turns the allow regex into an ignore-override regex which is strange. Otherwise the expectation of what to do with files that are neither ignored nor allowed seems a bit strange.

Again, not trying to be obstinate, and love the conversation. Thank you both again.

[dpsutton]

At the beginning of the project, we decided not to call hack-.. in favour of recommending users to use 'hack-local-variables-hook . I guess this decision was favouring purism over being practical.

Does this signal that you are willing to rethink the the current decision not to use hack-* or that indeed I need to figure out a way to hook on my side?

[yyoncho]

At the beginning of the project, we decided not to call hack-.. in favour of recommending users to use 'hack-local-variables-hook . I guess this decision was favouring purism over being practical.

Does this signal that you are willing to rethink the the current decision not to use hack-* or that

Spacemacs has a solution for that and it is all good. So, we might investigate how to make all the things easier for the users. The issue here is that the dir locals are not available in major-mode hook.

indeed I need to figure out a way to hook on my side?

There is an entry in lsp-mode FAQ explaining what you can do.

So, I reread your comments and it seems like you are trying to achieve not exactly what I was thinking in the first place. What you are trying to do is to override the project root, right? If yes, I will spend some time checking the code and see what we can do. Your solution might be working but it looks a bit more complex than it should (at least to me).

[dpsutton]

There is an entry in lsp-mode FAQ explaining what you can do.

yup. i'll remove the (hack-local-variables) call.

So, I reread your comments and it seems like you are trying to achieve not exactly what I was thinking in the first place.

I'm sorry about that. I should have been more clear with my problem statement. My aim is to provide the source directories i want watched so that i'm not registering 4400 watches in our project. With my change, there are 140 or so watches for source files. Our work project is a cljs project so we have lots of compilation output in development mode all of which are unnecessarily watched. We have migrations, static resources, a vendored js project with 122 subfolders for locales, etc. And the only thing i'm interested in are watching the folders containing the clj and cljs files.

(length
 (ht-keys
  (lsp-watch-descriptors (ht-get (lsp-session-watches)
                                 "/Users/dan/projects/clojure/acl"))))
;; 106 vs 473 and a warning about watching 4400 files without the subdirectories set.