diff --git a/CHANGELOG.md b/CHANGELOG.md index a40b565df1e..fd48038c2fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,11 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). colocation state (`status`) and to convert a non-colocated git repo into a colocated repo (`enable`) and vice-versa `disable`. +* Updated the executable bit representation in the local working copy to allow + ignoring executable bit changes on Unix. By default we try to detect the + filesystem's behavior, but this can be overridden manually by setting + `working-copy.exec-bit-change = "respect" | "ignore"`. + ### Fixed bugs * `jj metaedit --author-timestamp` twice with the same value no longer diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index cf473dcd2fc..2822e0bd4d1 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -936,6 +936,16 @@ "none" ], "default": "none" + }, + "exec-bit-change": { + "type": "string", + "description": "Whether to respect changes to executable bits on Unix. This is unused on Windows.", + "enum": [ + "respect", + "ignore", + "auto" + ], + "default": "auto" } } }, diff --git a/cli/src/merge_tools/diff_working_copies.rs b/cli/src/merge_tools/diff_working_copies.rs index 9bc0246e801..74488c314b0 100644 --- a/cli/src/merge_tools/diff_working_copies.rs +++ b/cli/src/merge_tools/diff_working_copies.rs @@ -12,6 +12,7 @@ use jj_lib::conflicts::ConflictMarkerStyle; use jj_lib::fsmonitor::FsmonitorSettings; use jj_lib::gitignore::GitIgnoreFile; use jj_lib::local_working_copy::EolConversionMode; +use jj_lib::local_working_copy::ExecChangeSetting; use jj_lib::local_working_copy::TreeState; use jj_lib::local_working_copy::TreeStateError; use jj_lib::local_working_copy::TreeStateSettings; @@ -152,6 +153,7 @@ pub(crate) fn check_out_trees( let tree_state_settings = TreeStateSettings { conflict_marker_style, eol_conversion_mode: EolConversionMode::None, + exec_change_setting: ExecChangeSetting::Auto, fsmonitor_settings: FsmonitorSettings::None, }; let mut state = TreeState::init(store.clone(), wc_path, state_dir, &tree_state_settings)?; diff --git a/cli/tests/test_file_chmod_command.rs b/cli/tests/test_file_chmod_command.rs index 6861efdb63b..a90264e9a2c 100644 --- a/cli/tests/test_file_chmod_command.rs +++ b/cli/tests/test_file_chmod_command.rs @@ -12,11 +12,39 @@ // See the License for the specific language governing permissions and // limitations under the License. +#[cfg(unix)] +use std::os::unix::fs::PermissionsExt; +#[cfg(unix)] +use std::path::Path; + +#[cfg(unix)] +use regex::Regex; + use crate::common::CommandOutput; use crate::common::TestEnvironment; use crate::common::TestWorkDir; use crate::common::create_commit_with_files; +/// Assert that a file's executable bit matches the expected value. +#[cfg(unix)] +#[track_caller] +fn assert_file_executable(path: &Path, expected: bool) { + let perms = path.metadata().unwrap().permissions(); + let actual = (perms.mode() & 0o100) == 0o100; + assert_eq!(actual, expected); +} + +/// Set the executable bit of a file on the filesystem. +#[cfg(unix)] +#[track_caller] +pub fn set_file_executable(path: &Path, executable: bool) { + let prev_mode = path.metadata().unwrap().permissions().mode(); + let is_executable = prev_mode & 0o100 != 0; + assert_ne!(executable, is_executable, "why are you calling this?"); + let new_mode = if executable { 0o755 } else { 0o644 }; + std::fs::set_permissions(path, PermissionsExt::from_mode(new_mode)).unwrap(); +} + #[must_use] fn get_log_output(work_dir: &TestWorkDir) -> CommandOutput { work_dir.run_jj(["log", "-T", "bookmarks"]) @@ -228,3 +256,184 @@ fn test_chmod_file_dir_deletion_conflicts() { [EOF] "); } + +#[cfg(unix)] +#[test] +fn test_chmod_exec_bit_settings() { + let test_env = TestEnvironment::default(); + test_env.run_jj_in(".", ["git", "init", "repo"]).success(); + let work_dir = test_env.work_dir("repo"); + let path = &work_dir.root().join("file"); + + // The timestamps in the `jj debug local-working-copy` output change, so we want + // to remove them before asserting the snapshot + let timestamp_regex = Regex::new(r"\b\d{10,}\b").unwrap(); + let redact_timestamp = |output: String| { + let output = timestamp_regex.replace_all(&output, ""); + output.into_owned() + }; + + // Load with an explicit "auto" value to test the deserialization. + test_env.add_config(r#"working-copy.exec-bit-change = "auto""#); + create_commit_with_files(&work_dir, "base", &[], &[("file", "base\n")]); + + let output = work_dir.run_jj(["debug", "local-working-copy"]); + insta::assert_snapshot!(output.normalize_stdout_with(redact_timestamp), @r#" + Current operation: OperationId("8c58a72d1118aa7d8b1295949a7fa8c6fcda63a3c89813faf2b8ca599ceebf8adcfcbeb8f0bbb6439c86b47dd68b9cf85074c9e57214c3fb4b632e0c9e87ad65") + Current tree: MergedTreeId { tree_ids: Resolved(TreeId("6d5f482d15035cdd7733b1b551d1fead28d22592")) } + Normal { exec_bit: ExecBit(false) } 5 None "file" + [EOF] + "#); // in-repo: false, on-disk: false (1/4) + + // 1. Start respecting the executable bit + test_env.add_config(r#"working-copy.exec-bit-change = "respect""#); + create_commit_with_files(&work_dir, "respect", &["base"], &[]); + + set_file_executable(path, true); + let output = work_dir.run_jj(["debug", "local-working-copy"]); + insta::assert_snapshot!(output.normalize_stdout_with(redact_timestamp), @r#" + Current operation: OperationId("3a6a78820e6892164ed55680b92fa679fbb4d6acd4135c7413d1b815bedcd2c24c85ac8f4f96c96f76012f33d31ffbf50473b938feadf36fcd9c92997789aeca") + Current tree: MergedTreeId { tree_ids: Resolved(TreeId("5201dbafb66dc1b28b029a262e1b206f6f93df1e")) } + Normal { exec_bit: ExecBit(true) } 5 None "file" + [EOF] + "#); // in-repo: true, on-disk: true (2/4) + + work_dir.run_jj(["file", "chmod", "n", "file"]).success(); + assert_file_executable(path, false); + + work_dir.run_jj(["file", "chmod", "x", "file"]).success(); + assert_file_executable(path, true); + + // 2. Now ignore the executable bit + create_commit_with_files(&work_dir, "ignore", &["base"], &[]); + test_env.add_config(r#"working-copy.exec-bit-change = "ignore""#); + set_file_executable(path, true); + + // chmod should affect the repo state, but not the on-disk file. + work_dir.run_jj(["file", "chmod", "n", "file"]).success(); + assert_file_executable(path, true); + let output = work_dir.run_jj(["debug", "local-working-copy"]); + insta::assert_snapshot!(output.normalize_stdout_with(redact_timestamp), @r#" + Current operation: OperationId("cab1801e16b54d5b413f638bdf74388520b51232c88db6b314ef64b054607ab82ae6ef0b1f707b52aa8d2131511f6f48f8ca52e465621ff38c442b0ec893f309") + Current tree: MergedTreeId { tree_ids: Resolved(TreeId("6d5f482d15035cdd7733b1b551d1fead28d22592")) } + Normal { exec_bit: ExecBit(true) } 5 None "file" + [EOF] + "#); // in-repo: false, on-disk: true (3/4) + + set_file_executable(path, false); + work_dir.run_jj(["file", "chmod", "x", "file"]).success(); + assert_file_executable(path, false); + let output = work_dir.run_jj(["debug", "local-working-copy"]); + insta::assert_snapshot!(output.normalize_stdout_with(redact_timestamp), @r#" + Current operation: OperationId("def8ce6211dcff6d2784d5309d36079c1cb6eeb70821ae144982c76d38ed76fedc8b84e4daddaac70f6a0aae1c301ff5b60e1baa6ac371dabd77cec3537d2c39") + Current tree: MergedTreeId { tree_ids: Resolved(TreeId("5201dbafb66dc1b28b029a262e1b206f6f93df1e")) } + Normal { exec_bit: ExecBit(false) } 5 None "file" + [EOF] + "#); // in-repo: true, on-disk: false (4/4) Yay! We've observed all possible states! + + // 3. Go back to respecting the executable bit + test_env.add_config(r#"working-copy.exec-bit-change = "respect""#); + assert_file_executable(path, false); + let output = work_dir.run_jj(["status"]); // TODO: broken + insta::assert_snapshot!(output, @r#" + The working copy has no changes. + Working copy (@) : znkkpsqq edd97391 ignore | (empty) ignore + Parent commit (@-): rlvkpnrz 1792382a base | base + [EOF] + "#); + let output = work_dir.run_jj(["file", "chmod", "x", "file"]); + insta::assert_snapshot!(output, @r#" + ------- stderr ------- + Working copy (@) now at: znkkpsqq 8ea2b9a1 ignore | ignore + Parent commit (@-) : rlvkpnrz 1792382a base | base + Added 0 files, modified 1 files, removed 0 files + [EOF] + "#); + assert_file_executable(path, true); + + work_dir.run_jj(["new", "base"]).success(); + set_file_executable(path, true); + let output = work_dir.run_jj(["debug", "local-working-copy"]); + insta::assert_snapshot!(output.normalize_stdout_with(redact_timestamp), @r#" + Current operation: OperationId("7babf6123ae6644e9f8cc702464da6c4bfcbfa002afc8c0d77e301ff8971a0c485b1fdd4b6c59bcf5039db2e54126f9c84c03ede489a0c8bb42a3e7f94df6bd9") + Current tree: MergedTreeId { tree_ids: Resolved(TreeId("5201dbafb66dc1b28b029a262e1b206f6f93df1e")) } + Normal { exec_bit: ExecBit(true) } 5 None "file" + [EOF] + "#); +} + +#[cfg(unix)] +#[test] +fn test_chmod_exec_bit_ignore() { + let test_env = TestEnvironment::default(); + test_env.run_jj_in(".", ["git", "init", "repo"]).success(); + let work_dir = test_env.work_dir("repo"); + let path = &work_dir.root().join("file"); + + test_env.add_config(r#"working-copy.exec-bit-change = "ignore""#); + + create_commit_with_files(&work_dir, "base", &[], &[("file", "base\n")]); + assert_file_executable(path, false); + + // 1. Reverting to "in-repo: true, on-disk: false" works. + create_commit_with_files(&work_dir, "repo-x-disk-n", &["base"], &[]); + work_dir.run_jj(["file", "chmod", "x", "file"]).success(); + assert_file_executable(path, false); + + // Commit, update the file, then reset the file. + work_dir.run_jj(["new"]).success(); + work_dir.write_file(path, "something"); + work_dir.run_jj(["abandon"]).success(); + // The on-disk exec bit should remain false. + assert_file_executable(path, false); + + // 2. Reverting to "in-repo: false, on-disk: true" works. + create_commit_with_files(&work_dir, "repo-n-disk-x", &["base"], &[]); + set_file_executable(path, true); + work_dir.run_jj(["file", "chmod", "n", "file"]).success(); + assert_file_executable(path, true); + + // Commit, update the file, then reset the file. + work_dir.run_jj(["new"]).success(); + work_dir.write_file(path, "something"); + work_dir.run_jj(["abandon"]).success(); + // The on-disk exec bit should remain true. + assert_file_executable(path, true); +} + +#[cfg(unix)] +#[test] +fn test_chmod_exec_bit_ignore_then_respect() { + let test_env = TestEnvironment::default(); + test_env.run_jj_in(".", ["git", "init", "repo"]).success(); + let work_dir = test_env.work_dir("repo"); + let path = &work_dir.root().join("file"); + + // Start while ignoring executable bits. + test_env.add_config(r#"working-copy.exec-bit-change = "ignore""#); + create_commit_with_files(&work_dir, "base", &[], &[("file", "base\n")]); + + // Set the in-repo executable bit to true. + let output = work_dir.run_jj(["file", "chmod", "x", "file"]); + insta::assert_snapshot!(output, @r#" + ------- stderr ------- + Working copy (@) now at: rlvkpnrz cb3f99cb base | base + Parent commit (@-) : zzzzzzzz 00000000 (empty) (no description set) + Added 0 files, modified 1 files, removed 0 files + [EOF] + "#); + assert_file_executable(path, false); + + test_env.add_config(r#"working-copy.exec-bit-change = "respect""#); + + // This simultaneously snapshots and updates the executable bit. + let output = work_dir.run_jj(["file", "chmod", "x", "file"]); + insta::assert_snapshot!(output, @r#" + ------- stderr ------- + Working copy (@) now at: rlvkpnrz f10cb843 base | base + Parent commit (@-) : zzzzzzzz 00000000 (empty) (no description set) + Added 0 files, modified 1 files, removed 0 files + [EOF] + "#); + assert_file_executable(path, true); +} diff --git a/cli/tests/test_working_copy.rs b/cli/tests/test_working_copy.rs index 90d78fb06e4..fa039385629 100644 --- a/cli/tests/test_working_copy.rs +++ b/cli/tests/test_working_copy.rs @@ -350,13 +350,8 @@ fn test_conflict_marker_length_stored_in_working_copy() { // The timestamps in the `jj debug local-working-copy` output change, so we want // to remove them before asserting the snapshot let timestamp_regex = Regex::new(r"\b\d{10,}\b").unwrap(); - // On Windows, executable is always `()`, but on Unix-like systems, it's `true` - // or `false`, so we want to remove it from the output as well - let executable_regex = Regex::new("executable: [^ ]+").unwrap(); - let redact_output = |output: String| { let output = timestamp_regex.replace_all(&output, ""); - let output = executable_regex.replace_all(&output, ""); output.into_owned() }; @@ -365,7 +360,7 @@ fn test_conflict_marker_length_stored_in_working_copy() { insta::assert_snapshot!(output.normalize_stdout_with(redact_output), @r#" Current operation: OperationId("da3b34243efe5ea04830cd2211b5be79444fbc2ef23681361fd2f551ebb86772bff21695da95b72388306e028bf04c6d76db10bf4cbd3a08eb34bf744c8900c7") Current tree: MergedTreeId { tree_ids: Conflicted([TreeId("381273b50cf73f8c81b3f1502ee89e9bbd6c1518"), TreeId("771f3d31c4588ea40a8864b2a981749888e596c2"), TreeId("f56b8223da0dab22b03b8323ced4946329aeb4e0")]) } - Normal { } 249 Some(MaterializedConflictData { conflict_marker_len: 11 }) "file" + Normal { exec_bit: ExecBit(false) } 249 Some(MaterializedConflictData { conflict_marker_len: 11 }) "file" [EOF] "#); @@ -428,7 +423,7 @@ fn test_conflict_marker_length_stored_in_working_copy() { insta::assert_snapshot!(output.normalize_stdout_with(redact_output), @r#" Current operation: OperationId("3de33bbfe3a9df8a052cc243aeedac6a3240d6115cb88f2779a1b6f1289288c6e78153875e48e41c17c098418f681bc872c54743e76b9e210f08533c50fc5a26") Current tree: MergedTreeId { tree_ids: Conflicted([TreeId("381273b50cf73f8c81b3f1502ee89e9bbd6c1518"), TreeId("771f3d31c4588ea40a8864b2a981749888e596c2"), TreeId("3329c18c95f7b7a55c278c2259e9c4ce711fae59")]) } - Normal { } 289 Some(MaterializedConflictData { conflict_marker_len: 11 }) "file" + Normal { exec_bit: ExecBit(false) } 289 Some(MaterializedConflictData { conflict_marker_len: 11 }) "file" [EOF] "#); @@ -463,7 +458,7 @@ fn test_conflict_marker_length_stored_in_working_copy() { insta::assert_snapshot!(output.normalize_stdout_with(redact_output), @r#" Current operation: OperationId("2676b66a8d17cf7913d2260285abe6f3ca4c8dc8f3fdfb3f54a4d566c9199670f80123a7174b553ff67c13c20c6827cde2429847a7949c19bc52f2397139e4c9") Current tree: MergedTreeId { tree_ids: Resolved(TreeId("6120567b3cb2472d549753ed3e4b84183d52a650")) } - Normal { } 130 None "file" + Normal { exec_bit: ExecBit(false) } 130 None "file" [EOF] "#); } diff --git a/docs/config.md b/docs/config.md index b0bb0abcda7..426ab575852 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1674,6 +1674,39 @@ large repositories, this may cause `watchman` to fail and commands like `jj status` to take longer than expected. If you experience this run `jj debug watchman status` and tune your `inotify` limits. +## Respect or ignore executable bit permission changes + +Whether to respect or ignore changes to the executable bit for files on Unix. +This option is unused on Windows. + +`working-copy.exec-bit-change = "respect" | "ignore" | "auto" (default)` + +On Unix systems, files have a permission bit for whether they are executable as +scripts or binary code. Jujutsu stores this state in the repository and will +update it for files as you operate on a repository. If you set 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 when taking a snapshot. + +Setting this to `"ignore"` will make Jujutsu ignore the executable bit on the +filesystem when updating the state for the repository. 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 useful for Unix users dual-booting Windows, +Windows users accessing files from WSL, or anyone experimenting with other +filesystem configurations. + +On Unix if `"auto"` is set (the default), `jj` will try to detect whether the +filesystem supports changing executable bits to choose between `"respect"` or +`"ignore"`. On errors, we assume `"respect"`, except if permission was denied +which will assume `"ignore"`. + +On Windows, files have no executable bit so this option is unused. + +You can always use `jj file chmod` to update the recorded executable bit for a +file manually. If this option is `"respect"`, `jj` will also attempt to +propagate that change to the filesystem. + ## Snapshot settings ### Paths to automatically track diff --git a/lib/src/config/misc.toml b/lib/src/config/misc.toml index 5e7a1b03307..fefb5a7f126 100644 --- a/lib/src/config/misc.toml +++ b/lib/src/config/misc.toml @@ -52,3 +52,4 @@ name = "" [working-copy] eol-conversion = "none" +exec-bit-change = "auto" diff --git a/lib/src/file_util.rs b/lib/src/file_util.rs index 683a996d7c3..936795eda6f 100644 --- a/lib/src/file_util.rs +++ b/lib/src/file_util.rs @@ -34,6 +34,8 @@ use tokio::io::AsyncRead; use tokio::io::AsyncReadExt as _; use tokio::io::ReadBuf; +#[cfg(unix)] +pub use self::platform::check_executable_bit_support; pub use self::platform::check_symlink_support; pub use self::platform::try_symlink; @@ -294,6 +296,7 @@ mod platform { use std::ffi::OsStr; use std::io; use std::os::unix::ffi::OsStrExt as _; + use std::os::unix::fs::PermissionsExt; use std::os::unix::fs::symlink; use std::path::Path; @@ -307,7 +310,27 @@ mod platform { Ok(data.as_bytes()) } - /// Symlinks are always available on UNIX + /// Whether changing executable bits is permitted on the filesystem of this + /// directory, and whether attempting to flip one has an observable effect. + pub fn check_executable_bit_support(path: impl AsRef) -> io::Result { + // Get current permissions and try to flip just the user's executable bit. + let temp_file = tempfile::tempfile_in(path)?; + let old_mode = temp_file.metadata()?.permissions().mode(); + let new_mode = old_mode ^ 0o100; + let result = temp_file.set_permissions(PermissionsExt::from_mode(new_mode)); + match result { + // If permission was denied, we do not have executable bit support. + Err(err) if err.kind() == io::ErrorKind::PermissionDenied => Ok(false), + Err(err) => Err(err), + Ok(()) => { + // Verify that the permission change was not silently ignored. + let mode = temp_file.metadata()?.permissions().mode(); + Ok(mode == new_mode) + } + } + } + + /// Symlinks are always available on Unix. pub fn check_symlink_support() -> io::Result { Ok(true) } @@ -383,6 +406,17 @@ mod tests { use super::*; use crate::tests::new_temp_dir; + #[test] + #[cfg(unix)] + fn exec_bit_support_in_temp_dir() { + // Temporary directories on Unix should always have executable support. + // Note that it would be problematic to test in a non-temp directory, as + // a developer's filesystem may or may not have executable bit support. + let dir = new_temp_dir(); + let supported = check_executable_bit_support(dir.path()).unwrap(); + assert!(supported); + } + #[test] fn test_path_bytes_roundtrip() { let bytes = b"ascii"; diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index f00540ad464..19624d6d438 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -126,38 +126,135 @@ use crate::working_copy::WorkingCopy; use crate::working_copy::WorkingCopyFactory; use crate::working_copy::WorkingCopyStateError; -/// On-disk state of file executable bit. -// TODO: maybe better to preserve the executable bit on all platforms, and -// ignore conditionally? #3949 -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub struct FileExecutableFlag(#[cfg(unix)] bool); +/// How to propagate executable bit changes in file metadata to/from the repo. +/// +/// On Windows, executable bits are always ignored, but on Unix they are +/// respected by default, but may be ignored by user settings or if we find +/// that the filesystem of the working copy doesn't support executable bits. +#[derive(Clone, Copy, Debug)] +enum ExecChangePolicy { + Ignore, + #[cfg(unix)] + Respect, +} -#[cfg(unix)] -impl FileExecutableFlag { - pub const fn from_bool_lossy(executable: bool) -> Self { - Self(executable) - } +/// The executable bit change setting as exposed to the user. +#[derive(Clone, Copy, Debug, Default, serde::Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum ExecChangeSetting { + Ignore, + Respect, + #[default] + Auto, +} - pub fn unwrap_or_else(self, _: impl FnOnce() -> bool) -> bool { - self.0 +#[cfg_attr(windows, expect(unused_variables))] +impl ExecChangePolicy { + /// Get the executable bit policy based on user settings and executable bit + /// support in the working copy's state path. + /// + /// On Unix we check whether executable bits are supported in the working + /// copy to determine respect/ignorance, but we default to respect. + fn new(exec_change_setting: ExecChangeSetting, state_path: &Path) -> Self { + #[cfg(windows)] + return Self::Ignore; + #[cfg(unix)] + return match exec_change_setting { + ExecChangeSetting::Ignore => Self::Ignore, + ExecChangeSetting::Respect => Self::Respect, + ExecChangeSetting::Auto => { + match crate::file_util::check_executable_bit_support(state_path) { + Ok(false) => Self::Ignore, + Ok(true) => Self::Respect, + Err(err) => { + tracing::warn!(?err, "Error when checking for executable bit support"); + Self::Respect + } + } + } + }; } } -// Windows doesn't support executable bit. -#[cfg(windows)] -impl FileExecutableFlag { - pub const fn from_bool_lossy(_executable: bool) -> Self { - Self() +/// On-disk state of file executable as cached in the file states. This does +/// *not* necessarily equal the `executable` field of [`TreeValue::File`]: the +/// two are allowed to diverge if and only if we're ignoring executable bit +/// changes. +/// +/// This will only ever be true on Windows if the repo is also being accessed +/// from a Unix version of jj, such as when accessed from WSL. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct ExecBit(bool); + +#[cfg_attr(windows, expect(unused_variables))] +impl ExecBit { + /// Get the executable bit for a tree value to write to the repo store. + /// + /// If we're ignoring the executable bit, then we fallback to the previous + /// in-repo executable bit if present. + fn for_tree_value( + self, + exec_policy: ExecChangePolicy, + prev_in_repo: impl FnOnce() -> Option, + ) -> bool { + match exec_policy { + ExecChangePolicy::Ignore => prev_in_repo().unwrap_or(false), + #[cfg(unix)] + ExecChangePolicy::Respect => self.0, + } + } + + /// Set the on-disk executable bit to be written based on the in-repo bit or + /// the previous on-disk executable bit. + /// + /// On Windows, we set this to `false` because when we later write files, we + /// always create them anew, so the executable bit will always be `false` + /// even if shared with a Unix machine. + /// + /// `prev_on_disk` is a closure because it is somewhat expensive and is only + /// used if ignoring the executable bit on Unix. + fn new_from_repo( + in_repo: bool, + exec_policy: ExecChangePolicy, + prev_on_disk: impl FnOnce() -> Option, + ) -> Self { + match exec_policy { + #[cfg(windows)] + ExecChangePolicy::Ignore => Self(false), + #[cfg(unix)] + ExecChangePolicy::Ignore => prev_on_disk().unwrap_or(Self(false)), + #[cfg(unix)] + ExecChangePolicy::Respect => Self(in_repo), + } } - pub fn unwrap_or_else(self, f: impl FnOnce() -> bool) -> bool { - f() + /// Load the on-disk executable bit from file metadata. + fn new_from_disk(metadata: &Metadata) -> Self { + #[cfg(unix)] + return Self(metadata.permissions().mode() & 0o111 != 0); + #[cfg(windows)] + return Self(false); } } +/// Set the executable bit of a file on-disk. This is a no-op on Windows. +/// +/// On Unix, we manually set the executable bit to the previous value on-disk. +/// This is necessary because we write all files by creating them new, so files +/// won't preserve their permissions naturally. +#[cfg_attr(windows, expect(unused_variables))] +fn set_executable(exec_bit: ExecBit, disk_path: &Path) -> Result<(), io::Error> { + #[cfg(unix)] + { + let mode = if exec_bit.0 { 0o755 } else { 0o644 }; + fs::set_permissions(disk_path, fs::Permissions::from_mode(mode))?; + } + Ok(()) +} + #[derive(Debug, PartialEq, Eq, Clone)] pub enum FileType { - Normal { executable: FileExecutableFlag }, + Normal { exec_bit: ExecBit }, Symlink, GitSubmodule, } @@ -190,19 +287,19 @@ 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 { - let executable = FileExecutableFlag::from_bool_lossy(false); Self { - file_type: FileType::Normal { executable }, + file_type: FileType::Normal { + exec_bit: ExecBit(false), + }, mtime: MillisSinceEpoch(0), size: 0, materialized_conflict_data: None, } } - fn for_file(executable: bool, size: u64, metadata: &Metadata) -> Self { - let executable = FileExecutableFlag::from_bool_lossy(executable); + fn for_file(exec_bit: ExecBit, size: u64, metadata: &Metadata) -> Self { Self { - file_type: FileType::Normal { executable }, + file_type: FileType::Normal { exec_bit }, mtime: mtime_from_metadata(metadata), size, materialized_conflict_data: None, @@ -346,6 +443,14 @@ impl<'a> FileStates<'a> { Some(state) } + /// Returns the executable bit state if `path` is a normal file. + pub fn get_exec_bit(&self, path: &RepoPath) -> Option { + match self.get(path)?.file_type { + FileType::Normal { exec_bit } => Some(exec_bit), + FileType::Symlink | FileType::GitSubmodule => None, + } + } + /// Faster version of `get("/")`. Requires that all entries share /// the same prefix `dir`. fn get_at(&self, dir: &RepoPath, name: &RepoPathComponent) -> Option { @@ -439,16 +544,16 @@ impl<'a> IntoIterator for FileStates<'a> { fn file_state_from_proto(proto: &crate::protos::local_working_copy::FileState) -> FileState { let file_type = match proto.file_type() { crate::protos::local_working_copy::FileType::Normal => FileType::Normal { - executable: FileExecutableFlag::from_bool_lossy(false), + exec_bit: ExecBit(false), }, - // On Windows, FileType::Executable can exist in files written by older - // versions of jj + // On Windows, `FileType::Executable` can exist if the repo is being + // shared with a Unix version of jj, such as when accessed from WSL. crate::protos::local_working_copy::FileType::Executable => FileType::Normal { - executable: FileExecutableFlag::from_bool_lossy(true), + exec_bit: ExecBit(true), }, crate::protos::local_working_copy::FileType::Symlink => FileType::Symlink, crate::protos::local_working_copy::FileType::Conflict => FileType::Normal { - executable: FileExecutableFlag::from_bool_lossy(false), + exec_bit: ExecBit(false), }, crate::protos::local_working_copy::FileType::GitSubmodule => FileType::GitSubmodule, }; @@ -467,8 +572,8 @@ fn file_state_from_proto(proto: &crate::protos::local_working_copy::FileState) - fn file_state_to_proto(file_state: &FileState) -> crate::protos::local_working_copy::FileState { let mut proto = crate::protos::local_working_copy::FileState::default(); let file_type = match &file_state.file_type { - FileType::Normal { executable } => { - if executable.unwrap_or_else(Default::default) { + FileType::Normal { exec_bit } => { + if exec_bit.0 { crate::protos::local_working_copy::FileType::Executable } else { crate::protos::local_working_copy::FileType::Normal @@ -699,6 +804,7 @@ fn mtime_from_metadata(metadata: &Metadata) -> MillisSinceEpoch { ) } +/// Create a new [`FileState`] from metadata. fn file_state(metadata: &Metadata) -> Option { let metadata_file_type = metadata.file_type(); let file_type = if metadata_file_type.is_dir() { @@ -706,12 +812,8 @@ fn file_state(metadata: &Metadata) -> Option { } else if metadata_file_type.is_symlink() { Some(FileType::Symlink) } else if metadata_file_type.is_file() { - #[cfg(unix)] - let executable = metadata.permissions().mode() & 0o111 != 0; - #[cfg(windows)] - let executable = false; - let executable = FileExecutableFlag::from_bool_lossy(executable); - Some(FileType::Normal { executable }) + let exec_bit = ExecBit::new_from_disk(metadata); + Some(FileType::Normal { exec_bit }) } else { None }; @@ -742,6 +844,8 @@ pub struct TreeStateSettings { /// file to the backend, and vice versa when it checks out code onto your /// filesystem. pub eol_conversion_mode: EolConversionMode, + /// Whether to ignore changes to the executable bit for files on Unix. + pub exec_change_setting: ExecChangeSetting, /// The fsmonitor (e.g. Watchman) to use, if any. pub fsmonitor_settings: FsmonitorSettings, } @@ -752,6 +856,7 @@ impl TreeStateSettings { Ok(Self { conflict_marker_style: user_settings.get("ui.conflict-marker-style")?, eol_conversion_mode: EolConversionMode::try_from_settings(user_settings)?, + exec_change_setting: user_settings.get("working-copy.exec-bit-change")?, fsmonitor_settings: FsmonitorSettings::from_settings(user_settings)?, }) } @@ -774,6 +879,7 @@ pub struct TreeState { watchman_clock: Option, conflict_marker_style: ConflictMarkerStyle, + exec_policy: ExecChangePolicy, fsmonitor_settings: FsmonitorSettings, target_eol_strategy: TargetEolStrategy, } @@ -834,10 +940,12 @@ impl TreeState { &TreeStateSettings { conflict_marker_style, eol_conversion_mode, + exec_change_setting, ref fsmonitor_settings, }: &TreeStateSettings, ) -> Self { let tree_id = store.empty_merged_tree_id(); + let exec_policy = ExecChangePolicy::new(exec_change_setting, &state_path); Self { store, working_copy_path, @@ -849,6 +957,7 @@ impl TreeState { symlink_support: check_symlink_support().unwrap_or(false), watchman_clock: None, conflict_marker_style, + exec_policy, fsmonitor_settings: fsmonitor_settings.clone(), target_eol_strategy: TargetEolStrategy::new(eol_conversion_mode), } @@ -1507,12 +1616,12 @@ impl FileSnapshotter<'_> { new_file_state.file_type.clone() }; let new_tree_values = match new_file_type { - FileType::Normal { executable } => self + FileType::Normal { exec_bit } => self .write_path_to_store( repo_path, disk_path, ¤t_tree_values, - executable, + exec_bit, maybe_current_file_state.and_then(|state| state.materialized_conflict_data), ) .block_on()?, @@ -1541,22 +1650,22 @@ impl FileSnapshotter<'_> { repo_path: &RepoPath, disk_path: &Path, current_tree_values: &MergedTreeValue, - executable: FileExecutableFlag, + exec_bit: ExecBit, materialized_conflict_data: Option, ) -> Result { if let Some(current_tree_value) = current_tree_values.as_resolved() { let id = self.write_file_to_store(repo_path, disk_path).await?; // On Windows, we preserve the executable bit from the current tree. - let executable = executable.unwrap_or_else(|| { + let executable = exec_bit.for_tree_value(self.tree_state.exec_policy, || { if let Some(TreeValue::File { id: _, executable, copy_id: _, }) = current_tree_value { - *executable + Some(*executable) } else { - false + None } }); // Preserve the copy id from the current tree @@ -1620,12 +1729,11 @@ impl FileSnapshotter<'_> { match new_file_ids.into_resolved() { Ok(file_id) => { // On Windows, we preserve the executable bit from the merged trees. - let executable = executable.unwrap_or_else(|| { - if let Some(merge) = current_tree_values.to_executable_merge() { - conflicts::resolve_file_executable(&merge).unwrap_or(false) - } else { - false - } + let executable = exec_bit.for_tree_value(self.tree_state.exec_policy, || { + current_tree_values + .to_executable_merge() + .as_ref() + .and_then(conflicts::resolve_file_executable) }); Ok(Merge::normal(TreeValue::File { id: file_id.unwrap(), @@ -1704,7 +1812,7 @@ impl TreeState { &self, disk_path: &Path, contents: impl AsyncRead + Send + Unpin, - executable: bool, + exec_bit: ExecBit, apply_eol_conversion: bool, ) -> Result { let mut file = File::options() @@ -1735,7 +1843,8 @@ impl TreeState { ), err: err.into(), })?; - self.set_executable(disk_path, executable)?; + set_executable(exec_bit, disk_path) + .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; // 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 @@ -1743,7 +1852,7 @@ impl TreeState { let metadata = file .metadata() .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; - Ok(FileState::for_file(executable, size as u64, &metadata)) + Ok(FileState::for_file(exec_bit, size as u64, &metadata)) } fn write_symlink(&self, disk_path: &Path, target: String) -> Result { @@ -1766,7 +1875,7 @@ impl TreeState { &self, disk_path: &Path, contents: &[u8], - executable: bool, + exec_bit: ExecBit, ) -> Result { let contents = self .target_eol_strategy @@ -1790,22 +1899,12 @@ impl TreeState { message: format!("Failed to write conflict to file {}", disk_path.display()), err: err.into(), })? as u64; - self.set_executable(disk_path, executable)?; + set_executable(exec_bit, disk_path) + .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; let metadata = file .metadata() .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; - Ok(FileState::for_file(executable, size, &metadata)) - } - - #[cfg_attr(windows, expect(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))?; - } - Ok(()) + Ok(FileState::for_file(exec_bit, size, &metadata)) } pub fn check_out(&mut self, new_tree: &MergedTree) -> Result { @@ -1923,6 +2022,11 @@ impl TreeState { continue; } + // We get the previous executable bit from the file states and not + // the tree value because only the file states store the on-disk + // executable bit. + let get_prev_exec = || self.file_states().get_exec_bit(&path); + // TODO: Check that the file has not changed before overwriting/removing it. let file_state = match after { MaterializedTreeValue::Absent | MaterializedTreeValue::AccessDenied(_) => { @@ -1937,14 +2041,17 @@ impl TreeState { continue; } MaterializedTreeValue::File(file) => { - self.write_file(&disk_path, file.reader, file.executable, true) + let exec_bit = + ExecBit::new_from_repo(file.executable, self.exec_policy, get_prev_exec); + self.write_file(&disk_path, file.reader, exec_bit, true) .await? } MaterializedTreeValue::Symlink { id: _, target } => { if self.symlink_support { self.write_symlink(&disk_path, target)? } else { - self.write_file(&disk_path, target.as_bytes(), false, false) + // The fake symlink file shouldn't be executable. + self.write_file(&disk_path, target.as_bytes(), ExecBit(false), false) .await? } } @@ -1963,10 +2070,14 @@ impl TreeState { marker_len: Some(conflict_marker_len), merge: self.store.merge_options().clone(), }; + let exec_bit = ExecBit::new_from_repo( + file.executable.unwrap_or(false), + self.exec_policy, + get_prev_exec, + ); let contents = materialize_merge_result_to_bytes(&file.contents, &options); - let mut file_state = self - .write_conflict(&disk_path, &contents, file.executable.unwrap_or(false)) - .await?; + let mut file_state = + self.write_conflict(&disk_path, &contents, exec_bit).await?; file_state.materialized_conflict_data = Some(MaterializedConflictData { conflict_marker_len: conflict_marker_len.try_into().unwrap_or(u32::MAX), }); @@ -1976,8 +2087,8 @@ impl TreeState { // Unless all terms are regular files, we can't do much // better than trying to describe the merge. let contents = id.describe(); - let executable = false; - self.write_conflict(&disk_path, contents.as_bytes(), executable) + // Since this is a dummy file, it shouldn't be executable. + self.write_conflict(&disk_path, contents.as_bytes(), ExecBit(false)) .await? } }; @@ -2011,9 +2122,12 @@ impl TreeState { id: _, executable, copy_id: _, - } => FileType::Normal { - executable: FileExecutableFlag::from_bool_lossy(executable), - }, + } => { + let get_prev_exec = || self.file_states().get_exec_bit(&path); + let exec_bit = + ExecBit::new_from_repo(executable, self.exec_policy, get_prev_exec); + FileType::Normal { exec_bit } + } TreeValue::Symlink(_id) => FileType::Symlink, TreeValue::GitSubmodule(_id) => { eprintln!("ignoring git submodule at {path:?}"); @@ -2026,7 +2140,7 @@ impl TreeState { Err(_values) => { // TODO: Try to set the executable bit based on the conflict FileType::Normal { - executable: FileExecutableFlag::from_bool_lossy(false), + exec_bit: ExecBit(false), } } }; @@ -2468,7 +2582,7 @@ mod tests { fn new_state(size: u64) -> FileState { FileState { file_type: FileType::Normal { - executable: FileExecutableFlag::from_bool_lossy(false), + exec_bit: ExecBit(false), }, mtime: MillisSinceEpoch(0), size, diff --git a/lib/tests/runner.rs b/lib/tests/runner.rs index 92bdeb2364f..b127cd3b037 100644 --- a/lib/tests/runner.rs +++ b/lib/tests/runner.rs @@ -25,6 +25,7 @@ mod test_init; mod test_load_repo; mod test_local_working_copy; mod test_local_working_copy_concurrent; +mod test_local_working_copy_executable_bit; mod test_local_working_copy_sparse; mod test_merge_trees; mod test_merged_tree; diff --git a/lib/tests/test_local_working_copy_executable_bit.rs b/lib/tests/test_local_working_copy_executable_bit.rs new file mode 100644 index 00000000000..d555fdeb2a7 --- /dev/null +++ b/lib/tests/test_local_working_copy_executable_bit.rs @@ -0,0 +1,146 @@ +// Copyright 2025 The Jujutsu Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#![cfg(unix)] // Nothing to test on Windows + +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::path::Path; +use std::sync::Arc; + +use jj_lib::backend::MergedTreeId; +use jj_lib::backend::TreeValue; +use jj_lib::commit::Commit; +use jj_lib::repo::Repo as _; +use jj_lib::repo_path::RepoPath; +use jj_lib::store::Store; +use testutils::TestTreeBuilder; +use testutils::TestWorkspace; +use testutils::repo_path; + +/// Assert that a file's executable bit matches the expected value. +#[track_caller] +fn assert_file_executable(path: &Path, expected: bool) { + let perms = path.metadata().unwrap().permissions(); + let actual = (perms.mode() & 0o100) == 0o100; + assert_eq!(actual, expected); +} + +/// Set the executable bit of a file on the filesystem. +#[track_caller] +fn set_file_executable(path: &Path, executable: bool) { + let prev_mode = path.metadata().unwrap().permissions().mode(); + let is_executable = prev_mode & 0o100 != 0; + assert_ne!(executable, is_executable, "why are you calling this?"); + let new_mode = if executable { 0o755 } else { 0o644 }; + fs::set_permissions(path, PermissionsExt::from_mode(new_mode)).unwrap(); +} + +/// Assert that a tree value's executable bit matches the expected value. +#[track_caller] +fn assert_tree_executable(tree_val: TreeValue, expected: bool) { + if let TreeValue::File { executable, .. } = tree_val { + assert_eq!(executable, expected); + } else { + panic!() + } +} + +/// Create a tree with an empty file having the given executable bit. Returns +/// the new tree id. +#[track_caller] +fn create_tree_executable( + store: &Arc, + repo_path: &RepoPath, + executable: bool, +) -> MergedTreeId { + let mut tree_builder = TestTreeBuilder::new(store.clone()); + tree_builder.file(repo_path, "").executable(executable); + tree_builder.write_merged_tree().id().clone() +} + +/// Build two commits that write the executable bit of a file as true/false. +#[track_caller] +fn prepare_exec_commits(ws: &TestWorkspace, repo_path: &RepoPath) -> (Commit, Commit) { + let store = ws.repo.store(); + let tree_exec = create_tree_executable(store, repo_path, true); + let tree_no_exec = create_tree_executable(store, repo_path, false); + assert_ne!(tree_exec, tree_no_exec); + + let commit_with_id = |id| testutils::commit_with_tree(ws.repo.store(), id); + let commit_exec = commit_with_id(tree_exec); + let commit_no_exec = commit_with_id(tree_no_exec); + assert_ne!(commit_exec, commit_no_exec); + + (commit_exec, commit_no_exec) +} + +/// Test that checking out a tree writes the correct executable bit to the +/// filesystem. +#[test] +fn test_exec_bit_checkout() { + let mut ws = TestWorkspace::init(); + let path = &ws.workspace.workspace_root().join("file"); + let repo_path = repo_path("file"); + + let (exec, no_exec) = prepare_exec_commits(&ws, repo_path); + let mut checkout_exec_commit = |executable| { + let commit = if executable { &exec } else { &no_exec }; + let op_id = ws.repo.op_id().clone(); + ws.workspace.check_out(op_id, None, commit).unwrap(); + }; + + // Checkout commits and ensure the filesystem is updated correctly. + assert!(!fs::exists(path).unwrap()); + for exec in [true, false, true] { + checkout_exec_commit(exec); + assert_file_executable(path, exec); + } +} + +/// Test that snapshotting files stores the correct executable bit in the tree. +#[test] +fn test_exec_bit_snapshot() { + let mut ws = TestWorkspace::init(); + let path = &ws.workspace.workspace_root().join("file"); + let repo_path = repo_path("file"); + + // Snapshot, then assert the tree has the expected executable bit. + let mut snapshot_assert_exec_bit = |expected| { + let tree = ws.snapshot().unwrap().into_merge().into_resolved().unwrap(); + let tree_val = tree.path_value(repo_path).unwrap().unwrap(); + assert_tree_executable(tree_val, expected); + }; + + // Snapshot tree values when the file is/isn't executable. + fs::write(path, "initial content").unwrap(); + snapshot_assert_exec_bit(false); + + fs::write(path, "first change").unwrap(); + snapshot_assert_exec_bit(false); + + set_file_executable(path, true); + snapshot_assert_exec_bit(true); + + fs::write(path, "second change").unwrap(); + snapshot_assert_exec_bit(true); + + // Back to the same contents as before, but different exec bit. + fs::write(path, "first change").unwrap(); + set_file_executable(path, false); + snapshot_assert_exec_bit(false); + + set_file_executable(path, true); + snapshot_assert_exec_bit(true); +}