Skip to content

fix (decompress): Expand-7zipArchive only delete temp dir / $extractDir if it is empty #6092

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.

Sign up for GitHub

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

Merged
niheaven merged 14 commits into from
Aug 9, 2024
Merged

fix (decompress): Expand-7zipArchive only delete temp dir / $extractDir if it is empty #6092

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f634ba0
Only remove temp dir if it's empty
o-l-a-v Aug 6, 2024
6f6932c
Added to changelog
o-l-a-v Aug 6, 2024
78f7e30
Name all parameters
o-l-a-v Aug 7, 2024
501ce50
Add test
niheaven Aug 7, 2024
1853d24
Use -eq instead of -le
o-l-a-v Aug 7, 2024
0ecdc8d
Use -eq instead of -le
o-l-a-v Aug 7, 2024
608bb98
Merge branch 'fix-6011' of https://github.com/o-l-a-v/ScoopInstaller-…
o-l-a-v Aug 7, 2024
041ecba
Correct typo
o-l-a-v Aug 7, 2024
46ba47b
Only check for child items if temp dir still exists
o-l-a-v Aug 7, 2024
fae981c
Adjusted changelog to latest change
o-l-a-v Aug 7, 2024
243fb50
Tweak test cases
niheaven Aug 8, 2024
c6817cf
Use ErrorAction Ignore instead of Test-Path
o-l-a-v Aug 8, 2024
4b4767f
Change param name too
o-l-a-v Aug 8, 2024
1c87c6a
Update chglog
niheaven Aug 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## [Unreleased](https://github.com/ScoopInstaller/Scoop/compare/master...develop)

### Bug Fixes

- **decompress**: `Expand-7zipArchive` Only delete temp dir / `$extractDir` if it exists and is empty ([#6011](https://github.com/ScoopInstaller/Scoop/issues/6011))

## [v0.5.2](https://github.com/ScoopInstaller/Scoop/compare/v0.5.1...v0.5.2) - 2024-07-26

### Bug Fixes
Expand Down
12 changes: 9 additions & 3 deletions lib/decompress.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,15 +123,21 @@ function Expand-7zipArchive {
}
if (!$IsTar -and $ExtractDir) {
movedir "$DestinationPath\$ExtractDir" $DestinationPath | Out-Null
# Remove temporary directory
Remove-Item "$DestinationPath\$($ExtractDir -replace '[\\/].*')" -Recurse -Force -ErrorAction Ignore
# Remove temporary directory if it still exists and is empty
$ExtractDirFullPath = [string] "$DestinationPath\$($ExtractDir -replace '[\\/].*')"
if (
(Test-Path -Path $ExtractDirFullPath -PathType 'Container') -and
(Get-ChildItem -Path $ExtractDirFullPath -Force).Count -eq 0
) {
Remove-Item -Path $ExtractDirFullPath -Recurse -Force -ErrorAction Ignore
}
Comment thread
o-l-a-v marked this conversation as resolved.
Outdated
Copy link
Copy Markdown
Member

@z-Fng z-Fng Oct 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

While testing manifests, I happened to run into an issue here.

When the $ExtractDir depth is greater than or equal to three, like the one below ($ExtractDir : keep\sub\sub-sub), the folder 'keep' will not be removed. The return value of Get-ChildItem for folder 'keep' is always greater than 0 as there exists a sub-folder.

└─ keep
   └─ sub
      └─  sub-sub
         └─  empty
   └─ othersub

Copy link
Copy Markdown
Contributor Author

@o-l-a-v o-l-a-v Oct 12, 2025

Choose a reason for hiding this comment

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

So one should look for files specifically? Add -File -Recurse to Get-ChildItem?

Copy link
Copy Markdown
Member

@z-Fng z-Fng Oct 12, 2025

Choose a reason for hiding this comment

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

Add -File -Recurse to Get-ChildItem?

When extraction is done, we cannot tell whether the folder is newly created or already existed.
If the folder already exists before extraction, even if it is an empty folder, we should not remove it.

Perhaps we could check if the folder exists recursively before 7-Zip actually starts extracting?

keep
   └─ sub
      └─  sub-sub
  1. Check if the folder $DestinationPath\keep exists.
    • If it does not exist, we can safely remove $DestinationPath\keep.
    • If it does exist, proceed to step 2.
  2. Check if the folder $DestinationPath\keep\sub exists.
    • If it does not exist, we can safely remove $DestinationPath\keep\sub.
    • If it does exist, proceed to step 3.
  3. ...

Copy link
Copy Markdown
Contributor Author

@o-l-a-v o-l-a-v Oct 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Can you create a new issue with an example manifest where this problem occurs?

There is also this feature request which would make this logic unnecessary:

Copy link
Copy Markdown
Member

@z-Fng z-Fng Oct 13, 2025

Choose a reason for hiding this comment

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

Can you create a new issue with an example manifest where this problem occurs?

OK. I'm just about to do it.

There is also this feature request which would make this logic unnecessary

It would be even better if this new feature fixes the issue.

Copy link
Copy Markdown
Member

@z-Fng z-Fng Oct 13, 2025

Choose a reason for hiding this comment

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

Can you create a new issue with an example manifest where this problem occurs?

}
if (Test-Path $LogPath) {
Remove-Item $LogPath -Force
}
if ($Removal) {
if (($Path -replace '.*\.([^\.]*)$', '$1') -eq '001') {
# Remove splited 7-zip archive parts
# Remove splitted 7-zip archive parts
Get-ChildItem "$($Path -replace '\.[^\.]*$', '').???" | Remove-Item -Force
} elseif (($Path -replace '.*\.part(\d+)\.rar$', '$1')[-1] -eq '1') {
# Remove splitted RAR archive parts
Expand Down
12 changes: 10 additions & 2 deletions test/Scoop-Decompress.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ Describe 'Decompression function' -Tag 'Scoop', 'Windows', 'Decompress' {
}
It 'Test cases should exist and hash should match' {
$testcases | Should -Exist
(Get-FileHash -Path $testcases -Algorithm SHA256).Hash.ToLower() | Should -Be 'afb86b0552187b8d630ce25d02835fb809af81c584f07e54cb049fb74ca134b6'
(Get-FileHash -Path $testcases -Algorithm SHA256).Hash.ToLower() | Should -Be '591072faabd419b77932b7023e5899b4e05c0bf8e6859ad367398e6bfe1eb203'
}
It 'Test cases should be extracted correctly' {
{ Microsoft.PowerShell.Archive\Expand-Archive -Path $testcases -DestinationPath $working_dir } | Should -Not -Throw
Expand Down Expand Up @@ -61,7 +61,7 @@ Describe 'Decompression function' -Tag 'Scoop', 'Windows', 'Decompress' {
$to = test_extract 'Expand-7zipArchive' $test1
$to | Should -Exist
"$to\empty" | Should -Exist
(Get-ChildItem $to).Count | Should -Be 3
(Get-ChildItem $to).Count | Should -Be 4
}

It 'extract "extract_dir" correctly' {
Expand All @@ -78,6 +78,14 @@ Describe 'Decompression function' -Tag 'Scoop', 'Windows', 'Decompress' {
(Get-ChildItem $to).Count | Should -Be 1
}

It 'extract "extract_dir" with nested folder with same name' {
$to = test_extract 'Expand-7zipArchive' $test1 $false 'keep\sub'
$to | Should -Exist
"$to\keep\empty" | Should -Exist
(Get-ChildItem $to).Count | Should -Be 1
(Get-ChildItem "$to\keep").Count | Should -Be 1
}

It 'extract nested compressed file' {
# file ext: tgz
$to = test_extract 'Expand-7zipArchive' $test2
Expand Down
Binary file modified test/fixtures/decompress/TestCases.zip
View file
Open in desktop
Binary file not shown.
⚡