diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d7a54be4..70fb1c8f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -97,6 +97,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * `jj git import` no longer crashes when all Git refs are removed. +* Git submodules are now ignored completely. Earlier, files present in the + submodule directory in the working copy would become added (tracked), and + later removed if you checked out another commit. You can now use `git` to + populate the submodule directory and `jj` will leave it alone. + ### Contributors Thanks to the people who made this release happen! @@ -109,6 +114,7 @@ Thanks to the people who made this release happen! * Ruben Slabbert (@rslabbert) * Waleed Khan (@arxanas) * Sean E. Russell (@xxxserxxx) + * Pranay Sashank (@pranaysashank) ## [0.5.1] - 2022-10-17 diff --git a/lib/src/protos/working_copy.proto b/lib/src/protos/working_copy.proto index 4eafcf29b..9e6a38acd 100644 --- a/lib/src/protos/working_copy.proto +++ b/lib/src/protos/working_copy.proto @@ -21,6 +21,7 @@ enum FileType { Symlink = 1; Executable = 2; Conflict = 3; + GitSubmodule = 4; } message FileState { diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index 2b0f040e0..5bc79d671 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -48,6 +48,7 @@ use crate::tree_builder::TreeBuilder; pub enum FileType { Normal { executable: bool }, Symlink, + GitSubmodule, Conflict { id: ConflictId }, } @@ -89,6 +90,14 @@ impl FileState { } } + fn for_gitsubmodule() -> Self { + FileState { + file_type: FileType::GitSubmodule, + mtime: MillisSinceEpoch(0), + size: 0, + } + } + #[cfg_attr(unix, allow(dead_code))] fn is_executable(&self) -> bool { if let FileType::Normal { executable } = &self.file_type { @@ -125,6 +134,7 @@ fn file_state_from_proto(proto: &crate::protos::working_copy::FileState) -> File let id = ConflictId::new(proto.conflict_id.to_vec()); FileType::Conflict { id } } + crate::protos::working_copy::FileType::GitSubmodule => FileType::GitSubmodule, }; FileState { file_type, @@ -143,6 +153,7 @@ fn file_state_to_proto(file_state: &FileState) -> crate::protos::working_copy::F proto.conflict_id = id.to_bytes(); crate::protos::working_copy::FileType::Conflict } + FileType::GitSubmodule => crate::protos::working_copy::FileType::GitSubmodule, }; proto.file_type = EnumOrUnknown::new(file_type); proto.mtime_millis_since_epoch = file_state.mtime.0; @@ -468,8 +479,19 @@ impl TreeState { self.working_copy_path.clone(), base_ignores, )]; + let mut tree_builder = self.store.tree_builder(self.tree_id.clone()); - let mut deleted_files: HashSet<_> = self.file_states.keys().cloned().collect(); + let mut deleted_files: HashSet<_> = self + .file_states + .iter() + .filter_map(|(path, state)| { + if state.file_type != FileType::GitSubmodule { + Some(path.clone()) + } else { + None + } + }) + .collect(); while let Some((dir, disk_dir, git_ignore)) = work.pop() { if sparse_matcher.visit(&dir).is_nothing() { continue; @@ -489,6 +511,12 @@ impl TreeState { continue; } let sub_path = dir.join(&RepoPathComponent::from(name)); + if let Some(file_state) = self.file_states.get(&sub_path) { + if file_state.file_type == FileType::GitSubmodule { + continue; + } + } + if file_type.is_dir() { // If the whole directory is ignored, skip it unless we're already tracking // some file in it. @@ -555,6 +583,7 @@ impl TreeState { // ignore it. return Ok(()); } + let disk_path = dir_entry.path(); let metadata = dir_entry.metadata().map_err(|err| SnapshotError::IoError { message: format!("Failed to stat file {}", disk_path.display()), @@ -661,6 +690,7 @@ impl TreeState { Ok(TreeValue::Symlink(id)) } FileType::Conflict { .. } => panic!("conflicts should be handled by the caller"), + FileType::GitSubmodule => panic!("git submodule cannot be written to store"), } } @@ -858,7 +888,7 @@ impl TreeState { TreeValue::Conflict(id) => self.write_conflict(&disk_path, &path, &id)?, TreeValue::GitSubmodule(_id) => { println!("ignoring git submodule at {:?}", path); - return Ok(()); + FileState::for_gitsubmodule() } TreeValue::Tree(_id) => { panic!("unexpected tree entry in diff at {:?}", path); @@ -895,8 +925,7 @@ impl TreeState { } (_, TreeValue::GitSubmodule(_id)) => { println!("ignoring git submodule at {:?}", path); - self.file_states.remove(&path); - return Ok(()); + FileState::for_gitsubmodule() } (_, TreeValue::Tree(_id)) => { panic!("unexpected tree entry in diff at {:?}", path); @@ -937,7 +966,7 @@ impl TreeState { TreeValue::Conflict(id) => FileType::Conflict { id }, TreeValue::GitSubmodule(_id) => { println!("ignoring git submodule at {:?}", path); - continue; + FileType::GitSubmodule } TreeValue::Tree(_id) => { panic!("unexpected tree entry in diff at {:?}", path); diff --git a/lib/tests/test_working_copy.rs b/lib/tests/test_working_copy.rs index a67fa07cf..4e15636e2 100644 --- a/lib/tests/test_working_copy.rs +++ b/lib/tests/test_working_copy.rs @@ -69,7 +69,7 @@ fn test_checkout_file_transitions(use_git: bool) { let store = repo.store().clone(); let workspace_root = test_workspace.workspace.workspace_root().clone(); - #[derive(Debug, Clone, Copy)] + #[derive(Debug, PartialEq, Eq, Clone, Copy)] enum Kind { Missing, Normal, @@ -669,6 +669,71 @@ fn test_dotgit_ignored(use_git: bool) { locked_wc.discard(); } +#[test] +fn test_gitsubmodule() { + // Tests that git submodules are ignored. + + let settings = testutils::user_settings(); + let mut test_workspace = TestWorkspace::init(&settings, true); + let repo = &test_workspace.repo; + let store = repo.store().clone(); + let workspace_root = test_workspace.workspace.workspace_root().clone(); + + let mut tree_builder = store.tree_builder(store.empty_tree_id().clone()); + + let added_path = RepoPath::from_internal_string("added"); + let submodule_path = RepoPath::from_internal_string("submodule"); + let added_submodule_path = RepoPath::from_internal_string("submodule/added"); + + tree_builder.set( + added_path.clone(), + TreeValue::File { + id: testutils::write_file(repo.store(), &added_path, "added\n"), + executable: false, + }, + ); + + let mut tx = repo.start_transaction(&settings, "create submodule commit"); + let submodule_id = create_random_commit(&settings, repo) + .write_to_repo(tx.mut_repo()) + .id() + .clone(); + tx.commit(); + + tree_builder.set( + submodule_path.clone(), + TreeValue::GitSubmodule(submodule_id), + ); + + let tree_id = tree_builder.write_tree(); + let tree = store.get_tree(&RepoPath::root(), &tree_id).unwrap(); + let wc = test_workspace.workspace.working_copy_mut(); + wc.check_out(repo.op_id().clone(), None, &tree).unwrap(); + + std::fs::create_dir(submodule_path.to_fs_path(&workspace_root)).unwrap(); + + testutils::write_working_copy_file( + &workspace_root, + &added_submodule_path, + "i am a file in a submodule\n", + ); + + // Check that the files present in the submodule are not tracked + // when we snapshot + let mut locked_wc = wc.start_mutation(); + let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap(); + locked_wc.discard(); + assert_eq!(new_tree_id, tree_id); + + // Check that the files in the submodule are not deleted + let file_in_submodule_path = added_submodule_path.to_fs_path(&workspace_root); + assert!( + file_in_submodule_path.metadata().is_ok(), + "{:?} should exist", + file_in_submodule_path + ); +} + #[cfg(unix)] #[test_case(false ; "local backend")] #[test_case(true ; "git backend")]