From 51d7d8ea98852896bb817448b3a65c9e560f79b0 Mon Sep 17 00:00:00 2001 From: Ian Wrzesinski Date: Sat, 14 Sep 2024 17:17:31 -0400 Subject: [PATCH 1/3] local_working_copy: Initial option for reporting exec bit changes on Unix This is just the initial support for the feature. Threading the value into the working copy via user configuration is in the next commit. --- lib/src/file_util.rs | 29 ++- lib/src/local_working_copy.rs | 327 ++++++++++++++++++++++++---------- 2 files changed, 261 insertions(+), 95 deletions(-) diff --git a/lib/src/file_util.rs b/lib/src/file_util.rs index 12c83afe9c..5655dc2e66 100644 --- a/lib/src/file_util.rs +++ b/lib/src/file_util.rs @@ -165,11 +165,38 @@ pub fn persist_content_addressed_temp_file>( #[cfg(unix)] mod platform { + use std::fs; use std::io; use std::os::unix::fs::symlink; + use std::os::unix::fs::PermissionsExt; use std::path::Path; + use std::path::PathBuf; - /// Symlinks are always available on UNIX + use tempfile::NamedTempFile; + + /// Whether executable bits are supported for the filesystem of this path. + pub fn check_executable_bit_support(path: &PathBuf) -> bool { + const DEFAULT: bool = true; // default to true on Unix + fn check_exec(path: &PathBuf) -> Result { + // get current permissions and update with exec bit: + let temp_file = NamedTempFile::new_in(path)?; + let old_mode = temp_file.as_file().metadata()?.permissions().mode(); + // TODO: Maybe check if we can disable the exec permission if we + // start executable? + let new_mode = old_mode | 0o100; + let new_perms = PermissionsExt::from_mode(new_mode); + if fs::set_permissions(temp_file.path(), new_perms).is_ok() { + let mode = temp_file.as_file().metadata()?.permissions().mode(); + Ok(new_mode == mode && new_mode != old_mode) + } else { + Ok(DEFAULT) + } + // TODO: Actually test this function. + } + check_exec(path).unwrap_or(DEFAULT) + } + + /// Symlinks are always available on Unix pub fn check_symlink_support() -> io::Result { Ok(true) } diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index da2f550814..386a09ba69 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -63,6 +63,8 @@ use crate::conflicts; use crate::conflicts::materialize_merge_result; use crate::conflicts::materialize_tree_value; use crate::conflicts::MaterializedTreeValue; +#[cfg(unix)] +use crate::file_util::check_executable_bit_support; use crate::file_util::check_symlink_support; use crate::file_util::try_symlink; #[cfg(feature = "watchman")] @@ -104,19 +106,115 @@ use crate::working_copy::WorkingCopy; use crate::working_copy::WorkingCopyFactory; use crate::working_copy::WorkingCopyStateError; +/// The executable bit for a filetype, potentially ignored. +/// +/// On Windows there is no executable bit, so this will always be Ignore. On +/// Unix it will usually be Exec(true|false), but may be ignored by a config +/// value or by a check we run when we load the config if not specified. +#[derive(Debug, Clone, Copy)] +pub enum ExecFlag { + Exec(bool), + Ignore, +} +// Note: cannot derive Eq or PartialEq since a == b == c does not imply a == c. +// e.g. `Exec(true) == Ignore == Exec(false)` but `Exec(true) != Exec(false)` + +impl ExecFlag { + fn matches(&self, other: &Self) -> bool { + match (self, other) { + (ExecFlag::Exec(a), ExecFlag::Exec(b)) => a == b, + // Always treat as equal if either is Ignore. + _ => true, + } + } + + /// Create a bool in an environment where we can't check a IgnoreExec value. + fn from_bool_unchecked(executable: bool) -> Self { + if cfg!(unix) { + ExecFlag::Exec(executable) + } else { + ExecFlag::Ignore + } + } +} + +/// Whether to ignore the executable bit when comparing files. The executable +/// state is always ignored on Windows, but is respected by default on Unix and +/// is ignored if we find that the filesystem doesn't support it or by user +/// configuration. #[cfg(unix)] -type FileExecutableFlag = bool; +#[derive(Debug, Clone, Copy)] +struct IgnoreExec(bool); #[cfg(windows)] -type FileExecutableFlag = (); +#[derive(Debug, Clone, Copy)] +struct IgnoreExec; + +impl IgnoreExec { + /// Load from user settings. If the setting is not given on Unix, then we + /// check whether executable bits are supported in the working copy's + /// filesystem and return true or false accordingly. + fn load_config(exec_config: Option, wc_path: &PathBuf) -> Self { + #[cfg(unix)] // check for executable support on Unix. + let ignore_exec = + IgnoreExec(exec_config.unwrap_or_else(|| !check_executable_bit_support(wc_path))); + #[cfg(windows)] + let (ignore_exec, _, _) = (IgnoreExec, exec_config, wc_path); // use the variables + ignore_exec + } -#[derive(Debug, PartialEq, Eq, Clone)] + /// Push into an Option config value for roundtripping. + fn as_config(self) -> Option { + #[cfg(unix)] + let exec_config = Some(self.0); + #[cfg(windows)] + let (exec_config, _) = (None, self); // use the variable + exec_config + } + + /// Resolve an executable bit into a flag for the FileType, potentially + /// ignoring it. + fn into_flag bool>(self, is_executable: F) -> ExecFlag { + #[cfg(unix)] + let exec_flag = if self.0 { + ExecFlag::Ignore + } else { + ExecFlag::Exec(is_executable()) + }; + #[cfg(windows)] + let (exec_flag, _, _) = (ExecFlag::Ignore, self, is_executable); // use the variables + exec_flag + } + + /// Convert a flag into the executable bit to write with a closure for a + /// default value. + fn exec_bit_to_write bool>(self, exec_flag: ExecFlag, default: F) -> bool { + #[cfg(unix)] + let executable = match (self.0, exec_flag) { + (false, ExecFlag::Exec(executable)) => executable, + (true | false, _) => default(), + }; + #[cfg(windows)] + let (executable, _, _) = (default(), self, exec_flag); // use the variables + executable + } +} + +#[derive(Debug, Clone)] pub enum FileType { - Normal { executable: FileExecutableFlag }, + Normal { exec_flag: ExecFlag }, Symlink, GitSubmodule, } -#[derive(Debug, PartialEq, Eq, Clone)] +impl Default for FileType { + fn default() -> Self { + FileType::Normal { + exec_flag: ExecFlag::Exec(false), + } + } +} + +#[derive(Debug, Clone)] pub struct FileState { pub file_type: FileType, pub mtime: MillisSinceEpoch, @@ -130,30 +228,41 @@ impl FileState { /// Indicates that a file exists in the tree but that it needs to be /// re-stat'ed on the next snapshot. fn placeholder() -> Self { - #[cfg(unix)] - let executable = false; - #[cfg(windows)] - let executable = (); FileState { - file_type: FileType::Normal { executable }, + file_type: FileType::default(), mtime: MillisSinceEpoch(0), size: 0, } } - fn for_file(executable: bool, size: u64, metadata: &Metadata) -> Self { - #[cfg(windows)] - let executable = { - // Windows doesn't support executable bit. - let _ = executable; - }; + /// Filestate for a normal file.. + fn for_file(size: u64, metadata: &Metadata, exec_flag: ExecFlag) -> Self { FileState { - file_type: FileType::Normal { executable }, + file_type: FileType::Normal { exec_flag }, mtime: mtime_from_metadata(metadata), size, } } + /// Whether this file state is compatible with another file state. The extra + /// complexity here comes from executable flags which always match on + /// Windows and might always match on Unix if ignore_exec is true. + fn matches(&self, other: &Self) -> bool { + use FileType::*; + let file_types_match = match (&self.file_type, &other.file_type) { + (GitSubmodule, GitSubmodule) | (Symlink, Symlink) => true, + (Normal { exec_flag: lhs }, Normal { exec_flag: rhs }) => lhs.matches(rhs), + _ => false, + }; + file_types_match && self.mtime == other.mtime && self.size == other.size + } + + /// Inverse of `self.matches(other)`. + fn differs(&self, other: &Self) -> bool { + !self.matches(other) + } + + /// Filestate for a symlink. fn for_symlink(metadata: &Metadata) -> Self { // When using fscrypt, the reported size is not the content size. So if // we were to record the content size here (like we do for regular files), we @@ -165,6 +274,7 @@ impl FileState { } } + /// Filestate for a git submodule. fn for_gitsubmodule() -> Self { FileState { file_type: FileType::GitSubmodule, @@ -334,6 +444,7 @@ pub struct TreeState { sparse_patterns: Vec, own_mtime: MillisSinceEpoch, symlink_support: bool, + ignore_exec: IgnoreExec, /// The most recent clock value returned by Watchman. Will only be set if /// the repo is configured to use the Watchman filesystem monitor and @@ -344,17 +455,14 @@ pub struct TreeState { fn file_state_from_proto(proto: &crate::protos::working_copy::FileState) -> FileState { let file_type = match proto.file_type() { crate::protos::working_copy::FileType::Normal => FileType::Normal { - executable: FileExecutableFlag::default(), + exec_flag: ExecFlag::from_bool_unchecked(false), }, - #[cfg(unix)] - crate::protos::working_copy::FileType::Executable => FileType::Normal { executable: true }, - // can exist in files written by older versions of jj - #[cfg(windows)] - crate::protos::working_copy::FileType::Executable => FileType::Normal { executable: () }, - crate::protos::working_copy::FileType::Symlink => FileType::Symlink, - crate::protos::working_copy::FileType::Conflict => FileType::Normal { - executable: FileExecutableFlag::default(), + // can exist for Windows in files written by older versions of jj + crate::protos::working_copy::FileType::Executable => FileType::Normal { + exec_flag: ExecFlag::from_bool_unchecked(true), }, + crate::protos::working_copy::FileType::Symlink => FileType::Symlink, + crate::protos::working_copy::FileType::Conflict => FileType::default(), crate::protos::working_copy::FileType::GitSubmodule => FileType::GitSubmodule, }; FileState { @@ -367,12 +475,10 @@ fn file_state_from_proto(proto: &crate::protos::working_copy::FileState) -> File fn file_state_to_proto(file_state: &FileState) -> crate::protos::working_copy::FileState { let mut proto = crate::protos::working_copy::FileState::default(); let file_type = match &file_state.file_type { - #[cfg(unix)] - FileType::Normal { executable: false } => crate::protos::working_copy::FileType::Normal, - #[cfg(unix)] - FileType::Normal { executable: true } => crate::protos::working_copy::FileType::Executable, - #[cfg(windows)] - FileType::Normal { executable: () } => crate::protos::working_copy::FileType::Normal, + FileType::Normal { + exec_flag: ExecFlag::Exec(true), + } => crate::protos::working_copy::FileType::Executable, + FileType::Normal { exec_flag: _ } => crate::protos::working_copy::FileType::Normal, FileType::Symlink => crate::protos::working_copy::FileType::Symlink, FileType::GitSubmodule => crate::protos::working_copy::FileType::GitSubmodule, }; @@ -479,7 +585,7 @@ fn mtime_from_metadata(metadata: &Metadata) -> MillisSinceEpoch { ) } -fn file_state(metadata: &Metadata) -> Option { +fn file_state(metadata: &Metadata, ignore_exec: IgnoreExec) -> Option { let metadata_file_type = metadata.file_type(); let file_type = if metadata_file_type.is_dir() { None @@ -487,13 +593,10 @@ fn file_state(metadata: &Metadata) -> Option { Some(FileType::Symlink) } else if metadata_file_type.is_file() { #[cfg(unix)] - if metadata.permissions().mode() & 0o111 != 0 { - Some(FileType::Normal { executable: true }) - } else { - Some(FileType::Normal { executable: false }) - } + let exec_flag = ignore_exec.into_flag(|| metadata.permissions().mode() & 0o111 != 0); #[cfg(windows)] - Some(FileType::Normal { executable: () }) + let exec_flag = ExecFlag::Ignore; + Some(FileType::Normal { exec_flag }) } else { None }; @@ -567,18 +670,26 @@ impl TreeState { Box::new(PrefixMatcher::new(&self.sparse_patterns)) } + /// Initialize an empty tree state and save it to the filesystem. pub fn init( store: Arc, working_copy_path: PathBuf, state_path: PathBuf, ) -> Result { - let mut wc = TreeState::empty(store, working_copy_path, state_path); + let mut wc = TreeState::empty(store, working_copy_path, state_path, None); wc.save()?; Ok(wc) } - fn empty(store: Arc, working_copy_path: PathBuf, state_path: PathBuf) -> TreeState { + /// Create a new empty tree state for this working copy path. + fn empty( + store: Arc, + working_copy_path: PathBuf, + state_path: PathBuf, + exec_config: Option, + ) -> TreeState { let tree_id = store.empty_merged_tree_id(); + let ignore_exec = IgnoreExec::load_config(exec_config, &working_copy_path); TreeState { store, working_copy_path, @@ -589,18 +700,23 @@ impl TreeState { own_mtime: MillisSinceEpoch(0), symlink_support: check_symlink_support().unwrap_or(false), watchman_clock: None, + ignore_exec, } } + /// Load an existing tree state if present, or initialize an empty one. pub fn load( store: Arc, working_copy_path: PathBuf, state_path: PathBuf, + exec_config: Option, ) -> Result { let tree_state_path = state_path.join("tree_state"); let file = match File::open(&tree_state_path) { Err(ref err) if err.kind() == std::io::ErrorKind::NotFound => { - return TreeState::init(store, working_copy_path, state_path); + let mut wc = TreeState::empty(store, working_copy_path, state_path, exec_config); + wc.save()?; + return Ok(wc); } Err(err) => { return Err(TreeStateError::ReadTreeState { @@ -611,7 +727,7 @@ impl TreeState { Ok(file) => file, }; - let mut wc = TreeState::empty(store, working_copy_path, state_path); + let mut wc = TreeState::empty(store, working_copy_path, state_path, exec_config); wc.read(&tree_state_path, file)?; Ok(wc) } @@ -624,6 +740,7 @@ impl TreeState { } } + /// Load the tree's data from the filesystem. fn read(&mut self, tree_state_path: &Path, mut file: File) -> Result<(), TreeStateError> { self.update_own_mtime(); let mut buf = Vec::new(); @@ -655,6 +772,7 @@ impl TreeState { Ok(()) } + /// Save the tree's data to the filesystem. #[allow(unknown_lints)] // XXX FIXME (aseipp): nightly bogons; re-test this occasionally #[allow(clippy::assigning_clones)] fn save(&mut self) -> Result<(), TreeStateError> { @@ -855,7 +973,8 @@ impl TreeState { file_states .iter() .filter(|(path, state)| { - fsmonitor_matcher.matches(path) && state.file_type != FileType::GitSubmodule + fsmonitor_matcher.matches(path) + && !matches!(state.file_type, FileType::GitSubmodule) }) .map(|(path, _state)| path.to_owned()) .collect() @@ -959,7 +1078,7 @@ impl TreeState { let path = dir.join(RepoPathComponent::new(name)); let maybe_current_file_state = file_states.get(&path); if let Some(file_state) = &maybe_current_file_state { - if file_state.file_type == FileType::GitSubmodule { + if matches!(file_state.file_type, FileType::GitSubmodule) { return Ok(()); } } @@ -994,7 +1113,7 @@ impl TreeState { }); } }; - if let Some(new_file_state) = file_state(&metadata) { + if let Some(new_file_state) = file_state(&metadata, self.ignore_exec) { present_files_tx.send(tracked_path.to_owned()).ok(); let update = self.get_updated_tree_value( tracked_path, @@ -1008,7 +1127,7 @@ impl TreeState { .send((tracked_path.to_owned(), tree_value)) .ok(); } - if new_file_state != current_file_state { + if new_file_state.differs(¤t_file_state) { file_states_tx .send((tracked_path.to_owned(), new_file_state)) .ok(); @@ -1062,7 +1181,7 @@ impl TreeState { max_size: HumanByteSize(max_new_file_size), }); } - if let Some(new_file_state) = file_state(&metadata) { + if let Some(new_file_state) = file_state(&metadata, self.ignore_exec) { present_files_tx.send(path.clone()).ok(); let update = self.get_updated_tree_value( &path, @@ -1074,7 +1193,10 @@ impl TreeState { if let Some(tree_value) = update { tree_entries_tx.send((path.clone(), tree_value)).ok(); } - if Some(&new_file_state) != maybe_current_file_state.as_ref() { + if maybe_current_file_state + .map(|fs| new_file_state.differs(&fs)) + .unwrap_or(true) + { file_states_tx.send((path, new_file_state)).ok(); } } @@ -1147,7 +1269,8 @@ impl TreeState { Some(current_file_state) => { // If the file's mtime was set at the same time as this state file's own mtime, // then we don't know if the file was modified before or after this state file. - current_file_state == new_file_state && current_file_state.mtime < self.own_mtime + current_file_state.matches(new_file_state) + && current_file_state.mtime < self.own_mtime } }; if clean { @@ -1166,8 +1289,8 @@ impl TreeState { new_file_state.file_type.clone() }; let new_tree_values = match new_file_type { - FileType::Normal { executable } => self - .write_path_to_store(repo_path, &disk_path, ¤t_tree_values, executable) + FileType::Normal { exec_flag } => self + .write_path_to_store(repo_path, &disk_path, ¤t_tree_values, exec_flag) .block_on()?, FileType::Symlink => { let id = self @@ -1190,24 +1313,19 @@ impl TreeState { repo_path: &RepoPath, disk_path: &Path, current_tree_values: &MergedTreeValue, - executable: FileExecutableFlag, + exec_flag: ExecFlag, ) -> Result { // If the file contained a conflict before and is now a normal file on disk, we // try to parse any conflict markers in the file into a conflict. if let Some(current_tree_value) = current_tree_values.as_resolved() { - #[cfg(unix)] - let _ = current_tree_value; // use the variable let id = self.write_file_to_store(repo_path, disk_path).await?; - // On Windows, we preserve the executable bit from the current tree. - #[cfg(windows)] - let executable = { - let () = executable; // use the variable - if let Some(TreeValue::File { id: _, executable }) = current_tree_value { - *executable - } else { - false - } - }; + // Use the given executable bit or return the current bit. + let executable = + self.ignore_exec + .exec_bit_to_write(exec_flag, || match current_tree_value { + Some(TreeValue::File { id: _, executable }) => *executable, + _ => false, + }); Ok(Merge::normal(TreeValue::File { id, executable })) } else if let Some(old_file_ids) = current_tree_values.to_file_merge() { let content = fs::read(disk_path).map_err(|err| SnapshotError::Other { @@ -1223,16 +1341,15 @@ impl TreeState { .block_on()?; match new_file_ids.into_resolved() { Ok(file_id) => { - // On Windows, we preserve the executable bit from the merged trees. - #[cfg(windows)] - let executable = { - let () = executable; // use the variable + // Use the given executable bit or preserve the executable + // bit from the merged trees. + let executable = self.ignore_exec.exec_bit_to_write(exec_flag, || { if let Some(merge) = current_tree_values.to_executable_merge() { - merge.resolve_trivial().copied().unwrap_or_default() + merge.resolve_trivial().copied().unwrap_or(false) } else { false } - }; + }); Ok(Merge::normal(TreeValue::File { id: file_id.unwrap(), executable, @@ -1269,7 +1386,7 @@ impl TreeState { message: format!("Failed to write file {}", disk_path.display()), err: err.into(), })?; - self.set_executable(disk_path, executable)?; + let exec_flag = self.set_executable_get_flag(disk_path, executable)?; // Read the file state from the file descriptor. That way, know that the file // exists and is of the expected type, and the stat information is most likely // accurate, except for other processes modifying the file concurrently (The @@ -1277,7 +1394,7 @@ impl TreeState { let metadata = file .metadata() .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; - Ok(FileState::for_file(executable, size, &metadata)) + Ok(FileState::for_file(size, &metadata, exec_flag)) } fn write_symlink(&self, disk_path: &Path, target: String) -> Result { @@ -1316,22 +1433,28 @@ impl TreeState { err: err.into(), })?; let size = conflict_data.len() as u64; - self.set_executable(disk_path, executable)?; + let exec_flag = self.set_executable_get_flag(disk_path, executable)?; let metadata = file .metadata() .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; - Ok(FileState::for_file(executable, size, &metadata)) + Ok(FileState::for_file(size, &metadata, exec_flag)) } - #[cfg_attr(windows, allow(unused_variables))] - fn set_executable(&self, disk_path: &Path, executable: bool) -> Result<(), CheckoutError> { + /// Maybe set the executable bit and return the flag or an error. This is a + /// no-op on Windows. + fn set_executable_get_flag( + &self, + disk_path: &Path, + executable: bool, + ) -> Result { + let exec_flag = self.ignore_exec.into_flag(|| executable); #[cfg(unix)] - { + if let ExecFlag::Exec(executable) = exec_flag { let mode = if executable { 0o755 } else { 0o644 }; fs::set_permissions(disk_path, fs::Permissions::from_mode(mode)) .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; - } - Ok(()) + }; + Ok(exec_flag) } pub fn check_out(&mut self, new_tree: &MergedTree) -> Result { @@ -1513,10 +1636,9 @@ impl TreeState { } else { let file_type = match after.into_resolved() { Ok(value) => match value.unwrap() { - #[cfg(unix)] - TreeValue::File { id: _, executable } => FileType::Normal { executable }, - #[cfg(windows)] - TreeValue::File { .. } => FileType::Normal { executable: () }, + TreeValue::File { id: _, executable } => FileType::Normal { + exec_flag: self.ignore_exec.into_flag(|| executable), + }, TreeValue::Symlink(_id) => FileType::Symlink, TreeValue::Conflict(_id) => { panic!("unexpected conflict entry in diff at {path:?}"); @@ -1529,11 +1651,17 @@ impl TreeState { panic!("unexpected tree entry in diff at {path:?}"); } }, - Err(_values) => { - // TODO: Try to set the executable bit based on the conflict - FileType::Normal { - executable: FileExecutableFlag::default(), + // TODO: Try to set the executable bit based on the conflict + Err(values) => { + let mut file_type = FileType::default(); + for value in values.adds().flatten() { + // Use the *last* added filetype from the merge + if let TreeValue::File { id: _, executable } = value { + let exec_flag = self.ignore_exec.into_flag(|| *executable); + file_type = FileType::Normal { exec_flag }; + } } + file_type } }; let file_state = FileState { @@ -1577,6 +1705,7 @@ pub struct LocalWorkingCopy { state_path: PathBuf, checkout_state: OnceCell, tree_state: OnceCell, + ignore_exec: IgnoreExec, } impl WorkingCopy for LocalWorkingCopy { @@ -1617,6 +1746,7 @@ impl WorkingCopy for LocalWorkingCopy { // TODO: It's expensive to reload the whole tree. We should copy it from `self` if it // hasn't changed. tree_state: OnceCell::new(), + ignore_exec: self.ignore_exec, }; let old_operation_id = wc.operation_id().clone(); let old_tree_id = wc.tree_id()?.clone(); @@ -1663,12 +1793,14 @@ impl LocalWorkingCopy { err: err.into(), }, )?; + let ignore_exec = tree_state.ignore_exec; Ok(LocalWorkingCopy { store, working_copy_path, state_path, checkout_state: OnceCell::new(), tree_state: OnceCell::with_value(tree_state), + ignore_exec, }) } @@ -1677,12 +1809,14 @@ impl LocalWorkingCopy { working_copy_path: PathBuf, state_path: PathBuf, ) -> LocalWorkingCopy { + let ignore_exec = IgnoreExec::load_config(None, &working_copy_path); LocalWorkingCopy { store, working_copy_path, state_path, checkout_state: OnceCell::new(), tree_state: OnceCell::new(), + ignore_exec, } } @@ -1731,6 +1865,7 @@ impl LocalWorkingCopy { self.store.clone(), self.working_copy_path.clone(), self.state_path.clone(), + self.ignore_exec.as_config(), ) }) .map_err(|err| WorkingCopyStateError { @@ -1979,12 +2114,18 @@ mod tests { RepoPath::from_internal_string(value) } + // Only for convenience in these tests. File states are *not* transitively + // equal (due to ExecFlag), so we should not implement PartialEq generally. + impl PartialEq for FileState { + fn eq(&self, other: &Self) -> bool { + self.matches(other) + } + } + #[test] fn test_file_states_merge() { let new_state = |size| FileState { - file_type: FileType::Normal { - executable: FileExecutableFlag::default(), - }, + file_type: FileType::default(), mtime: MillisSinceEpoch(0), size, }; @@ -2030,9 +2171,7 @@ mod tests { #[test] fn test_file_states_lookup() { let new_state = |size| FileState { - file_type: FileType::Normal { - executable: FileExecutableFlag::default(), - }, + file_type: FileType::default(), mtime: MillisSinceEpoch(0), size, }; From 660b01c8c46910865399b7fc52f412801f44ad2a Mon Sep 17 00:00:00 2001 From: Ian Wrzesinski Date: Sat, 14 Sep 2024 17:17:31 -0400 Subject: [PATCH 2/3] local_working_copy: Allow user to configure exec bit behavior on Unix --- CHANGELOG.md | 5 +++ cli/examples/custom-working-copy/main.rs | 15 +++++++-- cli/src/config-schema.json | 4 +++ cli/src/merge_tools/diff_working_copies.rs | 17 ++++++++-- cli/src/merge_tools/external.rs | 4 ++- cli/src/merge_tools/mod.rs | 10 ++++-- cli/src/ui.rs | 3 ++ docs/config.md | 32 +++++++++++++++++++ lib/src/local_working_copy.rs | 35 ++++++++++++++------- lib/src/settings.rs | 11 +++++++ lib/src/working_copy.rs | 3 ++ lib/src/workspace.rs | 7 ++++- lib/tests/test_local_working_copy.rs | 15 +++++++-- lib/tests/test_local_working_copy_sparse.rs | 1 + 14 files changed, 139 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e0c0c21288..baa3587cdf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,11 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * `jj commit` and `jj describe` now accept `--author` option allowing to quickly change author of given commit. +* Updated how we handle executable bits in the working copy to allow Unix to + ignore executable bit changes when using a nonstandard filesystem. We try to + detect the filesystem's behavior on Unix by default, but it can be overridden + manually by setting `core.ignore-executable-bit = true`. + ### Fixed bugs * Update working copy before reporting changes. This prevents errors during reporting diff --git a/cli/examples/custom-working-copy/main.rs b/cli/examples/custom-working-copy/main.rs index 276ad1fd83..095da7d70e 100644 --- a/cli/examples/custom-working-copy/main.rs +++ b/cli/examples/custom-working-copy/main.rs @@ -119,6 +119,7 @@ impl ConflictsWorkingCopy { state_path: PathBuf, operation_id: OperationId, workspace_id: WorkspaceId, + settings: &UserSettings, ) -> Result { let inner = LocalWorkingCopy::init( store, @@ -126,6 +127,7 @@ impl ConflictsWorkingCopy { state_path, operation_id, workspace_id, + settings, )?; Ok(ConflictsWorkingCopy { inner: Box::new(inner), @@ -133,8 +135,13 @@ impl ConflictsWorkingCopy { }) } - fn load(store: Arc, working_copy_path: PathBuf, state_path: PathBuf) -> Self { - let inner = LocalWorkingCopy::load(store, working_copy_path.clone(), state_path); + fn load( + store: Arc, + working_copy_path: PathBuf, + state_path: PathBuf, + settings: &UserSettings, + ) -> Self { + let inner = LocalWorkingCopy::load(store, working_copy_path.clone(), state_path, settings); ConflictsWorkingCopy { inner: Box::new(inner), working_copy_path, @@ -186,6 +193,7 @@ impl WorkingCopyFactory for ConflictsWorkingCopyFactory { state_path: PathBuf, operation_id: OperationId, workspace_id: WorkspaceId, + settings: &UserSettings, ) -> Result, WorkingCopyStateError> { Ok(Box::new(ConflictsWorkingCopy::init( store, @@ -193,6 +201,7 @@ impl WorkingCopyFactory for ConflictsWorkingCopyFactory { state_path, operation_id, workspace_id, + settings, )?)) } @@ -201,11 +210,13 @@ impl WorkingCopyFactory for ConflictsWorkingCopyFactory { store: Arc, working_copy_path: PathBuf, state_path: PathBuf, + settings: &UserSettings, ) -> Result, WorkingCopyStateError> { Ok(Box::new(ConflictsWorkingCopy::load( store, working_copy_path, state_path, + settings, ))) } } diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 997d62c829..de010abd66 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -178,6 +178,10 @@ "description": "Whether to use triggers to monitor for changes in the background." } } + }, + "ignore-executable-bit": { + "type": "boolean", + "description": "Whether to ignore changes to the executable bit for files on Unix. This is always true on Windows" } } }, diff --git a/cli/src/merge_tools/diff_working_copies.rs b/cli/src/merge_tools/diff_working_copies.rs index 749255326d..be2363178b 100644 --- a/cli/src/merge_tools/diff_working_copies.rs +++ b/cli/src/merge_tools/diff_working_copies.rs @@ -84,10 +84,11 @@ fn check_out( state_dir: PathBuf, tree: &MergedTree, sparse_patterns: Vec, + exec_config: Option, ) -> Result { std::fs::create_dir(&wc_dir).map_err(DiffCheckoutError::SetUpDir)?; std::fs::create_dir(&state_dir).map_err(DiffCheckoutError::SetUpDir)?; - let mut tree_state = TreeState::init(store, wc_dir, state_dir)?; + let mut tree_state = TreeState::init(store, wc_dir, state_dir, exec_config)?; tree_state.set_sparse_patterns(sparse_patterns)?; tree_state.check_out(tree)?; Ok(tree_state) @@ -135,6 +136,7 @@ pub(crate) fn check_out_trees( right_tree: &MergedTree, matcher: &dyn Matcher, output_is: Option, + exec_config: Option, ) -> Result { let changed_files: Vec<_> = left_tree .diff_stream(right_tree, matcher) @@ -153,6 +155,7 @@ pub(crate) fn check_out_trees( left_state_dir, left_tree, changed_files.clone(), + exec_config, )?; let right_tree_state = check_out( store.clone(), @@ -160,6 +163,7 @@ pub(crate) fn check_out_trees( right_state_dir, right_tree, changed_files.clone(), + exec_config, )?; let output_tree_state = output_is .map(|output_side| { @@ -174,6 +178,7 @@ pub(crate) fn check_out_trees( DiffSide::Right => right_tree, }, changed_files, + exec_config, ) }) .transpose()?; @@ -200,8 +205,16 @@ impl DiffEditWorkingCopies { matcher: &dyn Matcher, output_is: Option, instructions: Option<&str>, + exec_config: Option, ) -> Result { - let diff_wc = check_out_trees(store, left_tree, right_tree, matcher, output_is)?; + let diff_wc = check_out_trees( + store, + left_tree, + right_tree, + matcher, + output_is, + exec_config, + )?; let got_output_field = output_is.is_some(); set_readonly_recursively(diff_wc.left_working_copy_path()) diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index a01187b189..d8e9b7425b 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -255,6 +255,7 @@ pub fn edit_diff_external( matcher: &dyn Matcher, instructions: Option<&str>, base_ignores: Arc, + exec_config: Option, ) -> Result { let got_output_field = find_all_variables(&editor.edit_args).contains(&"output"); let store = left_tree.store(); @@ -265,6 +266,7 @@ pub fn edit_diff_external( matcher, got_output_field.then_some(DiffSide::Right), instructions, + exec_config, )?; let patterns = diffedit_wc.working_copies.to_command_variables(); @@ -296,7 +298,7 @@ pub fn generate_diff( tool: &ExternalMergeTool, ) -> Result<(), DiffGenerateError> { let store = left_tree.store(); - let diff_wc = check_out_trees(store, left_tree, right_tree, matcher, None)?; + let diff_wc = check_out_trees(store, left_tree, right_tree, matcher, None, ui.exec_config)?; set_readonly_recursively(diff_wc.left_working_copy_path()) .map_err(ExternalToolError::SetUpDir)?; set_readonly_recursively(diff_wc.right_working_copy_path()) diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 39da4dec1f..7993d94565 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -26,6 +26,7 @@ use jj_lib::matchers::Matcher; use jj_lib::merged_tree::MergedTree; use jj_lib::repo_path::RepoPath; use jj_lib::repo_path::RepoPathBuf; +use jj_lib::settings::ignore_executable_bit; use jj_lib::settings::ConfigResultExt as _; use jj_lib::settings::UserSettings; use jj_lib::working_copy::SnapshotError; @@ -178,6 +179,7 @@ pub struct DiffEditor { tool: MergeTool, base_ignores: Arc, use_instructions: bool, + exec_config: Option, } impl DiffEditor { @@ -190,7 +192,8 @@ impl DiffEditor { ) -> Result { let tool = get_tool_config(settings, name)? .unwrap_or_else(|| MergeTool::external(ExternalMergeTool::with_program(name))); - Self::new_inner(tool, settings, base_ignores) + let exec_config = ignore_executable_bit(settings.config()); + Self::new_inner(tool, settings, base_ignores, exec_config) } /// Loads the default diff editor from the settings. @@ -206,18 +209,20 @@ impl DiffEditor { None } .unwrap_or_else(|| MergeTool::external(ExternalMergeTool::with_edit_args(&args))); - Self::new_inner(tool, settings, base_ignores) + Self::new_inner(tool, settings, base_ignores, ui.exec_config) } fn new_inner( tool: MergeTool, settings: &UserSettings, base_ignores: Arc, + exec_config: Option, ) -> Result { Ok(DiffEditor { tool, base_ignores, use_instructions: settings.config().get_bool("ui.diff-instructions")?, + exec_config, }) } @@ -242,6 +247,7 @@ impl DiffEditor { matcher, instructions.as_deref(), self.base_ignores.clone(), + self.exec_config, ) } } diff --git a/cli/src/ui.rs b/cli/src/ui.rs index be604b87d0..c518eb9e8c 100644 --- a/cli/src/ui.rs +++ b/cli/src/ui.rs @@ -32,6 +32,7 @@ use std::thread::JoinHandle; use indoc::indoc; use itertools::Itertools as _; +use jj_lib::settings::ignore_executable_bit; use minus::MinusError; use minus::Pager as MinusPager; use tracing::instrument; @@ -259,6 +260,7 @@ pub struct Ui { progress_indicator: bool, formatter_factory: FormatterFactory, output: UiOutput, + pub exec_config: Option, } fn progress_indicator_setting(config: &config::Config) -> bool { @@ -366,6 +368,7 @@ impl Ui { paginate: pagination_setting(config)?, progress_indicator, output: UiOutput::new_terminal(), + exec_config: ignore_executable_bit(config), }) } diff --git a/docs/config.md b/docs/config.md index 1c1ae2aef6..1267eda9d1 100644 --- a/docs/config.md +++ b/docs/config.md @@ -951,6 +951,38 @@ snapshots on filestem changes by setting You can check whether Watchman is enabled and whether it is installed correctly using `jj debug watchman status`. +## Ignore the executable bit + +Whether to ignore changes to the executable bit for files on Unix. This option +is unused on Windows. + +`core.ignore-executable-bit = true | false` + +On Unix systems, files have permission bits for whether they are executable as +scripts or binary code. Jujutsu stores this executable bit within each commit +and will update it for files as you operate on a repository. If you update your +working copy to a commit where a file is recorded as executable or not, `jj` +will adjust the permission of that file. If you change a file's executable bit +through the filesystem, `jj` will record that change in the working-copy commit +on the next snapshot. + +Setting this to `true` will make Jujutsu ignore the executable bit on the +filesystem and only store the executable bit in the commit itself. In addition, +`jj` will not attempt to modify a file's executable bit and will add new files +as "not executable." This is already the behavior on Windows, and having the +option to enable this behavior is especially useful for Unix users dual-booting +Windows, Windows users accessing files from WSL, or anyone experimenting with +nonstandard filesystems. + +On Unix, Jujutsu will try to detect whether the filesystem supports executable +bits to choose a default value, but a configured value will always override the +default. On Windows systems there is no executable bit, and this option is not +used. + +You can always use `jj file chmod` to update the recorded executable bit for a +file in the commit manually. If this option is not set, `jj` will attempt to +propagate that change to the filesystem. + ## Snapshot settings ### Maximum size for new files diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 386a09ba69..a9119dbfd5 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -92,7 +92,9 @@ use crate::op_store::WorkspaceId; use crate::repo_path::RepoPath; use crate::repo_path::RepoPathBuf; use crate::repo_path::RepoPathComponent; +use crate::settings::ignore_executable_bit; use crate::settings::HumanByteSize; +use crate::settings::UserSettings; use crate::store::Store; use crate::tree::Tree; use crate::working_copy::CheckoutError; @@ -675,8 +677,9 @@ impl TreeState { store: Arc, working_copy_path: PathBuf, state_path: PathBuf, + exec_config: Option, ) -> Result { - let mut wc = TreeState::empty(store, working_copy_path, state_path, None); + let mut wc = TreeState::empty(store, working_copy_path, state_path, exec_config); wc.save()?; Ok(wc) } @@ -714,9 +717,7 @@ impl TreeState { let tree_state_path = state_path.join("tree_state"); let file = match File::open(&tree_state_path) { Err(ref err) if err.kind() == std::io::ErrorKind::NotFound => { - let mut wc = TreeState::empty(store, working_copy_path, state_path, exec_config); - wc.save()?; - return Ok(wc); + return TreeState::init(store, working_copy_path, state_path, exec_config); } Err(err) => { return Err(TreeStateError::ReadTreeState { @@ -1775,6 +1776,7 @@ impl LocalWorkingCopy { state_path: PathBuf, operation_id: OperationId, workspace_id: WorkspaceId, + settings: &UserSettings, ) -> Result { let proto = crate::protos::working_copy::Checkout { operation_id: operation_id.to_bytes(), @@ -1786,13 +1788,16 @@ impl LocalWorkingCopy { .open(state_path.join("checkout")) .unwrap(); file.write_all(&proto.encode_to_vec()).unwrap(); - let tree_state = - TreeState::init(store.clone(), working_copy_path.clone(), state_path.clone()).map_err( - |err| WorkingCopyStateError { - message: "Failed to initialize working copy state".to_string(), - err: err.into(), - }, - )?; + let tree_state = TreeState::init( + store.clone(), + working_copy_path.clone(), + state_path.clone(), + ignore_executable_bit(settings.config()), + ) + .map_err(|err| WorkingCopyStateError { + message: "Failed to initialize working copy state".to_string(), + err: err.into(), + })?; let ignore_exec = tree_state.ignore_exec; Ok(LocalWorkingCopy { store, @@ -1808,8 +1813,10 @@ impl LocalWorkingCopy { store: Arc, working_copy_path: PathBuf, state_path: PathBuf, + settings: &UserSettings, ) -> LocalWorkingCopy { - let ignore_exec = IgnoreExec::load_config(None, &working_copy_path); + let exec_config = ignore_executable_bit(settings.config()); + let ignore_exec = IgnoreExec::load_config(exec_config, &working_copy_path); LocalWorkingCopy { store, working_copy_path, @@ -1928,6 +1935,7 @@ impl WorkingCopyFactory for LocalWorkingCopyFactory { state_path: PathBuf, operation_id: OperationId, workspace_id: WorkspaceId, + settings: &UserSettings, ) -> Result, WorkingCopyStateError> { Ok(Box::new(LocalWorkingCopy::init( store, @@ -1935,6 +1943,7 @@ impl WorkingCopyFactory for LocalWorkingCopyFactory { state_path, operation_id, workspace_id, + settings, )?)) } @@ -1943,11 +1952,13 @@ impl WorkingCopyFactory for LocalWorkingCopyFactory { store: Arc, working_copy_path: PathBuf, state_path: PathBuf, + settings: &UserSettings, ) -> Result, WorkingCopyStateError> { Ok(Box::new(LocalWorkingCopy::load( store, working_copy_path, state_path, + settings, ))) } } diff --git a/lib/src/settings.rs b/lib/src/settings.rs index e787f0b825..04a9347b41 100644 --- a/lib/src/settings.rs +++ b/lib/src/settings.rs @@ -258,6 +258,17 @@ impl UserSettings { } } +/// Whether to ignore changes to the executable bit for files on Unix. On +/// Windows there is no executable bit and this config is unused. +/// +/// You can read more about this in the config documentation. +/// +/// This is not a method on UserSettings since it needs to be callable even when +/// only having the config itself (currently only used in `Ui::with_config`). +pub fn ignore_executable_bit(config: &config::Config) -> Option { + config.get_bool("core.ignore-executable-bit").ok() +} + /// This Rng uses interior mutability to allow generating random values using an /// immutable reference. It also fixes a specific seedable RNG for /// reproducibility. diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index cdd65247ff..030c3c6ba9 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -35,6 +35,7 @@ use crate::op_store::WorkspaceId; use crate::repo_path::RepoPath; use crate::repo_path::RepoPathBuf; use crate::settings::HumanByteSize; +use crate::settings::UserSettings; use crate::store::Store; /// The trait all working-copy implementations must implement. @@ -76,6 +77,7 @@ pub trait WorkingCopyFactory { state_path: PathBuf, operation_id: OperationId, workspace_id: WorkspaceId, + settings: &UserSettings, ) -> Result, WorkingCopyStateError>; /// Load an existing working copy. @@ -84,6 +86,7 @@ pub trait WorkingCopyFactory { store: Arc, working_copy_path: PathBuf, state_path: PathBuf, + settings: &UserSettings, ) -> Result, WorkingCopyStateError>; } diff --git a/lib/src/workspace.rs b/lib/src/workspace.rs index 7d03ca610f..58f60abc0c 100644 --- a/lib/src/workspace.rs +++ b/lib/src/workspace.rs @@ -142,6 +142,7 @@ fn init_working_copy( working_copy_state_path.clone(), repo.op_id().clone(), workspace_id, + user_settings, )?; let working_copy_type_path = working_copy_state_path.join("type"); fs::write(&working_copy_type_path, working_copy.name()).context(&working_copy_type_path)?; @@ -513,6 +514,7 @@ pub trait WorkspaceLoader { &self, store: &Arc, working_copy_factory: &dyn WorkingCopyFactory, + user_settings: &UserSettings, ) -> Result, WorkspaceLoadError>; } @@ -588,7 +590,8 @@ impl WorkspaceLoader for DefaultWorkspaceLoader { let repo_loader = RepoLoader::init_from_file_system(user_settings, &self.repo_path, store_factories)?; let working_copy_factory = get_working_copy_factory(self, working_copy_factories)?; - let working_copy = self.load_working_copy(repo_loader.store(), working_copy_factory)?; + let working_copy = + self.load_working_copy(repo_loader.store(), working_copy_factory, user_settings)?; let workspace = Workspace::new( &self.workspace_root, self.repo_path.clone(), @@ -606,11 +609,13 @@ impl WorkspaceLoader for DefaultWorkspaceLoader { &self, store: &Arc, working_copy_factory: &dyn WorkingCopyFactory, + user_settings: &UserSettings, ) -> Result, WorkspaceLoadError> { Ok(working_copy_factory.load_working_copy( store.clone(), self.workspace_root.to_owned(), self.working_copy_state_path.to_owned(), + user_settings, )?) } } diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index 0bf7d8cda6..b16ea7a24d 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/lib/tests/test_local_working_copy.rs @@ -608,8 +608,12 @@ fn test_checkout_discard() { // The change should be reflected in the working copy but not saved assert!(!file1_path.to_fs_path(&workspace_root).is_file()); assert!(file2_path.to_fs_path(&workspace_root).is_file()); - let reloaded_wc = - LocalWorkingCopy::load(store.clone(), workspace_root.clone(), state_path.clone()); + let reloaded_wc = LocalWorkingCopy::load( + store.clone(), + workspace_root.clone(), + state_path.clone(), + &testutils::user_settings(), + ); assert!(reloaded_wc.file_states().unwrap().contains_path(file1_path)); assert!(!reloaded_wc.file_states().unwrap().contains_path(file2_path)); drop(locked_ws); @@ -620,7 +624,12 @@ fn test_checkout_discard() { assert!(!wc.file_states().unwrap().contains_path(file2_path)); assert!(!file1_path.to_fs_path(&workspace_root).is_file()); assert!(file2_path.to_fs_path(&workspace_root).is_file()); - let reloaded_wc = LocalWorkingCopy::load(store.clone(), workspace_root, state_path); + let reloaded_wc = LocalWorkingCopy::load( + store.clone(), + workspace_root, + state_path, + &testutils::user_settings(), + ); assert!(reloaded_wc.file_states().unwrap().contains_path(file1_path)); assert!(!reloaded_wc.file_states().unwrap().contains_path(file2_path)); } diff --git a/lib/tests/test_local_working_copy_sparse.rs b/lib/tests/test_local_working_copy_sparse.rs index 6015e12b50..5a17f7ebf2 100644 --- a/lib/tests/test_local_working_copy_sparse.rs +++ b/lib/tests/test_local_working_copy_sparse.rs @@ -109,6 +109,7 @@ fn test_sparse_checkout() { repo.store().clone(), ws.workspace_root().to_path_buf(), wc.state_path().to_path_buf(), + &testutils::user_settings(), ); assert_eq!( wc.file_states().unwrap().paths().collect_vec(), From 3982b04d27f8ccff9c3de337db5bea9f06c68faf Mon Sep 17 00:00:00 2001 From: Ian Wrzesinski Date: Mon, 16 Sep 2024 09:58:03 -0400 Subject: [PATCH 3/3] diff_working_copies: Simplify tree checkout functions and update DiffSide enum. I got annoyed by the checkout function above in my previous commit and tried to compress the code a bit, and I think the result is much nicer. I'm not certain that a closure is the right solution here, but I think it becomes easier to reason about the two temp filenames that get generated. I initially changed Option to a bool, but found an enum more meaningful and more likely to be flexible in the future. I also moved the instruction writing to its own function. This makes the main `DiffEditWorkingCopies::check_out()` function is much nicer. J: This commit contains the following changes: --- cli/src/merge_tools/diff_working_copies.rs | 192 +++++++++------------ cli/src/merge_tools/external.rs | 22 ++- 2 files changed, 99 insertions(+), 115 deletions(-) diff --git a/cli/src/merge_tools/diff_working_copies.rs b/cli/src/merge_tools/diff_working_copies.rs index be2363178b..f349729de0 100644 --- a/cli/src/merge_tools/diff_working_copies.rs +++ b/cli/src/merge_tools/diff_working_copies.rs @@ -16,7 +16,6 @@ use jj_lib::matchers::EverythingMatcher; use jj_lib::matchers::Matcher; use jj_lib::merged_tree::MergedTree; use jj_lib::merged_tree::TreeDiffEntry; -use jj_lib::repo_path::RepoPathBuf; use jj_lib::store::Store; use jj_lib::working_copy::CheckoutError; use jj_lib::working_copy::SnapshotOptions; @@ -78,22 +77,6 @@ impl DiffWorkingCopies { } } -fn check_out( - store: Arc, - wc_dir: PathBuf, - state_dir: PathBuf, - tree: &MergedTree, - sparse_patterns: Vec, - exec_config: Option, -) -> Result { - std::fs::create_dir(&wc_dir).map_err(DiffCheckoutError::SetUpDir)?; - std::fs::create_dir(&state_dir).map_err(DiffCheckoutError::SetUpDir)?; - let mut tree_state = TreeState::init(store, wc_dir, state_dir, exec_config)?; - tree_state.set_sparse_patterns(sparse_patterns)?; - tree_state.check_out(tree)?; - Ok(tree_state) -} - pub(crate) fn new_utf8_temp_dir(prefix: &str) -> io::Result { let temp_dir = tempfile::Builder::new().prefix(prefix).tempdir()?; if temp_dir.path().to_str().is_none() { @@ -122,20 +105,24 @@ pub(crate) fn set_readonly_recursively(path: &Path) -> Result<(), std::io::Error } } +/// How to prepare tree states from the working copy for a diff viewer/editor. +/// +/// TwoWay: prepare a left and right tree; left is readonly. +/// ThreeWay: prepare left, right, and output trees; left + right are readonly. #[derive(Debug, Clone, Copy)] -pub(crate) enum DiffSide { - // Left, - Right, +pub(crate) enum DiffType { + TwoWay, + ThreeWay, } -/// Check out the two trees in temporary directories. Only include changed files -/// in the sparse checkout patterns. +/// Check out the two trees in temporary directories and make appropriate sides +/// readonly. Only include changed files in the sparse checkout patterns. pub(crate) fn check_out_trees( store: &Arc, left_tree: &MergedTree, right_tree: &MergedTree, matcher: &dyn Matcher, - output_is: Option, + diff_type: DiffType, exec_config: Option, ) -> Result { let changed_files: Vec<_> = left_tree @@ -145,43 +132,32 @@ pub(crate) fn check_out_trees( .block_on(); let temp_dir = new_utf8_temp_dir("jj-diff-").map_err(DiffCheckoutError::SetUpDir)?; - let left_wc_dir = temp_dir.path().join("left"); - let left_state_dir = temp_dir.path().join("left_state"); - let right_wc_dir = temp_dir.path().join("right"); - let right_state_dir = temp_dir.path().join("right_state"); - let left_tree_state = check_out( - store.clone(), - left_wc_dir, - left_state_dir, - left_tree, - changed_files.clone(), - exec_config, - )?; - let right_tree_state = check_out( - store.clone(), - right_wc_dir, - right_state_dir, - right_tree, - changed_files.clone(), - exec_config, - )?; - let output_tree_state = output_is - .map(|output_side| { - let output_wc_dir = temp_dir.path().join("output"); - let output_state_dir = temp_dir.path().join("output_state"); - check_out( - store.clone(), - output_wc_dir, - output_state_dir, - match output_side { - // DiffSide::Left => left_tree, - DiffSide::Right => right_tree, - }, - changed_files, - exec_config, - ) - }) - .transpose()?; + let temp_path = temp_dir.path(); + + let check_out = |name: &str, tree, files, read_only| -> Result { + let wc_dir = temp_path.join(name); + let state_dir = temp_path.join(format!("{}_state", name)); + std::fs::create_dir(&wc_dir).map_err(DiffCheckoutError::SetUpDir)?; + std::fs::create_dir(&state_dir).map_err(DiffCheckoutError::SetUpDir)?; + let mut tree_state = TreeState::init(store.clone(), wc_dir, state_dir, exec_config)?; + tree_state.set_sparse_patterns(files)?; + tree_state.check_out(tree)?; + if read_only { + set_readonly_recursively(tree_state.working_copy_path()) + .map_err(DiffCheckoutError::SetUpDir)?; + } + Ok(tree_state) + }; + + let left_tree_state = check_out("left", left_tree, changed_files.clone(), true)?; + let (right_tree_state, output_tree_state) = match diff_type { + DiffType::TwoWay => (check_out("right", right_tree, changed_files, false)?, None), + DiffType::ThreeWay => ( + check_out("right", right_tree, changed_files.clone(), true)?, + Some(check_out("output", right_tree, changed_files, false)?), + ), + }; + Ok(DiffWorkingCopies { _temp_dir: temp_dir, left_tree_state, @@ -203,7 +179,7 @@ impl DiffEditWorkingCopies { left_tree: &MergedTree, right_tree: &MergedTree, matcher: &dyn Matcher, - output_is: Option, + diff_type: DiffType, instructions: Option<&str>, exec_config: Option, ) -> Result { @@ -212,38 +188,42 @@ impl DiffEditWorkingCopies { left_tree, right_tree, matcher, - output_is, + diff_type, exec_config, )?; - let got_output_field = output_is.is_some(); + let instructions_path_to_cleanup = instructions + .map(|instructions| Self::write_edit_instructions(&diff_wc, instructions)) + .transpose()?; + Ok(Self { + working_copies: diff_wc, + instructions_path_to_cleanup, + }) + } - set_readonly_recursively(diff_wc.left_working_copy_path()) - .map_err(ExternalToolError::SetUpDir)?; - if got_output_field { - set_readonly_recursively(diff_wc.right_working_copy_path()) - .map_err(ExternalToolError::SetUpDir)?; - } - let output_wc_path = diff_wc - .output_working_copy_path() - .unwrap_or_else(|| diff_wc.right_working_copy_path()); + fn write_edit_instructions( + diff_wc: &DiffWorkingCopies, + instructions: &str, + ) -> Result { + let (right_wc_path, output_wc_path) = match diff_wc.output_working_copy_path() { + Some(output_path) => (Some(diff_wc.right_working_copy_path()), output_path), + None => (None, diff_wc.right_working_copy_path()), + }; let output_instructions_path = output_wc_path.join("JJ-INSTRUCTIONS"); // In the unlikely event that the file already exists, then the user will simply // not get any instructions. - let add_instructions = if !output_instructions_path.exists() { - instructions - } else { - None - }; - if let Some(instructions) = add_instructions { - let mut output_instructions_file = - File::create(&output_instructions_path).map_err(ExternalToolError::SetUpDir)?; - if diff_wc.right_working_copy_path() != output_wc_path { - let mut right_instructions_file = - File::create(diff_wc.right_working_copy_path().join("JJ-INSTRUCTIONS")) - .map_err(ExternalToolError::SetUpDir)?; - right_instructions_file - .write_all( - b"\ + if output_instructions_path.exists() { + return Ok(output_instructions_path); + } + let mut output_instructions_file = + File::create(&output_instructions_path).map_err(ExternalToolError::SetUpDir)?; + + // Write out our experimental three-way merge instructions first. + if let Some(right_wc_path) = right_wc_path { + let mut right_instructions_file = File::create(right_wc_path.join("JJ-INSTRUCTIONS")) + .map_err(ExternalToolError::SetUpDir)?; + right_instructions_file + .write_all( + b"\ The content of this pane should NOT be edited. Any edits will be lost. @@ -252,16 +232,16 @@ the following instructions may have been written with a 2-pane diff editing in mind and be a little inaccurate. ", - ) - .map_err(ExternalToolError::SetUpDir)?; - right_instructions_file - .write_all(instructions.as_bytes()) - .map_err(ExternalToolError::SetUpDir)?; - // Note that some diff tools might not show this message and delete the contents - // of the output dir instead. Meld does show this message. - output_instructions_file - .write_all( - b"\ + ) + .map_err(ExternalToolError::SetUpDir)?; + right_instructions_file + .write_all(instructions.as_bytes()) + .map_err(ExternalToolError::SetUpDir)?; + // Note that some diff tools might not show this message and delete the contents + // of the output dir instead. Meld does show this message. + output_instructions_file + .write_all( + b"\ Please make your edits in this pane. You are using the experimental 3-pane diff editor config. Some of @@ -269,18 +249,14 @@ the following instructions may have been written with a 2-pane diff editing in mind and be a little inaccurate. ", - ) - .map_err(ExternalToolError::SetUpDir)?; - } - output_instructions_file - .write_all(instructions.as_bytes()) + ) .map_err(ExternalToolError::SetUpDir)?; - }; - - Ok(Self { - working_copies: diff_wc, - instructions_path_to_cleanup: add_instructions.map(|_| output_instructions_path), - }) + } + // Now write the passed-in instructions. + output_instructions_file + .write_all(instructions.as_bytes()) + .map_err(ExternalToolError::SetUpDir)?; + Ok(output_instructions_path) } pub fn snapshot_results( diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index d8e9b7425b..e2f6940afb 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -27,7 +27,7 @@ use super::diff_working_copies::check_out_trees; use super::diff_working_copies::new_utf8_temp_dir; use super::diff_working_copies::set_readonly_recursively; use super::diff_working_copies::DiffEditWorkingCopies; -use super::diff_working_copies::DiffSide; +use super::diff_working_copies::DiffType; use super::ConflictResolveError; use super::DiffEditError; use super::DiffGenerateError; @@ -258,13 +258,18 @@ pub fn edit_diff_external( exec_config: Option, ) -> Result { let got_output_field = find_all_variables(&editor.edit_args).contains(&"output"); + let diff_type = if got_output_field { + DiffType::ThreeWay + } else { + DiffType::TwoWay + }; let store = left_tree.store(); let diffedit_wc = DiffEditWorkingCopies::check_out( store, left_tree, right_tree, matcher, - got_output_field.then_some(DiffSide::Right), + diff_type, instructions, exec_config, )?; @@ -298,11 +303,14 @@ pub fn generate_diff( tool: &ExternalMergeTool, ) -> Result<(), DiffGenerateError> { let store = left_tree.store(); - let diff_wc = check_out_trees(store, left_tree, right_tree, matcher, None, ui.exec_config)?; - set_readonly_recursively(diff_wc.left_working_copy_path()) - .map_err(ExternalToolError::SetUpDir)?; - set_readonly_recursively(diff_wc.right_working_copy_path()) - .map_err(ExternalToolError::SetUpDir)?; + let diff_wc = check_out_trees( + store, + left_tree, + right_tree, + matcher, + DiffType::TwoWay, + ui.exec_config, + )?; invoke_external_diff(ui, writer, tool, &diff_wc.to_command_variables()) }