Skip to content

Remove setting from Input#268

Merged
cole-h merged 2 commits intomainfrom
fetchers-settings-arg
Nov 18, 2025
Merged

Remove setting from Input#268
cole-h merged 2 commits intomainfrom
fetchers-settings-arg

Conversation

@edolstra
Copy link
Collaborator

@edolstra edolstra commented Nov 17, 2025

Motivation

Upstream NixOS#14550.

This fixes a crash in Rust code that creates a bunch of temporary fetchers::Settings objects. These end up as dangling pointers in the Input objects in InputCache.

Context

Summary by CodeRabbit

  • Refactor
    • Fetch configuration is now passed explicitly across the system, making resolution, fetching, cloning and registry lookups use consistent fetch settings rather than implicit defaults.
  • Bug Fixes
    • Lock checks and registry overrides now respect the active fetch settings, reducing inconsistent behavior during evaluation, prefetching, profiling and registry operations.

This is more straightforward and not subject to undocumented memory
safety restrictions.
Also easier to test.

(cherry picked from commit 292bd39)
@coderabbitai
Copy link

coderabbitai bot commented Nov 17, 2025

Walkthrough

This PR threads explicit fetchSettings (aka Settings) through fetching and flake resolution APIs, changing many method signatures to accept const Settings & and updating call sites to pass fetchSettings into registry lookups, accessor retrievals, fetch-to-store, clone, and lock checks.

Changes

Cohort / File(s) Change Summary
Core fetchers API & headers
src/libfetchers/include/nix/fetchers/fetchers.hh, src/libfetchers/include/nix/fetchers/registry.hh, src/libfetchers/include/nix/fetchers/input-cache.hh
Added const Settings & parameter to public APIs: Input::isLocked, fetchToStore, getAccessor, getAccessorUnchecked, clone, InputScheme virtuals, overrideRegistry, lookupInRegistries, and InputCache::getAccessor.
Fetchers implementations & cache/registry
src/libfetchers/fetchers.cc, src/libfetchers/registry.cc, src/libfetchers/input-cache.cc
Threaded Settings through implementations and internal calls; replaced input.settings usage with explicit settings; updated cache and registry lookups to accept settings.
Input scheme implementations
src/libfetchers/git.cc, src/libfetchers/github.cc, src/libfetchers/mercurial.cc, src/libfetchers/tarball.cc, src/libfetchers/path.cc, src/libfetchers/indirect.cc, src/libfetchers/builtin.cc
Updated virtual overrides to new signatures taking Settings; changed some Input input{settings}Input input{}; replaced call sites to pass settings into accessor/fetch/clone/isLocked flows.
Flake APIs & callers
src/libflake/include/nix/flake/flakeref.hh, src/libflake/flakeref.cc, src/libflake/flake.cc, src/libflake/lockfile.cc, src/libflake/flake-primops.cc
FlakeRef::resolve and FlakeRef::lazyFetch now accept fetchSettings; updated call sites and locking checks to pass fetchSettings.
Command-line / CLI / eval callers
src/libcmd/common-eval-args.cc, src/libcmd/installables.cc, src/libcmd/repl.cc
Updated calls to resolve(), lazyFetch(), overrideRegistry(), and isLocked() to pass fetchSettings from eval/command state.
Evaluation & expression code
src/libexpr/paths.cc, src/libexpr/primops/fetchMercurial.cc, src/libexpr/primops/fetchTree.cc
Propagated state.fetchSettings into fetchToStore, getAccessor, isLocked, and registry lookups.
Nix tooling & commands
src/nix/flake.cc, src/nix/profile.cc, src/nix/registry.cc, src/nix/flake-prefetch-inputs.cc
Updated flake resolution, prefetch, clone, lock-checks and accessor retrievals to use fetchSettings from evaluation context.
Tests
src/libfetchers-tests/git.cc
Updated test call sites to pass Settings into getAccessor() and related accessors.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant FlakeRef
    participant Registry
    participant Input
    participant Scheme
    participant Store

    Caller->>FlakeRef: resolve(fetchSettings, store)
    FlakeRef->>Registry: lookupInRegistries(fetchSettings, store, input)
    Registry-->>FlakeRef: resolved input

    Caller->>FlakeRef: lazyFetch(fetchSettings, store)
    FlakeRef->>Input: getAccessor(fetchSettings, store)
    Input->>Scheme: getAccessor(fetchSettings, store, input)
    Scheme->>Store: fetch / open / clone (uses settings)
    Scheme-->>Input: (accessor, lockedInput)
    Input-->>FlakeRef: (accessor, resolvedRef)
    FlakeRef-->>Caller: (accessor, lockedRef)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Areas requiring extra attention:

  • Consistent placement and const/reference semantics of the new const Settings & parameter across virtuals and overrides.
  • All call sites updated to pass the correct fetchSettings source (e.g., state.fetchSettings, getEvalState()->fetchSettings).
  • Registry/cache interactions (lookupInRegistries, overrideRegistry, InputCache::getAccessor) to ensure settings are used consistently.
  • Flake resolution and lock checks in flakeref.cc / flake.cc to validate behavior under pureEval and error messages.
  • Places where Input input{settings} was removed—confirm no implicit initialization behavior was lost.

