Fix the Nix < 2.20 NAR hash fallback for inputs with submodules - #624
Fix the Nix < 2.20 NAR hash fallback for inputs with submodules#624joshuaspence wants to merge 1 commit into
Conversation
`GitInputScheme::getAccessorFromCommit()` decided whether a lock was
produced with Nix < 2.20 or Nix >= 2.20 export semantics *before*
mounting submodules. For an input with `submodules = true` the expected
NAR hash covers the mounted tree, while both candidate hashes covered
only the bare top-level repo, so neither could ever match. Instead of
falling back to `git archive`/`git checkout` and warning, Nix hard-failed
with a NAR hash mismatch:
error: NAR hash mismatch in input
'git+ssh://git@example.org/repo.git?rev=...&shallow=1&submodules=1',
expected 'sha256-CnkK...' but got 'sha256-XNEq...'
`nix flake update <input>` recomputes rather than verifies the hash, so
the failure only showed up on the read path — anyone evaluating a lock
file written by Nix < 2.20 (or by Nix >= 2.20 with `nix-219-compat`
enabled) for a repo with submodules could not evaluate it at all.
Move the submodule mounting into a `getTree()` lambda so both candidate
trees are fully mounted before their NAR hashes are compared. Since the
Git filters that Nix < 2.20 applied also affect submodule contents,
reproducing such a hash requires exporting the submodules with the same
semantics; propagate this through the synthesized submodule inputs via a
new internal `__legacyExport` attribute (not part of `allowedAttrs()`,
never serialized into a lock file, following the `__final` convention).
When it's absent we're a top-level input and `nix-219-compat` decides;
when it's present we follow the top-level repo regardless of the setting.
`GitAccessorOptions` gains a matching `legacy` field so that
`makeFingerprint()` accounts for it. It is appended last, keeping the
legacy cache keys byte-identical to the previous hand-concatenated
`makeFingerprint(rev) + ";legacy"`.
Assisted-by: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesThe Git fetcher now propagates legacy export semantics to synthesized submodule inputs. Accessor fingerprints include the legacy mode, and NAR-hash fallback uses unified tree construction for both export modes. Legacy Git export
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change makes legacy Git hashes work correctly for inputs with submodules while preserving modern behavior and cache-key compatibility. No merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant GitInputScheme
participant GitAccessor
participant SubmoduleFetcher
participant NARHashVerifier
GitInputScheme->>GitAccessor: Build tree with selected export mode
GitAccessor->>SubmoduleFetcher: Mount submodules with __legacyExport
GitInputScheme->>NARHashVerifier: Verify selected tree hash
NARHashVerifier-->>GitInputScheme: Report match or mismatch
GitInputScheme->>GitAccessor: Build tree with alternate mode on mismatch
GitAccessor->>SubmoduleFetcher: Mount submodules with alternate export mode
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Motivation
Git inputs fetched with
submodules = truecannot be evaluated at all if their lockednarHashwas produced with Nix < 2.20 semantics. Instead of the intended "please update the NAR hash" warning, Nix fails hard:This affects any lock file written by Nix < 2.20, as well as locks written by Nix >= 2.20 with
nix-219-compatenabled — that setting was only ever meant to control the write path, with the read path staying backwards compatible in both directions.The failure is easy to miss, because
nix flake update <input>recomputes the hash rather than verifying it. So the lock silently changes for whoever happens to touch that input next, and hard-fails for everybody else.Context
GitInputScheme::getAccessorFromCommit()chooses between the libgit2 export and thegit archive/git checkoutexport by hashing both candidate trees and comparing againstinput.getNarHash()— but it did so before mounting submodules. Forsubmodules = truethe expected NAR hash covers the fully mounted tree, while both candidates covered only the bare top-level repo, so neither could ever match and the fallback was effectively dead code.Reproducer, using a repo whose submodule has
crlf text eol=crlfin its.gitattributes(git checkoutapplies that filter, libgit2 doesn't):Implementation
Submodule mounting moves into a
getTree(bool legacy)lambda, so both candidate trees are fully mounted before their NAR hashes are compared and the existing selection logic operates on comparable values.Reproducing a Nix < 2.20 hash also requires exporting the submodules with Nix < 2.20 semantics:
git checkoutapplies eol/textfilters inside submodules too, so mounting libgit2-exported submodules over agit checkout-exported top level cannot reconstruct the old hash. That is propagated through the synthesized submodule inputs via a new internal__legacyExportattribute:nix-219-compatdecides;The tri-state matters: without it,
getTree(false)inside anix-219-compatprocess would still export submodules the legacy way, breaking the modern-hash-under-compat direction.__legacyExportis deliberately not inallowedAttrs()and is never serialized into a lock file or a URL, following the existing__finalconvention.GitAccessorOptionsgains a matchinglegacyfield somakeFingerprint()accounts for it. It is appended last, which keeps legacy cache keys byte-identical to the previous hand-concatenatedoptions.makeFingerprint(rev) + ";legacy"— no cache invalidation.Testing
New regression test in
tests/functional/fetchGitSubmodules.shcovering both directions plus a genuinely-bad hash. It computes the two reference hashes in a throwaway store first, becausebuiltins.fetchTreemarks inputs carrying anarHashas final, andInput::getAccessorUnchecked()then returns an already-present store path without running the fetcher at all — which masks this bug, and is probably part of why it went unnoticed.meson test --suite mainand--suite flakespass, other than threebuild-remote-trustless-*tests that fail identically on an unmodified tree in my environment.Also verified against the real-world lock entry that prompted this — a repo with
submodules = 1andshallow = 1whose submodule tree contains 121 files affected by eol/textfilters. It now evaluates and emits:🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests