Avoid unnecessary Git refetches #270

edolstra · 2025-11-18T16:01:24Z

Motivation

This fixes the issue where updating a Git input does a non-shallow fetch, and then a subsequent eval does a shallow refetch because the revCount is already known. Now the subsequent eval will use the repo used in the first fetch.

It also gets rid of a hack in fetchTree that sets shallow = false, which is bad because it's inconsistent with other fetches.

NixOS#14588.

Depends on #269.

Context

Summary by CodeRabbit

Changes
- Removed automatic injection of shallow cloning for git inputs; shallow behavior is now explicit and Settings-aware.
- Fetch logic now detects when a full (non-shallow) repository already contains the desired revision and reuses it.
Tests
- Added a functional test validating non-shallow fetch behavior and clone reuse.
- Updated test fixtures and test suite entries to reflect the new shallow/non-shallow behavior.

coderabbitai · 2025-11-18T16:01:53Z

Walkthrough

Removes automatic injection of shallow = true for certain git inputs, adds GitInputScheme::canDoShallow() and Settings-aware shallow decisioning, introduces reuse of existing non-shallow cached clones, updates rev/lastModified handling, and adds/updates functional tests for shallow behavior.

Changes

Cohort / File(s)	Summary
Git fetcher core `src/libfetchers/git.cc`, `src/libexpr/primops/fetchTree.cc`	Remove automatic `shallow = true` injection in fetchTree path; add `GitInputScheme::canDoShallow(const Input&)`; make shallow decisions Settings-aware; add code paths to prefer reusing non-shallow cached clones and adjust rev/lastModified handling and control flow in `getAccessorFromCommit`.
Functional tests (flakes) `tests/functional/flakes/common.sh`, `tests/functional/flakes/shallow.sh`, `tests/functional/flakes/meson.build`	Add `number = 123;` to generated `flake.nix`; add `shallow.sh` test script and register it in `meson.build` to exercise non-shallow fetch behavior and clone reuse.
NixOS test update `tests/nixos/fetch-git/test-cases/fetchTree-shallow/default.nix`	Update description text and add `revCount = 1234` to the `git` spec in the test case.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant Fetcher as GitInputScheme
    participant Cache
    Caller->>Fetcher: getAccessor(input, Settings)
    rect rgb(240,248,255)
    Note over Fetcher: determine shallow eligibility\nvia canDoShallow(input) (shallow attr or revCount)
    end
    alt eligible for shallow
        rect rgb(255,250,240)
        Note over Fetcher: attempt shallow path
        Fetcher->>Cache: lookup shallow cache
        alt shallow cache hit
            Cache-->>Fetcher: shallow accessor
        else
            Fetcher->>Fetcher: perform shallow clone
            Fetcher->>Cache: store shallow
        end
        end
    end
    rect rgb(240,255,245)
    Note over Fetcher: always consider non-shallow reuse\n(if existing full clone has rev)
    Fetcher->>Cache: lookup non-shallow cache
    alt non-shallow cache hit (has rev)
        Cache-->>Fetcher: reuse full accessor
        Fetcher-->>Caller: return accessor (no shallow)
    else
        Fetcher->>Fetcher: perform full clone
        Fetcher->>Cache: store full
        Fetcher-->>Caller: return accessor
    end
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Inspect src/libfetchers/git.cc for correctness of canDoShallow() conditions, Settings propagation, and the new non-shallow reuse branch.
Verify src/libexpr/primops/fetchTree.cc no longer auto-injects shallow and that input construction still matches fetcher expectations.
Review tests/functional/flakes/shallow.sh for robustness and correct assertions around rev counts and clone reuse.

Possibly related PRs

Git fetcher: Don't compute revCount/lastModified if they're already specified #269 — touches git fetcher flow, revCount/lastModified handling and Settings threading; closely related to these changes.
Sync with upstream 2.32.2 #242 — modifies GitInputScheme/git fetch handling and URL/input processing; relevant to canDoShallow and fetcher adjustments.

Suggested reviewers

cole-h

Poem

