Git fetcher: Don't compute revCount/lastModified if they're already specified

[edolstra]

Motivation

We don't care if the user (or more likely the lock file) specifies an incorrect value for these attributes, since it doesn't matter for security (unlikely content hashes like narHash).

Depends on #268.

Context

Summary by CodeRabbit

• Bug Fixes

  • Relaxed strict matching for lastModified and revCount so they no longer trigger errors during non-final fetch operations.

• Refactor

  • Threaded centralized settings through Git fetch operations for consistent configuration handling.

• Tests

  • Removed the revCount-mismatch failure case.
  • Added a wait for port 80 in an Nix test to ensure the HTTP endpoint is ready before assertions.

[coderabbitai]

Walkthrough

Git fetcher methods were refactored to accept and thread a Settings parameter through public and internal fetch flows. Input::checkLocks no longer checks lastModified and revCount. Tests were adjusted to remove the revCount-failure assertion and a Nix test now waits for port 80 to be open.

Changes

Cohort / File(s)	Summary
Fetcher core relax locks src/libfetchers/fetchers.cc	Removed lastModified and revCount mismatch checks from Input::checkLocks, making lock validation less strict when input is not final.
Git fetcher: Settings threading src/libfetchers/git.cc	Added const Settings & settings parameter to public/internal methods (clone, getAccessor, isLocked, getAccessorFromCommit, getAccessorFromWorkdir) and replaced usages of input.settings with the threaded settings, including submodule handling, lastModified/revCount computations, and dirty checks.
Tests — flake metadata tests/nixos/tarball-flakes.nix	Removed assertion that expected failure for an incorrect revCount attribute in flake metadata fetch tests.
Tests — nginx readiness tests/nixos/content-encoding.nix	Added machine.wait_for_open_port(80) after starting nginx to ensure HTTP endpoint readiness before HTTP requests/assertions.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant GitScheme as GitInputScheme
    participant Repo as RepoInfo
    participant Store
    participant Accessor as SourceAccessor

    Caller->>GitScheme: getAccessor(settings, store, input)
    alt commit-based
        GitScheme->>GitScheme: getAccessorFromCommit(settings, store, repo, input)
        GitScheme->>Accessor: create/access accessor (commit)
        Note right of Accessor `#DDEBF7`: Uses provided settings\nfor revCount/lastModified/submodules
    else workdir-based
        GitScheme->>GitScheme: getAccessorFromWorkdir(settings, store, repo, input)
        GitScheme->>Accessor: create/access accessor (workdir)
        Note right of Accessor `#FFF2CC`: Uses provided settings\nfor dirty checks/submodules
    end
    GitScheme->>Caller: (Accessor, adjusted Input)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Signature changes across fetcher API require checking all callers and interface implementations.
• Verify submodule handling paths and that settings replaces input.settings everywhere necessary.
• Confirm removal of lastModified/revCount checks does not regress locking semantics.
• Review updated tests: tarball-flakes.nix (removed assertion) and content-encoding.nix (added port wait).

Possibly related PRs

• Remove setting from Input #268: Directly related refactor that threads Settings through fetcher methods and touches the same files.

Suggested reviewers

• cole-h
• grahamc

Poem

🐰 I threaded Settings through each Gitly flow,
Hopped past old locks where strict checks used to grow,
Tests now wait at port eighty's door,
Submodules hum, the fetcher bounds once more,
A little rabbit cheers — hop, go! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 3.30% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Git fetcher: Don't compute revCount/lastModified if they're already specified' clearly and specifically summarizes the main change: skipping recomputation of revCount and lastModified attributes when already specified.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch skip-revCount

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8095523 and 5c9b85e.

📒 Files selected for processing (1)

• tests/nixos/content-encoding.nix (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 / manual

🔇 Additional comments (1)

tests/nixos/content-encoding.nix (1)

134-134: Good defensive practice to prevent race conditions.

Adding wait_for_open_port(80) ensures the nginx HTTP endpoint is fully ready before issuing curl requests, preventing potential test flakiness from service startup timing.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

🚀 Deployed on https://691cc2f4144390ff03b83707--determinate-nix.netlify.app

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/libfetchers/git.cc (1)

870-890: Add conditional checks before computing revCount and lastModified in getAccessorFromWorkdir to match getAccessorFromCommit.

The code lacks the pre-existence checks present in getAccessorFromCommit. At line 872, revCount is unconditionally assigned without checking if already present. At lines 887-890, lastModified is unconditionally assigned without any pre-existence check.

Both should follow the pattern from getAccessorFromCommit (lines 747-750):

• Check if (!input.attrs.contains("revCount")) before assigning revCount at line 872
• Check if (!input.attrs.contains("lastModified")) before assigning lastModified at lines 887-890

This aligns with the PR objective: "Don't compute revCount/lastModified if they're already specified."

🧹 Nitpick comments (6)

src/libfetchers/fetchers.cc (1)

201-226: Centralized finalization in fetchToStore is correct; consider minor perf tweak.

The new fetchToStore implementation:

• Gets an accessor and intermediate Input via getAccessorUnchecked(settings, store).
• Copies into the store, computes narHash, marks the result as final, and runs checkLocks(*this, result).

This ensures the returned Input is always final and that security-relevant attributes (notably narHash) are verified against user/lock expectations.

If you want to avoid the extra queryPathInfo round‑trip, you could optionally use fetchToStore2’s hash result instead:

-        auto storePath = nix::fetchToStore(settings, *store, SourcePath(accessor), FetchMode::Copy, result.getName());
-
-        auto narHash = store->queryPathInfo(storePath)->narHash;
+        auto [storePath, narHash] =
+            nix::fetchToStore2(settings, *store, SourcePath(accessor), FetchMode::Copy, result.getName());

src/libflake/lockfile.cc (1)

36-55: Lockfile lock-status checks now consistently parameterized by fetchSettings

Passing fetchSettings into lockedRef.input.isLocked(...) in the LockedNode constructor and into the isConsideredLocked helper in LockFile::isUnlocked keeps all lock/dirty-lock decisions tied to the same configuration source. The “unlocked but with NAR hash” handling remains as before, but is now explicitly framed against the settings object.

If lock interpretation accrues more edge cases, consider factoring the “is this acceptable given settings (incl. allow-dirty-locks)?” predicate into a shared helper to keep LockedNode and isUnlocked in sync, but that’s a minor maintainability tweak rather than a functional concern.

Also applies to: 244-275

src/libfetchers/input-cache.cc (1)

8-41: Settings threading in InputCache::getAccessor looks correct; consider clarifying resolvedInput semantics

The new settings parameter is plumbed consistently into originalInput.getAccessor, lookupInRegistries, and resolvedInput.getAccessor, so behavior with respect to fetch configuration and registry use is preserved.

A minor semantic nuance (pre-existing): when a cached entry is hit for an indirect input, resolvedInput remains the original indirect input rather than the registry-resolved one. If callers conceptually expect resolvedInput to always be the post-registry input, you might consider storing and returning that explicitly from the cache entry; if not, current behavior is fine but could be documented.

src/libfetchers/path.cc (1)

104-107: Path getAccessor uses Settings cache and avoids overwriting user-supplied lastModified

The updated getAccessor:

• Uses settings.getCache()->upsert(...) to seed the source-path→hash cache, which matches the new configuration-centric design and avoids relying on Input to carry Settings.
• Preserves a user- or lockfile-supplied lastModified by only setting it from the filesystem mtime when !input.getLastModified(). This aligns with the PR’s intent to not recompute soft metadata like lastModified when it’s already specified.

The isLocked behavior (only treating path inputs with a narHash as locked) remains unchanged and appropriate for this scheme.

Also applies to: 119-121, 149-157

src/libcmd/common-eval-args.cc (1)

31-38: Use a single Settings source for resolve/lazyFetch vs fetchToStore

Here you pass the global fetchSettings into FlakeRef::resolve / lazyFetch, but use state.fetchSettings for fetchToStore. If these ever diverge (e.g. different purity or registry options per EvalState), behaviour could subtly differ between resolution and the final fetch. If EvalState::fetchSettings is meant to be authoritative, consider threading that through the resolve/lazyFetch calls as well; otherwise, a brief comment about the intended invariant between the two would avoid future confusion.

Also applies to: 184-188

src/nix/flake.cc (1)

1445-1449: Unify fetchSettings usage in flake prefetch to avoid subtle divergence

CmdFlakePrefetch::run uses fetchSettings for originalRef.resolve(...) but getEvalState()->fetchSettings for lazyFetch(...) and fetchToStore(...). If those Settings instances ever differ (e.g. due to per‑EvalState tweaks), prefetch could resolve with one configuration and fetch with another.

You can avoid this by capturing a single Settings & (likely from the EvalState) and using it consistently:

 void run(ref<Store> store) override
 {
-        auto originalRef = getFlakeRef();
-        auto resolvedRef = originalRef.resolve(fetchSettings, store);
-        auto [accessor, lockedRef] = resolvedRef.lazyFetch(getEvalState()->fetchSettings, store);
-        auto storePath =
-            fetchToStore(getEvalState()->fetchSettings, *store, accessor, FetchMode::Copy, lockedRef.input.getName());
+        auto evalState = getEvalState();
+        auto & fs = evalState->fetchSettings;
+
+        auto originalRef = getFlakeRef();
+        auto resolvedRef = originalRef.resolve(fs, store);
+        auto [accessor, lockedRef] = resolvedRef.lazyFetch(fs, store);
+        auto storePath =
+            fetchToStore(fs, *store, accessor, FetchMode::Copy, lockedRef.input.getName());

This keeps resolution, lazy fetch, and final store import all under the same configuration and also avoids calling getEvalState() multiple times.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 717da15 and ff1b4ea.

📒 Files selected for processing (29)

• src/libcmd/common-eval-args.cc (3 hunks)
• src/libcmd/installables.cc (1 hunks)
• src/libcmd/repl.cc (1 hunks)
• src/libexpr/paths.cc (1 hunks)
• src/libexpr/primops/fetchMercurial.cc (1 hunks)
• src/libexpr/primops/fetchTree.cc (3 hunks)
• src/libfetchers-tests/git.cc (1 hunks)
• src/libfetchers/builtin.cc (1 hunks)
• src/libfetchers/fetchers.cc (9 hunks)
• src/libfetchers/git.cc (11 hunks)
• src/libfetchers/github.cc (21 hunks)
• src/libfetchers/include/nix/fetchers/fetchers.hh (6 hunks)
• src/libfetchers/include/nix/fetchers/input-cache.hh (2 hunks)
• src/libfetchers/include/nix/fetchers/registry.hh (2 hunks)
• src/libfetchers/indirect.cc (3 hunks)
• src/libfetchers/input-cache.cc (1 hunks)
• src/libfetchers/mercurial.cc (7 hunks)
• src/libfetchers/path.cc (5 hunks)
• src/libfetchers/registry.cc (3 hunks)
• src/libfetchers/tarball.cc (7 hunks)
• src/libflake/flake-primops.cc (1 hunks)
• src/libflake/flake.cc (4 hunks)
• src/libflake/flakeref.cc (2 hunks)
• src/libflake/include/nix/flake/flakeref.hh (1 hunks)
• src/libflake/lockfile.cc (2 hunks)
• src/nix/flake-prefetch-inputs.cc (1 hunks)
• src/nix/flake.cc (5 hunks)
• src/nix/profile.cc (2 hunks)
• src/nix/registry.cc (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (21)

src/nix/flake-prefetch-inputs.cc (1)

src/libflake/include/nix/flake/flakeref.hh (3)

• fetchSettings (67-70)
• fetchSettings (72-72)
• fetchSettings (75-75)

src/libfetchers/mercurial.cc (2)

src/libfetchers/github.cc (29)

• input (137-159)
• input (137-137)
• input (163-163)
• input (377-380)
• input (377-377)
• input (382-385)
• input (382-382)
• input (387-390)
• input (387-387)
• 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)
• store (350-356)
• store (350-350)
• _input (161-179)
• _input (161-161)

src/libfetchers/fetchers.cc (2)

• fetchToStore (202-226)
• fetchToStore (202-202)

src/libfetchers/include/nix/fetchers/fetchers.hh (2)

src/libfetchers/git.cc (29)

• settings (163-628)
• settings (165-192)
• settings (165-165)
• settings (223-241)
• settings (223-223)
• settings (285-305)
• settings (285-286)
• settings (406-415)
• settings (406-406)
• settings (542-560)
• settings (542-546)
• settings (562-583)
• settings (562-566)
• input (243-271)
• input (243-243)
• input (273-283)
• input (273-273)
• input (307-310)
• input (307-307)
• input (312-377)
• input (312-316)
• input (420-423)
• input (420-420)
• input (425-428)
• input (425-425)
• input (430-433)
• input (430-430)
• input (435-438)
• input (435-435)

src/nix/flake.cc (14)

• store (128-139)
• store (128-128)
• store (162-172)
• store (162-162)
• store (216-317)
• store (216-216)
• store (322-326)
• store (322-322)
• store (360-848)
• store (360-360)
• store (881-978)
• store (881-881)
• store (1048-1054)
• store (1048-1048)

src/libfetchers/input-cache.cc (3)

src/libfetchers/fetchers.cc (2)

• getAccessor (285-299)
• getAccessor (285-285)

src/libfetchers/include/nix/fetchers/input-cache.hh (2)

• settings (19-19)
• originalInput (28-28)

src/libfetchers/registry.cc (2)

• lookupInRegistries (163-211)
• lookupInRegistries (164-164)

src/libexpr/paths.cc (2)

src/libfetchers/path.cc (19)

• settings (12-36)
• settings (12-12)
• settings (59-66)
• settings (59-59)
• settings (104-107)
• settings (104-104)
• settings (119-159)
• settings (120-120)
• input (68-79)
• input (68-68)
• input (81-84)
• input (81-81)
• input (86-93)
• input (86-90)
• input (95-102)
• input (95-95)
• input (109-117)
• input (109-109)
• input (122-122)

src/libexpr/include/nix/expr/eval.hh (2)

• settings (833-833)
• input (588-593)

src/libfetchers/registry.cc (1)

src/libfetchers/include/nix/fetchers/registry.hh (2)

• from (46-46)
• getRegistries (59-59)

src/libfetchers/include/nix/fetchers/input-cache.hh (1)

src/nix/flake-prefetch-inputs.cc (2)

• store (26-70)
• store (26-26)

src/libflake/include/nix/flake/flakeref.hh (1)

src/nix/flake.cc (16)

• store (128-139)
• store (128-128)
• store (162-172)
• store (162-162)
• store (216-317)
• store (216-216)
• store (322-326)
• store (322-322)
• store (360-848)
• store (360-360)
• store (881-978)
• store (881-881)
• store (1048-1054)
• store (1048-1048)
• store (1085-1137)
• store (1085-1085)

src/libflake/flake.cc (1)

src/libexpr/include/nix/expr/eval.hh (1)

• input (588-593)

src/libfetchers/indirect.cc (2)

src/libfetchers/git.cc (7)

• input (243-271)
• input (243-243)
• input (273-283)
• input (273-273)
• input (307-310)
• input (307-307)
• input (312-377)

src/nix/flake-prefetch-inputs.cc (2)

• store (26-70)
• store (26-26)

src/libfetchers/path.cc (3)

src/libfetchers/github.cc (29)

• input (137-159)
• input (137-137)
• input (163-163)
• input (377-380)
• input (377-377)
• input (382-385)
• input (382-382)
• input (387-390)
• input (387-387)
• 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)
• store (350-356)
• store (350-350)
• _input (161-179)
• _input (161-161)

src/libfetchers/include/nix/fetchers/fetchers.hh (4)

• store (158-158)
• store (176-176)
• store (243-246)
• store (243-243)

src/libfetchers/tarball.cc (2)

• store (407-415)
• store (407-407)

src/nix/registry.cc (2)

src/libflake/include/nix/flake/flakeref.hh (3)

• fetchSettings (67-70)
• fetchSettings (72-72)
• fetchSettings (75-75)

src/nix/flake-prefetch-inputs.cc (2)

• store (26-70)
• store (26-26)

src/libexpr/primops/fetchTree.cc (2)

src/libfetchers/include/nix/fetchers/registry.hh (1)

• lookupInRegistries (74-74)

src/libfetchers/registry.cc (2)

• lookupInRegistries (163-211)
• lookupInRegistries (164-164)

src/nix/flake.cc (3)

src/libflake/include/nix/flake/flakeref.hh (3)

• fetchSettings (67-70)
• fetchSettings (72-72)
• fetchSettings (75-75)

src/libflake/lockfile.cc (2)

• getFlakeRef (16-34)
• getFlakeRef (17-17)

src/nix/flake-prefetch-inputs.cc (2)

• store (26-70)
• store (26-26)

src/libflake/flakeref.cc (3)

src/libflake/include/nix/flake/flakeref.hh (3)

• fetchSettings (67-70)
• fetchSettings (72-72)
• fetchSettings (75-75)

src/libfetchers/include/nix/fetchers/registry.hh (1)

• lookupInRegistries (74-74)

src/libfetchers/registry.cc (2)

• lookupInRegistries (163-211)
• lookupInRegistries (164-164)

src/libcmd/installables.cc (1)

src/libflake/include/nix/flake/flakeref.hh (3)

• fetchSettings (67-70)
• fetchSettings (72-72)
• fetchSettings (75-75)

src/libcmd/common-eval-args.cc (3)

src/libflake/include/nix/flake/flakeref.hh (3)

• fetchSettings (67-70)
• fetchSettings (72-72)
• fetchSettings (75-75)

src/libfetchers/include/nix/fetchers/registry.hh (2)

• overrideRegistry (61-61)
• from (46-46)

src/libfetchers/registry.cc (2)

• overrideRegistry (127-130)
• overrideRegistry (127-127)

src/libfetchers/git.cc (3)

src/libfetchers/github.cc (29)

• input (137-159)
• input (137-137)
• input (163-163)
• input (377-380)
• input (377-377)
• input (382-385)
• input (382-382)
• input (387-390)
• input (387-387)
• 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)
• store (350-356)
• store (350-350)
• _input (161-179)
• _input (161-161)

src/libfetchers/fetchers.cc (6)

• getLastModified (478-483)
• getLastModified (478-478)
• getRevCount (471-476)
• getRevCount (471-471)
• fromAttrs (73-115)
• fromAttrs (73-73)

src/libflake/flakeref.cc (2)

• fromAttrs (253-260)
• fromAttrs (253-253)

src/libfetchers/github.cc (3)

src/libfetchers/git.cc (29)

• input (243-271)
• input (243-243)
• input (273-283)
• input (273-273)
• input (307-310)
• input (307-307)
• input (312-377)
• input (312-316)
• input (420-423)
• input (420-420)
• input (425-428)
• input (425-425)
• input (430-433)
• input (430-430)
• input (435-438)
• input (435-435)
• settings (163-628)
• settings (165-192)
• settings (165-165)
• settings (223-241)
• settings (223-223)
• settings (285-305)
• settings (285-286)
• settings (406-415)
• settings (406-406)
• settings (542-560)
• settings (542-546)
• settings (562-583)
• settings (562-566)

src/libfetchers/include/nix/fetchers/fetchers.hh (8)

• settings (53-53)
• settings (55-55)
• settings (62-62)
• store (158-158)
• store (176-176)
• store (243-246)
• store (243-243)
• ref (144-144)

src/libfetchers/tarball.cc (9)