Suggested reviewers

  • grahamc
  • cole-h

Poem

🐇 Through stores and registries I bound,

settings in paw, no defaults found.
Threads explicit, tidy and neat,
fetchers hop with steadier beat.
A rabbit cheers: the code's complete 🌿

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 3.19% 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 'Remove setting from Input' directly summarizes the main change in this PR, which is removing the stored Settings from the Input struct and threading it through as an explicit parameter.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fetchers-settings-arg

📜 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 f5bdba4 and 22bf198.

📒 Files selected for processing (1)
  • src/libcmd/repl.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). (2)
  • GitHub Check: build_aarch64-darwin / build
  • GitHub Check: build_x86_64-linux / build
🔇 Additional comments (1)
src/libcmd/repl.cc (1)

742-742: LGTM! Settings parameter correctly threaded.

The addition of fetchSettings to isLocked() is correct and aligns with the PR's objective to explicitly thread settings through method calls rather than storing them in Input objects.


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

@github-actions
Copy link

github-actions bot commented Nov 17, 2025

@github-actions github-actions bot temporarily deployed to pull request November 17, 2025 19:27 Inactive
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
src/libfetchers/git.cc (1)

895-915: getAccessor/isLocked overrides correctly updated for Settings-based API

  • getAccessor(const Settings &, ref<Store>, const Input &) now dispatches to either getAccessorFromCommit or getAccessorFromWorkdir with the same settings object passed through, so callers like Input::getAccessor and InputCache can supply the appropriate context.
  • isLocked(const Settings &, const Input &) still just checks for a non‑null, non‑nullRev rev, ignoring Settings. That’s fine given that higher‑level code (e.g., lockfile handling) separately accounts for allowDirtyLocks.

Only minor nit is that settings is currently unused in isLocked, but that’s harmless and keeps the signature consistent with the base interface.

Also applies to: 948-952

src/libfetchers/mercurial.cc (2)

157-323: Mercurial fetch path now consistently uses Settings for cache and dirty-tree behavior

fetchToStore now takes const Settings & settings and uses it for:

  • Dirty working tree checks (allowDirty, warnDirty).
  • All cache interactions via settings.getCache().

The rest of the logic (local/remote handling, ref/rev resolution, cache keys, and store writes) is unchanged, so behavior should be preserved while avoiding dangling settings in Input. If you want, you could factor auto cache = settings.getCache(); to avoid repeated lookups, but that’s purely a micro-optimization.


340-343: Unused settings parameter in isLocked

isLocked now has a const Settings & settings parameter but doesn’t use it, returning bool(input.getRev()). That’s fine semantically, but to avoid potential -Wunused-parameter warnings you might consider omitting the parameter name (const Settings &) or marking it as unused.

src/libfetchers/tarball.cc (1)

343-362: FileInputScheme accessor updated to use Settings-aware downloadFile