🐇 I hopped through git and checked each tree,

no shallow trick imposed on me.
I peek the cache, reuse with care,
keep revs intact, avoid thin air.
A tiny hop — fewer bytes to spare.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 4.26% 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 'Avoid unnecessary Git refetches' directly describes the main change: preventing redundant Git repository fetches by reusing already-fetched non-shallow clones when revCount is known.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch shallow-optimization

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

Provide your own instructions using the high_level_summary_instructions setting.
Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

📝 Description — Summarize the main change in 50–60 words, explaining what was done.

📓 References — List relevant issues, discussions, documentation, or related PRs.

📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.

📊 Contributor Summary — Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |

✔️ Additional Notes — Add any extra reviewer context.
Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

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

github-actions · 2025-11-18T16:06:27Z

🚀 Deployed on https://691cea205cf7f664d1972496--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/input-cache.cc (1)

8-41: Potential loss of registry extraAttrs when reusing a cached resolved input — escalated to major issue

The concern raised is valid and warrants escalation to major severity. Callers explicitly depend on registry-provided attributes being present in extraAttrs:

In src/libflake/flake.cc:340, the code extracts the "dir" attribute from cachedInput.extraAttrs with a fallback to originalRef.subdir, demonstrating that registry attributes are relied upon for correct behavior.
The new logic reuses a cached CachedInput when resolvedInput is already in the cache, but fails to merge new extraAttrs from the registry lookup into that reused entry.
This causes registry-provided directory overrides (commonly "dir") to be silently discarded when the same input is accessed both directly and indirectly.

The suggested fix is necessary: merge extraAttrs into the reused cache entry when !extraAttrs.empty():

