Remove leading slash from file name if path is absolute #53
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
==========================
==========================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
stillbeingnick
left a comment
stillbeingnick
left a comment
There was a problem hiding this comment.
Good stuff! But we also need to make sure we normalize dir paths and file paths. Both BSD and GNU tar strip leading '/' on paths for dirs and files.
Rework normalize_name to return
stillbeingnick
left a comment
stillbeingnick
left a comment
There was a problem hiding this comment.
Awesome! So this has the knock on effect of mirroring gnu and bsd that displays the "removing leading ..." message for each dir that needs to to be stripped.
|
Hi @stillbeingnick! What do you think, does it make sense to add a test for that feature too? |
Yes testing is always the way to go @zxvfc 👍 Adding a new test in |
stillbeingnick
left a comment
stillbeingnick
left a comment
There was a problem hiding this comment.
Minor things, 1 test name change and splitting 1 test apart (creating and extracting absolute path) for some better unit coverage. Right now we are really laying foundational things and I want our Tar to be built on concrete and not sand, so I really appreciate the participation, refactoring, and willingness!
tests/by-util/test_tar.rs
Outdated
| new_ucmd!() | ||
| .args(&["-xf", "archive.tar"]) | ||
| .current_dir(at.as_string()) | ||
| .succeeds(); | ||
|
|
||
| let expected_path = format!( | ||
| "{}{separator}{file_name}", | ||
| abs_path.strip_prefix(separator).unwrap() | ||
| ); | ||
| assert!(at.file_exists(expected_path)); |
There was a problem hiding this comment.
I appreciate the thoroughness of the test! But we need to break this out to another test since Creating and Extracting are two different operations. I want to try to keep testing separated at least for now by main operation modes. We will end up repeating some boilerplate for the testing (ie. creating the archive so we can extract it) but we can abstract and create correct dependencies in testing once the main operations modes are in place.
There was a problem hiding this comment.
Yep, sounds reasonable. Split now!
| fn test_verbose() { | ||
| let (at, mut ucmd) = at_and_ucmd!(); | ||
|
|
||
| let separator = path::MAIN_SEPARATOR; | ||
| let dir1_path = "dir1"; | ||
| let dir2_path = format!("{dir1_path}{separator}dir2"); | ||
| let file1_path = format!("{dir1_path}{separator}file1.txt"); | ||
| let file2_path = format!("{dir2_path}{separator}file2.txt"); | ||
|
|
||
| at.mkdir(dir1_path); | ||
| at.mkdir(&dir2_path); | ||
| at.write(&file1_path, "test content 1"); | ||
| at.write(&file2_path, "test content 2"); | ||
|
|
||
| ucmd.args(&["-cvf", "archive.tar", dir1_path]) | ||
| .succeeds() | ||
| .stdout_contains(dir1_path) | ||
| .stdout_contains(dir2_path) | ||
| .stdout_contains(file1_path) | ||
| .stdout_contains(file2_path); | ||
| } |
There was a problem hiding this comment.
Okay one this is very concise and well done so 🥇 great job. Small nit to pick lets change the name of this test to something like test_create_dir_verbose(). This way we know exactly what it is doing.
stillbeingnick
left a comment
stillbeingnick
left a comment
There was a problem hiding this comment.
Awesome stuff! Thank you for making the changes it will really help in the long term! LGTM 👍
|
@stillbeingnick, awesome! Thank you so much for the review and guidance, I really appreciate it. Shall we now close and merge it? |
2 participants
This PR removes leading slash from file names if path is absolute.
Closes: #44