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

refactor(error): improve error messages and file handling #334

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

simonsan
Copy link
Contributor

@simonsan simonsan commented Oct 14, 2024

  • Create parent directory if it does not exist before opening the file for writing.

Fixes rustic-rs/rustic#1315
Fixes #310

@simonsan simonsan added A-errors Area: error handling needs improvement C-refactor Category: Refactoring of already existing code labels Oct 14, 2024
@simonsan simonsan requested a review from aawsome October 14, 2024 16:03
@simonsan simonsan added the S-waiting-for-review Status: PRs waiting for review label Oct 14, 2024
@simonsan
Copy link
Contributor Author

I kept:

        for i in 0u8..=255 {
            let filename = self.path.join("data").join(hex::encode([i]));
            fs::create_dir_all(&filename).map_err(|err| {
                LocalBackendErrorKind::DirectoryCreationFailed {
                    path: filename.clone(),
                    source: err,
                }
            })?;
        }

as that impacts compatibility with restic, and a repository without that, wouldn't be able to be read by it. Hence, I thought, we keep that, but we make sure, that also without these directories (in case someone deletes them), we are able to create and write to these directories. Not sure, how else we could handle that.

Copy link
Member

@aawsome aawsome left a comment

Choose a reason for hiding this comment

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

Good catch!
I have just found a small thing I would change in this PR...

crates/backend/src/local.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@171e2aa). Learn more about missing BASE report.

Files with missing lines Patch % Lines
crates/core/src/commands/check.rs 40.0% 3 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
crates/backend/src/local.rs 0.0% <ø> (ø)
crates/core/src/commands/check.rs 65.4% <40.0%> (ø)

@simonsan simonsan requested a review from aawsome October 16, 2024 13:30
@simonsan simonsan added S-blocked Status: Blocked from merging/working on due to some issue and removed S-waiting-for-review Status: PRs waiting for review labels Oct 24, 2024
@simonsan simonsan marked this pull request as draft October 24, 2024 01:19
@simonsan simonsan added this to the NEXT milestone Oct 30, 2024
- Update error messages for file operations in the `LocalBackendErrorKind` enum.
- Refactor the `ReadBackend` and `WriteBackend` implementations in the `LocalBackend` module to handle file opening errors more accurately.
- Add error variants `OpeningFileForPartialReadingFailed` and `OpeningFileForWritingFailed` to provide specific information about file opening failures.
- Create parent directory if it does not exist before opening the file for writing.

Fixes #rustic-rs/rustic#1315

Signed-off-by: simonsan <[email protected]>
Signed-off-by: simonsan <[email protected]>
@simonsan simonsan marked this pull request as ready for review November 17, 2024 02:09
@simonsan simonsan removed the S-blocked Status: Blocked from merging/working on due to some issue label Nov 17, 2024
@simonsan simonsan requested review from aawsome and removed request for aawsome November 17, 2024 02:12
@simonsan simonsan added the S-waiting-for-review Status: PRs waiting for review label Nov 17, 2024
Copy link
Contributor

@nardoor nardoor left a comment

Choose a reason for hiding this comment

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

Hello,

Looks good to me!
Just one question about an error log.

let data = match be.read_full(FileType::Pack, &id) {
Ok(data) => data,
Err(err) => {
error!("Error reading data for pack {id} : {err}");
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: should this error log be different from the one line 318?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-errors Area: error handling needs improvement C-refactor Category: Refactoring of already existing code S-waiting-for-review Status: PRs waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can we avoid creating empty folders when initializing a repo? Uncaught rclone errors causing rustic panic
3 participants