working_copy: existing submodule directory on checkout is ok

CHANGELOG.md

@@ -28,6 +28,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 * Broken symlink on Windows. [#6934](https://github.com/jj-vcs/jj/issues/6934).
 
+* Pre-existing Git submodule directories are no longer considered conflicts in
+  checkouts. [#8065](https://github.com/jj-vcs/jj/issues/8065).
+
 ## [0.36.0] - 2025-12-03
 
 ### Release highlights

lib/src/local_working_copy.rs

@@ -735,6 +735,31 @@ fn remove_old_file(disk_path: &Path) -> Result<bool, CheckoutError> {
     }
 }
 
+/// Removes existing submodule directory named `disk_path` if any. Returns
+/// `Ok(true)` if the directory was there and got removed, meaning that new file
+/// can be safely created.
+///
+/// The directory will not be removed if it is not empty, as it could contain
+/// untracked or modified files. This is in line with Git's behavior.
+///
+/// If the existing directory points to ".git" or ".jj", this function returns
+/// an error.
+fn remove_old_submodule_dir(disk_path: &Path) -> Result<bool, CheckoutError> {
+    reject_reserved_existing_path(disk_path)?;
+    match fs::remove_dir(disk_path) {
+        Ok(()) => Ok(true),
+        Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(false),
+        Err(err) if err.kind() == io::ErrorKind::DirectoryNotEmpty => Ok(false),
+        Err(err) => Err(CheckoutError::Other {
+            message: format!(
+                "Failed to remove submodule directory {}",
+                disk_path.display()
+            ),
+            err: err.into(),
+        }),
+    }
+}
+
 /// Checks if new file or symlink named `disk_path` can be created.
 ///
 /// If the file already exists, this function return `Ok(false)` to signal
@@ -2173,14 +2198,31 @@ impl TreeState {
 
                 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)?;
+            let present_file_deleted = before.is_present()
+                && if matches!(before.as_normal(), Some(TreeValue::GitSubmodule(_))) {
+                    remove_old_submodule_dir(&disk_path)?
+                } else {
+                    remove_old_file(&disk_path)?
+                };
+
             // If not, create temporary file to test the path validity.
             if !present_file_deleted && !can_create_new_file(&disk_path)? {
-                changed_file_states.push((path, FileState::placeholder()));
-                stats.skipped_files += 1;
-                continue;
+                if disk_path.is_dir() && matches!(after, MaterializedTreeValue::GitSubmodule(_)) {
+                    // Failing to materalize submodule, over a directory which
+                    // is presumably the submodule before it was added in a
+                    // commit, is ok.
+                } else if matches!(before.as_normal(), Some(TreeValue::GitSubmodule(_)))
+                    && after.is_absent()
+                {
+                    // Failing to delete un-tracked submodule directory is ok,
+                    // as the, possibly untracked, contents would otherwise be
+                    // lost.
+                } else {
+                    changed_file_states.push((path, FileState::placeholder()));
+                    stats.skipped_files += 1;
+                    continue;
+                }
             }
 
             // We get the previous executable bit from the file states and not
@@ -2224,6 +2266,11 @@ impl TreeState {
                 }
                 MaterializedTreeValue::GitSubmodule(_) => {
                     eprintln!("ignoring git submodule at {path:?}");
+                    // Git behavior: Create the submodule directory but don't
+                    // populate/overwrite the contents.
+                    if fs::create_dir_all(&disk_path).is_err() {
+                        eprintln!("warning: failed to create submodule directory {path:?}");
+                    }
                     FileState::for_gitsubmodule()
                 }
                 MaterializedTreeValue::Tree(_) => {

lib/tests/test_local_working_copy.rs

@@ -375,8 +375,9 @@ fn test_checkout_file_transitions(backend: TestRepoBackend) {
                 assert!(metadata.is_dir(), "{path:?} should be a directory");
             }
             Kind::GitSubmodule => {
-                // Not supported for now
-                assert!(maybe_metadata.is_err(), "{path:?} should not exist");
+                assert!(maybe_metadata.is_ok(), "{path:?} should exist");
+                let metadata = maybe_metadata.unwrap();
+                assert!(metadata.is_dir(), "{path:?} should be a directory");
             }
         };
     }
@@ -1407,7 +1408,7 @@ fn test_git_submodule(gitignore_content: &str) {
     );
 
     let tree_id1 = tree_builder.write_tree().unwrap();
-    let commit1 = commit_with_tree(repo.store(), tree_id1.clone());
+    let commit1_submod = commit_with_tree(repo.store(), tree_id1.clone());
 
     let mut tree_builder = MergedTreeBuilder::new(tree_id1.clone());
     let submodule_id2 = write_random_commit(tx.repo_mut()).id().clone();
@@ -1416,13 +1417,28 @@ fn test_git_submodule(gitignore_content: &str) {
         Merge::normal(TreeValue::GitSubmodule(submodule_id2)),
     );
     let tree_id2 = tree_builder.write_tree().unwrap();
-    let commit2 = commit_with_tree(repo.store(), tree_id2.clone());
-
-    let ws = &mut test_workspace.workspace;
-    ws.check_out(repo.op_id().clone(), None, &commit1).unwrap();
+    let commit2_submod = commit_with_tree(repo.store(), tree_id2.clone());
 
