Repository navigation
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughCached downloads now use a helper that attempts a hard link for matching archive files when source and destination are on the same drive. If linking is not used or fails, the helper copies the file. Uncached downloads still move the file. Tests check archive hard links and copy behavior for scripts. ChangesCached download placement
Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The new hard-link test may fail on some PowerShell 7 versions even when the feature works, which could break CI. Asserting LinkType instead of Target fixes this before merge. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![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 info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
99284feb-c958-4b8b-984e-db7f42cbb66e
📒 Files selected for processing (1)
lib/download.ps1
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
8cc72c1 to
b4ebd02
Compare
b4ebd02 to
f0f9506
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
20310013-2062-40c1-895a-10a333cec815
📒 Files selected for processing (2)
lib/download.ps1test/Scoop-Download.Tests.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/download.ps1
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| $stagedZip = Join-Path $testDir 'staged.zip' | ||
| Link-OrCopyFile $cacheArchive $stagedZip | ||
| Test-Path $stagedZip | Should -BeTrue | ||
| (Get-Item $stagedZip).Target | Should -Not -BeNullOrEmpty |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- diff ---'
git diff --no-ext-diff --unified=50 e6aa3b366bdee8ed138c1e0f7b85192ebdd35d0f e5642ded71e2d0035db6bf346bf2dac7d9db0cfc -- test/Scoop-Download.Tests.ps1
printf '%s\n' '--- target test and helper references ---'
rg -n -C 5 'Link-OrCopyFile|stagedZip|Target|LinkType' test/Scoop-Download.Tests.ps1
rg -n -F -- 'function Link-OrCopyFile' . --glob '!vendor/**' --glob '!node_modules/**' || test "$?" -eq 1
printf '%s\n' '--- PowerShell version configuration ---'
rg -n -i -C 3 'pwsh|powershell|PowerShellVersion|Pester|windows-latest|windows-' .github test appveyor.yml azure-pipelines.yml 2>/dev/null || test "$?" -eq 1
printf '%s\n' '--- repository files mentioning supported PowerShell ---'
rg -n -i -C 2 'PowerShell 5|PowerShell 7|PowerShell Core|PowerShell version|minimum.*PowerShell|requires.*PowerShell' README.md CONTRIBUTING.md .github docs 2>/dev/null || test "$?" -eq 1Repository: ScoopInstaller/Scoop
Length of output: 17426
🤖 get_repo_knowledge executed:
get_repo_knowledge ScoopInstaller/Scoop /tmp/coderabbit-repo-knowledge/scoopinstaller-scoop-1ef5db97/learnings /tmp/coderabbit-repo-knowledge/scoopinstaller-scoop-1ef5db97/architecture
Length of output: 15047
🌐 Web query:
PowerShell FileSystemInfo Target property hard link empty PowerShell 7 LinkType hardlink behavior documentation issue 20938
💡 Result:
**Issue #20938 is closed as “Resolution-Won’t Fix.”** It reports that in PowerShell 7.4.0, `Get-Item` identifies a hard link as `LinkType = HardLink`, but `LinkTarget` and the legacy `Target` property are empty. The reporter contrasts that with Windows PowerShell 5, where `Target` was populated. (GitHub issue 20938 in PowerShell/PowerShell (link omitted to avoid creating a cross-reference))
The PowerShell 7.5 FileSystem provider docs describe `LinkType` and `LinkTarget`, and note that their values depend on the platform and filesystem—but they don’t promise a target path for hard links. ([learn.microsoft.com](https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_filesystem_provider?view=powershell-7.5))
**Bottom line:** use `LinkType` to identify a hard link; don’t rely on `Target`/`LinkTarget` to enumerate its other names. The issue’s specific reproduction is for 7.4.0; the available issue and docs don’t establish behavior for every later PowerShell version.
Citations:
- 1: GitHub issue 20938 in PowerShell/PowerShell (link omitted to avoid creating a cross-reference)
- 2: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_filesystem_provider?view=powershell-7.5
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- CI workflow ---'
nl -ba .github/workflows/ci.yml | sed -n '1,36p'
printf '%s\n' '--- helper ---'
nl -ba lib/download.ps1 | sed -n '40,88p'
printf '%s\n' '--- test block ---'
nl -ba test/Scoop-Download.Tests.ps1 | sed -n '50,90p'
printf '%s\n' '--- test requirements ---'
nl -ba test/bin/test.ps1 | sed -n '1,26p'Repository: ScoopInstaller/Scoop
Length of output: 6441
🌐 Web query:
PowerShell Windows PowerShell 5.1 Get-Item LinkType property unavailable LinkType PowerShell 6 file system provider
💡 Result:
`LinkType` is **not exclusive to PowerShell 6+**. Microsoft’s Windows PowerShell 5.1 documentation shows `LinkType` on a FileSystem item, and the 5.1 docs’ example output includes `LinkType : HardLink`. ([github.com](https://github.com/MicrosoftDocs/PowerShell-Docs/blob/main/reference/5.1/Microsoft.PowerShell.Core/About/about_Properties.md))
If `(Get-Item $path).LinkType` is blank or missing in 5.1, check that the item is actually a link and that the path uses the FileSystem provider. In PowerShell 7.5, Microsoft documents that `LinkType` describes linked items and that values depend on the platform and filesystem; Windows may also return no `LinkType` for certain `AppExecLink` items. ([learn.microsoft.com](https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_filesystem_provider?view=powershell-7.5))
To inspect what your 5.1 session returns, try:
```powershell
$item = Get-Item -LiteralPath $path
$item | Get-Member
$item | Format-List LinkType, Target, Mode
```
The docs establish that 5.1 can expose `LinkType`; they don’t establish that every filesystem item or every link type will have a value.
Citations:
- 1: https://github.com/MicrosoftDocs/PowerShell-Docs/blob/main/reference/5.1/Microsoft.PowerShell.Core/About/about_Properties.md
- 2: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_filesystem_provider?view=powershell-7.5
Assert LinkType for the hard-link test.
When the pwsh leg uses an affected version (PowerShell 7.4 does), Get-Item can return an empty Target for a valid hard link, causing this assertion to fail. LinkType reports HardLink in both PowerShell 7.4 and Windows PowerShell 5.1.
🐛 Suggested fix
- (Get-Item $stagedZip).Target | Should -Not -BeNullOrEmpty
+ (Get-Item $stagedZip).LinkType | Should -Be 'HardLink'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (Get-Item $stagedZip).Target | Should -Not -BeNullOrEmpty | |
| (Get-Item $stagedZip).LinkType | Should -Be 'HardLink' |
e5642de to
fa2284b
Compare
This branch has not been deployed
1 participant
📋 Problème Traité
Dans
lib/download.ps1, la fonctionInvoke-CachedDownloadeffectue une duplication physique intégrale parCopy-Itemde chaque archive téléchargée depuis le cache (~/.scoop/cache/) vers le répertoire d'installation temporaire de l'application :Conséquences :
💡 Solution Apportée
Remplacement de la copie physique aveugle par une stratégie zéro-copie via Hardlink NTFS (
New-Item -ItemType HardLink) lorsque le cache et la cible se trouvent sur le même volume (cas par défaut de Scoop dans$env:USERPROFILE\scoop) :Invoke-Extraction -Removalne fait que décrémenter le compteur de liens NTFS sans toucher au cache.Copy-Itemest assuré.🔗 Issue Associée
Amélioration des performances I/O et réduction de l'empreinte disque lors de l'installation depuis le cache.
✅ Validation & Actions Réalisées
Note
perf(download): use zero-copy NTFS hardlinks for cached payload extraction🧪 Reproduction de l'Erreur
Script démontrant la lenteur et la duplication de
Copy-Itemsur un fichier de 200 Mo :🔬 Test de la Correction
Script validant la correction, l'absence de duplication et la persistance du cache :
No new blocking issue was established, so the PR appears safe to merge on this review.
Summary
The PR stages cached archives with hardlinks where possible, retains copying for other payloads, and adds download tests. Since the previous review, it has changed the hardlink test to assert
LinkTyperather thanTarget.Reviews (6) · Last reviewed commit: "perf(download): restrict zero-copy hardl..."