From d56ae79d3fc5b322c39fed9ccdc43a63e2f0456f Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Wed, 9 Mar 2022 22:41:09 -0800 Subject: [PATCH] working_copy: let caller pass in base Git ignores (#65, #87) The library crate shouldn't look up the user's `$HOME` directory (maybe the library is used by a server process), so let's have the caller pass it into the library crate instead. --- lib/src/working_copy.rs | 29 ++++++++++++----------- lib/tests/test_working_copy.rs | 25 +++++++++---------- lib/tests/test_working_copy_concurrent.rs | 3 ++- src/commands.rs | 28 +++++++++++++++++++--- src/diff_edit.rs | 4 +++- 5 files changed, 58 insertions(+), 31 deletions(-) diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index 492c0d55c..58fc58276 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -293,18 +293,12 @@ impl TreeState { // Look for changes to the working copy. If there are any changes, create // a new tree from it and return it, and also update the dirstate on disk. - pub fn write_tree(&mut self) -> TreeId { - // TODO: We should probably have the caller pass in the home directory to the - // library crate instead of depending on $HOME directly here. We should also - // have the caller (within the library crate) chain that the - // .jj/git/info/exclude file if we're inside a git-backed repo. - let mut git_ignore = GitIgnoreFile::empty(); - if let Ok(home_dir) = std::env::var("HOME") { - let home_dir_path = PathBuf::from(home_dir); - git_ignore = git_ignore.chain_with_file("", home_dir_path.join(".gitignore")); - } - - let mut work = vec![(RepoPath::root(), self.working_copy_path.clone(), git_ignore)]; + pub fn write_tree(&mut self, base_ignores: Arc) -> TreeId { + let mut work = vec![( + RepoPath::root(), + 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(); while !work.is_empty() { @@ -869,8 +863,15 @@ impl LockedWorkingCopy<'_> { &self.old_tree_id } - pub fn write_tree(&mut self) -> TreeId { - self.wc.tree_state().as_mut().unwrap().write_tree() + // The base_ignores are passed in here rather than being set on the TreeState + // because the TreeState may be long-lived if the library is used in a + // long-lived process. + pub fn write_tree(&mut self, base_ignores: Arc) -> TreeId { + self.wc + .tree_state() + .as_mut() + .unwrap() + .write_tree(base_ignores) } pub fn check_out(&mut self, new_tree: &Tree) -> Result { diff --git a/lib/tests/test_working_copy.rs b/lib/tests/test_working_copy.rs index 7a341a22d..804aa08b4 100644 --- a/lib/tests/test_working_copy.rs +++ b/lib/tests/test_working_copy.rs @@ -20,6 +20,7 @@ use std::sync::Arc; use itertools::Itertools; use jujutsu_lib::backend::{Conflict, ConflictPart, TreeValue}; +use jujutsu_lib::gitignore::GitIgnoreFile; use jujutsu_lib::op_store::WorkspaceId; use jujutsu_lib::repo::ReadonlyRepo; use jujutsu_lib::repo_path::{RepoPath, RepoPathComponent}; @@ -39,7 +40,7 @@ fn test_root(use_git: bool) { let wc = test_workspace.workspace.working_copy_mut(); let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.write_tree(); + let new_tree_id = locked_wc.write_tree(GitIgnoreFile::empty()); locked_wc.discard(); let checkout_id = repo.view().get_checkout(&WorkspaceId::default()).unwrap(); let checkout_commit = repo.store().get_commit(checkout_id).unwrap(); @@ -215,7 +216,7 @@ fn test_checkout_file_transitions(use_git: bool) { // Check that the working copy is clean. let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.write_tree(); + let new_tree_id = locked_wc.write_tree(GitIgnoreFile::empty()); locked_wc.discard(); assert_eq!(new_tree_id, right_tree_id); @@ -317,7 +318,7 @@ fn test_reset() { assert!(ignored_path.to_fs_path(&workspace_root).is_file()); assert!(!wc.file_states().contains_key(&ignored_path)); let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.write_tree(); + let new_tree_id = locked_wc.write_tree(GitIgnoreFile::empty()); assert_eq!(new_tree_id, *tree_without_file.id()); locked_wc.discard(); @@ -330,7 +331,7 @@ fn test_reset() { assert!(ignored_path.to_fs_path(&workspace_root).is_file()); assert!(!wc.file_states().contains_key(&ignored_path)); let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.write_tree(); + let new_tree_id = locked_wc.write_tree(GitIgnoreFile::empty()); assert_eq!(new_tree_id, *tree_without_file.id()); locked_wc.discard(); @@ -342,7 +343,7 @@ fn test_reset() { assert!(ignored_path.to_fs_path(&workspace_root).is_file()); assert!(wc.file_states().contains_key(&ignored_path)); let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.write_tree(); + let new_tree_id = locked_wc.write_tree(GitIgnoreFile::empty()); assert_eq!(new_tree_id, *tree_with_file.id()); locked_wc.discard(); } @@ -418,7 +419,7 @@ fn test_commit_racy_timestamps(use_git: bool) { .unwrap(); } let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.write_tree(); + let new_tree_id = locked_wc.write_tree(GitIgnoreFile::empty()); locked_wc.discard(); assert_ne!(new_tree_id, previous_tree_id); previous_tree_id = new_tree_id; @@ -452,7 +453,7 @@ fn test_gitignores(use_git: bool) { let wc = test_workspace.workspace.working_copy_mut(); let mut locked_wc = wc.start_mutation(); - let new_tree_id1 = locked_wc.write_tree(); + let new_tree_id1 = locked_wc.write_tree(GitIgnoreFile::empty()); locked_wc.finish(repo.op_id().clone()); let tree1 = repo .store() @@ -482,7 +483,7 @@ fn test_gitignores(use_git: bool) { testutils::write_working_copy_file(&workspace_root, &subdir_ignored_path, "2"); let mut locked_wc = wc.start_mutation(); - let new_tree_id2 = locked_wc.write_tree(); + let new_tree_id2 = locked_wc.write_tree(GitIgnoreFile::empty()); locked_wc.discard(); let tree2 = repo .store() @@ -542,7 +543,7 @@ fn test_gitignores_checkout_overwrites_ignored(use_git: bool) { // Check that the file is in the tree created by committing the working copy let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.write_tree(); + let new_tree_id = locked_wc.write_tree(GitIgnoreFile::empty()); locked_wc.discard(); let new_tree = repo .store() @@ -588,7 +589,7 @@ fn test_gitignores_ignored_directory_already_tracked(use_git: bool) { // Check that the file is still in the tree created by committing the working // copy (that it didn't get removed because the directory is ignored) let mut locked_wc = wc.start_mutation(); - let new_tree_id = locked_wc.write_tree(); + let new_tree_id = locked_wc.write_tree(GitIgnoreFile::empty()); locked_wc.discard(); let new_tree = repo .store() @@ -619,7 +620,7 @@ fn test_dotgit_ignored(use_git: bool) { "contents", ); let mut locked_wc = test_workspace.workspace.working_copy_mut().start_mutation(); - let new_tree_id = locked_wc.write_tree(); + let new_tree_id = locked_wc.write_tree(GitIgnoreFile::empty()); assert_eq!(new_tree_id, *repo.store().empty_tree_id()); locked_wc.discard(); std::fs::remove_dir_all(&dotgit_path).unwrap(); @@ -631,7 +632,7 @@ fn test_dotgit_ignored(use_git: bool) { "contents", ); let mut locked_wc = test_workspace.workspace.working_copy_mut().start_mutation(); - let new_tree_id = locked_wc.write_tree(); + let new_tree_id = locked_wc.write_tree(GitIgnoreFile::empty()); assert_eq!(new_tree_id, *repo.store().empty_tree_id()); locked_wc.discard(); } diff --git a/lib/tests/test_working_copy_concurrent.rs b/lib/tests/test_working_copy_concurrent.rs index 7a38ffb53..df704c177 100644 --- a/lib/tests/test_working_copy_concurrent.rs +++ b/lib/tests/test_working_copy_concurrent.rs @@ -15,6 +15,7 @@ use std::cmp::max; use std::thread; +use jujutsu_lib::gitignore::GitIgnoreFile; use jujutsu_lib::repo_path::RepoPath; use jujutsu_lib::testutils; use jujutsu_lib::working_copy::CheckoutError; @@ -128,7 +129,7 @@ fn test_checkout_parallel(use_git: bool) { // write_tree() should take the same lock as check_out(), write_tree() // should never produce a different tree. let mut locked_wc = workspace.working_copy_mut().start_mutation(); - let new_tree_id = locked_wc.write_tree(); + let new_tree_id = locked_wc.write_tree(GitIgnoreFile::empty()); locked_wc.discard(); assert!(tree_ids.contains(&new_tree_id)); }); diff --git a/src/commands.rs b/src/commands.rs index f486d5cc9..b702a443d 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -40,6 +40,7 @@ use jujutsu_lib::dag_walk::topo_order_reverse; use jujutsu_lib::diff::{Diff, DiffHunk}; use jujutsu_lib::files::DiffLine; use jujutsu_lib::git::{GitExportError, GitFetchError, GitImportError, GitRefUpdate}; +use jujutsu_lib::gitignore::GitIgnoreFile; use jujutsu_lib::index::HexPrefix; use jujutsu_lib::matchers::{EverythingMatcher, Matcher, PrefixMatcher}; use jujutsu_lib::op_heads_store::OpHeadsStore; @@ -381,6 +382,19 @@ impl WorkspaceCommandHelper { self.working_copy_shared_with_git } + fn base_ignores(&self) -> Arc { + let mut git_ignores = GitIgnoreFile::empty(); + if let Ok(home_dir) = std::env::var("HOME") { + let home_dir_path = PathBuf::from(home_dir); + // TODO: Look up the name of the file in the core.excludesFile config instead of + // hard-coding its name like this. + git_ignores = git_ignores.chain_with_file("", home_dir_path.join(".gitignore")); + } + // TODO: Chain with the .jj/git/info/exclude file if we're inside a git-backed + // repo. + git_ignores + } + fn resolve_revision_arg( &mut self, ui: &mut Ui, @@ -498,6 +512,7 @@ impl WorkspaceCommandHelper { return Ok(()); } }; + let base_ignores = self.base_ignores(); let mut locked_wc = self.workspace.working_copy_mut().start_mutation(); // Check if the working copy commit matches the repo's view. It's fine if it // doesn't, but we'll need to reload the repo so the new commit is @@ -557,7 +572,7 @@ impl WorkspaceCommandHelper { ))); } } - let new_tree_id = locked_wc.write_tree(); + let new_tree_id = locked_wc.write_tree(base_ignores); if new_tree_id != *checkout_commit.tree_id() { let mut tx = self.repo.start_transaction("commit working copy"); let mut_repo = tx.mut_repo(); @@ -595,7 +610,13 @@ impl WorkspaceCommandHelper { right_tree: &Tree, instructions: &str, ) -> Result { - crate::diff_edit::edit_diff(&self.settings, left_tree, right_tree, instructions) + crate::diff_edit::edit_diff( + &self.settings, + left_tree, + right_tree, + instructions, + self.base_ignores(), + ) } fn start_transaction(&self, description: &str) -> Transaction { @@ -1839,6 +1860,7 @@ fn cmd_untrack( }; let mut tx = workspace_command.start_transaction("untrack paths"); + let base_ignores = workspace_command.base_ignores(); let mut locked_working_copy = workspace_command.working_copy_mut().start_mutation(); if current_checkout.tree_id() != locked_working_copy.old_tree_id() { return Err(CommandError::UserError( @@ -1856,7 +1878,7 @@ fn cmd_untrack( locked_working_copy.reset(&new_tree)?; // Commit the working copy again so we can inform the user if paths couldn't be // untracked because they're not ignored. - let wc_tree_id = locked_working_copy.write_tree(); + let wc_tree_id = locked_working_copy.write_tree(base_ignores); if wc_tree_id != new_tree_id { let wc_tree = store.get_tree(&RepoPath::root(), &wc_tree_id)?; let added_back = wc_tree.entries_matching(matcher.as_ref()).collect_vec(); diff --git a/src/diff_edit.rs b/src/diff_edit.rs index ed780c331..35e0ad678 100644 --- a/src/diff_edit.rs +++ b/src/diff_edit.rs @@ -19,6 +19,7 @@ use std::process::Command; use std::sync::Arc; use jujutsu_lib::backend::{BackendError, TreeId}; +use jujutsu_lib::gitignore::GitIgnoreFile; use jujutsu_lib::matchers::EverythingMatcher; use jujutsu_lib::repo_path::RepoPath; use jujutsu_lib::settings::UserSettings; @@ -79,6 +80,7 @@ pub fn edit_diff( left_tree: &Tree, right_tree: &Tree, instructions: &str, + base_ignores: Arc, ) -> Result { // First create partial Trees of only the subset of the left and right trees // that affect files changed between them. @@ -148,7 +150,7 @@ pub fn edit_diff( // Create a Tree based on the initial right tree, applying the changes made to // that directory by the diff editor. - let new_right_partial_tree_id = right_tree_state.write_tree(); + let new_right_partial_tree_id = right_tree_state.write_tree(base_ignores); let new_right_partial_tree = store.get_tree(&RepoPath::root(), &new_right_partial_tree_id)?; let new_tree_id = merge_trees(right_tree, &right_partial_tree, &new_right_partial_tree)?;