feat: Add Repository::lock_repo #163
Conversation
aawsome
changed the title
aawsome
force-pushed
the
lock
branch
3 times, most recently
from
b468d07
to
0fea41b
Compare
aawsome
force-pushed
the
lock
branch
3 times, most recently
from
bed411c
to
14488d4
Compare
aawsome
changed the title
aawsome
changed the title
aawsome
changed the title
crates/core/src/backend.rs
Outdated
| fn lock(&self, tpe: FileType, id: &Id, until: Option<DateTime<Local>>) -> Result<()> { | ||
| debug!("no locking implemented. {tpe:?}, {id}, {until:?}"); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
I would check here for ::can_lock() and if that is true and ::lock() is being called in the default implementation, I would panic!() or unimplemented!("no locking implemented") as otherwise someone could actually think, they can use this and won't reimplement it. It should definitely not silently continue here.
if self.can_lock() {
unimplemented!("no locking implemented");
} There was a problem hiding this comment.
Especially because it doesn't implement the expected behaviour.
e.g. how axum does it: https://github.com/tokio-rs/axum/releases/tag/axum-v0.8.0-alpha.1
breaking: Upgrade matchit to 0.8, changing the path parameter syntax from /:single and /*many to /{single} and /{*many}; the old syntax produces a panic to avoid silent change in behavior (#2645)
| true | ||
| } | ||
|
|
||
| fn lock(&self, tpe: FileType, id: &Id, until: Option<DateTime<Local>>) -> Result<()> { |
There was a problem hiding this comment.
::lock() should never be callable/running its body, if ::can_lock() isn't returning true, I think?
There was a problem hiding this comment.
A type state design might be better suited here
There was a problem hiding this comment.
But we do have a problem with the type state pattern here: The Repository traits need to be object save as we do create dynamic trait objects here.
Do you know a way to implement a type state pattern in combination with object safety?
| if !repo.be.can_lock() { | ||
| return Err(CommandErrorKind::NoLockingConfigured.into()); | ||
| } | ||
|
|
There was a problem hiding this comment.
I see we check that here, but I think this should be checked within the ::lock() itself, so people using it, can't forget to call it.
crates/core/src/commands/lock.rs
Outdated
| if let Err(e) = backend.lock(ID::TYPE, id, until) { | ||
| // FIXME: Use error handling | ||
| error!("lock failed for {:?} {id:?}. {e}", ID::TYPE); | ||
| }; |
There was a problem hiding this comment.
Is it actually enough to just log an error here? If something like that lock can't be acquired, I would throw an error actually, so the command is not being run, right? The expectation on a function, that is called lock_files should be that is errors out, when it can't lock the files, and not just logs an error I feel.
There was a problem hiding this comment.
This is the same error handling discussion as in many places: The best would here to log an error, continue and collect the errors and at the end fail with the list of errors...
There was a problem hiding this comment.
Yeah :/ But keeping it running and not failing early also opens room up for possible bugs I think that could affect repository integrity? (in general, not regarding the locking discussion).
When you say at the end, what do you mean by that. Just in the end of the program's run? Or at the end of the function?
Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>
|
I pushed some of my proposed changes in a commit, to better explain what I mean. I'm not so sure about the design between |
Adds
Repository::lock_repowhich locks all repository files. Also the backend trait is extended to support locking and alock_commandhas been added toRepositoryOptionswhich turns any backend into a backend capable of locking.see rustic-rs/rustic#1050
Missing points: