Skip to content

Commit

Permalink
change: Get and set mode using std, still on open file descriptor
Browse files Browse the repository at this point in the history
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 GitoxideLabs#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.)

- Gain 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-GitoxideLabs#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.
  • Loading branch information
EliahKagan committed Jan 26, 2025
1 parent 810b5cf commit 2ae2a23
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 22 deletions.
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 0 additions & 6 deletions gix-worktree-state/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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",
] }
26 changes: 11 additions & 15 deletions gix-worktree-state/src/checkout/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}

Expand All @@ -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))]
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit 2ae2a23

Please sign in to comment.