From 76524dd6d38f5148600e3e9d26ad565f2e36bedc Mon Sep 17 00:00:00 2001 From: Ian Wrzesinski Date: Sat, 14 Sep 2024 17:17:31 -0400 Subject: [PATCH] Add config for reporting executable bit changes --- 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 | 20 ++ lib/src/file_util.rs | 27 ++ lib/src/local_working_copy.rs | 375 ++++++++++++++------ lib/src/settings.rs | 19 + lib/src/working_copy.rs | 2 + lib/src/workspace.rs | 10 +- lib/tests/test_local_working_copy.rs | 15 +- lib/tests/test_local_working_copy_sparse.rs | 1 + 15 files changed, 412 insertions(+), 115 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f753571657..bcaa7cdd9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,11 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * `jj op undo` now reports information on the operation that has been undone. +* Updated how we handle executable bits in the working copy to allow unix to + disable reporting executable bit changes if they're using a nonstandard + filesystem. We try to detect this on unix by default, but it can be overrident + manually by setting `core.report-executable-bit = false`. + ### Fixed bugs * Fixed panic when parsing invalid conflict markers of a particular form. diff --git a/cli/examples/custom-working-copy/main.rs b/cli/examples/custom-working-copy/main.rs index 431c5c7209..85f5d96e42 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, + config: &config::Config, ) -> Result { let inner = LocalWorkingCopy::init( store, @@ -126,6 +127,7 @@ impl ConflictsWorkingCopy { state_path, operation_id, workspace_id, + config, )?; 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, + config: &config::Config, + ) -> Self { + let inner = LocalWorkingCopy::load(store, working_copy_path.clone(), state_path, config); 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, + config: &config::Config, ) -> Result, WorkingCopyStateError> { Ok(Box::new(ConflictsWorkingCopy::init( store, @@ -193,6 +201,7 @@ impl WorkingCopyFactory for ConflictsWorkingCopyFactory { state_path, operation_id, workspace_id, + config, )?)) } @@ -201,11 +210,13 @@ impl WorkingCopyFactory for ConflictsWorkingCopyFactory { store: Arc, working_copy_path: PathBuf, state_path: PathBuf, + config: &config::Config, ) -> Result, WorkingCopyStateError> { Ok(Box::new(ConflictsWorkingCopy::load( store, working_copy_path, state_path, + config, ))) } } diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 997d62c829..084f3fe959 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." } } + }, + "report-executable-bit": { + "type": "boolean", + "description": "Whether to report changes to the executable bit for files on unix. This is ignored 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..195f88c993 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::report_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 = report_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..7009a9bb87 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::report_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: report_executable_bit(config), }) } diff --git a/docs/config.md b/docs/config.md index 1c1ae2aef6..3ef12688fd 100644 --- a/docs/config.md +++ b/docs/config.md @@ -951,6 +951,26 @@ snapshots on filestem changes by setting You can check whether Watchman is enabled and whether it is installed correctly using `jj debug watchman status`. +## Report executable bit + +`core.report-executable-bit = true | false` + +Whether to report changes to the executable bit for files on unix. On windows +there is no executable bit and this config is completely ignored. + +On unix we determine the default value by querying the filesystem as to whether +executable bits are supported. If they are, we will report changes to files' +executable bit and update the repository. Otherwise we will only update files if +they are manually changed via `jj file chmod`. + +On unix setting this to `false` will disable executable bit reporting and +setting it to `true` will enable it regardless of our filesystem check. This can +be useful for users dual-booting windows or experimenting with nonstandard +filesystems. + +You can manually update an executable bit with `jj file chmod x ` or +`jj file chmod x `. + ## Snapshot settings ### Maximum size for new files diff --git a/lib/src/file_util.rs b/lib/src/file_util.rs index 12c83afe9c..a9e345613f 100644 --- a/lib/src/file_util.rs +++ b/lib/src/file_util.rs @@ -165,9 +165,36 @@ 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; + + 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 { diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 1e5398f408..2b1eec56a1 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")] @@ -90,6 +92,7 @@ use crate::op_store::WorkspaceId; use crate::repo_path::RepoPath; use crate::repo_path::RepoPathBuf; use crate::repo_path::RepoPathComponent; +use crate::settings::report_executable_bit; use crate::settings::HumanByteSize; use crate::store::Store; use crate::tree::Tree; @@ -104,19 +107,130 @@ use crate::working_copy::WorkingCopy; use crate::working_copy::WorkingCopyFactory; use crate::working_copy::WorkingCopyStateError; +/// None means we are ignoring the executable bit and don't care what it is. +/// +/// On Windows there is no executable bit, so this will always be None. On Unix +/// it will usually be Some(_), but may be disabled by a config value for users +/// with non-standard filesystems or who use a nonstandard filesystem on unix. +/// +/// This represents *whether* a file is executable, but not whether that state +/// is to be reported. +#[derive(Debug, Clone, Copy)] +pub enum ExecFlag { + Exec(bool), + DontCare, +} +// Note: cannot derive Eq or PartialEq since a == b == c does not imply a == c. +// e.g. Exec(true) == DontCare == Exec(false) but Exec(true) != Exec(false) + +impl Default for ExecFlag { + fn default() -> Self { + #[cfg(unix)] + let exec_flag = ExecFlag::Exec(false); + #[cfg(windows)] + let exec_flag = ExecFlag::DontCare; + exec_flag + } +} + +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 None. + _ => true, + } + } + + /// Create a bool in an environment where we can't check a ReportExec value. + fn from_bool_unchecked(executable: bool) -> Self { + #[cfg(unix)] + let exec_flag = ExecFlag::Exec(executable); + #[cfg(windows)] + let exec_flag = ExecFlag::DontCare; + exec_flag + } +} + +/// Whether to report the executable bit when comparing files. The executable +/// state is not reported on windows, but may be reported on unix depending on +/// the filesystem and user configuration. #[cfg(unix)] -type FileExecutableFlag = bool; +#[derive(Debug, Clone, Copy)] +struct ReportExec(bool); #[cfg(windows)] -type FileExecutableFlag = (); +#[derive(Debug, Clone, Copy)] +struct ReportExec(); + +impl ReportExec { + /// 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 { + // TODO: update comments everywhere that we are checking now. + #[cfg(unix)] // check for executable support on Unix. + let report_exec = + ReportExec(exec_config.unwrap_or_else(|| check_executable_bit_support(wc_path))); + #[cfg(windows)] + let (report_exec, _) = (ReportExec(), exec_config); // use the variable + report_exec + } -#[derive(Debug, PartialEq, Eq, Clone)] + /// Push back into an Option config value for roundtripping. + fn as_config(self) -> Option { + // TODO: update comments everywhere that we are checking now. + #[cfg(unix)] // check for executable support on Unix. + let exec_config = Some(self.0); + #[cfg(windows)] + let (exec_config, _) = (None, self); // use the variable + exec_config + } + + /// Maybe report the executable bit as a flag for the FileType. + fn report_exec_bit bool>(self, is_executable: F) -> ExecFlag { + #[cfg(unix)] + let exec_flag = if self.0 { + // report the bit only if we care. + ExecFlag::Exec(is_executable()) + } else { + // don't report the bit (emulating windows) + ExecFlag::DontCare + }; + #[cfg(windows)] + let (exec_flag, _, _) = (ExecFlag::DontCare, 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) { + (true, 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::default(), + } + } +} + +#[derive(Debug, Clone)] pub struct FileState { pub file_type: FileType, pub mtime: MillisSinceEpoch, @@ -130,30 +244,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 report_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 +290,7 @@ impl FileState { } } + /// Filestate for a git submodule. fn for_gitsubmodule() -> Self { FileState { file_type: FileType::GitSubmodule, @@ -334,6 +460,7 @@ pub struct TreeState { sparse_patterns: Vec, own_mtime: MillisSinceEpoch, symlink_support: bool, + report_exec: ReportExec, /// 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 +471,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 +491,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,21 +601,19 @@ fn mtime_from_metadata(metadata: &Metadata) -> MillisSinceEpoch { ) } -fn file_state(metadata: &Metadata) -> Option { +fn file_state(metadata: &Metadata, report_exec: ReportExec) -> Option { let metadata_file_type = metadata.file_type(); let file_type = if metadata_file_type.is_dir() { None } else if metadata_file_type.is_symlink() { Some(FileType::Symlink) } else if metadata_file_type.is_file() { + // TODO: here #[cfg(unix)] - if metadata.permissions().mode() & 0o111 != 0 { - Some(FileType::Normal { executable: true }) - } else { - Some(FileType::Normal { executable: false }) - } + let exec_flag = report_exec.report_exec_bit(|| metadata.permissions().mode() & 0o111 != 0); #[cfg(windows)] - Some(FileType::Normal { executable: () }) + let exec_flag = ExecFlag::DontCare; + Some(FileType::Normal { exec_flag }) } else { None }; @@ -571,14 +691,21 @@ 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); + let mut wc = TreeState::empty(store, working_copy_path, state_path, exec_config); wc.save()?; Ok(wc) } - fn empty(store: Arc, working_copy_path: PathBuf, state_path: PathBuf) -> TreeState { + fn empty( + store: Arc, + working_copy_path: PathBuf, + state_path: PathBuf, + exec_config: Option, + ) -> TreeState { let tree_id = store.empty_merged_tree_id(); + let report_exec = ReportExec::load_config(exec_config, &working_copy_path); TreeState { store, working_copy_path, @@ -589,6 +716,7 @@ impl TreeState { own_mtime: MillisSinceEpoch(0), symlink_support: check_symlink_support().unwrap_or(false), watchman_clock: None, + report_exec, } } @@ -596,11 +724,12 @@ impl TreeState { 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); + return TreeState::init(store, working_copy_path, state_path, exec_config); } Err(err) => { return Err(TreeStateError::ReadTreeState { @@ -611,7 +740,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) } @@ -855,7 +984,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 +1089,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 +1124,7 @@ impl TreeState { }); } }; - if let Some(new_file_state) = file_state(&metadata) { + if let Some(new_file_state) = file_state(&metadata, self.report_exec) { present_files_tx.send(tracked_path.to_owned()).ok(); let update = self.get_updated_tree_value( tracked_path, @@ -1008,7 +1138,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 +1192,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.report_exec) { present_files_tx.send(path.clone()).ok(); let update = self.get_updated_tree_value( &path, @@ -1074,7 +1204,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 +1280,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 +1300,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 +1324,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.report_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 +1352,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.report_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 +1397,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 +1405,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 +1444,33 @@ 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> { - #[cfg(unix)] - { - 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))?; + /// 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.report_exec.report_exec_bit(|| executable); + match exec_flag { + ExecFlag::Exec(executable) => { + let mode = if executable { 0o755 } else { 0o644 }; + #[cfg(unix)] + fs::set_permissions(disk_path, fs::Permissions::from_mode(mode)) + .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; + #[cfg(windows)] + let _ = mode; // use the variable + } + ExecFlag::DontCare => {} } - Ok(()) + Ok(exec_flag) } pub fn check_out(&mut self, new_tree: &MergedTree) -> Result { @@ -1513,10 +1652,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.report_exec.report_exec_bit(|| executable), + }, TreeValue::Symlink(_id) => FileType::Symlink, TreeValue::Conflict(_id) => { panic!("unexpected conflict entry in diff at {path:?}"); @@ -1529,11 +1667,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.report_exec.report_exec_bit(|| *executable); + file_type = FileType::Normal { exec_flag }; + } } + file_type } }; let file_state = FileState { @@ -1577,6 +1721,7 @@ pub struct LocalWorkingCopy { state_path: PathBuf, checkout_state: OnceCell, tree_state: OnceCell, + report_exec: ReportExec, } impl WorkingCopy for LocalWorkingCopy { @@ -1617,6 +1762,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(), + report_exec: self.report_exec, }; let old_operation_id = wc.operation_id().clone(); let old_tree_id = wc.tree_id()?.clone(); @@ -1644,6 +1790,7 @@ impl LocalWorkingCopy { state_path: PathBuf, operation_id: OperationId, workspace_id: WorkspaceId, + config: &config::Config, ) -> Result { let proto = crate::protos::working_copy::Checkout { operation_id: operation_id.to_bytes(), @@ -1655,19 +1802,24 @@ 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(), + report_executable_bit(config), + ) + .map_err(|err| WorkingCopyStateError { + message: "Failed to initialize working copy state".to_string(), + err: err.into(), + })?; + let report_exec = tree_state.report_exec; Ok(LocalWorkingCopy { store, working_copy_path, state_path, checkout_state: OnceCell::new(), tree_state: OnceCell::with_value(tree_state), + report_exec, }) } @@ -1675,13 +1827,17 @@ impl LocalWorkingCopy { store: Arc, working_copy_path: PathBuf, state_path: PathBuf, + config: &config::Config, ) -> LocalWorkingCopy { + let exec_config = report_executable_bit(config); + let report_exec = ReportExec::load_config(exec_config, &working_copy_path); LocalWorkingCopy { store, working_copy_path, state_path, checkout_state: OnceCell::new(), tree_state: OnceCell::new(), + report_exec, } } @@ -1730,6 +1886,7 @@ impl LocalWorkingCopy { self.store.clone(), self.working_copy_path.clone(), self.state_path.clone(), + self.report_exec.as_config(), ) }) .map_err(|err| WorkingCopyStateError { @@ -1792,6 +1949,7 @@ impl WorkingCopyFactory for LocalWorkingCopyFactory { state_path: PathBuf, operation_id: OperationId, workspace_id: WorkspaceId, + config: &config::Config, ) -> Result, WorkingCopyStateError> { Ok(Box::new(LocalWorkingCopy::init( store, @@ -1799,6 +1957,7 @@ impl WorkingCopyFactory for LocalWorkingCopyFactory { state_path, operation_id, workspace_id, + config, )?)) } @@ -1807,11 +1966,13 @@ impl WorkingCopyFactory for LocalWorkingCopyFactory { store: Arc, working_copy_path: PathBuf, state_path: PathBuf, + config: &config::Config, ) -> Result, WorkingCopyStateError> { Ok(Box::new(LocalWorkingCopy::load( store, working_copy_path, state_path, + config, ))) } } @@ -1970,12 +2131,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, }; @@ -2021,9 +2188,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, }; @@ -2081,4 +2246,6 @@ mod tests { assert_eq!(file_states.get(repo_path("bc")), Some(new_state(5))); assert_eq!(file_states.get(repo_path("z")), None); } + + // TODO: Add new tests for `report_exec` states. } diff --git a/lib/src/settings.rs b/lib/src/settings.rs index e3cffb5f1f..575e878897 100644 --- a/lib/src/settings.rs +++ b/lib/src/settings.rs @@ -258,6 +258,25 @@ impl UserSettings { } } +/// Whether to report changes to the executable bit for files on unix. On +/// windows there is no executable bit and this config is completely ignored. +/// +/// On unix we determine the default value by querying the filesystem as to +/// whether executable bits are supported. If they are, we will report changes +/// to files' executable bit and update the repository. Otherwise we will only +/// update files if they are manually changed via `jj file chmod`. +/// +/// On unix setting this to `false` will disable executable bit reporting and +/// setting it to `true` will enable it regardless of our filesystem check. This +/// can be useful for users dual-booting windows or experimenting with +/// nonstandard filesystems. +/// +/// 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 report_executable_bit(config: &config::Config) -> Option { + config.get_bool("core.report-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 f038e884c8..33b702cc97 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -76,6 +76,7 @@ pub trait WorkingCopyFactory { state_path: PathBuf, operation_id: OperationId, workspace_id: WorkspaceId, + config: &config::Config, ) -> Result, WorkingCopyStateError>; /// Load an existing working copy. @@ -84,6 +85,7 @@ pub trait WorkingCopyFactory { store: Arc, working_copy_path: PathBuf, state_path: PathBuf, + config: &config::Config, ) -> Result, WorkingCopyStateError>; } diff --git a/lib/src/workspace.rs b/lib/src/workspace.rs index 7d03ca610f..8f51e1a2b4 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.config(), )?; 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, + config: &config::Config, ) -> Result, WorkspaceLoadError>; } @@ -588,7 +590,11 @@ 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.config(), + )?; let workspace = Workspace::new( &self.workspace_root, self.repo_path.clone(), @@ -606,11 +612,13 @@ impl WorkspaceLoader for DefaultWorkspaceLoader { &self, store: &Arc, working_copy_factory: &dyn WorkingCopyFactory, + config: &config::Config, ) -> Result, WorkspaceLoadError> { Ok(working_copy_factory.load_working_copy( store.clone(), self.workspace_root.to_owned(), self.working_copy_state_path.to_owned(), + config, )?) } } diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index 0bf7d8cda6..1cf58baf2d 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().config(), + ); 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::base_config().build().unwrap(), + ); 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..0371261672 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::base_config().build().unwrap(), ); assert_eq!( wc.file_states().unwrap().paths().collect_vec(),