Skip to content

Commit

Permalink
Convert the mode more portably
Browse files Browse the repository at this point in the history
The previous conversion worked on Linux but failed on macOS where
modes, as represented in system data structures, are 16-bit rather
than 32-bit.

The previous commit broke builds on macOS (and possibly other
OSes not currently tested on CI), which this should fix.

This commit also does some other refactorings:

- Simplify the way conversions are represented.

- Express with `expect` that, by this point, there are no unknown
  bits, rather than doing something that would silently preserve or
  silently discard them. There would have to be a bug (and probably
  in `gix-worktree-state` itself) for that not to be the case.

- Clarify the TODO comment, and also weaken it since there might be
  some reason for `set_mode_executable` to keep using `Permissions`
  in some way.
  • Loading branch information
EliahKagan committed Jan 21, 2025
1 parent 2364b3f commit ba92611
Showing 1 changed file with 5 additions and 4 deletions.
9 changes: 5 additions & 4 deletions gix-worktree-state/src/checkout/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -287,10 +287,11 @@ pub(crate) fn finalize_entry(
if let Some(path) = set_executable_after_creation {
let old_perm = std::fs::symlink_metadata(path)?.permissions();
if let Some(new_perm) = set_mode_executable(old_perm) {
// TODO: If the `fchmod` approach is kept, `set_mode_executable` shouldn't operate on std::fs::Permissions.
use std::os::{fd::AsFd, unix::fs::PermissionsExt};
rustix::fs::fchmod(file.as_fd(), new_perm.mode().into())
.map_err(|errno| std::io::Error::from_raw_os_error(errno.raw_os_error()))?;
// TODO: If we keep `fchmod`, maybe change `set_mode_executable` not to use `std::fs::Permissions`.
use std::os::unix::fs::PermissionsExt;
let mode = rustix::fs::Mode::from_bits(new_perm.mode())
.expect("`set_mode_executable` shouldn't preserve or add unknown bits");
rustix::fs::fchmod(&file, mode).map_err(std::io::Error::from)?;
}
}
// NOTE: we don't call `file.sync_all()` here knowing that some filesystems don't handle this well.
Expand Down

0 comments on commit ba92611

Please sign in to comment.