Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get and set mode using std, still on open file descriptor #1811

Merged
merged 2 commits into from
Jan 26, 2025

Conversation

EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented Jan 26, 2025

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.

Background

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.

Rationale

While the above details can, in principle, change without warning, per 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.

This pull request intends to maintain the effect of #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.)

  • Use 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-#1803 behavior of using functions that operated on paths.

Relationship between functions that get and set mode bits

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

Future directions

It may be that some uses of rustix::fs facilities can be similarly replaced in gix_fs, but this PR does not include any such changes.

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.)

- Use 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.
The `let_readers_execute` function computes a new mode with `+x`
set reasonably based on an old mode. It is only useful on Unix-like
systems. But there may be a benefit to being able to detect more
bugs affecting one platform even on another; this can help catch
bugs slightly earlier in the development process.

Now that `let_readers_execute` only longer depends on a type that
varies across Unix-like systems and is only defined on such systems
(instead representing modes as `u32` on all of them), it no longer
contains anything that can't build or be tested everywhere. So this
lets its tests run on all systems, even non-Unix-like ones.
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastic, thank so much!

I particularly like the tables that show which parts of std correspond to which Unix calls respectively.

It also means that I don't know the std::fs::File API very well as I could have caught this in the previous PR - instead I was convinced that the std API still can't do it.

And it's true that other uses of rustix could use a review to see if they are equally covered by std.

And I'd hope I have now internalised it so I'd be able to catch such cases in future.

@Byron Byron merged commit f3dc83b into GitoxideLabs:main Jan 26, 2025
21 checks passed
@EliahKagan EliahKagan deleted the run-ci/fchmod-next branch January 26, 2025 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants