feat: Adding central Git store for CAS#6003
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements a centralized Git store for CAS to optimize Git usage during cache misses. Instead of performing full shallow clones per operation, Terragrunt now maintains one bare repository per remote URL under the cache directory, performs incremental ref fetches, shares concurrent operations across runs, and falls back to temporary cloning if the shared store is unavailable. Supporting changes include context-aware VFS locking, new Git runner methods, modernized error handling, and remote source detection for proper URL preprocessing. ChangesCentral Git Store Optimization
Sequence DiagramsequenceDiagram
participant Client as Client
participant CAS as CAS
participant Detector as Detector
participant Store as GitStore
participant Lock as Per-URL Lock
participant Git as GitRunner
participant Remote as Remote Repo
participant Fallback as Fallback<br/>Temp Clone
Client->>CAS: Clone(ctx, url, ref)
CAS->>Detector: DetectRemoteSource(url)
Detector->>CAS: normalized url
CAS->>CAS: populateTreeFromGit()
CAS->>Store: EnsureRef(ctx, url, ref, hash, depth)
Store->>Store: EntryPathForURL(url)
Store->>Lock: LockContext(ctx, per-URL lock)
alt Lock acquired
Lock-->>Store: Unlocker
Store->>Git: InitBare()
Git->>Store: ✓
Store->>Git: HasObject(ctx, hash)
Git->>Store: false (not present)
Store->>Git: Fetch(ctx, url, ref, depth)
Git->>Remote: fetch --depth N ref
Remote-->>Git: pack files
Git->>Store: ✓
Store->>Git: HasObject(ctx, hash)
Git->>Store: true (now present)
Store->>Client: (repo path, Unlocker)
Client->>Lock: Release()
else Lock timeout or store error
Store->>Store: Log warning
Store->>Fallback: Bare clone to temp dir
Fallback->>Remote: git clone --bare --depth N
Remote-->>Fallback: repo
Fallback->>Client: (temp repo path, no-op Unlocker)
end
Client->>Git: LsTreeRecursive(from repo path)
Git->>Client: trees & blobs
Client->>CAS: Store all objects
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
yhakbar
force-pushed
the
feat/adding-central-git-store
branch
from
56ea209 to
343dcdf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cas/cas.go (1)
416-444:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClose
tmpHandleon theCatFileerror path.If
runner.CatFilefails, the function returns without closing the temp file first. That leaks the descriptor and can leave cleanup stuck on Windows.🩹 Proposed fix
tmpHandle, err := content.GetTmpHandle(hash) if err != nil { return err } + tmpClosed := false + defer func() { + if !tmpClosed { + err = errors.Join(err, tmpHandle.Close()) + } + }() tmpPath := tmpHandle.Name() @@ if err = tmpHandle.Close(); err != nil { return err } + tmpClosed = true if err = c.fs.Rename(tmpPath, content.getPath(hash)); err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cas/cas.go` around lines 416 - 444, runner.CatFile failure currently returns without closing tmpHandle, leaking the descriptor; ensure tmpHandle is closed on the CatFile error path by calling tmpHandle.Close() (and optionally capture/join its error) before returning the error from the block containing runner.CatFile, so the temporary file handle is always closed even if CatFile fails; reference tmpHandle, runner.CatFile, and tmpHandle.Close in your fix and preserve the existing defer that removes the tmpPath.
🧹 Nitpick comments (1)
internal/cas/stacks.go (1)
44-47: 💤 Low valueConsider preserving the original URL for error reporting.
If
DetectRemoteSourcereturns an error,repoURLon line 46 may have been modified (or set to empty string) by the failed call. The error message could display an unexpected value.♻️ Suggested fix
+ originalURL := repoURL repoURL, err := DetectRemoteSource(repoURL) if err != nil { - return nil, fmt.Errorf("failed to detect source URL %q: %w", repoURL, err) + return nil, fmt.Errorf("failed to detect source URL %q: %w", originalURL, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cas/stacks.go` around lines 44 - 47, Before calling DetectRemoteSource, save the original repo URL to a separate variable (e.g., origRepoURL) so the original value can be used in error messages; then call DetectRemoteSource(repoURL) and, on error, return an error that references origRepoURL (not the possibly mutated repoURL) in the fmt.Errorf call. Update the block around DetectRemoteSource to use origRepoURL for error reporting while continuing to use the returned repoURL on success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cas/cas.go`:
- Around line 134-139: In CAS.New(), don't abort on NewGitStore failure: instead
of returning the error, set c.gitStore = nil (or a disabled marker) and proceed
so CAS construction degrades to the clone fallback; update the NewGitStore error
handling where c.gitStore is assigned to swallow the error (and optionally log
it) rather than return. Then modify populateTreeFromGit to check for a
nil/disabled c.gitStore before calling EnsureRef and follow the temporary-clone
flow when the store is nil, ensuring EnsureRef is only invoked on a valid git
store.
In `@internal/cas/getter.go`:
- Around line 65-74: NewCASGetter can receive a nil opts which later causes a
panic when Get dereferences g.Opts; update NewCASGetter to check if opts == nil
and if so allocate a zero-value CloneOptions before assigning to CASGetter.
Reference the NewCASGetter constructor and CASGetter.Opts (and the Get method
that dereferences g.Opts) and ensure the function returns a CASGetter with Opts
non-nil to make the constructor safe for callers passing nil.
---
Outside diff comments:
In `@internal/cas/cas.go`:
- Around line 416-444: runner.CatFile failure currently returns without closing
tmpHandle, leaking the descriptor; ensure tmpHandle is closed on the CatFile
error path by calling tmpHandle.Close() (and optionally capture/join its error)
before returning the error from the block containing runner.CatFile, so the
temporary file handle is always closed even if CatFile fails; reference
tmpHandle, runner.CatFile, and tmpHandle.Close in your fix and preserve the
existing defer that removes the tmpPath.
---
Nitpick comments:
In `@internal/cas/stacks.go`:
- Around line 44-47: Before calling DetectRemoteSource, save the original repo
URL to a separate variable (e.g., origRepoURL) so the original value can be used
in error messages; then call DetectRemoteSource(repoURL) and, on error, return
an error that references origRepoURL (not the possibly mutated repoURL) in the
fmt.Errorf call. Update the block around DetectRemoteSource to use origRepoURL
for error reporting while continuing to use the returned repoURL on success.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6555569a-6fc5-4075-acf0-4468c6a8a790
📒 Files selected for processing (14)
internal/cas/cas.gointernal/cas/cas_test.gointernal/cas/content.gointernal/cas/errors.gointernal/cas/getter.gointernal/cas/getter_detect_test.gointernal/cas/gitstore.gointernal/cas/gitstore_test.gointernal/cas/stacks.gointernal/cas/stacks_test.gointernal/cas/tree.gointernal/git/errors.gointernal/git/git.gointernal/git/git_test.go
Comment on lines
+134
to
+139
| gs, err := NewGitStore(c.fs, g, filepath.Join(c.storePath, "git")) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| c.gitStore = gs |
Contributor
There was a problem hiding this comment.
Make git-store setup degradable instead of aborting CAS.New().
The central store is an optimization, but this path currently fails construction before populateTreeFromGit can use its temporary-clone fallback. A bad .../cas/store/git entry disables CAS entirely instead of degrading to the older clone flow.
💡 Suggested direction
- gs, err := NewGitStore(c.fs, g, filepath.Join(c.storePath, "git"))
- if err != nil {
- return nil, err
- }
-
- c.gitStore = gs
+ gs, err := NewGitStore(c.fs, g, filepath.Join(c.storePath, "git"))
+ if err == nil {
+ c.gitStore = gs
+ }populateTreeFromGit would then need a nil/disabled-store branch before calling EnsureRef.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cas/cas.go` around lines 134 - 139, In CAS.New(), don't abort on
NewGitStore failure: instead of returning the error, set c.gitStore = nil (or a
disabled marker) and proceed so CAS construction degrades to the clone fallback;
update the NewGitStore error handling where c.gitStore is assigned to swallow
the error (and optionally log it) rather than return. Then modify
populateTreeFromGit to check for a nil/disabled c.gitStore before calling
EnsureRef and follow the temporary-clone flow when the store is nil, ensuring
EnsureRef is only invoked on a valid git store.
Contributor
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 `@internal/cas/gitstore.go`:
- Around line 44-53: The NewGitStore constructor must validate inputs: ensure
the git runner argument is non-nil and the rootPath is non-empty (and optionally
trim whitespace); if either is invalid, return a clear wrapped error instead of
proceeding (to prevent later panics at s.runner.WithWorkDir). Update NewGitStore
to check runner == nil and rootPath == "" (or strings.TrimSpace(rootPath) == "")
and return an error that includes ErrGitStorePath (or a new sentinel like
ErrInvalidGitStoreConfig) and a descriptive message; keep the existing MkdirAll
logic only after validation so the function never constructs a GitStore with a
nil runner or empty rootPath.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9961822-0b43-466c-b131-bb0a1a5ef101
📒 Files selected for processing (4)
docs/src/content/docs/03-features/07-caching/04-cas.mdxdocs/src/data/changelog/v1.0.4/cas-central-git-store.mdxinternal/cas/gitstore.gointernal/vfs/vfs.go
✅ Files skipped from review due to trivial changes (1)
- docs/src/data/changelog/v1.0.4/cas-central-git-store.mdx
Comment on lines
+44
to
+53
| func NewGitStore(fs vfs.FS, runner *git.GitRunner, rootPath string) (*GitStore, error) { | ||
| if err := fs.MkdirAll(rootPath, DefaultDirPerms); err != nil { | ||
| return nil, fmt.Errorf("create git store at %s: %w", rootPath, errors.Join(ErrGitStorePath, err)) | ||
| } | ||
|
|
||
| return &GitStore{ | ||
| runner: runner, | ||
| rootPath: rootPath, | ||
| }, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Validate constructor inputs to avoid downstream panic paths.
Line 44 currently accepts a nil runner and empty rootPath. A nil runner can panic later at Line 102 (s.runner.WithWorkDir(...)), and an empty root path can produce unintended filesystem behavior.
Suggested fix
func NewGitStore(fs vfs.FS, runner *git.GitRunner, rootPath string) (*GitStore, error) {
+ if runner == nil {
+ return nil, fmt.Errorf("create git store: nil git runner: %w", ErrGitStorePath)
+ }
+
+ if rootPath == "" {
+ return nil, fmt.Errorf("create git store: empty root path: %w", ErrGitStorePath)
+ }
+
if err := fs.MkdirAll(rootPath, DefaultDirPerms); err != nil {
return nil, fmt.Errorf("create git store at %s: %w", rootPath, errors.Join(ErrGitStorePath, err))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cas/gitstore.go` around lines 44 - 53, The NewGitStore constructor
must validate inputs: ensure the git runner argument is non-nil and the rootPath
is non-empty (and optionally trim whitespace); if either is invalid, return a
clear wrapped error instead of proceeding (to prevent later panics at
s.runner.WithWorkDir). Update NewGitStore to check runner == nil and rootPath ==
"" (or strings.TrimSpace(rootPath) == "") and return an error that includes
ErrGitStorePath (or a new sentinel like ErrInvalidGitStoreConfig) and a
descriptive message; keep the existing MkdirAll logic only after validation so
the function never constructs a GitStore with a nil runner or empty rootPath.
182616f to
44055e9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/src/content/docs/03-features/07-caching/04-cas.mdx`:
- Line 209: The docs say Terragrunt logs a warning when a shared fetch
hangs/fails, but the fallback path currently uses Debugf; change the logging
call in the shared-fetch fallback path from Debugf to Warnf (preserve or
slightly adjust the message text to match the docs' wording) so operators will
see it at normal log levels; locate the Debugf call in the fallback branch in
cas.go (the shared-fetch/fallback logic) and replace it with a Warnf call.
In `@internal/cas/gitstore.go`:
- Around line 113-121: EnsureRef currently passes depth==0 through to
runner.Fetch which omits --depth and causes a full-history fetch; normalize the
effective depth before calling runner.Fetch (e.g., compute effectiveDepth :=
depth; if effectiveDepth == 0 { effectiveDepth = configuredDefaultShallowDepth }
or resolve it once in the caller and pass that value into EnsureRef) and use
that effectiveDepth in the call to runner.Fetch(ctx, url, fetchRef,
effectiveDepth); update any callers or documentation of EnsureRef to reflect
that EnsureRef expects a resolved/shallow-normalized depth if you choose to
resolve upstream.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4828d8e8-0bac-4ac9-b042-ce480dfada4d
📒 Files selected for processing (6)
docs/src/content/docs/03-features/07-caching/04-cas.mdxdocs/src/data/changelog/v1.0.4/cas-central-git-store.mdxinternal/cas/gitstore.gointernal/cas/gitstore_test.gointernal/vfs/vfs.gointernal/vfs/vfs_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/src/data/changelog/v1.0.4/cas-central-git-store.mdx
denis256
reviewed
May 1, 2026
yhakbar
force-pushed
the
feat/adding-central-git-store
branch
from
3baec29 to
3a8acae
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
internal/cas/gitstore.go (2)
132-139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
depth == 0before fetch to avoid unbounded history pulls.Passing
0directly intoFetchcan bypass shallow behavior and turn cache-miss fetches into full-history downloads.Suggested fix
if !has { fetchRef := ref if fetchRef == "" { fetchRef = "HEAD" } + effectiveDepth := depth + if effectiveDepth == 0 { + effectiveDepth = DefaultCASCloneDepth + } - if err := runner.Fetch(ctx, url, fetchRef, depth); err != nil { + if err := runner.Fetch(ctx, url, fetchRef, effectiveDepth); err != nil { return "", nil, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cas/gitstore.go` around lines 132 - 139, When a cache miss triggers runner.Fetch in the block that computes fetchRef, ensure the depth parameter is normalized so a zero value doesn't request full history: before calling runner.Fetch(ctx, url, fetchRef, depth) set a localDepth (or reuse depth) to a default shallow value when depth == 0 (e.g., localDepth := depth; if localDepth == 0 { localDepth = 1 or DEFAULT_SHALLOW_DEPTH }) and pass that normalized value into runner.Fetch; update references around has, fetchRef and runner.Fetch to use the normalized depth variable.
48-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard constructor inputs before storing state.
NewGitStorecurrently accepts a nilrunnerand emptyrootPath. A nil runner can panic later whenEnsureRefcallss.runner.WithWorkDir(...); empty path should fail fast with a clear config error.Suggested fix
func NewGitStore(fs vfs.FS, runner *git.GitRunner, rootPath string) (*GitStore, error) { if !vfs.IsOSFS(fs) { return nil, ErrGitStoreFSNotOS } + + if runner == nil { + return nil, fmt.Errorf("create git store: nil runner: %w", ErrGitStorePath) + } + if rootPath == "" { + return nil, fmt.Errorf("create git store: empty root path: %w", ErrGitStorePath) + } if err := fs.MkdirAll(rootPath, DefaultDirPerms); err != nil { return nil, fmt.Errorf("create git store at %s: %w", rootPath, errors.Join(ErrGitStorePath, err)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cas/gitstore.go` around lines 48 - 60, NewGitStore must validate inputs before storing state: return an explicit error if runner is nil or rootPath is empty/whitespace so callers fail fast and avoid panics later (e.g., when EnsureRef calls s.runner.WithWorkDir). In NewGitStore check that runner != nil and that strings.TrimSpace(rootPath) != "" and return distinct sentinel errors (or wrapped fmt.Errorf) like ErrGitStoreNoRunner and ErrGitStoreInvalidPath; only after these validations create and return the &GitStore{runner: runner, rootPath: rootPath}.
🧹 Nitpick comments (1)
internal/cas/gitstore.go (1)
73-155: 🏗️ Heavy liftReduce
EnsureRefcomplexity by splitting lock/init/fetch phases.
EnsureRefcurrently mixes lock lifecycle, repo init, fetch, and postconditions in one flow. Extracting focused helpers would make lock-release guarantees easier to audit and address the cognitive complexity warning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cas/gitstore.go` around lines 73 - 155, EnsureRef is doing too much: split its lock lifecycle, repo initialization, and fetch/verification into separate helper functions to reduce complexity and make lock-release clear. Extract a function like acquireRepoLock(ctx, fs, url) that computes repoPaths, creates dir, locks and returns (repoPath, lockPath, unlocker), another like ensureBareRepoInitialized(ctx, runner, fs, repoPath) that calls bareRepoInitialized and runner.InitBare, and a third like fetchAndVerify(ctx, runner, url, ref, hash, depth) that runs runner.Fetch and runner.HasObject and returns a GitStoreObjectMissingError when appropriate; then rewrite EnsureRef to call these helpers in sequence, manage the unlocker only in the lock helper/EnsureRef so release is obvious (remove the released bool dance), and keep error wrapping consistent around filesystem and lock errors as in the original (use ErrGitStorePath/ErrGitStoreLock and fmt.Errorf).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/src/content/docs/03-features/07-caching/04-cas.mdx`:
- Around line 121-123: Update the CAS cleanup guidance to explicitly mention
that users can safely reclaim most clone-related disk space by removing the git
subdirectory (cas/store/git/) when Terragrunt is not running; add a short
sentence after the existing CAS deletion guidance clarifying "You can delete
cas/store/git/ to reclaim most clone-related space when no Terragrunt processes
are active, but avoid deleting it during operations as this can race with
in-flight reads/writes/locks" and keep the warning about avoiding partial
deletions and not removing anything while Terragrunt is running.
In `@internal/cas/content.go`:
- Around line 180-183: The MkdirAll error handling in the partition creation
uses the sentinel ErrCreateDir instead of the actual returned error; update the
block in the method that computes partitionDir (using c.getPartition) so that
the returned fmt.Errorf wraps the real err from c.fs.MkdirAll (e.g., return
fmt.Errorf("create partition dir %s: %w", partitionDir, err)) while keeping the
descriptive message and existing symbols (partitionDir, c.getPartition,
c.fs.MkdirAll, ErrCreateDir) for context.
- Around line 150-158: The error handling after c.fs.MkdirAll is discarding the
real mkdir error by returning ErrCreateDir instead of wrapping the actual err;
update both returns so they wrap the original err (from c.fs.MkdirAll) using %w
(e.g., in the store directory case replace ErrCreateDir with err in
fmt.Errorf("create store dir %s: %w", c.store.Path(), err)) and do the same for
the partition directory case (fmt.Errorf("create partition dir %s: %w",
partitionDir, err)) so the true underlying mkdir error is preserved; locate
these changes around c.fs.MkdirAll, c.store.Path(), and c.getPartition.
- Around line 213-215: The call to c.fs.MkdirAll(partitionDir, DefaultDirPerms)
incorrectly wraps the sentinel ErrCreateDir instead of the actual error (err)
and reuses the literal format "create partition dir %s: %w" in multiple places;
change the fmt.Errorf to wrap the returned err (i.e., fmt.Errorf("create
partition dir %s: %w", partitionDir, err)) and extract the format string into a
shared constant (e.g., createPartitionDirFmt) used by the same call sites to
avoid duplication and satisfy SonarCloud; update the code around c.fs.MkdirAll,
partitionDir, DefaultDirPerms and any repeated occurrences to use the new
constant and wrap err.
- Around line 107-114: The current returns after MkdirAll use the sentinel
ErrCreateDir as the wrapped error, losing the original OS error from
c.fs.MkdirAll; change the fmt.Errorf calls that now do fmt.Errorf("...: %w",
ErrCreateDir) to preserve the original err (wrap err with %w) and if you still
need the sentinel for errors.Is, include both by formatting like "...: %w: %v",
ErrCreateDir, err so the original err is visible while keeping the sentinel
check; update the two locations using c.store.Path() and partitionDir (from
c.getPartition(hash)) that call c.fs.MkdirAll accordingly.
---
Duplicate comments:
In `@internal/cas/gitstore.go`:
- Around line 132-139: When a cache miss triggers runner.Fetch in the block that
computes fetchRef, ensure the depth parameter is normalized so a zero value
doesn't request full history: before calling runner.Fetch(ctx, url, fetchRef,
depth) set a localDepth (or reuse depth) to a default shallow value when depth
== 0 (e.g., localDepth := depth; if localDepth == 0 { localDepth = 1 or
DEFAULT_SHALLOW_DEPTH }) and pass that normalized value into runner.Fetch;
update references around has, fetchRef and runner.Fetch to use the normalized
depth variable.
- Around line 48-60: NewGitStore must validate inputs before storing state:
return an explicit error if runner is nil or rootPath is empty/whitespace so
callers fail fast and avoid panics later (e.g., when EnsureRef calls
s.runner.WithWorkDir). In NewGitStore check that runner != nil and that
strings.TrimSpace(rootPath) != "" and return distinct sentinel errors (or
wrapped fmt.Errorf) like ErrGitStoreNoRunner and ErrGitStoreInvalidPath; only
after these validations create and return the &GitStore{runner: runner,
rootPath: rootPath}.
---
Nitpick comments:
In `@internal/cas/gitstore.go`:
- Around line 73-155: EnsureRef is doing too much: split its lock lifecycle,
repo initialization, and fetch/verification into separate helper functions to
reduce complexity and make lock-release clear. Extract a function like
acquireRepoLock(ctx, fs, url) that computes repoPaths, creates dir, locks and
returns (repoPath, lockPath, unlocker), another like
ensureBareRepoInitialized(ctx, runner, fs, repoPath) that calls
bareRepoInitialized and runner.InitBare, and a third like fetchAndVerify(ctx,
runner, url, ref, hash, depth) that runs runner.Fetch and runner.HasObject and
returns a GitStoreObjectMissingError when appropriate; then rewrite EnsureRef to
call these helpers in sequence, manage the unlocker only in the lock
helper/EnsureRef so release is obvious (remove the released bool dance), and
keep error wrapping consistent around filesystem and lock errors as in the
original (use ErrGitStorePath/ErrGitStoreLock and fmt.Errorf).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40eed2be-0eb4-462a-b954-e369f26f210e
📒 Files selected for processing (18)
docs/src/content/docs/03-features/07-caching/04-cas.mdxdocs/src/data/changelog/v1.0.5/cas-central-git-store.mdxinternal/cas/cas.gointernal/cas/cas_test.gointernal/cas/content.gointernal/cas/errors.gointernal/cas/getter.gointernal/cas/getter_detect_test.gointernal/cas/gitstore.gointernal/cas/gitstore_test.gointernal/cas/stacks.gointernal/cas/stacks_test.gointernal/cas/tree.gointernal/git/errors.gointernal/git/git.gointernal/git/git_test.gointernal/vfs/vfs.gointernal/vfs/vfs_test.go
✅ Files skipped from review due to trivial changes (2)
- docs/src/data/changelog/v1.0.5/cas-central-git-store.mdx
- internal/git/errors.go
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/cas/stacks_test.go
- internal/cas/stacks.go
- internal/cas/getter.go
- internal/cas/gitstore_test.go
- internal/cas/tree.go
- internal/vfs/vfs.go
- internal/cas/getter_detect_test.go
- internal/cas/errors.go
- internal/vfs/vfs_test.go
- internal/git/git.go
- internal/cas/cas.go
Comment on lines
107
to
+114
| if err = c.fs.MkdirAll(c.store.Path(), DefaultDirPerms); err != nil { | ||
| return wrapError("create_store_dir", c.store.Path(), ErrCreateDir) | ||
| return fmt.Errorf("create store dir %s: %w", c.store.Path(), ErrCreateDir) | ||
| } | ||
|
|
||
| // Ensure partition directory exists | ||
| partitionDir := c.getPartition(hash) | ||
| if err = c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil { | ||
| return wrapError("create_partition_dir", partitionDir, ErrCreateDir) | ||
| return fmt.Errorf("create partition dir %s: %w", partitionDir, ErrCreateDir) |
Contributor
There was a problem hiding this comment.
Original error is lost when wrapping ErrCreateDir.
Lines 108 and 114 wrap the sentinel ErrCreateDir instead of the actual err returned by MkdirAll. This discards the underlying OS error (e.g., permission denied, disk full), making debugging harder. Other error sites in this file correctly wrap err.
🛠️ Proposed fix to preserve original error
if err = c.fs.MkdirAll(c.store.Path(), DefaultDirPerms); err != nil {
- return fmt.Errorf("create store dir %s: %w", c.store.Path(), ErrCreateDir)
+ return fmt.Errorf("create store dir %s: %w", c.store.Path(), err)
}
// Ensure partition directory exists
partitionDir := c.getPartition(hash)
if err = c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil {
- return fmt.Errorf("create partition dir %s: %w", partitionDir, ErrCreateDir)
+ return fmt.Errorf("create partition dir %s: %w", partitionDir, err)
}If the sentinel is needed for errors.Is checks, consider wrapping both:
return fmt.Errorf("create partition dir %s: %w: %v", partitionDir, ErrCreateDir, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err = c.fs.MkdirAll(c.store.Path(), DefaultDirPerms); err != nil { | |
| return wrapError("create_store_dir", c.store.Path(), ErrCreateDir) | |
| return fmt.Errorf("create store dir %s: %w", c.store.Path(), ErrCreateDir) | |
| } | |
| // Ensure partition directory exists | |
| partitionDir := c.getPartition(hash) | |
| if err = c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil { | |
| return wrapError("create_partition_dir", partitionDir, ErrCreateDir) | |
| return fmt.Errorf("create partition dir %s: %w", partitionDir, ErrCreateDir) | |
| if err = c.fs.MkdirAll(c.store.Path(), DefaultDirPerms); err != nil { | |
| return fmt.Errorf("create store dir %s: %w", c.store.Path(), err) | |
| } | |
| // Ensure partition directory exists | |
| partitionDir := c.getPartition(hash) | |
| if err = c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil { | |
| return fmt.Errorf("create partition dir %s: %w", partitionDir, err) |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 114-114: Define a constant instead of duplicating this literal "create partition dir %s: %w" 4 times.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cas/content.go` around lines 107 - 114, The current returns after
MkdirAll use the sentinel ErrCreateDir as the wrapped error, losing the original
OS error from c.fs.MkdirAll; change the fmt.Errorf calls that now do
fmt.Errorf("...: %w", ErrCreateDir) to preserve the original err (wrap err with
%w) and if you still need the sentinel for errors.Is, include both by formatting
like "...: %w: %v", ErrCreateDir, err so the original err is visible while
keeping the sentinel check; update the two locations using c.store.Path() and
partitionDir (from c.getPartition(hash)) that call c.fs.MkdirAll accordingly.
Comment on lines
150
to
158
| if err = c.fs.MkdirAll(c.store.Path(), DefaultDirPerms); err != nil { | ||
| return wrapError("create_store_dir", c.store.Path(), ErrCreateDir) | ||
| return fmt.Errorf("create store dir %s: %w", c.store.Path(), ErrCreateDir) | ||
| } | ||
|
|
||
| // Ensure partition directory exists | ||
| partitionDir := c.getPartition(hash) | ||
| if err = c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil { | ||
| return wrapError("create_partition_dir", partitionDir, ErrCreateDir) | ||
| return fmt.Errorf("create partition dir %s: %w", partitionDir, ErrCreateDir) | ||
| } |
Contributor
There was a problem hiding this comment.
Same issue: ErrCreateDir replaces actual error.
Lines 151 and 157 have the same problem as the Store function—the actual err from MkdirAll is discarded.
🛠️ Proposed fix
if err = c.fs.MkdirAll(c.store.Path(), DefaultDirPerms); err != nil {
- return fmt.Errorf("create store dir %s: %w", c.store.Path(), ErrCreateDir)
+ return fmt.Errorf("create store dir %s: %w", c.store.Path(), err)
}
// Ensure partition directory exists
partitionDir := c.getPartition(hash)
if err = c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil {
- return fmt.Errorf("create partition dir %s: %w", partitionDir, ErrCreateDir)
+ return fmt.Errorf("create partition dir %s: %w", partitionDir, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err = c.fs.MkdirAll(c.store.Path(), DefaultDirPerms); err != nil { | |
| return wrapError("create_store_dir", c.store.Path(), ErrCreateDir) | |
| return fmt.Errorf("create store dir %s: %w", c.store.Path(), ErrCreateDir) | |
| } | |
| // Ensure partition directory exists | |
| partitionDir := c.getPartition(hash) | |
| if err = c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil { | |
| return wrapError("create_partition_dir", partitionDir, ErrCreateDir) | |
| return fmt.Errorf("create partition dir %s: %w", partitionDir, ErrCreateDir) | |
| } | |
| if err = c.fs.MkdirAll(c.store.Path(), DefaultDirPerms); err != nil { | |
| return fmt.Errorf("create store dir %s: %w", c.store.Path(), err) | |
| } | |
| // Ensure partition directory exists | |
| partitionDir := c.getPartition(hash) | |
| if err = c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil { | |
| return fmt.Errorf("create partition dir %s: %w", partitionDir, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cas/content.go` around lines 150 - 158, The error handling after
c.fs.MkdirAll is discarding the real mkdir error by returning ErrCreateDir
instead of wrapping the actual err; update both returns so they wrap the
original err (from c.fs.MkdirAll) using %w (e.g., in the store directory case
replace ErrCreateDir with err in fmt.Errorf("create store dir %s: %w",
c.store.Path(), err)) and do the same for the partition directory case
(fmt.Errorf("create partition dir %s: %w", partitionDir, err)) so the true
underlying mkdir error is preserved; locate these changes around c.fs.MkdirAll,
c.store.Path(), and c.getPartition.
Comment on lines
180
to
183
| partitionDir := c.getPartition(hash) | ||
| if err = c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil { | ||
| return wrapError("create_partition_dir", partitionDir, ErrCreateDir) | ||
| return fmt.Errorf("create partition dir %s: %w", partitionDir, ErrCreateDir) | ||
| } |
Contributor
There was a problem hiding this comment.
Same issue: line 182 wraps ErrCreateDir instead of err.
🛠️ Proposed fix
if err = c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil {
- return fmt.Errorf("create partition dir %s: %w", partitionDir, ErrCreateDir)
+ return fmt.Errorf("create partition dir %s: %w", partitionDir, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| partitionDir := c.getPartition(hash) | |
| if err = c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil { | |
| return wrapError("create_partition_dir", partitionDir, ErrCreateDir) | |
| return fmt.Errorf("create partition dir %s: %w", partitionDir, ErrCreateDir) | |
| } | |
| partitionDir := c.getPartition(hash) | |
| if err = c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil { | |
| return fmt.Errorf("create partition dir %s: %w", partitionDir, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cas/content.go` around lines 180 - 183, The MkdirAll error handling
in the partition creation uses the sentinel ErrCreateDir instead of the actual
returned error; update the block in the method that computes partitionDir (using
c.getPartition) so that the returned fmt.Errorf wraps the real err from
c.fs.MkdirAll (e.g., return fmt.Errorf("create partition dir %s: %w",
partitionDir, err)) while keeping the descriptive message and existing symbols
(partitionDir, c.getPartition, c.fs.MkdirAll, ErrCreateDir) for context.
Comment on lines
213
to
215
| if err := c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil { | ||
| return nil, wrapError("create_partition_dir", partitionDir, ErrCreateDir) | ||
| return nil, fmt.Errorf("create partition dir %s: %w", partitionDir, ErrCreateDir) | ||
| } |
Contributor
There was a problem hiding this comment.
Same issue: line 214 wraps ErrCreateDir instead of err.
Additionally, as flagged by SonarCloud, the literal "create partition dir %s: %w" appears 4 times. Consider extracting to a constant if keeping the sentinel pattern, or simply wrap err consistently.
🛠️ Proposed fix
if err := c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil {
- return nil, fmt.Errorf("create partition dir %s: %w", partitionDir, ErrCreateDir)
+ return nil, fmt.Errorf("create partition dir %s: %w", partitionDir, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil { | |
| return nil, wrapError("create_partition_dir", partitionDir, ErrCreateDir) | |
| return nil, fmt.Errorf("create partition dir %s: %w", partitionDir, ErrCreateDir) | |
| } | |
| if err := c.fs.MkdirAll(partitionDir, DefaultDirPerms); err != nil { | |
| return nil, fmt.Errorf("create partition dir %s: %w", partitionDir, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cas/content.go` around lines 213 - 215, The call to
c.fs.MkdirAll(partitionDir, DefaultDirPerms) incorrectly wraps the sentinel
ErrCreateDir instead of the actual error (err) and reuses the literal format
"create partition dir %s: %w" in multiple places; change the fmt.Errorf to wrap
the returned err (i.e., fmt.Errorf("create partition dir %s: %w", partitionDir,
err)) and extract the format string into a shared constant (e.g.,
createPartitionDirFmt) used by the same call sites to avoid duplication and
satisfy SonarCloud; update the code around c.fs.MkdirAll, partitionDir,
DefaultDirPerms and any repeated occurrences to use the new constant and wrap
err.
Collaborator
Author
|
I'm gonna rebase this PR on top of #6030 so that this PR and all the stuff that is chained off it uses the consolidated go-getter logic. |
yhakbar
force-pushed
the
feat/adding-central-git-store
branch
from
3a8acae to
b04a540
Compare
yhakbar
force-pushed
the
chore/consolidating-go-getter-routes
branch
from
766c4b6 to
505e915
Compare
yhakbar
force-pushed
the
feat/adding-central-git-store
branch
from
b04a540 to
1b39d0c
Compare
denis256
reviewed
May 12, 2026
yhakbar
force-pushed
the
feat/adding-central-git-store
branch
from
e680404 to
b94df21
Compare
denis256
approved these changes
May 12, 2026
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.
Description
Fixes #3976.
Relates to #5558
Leverages persisted shared bare Git repositories in the central shared directory used by the CAS store to prevent duplicitive work in fetching Git repositories for updates to the CAS.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
New Features
Documentation
Tests