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

fix(tar): enable unix paths in tar RootPath #582

Merged
merged 15 commits into from Aug 16, 2022
Merged

fix(tar): enable unix paths in tar RootPath #582

merged 15 commits into from Aug 16, 2022

Conversation

sensslen
Copy link
Contributor

Fix Path RootPath property usage in TarArchive by applying the same transformation as used when stripping a TarEntry path.

see #581 #338

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

@Numpsy
Copy link
Contributor

Numpsy commented Feb 17, 2021

this PR seems to include a set of Zip changes from one of the other (unmerged) async related PRs, rather than being against the master branch or being just the described issue?

sensslen and others added 4 commits February 17, 2021 15:10
(cherry picked from commit 14a6292)
(cherry picked from commit 93e3464)
…ut code duplication

- Add unit test that tests the compression and extraction

(cherry picked from commit cf007d5)
(cherry picked from commit 29ac1d3)
(cherry picked from commit a90e117)
(cherry picked from commit 216ab62)
(cherry picked from commit 376ca1e)
(cherry picked from commit 159dbd4)
@sensslen
Copy link
Contributor Author

You are correct - I got confused by that branch that was called something like origin/.../master. Sorry! I corrected this mistake.

@piksel
Copy link
Member

piksel commented Feb 17, 2021

Yeah, that's what the gh tool does when you check out PRs. I should rename it.

@sensslen
Copy link
Contributor Author

Is there anything witholding this PR to be merged? We're curently spinning our own version of SharpZipLib which includes this fix but I would really like to get rid of it as soon as possible!

@piksel
Copy link
Member

piksel commented Mar 19, 2021

Well, there are some problems with DropPathRoot. It will throw exceptions on .NET 4.6 if you give it certain paths which alters the behaviour (and can throw upon reading files etc, which isn't great).
See #593 which deals with this. I think I have found all the problems with it though, so that should be merged pretty soon. The plan is to use it for all the path handling code that is currently split between zip and tar. It would basically just slot in as a replacement for the ClearTarPath() method in this PR.

@sensslen
Copy link
Contributor Author

Good to hear, that you are working on this issue. Looking forward to the next release

Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

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

Added suggestions (that can be batch-commited if you want) for using the shared DropPathRoot and reuse of existing test helpers.

src/ICSharpCode.SharpZipLib/Tar/TarStringExtension.cs Outdated Show resolved Hide resolved
src/ICSharpCode.SharpZipLib/Tar/TarStringExtension.cs Outdated Show resolved Hide resolved
test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs Outdated Show resolved Hide resolved
test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs Outdated Show resolved Hide resolved
test/ICSharpCode.SharpZipLib.Tests/Tar/TarTests.cs Outdated Show resolved Hide resolved
src/ICSharpCode.SharpZipLib/Tar/TarEntry.cs Outdated Show resolved Hide resolved
src/ICSharpCode.SharpZipLib/Tar/TarArchive.cs Outdated Show resolved Hide resolved
sensslen and others added 8 commits May 22, 2021 12:33
Co-authored-by: nils måsén <nils@piksel.se>
Co-authored-by: nils måsén <nils@piksel.se>
Co-authored-by: nils måsén <nils@piksel.se>
Co-authored-by: nils måsén <nils@piksel.se>
Co-authored-by: nils måsén <nils@piksel.se>
Co-authored-by: nils måsén <nils@piksel.se>
Co-authored-by: nils måsén <nils@piksel.se>
Co-authored-by: nils måsén <nils@piksel.se>
@sensslen
Copy link
Contributor Author

Thanks a lot for your suggestions. They do absolutely make sense. Just out of curiosity. Didn't #593 address this issue already and this PR is thus obsolete now?

@codecov
Copy link

codecov bot commented May 23, 2021

Codecov Report

Merging #582 (f1b175a) into master (79614c5) will increase coverage by 0.41%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #582      +/-   ##
==========================================
+ Coverage   74.13%   74.54%   +0.41%     
==========================================
  Files          71       72       +1     
  Lines        8450     8450              
==========================================
+ Hits         6264     6299      +35     
+ Misses       2186     2151      -35     
Impacted Files Coverage Δ
src/ICSharpCode.SharpZipLib/Tar/TarArchive.cs 48.70% <100.00%> (+4.63%) ⬆️
src/ICSharpCode.SharpZipLib/Tar/TarEntry.cs 78.44% <100.00%> (+10.65%) ⬆️
.../ICSharpCode.SharpZipLib/Tar/TarStringExtension.cs 100.00% <100.00%> (ø)
src/ICSharpCode.SharpZipLib/Tar/TarHeader.cs 84.47% <0.00%> (+3.61%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sensslen
Copy link
Contributor Author

I'd love to hear why this PR is still not merged....

@piksel
Copy link
Member

piksel commented Jan 24, 2022

I'd love to hear why this PR is still not merged....

I have been busy with life and other projects. It's still in the pipeline for 1.4.

@sensslen
Copy link
Contributor Author

I'd love to hear why this PR is still not merged....

I have been busy with life and other projects. It's still in the pipeline for 1.4.

No Worries

also updates the references to test util helpers
@piksel piksel changed the title Fix RootPath fix(tar): enable unix paths in tar RootPath Aug 16, 2022
@piksel piksel merged commit bdec777 into icsharpcode:master Aug 16, 2022
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

3 participants