Apply exclude filter when handling fs events.

[busykai]

This resolves the issue with ProjectModel not applying exclude list when handling FileSystem change.

This PR fixes part 2 of the issues described in #10183.

EDIT:

This PR fixes #10183, see this comment for the issue definition:

• 5d181f5 fixes the part 2 -- do not allow the fs events to change the project model, apply filtering
• 6514f20 fixes the part 1 -- do not change FileSystem._index when the requested entry is filtered.

It might be so that 6514f20 actually draws 5d181f5 useless since no more events will be generated and the filtering will be done at FileSystem level, but there's no penalty on having 5d181f5 in place as well.

[busykai]

@dangoor, @redmunds, @peterflynn I believe this fixes an important omission in the current FileSystem vs ProjectModel filtering mechanism. brackets-userland/brackets-git#436 demonstrates it nicely. there was no other way around it, but to use underlying brackets.fs to avoid side-effects of FileSystem.resolve() on ProjectModel and FileSystem._index.

[dangoor]

@busykai This is an important thing to fix ultimately. I know @peterflynn has thought about fixing this problem generally (see #8816) because there are a bunch of details to ultimately have file filtering handled in a consistent manner.

The change you've got here makes sense to me, but I'd prefer someone who knows the filesystem better to decide about that half of the change.

Also, I'd rather not see ProjectModel changes that don't include a test change.

[busykai]

@dangoor, @peterflynn, I don't see any work done on the issue #8816 so far, so we can build what's needed on top of this PR or use this as a temporary workaround in case you were thinking of something radical, such as shared preferences-based excludes for both FileSystem and ProjectModel -- they are shared now quite awkwardly. So if you tell me the direction of this is right, at least as a temporary solution to quite disturbing problems (e.g. with Brackets-git installed the editor gets swamped with fs events on any external git command), I can finish this up (mainly tests are missing as @dangoor noted).

[ingorichter]

@busykai I agree, that we should start with this and iterate on it. If this is an improvement already, we should get it in. Since the big story is around for such a long time, I assume that it won't be addressed any time soon either. We can update the story and mention this fix specifically to keep track of already applied fixes.
I think that @peterflynn should have a final look at this change. We should make a fast decision on this.

[busykai]

@ingorichter, good! I will add tests for ProjectModel meanwhile as per @dangoor's suggestion.

[peterflynn]

So my concern with this is that FileSystem is supposed to guarantee that there are never two File/Directory entries representing the same path. Without putting entries in the index, that's impossible.

There aren't too many places in Brackets where we actually rely on that assumption, and I'm not sure if things inside the .git folder could ever run up against any of those cases anyway... but as a generic fix this makes me a little uncomfortable.

I wonder if a cleaner approach would be this model:

• Any entry you can ever create will be in the index, even if the watchroot filter would exclude it
• The watchroot filter takes on a more precise meaning:
  • Filtered entries are excluded from Directory.getContents()
  • Filtered entries are never cached
  • No change events are dispatched from FileSystem for any filtered entries

This would still fix the project tree bug, without violating the 'singleton' entry guarantee, and still allows using these FS APIs to read/write filtered files. I think this is also pretty easy to implement:

• Directory.getContents() already filters its results
• Filtered entries already return false from _isWatched(), ensuring no caching
• I think events can be excluded by just changing FileSystem._handleExternalChange() at line 801 to check if (entry && entry._isWatched()) {.

Note that there's still some performance overhead from .git churn because the Node native watcher sends all the events over to FileSystem before they get ignored in _handleExternalChange(), but that's true even in vanilla Brackets with no extensions touching the .git folder -- we've never done any filtering on the Node side so far. And _isWatched() caches its result, so the new check itself shouldn't add much overhead at all.

@zaggino @dangoor @ingorichter - Thoughts?

[busykai]

@peterflynn Nice! I'm not sure if you were asking my opinion, but I think that this is a nice way to keep _index consistent and maintain uniqueness of the FileSystemEntry instances.

[dangoor]

@peterflynn That seems reasonable to me

[busykai]

@peterflynn, so I reverted these changes and created an integration test.

your suggestion works when the changes are external. it seemingly fixes the problem with running git as an external process -- _handleExternalChange ignores fs events because either 1) changes coming for an entry that is not watched or 2) changes are coming for the entries that do not exist in the _index. however, there's one case when if Brackets itself writes to a file under the entry which is not watched (in my test case .git/test), then the entry does exist and it is watched. which, in turn, results in the same effect -- everything appears in the ProjectModel.

