WIP: feat: support strip for all archive #117
Conversation
WalkthroughThis pull request implements file path stripping functionality during decompression operations. The core change adds support for a Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant makeUncompressFn
participant stripFileName
participant UncompressStream
participant FS as File System
User->>makeUncompressFn: uncompress(source, dest, {strip: 1})
makeUncompressFn->>makeUncompressFn: derive strip from opts.strip
makeUncompressFn->>makeUncompressFn: remove strip from opts
makeUncompressFn->>UncompressStream: create stream with cleaned opts
UncompressStream->>UncompressStream: emit entry events
makeUncompressFn->>stripFileName: normalize path (strip=1, header.name)
stripFileName-->>makeUncompressFn: return normalized path
makeUncompressFn->>FS: write file to normalized destination
FS-->>User: extraction complete with paths stripped
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
似乎 windows 上的 dircompare 是不稳定的 |
There was a problem hiding this comment.
Pull Request Overview
This PR extends the strip functionality to support all archive types (tar, tgz, and zip) by implementing it uniformly in the makeUncompressFn utility function.
Key Changes:
- Adds centralized strip handling in
makeUncompressFnfor all archive types - Improves test reliability with better synchronization
- Enhances assertion messages for clearer test failure diagnostics
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/utils.js | Implements strip parameter extraction and application in makeUncompressFn, enabling strip functionality for tar and tgz archives while maintaining compatibility with zip's existing implementation |
| test/zip/uncompress_stream.test.js | Addresses test timing issue by deferring assertions and improves error messages with descriptive assert.equal statement |
| test/zip/index.test.js | Enhances assertion with descriptive error message for file mode comparison |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bytemain
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/utils.js(2 hunks)test/zip/index.test.js(1 hunks)test/zip/uncompress_stream.test.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/zip/index.test.js (1)
test/zip/file_stream.test.js (1)
assert(9-9)
test/zip/uncompress_stream.test.js (1)
test/util.js (2)
stream(1-1)pipelinePromise(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Node.js / Test (windows-latest, 24)
- GitHub Check: Node.js / Test (windows-latest, 18)
🔇 Additional comments (6)
lib/utils.js (2)
92-93: Good practice: removing strip from opts before passing to UncompressStream.Correctly removes the
stripoption after extraction to avoid passing it to the underlying UncompressStream constructor, which handles entry emission but not path stripping.
115-115: stripFileName correctly handles all entry types—verification passed.The function receives the
typeparameter and uses it appropriately at line 221 to normalize directory paths (adding trailing slash). All three entry types are properly handled in the decompression flow:
- file (lines 116–127): Creates parent directory, writes file
- symlink (lines 128–142): Creates parent directory, creates symlink with relative target
- directory (lines 143–148): Creates directory with recursive flag
No issues found.
test/zip/uncompress_stream.test.js (3)
37-46: Improved error handling with pipelinePromise.The change from direct stream piping to
pipelinePromisewith.then(next).catch(done)provides better error propagation. The addition of explicitendevent handling for directory entries (line 45) ensures proper flow control.
196-197: Descriptive assertion messages aid debugging.The change from
assert(res.distinct === 0)toassert.equal(res.distinct, 0, 'distinct files count mismatch')improves debugging capabilities, especially given the PR author's comment about dircompare instability on Windows.
187-216: Strip handling in UncompressStream is correctly implemented.Verification confirms that ZipUncompressStream properly handles the
stripoption:
- The constructor stores the option at line 37:
this._strip = Number(opts.strip) || 0;- Before emitting the 'entry' event, the fileName is stripped via the
[STRIP_NAME]method (line 110-114), which callsutils.stripFileName()- The header.name passed to the 'entry' event listener is already stripped
- The test's expectation (line 205) that header.name is pre-stripped is correct
The implementation is sound.
test/zip/index.test.js (1)
291-291: Improved assertion validates mode preservation dynamically.The change from
assert(stat.mode === 32804)toassert.equal(stat.mode, originStat.mode, 'file mode should be same after uncompress')is an improvement. It validates that the file mode is preserved through compression/uncompression rather than checking against a hardcoded value, making the test more robust across different systems.
bytemain
changed the title
bytemain
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/tar/index.test.js (1)
247-248: Consider adding assertion messages for consistency.The
strip: 1test includes helpful error messages like'distinct files count mismatch'(lines 236-237), but these assertions lack them. Adding messages would improve debugging consistency across tests.- assert.equal(res.distinct, 0); - assert.equal(res.equal, 4); + assert.equal(res.distinct, 0, 'distinct files count mismatch'); + assert.equal(res.equal, 4, 'equal files count mismatch');test/tgz/index.test.js (1)
237-246: Same verification and consistency concerns as tar tests.This test has the same dependencies and concerns as the tar strip: 2 test:
- Relies on the
fixtures/xxx-strip2fixture (verification already requested for tar tests)- Subject to the same Windows
dircompareinstability mentioned in the PR comments- Missing assertion messages for
distinctandequalcounts (lines 242-243)For consistency, consider adding descriptive assertion messages:
- assert.equal(res.distinct, 0); - assert.equal(res.equal, 4); + assert.equal(res.distinct, 0, 'distinct files count mismatch'); + assert.equal(res.equal, 4, 'equal files count mismatch');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/tar/index.test.js(1 hunks)test/tgz/index.test.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/tgz/index.test.js (1)
test/tgz/uncompress_stream.test.js (8)
sourceFile(13-13)path(4-4)os(3-3)uuid(5-5)originalDir(12-12)compressing(10-10)dircompare(8-8)assert(6-6)
test/tar/index.test.js (1)
test/tar/uncompress_stream.test.js (8)
sourceFile(14-14)path(4-4)os(3-3)uuid(5-5)originalDir(13-13)compressing(11-11)dircompare(8-8)assert(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Node.js / Test (ubuntu-latest, 24)
🔇 Additional comments (3)
test/tar/index.test.js (2)
230-240: LGTM! Test logic is sound.The test correctly validates that
strip: 1removes the top-level directory component, and the comparison againstoriginalDiris appropriate since strippingxxx/from the archive should yield the same structure as the originalxxxdirectory contents.
242-251: I need to investigate whether the Windows dircompare instability mentioned in the original review is a documented issue in the codebase.The web search didn't return specific information about thedircomparenpm package and Windows stability. Let me search more specifically.Based on my verification:
- ✓ The
xxx-strip2fixture exists and contains the expected structure (4 files/items)- The
dir-comparenpm package documentation shows no documented Windows stability issues- The package is actively maintained with no known vulnerabilities
- The Windows instability claim in the original review appears unsubstantiated and lacks evidence
The fixture exists and the test is correctly structured.
The
xxx-strip2fixture is present with valid content (bar.txt, bin, foo, test.js). The test correctly expects 4 equal files and 0 directories. Thedir-comparepackage shows no documented Windows instability issues. The original suggestion to add Windows-specific handling lacks concrete evidence and is not warranted.test/tgz/index.test.js (1)
225-235: LGTM! Consistent with tar tests.The test correctly validates the
strip: 1functionality for tgz archives, maintaining consistency with the tar implementation.
1 participant
Previous, only
zip streamsupport strip option, now support for all uncompress streamtodo:
Summary by CodeRabbit
Release Notes
New Features
stripoption to uncompress operations, allowing removal of leading path components from extracted file structures.Tests