Cache getLegacyGitAccessor()#396
Merged
Merged
Conversation
This prevents repeated calls to git checkout / git archive.
📝 WalkthroughWalkthroughUpdated Changes
Sequence DiagramsequenceDiagram
participant Caller as getAccessorFromCommit
participant Accessor as getLegacyGitAccessor
participant Cache as settings.getCache()
participant Export as Legacy Git Export
participant Store as Fixed-output Store
Caller->>Accessor: call(settings, repo, options)
Accessor->>Accessor: compute fingerprint & cacheKey
Accessor->>Cache: lookup(cacheKey)
alt cache hit
Cache-->>Accessor: return stored SRI
Accessor->>Accessor: parse SRI -> nar path
Accessor->>Store: maybeQueryPathInfo(path)
Store-->>Accessor: path info
Accessor-->>Caller: return accessor (cached)
else cache miss
Cache-->>Accessor: not found
Accessor->>Export: run legacy git export (git init/fetch/checkout)
Export-->>Accessor: narHash SRI
Accessor->>Cache: upsert(cacheKey, {"hash": SRI})
Accessor-->>Caller: return accessor (new, fingerprint set)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libfetchers/git.cc`:
- Around line 843-847: The git init call uses the unsupported "-b master" form;
replace that argument list in the runProgram invocation that calls git init (the
runProgram entry that currently uses {"init", tmpDir, "-b", "master"}) with the
documented compatibility pattern using "-c",
"init.defaultBranch=<gitInitialBranch>", "init", tmpDir and reuse the existing
gitInitialBranch variable so the init honors the configured default branch
consistently with the other git invocations.
cole-h
approved these changes
Mar 24, 2026
| ? [&]() { | ||
| // Nix < 2.20 used `git checkout` for repos with submodules. | ||
| runProgram({.program = "git", .args = {"init", tmpDir}}); | ||
| runProgram({.program = "git", .args = {"init", tmpDir, "-b", "master"}}); |
Member
There was a problem hiding this comment.
Why is the -b master required now? Just to make it "reproducible" (more easily correctly cache-able)?
Member
There was a problem hiding this comment.
Also related (I think): the LLM comment above.
Collaborator
Author
There was a problem hiding this comment.
It's to shut up a harmless warning about not having a default branch.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/libfetchers/git.cc (1)
802-805: Consider acceptingoptionsby value to avoid mutating caller's state.The function modifies
options.exportIgnoreon line 805, butoptionsis passed by mutable reference. While the current control flow doesn't cause issues (the submodules block is guarded byif (options.submodules)), mutating input parameters can lead to subtle bugs during future refactoring.♻️ Suggested fix
ref<SourceAccessor> getLegacyGitAccessor( const Settings & settings, Store & store, RepoInfo & repoInfo, const std::filesystem::path & repoDir, const Hash & rev, - GitAccessorOptions & options) const + GitAccessorOptions options) const { - if (!options.submodules) - options.exportIgnore = true; + options.exportIgnore = options.exportIgnore || !options.submodules;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libfetchers/git.cc` around lines 802 - 805, This function currently takes GitAccessorOptions &options and mutates options.exportIgnore when !options.submodules; change the parameter to take GitAccessorOptions options (pass by value) so mutations affect only the local copy, update the function signature accordingly, and ensure all internal uses refer to the local options (including the existing check of options.submodules and assignment to options.exportIgnore) to avoid mutating the caller's state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/libfetchers/git.cc`:
- Around line 802-805: This function currently takes GitAccessorOptions &options
and mutates options.exportIgnore when !options.submodules; change the parameter
to take GitAccessorOptions options (pass by value) so mutations affect only the
local copy, update the function signature accordingly, and ensure all internal
uses refer to the local options (including the existing check of
options.submodules and assignment to options.exportIgnore) to avoid mutating the
caller's state.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This prevents repeated calls to
git checkout/git archivewhennix-219-compatis enabled.Context
Summary by CodeRabbit