Skip to content

Commit ddaf8a6

Browse files
committed
Convert the mode more portably
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.
1 parent 0becc91 commit ddaf8a6

File tree

1 file changed

+5
-4
lines changed

1 file changed

+5
-4
lines changed

gix-worktree-state/src/checkout/entry.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -287,10 +287,11 @@ pub(crate) fn finalize_entry(
287287
if let Some(path) = set_executable_after_creation {
288288
let old_perm = std::fs::symlink_metadata(path)?.permissions();
289289
if let Some(new_perm) = set_mode_executable(old_perm) {
290-
// TODO: If the `fchmod` approach is kept, `set_mode_executable` shouldn't operate on std::fs::Permissions.
291-
use std::os::{fd::AsFd, unix::fs::PermissionsExt};
292-
rustix::fs::fchmod(file.as_fd(), new_perm.mode().into())
293-
.map_err(|errno| std::io::Error::from_raw_os_error(errno.raw_os_error()))?;
290+
// TODO: If we keep `fchmod`, maybe change `set_mode_executable` not to use `std::fs::Permissions`.
291+
use std::os::unix::fs::PermissionsExt;
292+
let mode = rustix::fs::Mode::from_bits(new_perm.mode())
293+
.expect("`set_mode_executable` shouldn't preserve or add unknown bits");
294+
rustix::fs::fchmod(&file, mode).map_err(std::io::Error::from)?;
294295
}
295296
}
296297
// NOTE: we don't call `file.sync_all()` here knowing that some filesystems don't handle this well.

0 commit comments

Comments
 (0)