• url (235-243)
• url (235-235)
• url (245-245)
• url (335-341)
• url (335-335)
• url (372-379)
• url (372-372)
• downloadFile (15-105)
• downloadFile (15-20)

src/libfetchers/fetchers.cc (5)

src/libfetchers/git.cc (33)

• input (243-271)
• input (243-243)
• input (273-283)
• input (273-273)
• input (307-310)
• input (307-307)
• input (312-377)
• input (312-316)
• input (420-423)
• input (420-420)
• input (425-428)
• input (425-425)
• input (430-433)
• input (430-430)
• input (435-438)
• input (435-435)
• isLocked (949-953)
• isLocked (949-949)
• settings (163-628)
• settings (165-192)
• settings (165-165)
• settings (223-241)
• settings (223-223)
• settings (285-305)
• settings (285-286)
• settings (406-415)
• settings (406-406)
• settings (542-560)
• settings (542-546)
• settings (562-583)
• settings (562-566)
• getAccessor (895-916)
• getAccessor (896-896)

src/libfetchers/include/nix/fetchers/fetchers.hh (7)

• settings (53-53)
• settings (55-55)
• settings (62-62)
• store (158-158)
• store (176-176)
• store (243-246)
• store (243-243)

src/libfetchers/fetch-to-store.cc (2)

• fetchToStore (15-26)
• fetchToStore (15-23)

src/libfetchers/include/nix/fetchers/fetch-to-store.hh (1)

• fetchToStore (17-25)

src/libfetchers/input-cache.cc (2)

• getAccessor (8-41)
• getAccessor (8-9)

src/libfetchers/tarball.cc (2)

src/libfetchers/github.cc (29)

• input (137-159)
• input (137-137)
• input (163-163)
• input (377-380)
• input (377-377)
• input (382-385)
• input (382-382)
• input (387-390)
• input (387-387)
• 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)
• store (350-356)
• store (350-350)
• _input (161-179)
• _input (161-161)

src/libfetchers/fetchers.cc (4)

• fromURL (44-47)
• fromURL (44-44)
• fromURL (58-71)
• fromURL (58-58)

⏰ 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 (47)

src/libexpr/primops/fetchMercurial.cc (1)

82-97: Updated fetchToStore call correctly threads fetchSettings.

Passing state.fetchSettings into input.fetchToStore matches the new API and keeps the fetch consistent with how the Input was constructed; no further changes needed here.

src/libcmd/installables.cc (1)

172-193: overrideRegistry now correctly receives fetchSettings.

The updated overrideRegistry call uses the same fetchSettings instance as other flake/fetch operations in this command, keeping registry overrides aligned with the active fetch configuration.

src/libexpr/paths.cc (1)

100-106: Settings-aware isLocked usage is consistent with new API.

Switching to input.isLocked(fetchSettings) lets scheme-specific locking rules honor configuration (like tarball trust) while preserving the existing guard around computing narHash.

src/libfetchers-tests/git.cc (1)

189-201: Test updated correctly for getAccessor(settings, store).

Passing the same settings used in Input::fromAttrs into getAccessor is the expected adaptation to the new signature; the regression test semantics remain unchanged.

src/libfetchers/include/nix/fetchers/fetchers.hh (1)

83-177: Explicit Settings parameters on Input/InputScheme APIs look sound.

Requiring const Settings & on locking, fetching, accessor, and clone operations removes hidden configuration from Input and makes cache/policy decisions explicit at call sites. The updated signatures across Input and InputScheme are consistent and form a clear, coherent API surface.

Also applies to: 219-251

src/libfetchers/include/nix/fetchers/registry.hh (1)

59-75: Registry helpers now thread Settings consistently.

Adding const Settings & to overrideRegistry and lookupInRegistries aligns registry operations with the rest of the fetcher API and enables consistent, Settings-aware behavior.

src/libfetchers/fetchers.cc (6)

73-115: fromAttrs correctly drops implicit Settings from Input construction.

Using a default-constructed Input and filling only attrs (with optional scheme later) matches the new design where Settings are passed into operations instead of stored inside Input. The raw() fallback for unknown schemes still behaves as before, only relying on attribute-level helpers used by fixupInput.

---

163-166: Input::isLocked now properly defers to scheme with Settings.

Delegating to scheme->isLocked(settings, *this) enables scheme-specific locking rules that can depend on configuration (e.g., tarball trust). This matches the intent behind the new Settings-aware isLocked API.

---

285-299: Input::getAccessor now finalizes results with Settings-aware backing.

By calling getAccessorUnchecked(settings, store), then marking the result __final = true and running checkLocks(*this, result), getAccessor guarantees the returned Input is consistent with the original specification and any lock attributes, while using the configured Settings for fetch behavior.

---

316-385: getAccessorUnchecked correctly uses Settings for cache/substitution.

The updated implementation:

• Uses settings.getCache() to upsert the source‑path→hash mapping when a fingerprint is available.
• Computes a store path from narHash only for final inputs, reusing existing store content when possible.
• Falls back to scheme->getAccessor(settings, store, *this) and then optionally to substitution via ensurePath.

This is a clean separation between “pure accessor” logic and the higher‑level locking that getAccessor/fetchToStore add on top.

---

394-398: Input::clone now cleanly delegates with explicit Settings.

Requiring Settings in Input::clone and forwarding it to scheme->clone(settings, store, *this, destDir) keeps clone behavior consistent with other Settings‑aware operations and avoids any hidden globals.

---

510-523: InputScheme::clone uses Settings-aware accessor and tar restore correctly.

The default clone implementation now calls getAccessor(settings, store, input), then streams the tree to destDir. This ensures any scheme-specific logic (including trust policy and caching) is honored while keeping the generic clone path simple.