-            if (useRegistries != UseRegistries::No) {
-                auto [res, extraAttrs] = lookupInRegistries(settings, store, originalInput, useRegistries);
-                resolvedInput = std::move(res);
-                fetched = lookup(resolvedInput);
-                if (!fetched) {
-                    auto [accessor, lockedInput] = resolvedInput.getAccessor(settings, store);
-                    fetched.emplace(
-                        CachedInput{.lockedInput = lockedInput, .accessor = accessor, .extraAttrs = extraAttrs});
-                }
-                upsert(resolvedInput, *fetched);
+            if (useRegistries != UseRegistries::No) {
+                auto [res, extraAttrs] = lookupInRegistries(settings, store, originalInput, useRegistries);
+                resolvedInput = std::move(res);
+                fetched = lookup(resolvedInput);
+                if (!fetched) {
+                    auto [accessor, lockedInput] = resolvedInput.getAccessor(settings, store);
+                    fetched.emplace(
+                        CachedInput{.lockedInput = lockedInput, .accessor = accessor, .extraAttrs = extraAttrs});
+                } else if (!extraAttrs.empty()) {
+                    fetched->extraAttrs.insert(extraAttrs.begin(), extraAttrs.end());
+                }
+                upsert(resolvedInput, *fetched);

🧹 Nitpick comments (5)

src/nix/profile.cc (1)

714-719: Using EvalState fetch settings for lock checks is consistent with the new API

Switching the isLocked checks to getEvalState()->fetchSettings for both the original and locked flake refs keeps lock evaluation tied to the current evaluation settings instead of stale state inside Input, which aligns with the fetch-settings threading in this PR. If you touch this again, you could optionally cache auto & fs = getEvalState()->fetchSettings; locally to avoid repeating the call, but that’s purely cosmetic.

Also applies to: 743-745

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

61-62: Registry APIs now correctly thread Settings; doc comment is slightly stale

The new overrideRegistry and lookupInRegistries signatures taking const Settings & look consistent with the rest of the fetch-settings refactor and make the dependency explicit. The comment above lookupInRegistries still talks about a filter callback, which no longer matches the signature (and likely predates this change); consider updating it when convenient to describe UseRegistries instead.

Also applies to: 69-75
src/libfetchers/path.cc (1)
20-36: PathInputScheme’s explicit Settings usage looks good; a couple of minor polish points

Swapping to Input input{} in inputFromURL / inputFromAttrs and relying on the explicit Settings & parameters elsewhere is consistent with removing Settings from Input and keeps construction simple.

The new isLocked(const Settings & settings, const Input & input) override doesn’t use settings; if you compile with -Wunused-parameter, you may want to drop the parameter name (const Settings &) or mark it [[maybe_unused]].

In getAccessor, moving from input.settings->getCache() to settings.getCache() matches the new ownership model. While you’re here, you could avoid the extra queryPathInfo by reusing info when computing accessor->fingerprint:
-        auto info = store->queryPathInfo(*storePath);
-        accessor->fingerprint =
-            fmt("path:%s", store->queryPathInfo(*storePath)->narHash.to_string(HashFormat::SRI, true));
+        auto info = store->queryPathInfo(*storePath);
+        accessor->fingerprint =
+            fmt("path:%s", info->narHash.to_string(HashFormat::SRI, true));
Also applies to: 59-66, 104-107, 119-159
src/libfetchers/registry.cc (1)

127-130: Settings-threading in registry override and lookup is consistent

overrideRegistry and lookupInRegistries now both take const Settings & and delegate to getFlagRegistry(settings) / getRegistries(settings, store), keeping registry resolution semantics intact while avoiding implicit settings on Input. The cycle detection and error paths are unchanged.

Also applies to: 163-211

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

83-84: Public fetcher API now threads Settings explicitly—ensure downstream users are updated

The header changes (Settings-aware Input::isLocked/fetchToStore/getAccessor/getAccessorUnchecked/clone and InputScheme::clone/getAccessor/isLocked) are consistent with the corresponding implementations and the design of this PR.

However, this is a breaking change for any out-of-tree code implementing InputScheme or calling these methods directly. It would be good to:

Call this out in release/upgrade notes.

Verify that any plugins or external fetcher schemes in your ecosystem are updated to the new signatures.

Also applies to: 116-117, 136-137, 140-141, 146-147, 219-221, 230-232, 248-251

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

📒 Files selected for processing (30)

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 (14 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)
tests/nixos/tarball-flakes.nix (0 hunks)

💤 Files with no reviewable changes (1)

tests/nixos/tarball-flakes.nix

🧰 Additional context used

🧬 Code graph analysis (21)

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/registry.cc (1)

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

from (46-46)

getRegistries (59-59)

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/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/nix/registry.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/include/nix/fetchers/fetchers.hh (1)

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/libexpr/primops/fetchTree.cc (3)

src/libfetchers/git.cc (16)

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/include/nix/fetchers/registry.hh (1)

lookupInRegistries (74-74)

src/libfetchers/registry.cc (2)

lookupInRegistries (163-211)

lookupInRegistries (164-164)

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/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/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/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 (4)

fromURL (44-47)

fromURL (44-44)

fromURL (58-71)

fromURL (58-58)

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/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/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/installables.cc (1)

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

fetchSettings (67-70)

fetchSettings (72-72)

fetchSettings (75-75)

src/libfetchers/git.cc (4)

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/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 (628-699)

rev (628-628)

repo (277-280)

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/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/fetchers.cc (5)

src/libfetchers/git.cc (31)

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)

getAccessor (920-941)

getAccessor (921-921)

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/libflake/flake.cc (1)

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

input (588-593)

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 (42)

src/libcmd/repl.cc (1)

742-743: No issues found. Changes are correct.

The fetchSettings is properly accessible as a global extern variable declared in common-eval-args.hh, consistent with its usage throughout the codebase. The logic correctly checks for unlocked flake references in pure eval mode, and the error message is clear and actionable.

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

6-7: Settings forward-declaration and getAccessor signature look consistent

Forward-declaring fetchers::Settings and threading it explicitly into InputCache::getAccessor matches the broader API changes (Settings as first param) and keeps this header independent of the concrete Settings definition while only using it by reference. No issues from a type or ABI perspective.

Also applies to: 18-19

src/libcmd/installables.cc (1)

171-194: overrideRegistry now uses fetchSettings consistently

Passing fetchSettings into overrideRegistry aligns this call with the surrounding uses of fetchSettings (parseFlakeRef, Input::fromAttrs) and with the updated registry API, so the indirect-input registry entries will honor the same fetch configuration. Looks correct.

src/libexpr/paths.cc (1)

102-105: Lock check correctly parameterized with fetchSettings

Using input.isLocked(fetchSettings) in the narHash condition makes the lockability decision honor the current fetch settings (e.g. whether tarball-based inputs are trusted) instead of any implicit/default behavior. Given isLocked is implemented as a pure metadata check, this should not introduce unintended fetches and keeps mount-time narHash computation consistent with the new locking semantics.

src/libfetchers/github.cc (4)

36-41: Input construction decoupled from Settings and signatures updated cleanly

GitArchiveInputScheme::inputFromURL / inputFromAttrs now accept const Settings & but construct Input input{} without embedding settings, and then fill attrs. This matches the refactored Input API (no internal settings pointer) while still letting callers supply settings to later operations. The signature change itself is consistent with other input schemes.

Also applies to: 95-111, 127-135

236-239: Settings-aware archive download, cache use, and locking behavior look coherent

downloadArchive and getAccessor now take const Settings & and use settings.getCache() / settings.getTarballCache() and settings.trustTarballsFromGitForges, removing the dependency on per-Input settings. isLocked now requires a revision and either a NAR hash or trustTarballsFromGitForges, which matches the comment about needing integrity information unless tarballs from git forges are explicitly trusted. Overall this centralizes policy in Settings and should avoid refetching when the tarball cache and trust settings allow reuse.

Also applies to: 246-248, 255-258, 264-275, 281-297, 318-339, 341-348

392-405: GitHub scheme correctly threads Settings through API and archive paths

The GitHub-specific overrides (getRevFromRef, getDownloadUrl, clone) consistently accept and use const Settings &—for auth headers, cached API/tarball downloads via downloadFile(store, settings, ...), and for constructing a git+https:// input via Input::fromURL(settings, ...) that is then cloned with .clone(settings, store, destDir). This aligns with the generic Git input code and should preserve behavior while honoring the configured caches and tokens.

Also applies to: 412-427, 429-436

465-479: GitLab and SourceHut schemes follow the same Settings-plumbing pattern

For both GitLab and SourceHut, the updated getRevFromRef, getDownloadUrl, and clone methods now accept const Settings & and use it for auth headers and downloadFile(store, settings, ...) calls, and to construct git+https:// URLs via Input::fromURL(settings, ...) before delegating to .clone(settings, store, destDir). The behavior mirrors the GitHub scheme and the generic git fetcher, so the change looks correct and consistent.

Also applies to: 491-508, 510-520, 540-557, 572-587, 589-601, 603-612

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

11-19: Purity check now depends explicitly on EvalState’s fetch settings

Using flakeRef.input.isLocked(state.fetchSettings) ties the builtins.getFlake purity guard to the current fetch configuration (e.g. tarball trust, scheme-specific locking rules) instead of a hard-wired notion of “locked”. This is consistent with the new isLocked(Settings) API; just be aware this may slightly change which flake refs are accepted under pureEval depending on settings, and confirm that matches your intended semantics.

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

49-49: LGTM: Fetch settings properly threaded through accessor retrieval.

The update to pass fetchSettings as the first parameter to getAccessor is correct and aligns with the broader API changes across the PR.

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

84-84: LGTM: Fetch settings properly threaded through fetchToStore.

The update correctly passes state.fetchSettings as the first parameter, consistent with the API changes throughout the PR.

src/libfetchers/builtin.cc (1)

51-51: LGTM: Fetch settings properly passed to accessor.

The update correctly threads myFetchSettings through the getAccessor call, maintaining consistency with the API changes.

src/nix/registry.cc (1)

193-195: LGTM: Fetch settings consistently threaded through registry pinning.

The updates correctly pass fetchSettings to resolve, getAccessor, and isLocked, maintaining consistency throughout the resolution chain.

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

199-199: LGTM: Test updated to match new API signature.

The test correctly passes the settings parameter to getAccessor, maintaining alignment with the updated API.

src/libflake/lockfile.cc (2)

44-44: LGTM: Fetch settings threaded through lock validation.

The update correctly passes fetchSettings to isLocked, consistent with the API changes.

265-265: LGTM: Fetch settings threaded through lock check lambda.

The isConsideredLocked lambda correctly uses fetchSettings in both the isLocked check and the allowDirtyLocks condition.

src/libfetchers/indirect.cc (3)

47-47: LGTM: Input construction updated to not store settings.

The change from Input input{settings} to Input input{} aligns with the refactor to make settings an explicit parameter rather than storing it in the Input object.

79-79: LGTM: Input construction updated to not store settings.

Consistent with the broader refactor to pass settings explicitly rather than storing them.

109-110: LGTM: getAccessor signature properly updated.

The method signature correctly adds const Settings & settings as the first parameter, maintaining consistency with the API changes across all fetcher implementations.

src/nix/flake.cc (5)

258-258: LGTM: Lock status check properly uses fetch settings.

The update correctly passes fetchSettings to the isLocked check.

1053-1053: LGTM: Fetch settings threaded through resolve and clone.

The chained calls correctly pass fetchSettings to both resolve and clone operations.

1092-1092: LGTM: Fetch settings passed to fetchToStore.

The update correctly threads fetchSettings through the fetchToStore call.

1104-1107: LGTM: Fetch settings passed to nested fetchToStore.

The conditional expression correctly passes fetchSettings to fetchToStore in the non-dry-run branch.

1445-1446: LGTM: Fetch settings threaded through prefetch operations.

Both resolve and lazyFetch correctly receive fetchSettings as their first parameter, maintaining consistency with the API changes.

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

67-71: FlakeRef resolve/lazyFetch signatures align with Settings-threading refactor

Extending FlakeRef::resolve and lazyFetch to take const fetchers::Settings & explicitly is consistent with the rest of the fetch pipeline and keeps registry resolution and fetching behavior tied to the active settings. The header surface remains clear and the default for UseRegistries is preserved.

Also applies to: 74-76

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

31-42: resolve/lazyFetch updates look correct; verify choice of global vs state fetch settings

Threading fetchSettings into flakeRef.resolve(...).lazyFetch(...) in both the EvalSettings “flake” handler and the flake: lookup path is required by the new signatures and ensures those steps honor the configured fetch behavior.

One subtle point: both parseFlakeRef and resolve here use the global fetchSettings, whereas the subsequent fetchToStore calls use state.fetchSettings. If those two settings objects can differ (e.g., per-eval overrides layered on top of global config), resolution and fetching might observe slightly different configuration. If they are guaranteed to be equivalent, this is fine; otherwise it may be safer to route the same effective settings through both resolution and fetching (e.g., using the state.fetchSettings view where possible).

Also applies to: 183-190

124-131: override-flake now passes Settings into registry override as expected

Using fetchers::overrideRegistry(fetchSettings, from.input, to.input, extraAttrs) ensures that override entries are registered under the same Settings instance that drives other registry lookups in this command. This is consistent with the rest of the Settings-threading changes.

src/libflake/flakeref.cc (2)

38-43: Explicit Settings in FlakeRef::resolve look correct

Passing fetchSettings through to lookupInRegistries aligns this path with the new registry API and avoids relying on any implicit settings on Input. Control flow and error behavior remain unchanged.

262-267: lazyFetch correctly threads Settings into Input::getAccessor

The updated signature and call to input.getAccessor(fetchSettings, store) match the new accessor API and keep the returned (accessor, lockedInput) behavior intact.

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

85-86: fetchTree’s Settings propagation and cache usage look sound

fetchTree now:

Uses a plain fetchers::Input input{} and always initializes it via fromAttrs/fromURL.

Calls lookupInRegistries(state.fetchSettings, state.store, input, UseRegistries::Limited) only in the non‑pure, indirect case.

Uses input.isLocked(state.fetchSettings) for the pure-eval check, aligning with the new fetcher interface.

Invokes state.inputCache->getAccessor(state.fetchSettings, state.store, input, UseRegistries::No) so the cache operates on already-resolved inputs without further registry lookups.

This matches the new Settings-aware fetcher APIs and the PR’s goal of making fetch behavior consistent, without changing the user-visible semantics of fetchTree/fetchGit.

Also applies to: 185-187, 188-200, 211-213

src/libfetchers/mercurial.cc (1)

83-95: Mercurial fetcher’s Settings-aware refactor preserves behavior

The Mercurial scheme now:

Builds inputs via Input input{} in inputFromAttrs, matching the new Input design without embedded settings.

Threads const Settings & settings through fetchToStore and getAccessor, using:

settings.allowDirty / settings.warnDirty for dirty-tree handling, and

settings.getCache() for lookupWithTTL, lookupStorePath, and upsert on Mercurial cache entries.

Leaves the fetch algorithm, cache keys, and rev/ref bookkeeping unchanged.

Updates isLocked to match the new interface while still simply checking for input.getRev().

Overall, this aligns Mercurial with the new Settings-aware fetcher APIs without introducing functional changes.

Also applies to: 157-323, 325-343

src/libflake/flake.cc (2)

337-338: Threading fetchSettings into input cache accessors looks correct

Passing state.fetchSettings into state.inputCache->getAccessor(...) in both places maintains existing behavior while aligning with the new InputCache::getAccessor signature; accessor reuse remains driven by the same settings context. No further changes needed here.

Also applies to: 354-355

674-675: Using isLocked(fetchSettings) and getAccessor(fetchSettings, …) keeps lock semantics consistent

The updated calls to input.ref->input.isLocked(state.fetchSettings) and state.inputCache->getAccessor(state.fetchSettings, …) correctly route lock checks and fetches through the new Settings‑aware APIs without changing logic. This cleanly prepares for schemes whose lock status may depend on settings.

Also applies to: 747-748

src/libfetchers/fetchers.cc (3)

73-88: Default-constructing Input in fromAttrs matches the new design

Switching the “raw” path in Input::fromAttrs to Input input; (instead of a Settings‑bound construction) is consistent with removing Settings from Input state. fixupInput(input) still validates common attributes, so behavior for unknown schemes is preserved.

163-166: Settings-aware Input fetch and accessor methods are wired consistently

Input::isLocked(const Settings & settings) now delegates to scheme->isLocked(settings, *this) when a scheme exists, which centralizes lock semantics in schemes.

fetchToStore, getAccessor, and getAccessorUnchecked all take const Settings & settings and pass it through to scheme implementations and nix::fetchToStore(settings, …), and use settings.getCache() for source-path→hash caching.

Error handling and lock checking (checkLocks) remain unchanged.

Signatures and internal calls are coherent and match the new Settings‑first API; no functional issues spotted.

Also applies to: 201-227, 285-299, 316-337

394-398: Cloning via Settings-aware APIs is straightforward and consistent

Input::clone(const Settings &, ref<Store>, …) forwarding to scheme->clone(settings, store, *this, destDir) and InputScheme::clone using getAccessor(settings, store, input) to materialize the tree both align with the new API surface. The clone semantics remain the same, just with explicit settings.

Also applies to: 510-517

src/libfetchers/tarball.cc (3)

217-228: downloadTarball now correctly reuses the Settings-aware accessor path

Routing downloadTarball through Input::fromAttrs(settings, …) and input.getAccessor(settings, store) ensures tarball downloads go through the same Settings-aware accessor machinery and caching as other inputs, rather than bypassing it. This looks correct and matches the broader API changes.

250-287: Curl-based schemes’ construction and locking semantics stay minimal and correct

Using Input input{} in CurlInputScheme::inputFromURL / inputFromAttrs is compatible with the new Input layout and simply initializes an empty attribute set before filling it.

CurlInputScheme::isLocked(const Settings &, const Input &) basing lock status solely on narHash is unchanged in practice and ignores Settings as expected.

No issues here; the behavior remains straightforward.

Also applies to: 303-310, 322-325

343-353: File and tarball schemes now consistently honor Settings and tarball cache

FileInputScheme::getAccessor(const Settings &, ref<Store>, …) now calls downloadFile(store, settings, …), letting the per-evaluation cache (settings.getCache()) control HTTP/file caching.

TarballInputScheme::getAccessor(const Settings &, …) passes settings into downloadTarball_ and uses settings.getTarballCache()->treeHashToNarHash(settings, result.treeHash) to derive narHash, aligning nar-hash computation and caching with the new Settings-aware tarball cache API.

These changes are cohesive with the rest of the PR; behavior is as intended.

Also applies to: 381-403

src/libfetchers/git.cc (3)

223-241: inputFromAttrs’s switch to default Input construction is safe

Using Input input{}; and then normalizing attributes (URL fixup, shallow/submodules/allRefs flags) keeps the behavior of Git inputs the same while decoupling Input from any embedded Settings pointer. No functional differences here.

630-638: Shallow-clone decision and repo reuse logic look correct and match the PR goal

The new canDoShallow and the refactored getAccessorFromCommit achieve the intended optimization and preserve semantics:

canDoShallow enabling shallow clones when either shallow = true or revCount is already known matches the comment and avoids recomputing revCount for already-locked inputs.

When rev is known and shallow is true, reusing a pre-existing non-shallow cache (cacheDirNonShallow) if it already contains the desired commit cleanly avoids creating a redundant shallow clone; the early goto have_rev path only does read-only operations on that repo.

lastModified and revCount are now computed only when the corresponding attributes are absent. This avoids unnecessary work and respects caller-provided metadata; the added comments make the trade-off explicit.

The shallow-repo guard now only triggers when we need to compute revCount (!shallow && !input.attrs.contains("revCount")), which means inputs explicitly marked shallow or already carrying a revCount will no longer error on shallow repositories.

Control flow is a bit intricate but internally consistent; the optimization to reuse non-shallow clones for revCount-bearing inputs directly addresses the “non-shallow then shallow refetch” issue described in the PR.

Also applies to: 640-781

819-821: Submodule handling, workdir accessors, and Settings-aware wiring are cohesive

For both committed and workdir submodules, constructing submodule inputs with Input::fromAttrs(settings, …) and then calling submoduleInput.getAccessor(settings, store) ensures submodule fetches participate fully in the Settings-aware caching and locking logic, rather than hard-coding any global settings.

getAccessorFromWorkdir now uses settings for getRevCount, getLastModified, and repoInfo.warnDirty(settings), so allowDirty/warnDirty and revCount caching respect the caller’s fetch settings.

Top-level getAccessor(const Settings &, ref<Store>, …) cleanly chooses between commit-based and workdir-based accessors based on whether we have a ref/rev or only a working tree, with an early guard for the unsupported exportIgnore && submodules combination.

GitInputScheme::isLocked(const Settings &, const Input &) still defines “locked” as “has a non-null, non-nullRev rev”, which matches previous behavior; Settings is correctly ignored here.

Overall, the Git fetcher’s Settings threading and submodule behavior look consistent and aligned with the rest of the refactor.

Also applies to: 869-871, 895-903, 914-916, 836-918, 920-941, 974-978

cole-h · 2025-11-18T16:43:52Z

src/libfetchers/git.cc

+                        repoUrl.to_string(),
+                        rev->gitRev());
+                    repoDir = cacheDirNonShallow;
+                    goto have_rev;


