refactor restore command #741
Conversation
| } | ||
|
|
||
| /// collect restore information, scan existing files and remove superfluous files | ||
| pub(crate) fn collect_and_remove<P: ProgressBars, S: Indexed>( |
There was a problem hiding this comment.
Could we split up this method into something that collects and something that processes? It's currently nearly 200 LOC, which makes it hard to test in the future and also not easy to maintain, I feel.
There was a problem hiding this comment.
Yes, agree that the restore command is pretty complex on one hand and pretty optimized on the other hand...
I also thought about improving it, but then decided to do minimal changes in order to present the command in the API.
Can we work on the restore command in future?
Yes, it is very ugly that there is no possible repo method which would just analyze what needs to be done. But for instance regarding the removal of existing files, you have two alternatives:
- save the files to-remove in some kind of list (which can be very memory consuming if you delete a lot by this command)
- scan the destination (a the repository tree) a second time which can be quite costly.
I think refactoring this would need a bit more thinking and hence I don't want to do this parallel to the current task.
There was a problem hiding this comment.
Yeah, probably we should write unit tests when refactoring it into smaller parts then later!
|
|
||
| /// [`restore_contents`] restores all files contents as described by `file_infos` | ||
| /// using the [`DecryptReadBackend`] `be` and writing them into the [`LocalBackend`] `dest`. | ||
| fn restore_contents<P: ProgressBars, S: Open>( |
There was a problem hiding this comment.
Same comment, could we split this one as well?
There was a problem hiding this comment.
Wouldn't do this with this change. This function is basically just moved from the CLI crate where it was as this. At least now this function is very local within rustic_core and can be easily refactored.
| // TODO: This is the same logic as in backend/ignore.rs => consollidate! | ||
| let mtime = meta | ||
| .modified() | ||
| .ok() | ||
| .map(|t| DateTime::<Utc>::from(t).with_timezone(&Local)); | ||
| if meta.len() == file.meta.size && mtime == file.meta.mtime { | ||
| // File exists with fitting mtime => we suspect this file is ok! | ||
| debug!("file {name:?} exists with suitable size and mtime, accepting it!"); | ||
| self.matched_size += file.meta.size; | ||
| return Ok(AddFileResult::Existing); | ||
| } |
There was a problem hiding this comment.
Maybe this is a good candidate to split out as well.
aawsome
force-pushed
the
refactor-restore
branch
from
eaa31a5
to
2d36277
Compare
|
I added an example in the example dir. |
aawsome
force-pushed
the
refactor-restore
branch
from
2d36277
to
8e86f07
Compare
aawsome
force-pushed
the
refactor-restore
branch
2 times, most recently
from
abda6f1
to
fe6bd32
Compare
| ..Default::default() | ||
| }; | ||
|
|
||
| let repo = Repository::new(&repo_opts) |
There was a problem hiding this comment.
| let repo = Repository::new(&repo_opts) | |
| let repo = Repository::with_options(&repo_opts) |
Maybe it's better to make it explicit?
| .unwrap(); | ||
|
|
||
| // use latest snapshot without filtering snapshots | ||
| let node = repo.node_from_snapshot_path("latest", |_| true).unwrap(); |
There was a problem hiding this comment.
These parameters could be a reasonable default. Like latest snapshot without filtering and may be a good candidate for an own method.
There was a problem hiding this comment.
IMO, it is actually vice versa: When you want the latest snapshot, you usually specify extra conditions of w.r.t. what you want the latest snapshot.
On the other hand, if you specify a snapshots by (part of the) ID, you usually don't need any filtering as the snapshot is already defined.
I think the use cases how to get to a certain node which you want to restore will vary a lot depending on the user interface you are using. If you have a CLI, the node_from_snapshot_path is quite handy as it allows to give a string and will automatically resolve ids or latest snapshots from the string.
If you have some kind of GUI, I would prefer just fetching all snapshots using repo.get_matching_snapshots() or even repo.get_all_snapshots() (not present yet but would be repo.get_matching_snapshots() without filtering). Then a user could manually dive into some tree and pick the Node to be used for the restore.
TL;DR: Yes, we need to think about the API, but here I wouldn't do so ;-)
There was a problem hiding this comment.
These notes are also meant as some future reference when we work on the API, by the way. Not really to change here, just so we are on the same boat (hence why I chose the comment instead of changes review feature).
For example, I would prefer both, get_all_snapshots() as a wrapper around (in this case) get_matching_snapshots() and get_latest_snapshot() etc.
| // use list of the snapshot contents using no additional filtering | ||
| let recursive = true; | ||
| let streamer_opts = TreeStreamerOptions::default(); | ||
| let ls = repo.ls(&node, &streamer_opts, recursive).unwrap(); | ||
|
|
||
| let destination = "./restore/"; // restore to this destination dir | ||
| let create = true; // create destination dir, if it doesn't exist | ||
| let dest = LocalDestination::new(destination, create, !node.is_dir()).unwrap(); | ||
|
|
||
| let opts = RestoreOpts::default(); | ||
| let dry_run = false; | ||
| // create restore infos. Note: this also already creates needed dirs in the destination | ||
| let restore_infos = repo | ||
| .prepare_restore(&opts, ls.clone(), &dest, dry_run) | ||
| .unwrap(); |
There was a problem hiding this comment.
I think this is overly verbose for the user-facing API. I think we should either replace all boolean values that are user-facing for these kinds of options with enums here, or wrap the booleans with a new type.
We could also use a DescriptorStruct that we pass to T.restore(), so we are able to have all the settings for the user in one place exposing certain values as reasonable defaults, could even combine that with a builder for the DescriptorStruct:
rust-unofficial/patterns#239 (comment)
aawsome
force-pushed
the
refactor-restore
branch
from
fe6bd32
to
e033ae9
Compare
No description provided.