FileInputScheme::getAccessor now takes const Settings & settings and calls downloadFile(store, settings, ...), then injects narHash from the resulting store path. This is consistent with the new downloadFile signature and keeps behavior intact. One minor nit: most other APIs take settings before store, while downloadFile and downloadTarball use store, settings; consider unifying parameter order for consistency if you touch this again.

📜 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 f5bdba4.

📒 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 (24)
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/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/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/libflake/lockfile.cc (2)
src/libflake/include/nix/flake/flakeref.hh (3)
  • fetchSettings (67-70)
  • fetchSettings (72-72)
  • fetchSettings (75-75)
src/libfetchers/include/nix/fetchers/fetchers.hh (8)
  • input (215-215)
  • input (217-217)
  • input (222-222)
  • input (224-228)
  • input (238-241)
  • input (238-238)
  • input (253-256)
  • input (253-253)
src/libflake/flake-primops.cc (1)
src/libcmd/repl.cc (1)
  • flakeRef (96-96)
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/libfetchers/input-cache.cc (3)
src/libfetchers/fetchers.cc (2)
  • getAccessor (299-313)
  • getAccessor (299-299)
src/libfetchers/include/nix/fetchers/registry.hh (1)
  • lookupInRegistries (74-74)
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/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/libcmd/repl.cc (1)
src/libflake/include/nix/flake/flakeref.hh (3)
  • fetchSettings (67-70)
  • fetchSettings (72-72)
  • fetchSettings (75-75)
src/libfetchers/mercurial.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/tarball.cc (2)
  • store (407-415)
  • store (407-407)
src/libfetchers/fetchers.cc (2)
  • fetchToStore (202-226)
  • fetchToStore (202-202)
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/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/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 (492-497)
  • getLastModified (492-492)
  • getRevCount (485-490)
  • getRevCount (485-485)
  • 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/libcmd/installables.cc (1)
src/libflake/include/nix/flake/flakeref.hh (3)
  • fetchSettings (67-70)
  • fetchSettings (72-72)
  • fetchSettings (75-75)
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/libfetchers/path.cc (1)
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 (3)
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/libfetchers/github.cc (5)
  • settings (35-111)
  • settings (36-36)
  • settings (127-135)
  • store (350-356)
  • store (350-350)
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/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/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)
src/libfetchers/fetchers.cc (3)
src/libfetchers/include/nix/fetchers/fetchers.hh (28)
  • input (215-215)
  • input (217-217)
  • input (222-222)
  • input (224-228)
  • input (238-241)
  • input (238-238)
  • input (253-256)
  • input (253-253)
  • settings (53-53)
  • settings (55-55)
  • settings (62-62)
  • settings (83-83)
  • settings (116-116)
  • settings (136-136)
  • settings (140-140)
  • settings (146-146)
  • settings (193-193)
  • settings (195-195)
  • settings (219-220)
  • settings (231-231)
  • settings (248-251)
  • settings (248-248)
  • settings (258-262)
  • settings (259-259)
  • 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/input-cache.cc (2)
  • getAccessor (8-41)
  • getAccessor (8-9)
src/libfetchers/include/nix/fetchers/registry.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/libfetchers/github.cc (5)
  • settings (35-111)
  • settings (36-36)
  • settings (127-135)
  • store (350-356)
  • store (350-350)
⏰ 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). (4)
  • GitHub Check: build_x86_64-linux / manual
  • GitHub Check: build_x86_64-linux / test
  • GitHub Check: build_x86_64-linux / vm_tests_smoke
  • GitHub Check: build_aarch64-darwin / build
🔇 Additional comments (62)
src/libexpr/primops/fetchMercurial.cc (1)

84-84: LGTM! Settings parameter correctly threaded through.

The addition of state.fetchSettings as the first parameter to fetchToStore correctly implements the refactoring to pass settings explicitly rather than storing them in Input objects. This aligns with the PR objective to fix the dangling pointer crash.

src/libexpr/paths.cc (1)

102-104: LGTM! Settings properly threaded through API call.

The change correctly passes fetchSettings to input.isLocked(), aligning with the PR's objective of explicitly threading settings rather than storing them in Input objects. This prevents the dangling pointer issue mentioned in the PR description.