goto makes me feel things...

builtins.fetchTree was setting `shallow = true` when fetching from git. That's bad because it makes it behave inconsistently from non-fetchTree fetches, e.g. when updating an input. Instead, the Git fetcher now will do a shallow fetch automatically if `revCount` is already set (e.g. when fetching a lock). Fixes NixOS#14588.

This fixes the issue where updating a Git input does a non-shallow fetch, and then a subsequent eval does a shallow refetch because the revCount is already known. Now the subsequent eval will use the repo used in the first fetch.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/nixos/fetch-git/test-cases/fetchTree-shallow/default.nix (1)
31-31: Clarify the intent of revCount = 1234.

The test creates only 2 commits (lines 10-17), but revCount is set to 1234, which doesn't match the actual commit count. This could be:

Intentionally testing that an incorrect/stale revCount doesn't break shallow fetching

Testing the PR's fix where a pre-known revCount should allow reuse of the existing clone

Consider adding a comment explaining why this specific value is used and what scenario it's testing to improve test maintainability.

For example:
+        # Use an arbitrary revCount to test that pre-known revCount values
+        # don't trigger unnecessary refetches (issue #14588)
         revCount = 1234;

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d818593 and ced5dbe.

📒 Files selected for processing (1)

tests/nixos/fetch-git/test-cases/fetchTree-shallow/default.nix (2 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). (3)

