diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 285f70bf64b..fe93d9f1413 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -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. @@ -1883,6 +1884,10 @@ impl TreeState { Err(err) => (path, Err(err)), }) .buffered(self.store.concurrency().max(1)); + + let mut prev_created_path: Option = None; + let mut path_prefix; + while let Some((path, data)) = diff_stream.next().await { let (before, after) = data?; if after.is_absent() { @@ -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. @@ -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); diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index 40466088239..dd37cd89af0 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/lib/tests/test_local_working_copy.rs @@ -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] @@ -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")]