-    std::fs::create_dir(submodule_path.to_fs_path_unchecked(&workspace_root)).unwrap();
+    // A commit with a file instead of the submodule at the same path
+    let mut tree_builder = MergedTreeBuilder::new(store.empty_merged_tree());
+    tree_builder.set_or_remove(
+        submodule_path.to_owned(),
+        Merge::normal(TreeValue::File {
+            id: testutils::write_file(
+                repo.store(),
+                submodule_path,
+                "file with same path as submodule\n",
+            ),
+            executable: false,
+            copy_id: CopyId::new(vec![]),
+        }),
+    );
+    let tree_id3 = tree_builder.write_tree().unwrap();
+    let commit3_file_clash = commit_with_tree(repo.store(), tree_id3.clone());
 
+    let ws = &mut test_workspace.workspace;
+    ws.check_out(repo.op_id().clone(), None, &commit1_submod)
+        .unwrap();
     testutils::write_working_copy_file(
         &workspace_root,
         added_submodule_path,
@@ -1446,7 +1462,8 @@ fn test_git_submodule(gitignore_content: &str) {
     // Check out new commit updating the submodule, which shouldn't fail because
     // of existing submodule files
     let ws = &mut test_workspace.workspace;
-    ws.check_out(repo.op_id().clone(), None, &commit2).unwrap();
+    ws.check_out(repo.op_id().clone(), None, &commit2_submod)
+        .unwrap();
 
     // Check that the files in the submodule are not deleted
     let file_in_submodule_path = added_submodule_path.to_fs_path_unchecked(&workspace_root);
@@ -1467,7 +1484,88 @@ fn test_git_submodule(gitignore_content: &str) {
     let stats = ws
         .check_out(repo.op_id().clone(), None, &store.root_commit())
         .unwrap();
-    assert_eq!(stats.skipped_files, 1);
+    assert_eq!(stats.skipped_files, 0, "Empty tree should checkout cleanly");
+
+    // Start with an empty submodule directory and check out a commit without
+    // the submodule
+    let ws = &mut test_workspace.workspace;
+    ws.check_out(repo.op_id().clone(), None, &commit1_submod)
+        .unwrap();
+    std::fs::remove_file(added_submodule_path.to_fs_path_unchecked(&workspace_root)).unwrap();
+    let ws = &mut test_workspace.workspace;
+    ws.check_out(repo.op_id().clone(), None, &store.root_commit())
+        .unwrap();
+
+    // Check that the empty submodule directory was removed
+    let submodule_dir = submodule_path.to_fs_path_unchecked(&workspace_root);
+    assert!(
+        submodule_dir.metadata().is_err(),
+        "{submodule_dir:?} should not exist"
+    );
+
+    // Go back to a commit with the submodule
+    let ws = &mut test_workspace.workspace;
+    ws.check_out(repo.op_id().clone(), None, &commit2_submod)
+        .unwrap();
+
+    // Check that the empty submodule directory was created
+    let submodule_dir = submodule_path.to_fs_path_unchecked(&workspace_root);
+    assert!(
+        submodule_dir.metadata().is_ok(),
+        "{submodule_dir:?} should exist"
+    );
+    assert_eq!(stats.skipped_files, 0);
+
+    // Restore submodule contents (pretend that the user did `git submodule update`)
+    testutils::write_working_copy_file(
+        &workspace_root,
+        added_submodule_path,
+        "i am a file in a submodule\n",
+    );
+
+    // Check that the files in the submodule are not deleted after checking out
+    // a commit without the submodule
+    let ws = &mut test_workspace.workspace;
+    let stats = ws
+        .check_out(repo.op_id().clone(), None, &store.root_commit())
+        .unwrap();
+    let file_in_submodule_path = added_submodule_path.to_fs_path_unchecked(&workspace_root);
+    assert!(
+        file_in_submodule_path.metadata().is_ok(),
+        "{file_in_submodule_path:?} should exist"
+    );
+
+    // Check that checking out a submodule over an existing directory with the
+    // same path does not result in a conflict and that the submodule is still
+    // recorded as a submodule in the commit
+    let ws = &mut test_workspace.workspace;
+    ws.check_out(repo.op_id().clone(), None, &commit2_submod)
+        .unwrap();
+    assert_eq!(stats.skipped_files, 0);
+    let (new_tree, _stats) = test_workspace
+        .snapshot_with_options(&snapshot_options)
+        .unwrap();
+    assert_tree_eq!(new_tree, tree_id2);
+
+    // Restore submodule contents (pretend that the user did `git submodule update`)
+    testutils::write_working_copy_file(
+        &workspace_root,
+        added_submodule_path,
+        "i am a file in a submodule\n",
+    );
+
+    // Check out a commit which tries to place a file at the same path
+    let ws = &mut test_workspace.workspace;
+    ws.check_out(repo.op_id().clone(), None, &commit3_file_clash)
+        .unwrap();
+
+    // Check that the submodule is not replaced by the file, preserving the
+    // user's existing submodule files
+    let file_in_submodule_path = added_submodule_path.to_fs_path_unchecked(&workspace_root);
+    assert!(
+        file_in_submodule_path.metadata().is_ok(),
+        "{file_in_submodule_path:?} should exist"
+    );
 }
 
 #[test]