[CLOSED] Apply exclude filter when handling fs events.

[core-ai-bot]

Issue by busykai
Saturday Jan 03, 2015 at 23:48 GMT
Originally opened as adobe/brackets#10304

---

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 included the following code: https://github.com/adobe/brackets/pull/10304/commits

[core-ai-bot]

Comment by busykai
Sunday Jan 04, 2015 at 01:45 GMT

---

@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.

[core-ai-bot]

Comment by dangoor
Tuesday Jan 06, 2015 at 14:36 GMT

---

@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.

[core-ai-bot]

Comment by busykai
Tuesday Jan 06, 2015 at 14:50 GMT

---

@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).

[core-ai-bot]

Comment by ingorichter
Thursday Jan 08, 2015 at 19:18 GMT

---

@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.

[core-ai-bot]

Comment by busykai
Thursday Jan 08, 2015 at 19:30 GMT

---

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

[core-ai-bot]

Comment by peterflynn
Thursday Jan 08, 2015 at 23:15 GMT

---

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?

[core-ai-bot]

Comment by busykai
Friday Jan 09, 2015 at 01:43 GMT

---

@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.

[core-ai-bot]

Comment by dangoor
Friday Jan 09, 2015 at 02:04 GMT

---

@peterflynn That seems reasonable to me

[core-ai-bot]

Comment by busykai
Friday Jan 09, 2015 at 22:06 GMT

---

@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?

[core-ai-bot]

Comment by ingorichter
Friday Jan 09, 2015 at 23:33 GMT

---

@peterflynn that sounds like a good solution.

[core-ai-bot]

Comment by peterflynn
Saturday Jan 10, 2015 at 02:37 GMT

---

@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.

[core-ai-bot]

Comment by peterflynn
Saturday Jan 10, 2015 at 02:44 GMT

---

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.

[core-ai-bot]

Comment by busykai
Monday Jan 12, 2015 at 17:12 GMT

---

@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.

[core-ai-bot]

Comment by busykai
Wednesday Jan 14, 2015 at 06:12 GMT

---

@peterflynn, this is ready for review.

[core-ai-bot]

Comment by busykai
Friday Apr 03, 2015 at 17:01 GMT

---

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

[core-ai-bot]

Comment by busykai
Thursday May 28, 2015 at 03:37 GMT

---

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

[core-ai-bot]

Comment by zaggino
Thursday May 28, 2015 at 09:36 GMT

---

Yep, this is taking far too long...

[core-ai-bot]

Comment by abose
Sunday Jul 12, 2015 at 16:31 GMT

---

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

[core-ai-bot]

Comment by zaggino
Sunday Jul 12, 2015 at 21:27 GMT

---

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

[core-ai-bot]

Comment by abose
Monday Jul 13, 2015 at 10:39 GMT

---

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?

[core-ai-bot]

Comment by busykai
Wednesday Jul 15, 2015 at 18:05 GMT

---

@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.

[core-ai-bot]

Comment by abose
Wednesday Jul 15, 2015 at 18:39 GMT

---

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.

[core-ai-bot]

Comment by busykai
Saturday Jul 25, 2015 at 13:33 GMT

---

Thank you,@abose!