src/libfetchers/github.cc (4)

35-137: Settings-aware GitArchive machinery looks correct and matches intent.

Within GitArchiveInputScheme:

• Inputs are now default-constructed and filled via attrs, which is consistent with removing Settings from Input itself.
• Access token lookup and header construction (getAccessToken, makeHeadersWithAuthTokens) now take const Settings & and use settings.accessTokens explicitly.
• downloadArchive threads Settings into getRevFromRef, the HTTP download, and both the general cache and tarball cache; the cache keys remain strictly rev-based.
• getAccessor obtains a tarball accessor from settings.getTarballCache() and only computes narHash when !settings.trustTarballsFromGitForges, which avoids unnecessary hashing when tarballs are trusted.
• isLocked now requires a rev and either trusted-tarball configuration or a narHash, which lines up with the security rationale around content hashes vs. metadata like lastModified.

This all fits well with the broader Settings threading and the locking semantics described in the PR.

Also applies to: 181-205, 206-229, 236-239, 246-316, 318-339, 341-348

---

392-436: GitHub-specific overrides correctly propagate Settings.

For GitHubInputScheme:

• getRevFromRef now takes Settings, uses it to build auth headers, and calls downloadFile(store, settings, ...), keeping API rate-limiting/auth consistent with configuration.
• getDownloadUrl uses makeHeadersWithAuthTokens(settings, ...) so archive downloads honor access tokens.
• clone builds a git+https:// URL and uses Input::fromURL(settings, ...) followed by .clone(settings, store, destDir), ensuring both Git and GitHub paths share the same Settings-driven behavior.

No issues spotted in these adaptations.

---

465-520: GitLab overrides are correctly Settings-threaded.

The GitLab-specific getRevFromRef, getDownloadUrl, and clone functions now:

• Accept const Settings & settings.
• Use makeHeadersWithAuthTokens(settings, ...) and downloadFile(store, settings, ...).
• Create Git URLs via Input::fromURL(settings, ...) before delegating to clone(settings, store, destDir).

This keeps GitLab behavior consistent with GitHub/SourceHut and the rest of the fetcher stack.

---

540-612: SourceHut overrides also correctly use Settings.

For SourceHutInputScheme:

• getRevFromRef and the two downloadFile call sites now take and forward Settings, including into header construction.
• getDownloadUrl and clone similarly honor Settings when building headers and when creating git+https:// URLs via Input::fromURL(settings, ...).

These changes complete the Settings-aware treatment across all GitArchive-based schemes.

src/libflake/flake-primops.cc (1)

11-32: Consistent use of fetch settings in pure-eval lock guard

Passing state.fetchSettings into flakeRef.input.isLocked(...) lines up with the parseFlakeRef(state.fetchSettings, …) call and ensures the pure-eval lock guard uses the same fetch configuration as parsing and locking. No issues spotted here.

src/nix/registry.cc (1)

185-201: Pin path now correctly threads fetchSettings through resolve and accessor

The CmdRegistryPin::run flow now consistently passes fetchSettings into lockedRef.resolve(...), resolvedInput.getAccessor(...), and resolved.isLocked(...). That matches the updated APIs and ensures registry pinning respects the same fetch configuration as the rest of the flake stack. Behavior looks correct.

src/nix/flake-prefetch-inputs.cc (1)

46-53: Accessor retrieval now aligned with settings-aware API

Switching to lockedRef.input.getAccessor(fetchSettings, store) is consistent with the new accessor signature and with the use of fetchSettings in the subsequent fetchToStore call. The prefetch path should now honor the same fetch configuration (tokens, cache, etc.) as other flake fetch paths.

src/libfetchers/builtin.cc (1)

31-52: Consistent Settings usage in builtin fetch-tree

builtinFetchTree now uses myFetchSettings both to construct the Input and to call getAccessor(...). That keeps this builtin self-contained and ensures any fetch-related configuration (e.g., access tokens) is applied consistently in the detached tmp store context. Looks good.

src/libcmd/repl.cc (1)

741-758: Pure-eval load-flake guard now keyed off fetchSettings

The :load-flake path now checks flakeRef.input.isLocked(fetchSettings) under evalSettings.pureEval, matching how the reference is parsed and bringing it in line with other flake entry points. The updated error message is also clearer for users. No issues here.

src/libexpr/primops/fetchTree.cc (1)

85-88: fetchTree now consistently threads state.fetchSettings through fetch and cache paths

Within fetchTree:

• Initial fetchers::Input input{} is safely overwritten by Input::fromAttrs(state.fetchSettings, …) or Input::fromURL(state.fetchSettings, …) before any meaningful use.
• Registry resolution, pure-eval lock checking, and the inputCache->getAccessor(...) call all now take state.fetchSettings, so the same fetch configuration drives registry lookups, lock classification, and cached accessor resolution.

This matches the updated fetcher API surface and aligns this primop with the Settings-driven behavior elsewhere. The narHash/pure-eval warning path remains intact. No functional issues spotted.

Also applies to: 190-205, 216-221

src/libfetchers/indirect.cc (1)

47-56: Indirect input scheme correctly adapted to the new Settings-aware API

