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.

Sign up for GitHub

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

Jump to bottom

Fix empty filename case #204

Merged
merged 3 commits into from
Mar 10, 2024
Merged

Fix empty filename case #204

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
27521c4
Fix empty filename case
PaDarochek Mar 6, 2024
f1c99b8
Fix canonicalize
PaDarochek Mar 6, 2024
6a706c8
Add comments
PaDarochek Mar 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions casr/src/bin/casr-cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -824,12 +824,16 @@
.map(|res| res.unwrap().path())
.any(|e| e.extension().is_some() && e.extension().unwrap() == "casrep")
{
clusters.push((dir.to_path_buf(), 0));
SweetVishnya marked this conversation as resolved.
Show resolved Hide resolved
clusters.push((dir.canonicalize().unwrap().to_path_buf(), 0));
}

for (clpath, _) in clusters {
let cluster = clpath.as_path();
let filename = cluster.file_name().unwrap().to_str().unwrap();
let filename = if let Some(cl_filename) = cluster.file_name() {
cl_filename.to_str().unwrap()
} else {
cluster.to_str().unwrap()

Check warning on line 835 in casr/src/bin/casr-cli.rs

View check run for this annotation

Codecov / codecov/patch

casr/src/bin/casr-cli.rs#L835 

Added line #L835 was not covered by tests
Copy link
Collaborator

@SweetVishnya SweetVishnya Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

why should we use full path here?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the filename is empty it means that it ends with '.' or is '/' and we couldn't canonicalize it. Then we have no other options besides using this path as is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the filename is empty it means that it ends with '.' or is '/' and we couldn't canonicalize it. Then we have no other options besides using this path as is.

Could you provide some use cases, when and why this situation happens?
It seems to me, that this code filter directories.

Copy link
Member Author

@PaDarochek PaDarochek Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The case is when casr-cli is called with an argument of directory name, and that directory contains raw casreps. For example, if casreps are located in root directory (a poked-out case, but still), or in cwd '.' and we couldn't canonicalize it for some reason. The bug was detected when calling casr-cli on cwd with raw reports.
This code adds directory to clusters if it contains raw reports.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a comment in code stating this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

};

// Ubsan indicator for minimize logging
let mut ubsan = true;
Expand Down
Loading