Repository navigation
extract: add -k/--keep-old-files - #404
ozenyunus35 wants to merge 3 commits into
Conversation
Like GNU tar, -k leaves an existing file as it is and reports it as an
error ("Cannot open: File exists"); an existing directory is fine. The
other members are still extracted and the run exits with status 2.
Closes uutils#191
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #404 +/- ##
==========================================
+ Coverage 96.84% 98.07% +1.22%
==========================================
Files 11 15 +4
Lines 1492 1925 +433
Branches 29 41 +12
==========================================
+ Hits 1445 1888 +443
+ Misses 46 36 -10
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will not alter performance
|
|
|
||
| // -k: like GNU tar, an existing file is an error and is left as it is; | ||
| // an existing directory is fine. | ||
| if keep_old && !entry.header().entry_type().is_dir() && fs::symlink_metadata(&path).is_ok() |
There was a problem hiding this comment.
this checks the raw member path, so /etc/foo or ../foo is looked up outside the target dir, no?
it also runs before the .. check, so an existing ../foo says "File exists" instead of the .. message.
could you please check the path unpack_in will actually write to, and add a test for it?
There was a problem hiding this comment.
Right on both counts. It now checks the path unpack_in writes to (leading / and . dropped), and a member with a .. component is left to the .. check. Added tests for /kept.txt landing on an existing kept.txt and for an existing ../item.
| .succeeds(); | ||
| at.write("a.txt", "already here"); | ||
| at.remove("b.txt"); | ||
|
|
There was a problem hiding this comment.
the asserts already say this, the comment can go
| .stderr_contains("Exiting with failure status due to previous errors"); | ||
| assert_eq!(at.read("a.txt"), "already here"); | ||
| assert_eq!(at.read("b.txt"), "also from archive"); | ||
| } |
There was a problem hiding this comment.
please also add a test where the member is a directory that already exists (it should not be an error)
There was a problem hiding this comment.
Added test_extract_keep_old_files_existing_directory.
Look up the path unpack_in writes (leading / and . dropped) instead of the raw member name, and leave a member with a .. component to the .. check. Test a leading /, a .. member and an existing directory.
| assert_eq!(at.read("d/f.txt"), "from archive"); | ||
| } | ||
|
|
||
| fn keep_old_members_tar(names: &[&str]) -> Vec<u8> { |
There was a problem hiding this comment.
almost the same code as in test_extract_reports_member_with_parent_dir_and_continues, could you please use this helper there too?
and maybe a less -k specific name, like tar_with_members
# Conflicts: # src/uu/tar/src/tar.rs # tests/by-util/test_tar.rs
|
Closing this, as I can't follow up on it further. Thanks for the reviews. |
2 participants
Closes #191
With
-k/--keep-old-files, extract leaves an existing file as it is and reports it the way GNU tar does (tar: a.txt: Cannot open: File exists); an existing directory is not an error. The other members are still extracted, and the run ends with "Exiting with failure status due to previous errors" and status 2, using the same recoverable-error path as the..member check.extract_archivegets akeep_oldflag; the unit test and bench callers passfalse. Addedtest_extract_keep_old_files, which checks the message, the exit status, that the existing file keeps its content and that the other member is extracted. It fails without the change.This touches the same
extract_archivesignature as #403 (-O), so whichever lands second needs a small rebase; I'll take care of it.I used an AI assistant to help draft parts of the code and tests; I reviewed the changes and ran the tests myself.