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

Improve zip errors when paths are not utf8 valid #181

Merged
merged 3 commits into from
Nov 10, 2021

Conversation

marcospb19
Copy link
Member

No description provided.

@marcospb19
Copy link
Member Author

if !invalid_unicode_filenames.is_empty() {
    let error = FinalError::with_title("Cannot build zip archive")
        .detail("Zip archives require files to have valid UTF-8 paths")
        .detail(format!("Files with invalid paths: {}", concatenate_os_str_list(&invalid_unicode_filenames)));

    return Err(error.into());
}

@GabrielSimonetto this is an error produced inside of the archive logic, but we don't have the name of the path that is being outputted, I just had an new idea!

What if we grab the Result<_, Error> and call a function on it before propagating up.

What this function would do: check if it is Error::Custom, and add a .detail() line to it, then returning with ?. This way we could add the name of the output_path to an FinalError that was created INSIDE of the archive functions.

This idea is meant to solve #131.

@marcospb19 marcospb19 merged commit 7f97715 into master Nov 10, 2021
@GabrielSimonetto
Copy link
Collaborator

Hmm, but I feel like we need to go back to the caller to know which was the output_path, but you said:

What if we grab the Result<_, Error> and call a function on it before propagating up.

How will a function inside this context be able to grab the output_path?

@marcospb19 marcospb19 deleted the improve-zip-non-utf8-errors branch November 13, 2021 13:59
@marcospb19
Copy link
Member Author

marcospb19 commented Nov 13, 2021

How will a function inside this context be able to grab the output_path?

Would be done with a function outside.

Inside of the zip function:

if !invalid_unicode_filenames.is_empty() {
    let error = FinalError::with_title("Cannot build zip archive")
        .detail("Zip archives require files to have valid UTF-8 paths")
        .detail(format!("Files with invalid paths: {}", concatenate_os_str_list(&invalid_unicode_filenames)));

    return Err(error.into());
}

Outside currently:

archive::zip::build_archive_from_paths(&files, &mut vec_buffer)?;

But could be like:

archive::zip::build_archive_from_paths(&files, &mut vec_buffer)
    .map_err(|err| add_detail(err, format!("Occurred in {}", output_path.display())))?;

@marcospb19
Copy link
Member Author

But I'm convinced that we don't really need this for compression, because there will always just be one compression output file.

But for extracting, we need to say the name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants