GeezerVison

Reviewing files that changed from the base of the PR and between b2d231b and 0ef2c58.

• lib/utils.js (2 hunks)
• test/zip/index.test.js (1 hunks)
• test/zip/uncompress_stream.test.js (2 hunks)

• GitHub Check: Node.js / Test (windows-latest, 24)
• GitHub Check: Node.js / Test (windows-latest, 18)

lib/utils.js (2)

92-93: Good practice: removing strip from opts before passing to UncompressStream.

Correctly removes the strip option 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 type parameter 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 pipelinePromise with .then(next).catch(done) provides better error propagation. The addition of explicit end event handling for directory entries (line 45) ensures proper flow control.

---

196-197: Descriptive assertion messages aid debugging.

The change from assert(res.distinct === 0) to assert.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 strip option:

1. The constructor stores the option at line 37: this._strip = Number(opts.strip) || 0;
2. Before emitting the 'entry' event, the fileName is stripped via the [STRIP_NAME] method (line 110-114), which calls utils.stripFileName()
3. The header.name passed to the 'entry' event listener is already stripped
4. 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) to assert.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.

Reviewing files that changed from the base of the PR and between 0ef2c58 and d1e2c10.

• test/tar/index.test.js (1 hunks)
• test/tgz/index.test.js (1 hunks)

• GitHub Check: Node.js / Test (ubuntu-latest, 24)

test/tar/index.test.js (2)

230-240: LGTM! Test logic is sound.

The test correctly validates that strip: 1 removes the top-level directory component, and the comparison against originalDir is appropriate since stripping xxx/ from the archive should yield the same structure as the original xxx directory 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 the dircompare npm package and Windows stability. Let me search more specifically.

Based on my verification:

1. ✓ The xxx-strip2 fixture exists and contains the expected structure (4 files/items)
2. The dir-compare npm package documentation shows no documented Windows stability issues
3. The package is actively maintained with no known vulnerabilities
4. The Windows instability claim in the original review appears unsubstantiated and lacks evidence

The fixture exists and the test is correctly structured.

The xxx-strip2 fixture is present with valid content (bar.txt, bin, foo, test.js). The test correctly expects 4 equal files and 0 directories. The dir-compare package 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: 1 functionality for tgz archives, maintaining consistency with the tar implementation.