scriptoriumadmin/Roo-Code
1
0
Fork 0
mirror of https://github.com/RooVetGit/Roo-Code.git synced 2026-09-07 08:26:51 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

fix: address review feedback on nested git repo handling

Browse source 
Improvements based on reviewer bot feedback:

1. Enhanced safety check (HIGH priority):
   - Now uses 'git ls-files -s --cached' to detect mode 160000 entries
   - Catches newly added/removed gitlinks, not just pointer changes
   - Added check for .gitmodules changes with warning log

2. Improved pathspec exclusion (MEDIUM priority):
   - Added 'git rm --cached' to remove existing gitlinks before staging
   - Prevents staging of gitlink entries already in the index
   - Ensures complete exclusion of nested repo changes

3. Better error logging (LOW priority):
   - Added warning logs for git command failures in findNestedRepos()
   - Helps debugging while avoiding feature breakage
   - Distinguishes expected errors (no .gitmodules) from real issues

All tests continue to pass with these enhancements.
This commit is contained in:
daniel-lxs 2025-10-27 12:19:13 -05:00
parent b02bf485a4
commit 0d089b193c
No known key found for this signature in database
GPG key ID: 21C74479048B3AA6
1 changed files with 44 additions and 10 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

54
src/services/checkpoints/ShadowCheckpointService.ts
View file

@ -146,6 +146,17 @@ export abstract class ShadowCheckpointService extends EventEmitter {
)
}
// Remove any existing gitlinks from the index before staging
for (const repoPath of nestedRepos) {
try {
await git.raw(["rm", "--cached", "--ignore-unmatch", "-r", repoPath])
} catch (error) {
this.log(
`[${this.constructor.name}#stageAll] failed to remove cached gitlink ${repoPath}: ${error instanceof Error ? error.message : String(error)}`,
)
}
}
// Build add command with pathspec excludes
const addArgs: string[] = ["-A", ":/"]
for (const repoPath of nestedRepos) {
@ -155,10 +166,24 @@ export abstract class ShadowCheckpointService extends EventEmitter {
// Stage files
await git.add(addArgs)
// Safety check: ensure no submodule changes staged
const diffSummary = await git.raw(["diff", "--cached", "--summary"])
if (diffSummary.includes("Submodule")) {
throw new Error("Submodule changes detected in staging area - this should not happen")
// Enhanced safety check: verify no mode 160000 entries in staging area
const stagedFiles = await git.raw(["ls-files", "-s", "--cached"])
const gitlinkEntries = stagedFiles
.split("\n")
.filter((line) => line.startsWith("160000"))
.map((line) => line.split(/\s+/)[3])
.filter(Boolean)
if (gitlinkEntries.length > 0) {
throw new Error(
`Gitlink entries detected in staging area: ${gitlinkEntries.join(", ")} - this should not happen`,
)
}
// Additional check for .gitmodules changes
const diffSummary = await git.raw(["diff", "--cached", "--name-only"])
if (diffSummary.includes(".gitmodules")) {
this.log(`[${this.constructor.name}#stageAll] warning: .gitmodules changes detected in staging area`)
}
} catch (error) {
this.log(
@ -178,8 +203,13 @@ export abstract class ShadowCheckpointService extends EventEmitter {
const match = line.match(/submodule\..*\.path\s+(.+)/)
if (match) nestedRepos.add(match[1])
}
} catch {
// No .gitmodules file or error reading it
} catch (error) {
// No .gitmodules file is expected in most cases, only log if it's a real error
if (error instanceof Error && !error.message.includes("exit code 1")) {
this.log(
`[${this.constructor.name}#findNestedRepos] warning: failed to read .gitmodules: ${error.message}`,
)
}
}
// 2. From index (gitlinks with mode 160000)
@ -191,8 +221,10 @@ export abstract class ShadowCheckpointService extends EventEmitter {
if (parts[3]) nestedRepos.add(parts[3])
}
}
} catch {
// Ignore errors
} catch (error) {
this.log(
`[${this.constructor.name}#findNestedRepos] warning: failed to list files from index: ${error instanceof Error ? error.message : String(error)}`,
)
}
// 3. From filesystem (any nested .git directory or worktree)
@ -244,8 +276,10 @@ export abstract class ShadowCheckpointService extends EventEmitter {
const repoDir = path.dirname(result.path)
nestedRepos.add(repoDir)
}
} catch {
// Ignore errors reading .git file
} catch (error) {
this.log(
`[${this.constructor.name}#findNestedRepos] warning: failed to read .git file at ${result.path}: ${error instanceof Error ? error.message : String(error)}`,
)
}
}
}