From 0becc913c57afd8a0bf95e97c14ba1130f662f1a Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 21 Jan 2025 06:02:15 -0500 Subject: [PATCH 1/8] change: Set +x via open file descriptor, not path When executable permissions were set on a file being checked out, and when it was done after the file was already created, this was done on its path, using `chmod` (via a higher level abstraction). But we have an open `File` object for it already, at that point. This uses `fchmod` on the existing open file descriptor (obtained from the `File` object), instead of `chmod` on the path. (Since #1764, the file mode has also been read first, to figure out what new mode to set it to for +x. That uses `lstat` (via a higher level abstraction). The change here is therefore really only half of what should be done. To complete it, and also to avoid new problems due to a new inconsistency, that should be done with `fstat` on the file descriptor instead of `lstat` on the path. That will be done in a directly fortcoming commit -- after portability issues in the type of the `st_mode` field, which is not actually `u32` on all systems, are examined.) --- Cargo.lock | 1 + gix-worktree-state/Cargo.toml | 6 ++++++ gix-worktree-state/src/checkout/entry.rs | 5 ++++- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index d022dc163a6..bdf659a431d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2928,6 +2928,7 @@ 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 deefbdf6b8e..5f5a2b960bb 100644 --- a/gix-worktree-state/Cargo.toml +++ b/gix-worktree-state/Cargo.toml @@ -29,3 +29,9 @@ 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 aff4ac01032..c04c0a90d2c 100644 --- a/gix-worktree-state/src/checkout/entry.rs +++ b/gix-worktree-state/src/checkout/entry.rs @@ -287,7 +287,10 @@ 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) { - std::fs::set_permissions(path, new_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()))?; } } // NOTE: we don't call `file.sync_all()` here knowing that some filesystems don't handle this well. From ddaf8a6cfc5ffbc87124605fe9644ca8d9bdaa29 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 21 Jan 2025 17:18:44 -0500 Subject: [PATCH 2/8] 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. --- gix-worktree-state/src/checkout/entry.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/gix-worktree-state/src/checkout/entry.rs b/gix-worktree-state/src/checkout/entry.rs index c04c0a90d2c..da4c81ac1ca 100644 --- a/gix-worktree-state/src/checkout/entry.rs +++ b/gix-worktree-state/src/checkout/entry.rs @@ -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. From d476c06d36dbf029ca4b3260e4f43d67e7bc77c1 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 21 Jan 2025 18:42:16 -0500 Subject: [PATCH 3/8] Really convert the mode more portably While replacing `into()` with `rustix::fs::Mode::from_bits(...)` helped in expressing (and enforcing) the desired semantics for bits that fit in `mode_t` but whose meaning is unrecognized, it did not actually make the code build on macOS, and probably was not enough to improve portability to any other systems where `mode_t` is not `u32`. This commit adds an explicit checked conversion of the value we get from calling `mode()` on a `Permissions` object, which is aways `u32`, into whatever type `mode_t` is defined as, which is `u32` on most systems but `u16` on macOS and possibly others. --- gix-worktree-state/src/checkout/entry.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gix-worktree-state/src/checkout/entry.rs b/gix-worktree-state/src/checkout/entry.rs index da4c81ac1ca..c03accb5b31 100644 --- a/gix-worktree-state/src/checkout/entry.rs +++ b/gix-worktree-state/src/checkout/entry.rs @@ -289,7 +289,8 @@ pub(crate) fn finalize_entry( if let Some(new_perm) = set_mode_executable(old_perm) { // 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()) + let raw_mode = new_perm.mode().try_into().expect("mode fits in `st_mode`"); + let mode = rustix::fs::Mode::from_bits(raw_mode) .expect("`set_mode_executable` shouldn't preserve or add unknown bits"); rustix::fs::fchmod(&file, mode).map_err(std::io::Error::from)?; } From 492ac8b2c706f6ddcf12585fc5c39ce6d3fc17cb Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 21 Jan 2025 19:55:39 -0500 Subject: [PATCH 4/8] Improve warning suppressions in `finalize_entry` - Make the conditional `unused_variable` suppression specific to the `set_executable_after_creation` parameter, which is all that needs it. This also makes it more immediately clear that nothing along the lines of `chmod +x` is done (or relevant) on Windows. - Allow `useless_conversion` for the conversion from `u32` to `rustix::fs::RawMode`. The `RawMode` type is what `rustix::fs` calls `mode_t` as used for the `st_mode` field of a `stat` structure. On most operating systems, it is `u32`, but this is not guaranteed by POSIX (which does not even guarantee that it is unsigned). It is `u16` at least on macOS and possibly other systems, and I am unsure if there are systems this code can run on where it is some other type besides `u16` or `u32`. For now, this does not attempt to make that suppression conditional, even though it is only needed when `RawMode` is already `u32`. I don't if there is a good way to express the condition under which it should apply. If there is, then it could be made conditional or, if it is truly reliable, then the conversion itself could be made conditional (though I suspect it is omitted when not needed in release builds, by optimization). --- gix-worktree-state/src/checkout/entry.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/gix-worktree-state/src/checkout/entry.rs b/gix-worktree-state/src/checkout/entry.rs index c03accb5b31..a7fa5d9760b 100644 --- a/gix-worktree-state/src/checkout/entry.rs +++ b/gix-worktree-state/src/checkout/entry.rs @@ -275,20 +275,21 @@ pub(crate) fn open_file( try_op_or_unlink(path, overwrite_existing, |p| options.open(p)).map(|f| (f, set_executable_after_creation)) } -/// Close `file` and store its stats in `entry`, possibly setting `file` executable depending on `set_executable_after_creation`. -#[cfg_attr(windows, allow(unused_variables))] +/// Close `file` and store its stats in `entry`, possibly setting `file` executable depending on +/// `set_executable_after_creation`. pub(crate) fn finalize_entry( entry: &mut gix_index::Entry, file: std::fs::File, - set_executable_after_creation: Option<&Path>, + #[cfg_attr(windows, allow(unused_variables))] set_executable_after_creation: Option<&Path>, ) -> Result<(), crate::checkout::Error> { // For possibly existing, overwritten files, we must change the file mode explicitly. #[cfg(unix)] 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 we keep `fchmod`, maybe change `set_mode_executable` not to use `std::fs::Permissions`. + // TODO: If we keep `fchmod`, maybe `set_mode_executable` shouldn't use `std::fs::Permissions`. use std::os::unix::fs::PermissionsExt; + #[allow(clippy::useless_conversion)] // mode_t is u32 on many but not all OSes. It's u16 on macOS. let raw_mode = new_perm.mode().try_into().expect("mode fits in `st_mode`"); let mode = rustix::fs::Mode::from_bits(raw_mode) .expect("`set_mode_executable` shouldn't preserve or add unknown bits"); From de939de5c746fddcfca216a27ae4b5e7b4b24d40 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 21 Jan 2025 22:25:02 -0500 Subject: [PATCH 5/8] change: Get mode before +x using open file descriptor, not path This uses `fstat` rather than a method that delegates to `lstat`, to get the current mode bits of the still-open file using the open file descriptor (obtained from the `File` object) rather than opening the path, which could be a different file in the case of concurrent modification by code outside gitoxide. This is half of the change to using the file descriptor rather than the path for the operations that adjust the mode by adding executable bitsi. (As before, they are added for whoever already had read bits, and we remove setuid, setgid, and sticky bits, so that +x does not cause a security problem or other potentially unexpected effects.) The other half of this change was already done, appearing in a recent `change:` commit. It consisted of setting the mode using `fchmod` instead of a method that delegated to `chmod`. With only that change, the overall effect would be worse, since possible race conditions could introduce even greater inconsistency. With both changes, the effect is consistent. Race conditions can still occur, but their effect is probably less severe than before either of the changes were made. One of the ways this may be less subject to race conditions is that, because neither obtaining the old mode nor setting the new one uses a path, but only the open file descriptor, an unexpected typechange will not occur: having opened a regular file, the open file will not turn into a directory, symlink, or anything else, *as seen by* the open file descriptor. The file may be moved or replaced, but the open file descriptor "follows" the move and does not alias the separate file that now has the original path. Because we only intend to add executable bits to regular files, it is no longer necessary to handle this as a race condition and keep going. Attempting to add +x to any other type of thing should now only be possible if there is a bug in the caller. So the check for that, and the associated Option<...> logic used to keep things testable, is not needed. This still does the check, but panics if the file descriptor does not represent a regular file. The tests of this are mostly removed, but two tests that it does panic for the kinds of non-regular files we are most likely to encounter are added. (It may eventually make sense check this and panic only in debug builds, or not at all.) There are now two `fstat` calls: the one that is added to check the `st_mode` to figure out what to pass to `fchmod`, and the preexisting higher level invocation (that delegates to `fstat`) done just before closing the file, to update `entry.stat`. Using the same call for both and accounting for any effect of `fchmod` might be more complicated, but more importantly, it does not seem like it would be correct. If attempting to set the permissions reports success but has an unexpected effect, perhaps due to restrictions of the underlying filesystem or how it is mounted (which we may not be guarnateed to have detected in a probe to decide if +x is supported), we would want the real stat. Because this no longer uses `Permissions` and `PermissionsExt` facilities to check the mode before adding executable bits or to add the executable bits, it also includes some refactoring that is possible now that they are not being used. --- gix-worktree-state/src/checkout/entry.rs | 170 ++++++++++------------- 1 file changed, 77 insertions(+), 93 deletions(-) diff --git a/gix-worktree-state/src/checkout/entry.rs b/gix-worktree-state/src/checkout/entry.rs index a7fa5d9760b..d1b6486d213 100644 --- a/gix-worktree-state/src/checkout/entry.rs +++ b/gix-worktree-state/src/checkout/entry.rs @@ -285,16 +285,9 @@ pub(crate) fn finalize_entry( // For possibly existing, overwritten files, we must change the file mode explicitly. #[cfg(unix)] 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 we keep `fchmod`, maybe `set_mode_executable` shouldn't use `std::fs::Permissions`. - use std::os::unix::fs::PermissionsExt; - #[allow(clippy::useless_conversion)] // mode_t is u32 on many but not all OSes. It's u16 on macOS. - let raw_mode = new_perm.mode().try_into().expect("mode fits in `st_mode`"); - let mode = rustix::fs::Mode::from_bits(raw_mode) - .expect("`set_mode_executable` shouldn't preserve or add unknown bits"); - rustix::fs::fchmod(&file, mode).map_err(std::io::Error::from)?; - } + let old_raw_mode = rustix::fs::fstat(&file).map_err(std::io::Error::from)?.st_mode; + let new_mode = let_readers_execute(old_raw_mode); + rustix::fs::fchmod(&file, new_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. // revisit this once there is a bug to fix. @@ -303,102 +296,93 @@ pub(crate) fn finalize_entry( Ok(()) } +/// Given the st_mode of a regular file, compute the mode with executable bits safely added. +/// +/// 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. #[cfg(unix)] -fn set_mode_executable(mut perm: std::fs::Permissions) -> Option { - use std::os::unix::fs::PermissionsExt; - let mut mode = perm.mode(); - if mode & 0o170000 != 0o100000 { - return None; // Stop if we don't have a regular file anymore. - } - mode &= 0o777; // Clear non-rwx bits (setuid, setgid, sticky). - mode |= (mode & 0o444) >> 2; // Let readers also execute. - perm.set_mode(mode); - Some(perm) +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") } #[cfg(all(test, unix))] mod tests { - fn pretty(maybe_mode: Option) -> String { - match maybe_mode { - Some(mode) => format!("Some({mode:04o})"), - None => "None".into(), - } - } - #[test] - fn set_mode_executable() { + fn let_readers_execute() { let cases = [ // Common cases: - (0o100755, Some(0o755)), - (0o100644, Some(0o755)), - (0o100750, Some(0o750)), - (0o100640, Some(0o750)), - (0o100700, Some(0o700)), - (0o100600, Some(0o700)), - (0o100775, Some(0o775)), - (0o100664, Some(0o775)), - (0o100770, Some(0o770)), - (0o100660, Some(0o770)), - (0o100764, Some(0o775)), - (0o100760, Some(0o770)), + (0o100755, 0o755), + (0o100644, 0o755), + (0o100750, 0o750), + (0o100640, 0o750), + (0o100700, 0o700), + (0o100600, 0o700), + (0o100775, 0o775), + (0o100664, 0o775), + (0o100770, 0o770), + (0o100660, 0o770), + (0o100764, 0o775), + (0o100760, 0o770), // Less common: - (0o100674, Some(0o775)), - (0o100670, Some(0o770)), - (0o100000, Some(0o000)), - (0o100400, Some(0o500)), - (0o100440, Some(0o550)), - (0o100444, Some(0o555)), - (0o100462, Some(0o572)), - (0o100242, Some(0o252)), - (0o100167, Some(0o177)), + (0o100674, 0o775), + (0o100670, 0o770), + (0o100000, 0o000), + (0o100400, 0o500), + (0o100440, 0o550), + (0o100444, 0o555), + (0o100462, 0o572), + (0o100242, 0o252), + (0o100167, 0o177), // With set-user-ID, set-group-ID, and sticky bits: - (0o104755, Some(0o755)), - (0o104644, Some(0o755)), - (0o102755, Some(0o755)), - (0o102644, Some(0o755)), - (0o101755, Some(0o755)), - (0o101644, Some(0o755)), - (0o106755, Some(0o755)), - (0o106644, Some(0o755)), - (0o104750, Some(0o750)), - (0o104640, Some(0o750)), - (0o102750, Some(0o750)), - (0o102640, Some(0o750)), - (0o101750, Some(0o750)), - (0o101640, Some(0o750)), - (0o106750, Some(0o750)), - (0o106640, Some(0o750)), - (0o107644, Some(0o755)), - (0o107000, Some(0o000)), - (0o106400, Some(0o500)), - (0o102462, Some(0o572)), - // Where it was replaced with a directory due to a race: - (0o040755, None), - (0o040644, None), - (0o040600, None), - (0o041755, None), - (0o041644, None), - (0o046644, None), - // Where it was replaced with a symlink due to a race: - (0o120777, None), - (0o120644, None), - // Where it was replaced with some other non-regular file due to a race: - (0o140644, None), - (0o060644, None), - (0o020644, None), - (0o010644, None), + (0o104755, 0o755), + (0o104644, 0o755), + (0o102755, 0o755), + (0o102644, 0o755), + (0o101755, 0o755), + (0o101644, 0o755), + (0o106755, 0o755), + (0o106644, 0o755), + (0o104750, 0o750), + (0o104640, 0o750), + (0o102750, 0o750), + (0o102640, 0o750), + (0o101750, 0o750), + (0o101640, 0o750), + (0o106750, 0o750), + (0o106640, 0o750), + (0o107644, 0o755), + (0o107000, 0o000), + (0o106400, 0o500), + (0o102462, 0o572), ]; - for (old_mode, expected) in cases { - use std::os::unix::fs::PermissionsExt; - let old_perm = std::fs::Permissions::from_mode(old_mode); - let actual = super::set_mode_executable(old_perm).map(|perm| perm.mode()); + for (st_mode, raw_expected) in cases { + let expected = rustix::fs::Mode::from_bits(raw_expected).expect("expected mode is a mode"); + let actual = super::let_readers_execute(st_mode); assert_eq!( - actual, - expected, - "{old_mode:06o} should become {}, became {}", - pretty(expected), - pretty(actual) + actual, expected, + "{st_mode:06o} should become {expected:04o}, became {actual:04o}" ); } } + + #[test] + #[should_panic] + fn let_readers_execute_panics_on_directory() { + super::let_readers_execute(0o040644); + } + + #[test] + #[should_panic] + fn let_readers_execute_should_panic_on_symlink() { + super::let_readers_execute(0o120644); + } } From fc82e786cc88a69ca6cfde665339e4a48dad5484 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 23 Jan 2025 22:54:57 -0500 Subject: [PATCH 6/8] Extract `fstat` and `fchmod` logic to a helper This extracts the logic that calls `fstat`, transforms the mode with `let_readers_execute`, and writes it back through the same file descriptor with `fchmod`, introducing a new helper function, `set_executable`, for it. The reason to do this is that it makes the issue of what kind of `Error` appears in the returned `Result` clearer. This may be slightly clearer to humans. It's significnatly clearer to the Rust compiler, allowing both `map_err(std::io::Error::from)` calls to be eliminated without having to write anything confusing or inelegant. (This is separate from the reason the `let_readers_execute` function, which does not access the filesystem, is its own function. That is so it's easy to test the mode transformation logic.) --- gix-worktree-state/src/checkout/entry.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/gix-worktree-state/src/checkout/entry.rs b/gix-worktree-state/src/checkout/entry.rs index d1b6486d213..f12b4a94728 100644 --- a/gix-worktree-state/src/checkout/entry.rs +++ b/gix-worktree-state/src/checkout/entry.rs @@ -285,9 +285,7 @@ pub(crate) fn finalize_entry( // For possibly existing, overwritten files, we must change the file mode explicitly. #[cfg(unix)] if let Some(path) = set_executable_after_creation { - let old_raw_mode = rustix::fs::fstat(&file).map_err(std::io::Error::from)?.st_mode; - let new_mode = let_readers_execute(old_raw_mode); - rustix::fs::fchmod(&file, new_mode).map_err(std::io::Error::from)?; + set_executable(&file)?; } // NOTE: we don't call `file.sync_all()` here knowing that some filesystems don't handle this well. // revisit this once there is a bug to fix. @@ -296,6 +294,17 @@ pub(crate) fn finalize_entry( Ok(()) } +/// Use `fstat` and `fchmod` on a file descriptor to make a regular file executable. +/// +/// 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)?; + Ok(()) +} + /// Given the st_mode of a regular file, compute the mode with executable bits safely added. /// /// Currently this adds executable bits for whoever has read bits already. It doesn't use the umask. From 588e6b01327abfbd6bee8900fc1882ef82744c19 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 23 Jan 2025 23:37:38 -0500 Subject: [PATCH 7/8] Make `set_executable_after_creation` everywhere `bool` In `finalize_entry` and its callers, `set_executable_after_creation` was previously an `Option<&Path>`, because `chmod` was called (via a higher level abstraction) on the path to make the file executable. From #1764, `lstat` (via a higher level abstraction) was likewise called on the path to find out what its old mode was, to figure out what mode to set to achieve +x. Now that the `lstat` is replaced with `fstat`, and the `chmod` is replaced with `fchmod`, figuring out how to set +x and setting it are done entirely on the file descriptor, with the path unused. It turns out that this was the only use of that path in that context. Accordingly, this makes `set_executable_after_creation` a `bool` in both `finalize_entry` and its callers, and removes code that was only used for handling that path. This includes eliminating the `rela_path_as_path` variable in `checkout::chunk::process_delayed_filter_results`, which was apparently only used for this purpose. (All writes and reads on it were for preparing and passing a path to `finalize_entry`. The `finalize_entry` function had only used it to set the file executable, and had documented that this was its purpose.) --- gix-worktree-state/src/checkout/chunk.rs | 6 +----- gix-worktree-state/src/checkout/entry.rs | 9 ++++----- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/gix-worktree-state/src/checkout/chunk.rs b/gix-worktree-state/src/checkout/chunk.rs index 7f23722f59e..92663ae48d6 100644 --- a/gix-worktree-state/src/checkout/chunk.rs +++ b/gix-worktree-state/src/checkout/chunk.rs @@ -177,7 +177,6 @@ where // We process each key and do as the filter process tells us, while collecting data about the overall progress. let keys: BTreeSet<_> = delayed_filter_results.iter().map(|d| d.key.clone()).collect(); let mut unknown_paths = Vec::new(); - let mut rela_path_as_path = Default::default(); for key in keys { loop { let rela_paths = ctx.filters.driver_state_mut().list_delayed_paths(&key)?; @@ -229,10 +228,7 @@ where entry::finalize_entry( delayed.entry, write.inner.into_inner().map_err(std::io::IntoInnerError::into_error)?, - set_executable_after_creation.then(|| { - rela_path_as_path = gix_path::from_bstr(delayed.entry_path); - rela_path_as_path.as_ref() - }), + set_executable_after_creation, )?; delayed_files += 1; files.fetch_add(1, Ordering::Relaxed); diff --git a/gix-worktree-state/src/checkout/entry.rs b/gix-worktree-state/src/checkout/entry.rs index f12b4a94728..0ddf97a6641 100644 --- a/gix-worktree-state/src/checkout/entry.rs +++ b/gix-worktree-state/src/checkout/entry.rs @@ -135,7 +135,7 @@ where }; // For possibly existing, overwritten files, we must change the file mode explicitly. - finalize_entry(entry, file, set_executable_after_creation.then_some(dest))?; + finalize_entry(entry, file, set_executable_after_creation)?; num_bytes } gix_index::entry::Mode::SYMLINK => { @@ -275,16 +275,15 @@ pub(crate) fn open_file( try_op_or_unlink(path, overwrite_existing, |p| options.open(p)).map(|f| (f, set_executable_after_creation)) } -/// Close `file` and store its stats in `entry`, possibly setting `file` executable depending on -/// `set_executable_after_creation`. +/// Close `file` and store its stats in `entry`, possibly setting `file` executable. pub(crate) fn finalize_entry( entry: &mut gix_index::Entry, file: std::fs::File, - #[cfg_attr(windows, allow(unused_variables))] set_executable_after_creation: Option<&Path>, + #[cfg_attr(windows, allow(unused_variables))] set_executable_after_creation: bool, ) -> Result<(), crate::checkout::Error> { // For possibly existing, overwritten files, we must change the file mode explicitly. #[cfg(unix)] - if let Some(path) = set_executable_after_creation { + if set_executable_after_creation { set_executable(&file)?; } // NOTE: we don't call `file.sync_all()` here knowing that some filesystems don't handle this well. From ee7b10cf8a9ab1367ed7079dd0cef7400f55a36a Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 23 Jan 2025 23:47:52 -0500 Subject: [PATCH 8/8] Thanks clippy --- 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 0ddf97a6641..8df37947113 100644 --- a/gix-worktree-state/src/checkout/entry.rs +++ b/gix-worktree-state/src/checkout/entry.rs @@ -298,9 +298,9 @@ 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 old_raw_mode = rustix::fs::fstat(file)?.st_mode; let new_mode = let_readers_execute(old_raw_mode); - rustix::fs::fchmod(&file, new_mode)?; + rustix::fs::fchmod(file, new_mode)?; Ok(()) }