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.

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

Initial fsmonitor implementation #362

Closed
wants to merge 4 commits into from

Conversation

arxanas
Copy link
Collaborator

@arxanas arxanas commented Jun 6, 2022

It might be a good idea to make a separate crate for fsmonitor, as it has some pretty heavyweight dependencies.

@arxanas arxanas marked this pull request as ready for review June 8, 2022 11:41
lib/src/fsmonitor.rs Outdated Show resolved Hide resolved
lib/src/fsmonitor.rs Outdated Show resolved Hide resolved
lib/src/fsmonitor.rs Outdated Show resolved Hide resolved
src/commands.rs Outdated Show resolved Hide resolved
src/commands.rs Outdated Show resolved Hide resolved
should_recurse: bool,
}
let repo_root = RepoPath::root();
let repo_root_pathbuf = repo_root.to_fs_path(&self.working_copy_path);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the same as self.working_copy_path.to_path_buf()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martinvonz The working_copy_path is relative here, so we won't be able to examine the file on disk without absolutizing it using to_fs_path.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it looks like we canonicalize the path in the "constructor" here:

working_copy_path: working_copy_path.canonicalize().unwrap(),
(I think the comment just above is obsolete; we don't use libgit2's for .gitignores anymore)

lib/src/working_copy.rs Outdated Show resolved Hide resolved
let previous_clock = None;
fsmonitor.query_changed_files(previous_clock).await
})
.await
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never used async/await, so please forgive this possibly naive question. Why do we start an async task (with spawn(), IIUC) if we're going to wait for it to finish here right away?

lib/src/working_copy.rs Outdated Show resolved Hide resolved
dir,
disk_dir: repo_root_pathbuf.join(path),

// FIXME: it's possible that we missed a `.gitignore`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can fix this by creating a FilesMatcher from changed_files and then intersect that with the sparse_matcher. It's just that we don't have a simple way of intersecting matchers yet. However, I have one lying around from some unfinished work on making auto-add optional (#323). Let me send that and you can see if that helps. To clarify, the idea is to always create a single initial WorkItem (thanks for creating a struct for that, btw) and use different matchers instead.

martinvonz added a commit that referenced this pull request Jun 10, 2022
I plan to use this matcher for some future `jj add` command (for
#323). The idea is that we'll do a path-restricted walk of the working
copy based on the intersection of the sparse patterns and any patterns
specified by the user. However, I think it will be useful before that,
for @arxanas's fsmonitor feature (#362).
martinvonz added a commit that referenced this pull request Jun 10, 2022
I plan to use this matcher for some future `jj add` command (for
#323). The idea is that we'll do a path-restricted walk of the working
copy based on the intersection of the sparse patterns and any patterns
specified by the user. However, I think it will be useful before that,
for @arxanas's fsmonitor feature (#362).
This identifies the directory as Watchman-enabled. Additional config settings can go in this file.
@arxanas arxanas marked this pull request as draft June 11, 2022 02:14
@arxanas
Copy link
Collaborator Author

arxanas commented Jan 11, 2023

I'm not working on a large repository as part of my work anymore, so I haven't needed this. Might resurrect later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants