From 51724d15a4ae5a41b4a97412b2a847366c61f98d Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 25 Jan 2025 19:07:45 -0500 Subject: [PATCH 1/2] change: Get and set mode using std, still on open file descriptor This replaces explicit `fstat` and `fchmod` calls (via `rustix`) with `File::metadata` and `File::set_permissions`, respectively. The change here is confined to `gix-worktree-state` and, more specifically, to the operation of checking the mode of an open file and setting a new mode based on it with some executable bits added. In practice, currently, on Unix-like systems: - `File::metadata` either: * calls `fstat`, or * calls `statx` in a way that causes it to operate similarly to `fstat` (this is used on Linux, in versions with `statx`). This is not explicitly documented, though calling `stat` or `lstat`, or calling `statx` in a way that would cause it to behave like `stat` or `lstat` rather than `fstat`, would not preserve the documented behavior of `File::metadata`, which operates on a `File` and does not take a path. - `File::set_permissions` calls `fchmod`. This is explicitly documented and `fchmod` is listed as an alias for documentation purposes. But it is noted as an implementation detail that may change without warning. However, calling `chmod` or `lchmod` would not preserve the documented behavior of `File::set_permissions`, which (like `File::metadata`) operates on a `File` and does not take a path. While these details can, in principle, change without warning, per https://doc.rust-lang.org/stable/std/io/index.html#platform-specific-behavior, it seems that, in preserving the documented semantics of operating on the file referenced by the file descriptor (rather than a path), the behavior would still have to be correct for our use here. The change in this commit intends to maintain the effect of #1803 with two minor improvements for maintainability and clarity: - No longer require `gix-worktree-state` to depend directly on `rustix`. (This is minor because it still depends transitively on it through `gix-fs`, though some uses of `rustix::fs` in `gix-fs` might likewise be possible to replace in the future.) - Use the standard library's slightly higher level interface where modes are treated as `u32` even on operating systems where they are different (e.g. `u16` on macOS). This removes operations that do not correspond to what the code is conceptually doing. It also lets the function that computes the new mode from the old mode no longer depend on a type that differs across Unix-like targets. Importantly, this continues to differ from the pre-#1803 behavior of using functions that operated on paths. For reading metadata on a file on Unix-like systems, the current general correspondence between Rust `std`, POSIX functions, and `statx`, where the rightmost three columns pertain to how `statx` is called, is: | std::fs::* | POSIX | fd | path | *distinctive* flags | |------------------|-------|----------|------|---------------------| | metadata | stat | AT_FDCWD | path | (none) | | symlink_metadata | lstat | AT_FDCWD | path | AT_SYMLINK_NOFOLLOW | | File::metadata | fstat | file | "" | AT_EMPTY_PATH | For writing metadata on a file on Unix-like systems, the current correspondence between Rust `std` and POSIX functions is: | std::fs::* | POSIX | |-----------------------| -------| | set_permissions | chmod | | (none) | lchmod | | File::set_permissions | fchmod | It may be that some uses of `rustix::fs` facilities can be similarly replaced in `gix_fs`, but this commit does not include any such changes. --- Cargo.lock | 1 - gix-worktree-state/Cargo.toml | 6 ------ gix-worktree-state/src/checkout/entry.rs | 26 ++++++++++-------------- 3 files changed, 11 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bdf659a431d..d022dc163a6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2928,7 +2928,6 @@ dependencies = [ "gix-path 0.10.14", "gix-worktree 0.39.0", "io-close", - "rustix", "thiserror 2.0.3", ] diff --git a/gix-worktree-state/Cargo.toml b/gix-worktree-state/Cargo.toml index 5f5a2b960bb..deefbdf6b8e 100644 --- a/gix-worktree-state/Cargo.toml +++ b/gix-worktree-state/Cargo.toml @@ -29,9 +29,3 @@ gix-filter = { version = "^0.17.0", path = "../gix-filter" } io-close = "0.3.7" thiserror = "2.0.0" bstr = { version = "1.3.0", default-features = false } - -[target.'cfg(unix)'.dependencies] -rustix = { version = "0.38.20", default-features = false, features = [ - "std", - "fs", -] } diff --git a/gix-worktree-state/src/checkout/entry.rs b/gix-worktree-state/src/checkout/entry.rs index 8df37947113..e3aea3c3bd7 100644 --- a/gix-worktree-state/src/checkout/entry.rs +++ b/gix-worktree-state/src/checkout/entry.rs @@ -298,9 +298,10 @@ pub(crate) fn finalize_entry( /// See `let_readers_execute` for the exact details of how the mode is transformed. #[cfg(unix)] fn set_executable(file: &std::fs::File) -> Result<(), std::io::Error> { - let old_raw_mode = rustix::fs::fstat(file)?.st_mode; - let new_mode = let_readers_execute(old_raw_mode); - rustix::fs::fchmod(file, new_mode)?; + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + let old_mode = file.metadata()?.mode(); + let new_mode = let_readers_execute(old_mode); + file.set_permissions(std::fs::Permissions::from_mode(new_mode))?; Ok(()) } @@ -309,17 +310,13 @@ fn set_executable(file: &std::fs::File) -> Result<(), std::io::Error> { /// Currently this adds executable bits for whoever has read bits already. It doesn't use the umask. /// Set-user-ID and set-group-ID bits are unset for safety. The sticky bit is also unset. /// -/// This returns only mode bits, not file type. The return value can be passed to chmod or fchmod. +/// This returns only mode bits, not file type. The return value can be used in chmod or fchmod. #[cfg(unix)] -fn let_readers_execute(mut raw_mode: rustix::fs::RawMode) -> rustix::fs::Mode { - assert_eq!( - raw_mode & 0o170000, - 0o100000, - "bug in caller if not from a regular file" - ); - raw_mode &= 0o777; // Clear type, non-rwx mode bits (setuid, setgid, sticky). - raw_mode |= (raw_mode & 0o444) >> 2; // Let readers also execute. - rustix::fs::Mode::from_bits(raw_mode).expect("all bits recognized") +fn let_readers_execute(mut mode: u32) -> u32 { + assert_eq!(mode & 0o170000, 0o100000, "bug in caller if not from a regular file"); + mode &= 0o777; // Clear type, non-rwx mode bits (setuid, setgid, sticky). + mode |= (mode & 0o444) >> 2; // Let readers also execute. + mode } #[cfg(all(test, unix))] @@ -372,8 +369,7 @@ mod tests { (0o106400, 0o500), (0o102462, 0o572), ]; - for (st_mode, raw_expected) in cases { - let expected = rustix::fs::Mode::from_bits(raw_expected).expect("expected mode is a mode"); + for (st_mode, expected) in cases { let actual = super::let_readers_execute(st_mode); assert_eq!( actual, expected, From 53ded78a4dd741c7e2eb8c38f1d5ca17b3ed0943 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 25 Jan 2025 22:28:22 -0500 Subject: [PATCH 2/2] Test `let_readers_execute` on all targets The `let_readers_execute` function computes a new mode with `+x` set reasonably based on an old mode. It is only useful on Unix-like systems. But there may be a benefit to being able to detect more bugs affecting one platform even on another; this can help catch bugs slightly earlier in the development process. Now that `let_readers_execute` only longer depends on a type that varies across Unix-like systems and is only defined on such systems (instead representing modes as `u32` on all of them), it no longer contains anything that can't build or be tested everywhere. So this lets its tests run on all systems, even non-Unix-like ones. --- gix-worktree-state/src/checkout/entry.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gix-worktree-state/src/checkout/entry.rs b/gix-worktree-state/src/checkout/entry.rs index e3aea3c3bd7..602aa280308 100644 --- a/gix-worktree-state/src/checkout/entry.rs +++ b/gix-worktree-state/src/checkout/entry.rs @@ -311,7 +311,7 @@ fn set_executable(file: &std::fs::File) -> Result<(), std::io::Error> { /// Set-user-ID and set-group-ID bits are unset for safety. The sticky bit is also unset. /// /// This returns only mode bits, not file type. The return value can be used in chmod or fchmod. -#[cfg(unix)] +#[cfg(any(unix, test))] fn let_readers_execute(mut mode: u32) -> u32 { assert_eq!(mode & 0o170000, 0o100000, "bug in caller if not from a regular file"); mode &= 0o777; // Clear type, non-rwx mode bits (setuid, setgid, sticky). @@ -319,7 +319,7 @@ fn let_readers_execute(mut mode: u32) -> u32 { mode } -#[cfg(all(test, unix))] +#[cfg(test)] mod tests { #[test] fn let_readers_execute() {