From 47307556dd72bdc0de0d92d24a5f1c54844ed17a Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sun, 8 Sep 2024 04:09:01 +0900 Subject: [PATCH] working_copy: pass SnapshotOptions by reference Though SnapshotOptions can be cheaply cloned, it doesn't make much sense that snapshot() consumes a settings-like object. --- cli/examples/custom-working-copy/main.rs | 9 ++++++--- cli/src/cli_util.rs | 2 +- cli/src/commands/file/untrack.rs | 2 +- cli/src/merge_tools/diff_working_copies.rs | 2 +- lib/src/local_working_copy.rs | 18 +++++++++--------- lib/src/working_copy.rs | 2 +- lib/tests/test_local_working_copy.rs | 12 ++++++------ .../test_local_working_copy_concurrent.rs | 2 +- lib/testutils/src/lib.rs | 4 ++-- 9 files changed, 28 insertions(+), 25 deletions(-) diff --git a/cli/examples/custom-working-copy/main.rs b/cli/examples/custom-working-copy/main.rs index fbe2e3ab7a..431c5c7209 100644 --- a/cli/examples/custom-working-copy/main.rs +++ b/cli/examples/custom-working-copy/main.rs @@ -232,9 +232,12 @@ impl LockedWorkingCopy for LockedConflictsWorkingCopy { self.inner.old_tree_id() } - fn snapshot(&mut self, mut options: SnapshotOptions) -> Result { - options.base_ignores = options.base_ignores.chain("", "/.conflicts".as_bytes())?; - self.inner.snapshot(options) + fn snapshot(&mut self, options: &SnapshotOptions) -> Result { + let options = SnapshotOptions { + base_ignores: options.base_ignores.chain("", "/.conflicts".as_bytes())?, + ..options.clone() + }; + self.inner.snapshot(&options) } fn check_out(&mut self, commit: &Commit) -> Result { diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 7cf29a4471..03c87b1627 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1367,7 +1367,7 @@ See https://martinvonz.github.io/jj/latest/working-copy/#stale-working-copy \ }; self.user_repo = ReadonlyUserRepo::new(repo); let progress = crate::progress::snapshot_progress(ui); - let new_tree_id = locked_ws.locked_wc().snapshot(SnapshotOptions { + let new_tree_id = locked_ws.locked_wc().snapshot(&SnapshotOptions { base_ignores, fsmonitor_settings, progress: progress.as_ref().map(|x| x as _), diff --git a/cli/src/commands/file/untrack.rs b/cli/src/commands/file/untrack.rs index 1999970bc1..72a9ceb2e0 100644 --- a/cli/src/commands/file/untrack.rs +++ b/cli/src/commands/file/untrack.rs @@ -68,7 +68,7 @@ pub(crate) fn cmd_file_untrack( locked_ws.locked_wc().reset(&new_commit)?; // 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_ws.locked_wc().snapshot(SnapshotOptions { + let wc_tree_id = locked_ws.locked_wc().snapshot(&SnapshotOptions { base_ignores, fsmonitor_settings: command.settings().fsmonitor_settings()?, progress: None, diff --git a/cli/src/merge_tools/diff_working_copies.rs b/cli/src/merge_tools/diff_working_copies.rs index 0845ce2366..e017a8de6b 100644 --- a/cli/src/merge_tools/diff_working_copies.rs +++ b/cli/src/merge_tools/diff_working_copies.rs @@ -282,7 +282,7 @@ diff editing in mind and be a little inaccurate. let mut output_tree_state = diff_wc .output_tree_state .unwrap_or(diff_wc.right_tree_state); - output_tree_state.snapshot(SnapshotOptions { + output_tree_state.snapshot(&SnapshotOptions { base_ignores, fsmonitor_settings: FsmonitorSettings::None, progress: None, diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 4e396df894..4e26b387e8 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -792,7 +792,7 @@ 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. #[instrument(skip_all)] - pub fn snapshot(&mut self, options: SnapshotOptions) -> Result { + pub fn snapshot(&mut self, options: &SnapshotOptions) -> Result { let SnapshotOptions { base_ignores, fsmonitor_settings, @@ -802,7 +802,7 @@ impl TreeState { let sparse_matcher = self.sparse_matcher(); - let fsmonitor_clock_needs_save = fsmonitor_settings != FsmonitorSettings::None; + let fsmonitor_clock_needs_save = *fsmonitor_settings != FsmonitorSettings::None; let mut is_dirty = fsmonitor_clock_needs_save; let FsmonitorMatcher { matcher: fsmonitor_matcher, @@ -829,7 +829,7 @@ impl TreeState { let directory_to_visit = DirectoryToVisit { dir: RepoPathBuf::root(), disk_dir: self.working_copy_path.clone(), - git_ignore: base_ignores, + git_ignore: base_ignores.clone(), file_states: self.file_states.all(), }; self.visit_directory( @@ -839,8 +839,8 @@ impl TreeState { file_states_tx, present_files_tx, directory_to_visit, - progress, - max_new_file_size, + *progress, + *max_new_file_size, ) })?; @@ -1075,13 +1075,13 @@ impl TreeState { #[instrument(skip_all)] fn make_fsmonitor_matcher( &self, - fsmonitor_settings: FsmonitorSettings, + fsmonitor_settings: &FsmonitorSettings, ) -> Result { let (watchman_clock, changed_files) = match fsmonitor_settings { FsmonitorSettings::None => (None, None), - FsmonitorSettings::Test { changed_files } => (None, Some(changed_files)), + FsmonitorSettings::Test { changed_files } => (None, Some(changed_files.clone())), #[cfg(feature = "watchman")] - FsmonitorSettings::Watchman(config) => match self.query_watchman(&config) { + FsmonitorSettings::Watchman(config) => match self.query_watchman(config) { Ok((watchman_clock, changed_files)) => (Some(watchman_clock.into()), changed_files), Err(err) => { tracing::warn!(?err, "Failed to query filesystem monitor"); @@ -1830,7 +1830,7 @@ impl LockedWorkingCopy for LockedLocalWorkingCopy { &self.old_tree_id } - fn snapshot(&mut self, options: SnapshotOptions) -> Result { + fn snapshot(&mut self, options: &SnapshotOptions) -> Result { let tree_state = self .wc .tree_state_mut() diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index ad6a4e696e..5699c4f3a6 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -100,7 +100,7 @@ pub trait LockedWorkingCopy { fn old_tree_id(&self) -> &MergedTreeId; /// Snapshot the working copy and return the tree id. - fn snapshot(&mut self, options: SnapshotOptions) -> Result; + fn snapshot(&mut self, options: &SnapshotOptions) -> Result; /// Check out the specified commit in the working copy. fn check_out(&mut self, commit: &Commit) -> Result; diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index 0d2383bf9f..862b531de3 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/lib/tests/test_local_working_copy.rs @@ -756,7 +756,7 @@ fn test_snapshot_racy_timestamps() { .unwrap(); let new_tree_id = locked_ws .locked_wc() - .snapshot(SnapshotOptions::empty_for_test()) + .snapshot(&SnapshotOptions::empty_for_test()) .unwrap(); assert_ne!(new_tree_id, previous_tree_id); previous_tree_id = new_tree_id; @@ -790,7 +790,7 @@ fn test_snapshot_special_file() { let mut locked_ws = ws.start_working_copy_mutation().unwrap(); let tree_id = locked_ws .locked_wc() - .snapshot(SnapshotOptions::empty_for_test()) + .snapshot(&SnapshotOptions::empty_for_test()) .unwrap(); locked_ws.finish(OperationId::from_hex("abc123")).unwrap(); let tree = store.get_root_tree(&tree_id).unwrap(); @@ -1204,7 +1204,7 @@ fn test_fsmonitor() { let fs_paths = paths.iter().map(|p| p.to_fs_path(Path::new(""))).collect(); locked_ws .locked_wc() - .snapshot(SnapshotOptions { + .snapshot(&SnapshotOptions { fsmonitor_settings: FsmonitorSettings::Test { changed_files: fs_paths, }, @@ -1283,16 +1283,16 @@ fn test_snapshot_max_new_file_size() { ..SnapshotOptions::empty_for_test() }; test_workspace - .snapshot_with_options(options.clone()) + .snapshot_with_options(&options) .expect("files exactly matching the size limit should succeed"); std::fs::write(small_path.to_fs_path(&workspace_root), vec![0; limit + 1]).unwrap(); test_workspace - .snapshot_with_options(options.clone()) + .snapshot_with_options(&options) .expect("existing files may grow beyond the size limit"); // A new file of 1KiB + 1 bytes should fail std::fs::write(large_path.to_fs_path(&workspace_root), vec![0; limit + 1]).unwrap(); let err = test_workspace - .snapshot_with_options(options.clone()) + .snapshot_with_options(&options) .expect_err("new files beyond the size limit should fail"); assert!( matches!(err, SnapshotError::NewFileTooLarge { .. }), diff --git a/lib/tests/test_local_working_copy_concurrent.rs b/lib/tests/test_local_working_copy_concurrent.rs index f1bd0c0467..ed1068a32c 100644 --- a/lib/tests/test_local_working_copy_concurrent.rs +++ b/lib/tests/test_local_working_copy_concurrent.rs @@ -138,7 +138,7 @@ fn test_checkout_parallel() { let mut locked_ws = workspace.start_working_copy_mutation().unwrap(); let new_tree_id = locked_ws .locked_wc() - .snapshot(SnapshotOptions::empty_for_test()) + .snapshot(&SnapshotOptions::empty_for_test()) .unwrap(); assert!(tree_ids.contains(&new_tree_id)); }); diff --git a/lib/testutils/src/lib.rs b/lib/testutils/src/lib.rs index a0cfabb62c..198dad18ed 100644 --- a/lib/testutils/src/lib.rs +++ b/lib/testutils/src/lib.rs @@ -267,7 +267,7 @@ impl TestWorkspace { /// new operation). pub fn snapshot_with_options( &mut self, - options: SnapshotOptions, + options: &SnapshotOptions, ) -> Result { let mut locked_ws = self.workspace.start_working_copy_mutation().unwrap(); let tree_id = locked_ws.locked_wc().snapshot(options)?; @@ -278,7 +278,7 @@ impl TestWorkspace { /// Like `snapshot_with_option()` but with default options pub fn snapshot(&mut self) -> Result { - self.snapshot_with_options(SnapshotOptions::empty_for_test()) + self.snapshot_with_options(&SnapshotOptions::empty_for_test()) } }