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

feat: Add Builder::permissions() method. #273

Merged
merged 5 commits into from
Feb 5, 2024

Conversation

Byron
Copy link
Contributor

@Byron Byron commented Feb 4, 2024

This PR allows the user to control the file mode set for newly created NameTempfile instances.

This is paramount for when tempfiles are used as scratch space that will be persisted and renamed into the final destination once all writes are completed. This is an effective way to prevent half-written files on disk and to assure that readers can only observe the final result of a change.

This paradigm is implemented in the gix-lock crate which is used by the gix-ref crate for editing Git references. Currently these are created with 0o600 permissions, which can cause issues when using sudo in downstream applications.

When this PR is merged, gix-ref can be adjusted to allow setting the mode just like Git does.

Review Notes

I did my best to adapt the least amount of code while staying true to the current organisation. Initially I named the new method mode, but realized that std::fs::Permissions exists which would allow the API to be more general, despite only one available platform for implementation.

Let me explain the commits in some detail and why it's one more than needed:

  • the first commit is a failing test that adds the new Builder method without an implementation. CI is red to show it's not working.
  • the second commits implements it to be least invasive, which violates the principle that platform dependent code should remain in imp. CI is green.
  • the third commit implements it so that permissions can be passed down to the platforms, which is more invasive, yet cleaner. CI is also green.

Please let me know which way you prefer as its also a trade-off.

With it it's possible to change the default mode of tempfiles on
unix systems.

The example also doesn't hide the fact that it is currently only
useful on Unix, but as the ecosystem matures, thanks to the usage
of `std::fs::Permissions` it should be able grow into more platforms
over time.
The implementation favors the least invasive solution even though
its forced to pull platform dependent code up.

The alternative would have been to alter the method signatures
of `create_named()` on all platforms.
This way, permissions can be passed and handled by platform specific
code, which avoid having to use platform dependent code in the top-level.
@NobodyXu

This comment was marked as off-topic.

@Stebalien
Copy link
Owner

@NobodyXu I agree if you're only target is Linux, but it's still useful to have a platform-independent way to do this (to, e.g., support MacOS which hasn't seen any filesystem API improvements in, basically ever).

@Stebalien Stebalien marked this pull request as ready for review February 4, 2024 18:48
src/lib.rs Outdated
Comment on lines 427 to 428
/// Permissions for directories aren't currently set even though it would
/// be possible on Unix systems.
Copy link
Owner

Choose a reason for hiding this comment

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

We need to implement this before we merge this patch, unfortunately. This builder is used to create temporary directories as well, and I don't really trust users to read the docs.

Copy link
Owner

Choose a reason for hiding this comment

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

See the discussion in #61.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a look at this and saw that std::fs::create_dir() is used to create the directory, I don't think the standard library has a way to handle this.
Did you have an idea on how to do this? Maybe a rustix chmod call?

Copy link
Owner

Choose a reason for hiding this comment

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

I think we'll need to use rustix::fs::mkdir.

Copy link
Owner

Choose a reason for hiding this comment

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

This comment is now out of date.

src/lib.rs Outdated Show resolved Hide resolved
@Stebalien
Copy link
Owner

Thanks! And yes, I prefer the final version (more invasive approach).

@Byron Byron force-pushed the permissions branch 2 times, most recently from 8bb3386 to c0c925d Compare February 4, 2024 19:45
@Byron
Copy link
Contributor Author

Byron commented Feb 4, 2024

Thanks for the quick review! If CI passes I have addressed the windows portion of it, but will definitely need some guidance to deal with the directory. If you have ideas and want to tinker, please feel free to push into this branch - sometimes that's easiest.

@NobodyXu Thanks for sharing! It's definitely something to consider for the time when performance needs to be optimised further. Personally I am most interested in doing what Git does at first, which seems to be pretty much what's happening here.

That way, there will be no surprises as would be the case with
doing nothing.
@Byron Byron force-pushed the permissions branch 2 times, most recently from 8a172f4 to 19ec472 Compare February 4, 2024 20:58
@Byron
Copy link
Contributor Author

Byron commented Feb 4, 2024

I have added mode-support for directories thanks to your hint with rustix::fs::mkdir and it appears to work. Further, trying to change permissions on unsupported platforms will result in an error that as well.
When CI is green it's ready for review, with the remark that I kept dir.rs nimble, with a structure sufficient for the task even though it's not quite the same as in file.

src/dir.rs Outdated Show resolved Hide resolved
src/dir.rs Outdated Show resolved Hide resolved
src/dir.rs Outdated
imp::create(path, permissions)
}

