Refactor getOrCreateListRepoClone into focused helpers in parser remote file listing#44179
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
getOrCreateListRepoClone into focused helpers in parser remote file listing
There was a problem hiding this comment.
Pull request overview
This PR refactors the git-clone fallback path used by the remote file listing parser by splitting the previously-large getOrCreateListRepoClone function into smaller helpers, while keeping the orchestration and behavior the same.
Changes:
- Extracted clone configuration derivation (ref normalization, host override handling, repo URL, cache key) into
resolveListRepoCloneConfigwith alistRepoCloneConfigstruct. - Extracted clone-cache read/validation into
getCachedListRepoClone. - Extracted the clone + cache write path into
cloneAndCacheListRepoClone, leavinggetOrCreateListRepoCloneas the coordinating function (including the singleflight + double cache-check flow).
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/remote_list_files.go | Refactors the list-repo clone caching and singleflight orchestration into focused helper functions to stay under the largefunc threshold while preserving behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Low
|
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. This PR only refactors production code in pkg/parser/remote_list_files.go (57 lines added, 43 deleted). Test Quality Sentinel analysis skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44179 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (57 additions detected). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — no blocking issues, but three suggestions worth addressing.
📋 Key Themes & Highlights
Key Themes
- Struct completeness:
listRepoCloneConfigomitsowner/repo, forcingcloneAndCacheListRepoCloneto take a mixed 6-arg signature instead of a clean(ctx, cfg). Extending the struct would complete the refactor. - Implicit locking contract:
getCachedListRepoCloneandcloneAndCacheListRepoCloneboth acquiregitListCloneCache.mu. This coupling is undocumented and could cause a deadlock if the helpers are re-composed in the future. - No new unit tests:
resolveListRepoCloneConfignow has testable boundaries (empty ref, host override, cache key format) but no tests were added. The helpers are straightforward to test in isolation.
Positive Highlights
- ✅ Singleflight double-checked locking pattern preserved correctly across the extraction
- ✅
deferadded to thegitListCloneCache.mu.Unlock()incloneAndCacheListRepoClone(was manualUnlock()in original) - ✅ Clean separation: config resolution, cache read, clone+write, and orchestration are now distinct concerns
- ✅ Orchestrator
getOrCreateListRepoClonereads exactly like the PR description's code block — the refactor matches its stated intent
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 61 AIC · ⌖ 5.77 AIC · ⊞ 6.6K
Comment /matt to run again
| }(); found { | ||
| return cloneDir, nil | ||
| } | ||
| func cloneAndCacheListRepoClone(ctx context.Context, owner, repo, ref, repoURL, cacheKey string) (string, error) { |
There was a problem hiding this comment.
[/codebase-design] cloneAndCacheListRepoClone takes owner/repo separately alongside config-derived ref/repoURL/cacheKey, creating a mixed-extraction call site at line 110 (ctx, owner, repo, config.ref, config.repoURL, config.cacheKey). Adding owner and repo to listRepoCloneConfig would complete the struct as a full clone-identity value and reduce this signature to (ctx, cfg).
💡 Suggested refactor
Extend the struct:
type listRepoCloneConfig struct {
owner string
repo string
ref string
repoURL string
cacheKey string
}Simplify the function:
func cloneAndCacheListRepoClone(ctx context.Context, cfg listRepoCloneConfig) (string, error) {
// use cfg.owner, cfg.repo, cfg.ref, cfg.repoURL, cfg.cacheKey
}
// call site becomes:
return cloneAndCacheListRepoClone(ctx, config)This makes listRepoCloneConfig a complete identity type and avoids unpacking the struct at the call site.
@copilot please address this.
There was a problem hiding this comment.
Done in 410fe4a. owner and repo are now fields on listRepoCloneConfig; cloneAndCacheListRepoClone takes (ctx context.Context, cfg listRepoCloneConfig) and uses cfg.* throughout.
| return cloneDir, true | ||
| } | ||
| delete(gitListCloneCache.dirs, cacheKey) | ||
| func getCachedListRepoClone(cacheKey string) (string, bool) { |
There was a problem hiding this comment.
[/tdd] The new getCachedListRepoClone and resolveListRepoCloneConfig helpers are now package-private, pure-ish functions that are straightforwardly unit-testable in isolation — but no tests cover them. At minimum, resolveListRepoCloneConfig has edge-case logic (empty ref, host override) that benefits from table-driven tests.
💡 Suggested test skeleton
func TestResolveListRepoCloneConfig(t *testing.T) {
tests := []struct {
name string
owner, repo, ref, host string
wantErr bool
checkFn func(t *testing.T, cfg listRepoCloneConfig)
}{
{name: "empty ref returns error", ref: " ", wantErr: true},
{name: "host override used in repoURL", owner: "o", repo: "r", ref: "main", host: "(ghes.example.com/redacted)", checkFn: func(t *testing.T, cfg listRepoCloneConfig) {
if !strings.Contains(cfg.repoURL, "ghes.example.com") {
t.Errorf("expected ghes host in repoURL, got %s", cfg.repoURL)
}
}},
{name: "cache key contains all identity fields", owner: "o", repo: "r", ref: "main", checkFn: func(t *testing.T, cfg listRepoCloneConfig) {
for _, part := range []string{"o", "r", "main"} {
if !strings.Contains(cfg.cacheKey, part) {
t.Errorf("missing %q in cacheKey %q", part, cfg.cacheKey)
}
}
}},
}
// ...
}@copilot please address this.
There was a problem hiding this comment.
Done in 410fe4a. Added TestResolveListRepoCloneConfig in remote_list_files_test.go with table-driven cases covering: empty ref error, whitespace-only ref error, host override in repoURL/cacheKey, cache key contains all identity fields, owner/repo fields on config, ref trimming, and public-org host selection.
There was a problem hiding this comment.
The refactoring cleanly decomposes getOrCreateListRepoClone into focused helpers while preserving exact behavior: singleflight double-check, cache validation with stale-entry eviction, and clone+cache write are all intact. listRepoCloneConfig correctly omits host since it is fully resolved into repoURL and cacheKey. Switching from explicit Lock/Unlock to defer Unlock in cloneAndCacheListRepoClone is equivalent and cleaner. No correctness, security, or behavioral issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 55.5 AIC · ⌖ 5.71 AIC · ⊞ 4.8K
There was a problem hiding this comment.
COMMENT — Two medium-severity issues found; both are non-blocking for a short-lived CLI process but become real risks if the code is reused or extended.
🔍 Findings
1. getCachedListRepoClone: mutex held during os.Stat I/O (medium)
The global cache mutex is held across the filesystem stat check, serializing all concurrent cache lookups behind a potential I/O stall. This was true in the original IIFE, but extracting it into a named helper crystallizes the pattern and makes it more likely to be copied or called in new contexts without realizing the contention risk.
2. cloneAndCacheListRepoClone: undocumented singleflight precondition + tmdir leak on concurrent misuse (medium)
The function unconditionally overwrites the cache entry with its freshly cloned tmpDir, relying entirely on the singleflight guard in its sole caller to prevent concurrent duplicates. This is fine today, but the exported contract is not documented. A direct call — from a test harness, a future code path, or even a refactor — could cause a tmpDir to be silently orphaned, since the success path has no equivalent of the failure-path os.RemoveAll.
The refactoring goal (splitting an oversized function) is achieved. The structure is cleaner and the behavior is preserved. The issues above are pre-existing in spirit but made slightly more hazardous by extraction.
🔎 Code quality review by PR Code Quality Reviewer · 178.2 AIC · ⌖ 6.1 AIC · ⊞ 5.4K
Comment /review to run again
| defer gitListCloneCache.mu.Unlock() | ||
|
|
||
| if cloneDir, ok := gitListCloneCache.dirs[cacheKey]; ok { | ||
| if stat, err := os.Stat(filepath.Join(cloneDir, ".git")); err == nil && stat.IsDir() { |
There was a problem hiding this comment.
Filesystem I/O () is called while holding the global cache mutex, serializing all concurrent cache lookups.
💡 Details and suggested fix
In getCachedListRepoClone, the mutex is held for the entire duration of the function including the os.Stat call on line 66. In the original IIFE this was identical, but extracting it into a standalone helper now silently bakes this behavior into every future caller.
Under contention (multiple goroutines needing a cache-miss check simultaneously), every goroutine must wait in line behind the goroutine doing the os.Stat syscall. If the cloneDir is on a slow or remote filesystem, this serializes all callers.
Suggested fix — copy the path while locked, then stat without the lock:
func getCachedListRepoClone(cacheKey string) (string, bool) {
gitListCloneCache.mu.Lock()
cloneDir, ok := gitListCloneCache.dirs[cacheKey]
gitListCloneCache.mu.Unlock()
if !ok {
return "", false
}
if stat, err := os.Stat(filepath.Join(cloneDir, ".git")); err == nil && stat.IsDir() {
return cloneDir, true
}
// Entry is stale — evict under lock.
gitListCloneCache.mu.Lock()
delete(gitListCloneCache.dirs, cacheKey)
gitListCloneCache.mu.Unlock()
return "", false
}Note: there is a benign TOCTOU window between the stat and the delete — a concurrent goroutine could re-add a fresh entry between those two steps — but singleflight ensures only one goroutine will actually clone, so the stale-eviction path is already racy in the original code.
There was a problem hiding this comment.
Fixed in 23c1c2e. getCachedListRepoClone now reads the cached path under a brief lock acquisition, releases the lock before os.Stat, and re-acquires only for the stale-entry eviction — matching the suggested pattern exactly.
| } | ||
| gitListCloneCache.mu.Lock() | ||
| defer gitListCloneCache.mu.Unlock() | ||
| gitListCloneCache.dirs[cacheKey] = tmpDir |
There was a problem hiding this comment.
cloneAndCacheListRepoClone silently assumes it will only ever be called inside a singleflight.Do guard, but has no documentation or enforcement of that precondition — if called directly, a concurrent duplicate invocation would orphan a tmpDir with no cleanup.
💡 Details and suggested fix
The function creates a tmpDir, clones into it, then unconditionally writes gitListCloneCache.dirs[cacheKey] = tmpDir (line 92). It does not re-check whether another caller has already inserted a fresh entry. The singleflight in getOrCreateListRepoClone serializes callers under the same key today — but this is an implicit contract that the extracted function does not document.
If cloneAndCacheListRepoClone is ever called directly (e.g. in a test or a second call-site added later), two goroutines with the same cacheKey would each clone successfully, the last writer would win, and the first goroutine's tmpDir leaks for the process lifetime. The cleanup on clone failure (lines 83-85) does not handle the success path.
Suggested fix — add a defensive cache check before writing:
gitListCloneCache.mu.Lock()
defer gitListCloneCache.mu.Unlock()
if existing, ok := gitListCloneCache.dirs[cacheKey]; ok {
// Another goroutine finished first; discard our redundant clone.
_ = os.RemoveAll(tmpDir)
return existing, nil
}
gitListCloneCache.dirs[cacheKey] = tmpDir
return tmpDir, nilAt minimum, add a doc-comment stating the singleflight precondition so callers cannot accidentally bypass it.
There was a problem hiding this comment.
Fixed in 23c1c2e. cloneAndCacheListRepoClone now performs a defensive check under the cache lock before writing: if another goroutine already populated the entry, the redundant tmpDir is cleaned up via os.RemoveAll and the existing entry is returned. The doc comment also explicitly states the singleflight precondition.
|
@copilot run pr-finisher skill |
|
@copilot please run the
|
…ndCacheListRepoClone to (ctx, cfg), add locking docs, add unit tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…nsive cache check in cloneAndCacheListRepoClone Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in 23c1c2e:
|
…ll errors on redundant clone cleanup Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the
|
All previously flagged review feedback has been addressed. Here's the current state:
Actions taken in previous iterations:
Hand-off: A maintainer needs to approve and re-trigger CI (close/reopen PR or |
🤖 PR Triage
Rationale: Splits over-limit Run §28924016278
|
|
@copilot lint go https://github.com/github/gh-aw/actions/runs/28920575862/job/85809599162 |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…mutexunlock linter Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. The |
|
🎉 This pull request is included in a new release. Release: |
pkg/parser/remote_list_files.gohad a single parser function (getOrCreateListRepoClone) over the customlargefunclimit. This change keeps behavior intact while splitting that path into smaller, composable helpers.Refactor scope (file-local, parser-only)
pkg/parser/remote_list_files.go.Helper extraction
listRepoCloneConfig+resolveListRepoCloneConfig(...)for ref/host/repo URL/cache key derivation and ref validation.getCachedListRepoClone(...).cloneAndCacheListRepoClone(...).Orchestration flow remains the same
getOrCreateListRepoClone(...)now coordinates: