From 361d8d6a218ec51310d37fee239d6999790055d6 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Tue, 22 Oct 2024 19:20:54 +0900
Subject: [PATCH] working_copy: check reserved names even if existing parent
 entry is symlink

Although the path would be skipped anyway, it seemed odd that ".git" was
rejected as a reserved path whereas ".git/pwned" wasn't. Let's make both fail.
---
 lib/src/local_working_copy.rs        | 12 +++----
 lib/tests/test_local_working_copy.rs | 51 ++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs
index 4c248709a..fe0288810 100644
--- a/lib/src/local_working_copy.rs
+++ b/lib/src/local_working_copy.rs
@@ -682,13 +682,10 @@ fn create_parent_dirs(
         // 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.
-        let new_dir_created = match fs::create_dir(&dir_path) {
-            Ok(()) => true, // New directory
+        let (new_dir_created, is_dir) = match fs::create_dir(&dir_path) {
+            Ok(()) => (true, true), // New directory
             Err(err) => match dir_path.symlink_metadata() {
-                Ok(m) if m.is_dir() => false, // Existing directory
-                Ok(_) => {
-                    return Ok(None); // Skip existing file or symlink
-                }
+                Ok(m) => (false, m.is_dir()), // Existing file or directory
                 Err(_) => {
                     return Err(CheckoutError::Other {
                         message: format!(
@@ -707,6 +704,9 @@ fn create_parent_dirs(
                 fs::remove_dir(&dir_path).ok();
             }
         })?;
+        if !is_dir {
+            return Ok(None); // Skip existing file or symlink
+        }
     }
 
     let mut file_path = dir_path;
diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs
index ae4ebb63d..2b9b23fe0 100644
--- a/lib/tests/test_local_working_copy.rs
+++ b/lib/tests/test_local_working_copy.rs
@@ -2001,6 +2001,57 @@ fn test_check_out_reserved_file_path_vfat(vfat_path_str: &str, file_path_strs: &
     }
 }
 
+#[test_case(".git"; "root .git file")]
+#[test_case(".git/pwned"; "root .git dir")]
+fn test_check_out_reserved_file_path_dot_git_symlink(file_path_str: &str) {
+    if !check_symlink_support().unwrap() {
+        eprintln!("Skipping test because symlink isn't supported");
+        return;
+    }
+
+    let mut test_workspace = TestWorkspace::init();
+    let repo = &test_workspace.repo;
+    let workspace_root = test_workspace.workspace.workspace_root().to_owned();
+
+    // Create symlink .git -> ../git-repo
+    let git_repo_dir = test_workspace.env.root().join("git-repo");
+    let dot_git_path = workspace_root.join(".git");
+    std::fs::create_dir(&git_repo_dir).unwrap();
+    symlink_dir(&git_repo_dir, &dot_git_path);
+    assert!(dot_git_path.exists());
+
+    let file_path = repo_path(file_path_str);
+    let disk_path = file_path.to_fs_path_unchecked(&workspace_root);
+    let tree1 = create_tree(repo, &[(file_path, "contents")]);
+    let tree2 = create_tree(repo, &[]);
+    let commit1 = commit_with_tree(repo.store(), tree1);
+    let commit2 = commit_with_tree(repo.store(), tree2);
+
+    // Checkout should fail.
+    let ws = &mut test_workspace.workspace;
+    let result = ws.check_out(repo.op_id().clone(), None, &commit1);
+    assert_matches!(result, Err(CheckoutError::ReservedPathComponent { .. }));
+
+    // Therefore, "pwned" file shouldn't be created.
+    assert!(!git_repo_dir.join("pwned").exists());
+    assert!(!dot_git_path.join("pwned").exists());
+
+    // Pretend that the checkout somehow succeeded.
+    let mut locked_ws = ws.start_working_copy_mutation().unwrap();
+    locked_ws.locked_wc().reset(&commit1).block_on().unwrap();
+    locked_ws.finish(repo.op_id().clone()).unwrap();
+    if file_path_str != ".git" {
+        std::fs::write(&disk_path, "").unwrap();
+    }
+
+    // Check out empty tree, which tries to remove the file.
+    let result = ws.check_out(repo.op_id().clone(), None, &commit2);
+    assert_matches!(result, Err(CheckoutError::ReservedPathComponent { .. }));
+
+    // The existing file shouldn't be removed.
+    assert!(disk_path.exists());
+}
+
 #[test]
 fn test_fsmonitor() {
     let test_repo = TestRepo::init();