From 47067c1368a3d3e5831744f00ee5d1d18b70b271 Mon Sep 17 00:00:00 2001 From: Pranay Sashank Date: Thu, 24 Nov 2022 01:04:38 +0530 Subject: [PATCH] git: do not delete or track git submodules. A new FileType, GitSubmodule is added which is ignored. Files or directories having this type are not added to the work queue and are ignored in snapshot. Submodules are not created by jujutsu when resetting or checking out a tree, they should be currently managed using git. --- CHANGELOG.md | 6 +++ lib/src/protos/working_copy.proto | 1 + lib/src/working_copy.rs | 39 +++++++++++++++--- lib/tests/test_working_copy.rs | 67 ++++++++++++++++++++++++++++++- 4 files changed, 107 insertions(+), 6 deletions(-) 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")]