Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Evaluate removing the invalidation of parent directories when files change #11707

Closed
stuhood opened this issue Mar 16, 2021 · 1 comment · Fixed by #13566
Closed

Evaluate removing the invalidation of parent directories when files change #11707

stuhood opened this issue Mar 16, 2021 · 1 comment · Fixed by #13566

Comments

@stuhood
Copy link
Member

stuhood commented Mar 16, 2021

Since using watchman (which was deprecated in favor of notify in #9714), we have had a special case in place to invalidate the parent directory of a changed file, because in some cases we would not be notified about parent directory changes when a file was created or removed.

pants/src/rust/engine/watch/src/lib.rs

Lines 191 to 196 in 5d9d725

let mut paths_to_invalidate: Vec<PathBuf> = vec![];
if let Some(parent_dir) = path_relative_to_build_root.parent() {
paths_to_invalidate.push(parent_dir.to_path_buf());
}
paths_to_invalidate.push(path_relative_to_build_root);
paths_to_invalidate

It's possible that we could remove this case for some file change events: for example, if we can tell from the type of an event that only the content has changed.
@stuhood stuhood mentioned this issue Mar 16, 2021
Merged
@stuhood
Copy link
Member Author

stuhood commented Nov 10, 2021 •
edited
Loading

I've played with this a bit, and will post it soon. But unfortunately it's not quite as effective as we might have hoped. vim at least does the following when you write a file (:w):

21:24:32.83 [DEBUG] notify invalidating {"src/python/pants/util", "src/python/pants/util/4913"} because of Create(File)
21:24:32.83 [DEBUG] notify invalidating {"src/python/pants/util/4913", "src/python/pants/util"} because of Create(File)
21:24:32.84 [DEBUG] notify invalidating {"src/python/pants/util", "src/python/pants/util/4913"} because of Remove(File)
21:24:32.84 [DEBUG] notify invalidating {"src/python/pants/util", "src/python/pants/util/4913"} because of Modify(Metadata(Ownership))
21:24:32.84 [DEBUG] notify invalidating {"src/python/pants/util", "src/python/pants/util/dirutil_test.py"} because of Modify(Name(From))
21:24:32.85 [DEBUG] notify invalidating {"src/python/pants/util/dirutil_test.py", "src/python/pants/util"} because of Create(File)
21:24:32.85 [DEBUG] notify invalidating {"src/python/pants/util", "src/python/pants/util/dirutil_test.py"} because of Modify(Metadata(Any))
21:24:32.85 [DEBUG] notify invalidating {"src/python/pants/util", "src/python/pants/util/dirutil_test.py"} because of Create(File)
21:24:32.85 [DEBUG] notify invalidating {"src/python/pants/util", "src/python/pants/util/dirutil_test.py"} because of Modify(Metadata(Any))
21:24:32.85 [DEBUG] notify invalidating {"src/python/pants/util/dirutil_test.py", "src/python/pants/util"} because of Modify(Metadata(Ownership))
21:24:32.85 [DEBUG] notify invalidating {"src/python/pants/util/dirutil_test.py"} because of Modify(Data(Content))
21:24:32.87 [DEBUG] notify invalidating {"src/python/pants/util/4913", "src/python/pants/util"} because of Remove(File)
21:24:32.90 [DEBUG] notify invalidating {"src/python/pants/util", "src/python/pants/util/4913"} because of Modify(Metadata(Ownership))
21:24:32.92 [DEBUG] notify invalidating {"src/python/pants/util", "src/python/pants/util/dirutil_test.py"} because of Modify(Name(From))
21:24:32.94 [DEBUG] notify invalidating {"src/python/pants/util/dirutil_test.py", "src/python/pants/util"} because of Create(File)
21:24:32.95 [DEBUG] notify invalidating {"src/python/pants/util/dirutil_test.py", "src/python/pants/util"} because of Modify(Metadata(Any))
21:24:32.97 [DEBUG] notify invalidating {"src/python/pants/util", "src/python/pants/util/dirutil_test.py"} because of Create(File)
21:24:32.98 [DEBUG] notify invalidating {"src/python/pants/util/dirutil_test.py", "src/python/pants/util"} because of Modify(Metadata(Any))
21:24:32.99 [DEBUG] notify invalidating {"src/python/pants/util", "src/python/pants/util/dirutil_test.py"} because of Modify(Metadata(Ownership))
21:24:33.01 [DEBUG] notify invalidating {"src/python/pants/util/dirutil_test.py"} because of Modify(Data(Content))

...and I expect that this kind of behavior is fairly common.

@stuhood stuhood mentioned this issue Nov 10, 2021
Merged
stuhood added a commit that referenced this issue Nov 10, 2021
@stuhood
Do not invalidate parent paths when only file content has changed (#1…
c686c9c 
…3566)

We have avoided fine grained interaction with the `notify` crate so far, but now that we've seen that `notify` is reliable, we can slowly introduce more logic around event types.

This change skips invalidating the parent of a path when the event type is file-data-only (`Modify(Data(Content))`). As mentioned in #11707 though, it's not as effective as we might have hoped, since editors tend to write to temp files and then rename them. It might still have a positive impact in some cases, and seems like a relatively safe first step.

Fixes #11707.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Do not invalidate parent paths when only file content has changed stuhood/pants
1 participant
@stuhood