mod imp {
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer it if we created the entire directory structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that, even though it's a change that might have performance implications. For that reason alone I wouldn't do it without also introducing a new method for it, and do so in a separate PR.

Copy link
Owner

Choose a reason for hiding this comment

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

Wait, no. I mean: put the modules in different files/directories (imp/...). Sorry, I realize that that was confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, right, I could have guessed it. And it's done now. A note regarding naming: It feels like any should be other, but I decided not to do that as other means unsupported in the file folder, while here it is a catch-all.

Copy link
Owner

Choose a reason for hiding this comment

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

Hm. Yeah, I may go through and do a rename later but those are implementation details so.. meh.

@Stebalien
Copy link
Owner

So... somehow I missed this but: https://doc.rust-lang.org/std/fs/struct.DirBuilder.html#method.mode. We don't need to use rustix here.

@Byron
Copy link
Contributor Author

Byron commented Feb 5, 2024

So... somehow I missed this but: https://doc.rust-lang.org/std/fs/struct.DirBuilder.html#method.mode. We don't need to use rustix here.

Oh my, I missed that too! I clearly was biased when researching this topic and "didn't believe in mode being available for directories" 🤦‍♂️. Lesson learned, and the code was adjusted to something much better.

I made a note that recommends to not recursively create the directory by default but leave that to a separate PR along with a new flag in the builder maybe to control this, to keep the previous behaviour exactly the same. Right now, this still is the case.

If CI is green, this PR should be ready for re-review.

src/error.rs Outdated
Comment on lines 5 to 7
pub(crate) struct PathError {
pub(crate) path: PathBuf,
pub(crate) err: io::Error,
Copy link
Owner

Choose a reason for hiding this comment

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

This change can be revered.

}
dir_options
.create(&path)
.with_err_path(|| path.clone())
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
.with_err_path(|| path.clone())
.with_err_path(|| &path)

(the path is cloned automatically if there's an error)

src/lib.rs Outdated
Comment on lines 427 to 428
/// Permissions for directories aren't currently set even though it would
/// be possible on Unix systems.
Copy link
Owner

Choose a reason for hiding this comment

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

This comment is now out of date.

src/dir.rs Outdated
imp::create(path, permissions)
}

mod imp {
Copy link
Owner

Choose a reason for hiding this comment

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

Hm. Yeah, I may go through and do a rename later but those are implementation details so.. meh.

@Stebalien
Copy link
Owner

And with those changes, this should be good to go. Thanks!

@Byron
Copy link
Contributor Author

Byron commented Feb 5, 2024

Thanks a lot for the QA and sorry for the sloppiness, at least 2 of 3 I should have caught.
But here we are, and I think that's the version that is ready to go.

PS: A new release with these changes would help, as that way everything can trickle downstream to StackedGit. Thanks again.

Copy link
Owner

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

❤️

@Stebalien Stebalien merged commit 4a05e47 into Stebalien:master Feb 5, 2024
13 checks passed
@Stebalien
Copy link
Owner

Thanks a lot for the QA and sorry for the sloppiness, at least 2 of 3 I should have caught.

Don't be. That'll just make me feel worse about my PRs and that's why we have review 😆.

PS: A new release with these changes would help, as that way everything can trickle downstream to StackedGit. Thanks again.

Will do.

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.

3 participants