For indirect inputs:

• Default-constructing Input and then filling attrs in inputFromURL/inputFromAttrs is sufficient now that per-input Settings state lives outside Input.
• The updated getAccessor(const Settings &, ref<Store>, const Input &) override matches the new InputScheme contract and still correctly rejects direct fetching of indirect inputs.

These adjustments look consistent and safe.

Also applies to: 73-82, 109-113

src/libflake/include/nix/flake/flakeref.hh (1)

67-70: FlakeRef resolve/lazyFetch API now consistently threads Settings

Changing resolve and lazyFetch to take const fetchers::Settings & matches the implementations and other call sites, and keeps registry resolution and fetching under the right configuration context. Looks good.

Also applies to: 74-75

src/nix/profile.cc (1)

714-719: isLocked now uses the same fetchSettings context as evaluation/fetching

Passing getEvalState()->fetchSettings into input.isLocked(...) aligns the lock-status checks with the rest of the flake evaluation/fetching configuration in this command. The guard conditions around upgrades remain unchanged in spirit and the threading of settings looks correct.

Also applies to: 743-745

src/libflake/flakeref.cc (1)

38-43: resolve/lazyFetch implementations correctly forward Settings to registry and accessor APIs

resolve now invokes lookupInRegistries(fetchSettings, store, input, useRegistries), and lazyFetch uses input.getAccessor(fetchSettings, store). This matches the updated registry and fetcher APIs and keeps flake resolution and lazy fetching under the same Settings as the rest of the system. No issues spotted.

Also applies to: 262-267

src/libfetchers/include/nix/fetchers/input-cache.hh (1)

6-7: InputCache interface updated cleanly to accept Settings

Forward-declaring struct Settings and changing getAccessor to take const Settings & align the header with the implementation and the rest of the fetcher APIs. The const-ref parameter keeps the cache API lightweight while allowing configuration-dependent behavior. Looks good.

Also applies to: 18-20

src/libfetchers/path.cc (1)

20-36: Path Input construction now independent of Settings pointer

Switching to Input input{} in inputFromURL and inputFromAttrs is consistent with the broader change that moves configuration into an explicit Settings parameter rather than storing it inside Input. The attributes are still fully initialized from the URL/attrset, so behavior should be unchanged.

Also applies to: 59-66

src/libcmd/common-eval-args.cc (1)

125-131: Settings-aware overrideRegistry wiring looks correct

override-flake now routes through fetchers::overrideRegistry(fetchSettings, from.input, to.input, extraAttrs), matching the updated registry API and reusing the same fetchSettings instance used for parsing the refs. No behavioural regressions apparent here.

src/libflake/flake.cc (2)

337-359: Consistent use of EvalState’s fetchSettings for accessors and lock checks

getFlake and the lockfile update path now consistently pass state.fetchSettings into inputCache->getAccessor and input.ref->input.isLocked(...). This aligns registry use and “pure mode” / lockability checks with the evaluation’s fetch configuration, and the control-flow around refetching on self-attrs remains unchanged.

Also applies to: 674-676, 747-760

---

972-995: Fingerprint now incorporates revCount/lastModified without forcing recomputation

LockedFlake::getFingerprint now (a) uses the Settings-aware lockFile.isUnlocked(fetchSettings) gate and (b) appends revCount and lastModified only when present on lockedRef.input. That matches the comment about these influencing evaluation (especially for tarballs) while avoiding extra work when the attributes are absent.

src/libfetchers/mercurial.cc (1)

92-95: Mercurial fetcher correctly adopts Settings‑based API

The Mercurial scheme now fully threads const Settings & settings through inputFromAttrs, fetchToStore, getAccessor, and isLocked, and all uses of configuration (dirty-tree checks and cache lookups) go through this parameter (settings.allowDirty, settings.warnDirty, settings.getCache()). The change from Input input{settings}; to Input input{} matches the broader move away from embedding Settings in Input. The caching and rev/ref handling logic remains the same.

Also applies to: 157-323, 325-343

src/libfetchers/registry.cc (2)

127-130: overrideRegistry now correctly forwards Settings to the flag registry

The new overrideRegistry(const Settings &, ...) simply forwards the provided settings into getFlagRegistry(settings) and then calls add. This preserves the previous behaviour while aligning with the Settings-aware registry lifecycle.

---

163-181: Settings-aware lookupInRegistries preserves existing resolution semantics

lookupInRegistries now takes an explicit Settings & and uses it to obtain the registry list via getRegistries(settings, store). The resolution loop and UseRegistries::No/Limited/All handling are otherwise unchanged, so behaviour should be the same but no longer relies on implicit global configuration.

src/nix/flake.cc (2)

258-259: Metadata output now respects Settings-aware lock status and fingerprinting

In CmdFlakeMetadata, gating the “Locked URL” on flake.lockedRef.input.isLocked(fetchSettings) and calling lockedFlake.getFingerprint(store, fetchSettings) ensures both are computed only when the lockfile is fully locked under the current fetch configuration. This matches the new Settings-aware lock semantics.

Also applies to: 274-276

---

