Fix #6515 "Expand-7zipArchive won't remove top folder if $ExtractDir depth exceeds 2" - Idea 2

[o-l-a-v]

Description

Make Expand-7zipArchive -ExtractDir handle $ExtractDir depth of two or more.

This PR tries to fix it in a non-breaking way, unlike #6516.

Motivation and Context

Closes #6515

How Has This Been Tested?

PS > scoop install extras/amber
PS > scoop install extras/tor-browser

Checklist:

• I have read the Contributing Guide.
• I have ensured that I am targeting the develop branch.
• I have updated the documentation accordingly.
• I have updated the tests accordingly.
• I have added an entry in the CHANGELOG.

Summary by CodeRabbit

• Bug Fixes

  • 7‑Zip extraction now correctly removes the top-level folder when extracting into deeply nested directories, ensuring the expected folder structure.
  • More reliable cleanup of temporary extraction directories and related log files to avoid leftover artifacts.

• Refactor

  • Improved reliability and error handling for archive extraction operations.

• Documentation

  • Updated changelog under Unreleased > Bug Fixes to reflect the extraction fix.

[coderabbitai]

Walkthrough

Refactors Expand-7zipArchive to invoke 7z/tar with explicit -FilePath and arguments, standardizes path/log cmdlet parameter usage, and fixes extract-dir cleanup by using a depth-based iterative removal so the top folder is removed correctly when ExtractDir depth exceeds two.

Changes

Cohort / File(s)	Summary
Changelog updates CHANGELOG.md	Added an Unreleased bug fix entry documenting that Expand-7zipArchive now correctly removes the top folder when $ExtractDir depth exceeds 2.
Decompression logic and CLI invocation lib/decompress.ps1	Replaced direct CLI invocations with explicit -FilePath/-ArgumentList usage for 7z/tar via Invoke-ExternalCommand, normalized -Path/Split-Path usage, replaced negation patterns with -not, tightened log-file cleanup to target leaf log files, and reworked extraction-directory cleanup to a depth-based iterative removal strategy.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Caller as Caller
    participant Expand as Expand-7zipArchive
    participant CLI as 7z/tar CLI
    participant FS as File System

    Caller->>Expand: extract(archive, destination, ExtractDir)
    note right of Expand: Resolve paths, construct log path (using Split-Path)
    Expand->>FS: Pre-check existing directories (recursive checks)
    Expand->>CLI: Invoke via -FilePath with explicit ArgumentList (list/extract)
    CLI-->>Expand: Exit code & output

    alt success
        Expand->>FS: Depth-based iterative cleanup (remove empty segments from top until leaf content)
        note right of FS: Stop when non-empty or reached intended depth
        Expand-->>Caller: success
    else error
        Expand-->>Caller: error/status
    end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I hop through folders, nibbling strings of path,
Depth by depth I tidy up the aftermath.
Seven zips now answer, tidy and polite,
Logs kept tidy, top folder gone from sight.
Thump-thump! A fix delivered — small and bright. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes Check	⚠️ Warning	Alongside the targeted fix, the PR introduces broad refactorings—such as standardizing Invoke-ExternalCommand calls, tweaking log path construction, case normalization, and parameterization—that fall outside the scope of issue #6515 and are unrelated to the removal logic correction.	Please isolate the folder-removal fix in one commit or PR and move the stylistic or structural refactorings into a separate follow-up change to keep the bug fix reviewable and focused.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues Check	✅ Passed	The pull request implements the depth-based iterative cleanup strategy to remove the correct top-level folder for multi-component $ExtractDir paths, directly satisfying the core requirements of issue #6515 without altering existing manifest behavior or introducing regressions.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.
Title Check	✅ Passed	The title clearly indicates that this PR addresses issue #6515 by fixing Expand-7zipArchive’s behavior when the ExtractDir depth exceeds two, directly reflecting the primary change.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[o-l-a-v]

What do you think of this approach @z-Fng?

The first PR i made #6516 does it by implementing #6093, but that will likely break some manifests.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

lib/decompress.ps1 (1)

124-145: Core bug fix looks correct with minor suggestions.

The depth-based iterative cleanup algorithm correctly addresses the issue where parent directories weren't being removed when $ExtractDir depth exceeds 2. The logic properly checks each directory level from deepest to shallowest and stops when encountering a non-empty directory.

Minor suggestions for robustness:

1. Line 130: Consider using [int] instead of [byte] for $Depth to avoid the 255-level limit, though this should be sufficient for all practical cases.

2. Line 136: The .Count property might not exist when Get-ChildItem returns a single item in PowerShell versions prior to 6. Consider wrapping in @() for consistency:

   -        if ((Get-ChildItem -Path $CurrentDir -Force -ErrorAction 'Ignore').'Count' -gt 0) {
   +        if (@(Get-ChildItem -Path $CurrentDir -Force -ErrorAction 'Ignore').Count -gt 0) {

3. Lines 131-135: The path construction is correct but could be simplified using Join-Path:

   -        $CurrentDir = [string] [System.IO.Path]::Combine(
   -            $DestinationPath, (
   -                ($ExtractDirs | Select-Object -First $Depth) -join [System.IO.Path]::DirectorySeparatorChar
   -            )
   -        )
   +        $CurrentDir = Join-Path $DestinationPath (($ExtractDirs | Select-Object -First $Depth) -join [System.IO.Path]::DirectorySeparatorChar)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 20a178b and 358bbca.

📒 Files selected for processing (2)

• CHANGELOG.md (1 hunks)
• lib/decompress.ps1 (3 hunks)

🔇 Additional comments (4)

CHANGELOG.md (1)

18-18: LGTM! Changelog entry is clear and accurate.

The bug fix entry correctly describes the issue and references the appropriate GitHub issue.

lib/decompress.ps1 (3)

94-94: LGTM! Explicit parameter names improve clarity.

The changes to use explicit parameter names (-FilePath, -Path) and -not instead of ! follow PowerShell best practices and make the code more readable.

Also applies to: 109-111, 115-115

---

146-148: LGTM! Improved log file cleanup safety.

Adding the -PathType 'Leaf' check ensures only actual files are removed, preventing potential issues if the log path unexpectedly becomes a directory.

---

152-152: LGTM! Consistent use of explicit parameters.

Adding explicit -Path parameters to Get-ChildItem and Remove-Item calls maintains consistency with the other improvements in this file and follows PowerShell best practices.

Also applies to: 155-155, 158-158