store: Add accessors for the ostree repo #1509
Conversation
To make clearer what it is vs the other two storage types. In practice of course *most* acccesses to this are via the Deref, but that's a much bigger patch that will be a followup to this. Signed-off-by: Colin Walters <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Storage struct by renaming the sysroot field to ostree and introducing accessor methods for it. This improves encapsulation and clarity. The changes are applied consistently across the codebase.
I have one suggestion to further improve the new accessor methods by simplifying their signatures, which would make them more idiomatic and easier to use.
| /// Access the underlying ostree repository | ||
| pub(crate) fn get_ostree(&self) -> Result<&SysrootLock> { | ||
| Ok(&self.ostree) | ||
| } | ||
|
|
||
| /// Access the underlying ostree repository | ||
| pub(crate) fn get_ostree_cloned(&self) -> Result<ostree::Sysroot> { | ||
| let r = self.get_ostree()?; | ||
| Ok((*r).clone()) | ||
| } |
There was a problem hiding this comment.
These accessor functions can be simplified. Since neither get_ostree nor get_ostree_cloned can fail in their current implementation, they shouldn't return a Result.
Additionally, (*r).clone() in get_ostree_cloned is a bit confusing. It can be simplified to self.get_ostree().clone() after changing get_ostree's signature.
This change would make the API cleaner and more idiomatic, and simplify call sites by removing unnecessary ? operators.
/// Access the underlying ostree repository
pub(crate) fn get_ostree(&self) -> &SysrootLock {
&self.ostree
}
/// Access the underlying ostree repository
pub(crate) fn get_ostree_cloned(&self) -> ostree::Sysroot {
self.get_ostree().clone()
}There was a problem hiding this comment.
Yes the use of Result is intentional here because I want to change it to be a fallible operation in the future.
|
It will definitely get a bit wild in The nomenclature update is helpful for that, but maybe this file can be broken up a bit with some methods specific to their backend living in their own files? |
| @@ -42,7 +42,7 @@ pub(crate) struct Storage { | |||
| pub physical_root: Dir, | |||
|
|
|||
| /// The OSTree storage | |||
| pub sysroot: SysrootLock, | |||
| ostree: SysrootLock, | |||
There was a problem hiding this comment.
Right, the change in scope here was indeed intentional because your helper functions here are now doing the cloning, rather than having them cloned in deploy.rs, I see.
Yes this is preparatory work for that. |
To make clearer what it is vs the other two storage types.
In practice of course most acccesses to this are via the Deref, but that's a much bigger patch that will be a followup to this.