local_working_copy: on check out, ignore diff of Git submodule ids

This is different from skipped paths because the file state has to remain as
FileType::GitSubmodule in order to ignore the submodule directory when
snapshotting.

Fixes #4825.

lib/src/local_working_copy.rs

@@ -1509,23 +1509,39 @@ impl TreeState {
                     match values {
                         Ok((before, after)) => {
                             let result = materialize_tree_value(&self.store, &path, after).await;
-                            (path, result.map(|value| (before.is_present(), value)))
+                            (path, result.map(|value| (before, value)))
                         }
                         Err(err) => (path, Err(err)),
                     }
                 })
                 .buffered(self.store.concurrency().max(1)),
         );
         while let Some((path, data)) = diff_stream.next().await {
-            let (present_before, after) = data?;
+            let (before, after) = data?;
             if after.is_absent() {
                 stats.removed_files += 1;
-            } else if !present_before {
+            } else if before.is_absent() {
                 stats.added_files += 1;
             } else {
                 stats.updated_files += 1;
             }
 
+            // Existing Git submodule can be a non-empty directory on disk. We
+            // shouldn't attempt to manage it as a tracked path.
+            //
+            // TODO: It might be better to add general support for paths not
+            // tracked by jj than processing submodules specially. For example,
+            // paths excluded by .gitignore can be marked as such so that
+            // newly-"unignored" paths won't be snapshotted automatically.
+            if matches!(before.as_normal(), Some(TreeValue::GitSubmodule(_)))
+                && matches!(after, MaterializedTreeValue::GitSubmodule(_))
+            {
+                eprintln!("ignoring git submodule at {path:?}");
+                // Not updating the file state as if there were no diffs. Leave
+                // the state type as FileType::GitSubmodule if it was before.
+                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 {
@@ -1534,7 +1550,7 @@ impl TreeState {
                 continue;
             };
             // If the path was present, check reserved path first and delete it.
-            let was_present = present_before && remove_old_file(&disk_path)?;
+            let was_present = before.is_present() && remove_old_file(&disk_path)?;
             // If not, create temporary file to test the path validity.
             if !was_present && !can_create_new_file(&disk_path)? {
                 changed_file_states.push((path, FileState::placeholder()));

lib/tests/test_local_working_copy.rs

@@ -1165,7 +1165,7 @@ fn test_git_submodule() {
 
     let settings = testutils::user_settings();
     let mut test_workspace = TestWorkspace::init_with_backend(&settings, TestRepoBackend::Git);
-    let repo = &test_workspace.repo;
+    let repo = test_workspace.repo.clone();
     let store = repo.store().clone();
     let workspace_root = test_workspace.workspace.workspace_root().to_owned();
     let mut tx = repo.start_transaction(&settings);
@@ -1175,6 +1175,7 @@ fn test_git_submodule() {
     let added_submodule_path = RepoPath::from_internal_string("submodule/added");
 
     let mut tree_builder = MergedTreeBuilder::new(store.empty_merged_tree_id());
+
     tree_builder.set_or_remove(
         added_path.to_owned(),
         Merge::normal(TreeValue::File {
@@ -1183,17 +1184,27 @@ fn test_git_submodule() {
         }),
     );
 
-    let submodule_id = write_random_commit(tx.repo_mut(), &settings).id().clone();
+    let submodule_id1 = write_random_commit(tx.repo_mut(), &settings).id().clone();
+
+    tree_builder.set_or_remove(
+        submodule_path.to_owned(),
+        Merge::normal(TreeValue::GitSubmodule(submodule_id1)),
+    );
+
+    let tree_id1 = tree_builder.write_tree(&store).unwrap();
+    let commit1 = 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(), &settings).id().clone();
     tree_builder.set_or_remove(
         submodule_path.to_owned(),
-        Merge::normal(TreeValue::GitSubmodule(submodule_id)),
+        Merge::normal(TreeValue::GitSubmodule(submodule_id2)),
     );
+    let tree_id2 = tree_builder.write_tree(&store).unwrap();
+    let commit2 = commit_with_tree(repo.store(), tree_id2.clone());
 
-    let tree_id = tree_builder.write_tree(&store).unwrap();
-    let commit = commit_with_tree(repo.store(), tree_id.clone());
     let ws = &mut test_workspace.workspace;
-    ws.check_out(repo.op_id().clone(), None, &commit).unwrap();
+    ws.check_out(repo.op_id().clone(), None, &commit1).unwrap();
 
     std::fs::create_dir(submodule_path.to_fs_path_unchecked(&workspace_root)).unwrap();
 
@@ -1206,14 +1217,31 @@ fn test_git_submodule() {
     // Check that the files present in the submodule are not tracked
     // when we snapshot
     let new_tree = test_workspace.snapshot().unwrap();
-    assert_eq!(new_tree.id(), tree_id);
+    assert_eq!(new_tree.id(), tree_id1);
+
+    // Check that the files in the submodule are not deleted
+    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 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();
 
     // Check that the files in the submodule are not deleted
     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 the files present in the submodule are not tracked
+    // when we snapshot
+    let new_tree = test_workspace.snapshot().unwrap();
+    assert_eq!(new_tree.id(), tree_id2);
 }
 
 #[test]