1053-1054: Clone and archive correctly use the new Settings‑threaded APIs

• CmdFlakeClone::run now calls getFlakeRef().resolve(fetchSettings, store).input.clone(fetchSettings, store, destDir), matching the updated signatures.
• CmdFlakeArchive::run uses Input::fetchToStore(fetchSettings, store) at the top level and for each non-relative child input, extracting the StorePath via std::get<StorePath> from the tuple return type.

These are straightforward adaptations to the new fetch interface; control flow and dry‑run behaviour are preserved.

Also applies to: 1091-1093, 1104-1108

src/libfetchers/tarball.cc (5)

217-228: LGTM! Settings properly threaded through.

The explicit passing of settings to getAccessor is consistent with the new API design.

---

255-255: LGTM! Input initialization updated correctly.

The removal of the settings parameter from Input construction aligns with the PR's goal of explicit Settings threading rather than storing settings per-input.

Also applies to: 305-305

---

322-325: LGTM! Signature updated consistently.

The isLocked method signature now accepts Settings as the first parameter, maintaining API consistency across all input schemes.

---

343-362: LGTM! FileInputScheme properly updated.

The getAccessor signature and its call to downloadFile correctly thread the settings parameter through.

---

381-405: LGTM! TarballInputScheme properly updated.

The signature changes and Settings threading look correct. Note that line 402 passes settings twice: once to obtain the cache via settings.getTarballCache() and again to treeHashToNarHash(settings, ...). This appears intentional if the cache method needs settings for its computation logic.

src/libfetchers/git.cc (6)

234-240: LGTM! Input initialization updated correctly.

Consistent with the Settings threading pattern - Input no longer takes settings in its constructor.

---

285-305: LGTM! Clone signature updated consistently.

The clone method now accepts Settings as the first parameter, maintaining the uniform API pattern.

---

741-751: LGTM! Core PR objective implemented correctly.

The code now skips computing lastModified and revCount when these attributes are already present, which is the main goal of this PR. The rationale is sound: unlike narHash, incorrect values for these attributes don't compromise security, so avoiding expensive recomputation when values are supplied (e.g., from lock files) is a valid optimization.

---

794-795: LGTM! Submodule handling updated correctly.

Submodule inputs are properly created and accessed with the explicit settings parameter.

---

895-916: LGTM! Main getAccessor method updated correctly.

The signature includes the Settings parameter, and both code paths (commit vs workdir) correctly pass settings through to their respective helper methods.

---

949-953: LGTM! isLocked signature updated consistently.

The method now accepts Settings as the first parameter, maintaining API uniformity across all input schemes.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/libfetchers/git.cc (1)

741-751: Skip-when-present logic matches the PR intent; consider using typed accessors

The new guards on Line 744 and Line 749 correctly avoid recomputing lastModified and revCount when those attributes are already present, which aligns with the stated goal of trusting lock/user-provided values and avoiding expensive graph walks.

One small behavioral change to be aware of: this now skips recomputation even if the attributes exist but are malformed (e.g., wrong type), whereas previously they would be overwritten with a computed integer. If you want to preserve the old “fix up bad values” behavior while still skipping work when valid values are present, you could instead gate on the typed helpers:

-        if (!input.attrs.contains("lastModified"))
+        if (!input.getLastModified())
             input.attrs.insert_or_assign("lastModified", getLastModified(settings, repoInfo, repoDir, rev));

-        if (!getShallowAttr(input)) {
-            /* Like lastModified, skip revCount if supplied by the caller. */
-            if (!input.attrs.contains("revCount"))
-                input.attrs.insert_or_assign("revCount", getRevCount(settings, repoInfo, repoDir, rev));
-        }
+        if (!getShallowAttr(input)) {
+            /* Like lastModified, skip revCount if a valid one is supplied by the caller. */
+            if (!input.getRevCount())
+                input.attrs.insert_or_assign("revCount", getRevCount(settings, repoInfo, repoDir, rev));
+        }

If you’re fine with the new “caller can provide arbitrary values, even if unparsable” semantics, the current implementation is good as-is.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ff1b4ea and 8095523.

📒 Files selected for processing (3)

• src/libfetchers/fetchers.cc (0 hunks)
• src/libfetchers/git.cc (1 hunks)
• tests/nixos/tarball-flakes.nix (0 hunks)

💤 Files with no reviewable changes (2)

• tests/nixos/tarball-flakes.nix
• src/libfetchers/fetchers.cc

🧰 Additional context used

🧬 Code graph analysis (1)

src/libfetchers/git.cc (2)

src/libfetchers/fetchers.cc (4)

• getLastModified (478-483)
• getLastModified (478-478)
• getRevCount (471-476)
• getRevCount (471-471)

src/libfetchers/git-utils.cc (9)

• rev (350-390)
• rev (350-350)
• rev (392-397)
• rev (392-392)
• rev (524-524)
• rev (555-555)
• rev (558-558)
• rev (628-699)
• rev (628-628)

⏰ 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). (3)

• GitHub Check: build_x86_64-linux / vm_tests_smoke
• GitHub Check: build_x86_64-linux / manual
• GitHub Check: build_aarch64-darwin / build