Backward compatibility hack for Nix < 2.20 Git inputs using git-archive #278
Conversation
WalkthroughReplaces boolean git-accessor flags with a new public GitAccessorOptions and threads it through accessor APIs; updates fingerprint generation/propagation and adds a legacy git-archive fallback (nix-2.19 compatibility toggle); updates call sites, tarball/github fingerprints, tests, and related settings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Caller / Test
participant Fetcher as GitInputScheme
participant Repo as GitRepoImpl
participant Accessor as GitSourceAccessor
participant Store as Nix Store
Client->>Fetcher: fetch(input, expectedNarHash)
Fetcher->>Repo: getAccessor(rev, GitAccessorOptions{})
Repo->>Accessor: construct accessor (options stored)
Accessor->>Accessor: compute/return fingerprint (root or via options)
Accessor-->>Repo: return accessor (with fingerprint)
Repo-->>Fetcher: accessor
Fetcher->>Fetcher: set result.cachedFingerprint (root or computed)
Fetcher->>Store: fetch via accessor
alt narHash matches
Fetcher-->>Client: success
else narHash mismatch
Fetcher->>Fetcher: attempt getLegacyGitAccessor (archive path, nix219Compat-aware)
Fetcher->>Store: fetch via legacy/archive accessor
alt archive narHash matches
Fetcher-->>Client: success (legacy fingerprint)
else
Fetcher-->>Client: error (narHash mismatch)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
fwiw my opposition to this is on the premise that it it's very messy conceptually what it is doing. It's more like a "well I sure hope git archive does what it used to" vs. a well reasoned set of backward compatible behaviors we know will function. I'm very strongly in favor of #275 instead, because we know exactly what it supports and does, and can remain confident that it will continue to do that over time. If we simply cannot achieve something that solves the 99% of backwards compatibility issues in #275 I'm not going to be obstinate about this implementation, but I don't want to go with this approach lightly. |
edolstra
force-pushed
the
detsys/eelcodolstra/fh-1150-legacy-eol-handling-v2
branch
from
90bb354 to
cbfb908
Compare
![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 details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/libfetchers-tests/git-utils.cc(1 hunks)src/libfetchers/fetchers.cc(2 hunks)src/libfetchers/git-utils.cc(7 hunks)src/libfetchers/git.cc(7 hunks)src/libfetchers/github.cc(1 hunks)src/libfetchers/include/nix/fetchers/git-utils.hh(2 hunks)src/libfetchers/tarball.cc(1 hunks)tests/functional/fetchGit.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/libfetchers/tarball.cc
- src/libfetchers-tests/git-utils.cc
- src/libfetchers/include/nix/fetchers/git-utils.hh
🧰 Additional context used
🧬 Code graph analysis (3)
src/libfetchers/fetchers.cc (2)
src/libfetchers/github.cc (2)
store(350-356)store(350-350)src/libfetchers/include/nix/fetchers/fetchers.hh (4)
store(158-158)store(176-176)store(243-246)store(243-243)
src/libfetchers/git.cc (1)
src/libfetchers/git-utils.cc (13)
rev(350-390)rev(350-350)rev(392-397)rev(392-392)rev(524-524)rev(555-555)rev(558-558)rev(629-700)rev(629-629)settings(702-716)settings(702-702)makeFingerprint(741-744)makeFingerprint(741-741)
src/libfetchers/git-utils.cc (2)
src/libfetchers/include/nix/fetchers/git-utils.hh (8)
rev(30-30)rev(39-39)rev(41-41)rev(93-93)rev(100-100)rev(115-115)wd(102-103)ref(46-46)src/libfetchers/git.cc (4)
getAccessor(984-1005)getAccessor(985-985)makeFingerprint(641-648)makeFingerprint(641-641)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_x86_64-linux / build
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (14)
src/libfetchers/git-utils.cc (7)
741-744: LGTM! Clean fingerprint implementation.The fingerprint format is simple and clear: revision + optional flags.
761-770: LGTM! Proper options propagation.The constructor correctly stores the options and initializes the fingerprint. Since
optionsis copied tostate->options, there's no lifetime issue.
1318-1321: LGTM! API signature updated correctly.The method now accepts
GitAccessorOptionsand passes it through to the accessor constructor.
1324-1334: LGTM! Correct conditional wrapping.The accessor is appropriately wrapped with
GitExportIgnoreSourceAccessoronly whenoptions.exportIgnoreis enabled.
1336-1350: LGTM! Consistent workdir accessor handling.The workdir accessor follows the same pattern as the commit accessor for export-ignore handling.
1357-1389: LGTM! Submodule accessor creation updated correctly.The method correctly creates accessors with appropriate options for reading
.gitmodules(with export-ignore) and for enumerating submodule revisions (without).
704-704: LGTM! Correct default options for tree hash conversion.Using empty
GitAccessorOptions{}is appropriate here since we want default behavior (no export-ignore, no LFS smudging) for NAR hash computation.src/libfetchers/github.cc (1)
329-329: LGTM! API call updated to use new options struct.The change from a boolean parameter to empty
GitAccessorOptions{}correctly aligns with the updated accessor API.src/libfetchers/fetchers.cc (2)
330-331: LGTM! Fingerprint propagation in substitution path.When using a pre-existing store path, the fingerprint is correctly retrieved from the Input and assigned to both the accessor and the cached field.
361-364: LGTM! Bidirectional fingerprint synchronization.The logic correctly handles two cases:
- If the accessor has a fingerprint (e.g., from git-archive fallback), propagate it to the result
- Otherwise, compute the fingerprint from the input and assign it to the accessor
This is essential for the backward-compatibility mechanism where the accessor type may change based on NAR hash validation.
src/libfetchers/git.cc (3)
641-648: LGTM! Clean fingerprint generation.The method correctly constructs
GitAccessorOptionsfrom input attributes and generates a fingerprint including the submodules suffix when applicable.
816-820: LGTM! Standard accessor creation with options.The code correctly constructs
GitAccessorOptionsand passes them togetAccessor.
654-672: Verified: git archive command correctly applies Git filters (export-ignore), temp directory cleanup is correct.The test confirms the implementation works as intended. The git archive command properly respects export-ignore attributes, ensuring only tracked files (excluding those marked with export-ignore) are included in the archive. The AutoDelete cleanup pattern is safe.
tests/functional/fetchGit.sh (1)
314-345: Concern about duplication is unfounded—test appears only once in file.The file is 345 lines total, with this test block appearing at the end. A search for the
fetchTreepattern withnarHashfound only 3 occurrences in the entire file—all consecutive within this single test block (lines 337, 341, 344). There is no duplicate test block elsewhere in the file.The test logic itself is sound:
- Old narHash with filters applied → works with warning
- New narHash without filters → works normally
- Wrong narHash → fails with mismatch error
The
.gitattributessetup correctly tests CRLF, export-ignore, and export-subst rules.
src/libfetchers/git.cc
Outdated
| /* Backward compatibility hack for locks produced by Nix < 2.20 that depend on Nix applying Git filters or | ||
| * `export-ignore`. Nix >= 2.20 doesn't do those, so we may get a NAR hash mismatch. If that happens, try again | ||
| * with filters and export-ignore enabled. */ | ||
| if (auto expectedNarHash = input.getNarHash()) { | ||
| if (accessor->pathExists(CanonPath(".gitattributes"))) { | ||
| auto narHashNew = | ||
| fetchToStore2(settings, *store, {accessor}, FetchMode::DryRun, input.getName()).second; | ||
| if (expectedNarHash != narHashNew) { | ||
| GitAccessorOptions options2{.exportIgnore = true}; | ||
| auto accessor2 = getGitArchiveAccessor(*store, repoInfo, repoDir, rev); | ||
| accessor2->fingerprint = options2.makeFingerprint(rev) + ";f"; | ||
| auto narHashOld = | ||
| fetchToStore2(settings, *store, {accessor2}, FetchMode::DryRun, input.getName()).second; | ||
| if (expectedNarHash == narHashOld) { | ||
| warn( | ||
| "Git input '%s' specifies a NAR hash '%s' that was created by Nix < 2.20.\n" | ||
| "Nix >= 2.20 does not apply Git filters and `export-ignore` by default, which changes the NAR hash.\n" | ||
| "Please update the NAR hash to '%s'.", | ||
| input.to_string(), | ||
| expectedNarHash->to_string(HashFormat::SRI, true), | ||
| narHashNew.to_string(HashFormat::SRI, true)); | ||
| accessor = accessor2; | ||
| options = options2; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Backward-compatibility mechanism works but has some concerns.
The implementation correctly detects NAR hash mismatches and falls back to git-archive-based access. However, there are several points to consider:
-
Limited filter detection: The code only checks for
.gitattributesexistence, but Git filters can also be configured in.git/info/attributes,~/.gitattributes, and system-wide config. This means the fallback won't trigger for repos using filters configured outside.gitattributes. -
Unclear fingerprint suffix: The
";f"suffix in line 832 is not documented. What does it represent? It should be consistent with other suffixes like";e"(export-ignore) and";l"(LFS). -
Performance: On NAR hash mismatch, this performs two full NAR hash computations (lines 828 and 834). While using
DryRunmode, this could still be expensive for large repos. Consider if there's a way to optimize or cache the computation. -
Incomplete filter coverage: The fallback creates
GitAccessorOptions{.exportIgnore = true}but doesn't enablesmudgeLfs. If a lock file was created with Nix < 2.20 using LFS, won't this also mismatch? -
User experience: As noted in PR comments, this approach relies on "hoping that git archive behaves as expected" rather than explicitly matching Nix < 2.20 behavior. Consider if the warning message (lines 836-842) should be more prominent or if there should be a way to opt out.
Please clarify:
And verify filter detection:
🏁 Script executed:
#!/bin/bash
# Test if filters outside .gitattributes are detected
mkdir /tmp/test-git-filters
cd /tmp/test-git-filters
git init
git config user.email "[email protected]"
git config user.name "Test"
# Configure filter in .git/info/attributes instead of .gitattributes
mkdir -p .git/info
echo "*.txt export-ignore" > .git/info/attributes
echo "test" > file.txt
echo "ignored" > ignored.txt
git add .
git commit -m "test"
# Check if git archive respects .git/info/attributes
git archive HEAD | tar -t
cd /
rm -rf /tmp/test-git-filtersLength of output: 741
🌐 Web query:
What does the ";f" fingerprint suffix represent in Git accessor fingerprints?
💡 Result:
I don't have enough context to answer—could you paste an example line (where the ";f" appears) or tell me which tool/output shows the "accessor fingerprint"?
Now let me search the codebase for the fingerprint suffix meanings.
Based on my verification, I can now generate the rewritten review comment. The web search and shell test have confirmed several of the concerns:
-
Git archive by default takes attributes from the .gitattributes files in the tree being archived, but can also check $GIT_DIR/info/attributes, which means the code's reliance on only checking
.gitattributesmay miss filters configured elsewhere. -
The export-ignore parameter "approximates part of the git archive behavior" but "it is unknown whether the Git developers commit to the reproducibility of export-ignore in newer Git versions", which validates the concern about relying on git archive behavior.
-
The shell test confirmed that git archive does not respect
.git/info/attributeswhen set after committing files—supporting the concern about limited filter detection.
Backward-compatibility mechanism has valid concerns but logic is sound.
The fallback approach correctly detects NAR hash mismatches and switches to git-archive-based access. However, verification confirms several important limitations:
-
Limited filter detection: The code only checks
.gitattributesexistence (line 826), but git archive can also apply attributes from$GIT_DIR/info/attributes. Repositories using filters configured outside.gitattributeswon't trigger the fallback. -
Unreliable export-ignore behavior: The export-ignore parameter approximates git archive behavior, but "it is unknown whether the Git developers commit to the reproducibility of export-ignore in newer Git versions". This undermines the fallback's effectiveness for future Git versions.
-
Incomplete filter coverage: The fallback creates
GitAccessorOptions{.exportIgnore = true}but does not enablesmudgeLfs. If a lock file was created with Nix < 2.20 using LFS, this fallback won't match. -
Performance cost: On NAR hash mismatch, two full NAR hash computations are performed (lines 828 and 834), which could be expensive for large repositories.
-
Unclear fingerprint suffix: The
";f"suffix added to the fingerprint (line 832) is not documented. Its purpose and relationship to other suffixes like";e"(export-ignore) and";l"(LFS) should be clarified.
Consider adding a comment explaining the fingerprint suffix strategy and document why only exportIgnore is enabled in the fallback path, not LFS.
🤖 Prompt for AI Agents
In src/libfetchers/git.cc around lines 822-848, the fallback logic only checks
for .gitattributes, omits LFS in the alternate GitAccessorOptions, performs
extra hash work, and adds an undocumented ";f" fingerprint suffix; update the
code to also detect $GIT_DIR/info/attributes (e.g. check
repoDir/.git/info/attributes or via the accessor/git config API) so filters
configured outside .gitattributes trigger the fallback, set GitAccessorOptions
to enable both exportIgnore = true and smudgeLfs = true when trying the
archive-based accessor so LFS-produced NARs are matched, restructure the logic
to avoid redundant full hash computations (compute the DryRun hash for the
archive-style accessor only when needed and reuse it), and add a short code
comment documenting the fingerprint suffix convention (what ";f" means and how
it relates to other suffixes like export-ignore or LFS) and why only these
options are toggled in the fallback.
cbfb908 to
6e632cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/libfetchers/git.cc (1)
813-818: Legacy narHash fallback works but keeps earlier limitationsThe backward‑compatibility block correctly detects a narHash mismatch, computes:
narHashNewfrom the libgit2 accessor, andnarHashOldfrom a git‑archive accessor,and if
narHashOldmatches the user‑suppliednarHash, it switches to the archive‑based accessor and warns with the new hash. The use ofGitAccessorOptionsand the";f"suffix on the accessor fingerprint also ensures the store sees this as a distinct source.The earlier concerns still largely apply:
Limited filter detection: the fallback is gated purely on
.gitattributesexisting in the tree (pathExists(".gitattributes")). Repos that relied on filters configured in$GIT_DIR/info/attributesor global/system attributes won’t trigger the compatibility path, even if their old NARs depended on those filters.Filters vs. LFS coverage:
options2only setsexportIgnore = true;smudgeLfsstays false. Any pre‑2.20 lockfiles whose NAR hashes depended on LFS smudging will remain mismatched. If supporting those is in scope, you probably wantGitAccessorOptions{.exportIgnore = true, .smudgeLfs = true}here and corresponding tests.Extra work on mismatch: for mismatched narHashes you always:
- compute
narHashNewviafetchToStore2(DryRun, accessor); then- build a git‑archive tree and compute
narHashOldvia anotherfetchToStore2(DryRun, accessor2).
This is fine for rare legacy cases, but if you expect many such inputs, consider cachingnarHashNew/narHashOldsomewhere (e.g. keyed by rev + fingerprint) to avoid recomputing them across evaluations.Undocumented
";f"suffix: the";f"suffix you append to the archive accessor’s fingerprint is not documented near either fingerprint helper. A short comment explaining that;fmeans “legacy filters applied via git-archive” would make the convention clearer and help keep suffix semantics aligned withGitAccessorOptions::makeFingerprint/GitInputScheme::makeFingerprint.Attribute mismatch between input and options: in the successful fallback branch you assign
options = options2, but you don’t update the in‑memoryInputattrs (exportIgnore/lfs). As noted in the other comment, this meansgetFingerprint()for theInputwill not reflect the fallback choice, whereas the accessor fingerprint does. If you rely on both for caching, it may be safer to also update the attrs when the fallback is accepted.Submodule and workdir plumbing (propagating
exportIgnore/lfsdown to submodule inputs and using.exportIgnorefor workdir accessors) looks consistent with the new options model; the main open questions are around how broad you want this BC shim to be and how visibly you want to document the fingerprint and filter behavior.To confirm how much real‑world surface this covers, you might want to audit a sample of existing pre‑2.20 lockfiles for:
- presence of
.gitattributesvs.$GIT_DIR/info/attributes,- use of
export-ignore/export-substonly vs. additional filters/LFS.#!/usr/bin/env bash # Example: scan a checkout of known lockfiles for .gitattributes usage. fd -a '.gitattributes' . 2>/dev/null | headAlso applies to: 819-845, 853-879, 898-913
🧹 Nitpick comments (3)
src/libfetchers/include/nix/fetchers/git-utils.hh (1)
25-31: Options struct + accessor API change look coherentIntroducing
GitAccessorOptionsand threading it through theGitRepo::getAccessoroverloads is a straightforward, type-safe replacement for ad‑hoc booleans. CentralizingmakeFingerprinthere also keeps exportIgnore/smudgeLfs suffix logic in one place. It might be worth adding a brief comment nearGitAccessorOptionsdocumenting what the;e/;lsuffixes represent, but the design itself looks good.Also applies to: 99-103
src/libfetchers/git-utils.cc (1)
741-770: Centralized fingerprint + LFS handling is consistent
GitAccessorOptions::makeFingerprintand the updatedGitSourceAccessorconstructor cleanly couple fingerprinting and LFS behavior to the options object: fingerprints now distinguishexportIgnoreandsmudgeLfs, andlfs::Fetchis only instantiated when requested. This matches how options are assembled ingit.ccand avoids hidden global behavior.You may eventually want to use
state->optionsdirectly inreadBlobinstead of the separatelfsFetchfield, but that’s purely cosmetic right now.src/libfetchers/git.cc (1)
647-669: ---Verify permissions and symlinks are preserved through the full round-trip
getGitArchiveAccessorimplements BC correctly: rungit archivefor the given rev, unpack to temp, add to store, expose via FS accessor. The sanity check script confirmsgit archivepreserves file metadata (tested with basic file permissions). However, this validates only the archive creation step—not the full round-trip throughunpackTarfileandstore.addToStore.A couple of optional improvements if this path becomes hot:
- You always materialize a store path even when you only need a DryRun narHash comparison and might discard the result. If possible, consider a variant that hashes directly from the temp directory (or even from the archive stream) when running in DryRun mode.
- If
git archiveexits non‑zero, you currently rely onrunProgram2throwing; that's fine, but a short comment here that BC depends ongitbeing available on PATH (unlike the libgit2 path) could help future maintainers.To gain full confidence, verify that symlinks are preserved through the complete unpack and store-add cycle, and that the result matches Nix < 2.20 behavior on your target platforms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/libfetchers-tests/git-utils.cc(1 hunks)src/libfetchers/fetchers.cc(2 hunks)src/libfetchers/git-utils.cc(7 hunks)src/libfetchers/git.cc(7 hunks)src/libfetchers/github.cc(1 hunks)src/libfetchers/include/nix/fetchers/git-utils.hh(2 hunks)src/libfetchers/tarball.cc(1 hunks)tests/functional/fetchGit.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/libfetchers/github.cc
- src/libfetchers-tests/git-utils.cc
- src/libfetchers/fetchers.cc
🧰 Additional context used
🧬 Code graph analysis (4)
src/libfetchers/tarball.cc (1)
src/libfetchers/github.cc (16)
settings(35-111)settings(36-36)settings(127-135)settings(127-127)settings(182-204)settings(182-183)settings(206-213)settings(207-207)settings(215-228)settings(215-216)settings(236-236)settings(238-238)settings(246-316)settings(246-246)settings(318-339)settings(319-319)
src/libfetchers/git.cc (2)
src/libfetchers/git-utils.cc (11)
rev(350-390)rev(350-350)rev(392-397)rev(392-392)rev(524-524)rev(555-555)rev(558-558)rev(629-700)rev(629-629)makeFingerprint(741-744)makeFingerprint(741-741)src/libutil/include/nix/util/source-accessor.hh (1)
getFSSourceAccessor(228-228)
src/libfetchers/include/nix/fetchers/git-utils.hh (2)
src/libfetchers/git-utils.cc (10)
rev(350-390)rev(350-350)rev(392-397)rev(392-392)rev(524-524)rev(555-555)rev(558-558)rev(629-700)rev(629-629)wd(561-561)src/libutil/include/nix/util/source-accessor.hh (1)
displayPrefix(164-164)
src/libfetchers/git-utils.cc (2)
src/libfetchers/include/nix/fetchers/git-utils.hh (8)
rev(30-30)rev(39-39)rev(41-41)rev(93-93)rev(100-100)rev(115-115)wd(102-103)ref(46-46)src/libfetchers/git.cc (4)
getAccessor(981-1002)getAccessor(982-982)makeFingerprint(641-645)makeFingerprint(641-641)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (3)
tests/functional/fetchGit.sh (1)
314-345: Backward-compat test coverage looks solidThe new
git-eolblock exercises old vs. new narHash behavior, export-ignore, export-subst, and CRLF vs. LF semantics, and verifies both the warning text and the hard failure path. The setup and assertions are consistent with the rest of this script; no changes requested here.src/libfetchers/tarball.cc (1)
129-141: Accessor options defaulting preserves tarball behaviorSwitching to
getAccessor(treeHash, {}, displayPrefix)correctly adapts to the new options-based API while keeping tarball-cache accessors free of exportIgnore/LFS semantics. This should be behaviorally identical to the prior boolean call site.src/libfetchers/git-utils.cc (1)
552-562: Accessor option plumbing is correct—no issues foundThe grep results confirm that all
getAccessorcall sites pass explicitGitAccessorOptions:
git-utils.cc:704and1362pass{}or explicit{.exportIgnore = ...}git.cc:817and912passoptionsor explicit{.exportIgnore = ...}tarball.cc:139andgithub.cc:329pass{}No call sites omit the options parameter. The review comment's analysis is accurate.
grahamc
left a comment
grahamc
left a comment
There was a problem hiding this comment.
(leaving a stronger indicator than my previous note to avoid accidental merge)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/libfetchers/include/nix/fetchers/fetch-settings.hh (1)
126-134: Clarifynix-219-compatsemantics and intended usage in the help textThe
Setting<bool>wiring looks fine, but the description is a bit strong/ambiguous:
- “Nix will use
git archiverather thanlibgit2to copy Git inputs” reads like an unconditional override of the implementation. If the actual behavior is narrower (e.g., only for lock generation, or only in specific fallback paths), consider tightening the wording so users don’t over-assume what’s guaranteed.- It may help to explicitly call out that this is a migration/compatibility knob (for generating or working with Nix 2.19-style locks) and is not recommended for long‑term use, especially since the following sentence already warns that resulting locks may not be compatible with Nix ≥ 2.20.
Clearer wording here will make it easier for users to understand exactly when to enable this and what compatibility guarantees they are (and are not) getting.
src/libfetchers/git.cc (1)
651-674: Document fingerprint suffix and consider LFS handling.Two concerns with this git archive fallback accessor:
Undocumented fingerprint suffix: Line 671 appends
";f"to the fingerprint, but its meaning is not documented in code. Add a comment explaining that this suffix indicates the archive-based accessor path.Incomplete filter coverage: Line 670 only enables
exportIgnore = truebut notsmudgeLfs. If a Nix < 2.20 lock file was created with LFS-smudged content, this fallback won't match that narHash. Consider whether LFS should also be enabled here for complete backward compatibility.Apply this diff to document the suffix:
+ // Create an accessor using git archive for backward compatibility. + // The ";f" fingerprint suffix indicates this is an archive-based accessor + // that applies export-ignore filters. GitAccessorOptions options{.exportIgnore = true}; accessor->fingerprint = options.makeFingerprint(rev) + ";f";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/libfetchers/git.cc(7 hunks)src/libfetchers/include/nix/fetchers/fetch-settings.hh(1 hunks)tests/functional/fetchGit.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/libfetchers/git.cc (2)
src/libfetchers/git-utils.cc (11)
rev(350-390)rev(350-350)rev(392-397)rev(392-392)rev(524-524)rev(555-555)rev(558-558)rev(629-700)rev(629-629)makeFingerprint(741-744)makeFingerprint(741-741)src/libutil/include/nix/util/source-accessor.hh (1)
getFSSourceAccessor(228-228)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (5)
tests/functional/fetchGit.sh (1)
314-369: Comprehensive backward compatibility test coverage.The test successfully exercises the Nix < 2.20 compatibility path by:
- Creating a repository with CRLF line endings and gitattributes (export-ignore, export-subst)
- Verifying old narHash matches legacy git archive behavior (CRLF, ignored files removed)
- Verifying new narHash matches modern libgit2 behavior (LF, ignored files present)
- Testing narHash mismatch error handling
- Testing flake locking with both
--nix-219-compatand default semanticsHowever, note line 366 directly removes the SQLite cache file with a FIXME comment. Consider using a more robust cache invalidation mechanism or documenting why direct file removal is necessary here.
src/libfetchers/git.cc (4)
641-645: Good refactoring - fingerprint construction is now consistent.Extracting fingerprint construction into a helper method improves code clarity and ensures consistent suffix semantics across all call sites.
873-898: Submodule options propagation is correct.The changes properly propagate
exportIgnoreandsmudgeLfsoptions from the parent repository to submodules (lines 896, 898), ensuring consistent behavior across the repository tree.
932-932: Workdir accessor correctly uses options struct.The conversion from boolean parameters to
GitAccessorOptionsstruct is consistent with the overall refactoring in this PR.
1024-1048: Fingerprint generation now uses consistent helper.Both the clean (line 1027) and dirty (line 1043) paths now use the
makeFingerprinthelper, ensuring consistent fingerprint construction across all scenarios.
| if (settings.nix219Compat && !options.smudgeLfs && accessor->pathExists(CanonPath(".gitattributes"))) { | ||
| /* Use Nix 2.19 semantics to generate locks, but if a NAR hash is specified, support Nix >= 2.20 semantics | ||
| * as well. */ | ||
| warn("Using Nix 2.19 semantics to export Git repository '%s'.", input.to_string()); | ||
| auto accessorModern = accessor; | ||
| accessor = getGitArchiveAccessor(*store, repoInfo, repoDir, rev); | ||
| if (expectedNarHash) { | ||
| auto narHashLegacy = | ||
| fetchToStore2(settings, *store, {accessor}, FetchMode::DryRun, input.getName()).second; | ||
| if (expectedNarHash != narHashLegacy) { | ||
| auto narHashModern = | ||
| fetchToStore2(settings, *store, {accessorModern}, FetchMode::DryRun, input.getName()).second; | ||
| if (expectedNarHash == narHashModern) | ||
| accessor = accessorModern; | ||
| } | ||
| } | ||
| } else { | ||
| /* Backward compatibility hack for locks produced by Nix < 2.20 that depend on Nix applying Git filters, | ||
| * `export-ignore` or `export-subst`. Nix >= 2.20 doesn't do those, so we may get a NAR hash mismatch. If | ||
| * that happens, try again using `git archive`. */ | ||
| auto narHashNew = fetchToStore2(settings, *store, {accessor}, FetchMode::DryRun, input.getName()).second; | ||
| if (expectedNarHash && accessor->pathExists(CanonPath(".gitattributes"))) { | ||
| if (expectedNarHash != narHashNew) { | ||
| auto accessorLegacy = getGitArchiveAccessor(*store, repoInfo, repoDir, rev); | ||
| auto narHashLegacy = | ||
| fetchToStore2(settings, *store, {accessorLegacy}, FetchMode::DryRun, input.getName()).second; | ||
| if (expectedNarHash == narHashLegacy) { | ||
| warn( | ||
| "Git input '%s' specifies a NAR hash '%s' that was created by Nix < 2.20.\n" | ||
| "Nix >= 2.20 does not apply Git filters, `export-ignore` and `export-subst` by default, which changes the NAR hash.\n" | ||
| "Please update the NAR hash to '%s'.", | ||
| input.to_string(), | ||
| expectedNarHash->to_string(HashFormat::SRI, true), | ||
| narHashNew.to_string(HashFormat::SRI, true)); | ||
| accessor = accessorLegacy; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
Backward compatibility logic works but has detection limitations.
The dual-path approach correctly handles both Nix 2.19 compatibility mode and automatic fallback for older lock files. However, there are some limitations:
-
Limited filter detection: Lines 827 and 848 only check for
.gitattributesfile existence. Git can also apply filters from$GIT_DIR/info/attributes,~/.config/git/attributes, and system-wide config. Repositories using filters configured outside.gitattributeswon't trigger the fallback, leading to potential narHash mismatches. -
Performance trade-off: When a narHash mismatch occurs, the code performs two full NAR hash computations (lines 847+852 or 835+838). While using
DryRunmode, this could still be expensive for large repositories. This appears to be an intentional trade-off for correctness. -
Detection scope: The fallback only triggers when
.gitattributesexists, even though Git filters can affect the tree without an explicit attributes file (e.g., via global config).
Consider documenting these limitations in the warning messages or code comments so users understand when the fallback may not work.
Based on learnings from past reviews, consider verifying whether common Git configurations use .git/info/attributes or global attribute files:
🏁 Script executed:
#!/bin/bash
# Check if Git repositories commonly use alternative attribute sources
# Search for .git/info/attributes files in cached repos
cache_dir="${HOME}/.cache/nix/gitv3"
if [ -d "$cache_dir" ]; then
echo "Checking for .git/info/attributes files in cached repos..."
fd -H -t f "^attributes$" "$cache_dir" --exec sh -c 'grep -l "export-ignore\|export-subst\|eol=" "{}" 2>/dev/null' | head -20
echo "---"
echo "Total cached repos with .git/info/attributes filters:"
fd -H -t f "^attributes$" "$cache_dir" --exec sh -c 'grep -l "export-ignore\|export-subst\|eol=" "{}" 2>/dev/null' | wc -l
fiLength of output: 0
I need to gather more information to verify the review comment's concerns. Let me search the codebase for tests and documentation related to this logic, and check Git's attribute detection behavior.
Based on the web search results, Git consults $GIT_DIR/info/attributes file (which has the highest precedence) when deciding what attributes are assigned to a path, followed by .gitattributes files. The search results confirm that export-ignore can be placed in $GIT_DIR/info/attributes file, and this is indeed a valid location for keeping attributes that apply while archiving.
The review comment's concern is valid: the code at lines 827 and 848 only checks for .gitattributes file existence, but would miss filters configured in $GIT_DIR/info/attributes, which actually takes precedence in Git's attribute resolution order. Repositories using .git/info/attributes for export-ignore or export-subst attributes would not trigger the backward compatibility fallback, potentially resulting in undetected NAR hash mismatches.
Backward compatibility logic correctly handles known cases but misses filters in .git/info/attributes.
The filter detection at lines 827 and 848 only checks for .gitattributes presence. However, Git consults $GIT_DIR/info/attributes file (which has the highest precedence) for filter attributes. Repositories using .git/info/attributes for export-ignore or export-subst configuration will not trigger the fallback, potentially missing NAR hash mismatches.
The performance trade-off (dual hash computations on mismatch) is acceptable for correctness. Consider checking both .gitattributes and .git/info/attributes to improve filter detection coverage.
…haviour Before Nix 2.20, we used git, which applies Git filters (in particular doing end-of-line conversion based on .gitattributes), `export-ignore` and `export-subst`. In 2.20, we switched to libgit2 and stopped applying those, which is probably better for reproducibility (e.g. `export-subst` can do pretty crazy things). However, that breaks existing lock files / fetchTree calls for Git inputs that rely on them, since it invalidates the NAR hash. So as a backward compatibility hack, we now check the NAR hash computed over the Git tree without them. If there is a hash mismatch, we try again *with* them (by calling `git archive`, just like Nix < 2.20). If that succeeds, we print a warning and return the filtered tree. This is inefficient, but should be fine for legacy flakes.
This makes Nix generate Nix 2.19 compatible lock files by default (i.e. using `git archive`), while still supporting newer lock files.
edolstra
force-pushed
the
detsys/eelcodolstra/fh-1150-legacy-eol-handling-v2
branch
from
c994977 to
90ca8c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/libfetchers/git.cc (1)
650-681: Git archive accessor implementation for backward compatibility.The
getGitArchiveAccessormethod correctly:
- Creates a temporary directory for unpacking
- Runs
git archiveto apply filters and export rules- Unpacks and adds to store
- Sets appropriate fingerprint with
;fsuffixHowever, the
;fsuffix (line 678) is undocumented. Consider adding a brief comment explaining that;findicates "fallback" or "filtered via git-archive" to match the documentation pattern of;e(exportIgnore) and;l(LFS).+ // ";f" suffix indicates this accessor was produced via git-archive fallback + // for Nix < 2.20 compatibility (applying Git filters, export-ignore, export-subst) accessor->fingerprint = options.makeFingerprint(rev) + ";f";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/libfetchers/fetchers.cc(2 hunks)src/libfetchers/git-utils.cc(7 hunks)src/libfetchers/git.cc(8 hunks)src/libfetchers/include/nix/fetchers/fetch-settings.hh(1 hunks)src/libfetchers/include/nix/fetchers/git-utils.hh(2 hunks)tests/functional/fetchGit.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libfetchers/fetchers.cc
🧰 Additional context used
🧬 Code graph analysis (3)
src/libfetchers/git.cc (1)
src/libfetchers/include/nix/fetchers/git-utils.hh (7)
rev(31-31)rev(40-40)rev(42-42)rev(94-94)rev(101-101)rev(116-116)settings(122-122)
src/libfetchers/include/nix/fetchers/git-utils.hh (2)
src/libfetchers/git-utils.cc (10)
rev(350-390)rev(350-350)rev(392-397)rev(392-392)rev(524-524)rev(555-555)rev(558-558)rev(629-700)rev(629-629)wd(561-561)src/libutil/include/nix/util/source-accessor.hh (1)
displayPrefix(164-164)
src/libfetchers/git-utils.cc (1)
src/libfetchers/include/nix/fetchers/git-utils.hh (8)
rev(31-31)rev(40-40)rev(42-42)rev(94-94)rev(101-101)rev(116-116)wd(103-104)ref(47-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (12)
src/libfetchers/include/nix/fetchers/fetch-settings.hh (1)
126-134: Well-documented compatibility setting.The
nix219Compatsetting is clearly documented and appropriately defaults tofalse, making the legacy behavior opt-in. The documentation correctly warns users about the trade-off with Nix >= 2.20 compatibility.src/libfetchers/include/nix/fetchers/git-utils.hh (2)
25-32: Clean options struct design.The
GitAccessorOptionsstruct consolidates the boolean flags into a well-organized structure. The comment on line 29 clarifying where submodules are implemented is helpful for maintainability.
100-104: Consistent API update.Both
getAccessoroverloads now acceptGitAccessorOptions, providing a uniform interface for configuring accessor behavior.tests/functional/fetchGit.sh (2)
313-345: Comprehensive backward compatibility tests.The test suite properly validates:
- CRLF line ending conversion behavior
export-ignoreattribute handlingexport-substsubstitution- NAR hash matching for both legacy and modern semantics
- Warning messages when updating from old hashes
347-368: Good flake locking test coverage.The flake tests verify that:
--nix-219-compatproduces legacy-compatible locks witholdHash- Default behavior produces modern locks with
newHash- Both lock types can be consumed by either mode
src/libfetchers/git.cc (3)
641-648: Helper method cleanly extracts options from input.The
getGitAccessorOptionsmethod provides a clean abstraction for constructing the options struct from input attributes.
831-869: Backward compatibility logic correctly handles both modes.The dual-path approach is sound:
- When
nix219Compatis enabled, prefer git-archive output but accept modern hashes- When disabled, prefer libgit2 output but fall back to git-archive for legacy hashes
The warning message (lines 858-864) is helpful and actionable, telling users exactly what hash to use.
Note: As flagged in previous reviews, lines 831 and 852 only check for
.gitattributesfile existence. Git can also apply filters from$GIT_DIR/info/attributes, which this logic won't detect. This is an edge case but worth documenting in comments.Consider documenting this limitation in a code comment:
+ // Note: This only checks for .gitattributes in the tree. Git can also apply + // attributes from $GIT_DIR/info/attributes, which we don't detect here. if (expectedNarHash && accessor->pathExists(CanonPath(".gitattributes"))) {
1032-1037: FIXME appropriately documents fingerprint divergence.The FIXME comment correctly acknowledges that
getFingerprintmay return an inconsistent value for the legacy git-archive case. Since the accessor fingerprint (with;fsuffix) is set directly on the accessor object, this divergence is contained. The comment helps future maintainers understand this edge case.src/libfetchers/git-utils.cc (4)
741-744: Clear fingerprint suffix convention.The
makeFingerprintimplementation establishes a clear suffix scheme:
;eforexportIgnore;lforsmudgeLfsThis is consistent with the
;fsuffix used ingit.ccfor the git-archive fallback path.
761-769: Options correctly stored and fingerprint initialized.The
GitSourceAccessorconstructor properly:
- Stores options in state for later use (e.g., LFS smudging in
readBlob)- Initializes the fingerprint using
options.makeFingerprint(rev)
1318-1334: Accessor methods correctly propagate options.The
getRawAccessorandgetAccessorimplementations properly pass the options through to the accessor construction and export-ignore wrapper.
1357-1389: Submodule handling uses options-based approach.The
getSubmodulesmethod correctly:
- Passes
exportIgnoreoption to the accessor (line 1362)- Uses empty options
{}for the raw accessor when enumerating submodules (line 1379)This ensures export-ignore rules are respected when reading
.gitmodulesbut not when looking up submodule revisions.
That's what Nix < 2.20 did (which means that it behaved differently from the `submodules = false`, by not applying `export-ignore`).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/libfetchers/git.cc (2)
650-702: Verify LFS handling in legacy Git accessor.The legacy Git accessor correctly implements Nix < 2.20 semantics for submodules (via
git checkout) and non-submodules (viagit archive). However, at line 683, onlyexportIgnoreis set totruefor the archive path, whilesmudgeLfsis not explicitly enabled.If Nix < 2.20 applied LFS smudging and a lock file was created with LFS enabled, this fallback may not match the expected NAR hash. Consider whether
smudgeLfsshould also be enabled here, or document why it's safe to omit.Additionally, the
";legacy"fingerprint suffix (line 699) is not documented in code. Consider adding a comment explaining its purpose and relationship to other suffixes.Based on learnings from past reviews, the incomplete LFS coverage in the fallback path is a known concern.
1054-1080: Fingerprint logic has documented limitation for legacy cases.The FIXME at line 1057 correctly identifies that
getFingerprint()cannot determine whether the;legacysuffix should be appended, since that decision is made later ingetAccessorFromCommit()based on NAR hash matching.This means the
Input-level fingerprint may diverge from the actual accessor fingerprint when the legacy path is taken. While this is a known limitation (already flagged in past reviews), consider adding a comment explaining that for legacy NAR hash cases, the accessor's fingerprint is authoritative and may differ fromgetFingerprint().Based on learnings from past reviews, the fingerprint divergence between Input and accessor is a known issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/libfetchers/git.cc(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/libfetchers/git.cc (3)
src/libfetchers/include/nix/fetchers/git-utils.hh (6)
rev(31-31)rev(40-40)rev(42-42)rev(94-94)rev(101-101)rev(116-116)src/libfetchers/git-utils.cc (25)
rev(350-390)rev(350-350)rev(392-397)rev(392-392)rev(524-524)rev(555-555)rev(558-558)rev(629-700)rev(629-629)path(466-469)path(466-466)path(772-796)path(772-772)path(798-801)path(798-798)path(803-807)path(803-803)path(809-840)path(809-809)path(842-863)path(842-842)path(865-868)path(865-865)path(874-884)path(874-874)src/libutil/include/nix/util/mounted-source-accessor.hh (1)
makeMountedSourceAccessor(18-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_aarch64-darwin / build
- GitHub Check: build_x86_64-linux / build
🔇 Additional comments (5)
src/libfetchers/git.cc (5)
19-19: LGTM: Include added for NAR hash computation.The
fetch-to-store.hhinclude is necessary for thefetchToStore2calls used in the backward-compatibility logic.
641-648: LGTM: Clean abstraction for options construction.Consolidating the options construction in a single helper method improves consistency and reduces duplication throughout the file.
896-938: LGTM: Submodule handling correctly propagates options.The submodule logic properly propagates
exportIgnore,lfs, andsubmodulesflags through the recursive input construction (lines 922-924), and the fingerprint suffix";s"correctly indicates submodule mounting (line 933).
960-960: LGTM: Workdir accessor uses structured options.The migration from a boolean parameter to structured
GitAccessorOptionsimproves API consistency.
853-891: Based on my investigation, I can now provide the rewritten review comment.The filter detection concern about
.git/info/attributesis partially invalid for this codebase.The review comment raises concerns about filter detection being limited to
.gitattributesonly, citing missing checks for.git/info/attributes. However, this concern does not apply to the main code paths:
Legacy path (git archive): Uses the
git archivecommand, which by design only consults.gitattributesin the tree—not.git/info/attributes(since.git/is not exported). The check on line 853 and 874 for.gitattributesis correct and sufficient.Modern path (libgit2): While libgit2 respects git's filter precedence, including
.git/info/attributes, the code does not need to pre-check filter file locations since the NAR hash mismatch detection on lines 861-868 and 873-891 will catch any hash differences regardless of filter source.The dual NAR hash computations on mismatch (lines 861, 864, 873, 878) are an acceptable trade-off for correctness, though potentially expensive for large repositories.
The code does not require changes to check
.git/info/attributesseparately, as the existing NAR hash comparison logic handles filter-induced changes from any source.
edolstra
force-pushed
the
detsys/eelcodolstra/fh-1150-legacy-eol-handling-v2
branch
from
9d45626 to
f3aeeca
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/libflake/flakeref.cc (1)
63-66: Clarify and testnix219Compatsemantics arounddirstrippingThe gating here looks correct: when
nix219Compatis false we preserve existing behavior (always erase"dir"), and when it is true we intentionally let"dir"flow intoInput::fromURL, which is what you need for Nix≤2.19–style Git inputs.The subtle part is that
FlakeRef::canonicalize()still strips a matchingdirfrominput.attrs["url"]unconditionally. If any flows both (a) rely onnix219Compatfor legacy Git hashing and (b) callcanonicalize()before constructing the accessor, they will no longer see the legacy?dir=...URL even in compat mode.Two suggestions:
- Add a short comment here tying this to Nix 2.19 Git behavior and noting that
canonicalize()is responsible for normalizing lock entries, e.g.:- auto dir = getOr(parsedURL.query, "dir", ""); - if (!fetchSettings.nix219Compat) - parsedURL.query.erase("dir"); + auto dir = getOr(parsedURL.query, "dir", ""); + // In nix219Compat mode we intentionally keep `dir` in the underlying URL + // so Git inputs see the same query string as in Nix ≤ 2.19. + // FlakeRef::canonicalize() still strips it when normalising lock entries. + if (!fetchSettings.nix219Compat) + parsedURL.query.erase("dir");
- Double‑check (and ideally cover with tests) that the hash‑compatibility path exercises a non‑canonicalised
FlakeRef, and that enablingnix219Compatglobally doesn’t introduce surprising behavior for non‑Git/path flake refs carrying adirquery.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/libflake/flakeref.cc(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_aarch64-darwin / build
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/libfetchers/git-utils.cc(7 hunks)src/libfetchers/github.cc(2 hunks)src/libfetchers/mercurial.cc(1 hunks)src/libfetchers/tarball.cc(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/libfetchers/github.cc (1)
src/libfetchers/include/nix/fetchers/git-utils.hh (6)
rev(31-31)rev(40-40)rev(42-42)rev(94-94)rev(101-101)rev(116-116)
src/libfetchers/git-utils.cc (1)
src/libfetchers/include/nix/fetchers/git-utils.hh (8)
rev(31-31)rev(40-40)rev(42-42)rev(94-94)rev(101-101)rev(116-116)wd(103-104)ref(47-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_x86_64-linux / build
- GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (10)
src/libfetchers/mercurial.cc (1)
345-351: LGTM! Consistent fingerprint prefixing for Mercurial inputs.The
"hg:"prefix aligns with the standardized fingerprint format introduced across other fetchers (e.g.,"github:","tarball:","git:"), improving clarity and preventing potential collisions between different input types.src/libfetchers/github.cc (1)
328-329: LGTM! Correct migration to options-based accessor API.The empty initializer
{}properly constructs a defaultGitAccessorOptionsstruct, matching the updated signature ingit-utils.hh.src/libfetchers/tarball.cc (2)
139-140: LGTM! Correct migration to options-based accessor API.The empty initializer
{}properly matches the updatedgetAccessorsignature expectingGitAccessorOptions.
407-415: LGTM! Consistent fingerprint prefixing for tarball inputs.Adding
"tarball:"prefix to bothnarHashandrevfingerprints aligns with the standardized format across fetchers and distinguishes tarball inputs from other types.src/libfetchers/git-utils.cc (6)
741-744: LGTM! Clean fingerprint generation with option flags.The
makeFingerprintmethod provides a consistent way to generate fingerprints that encode the accessor options. The format"git:<rev>[;e][;l]"clearly distinguishes different configurations.
751-770: LGTM! Well-structured migration to options-based state.The
GitSourceAccessornow storesGitAccessorOptionsin its state, and the constructor properly initializeslfsFetchbased onoptions.smudgeLfs. The fingerprint is correctly generated viaoptions.makeFingerprint(rev).
1318-1350: LGTM! Consistent options propagation through accessor creation.Both
getRawAccessorandgetAccessoroverloads correctly propagateGitAccessorOptionsthrough the accessor creation chain. The export-ignore wrapping logic remains intact, now driven byoptions.exportIgnore.
1357-1389: LGTM! Correct use of designated initializers for options.The
getAccessorcall at line 1362 uses{.exportIgnore = exportIgnore}to explicitly set only the needed option, whilegetRawAccessorat line 1379 correctly uses{}since submodule revision lookup doesn't require special options.
702-716: LGTM!Using default
GitAccessorOptions{}for NAR hash computation is correct—the hash should be computed over the raw tree without export-ignore filtering or LFS smudging.
552-561: LGTM! Clear API documentation and consistent signatures.The updated declarations properly reflect the migration to
GitAccessorOptions. The comment clarifying thatgetRawAccessorcreates an accessor "with no regard for export-ignore" is helpful documentation.
| std::optional<std::string> getFingerprint(ref<Store> store, const Input & input) const override | ||
| { | ||
| if (auto rev = input.getRev()) | ||
| return rev->gitRev(); | ||
| return "github:" + rev->gitRev(); | ||
| else | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
Fingerprint prefix "github:" is used for all GitArchiveInputScheme subclasses.
GitArchiveInputScheme::getFingerprint is inherited by GitHubInputScheme, GitLabInputScheme, and SourceHutInputScheme. Using "github:" as the prefix for GitLab and SourceHut inputs is misleading and could cause fingerprint collisions if different forges return the same commit hash.
Consider using schemeName() to generate a scheme-specific prefix:
std::optional<std::string> getFingerprint(ref<Store> store, const Input & input) const override
{
if (auto rev = input.getRev())
- return "github:" + rev->gitRev();
+ return std::string{schemeName()} + ":" + rev->gitRev();
else
return std::nullopt;
}📝 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.
| std::optional<std::string> getFingerprint(ref<Store> store, const Input & input) const override | |
| { | |
| if (auto rev = input.getRev()) | |
| return rev->gitRev(); | |
| return "github:" + rev->gitRev(); | |
| else | |
| return std::nullopt; | |
| } | |
| std::optional<std::string> getFingerprint(ref<Store> store, const Input & input) const override | |
| { | |
| if (auto rev = input.getRev()) | |
| return std::string{schemeName()} + ":" + rev->gitRev(); | |
| else | |
| return std::nullopt; | |
| } |
🤖 Prompt for AI Agents
In src/libfetchers/github.cc around lines 350 to 356, the fingerprint prefix is
hardcoded as "github:" which is used by all GitArchiveInputScheme subclasses;
change the code to build the prefix from the scheme name (e.g. use schemeName()
or the instance's scheme getter) so the returned fingerprint is schemeName() +
":" + rev->gitRev() when rev is present, otherwise return std::nullopt, ensuring
unique, scheme-specific fingerprints and avoiding collisions across
GitHub/GitLab/SourceHut.
Motivation
Alternative to #275 using
git archive.Before Nix 2.20, we used the
gitCLI, which applies Git filters (in particular doing end-of-line conversion based on .gitattributes) ,export-ignoreandexport-subst. In 2.20, we switched tolibgit2and stopped doing those things, which is probably better for reproducibility. However, this breaks existing lock files /fetchTreecalls for Git inputs that rely on those features, since it invalidates the NAR hash.So as a backward compatibility hack, we now check the NAR hash computed over the Git tree without filters etc applied. If there is a hash mismatch, we try again with them (using
git archive). If that succeeds, we print a warning and return the latter tree.Context
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.