Verification confirms all 11 call sites of Input::isLocked() across the codebase have been updated to pass the settings parameter consistently. The reviewed change in src/libexpr/paths.cc:103 follows the same pattern used throughout the codebase.

src/libfetchers/github.cc (7)

95-95: LGTM: Input construction correctly updated.

The change from parameterized construction to default construction removes the stored Settings reference from Input objects, directly addressing the dangling pointer issue described in the PR objectives.

Also applies to: 132-132


236-236: LGTM: Virtual method signatures correctly updated.

All virtual methods that depend on Settings now receive it as an explicit parameter, ensuring Settings are threaded through the call chain without storing references in Input objects.

Also applies to: 238-238, 246-246, 318-319, 341-341


246-316: LGTM: Settings properly threaded through downloadArchive.

All cache operations and downstream method calls correctly use the explicit Settings parameter instead of accessing stored settings from Input objects.


318-339: LGTM: getAccessor correctly refactored.

The method properly receives and uses the Settings parameter for cache access and configuration checks (trustTarballsFromGitForges).


392-436: LGTM: GitHub scheme implementation correctly updated.

All methods in the GitHub implementation properly receive and propagate the Settings parameter through authentication header generation, file downloads, and clone operations.


465-520: LGTM: GitLab scheme implementation correctly updated.

The GitLab implementation mirrors the GitHub changes, consistently threading Settings through all method calls and operations.


540-612: LGTM: SourceHut scheme implementation correctly updated.

All methods in the SourceHut implementation properly receive and propagate Settings, including multiple downloadFile calls for reference resolution.

src/libcmd/installables.cc (1)

171-194: overrideRegistry Settings argument is wired correctly

Passing fetchSettings as the first argument to overrideRegistry alongside Input::fromAttrs(fetchSettings, …) keeps this call consistent with the new signature and the rest of the Settings‑threading changes. No issues spotted here.

src/libfetchers/git.cc (3)

223-241: Input construction no longer depends on embedded Settings

Switching to Input input{} and assigning attrs directly is consistent with removing Settings from Input. fixGitURL normalization plus getShallowAttr / getSubmodulesAttr / getAllRefsAttr still validate attributes without needing Settings. This aligns with the PR’s goal and doesn’t introduce new behavior.


630-808: Settings threading through commit-based accessor and submodules looks consistent

  • getAccessorFromCommit now takes const Settings & and uses it for:
    • getLastModified(settings, repoInfo, repoDir, rev)
    • getRevCount(settings, repoInfo, repoDir, rev) when not shallow.
  • Submodules now go through Input::fromAttrs(settings, std::move(attrs)) and submoduleInput.getAccessor(settings, store), so they no longer rely on any per‑Input Settings pointer.

Given that getLastModified / getRevCount only use settings.getCache() and repo data, this is a straightforward and correct migration to explicit Settings. Submodule Inputs and accessors share the same Settings instance as the parent, which is what we want for caching and policy.

Also applies to: 760-792


811-892: Workdir accessor now obeys Settings for revCount/lastModified and dirty handling

  • getAccessorFromWorkdir uses getRevCount(settings, repoInfo, repoPath, rev) for revCount when not shallow, mirroring commit mode.
  • repoInfo.warnDirty(settings) now explicitly consults settings.allowDirty / settings.warnDirty instead of any implicit/global state.
  • lastModified for workdir inputs uses getLastModified(settings, repoInfo, repoPath, headRev) when a headRev exists, falling back to 0 otherwise, which is consistent with the existing “nullRev” handling.

The new Settings parameter is used everywhere it matters; it doesn’t change semantics beyond making configuration explicit.

src/libfetchers/fetchers.cc (4)

73-115: Raw Input construction now decoupled from Settings

Using Input input; in the “raw” branch of fromAttrs and then calling fixupInput(input) is compatible with the removal of Settings from Input. fixupInput only touches attributes via getters (getType, getRef, getRevCount, getLastModified), so there’s no lost behavior from not having Settings here. This is a clean, minimal change toward the new API.


163-167: isLocked now explicitly depends on Settings via the scheme

Input::isLocked(const Settings & settings) delegating to scheme->isLocked(settings, *this) is the right abstraction: it keeps the policy (including any Settings‑dependent rules) inside the scheme while avoiding embedding Settings in Input. For inputs without a scheme, the method correctly returns false.


201-226: Fetch/accessor pipeline correctly threaded with Settings

  • Input::fetchToStore(const Settings &, ref<Store>) now:
    • Uses getAccessorUnchecked(settings, store) for the initial fetch.
    • Calls nix::fetchToStore(settings, *store, SourcePath(accessor), …) so store fetching respects the same Settings (e.g., caches).
  • Input::getAccessor(const Settings &, ref<Store>) similarly uses getAccessorUnchecked(settings, store) and then applies finalization + checkLocks.
  • Input::getAccessorUnchecked(const Settings &, ref<Store>):
    • Still reuses an existing store path when the input is final and present in the store.
    • Uses settings.getCache()->upsert(…) to populate the source‑path → hash cache when serving from an existing store path.
    • Delegates to scheme->getAccessor(settings, store, *this) when it needs to fetch, propagating Settings into the scheme.

Overall, the fetch path behavior is preserved while eliminating any reliance on a Settings pointer stored inside Input.

Also applies to: 299-313, 330-399


408-412: Clone APIs updated coherently to accept Settings

  • Input::clone(const Settings &, ref<Store>, const std::filesystem::path &) now forwards to scheme->clone(settings, store, *this, destDir).
  • InputScheme::clone in turn obtains an accessor via getAccessor(settings, store, input) and copies it into destDir.

This is consistent with the other Settings‑threaded methods and ensures clones use the same caching and policy context as regular fetches.

Also applies to: 524-531

src/libflake/lockfile.cc (1)

36-59: Lockfile locking checks now correctly use fetchSettings

  • In LockedNode, the unlocked‑input check now uses lockedRef.input.isLocked(fetchSettings), and still exempts relative inputs. The warning/throw behavior based on getNarHash() is unchanged.
  • LockFile::isUnlocked’s isConsideredLocked helper now bases its decision on input.isLocked(fetchSettings) plus the allowDirtyLocks && narHash fallback.

This keeps lockfile validation aligned with the new Settings‑aware Input::isLocked semantics while preserving the existing allow-dirty-locks behavior.

Also applies to: 244-276

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

9-33: getFlake pure-eval lock check now uses Settings-aware isLocked

The guard state.settings.pureEval && !flakeRef.input.isLocked(state.fetchSettings) correctly switches to the new Settings‑aware isLocked API, matching the behavior used when reading lockfiles and resolving flakes. No functional regression expected here.

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

49-49: LGTM! Settings parameter correctly threaded through.

The update to pass fetchSettings as the first parameter to getAccessor is correct and aligns with the refactored API signature.

src/nix/profile.cc (2)

714-714: LGTM! Settings parameter correctly added to isLocked check.

The update correctly passes getEvalState()->fetchSettings to the isLocked call, making the fetch settings explicit rather than implicit.


743-744: LGTM! Settings parameter correctly added to isLocked check.

The update correctly passes getEvalState()->fetchSettings to the isLocked call and maintains the comparison logic with the locked reference.

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

199-199: LGTM! Test correctly updated for new API signature.

The test properly passes settings as the first parameter to getAccessor, matching the updated API signature.

src/libfetchers/registry.cc (2)

127-130: LGTM! API signature correctly updated to accept Settings.

The overrideRegistry function now takes settings as the first parameter and correctly propagates it to getFlagRegistry.


163-179: LGTM! API signature correctly updated and Settings properly threaded through.

The lookupInRegistries function signature now includes settings as the first parameter, and it's correctly passed to getRegistries for registry resolution.

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

85-85: LGTM! Input construction correctly updated.

The Input is now default-constructed without settings, consistent with the refactored design where settings are passed explicitly to operations rather than stored in the Input object.


191-191: LGTM! Settings correctly passed to registry lookup.

The lookupInRegistries call now explicitly receives state.fetchSettings, ensuring registry resolution uses the correct fetch configuration.


193-193: LGTM! Settings correctly passed to lock check.

The isLocked call now explicitly receives state.fetchSettings, making the dependency on fetch settings explicit rather than implicit.


