fix(scoop-export|scoop-list): Make exported JSON key order deterministic - #6697
Open
shauneccles wants to merge 2 commits into
Open
fix(scoop-export|scoop-list): Make exported JSON key order deterministic#6697shauneccles wants to merge 2 commits into
shauneccles wants to merge 2 commits into
Conversation
`[PSCustomObject]$hashtable` and `ConvertToPrettyJson` both follow the
hashtable's enumeration order. For a hashtable literal PowerShell keeps the
written order, but a hashtable built by assigning keys one at a time enumerates
in bucket order, which depends on `String.GetHashCode()` -- and .NET randomizes
string hashing per process. Both export sites use the assignment pattern, so the
key order was stable within a process and different in the next one.
- scoop-export.ps1: `$export` is built by assigning .config/.buckets/.apps,
so the top-level key order varied.
- scoop-list.ps1: `$item` is built by assigning .Name/.Version/.Source/
.Updated/.Info before the `[PSCustomObject]` cast, so every app object in
the file was independently reordered. This dominated the diff.
Using `[ordered]@{}` is a drop-in for the assignment pattern and yields
insertion order, which preserves the existing readable Name/Version/Source/
Updated/Info layout without needing a sorting pass.
Relates to ScoopInstaller#5862
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughChangesOutput ordering
🚥 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Two one-line changes,
@{}→[ordered]@{}, so thatscoop exportproduces byte-identical output when nothing has changed.[PSCustomObject]$hashtableandConvertToPrettyJsonboth follow the hashtable's enumeration order. For a hashtable literal PowerShell preserves the written order, so[PSCustomObject]@{ a = 1; b = 2 }is stable — but a hashtable built by assigning keys one at a time enumerates in bucket order, which depends onString.GetHashCode(), and .NET randomizes string hashing per process. Both export sites use the assignment pattern, so the key order was stable within a process and different in the next one.libexec/scoop-export.ps1:8—$exportis built by assigning.config/.buckets/.apps, so the three top-level keys were emitted in a random order.libexec/scoop-list.ps1:29—$itemis built by assigning.Name/.Version/.Source/.Updated/.Infobefore the[PSCustomObject]cast at line 63, so every app object in the file was independently reordered. This is what dominates the diff.[ordered]@{}gives insertion order, which keeps the existing readableName, Version, Source, Updated, Infolayout — no sorting pass needed, and no behaviour change for consumers, sincescoop importreads by property name ($item.Name,$item.Source, …).Motivation and Context
Relates to #5862
scoop export's main use is checking the file into version control, and non-deterministic key order defeats that. On a machine committing a dailyscoop export, 62 of 80 commits were pure property reordering with no content change at all — typically over 100 changed lines each, from only ~18 installed apps. Real version bumps were buried in the noise.Worth noting #5862 is currently labelled
enhancement, but the output is non-deterministic rather than merely unsorted, which is arguably a bug. Both facets reported there — the top-level order in the original report and the per-app property order in the follow-up comment — have the same root cause and are both fixed here.How Has This Been Tested?
PowerShell 7.6.1 on Windows 11, against a real install with 18 apps and 1 bucket.
1. Determinism, running
bin/scoop.ps1 exportin 4 separate processes and hashing the output — once on unmodifieddevelop, once with this change:developUpdated, Version, Name, Info, SourceName, Version, Source, Updated, Info2. Minimal repro of the underlying mechanism (5 separate processes):
With
[ordered]@{}the same loop returns insertion order every time. This is also why exporting twice in one session looks stable — the reordering only shows up across processes.3.
scoop export -c— the riskiest part of the change, since it does$export.config.PSObject.Properties.Remove(...). Verified on both variants that the output is still valid JSON, thatconfigis present, and thatlast_update/root_path/global_path/cache_path/aliasare still stripped. Property access and.Remove()work the same on anOrderedDictionary.4.
scoop list— unchanged output (scoop-list.ps1backs the display path too): 22 lines, exit 0, identical on both variants.5. Linting —
Invoke-ScriptAnalyzerwith the repo'sPSScriptAnalyzerSettings.psd1: 0 findings on both changed files and 0 acrosslibexecas a whole, matchingScoop-00Linting.Tests.ps1.There is currently no test coverage for
scoop exportorscoop-list.ps1intest/, so nothing existing needed updating. A regression test would have to spawn a child process to defeat per-process hash seeding, which felt disproportionate for a two-line change — happy to add one if you'd prefer.Checklist:
developbranch.