-
Notifications
You must be signed in to change notification settings - Fork 138
tar: Fix name filter on Windows #1047
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1047 +/- ##
==========================================
+ Coverage 60.75% 60.80% +0.04%
==========================================
Files 268 268
Lines 33593 33590 -3
==========================================
+ Hits 20411 20425 +14
+ Misses 11510 11494 -16
+ Partials 1672 1671 -1
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
077af9d to
bf82623
Compare
`reservedCharsStr` used to be (fc7299c) a regex pattern where `[` and `]` are not part of the set, they are the containers of the set. This accidentally forbids legal filenames that contain `[` and/or `]`.
The check should be case-insensitive. E.g. `CON`, `con`, CoN`, etc. are all reserved.
Same behavior, just deduplicating and restructuring.
Same behavior restructured.
We check for a `.` suffix above, so the `..` branch will never run.
- Don't postfix symbols with their type name in Go (C-ism). - Comments should terminate with periods. - Use arrays over slices for fixed-size ranges. - No overly long lines. - Constant declarations typically precede variable declarations
bf82623 to
9274bf1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have drafted this first, too late now.
Gave this a second look and made some minor changes.
I'm content with this, so it should be ready for external review.
Edit: might be best to review commit-by-commit rather than starting with the full-diff. idk
This is for: ipfs/kubo#9370
Specifically this comment for context: ipfs/kubo#9370 (comment)
e579012 is the main fix
In short, this plain-string used to be a regex-string where
[and]have meaning, they're not part of the set so they should not be used withContainsAny.4b9e075
Is a secondary fix that catches reserved names even if their case differs.
I don't know if anyone has encountered this in IPFS yet, but I know file names like
conshow up in various operating system source trees.The rest of the patches are gardening and testing.
The sanitize tests hit 100% coverage with this, and I tested this explicitly in go-ipfs via
ipfs get.Prior to the patch, we hit the bug. After, we don't.
Probably good?