local_working_copy: skip existing symlinks consistently

If new file would overwrite an existing regular file, the file path is skipped.
It makes sense to apply the same rule to existing symlinks. Without this patch,
check out would fail if an existing path was a dead symlink or a symlink to
a directory.

lib/src/local_working_copy.rs

@@ -428,8 +428,9 @@ fn sparse_patterns_from_proto(
 /// Creates intermediate directories from the `working_copy_path` to the
 /// `repo_path` parent. Returns disk path for the `repo_path` file.
 ///
-/// If an intermediate directory exists and if it is a symlink, this function
+/// If an intermediate directory exists and if it is a file or symlink, this
-/// will return an error. The `working_copy_path` directory may be a symlink.
+/// function returns `Ok(None)` to signal that the path should be skipped.
+/// The `working_copy_path` directory may be a symlink.
 ///
 /// Note that this does not prevent TOCTOU bugs caused by concurrent checkouts.
 /// Another process may remove the directory created by this function and put a
@@ -443,24 +444,22 @@ fn create_parent_dirs(
     for c in parent_path.components() {
         dir_path.push(c.to_fs_name().map_err(|err| err.with_path(repo_path))?);
         match fs::create_dir(&dir_path) {
-            Ok(()) => {}
+            Ok(()) => {} // New directory
-            Err(_)
+            Err(err) => match dir_path.symlink_metadata() {
-                if dir_path
+                Ok(m) if m.is_dir() => {} // Existing directory
-                    .symlink_metadata()
+                Ok(_) => {
-                    .map(|m| m.is_dir())
+                    return Ok(None); // Skip existing file or symlink
-                    .unwrap_or(false) => {}
-            Err(err) => {
-                if dir_path.is_file() {
-                    return Ok(None);
                 }
-                return Err(CheckoutError::Other {
+                Err(_) => {
-                    message: format!(
+                    return Err(CheckoutError::Other {
-                        "Failed to create parent directories for {}",
+                        message: format!(
-                        repo_path.to_fs_path_unchecked(working_copy_path).display(),
+                            "Failed to create parent directories for {}",
-                    ),
+                            repo_path.to_fs_path_unchecked(working_copy_path).display(),
-                    err: err.into(),
+                        ),
-                });
+                        err: err.into(),
-            }
+                    })
+                }
+            },
         }
     }
 
@@ -485,6 +484,23 @@ fn remove_old_file(disk_path: &Path) -> Result<(), CheckoutError> {
     }
 }
 
+/// Checks if new file or symlink named `disk_path` can be created.
+///
+/// If the file already exists, this function return `Ok(false)` to signal
+/// that the path should be skipped.
+///
+/// This function can fail if `disk_path.parent()` isn't a directory.
+fn can_create_new_file(disk_path: &Path) -> Result<bool, CheckoutError> {
+    match disk_path.symlink_metadata() {
+        Ok(_) => Ok(false),
+        Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(true),
+        Err(err) => Err(CheckoutError::Other {
+            message: format!("Failed to stat {}", disk_path.display()),
+            err: err.into(),
+        }),
+    }
+}
+
 fn mtime_from_metadata(metadata: &Metadata) -> MillisSinceEpoch {
     let time = metadata
         .modified()
@@ -1441,11 +1457,12 @@ impl TreeState {
             };
             if present_before {
                 remove_old_file(&disk_path)?;
-            } else if disk_path.exists() {
+            } else if !can_create_new_file(&disk_path)? {
                 changed_file_states.push((path, FileState::placeholder()));
                 stats.skipped_files += 1;
                 continue;
             }
+
             // TODO: Check that the file has not changed before overwriting/removing it.
             let file_state = match after {
                 MaterializedTreeValue::Absent | MaterializedTreeValue::AccessDenied(_) => {

lib/tests/test_local_working_copy.rs

@@ -57,6 +57,17 @@ use testutils::write_random_commit;
 use testutils::TestRepoBackend;
 use testutils::TestWorkspace;
 
+fn check_icase_fs(dir: &Path) -> bool {
+    let test_file = tempfile::Builder::new()
+        .prefix("icase-")
+        .tempfile_in(dir)
+        .unwrap();
+    let orig_name = test_file.path().file_name().unwrap().to_str().unwrap();
+    let upper_name = orig_name.to_ascii_uppercase();
+    assert_ne!(orig_name, upper_name);
+    dir.join(upper_name).try_exists().unwrap()
+}
+
 fn to_owned_path_vec(paths: &[&RepoPath]) -> Vec<RepoPathBuf> {
     paths.iter().map(|&path| path.to_owned()).collect()
 }
@@ -1212,32 +1223,112 @@ fn test_check_out_existing_file_cannot_be_removed() {
 }
 
 #[test]
-fn test_existing_directory_symlink() {
+fn test_check_out_existing_directory_symlink() {
+    if !check_symlink_support().unwrap() {
+        eprintln!("Skipping test because symlink isn't supported");
+        return;
+    }
+
     let settings = testutils::user_settings();
     let mut test_workspace = TestWorkspace::init(&settings);
     let repo = &test_workspace.repo;
     let workspace_root = test_workspace.workspace.workspace_root().to_owned();
 
-    // Creates a symlink in working directory, and a tree that will add a file under
+    // Creates a symlink in working directory, and a tree that will add a file
-    // the symlinked directory.
+    // under the symlinked directory.
-    if check_symlink_support().unwrap_or(false) {
+    try_symlink("..", workspace_root.join("parent")).unwrap();
-        try_symlink("..", workspace_root.join("parent")).unwrap();
-    } else {
-        return;
-    }
 
     let file_path = RepoPath::from_internal_string("parent/escaped");
     let tree = create_tree(repo, &[(file_path, "contents")]);
     let commit = commit_with_tree(repo.store(), tree.id());
 
-    // Checkout should fail because "parent" already exists and is a symlink.
+    // Checkout doesn't fail, but the file should be skipped.
     let ws = &mut test_workspace.workspace;
-    assert!(ws.check_out(repo.op_id().clone(), None, &commit).is_err());
+    let stats = ws.check_out(repo.op_id().clone(), None, &commit).unwrap();
+    assert_eq!(stats.skipped_files, 1);
 
     // Therefore, "../escaped" shouldn't be created.
     assert!(!workspace_root.parent().unwrap().join("escaped").exists());
 }
 
+#[test]
+fn test_check_out_existing_directory_symlink_icase_fs() {
+    if !check_symlink_support().unwrap() {
+        eprintln!("Skipping test because symlink isn't supported");
+        return;
+    }
+
+    let settings = testutils::user_settings();
+    let mut test_workspace = TestWorkspace::init(&settings);
+    let repo = &test_workspace.repo;
+    let workspace_root = test_workspace.workspace.workspace_root().to_owned();
+    let is_icase_fs = check_icase_fs(&workspace_root);
+
+    // Creates a symlink in working directory, and a tree that will add a file
+    // under the symlinked directory.
+    try_symlink("..", workspace_root.join("parent")).unwrap();
+
+    let file_path = RepoPath::from_internal_string("PARENT/escaped");
+    let tree = create_tree(repo, &[(file_path, "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);
+    } else {
+        assert_eq!(stats.skipped_files, 0);
+    }
+
+    // Therefore, "../escaped" shouldn't be created.
+    assert!(!workspace_root.parent().unwrap().join("escaped").exists());
+}
+
+#[test_case(false; "symlink target does not exist")]
+#[test_case(true; "symlink target exists")]
+fn test_check_out_existing_file_symlink_icase_fs(victim_exists: bool) {
+    if !check_symlink_support().unwrap() {
+        eprintln!("Skipping test because symlink isn't supported");
+        return;
+    }
+
+    let settings = testutils::user_settings();
+    let mut test_workspace = TestWorkspace::init(&settings);
+    let repo = &test_workspace.repo;
+    let workspace_root = test_workspace.workspace.workspace_root().to_owned();
+    let is_icase_fs = check_icase_fs(&workspace_root);
+
+    // Creates a symlink in working directory, and a tree that will overwrite
+    // the symlink content.
+    try_symlink("../pwned", workspace_root.join("parent")).unwrap();
+    let victim_file_path = workspace_root.parent().unwrap().join("pwned");
+    if victim_exists {
+        std::fs::write(&victim_file_path, "old").unwrap();
+    }
+    assert_eq!(workspace_root.join("parent").exists(), victim_exists);
+
+    let file_path = RepoPath::from_internal_string("PARENT");
+    let tree = create_tree(repo, &[(file_path, "bad")]);
+    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);
+    } else {
+        assert_eq!(stats.skipped_files, 0);
+    }
+
+    // Therefore, "../pwned" shouldn't be updated.
+    if victim_exists {
+        assert_eq!(std::fs::read(&victim_file_path).unwrap(), b"old");
+    } else {
+        assert!(!victim_file_path.exists());
+    }
+}
+
 #[test]
 fn test_check_out_file_removal_over_existing_directory_symlink() {
     if !check_symlink_support().unwrap() {
@@ -1271,11 +1362,8 @@ fn test_check_out_file_removal_over_existing_directory_symlink() {
     assert!(file_path.to_fs_path_unchecked(&workspace_root).exists());
 
     // Check out empty tree, which tries to remove "parent/escaped".
-    let result = ws.check_out(repo.op_id().clone(), None, &commit2);
+    let stats = ws.check_out(repo.op_id().clone(), None, &commit2).unwrap();
-    assert_matches!(
+    assert_eq!(stats.skipped_files, 1);
-        result,
-        Err(CheckoutError::Other { message, .. }) if message.contains("create parent dir")
-    );
 
     // "../escaped" shouldn't be removed.
     assert!(victim_file_path.exists());