216-217: LGTM! Settings correctly passed to cache accessor retrieval.

The getAccessor call on the input cache now explicitly receives state.fetchSettings, ensuring consistent fetch behavior.

src/libfetchers/builtin.cc (1)

51-51: LGTM! Settings correctly passed to getAccessor.

The call now explicitly passes myFetchSettings (configured earlier with appropriate access tokens) to getAccessor, aligning with the refactored API.

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

6-6: LGTM! Forward declaration added appropriately.

The forward declaration of Settings avoids unnecessary includes while supporting the updated API signature.


18-19: LGTM! API signature correctly updated.

The getAccessor method now takes settings as the first parameter, making fetch settings explicit throughout the input cache layer.

src/libfetchers/indirect.cc (3)

47-47: LGTM! Input construction correctly updated.

The Input is now default-constructed without settings, consistent with the refactored design.


79-79: LGTM! Input construction correctly updated.

The Input is now default-constructed without settings, consistent with the refactored design.


109-113: LGTM! API signature correctly updated.

The getAccessor signature now includes the settings parameter, even though the implementation appropriately throws an error (indirect inputs cannot be fetched directly).

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

67-70: LGTM: Signature update is correct.

The addition of the fetchSettings parameter to FlakeRef::resolve is consistent with the PR objective of threading explicit Settings through the API.


74-75: LGTM: Signature update is correct.

The addition of the fetchSettings parameter to FlakeRef::lazyFetch aligns with the broader pattern of explicit Settings propagation throughout the codebase.

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

8-41: LGTM: Settings parameter properly propagated.

The settings parameter is correctly threaded through all getAccessor and lookupInRegistries calls within the method. The changes align with the PR's goal of explicit Settings propagation.

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

61-61: LGTM: Registry function signatures correctly updated.

Both overrideRegistry and lookupInRegistries now accept settings as their first parameter, maintaining consistency with the broader API changes across the codebase.

Also applies to: 73-74

src/nix/registry.cc (1)

193-195: LGTM: fetchSettings properly propagated.

All method calls in the CmdRegistryPin::run implementation correctly pass fetchSettings as required by the updated API signatures.

src/libflake/flake.cc (3)

337-338: LGTM: Settings properly propagated through input cache.

Both getAccessor calls correctly pass state.fetchSettings as the first parameter, maintaining consistency with the updated API signature.

Also applies to: 354-355


674-675: LGTM: isLocked call updated correctly.

The isLocked check now receives state.fetchSettings, ensuring consistent Settings access throughout the locking logic.


747-748: LGTM: Non-flake input accessor retrieval updated.

The getAccessor call for non-flake inputs properly propagates state.fetchSettings through the cache layer.

src/libfetchers/path.cc (3)

20-20: LGTM: Input construction updated correctly.

The Input objects are now constructed without passing settings, which aligns with the PR objective of removing the settings member from Input. Settings are now passed explicitly to methods that need them.

Also applies to: 63-63


104-107: LGTM: Method signatures correctly updated.

Both isLocked and getAccessor now accept settings as a parameter, consistent with the broader API refactoring to thread Settings explicitly.

Also applies to: 119-120


149-151: LGTM: Cache access uses passed settings.

The cache is correctly accessed via settings.getCache() rather than input.settings, confirming that Settings are no longer stored in Input objects.

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

31-32: LGTM: FlakeRef operations updated correctly.

Both instances of resolve and lazyFetch calls now properly pass fetchSettings as required by the updated API signatures.

Also applies to: 185-185


130-130: LGTM: Registry override updated correctly.

The overrideRegistry call correctly passes fetchSettings as the first parameter, consistent with the updated function signature.

src/nix/flake.cc (4)

258-258: LGTM: Lock check updated correctly.

The isLocked check now receives fetchSettings, ensuring consistent Settings propagation in the metadata command.


1053-1053: LGTM: Clone operation updated correctly.

Both resolve and clone calls properly pass fetchSettings, maintaining the explicit Settings threading pattern.


1092-1092: LGTM: fetchToStore calls updated correctly.

