Skip to content

lib: substantially reduce syscalls when attempting to create dirs #7695

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

lib: substantially reduce syscalls when attempting to create dirs #7695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 63 additions & 6 deletions lib/src/local_working_copy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,7 @@ fn create_parent_dirs(
for c in parent_path.components() {
// Ensure that the name is a normal entry of the current dir_path.
dir_path.push(c.to_fs_name().map_err(|err| err.with_path(repo_path))?);

// A directory named ".git" or ".jj" can be temporarily created. It
// might trick workspace path discovery, but is harmless so long as the
// directory is empty.
Expand Down Expand Up @@ -1883,6 +1884,10 @@ impl TreeState {
Err(err) => (path, Err(err)),
})
.buffered(self.store.concurrency().max(1));

let mut prev_created_path: Option<RepoPathBuf> = None;
let mut path_prefix;

while let Some((path, data)) = diff_stream.next().await {
let (before, after) = data?;
if after.is_absent() {
Expand All @@ -1909,13 +1914,59 @@ impl TreeState {
continue;
}

// Create parent directories no matter if after.is_present(). This
// ensures that the path never traverses symlinks.
let Some(disk_path) = create_parent_dirs(&self.working_copy_path, &path)? else {
changed_file_states.push((path, FileState::placeholder()));
stats.skipped_files += 1;
continue;
let adjusted_working_copy_path;
let adjusted_diff_file_path = if let Some(ref prev_path) = prev_created_path {
// Obtain the common prefix between these paths
path_prefix = RepoPathBuf::root();
path_prefix.extend(
prev_path
.components()
.zip(path.components())
.take_while(|(prev_comp, this_comp)| prev_comp == this_comp)
.map(|(comp, _)| comp),
);

adjusted_working_copy_path = path_prefix.to_fs_path(self.working_copy_path())?;

path.strip_prefix(&path_prefix)
.expect("path should start with common prefix")
} else {
adjusted_working_copy_path = self.working_copy_path.clone();
path.as_ref()
};

let disk_path = if adjusted_diff_file_path.is_root() {
// the path being "root" here implies that we had already processed a path like:
// "foo/bar/baz"
//
// and this path is:
// "foo/bar"
//
// and now this path has been converted to an empty string since its entire
// prefix has already been created. This means that we _dont_ need to
// create its parent dirs either.

path.to_fs_path(self.working_copy_path())?
} else {
// Create parent directories no matter if after.is_present(). This
// ensures that the path never traverses symlinks.
let Some(disk_path) =
create_parent_dirs(&adjusted_working_copy_path, adjusted_diff_file_path)?
else {
changed_file_states.push((path, FileState::placeholder()));
stats.skipped_files += 1;
continue;
};

// Cache this path for the next iteration. This must occur after
// `create_parent_dirs` to ensure that the path is only set when
// no symlinks are encountered. Otherwise there could be
// opportunity for a filesystem write-what-where attack.
prev_created_path = Some(path.clone());

disk_path
};

// If the path was present, check reserved path first and delete it.
let present_file_deleted = before.is_present() && remove_old_file(&disk_path)?;
// If not, create temporary file to test the path validity.
Expand All @@ -1928,11 +1979,17 @@ impl TreeState {
// TODO: Check that the file has not changed before overwriting/removing it.
let file_state = match after {
MaterializedTreeValue::Absent | MaterializedTreeValue::AccessDenied(_) => {
// Reset the previous path to avoid scenarios where this path is deleted,
// then on the next iteration recreation is skipped because of this
// optimization.
prev_created_path.take();

let mut parent_dir = disk_path.parent().unwrap();
loop {
if fs::remove_dir(parent_dir).is_err() {
break;
}

parent_dir = parent_dir.parent().unwrap();
}
deleted_files.insert(path);
Expand Down
28 changes: 18 additions & 10 deletions lib/tests/test_local_working_copy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1515,17 +1515,23 @@ fn test_check_out_existing_directory_symlink() {
// under the symlinked directory.
try_symlink("..", workspace_root.join("parent")).unwrap();

let file_path = repo_path("parent/escaped");
let tree = create_tree(repo, &[(file_path, "contents")]);
// Test two file paths writing to the same directory to ensure that
// any directory creation optimizations which depend on how
// `parent/escaped1` behaved don't allow `parent/escaped2` to be
// created
let file_path1 = repo_path("parent/escaped1");
let file_path2 = repo_path("parent/escaped2");
let tree = create_tree(repo, &[(file_path1, "contents"), (file_path2, "contents")]);
let commit = commit_with_tree(repo.store(), tree.id());

// Checkout doesn't fail, but the file should be skipped.
let ws = &mut test_workspace.workspace;
let stats = ws.check_out(repo.op_id().clone(), None, &commit).unwrap();
assert_eq!(stats.skipped_files, 1);
assert_eq!(stats.skipped_files, 2);

// Therefore, "../escaped" shouldn't be created.
assert!(!workspace_root.parent().unwrap().join("escaped").exists());
// Therefore, "../escaped*" paths shouldn't be created.
assert!(!workspace_root.parent().unwrap().join("escaped1").exists());
assert!(!workspace_root.parent().unwrap().join("escaped2").exists());
}

#[test]
Expand All @@ -1544,21 +1550,23 @@ fn test_check_out_existing_directory_symlink_icase_fs() {
// under the symlinked directory.
try_symlink("..", workspace_root.join("parent")).unwrap();

let file_path = repo_path("PARENT/escaped");
let tree = create_tree(repo, &[(file_path, "contents")]);
let file_path1 = repo_path("PARENT/escaped1");
let file_path2 = repo_path("PARENT/escaped2");
let tree = create_tree(repo, &[(file_path1, "contents"), (file_path2, "contents")]);
let commit = commit_with_tree(repo.store(), tree.id());

// Checkout doesn't fail, but the file should be skipped on icase fs.
let ws = &mut test_workspace.workspace;
let stats = ws.check_out(repo.op_id().clone(), None, &commit).unwrap();
if is_icase_fs {
assert_eq!(stats.skipped_files, 1);
assert_eq!(stats.skipped_files, 2);
} else {
assert_eq!(stats.skipped_files, 0);
}

// Therefore, "../escaped" shouldn't be created.
assert!(!workspace_root.parent().unwrap().join("escaped").exists());
// Therefore, "../escaped*" paths shouldn't be created.
assert!(!workspace_root.parent().unwrap().join("escaped1").exists());
assert!(!workspace_root.parent().unwrap().join("escaped2").exists());
}

#[test_case(false; "symlink target does not exist")]
Expand Down