From 0becc913c57afd8a0bf95e97c14ba1130f662f1a Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 21 Jan 2025 06:02:15 -0500 Subject: [PATCH] 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.