[nardoor]

This is a feature request from #398

[codecov]

Codecov Report

Attention: Patch coverage is 60.46512% with 17 lines in your changes missing coverage. Please review.

Project coverage is 42.7%. Comparing base (64d052d) to head (6d780b9).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/core/src/repofile/snapshotfile.rs	63.4%	15 Missing ⚠️
crates/core/src/commands/copy.rs	0.0%	2 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
crates/core/src/backend.rs	59.7% <ø> (+6.3%)	⬆️
crates/core/src/id.rs	54.0% <ø> (+2.7%)	⬆️
crates/core/src/commands/copy.rs	0.0% <0.0%> (ø)
crates/core/src/repofile/snapshotfile.rs	50.1% <63.4%> (+0.5%)	⬆️

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[aawsome]

I commited some small optimazations.

There is however one thing we should take into account: The Snapshotfile::from_str method is not used by all rustic commands. Especially rustic diff uses another method only accepting (multiple) ids which will not be affected by this change. Also rustic snapshots latest uses a method which groups snapshots and looks for the latest of each group - which is also not affected by this change.

So, to get a complete support we need to add this to other methods - why I strongly recommend we refactor this (and generally the snapshots handling) into smaller pieces which can be reused..

[nardoor]

Hello,

I still have in mind the refactor we talked about. I know how I'd do it, just have to take a moment to propose it.

Also I agree that snapshot identification for the user should be normalized for all commands!
I'll take a look at the different commands in a PR in rustic after this one is done.

[aawsome]

Hello,

I still have in mind the refactor we talked about. I know how I'd do it, just have to take a moment to propose it.

Also I agree that snapshot identification for the user should be normalized for all commands! I'll take a look at the different commands in a PR in rustic after this one is done.

Please note that it is not that easy: for some commands like rustic snapshots or rustic tag etc. it might be desirable to specify multiple snapshots. And it would be very inefficient to load them by looping over the current from_str method - for something like latest* this involves loading and parsing all snapshots!

So, yes, we need to take a look at the rustic commands and uniformly treat snapshot parsing+loading. But we also need to implement the needed algorithms to efficiently support a request of multiple snapshots - including something like latest abc123 <FULL_HEX> latest~5 - which we currently don't have.

[nardoor]

Thank you for the inputs on snapshot identification uniformization potential difficulties.

When you mention that using latest_n parses all snapshots, I understand that you mean all snapshots metadatas, is that correct?

[aawsome]

@nardoor can you please review the commits I added? Thanks!

[nardoor]

Overall to me it looks good.
The SnapshotRequest is still private to snapshotfile.rs for now. Tell me if I'm wrong but my understanding is that we plan to use to create APIs for rustic commands?

[aawsome]

The SnapshotRequest is still private to snapshotfile.rs for now. Tell me if I'm wrong but my understanding is that we plan to use to create APIs for rustic commands?

Yes. The question is if we want to just internally use SnapshotRequest, i.e. have APIs which require something like &[&str] - or if we want to use something like &[SnapshotRequest] in the API. I thought about the first, but rethinking it: using the second and a method to parse a &str or &[&str] into Vec<SnapshotRequest> might be an even better API as it allows to treat parse errors differently. But I think we should also do this in a follow-up PR.

[aawsome]

LGTM