GitHub Check: build_aarch64-darwin / manual
GitHub Check: build_aarch64-darwin / test
GitHub Check: build_x86_64-linux / build

🔇 Additional comments (1)

tests/nixos/fetch-git/test-cases/fetchTree-shallow/default.nix (1)

2-2: Good description update.

The change from "by default" to "if possible" accurately reflects the removal of automatic shallow = true injection mentioned in the PR objectives.

edolstra · 2025-11-24T16:49:30Z

Upstream PR: NixOS#14633

github-actions bot temporarily deployed to pull request November 18, 2025 16:06 Inactive

coderabbitai bot reviewed Nov 18, 2025

View reviewed changes

cole-h approved these changes Nov 18, 2025

View reviewed changes

github-actions bot temporarily deployed to pull request November 18, 2025 17:01 Inactive

edolstra force-pushed the shallow-optimization branch from e3fd7d9 to 27ae2e8 Compare November 18, 2025 17:16

github-actions bot temporarily deployed to pull request November 18, 2025 17:19 Inactive

edolstra added 3 commits November 18, 2025 20:53

Add test

c1660ef

edolstra force-pushed the shallow-optimization branch from 27ae2e8 to c1660ef Compare November 18, 2025 19:53

edolstra enabled auto-merge November 18, 2025 19:54