the root cause of this is that we filter by the file's base name without checking if the parent is not watched, that is we match /^\.git$/, but any file underneath .git wouldn't be matched (e.g. .git/test).

while one could argue whether this a bug or a feature, I suggest that we fix this by "inheriting" _watchedRootFilterResult from parent's entry _isWatched(). this will ensure that if an entry is filtered out, all its children would be filtered out as well.

what you think?

[ingorichter]

@peterflynn that sounds like a good solution.

[peterflynn]

@busykai It sounds like you've identified two separate problems:

1) _isWatched() doesn't produce the correct result for entries nested within a filtered folder.

• We either have to change the watchroot filter API to permit checking the full path, or recursively check all parent entries. Both entail some performance overhead (more complex string checking; or possibly needing to construct and immediately filter-test any missing Directory entries going up the chain between the original node and the watch root). Would either of those amount to overhead significant enough to care about?

2) File I/O done via Brackets APIs will dispatch FileSystem events for that item even if it's unwatched or filtered out.
(Note: these events don't come from file watchers; FS automatically dispatches the events since it knows a change did happen. See e.g. the _fireChangeEvent() call in File.write(). This bypasses _handleExternalChange(), so the extra checks we've added there don't help).

• It's not clear to me whether this is a bad thing. For unwatched nodes that aren't filtered out, Brackets may rely on this behavior -- i.e. expecting to always reliably receive events for its own changes -- pretty sure that's true for renames at least. So we could disable these 'forced events' only for filtered nodes, but not for other nodes where _isWatched() returns false. Or we could simply reintroduce that filter double-check in the tree code, accepting that we may get some events for files that are within the tree's scope but not supposed to be shown. The former sounds nicer to me, if there's a clean way to do it.

[peterflynn]

For issue 2, I've realized the solution I like (no 'forced events' for filtered entries) introduces a slight pitfall. If parts of Brackets expect to always reliably get change events for its own changes, that expectation is violated if the user (or an extension) manually hands that part of Brackets a filtered-out file. For the rename case this isn't currently possible, but that's only because you can only rename things that appear in the tree; in the future, we might allow renaming directly from the working set, and you can get filtered-out items into the working set by manually picking with File > Open.

One option is just to make sure no parts of Brackets have this assumption (if rename is the only case, may be easy to fix). Another option is to go with the other fix suggestion instead: just add back in the filtering double-check in the project tree code. Slight perf hit but probably negligible.

[busykai]

@peterflynn, I've done quite some experimenting over the weekend and I think that for the issue 1) we need to go with "inheriting" the parent _isWatched() if there's a watched root for the entry. I will clean up my change and push later today.

for the issue 2) it seems like if we don't cause _fireChangeEvent() on parent entries that are not watched in any of the "shortcuts" that Brackets File I/O API is taking (e.g. File.write()), then it would not disturb the ProjectModel. Note that these entries could only be directories.

[busykai]

@peterflynn, this call is actually not rejected, so the test is wrong. do you have a suggestion on how to find out if the entry is in the tree? i'm thinking inspecting actual DOM, but may there's a better way.

[busykai]

@peterflynn, this is ready for review.

[busykai]

@peterflynn, it'd be great if you could find some time to review this so we could finish it up for 1.3.

[busykai]

FWIW, Intel XDK has been using this improvement (in as-is form) for 5 months now without any issues.

[zaggino]

Yep, this is taking far too long...

[abose]

@zaggino i do not see the .git folder in my project tree. was the git extension updated to compensate for this?

[zaggino]

@abose yes, try removing this code from your git extension: https://github.com/zaggino/brackets-git/blob/master/src/git/GitCli.js#L961-L978

[abose]

I have also used the file system change events for the instant search pr- just to update the search results on project files change. #11375

Would this be safe to pull in this release. now that we know this has been in stable use for over 5 months?

[busykai]

@abose: I believe it's pretty safe to merge. We didn't have any troubles having this change in Intel XDK and we rely on file watchers heavily. it would be nice, though, if @peterflynn would weigh in since he had some comments earlier.

[abose]

Thnaks @busykai .
Will merge it once the new instant search branch is merged and all the unit test due to instant search are fixed. I'll ping peter abut this PR.

[busykai]

Thank you, @abose!