diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index bcbb2cfb3..f7b5082b1 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -1509,7 +1509,7 @@ 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)), } @@ -1517,15 +1517,31 @@ impl TreeState { .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())); diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index 9ef10a6bb..440d56b04 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/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_id)), + Merge::normal(TreeValue::GitSubmodule(submodule_id1)), ); - let tree_id = tree_builder.write_tree(&store).unwrap(); - let commit = commit_with_tree(repo.store(), tree_id.clone()); + 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_id2)), + ); + let tree_id2 = tree_builder.write_tree(&store).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, &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,7 +1217,7 @@ 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); @@ -1214,6 +1225,23 @@ fn test_git_submodule() { 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]