github-actions bot temporarily deployed to pull request November 18, 2025 19:56 Inactive

edolstra added this pull request to the merge queue Nov 18, 2025

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Nov 18, 2025

edolstra added this pull request to the merge queue Nov 18, 2025

edolstra removed this pull request from the merge queue due to a manual request Nov 18, 2025

edolstra enabled auto-merge November 18, 2025 21:40

edolstra disabled auto-merge November 18, 2025 21:43

Fix test

ced5dbe

edolstra force-pushed the shallow-optimization branch from d818593 to ced5dbe Compare November 18, 2025 21:46

edolstra enabled auto-merge November 18, 2025 21:46

coderabbitai bot reviewed Nov 18, 2025

View reviewed changes

github-actions bot temporarily deployed to pull request November 18, 2025 21:50 Inactive

edolstra added this pull request to the merge queue Nov 18, 2025

Merged via the queue into main with commit f14ea84 Nov 18, 2025
28 checks passed

edolstra deleted the shallow-optimization branch November 18, 2025 22:31

coderabbitai bot mentioned this pull request Nov 20, 2025

Backward compatibility hack for Nix < 2.20 Git inputs #275

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Avoid unnecessary Git refetches #270

Avoid unnecessary Git refetches #270

Uh oh!

edolstra commented Nov 18, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 18, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Nov 18, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

cole-h Nov 18, 2025

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

edolstra commented Nov 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Avoid unnecessary Git refetches #270

Avoid unnecessary Git refetches #270

Uh oh!

Conversation

edolstra commented Nov 18, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Context

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

github-actions bot commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

cole-h Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

edolstra commented Nov 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

edolstra commented Nov 18, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 18, 2025 •

edited

Loading

github-actions bot commented Nov 18, 2025 •

edited

Loading