All fetchToStore operations in the archive command now receive fetchSettings as required by the updated API signature.

Also applies to: 1104-1107


1445-1446: LGTM: Prefetch operations updated correctly.

Both resolve and lazyFetch calls properly propagate fetchSettings, with the latter correctly using getEvalState()->fetchSettings to access the Settings from the evaluation state.

src/libflake/flakeref.cc (2)

38-43: Resolve now threads fetchSettings correctly through registry lookup

FlakeRef::resolve now takes const fetchers::Settings & and forwards it into lookupInRegistries, returning a new FlakeRef with dir from extraAttrs or the existing subdir. This keeps previous behavior while removing any implicit coupling to per-Input settings.


262-267: lazyFetch updated to use Settings-aware Input::getAccessor

FlakeRef::lazyFetch now calls input.getAccessor(fetchSettings, store) and wraps the locked input back into a FlakeRef reusing subdir. This matches the new Input API and avoids storing settings on Input.

src/libfetchers/mercurial.cc (2)

83-95: Input construction decoupled from Settings storage

inputFromAttrs now uses Input input{}; and assigns attrs, which removes any hidden settings state from Input while preserving behavior. This aligns with the new model where settings are always passed explicitly.


325-338: getAccessor signature and implementation correctly adapted to Settings-aware fetch

MercurialInputScheme::getAccessor now accepts const Settings & settings and passes it into fetchToStore(settings, store, input), then builds an accessor from the resulting store path. This matches the new virtual interface and ensures cache and dirty-tree decisions respect the caller’s Settings.

src/libfetchers/tarball.cc (4)

217-228: downloadTarball now uses Settings-aware accessors

downloadTarball now goes through input.getAccessor(settings, store) rather than a settings-less accessor. This aligns with the new Input API and guarantees that tarball fetching respects the provided Settings (caches, trust flags, etc.).


255-287: Curl-based schemes now construct Input without embedding Settings

Both inputFromURL and inputFromAttrs in CurlInputScheme now use Input input{}; and fill attrs. This removes any coupling between Input and a particular Settings instance while preserving attribute semantics.

Also applies to: 303-310


322-325: Tarball scheme’s isLocked correctly adopts new signature

CurlInputScheme::isLocked now takes const Settings & but still bases lockedness solely on input.getNarHash(). That matches the prior behavior and the documented meaning of “locked” for tarball-like inputs.


381-405: TarballInputScheme accessor now fully Settings-aware

TarballInputScheme::getAccessor now:

  • Calls downloadTarball_(settings, ...).
  • Re-parses immutable URLs via Input::fromURL(settings, ...).
  • Uses settings.getTarballCache()->treeHashToNarHash(settings, result.treeHash) when populating narHash.

This threads Settings through all relevant paths (download, caching, hash computation) and removes any reliance on Input-stored settings. Behavior looks preserved and more robust.

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

83-147: Input API cleanly decoupled from embedded Settings

The Input methods:

  • isLocked(const Settings & settings) const
  • fetchToStore(const Settings & settings, ref<Store> store) const
  • getAccessor(const Settings & settings, ref<Store> store) const
  • getAccessorUnchecked(const Settings & settings, ref<Store> store) const
  • clone(const Settings & settings, ref<Store> store, const std::filesystem::path & destDir) const

now all take Settings explicitly. This matches the rest of the PR and removes the need for Input to hold onto a Settings pointer, which directly addresses the dangling-pointer crash described in the PR. The signatures are coherent and consistent with downstream usage shown in other files.


219-251: InputScheme virtual interface updated consistently to accept Settings

InputScheme::clone, getAccessor, and isLocked now all take const Settings & settings explicitly, matching the new Input methods and the overrides in concrete schemes (git, tarball, Mercurial, etc.). The default implementations still behave as before (no-op clone, false for isLocked), so existing semantics are preserved while enabling Settings-dependent schemes.

@cole-h cole-h added this pull request to the merge queue Nov 18, 2025
Merged via the queue into main with commit d7ff2fe Nov 18, 2025
28 checks passed
@cole-h cole-h deleted the fetchers-settings-arg branch November 18, 2025 17:13
This was referenced Jan 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants