Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/i18n/locales/en/common.json
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,9 @@
"image_copied_to_clipboard": "Image data URI copied to clipboard",
"image_saved": "Image saved to {{path}}",
"mode_exported": "Mode '{{mode}}' exported successfully",
"mode_imported": "Mode imported successfully"
"mode_imported": "Mode imported successfully",
"nested_git_repo_excluded": "Nested git repository at '{{path}}' will be excluded from checkpoints",
"nested_git_repos_excluded": "{{count}} nested git repositories will be excluded from checkpoints"
},
"answers": {
"yes": "Yes",
Expand Down
60 changes: 36 additions & 24 deletions src/services/checkpoints/ShadowCheckpointService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,20 +70,6 @@ export abstract class ShadowCheckpointService extends EventEmitter {
throw new Error("Shadow git repo already initialized")
}

const nestedGitPath = await this.getNestedGitRepository()

if (nestedGitPath) {
// Show persistent error message with the offending path
const relativePath = path.relative(this.workspaceDir, nestedGitPath)
const message = t("common:errors.nested_git_repos_warning", { path: relativePath })
vscode.window.showErrorMessage(message)

throw new Error(
`Checkpoints are disabled because a nested git repository was detected at: ${relativePath}. ` +
"Please remove or relocate nested git repositories to use the checkpoints feature.",
)
}

await fs.mkdir(this.checkpointsDir, { recursive: true })
const git = simpleGit(this.checkpointsDir)
const gitVersion = await git.version()
Expand All @@ -102,6 +88,7 @@ export abstract class ShadowCheckpointService extends EventEmitter {
)
}

// Write exclude file which will include nested git repos
await this.writeExcludeFile()
this.baseHash = await git.revparse(["HEAD"])
} else {
Expand All @@ -111,6 +98,7 @@ export abstract class ShadowCheckpointService extends EventEmitter {
await git.addConfig("commit.gpgSign", "false") // Disable commit signing for shadow repo.
await git.addConfig("user.name", "Roo Code")
await git.addConfig("user.email", "[email protected]")
// Write exclude file which will include nested git repos
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo/wording concern: The comment on line 101 says "Write exclude file which will include nested git repos", but based on the commit message, the intention is to exclude nested git repos. Please verify if "include" should be changed to "exclude" to maintain consistency.

Suggested change
// Write exclude file which will include nested git repos
// Write exclude file which will exclude nested git repos

await this.writeExcludeFile()
await this.stageAll(git)
const { commit } = await git.commit("initial commit", { "--allow-empty": null })
Expand Down Expand Up @@ -147,6 +135,16 @@ export abstract class ShadowCheckpointService extends EventEmitter {
protected async writeExcludeFile() {
await fs.mkdir(path.join(this.dotGitDir, "info"), { recursive: true })
const patterns = await getExcludePatterns(this.workspaceDir)

// Add nested git repositories to exclude patterns
const nestedGitPaths = await this.getNestedGitRepositories()
for (const gitPath of nestedGitPaths) {
const relativePath = path.relative(this.workspaceDir, gitPath)
// Add the directory and all its contents to exclude patterns
patterns.push(relativePath + "/")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Normalize to POSIX-style separators before writing to exclude to ensure Git matching across platforms.

Suggested change
patterns.push(relativePath + "/")
const relativePath = path
.relative(this.workspaceDir, gitPath)
.split(path.sep)
.join('/');
patterns.push(`${relativePath}/`)

this.log(`[${this.constructor.name}#writeExcludeFile] excluding nested git repo: ${relativePath}`)
}

await fs.writeFile(path.join(this.dotGitDir, "info", "exclude"), patterns.join("\n"))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: Add a trailing newline when writing .git/info/exclude for consistency with POSIX tools and Git defaults.

Suggested change
await fs.writeFile(path.join(this.dotGitDir, "info", "exclude"), patterns.join("\n"))
await fs.writeFile(path.join(this.dotGitDir, "info", "exclude"), patterns.join("\n") + "\n")

}

Expand All @@ -160,7 +158,7 @@ export abstract class ShadowCheckpointService extends EventEmitter {
}
}

private async getNestedGitRepository(): Promise<string | null> {
private async getNestedGitRepositories(): Promise<string[]> {
try {
// Find all .git/HEAD files that are not at the root level.
const args = ["--files", "--hidden", "--follow", "-g", "**/.git/HEAD", this.workspaceDir]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: This only finds nested repos with a .git directory containing HEAD; submodules/worktrees often have .git as a file pointing to a gitdir. Consider also detecting .git files and resolving their gitdir targets to exclude them.

Expand All @@ -182,32 +180,46 @@ export abstract class ShadowCheckpointService extends EventEmitter {
)
})

if (nestedGitPaths.length > 0) {
// Get the first nested git repository path
const repoDirs: string[] = []
for (const gitPath of nestedGitPaths) {
// Remove .git/HEAD from the path to get the repository directory
const headPath = nestedGitPaths[0].path
const headPath = gitPath.path

// Use path module to properly extract the repository directory
// The HEAD file is at .git/HEAD, so we need to go up two directories
const gitDir = path.dirname(headPath) // removes HEAD, gives us .git
const repoDir = path.dirname(gitDir) // removes .git, gives us the repo directory

const absolutePath = path.join(this.workspaceDir, repoDir)
repoDirs.push(absolutePath)
}

if (repoDirs.length > 0) {
this.log(
`[${this.constructor.name}#getNestedGitRepository] found ${nestedGitPaths.length} nested git repositories, first at: ${repoDir}`,
`[${this.constructor.name}#getNestedGitRepositories] found ${repoDirs.length} nested git repositories`,
)
return absolutePath

// Show informational message to user
if (repoDirs.length === 1) {
const relativePath = path.relative(this.workspaceDir, repoDirs[0])
vscode.window.showInformationMessage(
t("common:info.nested_git_repo_excluded", { path: relativePath }),
)
} else {
vscode.window.showInformationMessage(
t("common:info.nested_git_repos_excluded", { count: repoDirs.length }),
)
}
}

return null
return repoDirs
} catch (error) {
this.log(
`[${this.constructor.name}#getNestedGitRepository] failed to check for nested git repos: ${error instanceof Error ? error.message : String(error)}`,
`[${this.constructor.name}#getNestedGitRepositories] failed to check for nested git repos: ${error instanceof Error ? error.message : String(error)}`,
)

// If we can't check, assume there are no nested repos to avoid blocking the feature.
return null
// If we can't check, return empty array to avoid blocking the feature.
return []
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])(
})

describe(`${klass.name}#hasNestedGitRepositories`, () => {
it("throws error when nested git repositories are detected during initialization", async () => {
it("excludes nested git repositories from checkpoints instead of throwing error", async () => {
// Create a new temporary workspace and service for this test.
const shadowDir = path.join(tmpDir, `${prefix}-nested-git-${Date.now()}`)
const workspaceDir = path.join(tmpDir, `workspace-nested-git-${Date.now()}`)
Expand Down Expand Up @@ -437,11 +437,25 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])(

const service = new klass(taskId, shadowDir, workspaceDir, () => {})

// Verify that initialization throws an error when nested git repos are detected
// The error message now includes the specific path of the nested repository
await expect(service.initShadowGit()).rejects.toThrowError(
/Checkpoints are disabled because a nested git repository was detected at:/,
)
// Verify that initialization succeeds and excludes nested git repos
await expect(service.initShadowGit()).resolves.not.toThrow()
expect(service.isInitialized).toBe(true)

// Verify that the nested git repo is excluded by checking the exclude file
const excludesPath = path.join(service.checkpointsDir, ".git", "info", "exclude")
const excludeContent = await fs.readFile(excludesPath, "utf-8")
expect(excludeContent).toContain("nested-project/")

// Verify that changes in the nested repo are not tracked
await fs.writeFile(nestedFile, "Modified content in nested repo")
const checkpoint = await service.saveCheckpoint("Test checkpoint")
// Should not create a checkpoint since nested repo changes are excluded
expect(checkpoint?.commit).toBeFalsy()

// Verify that changes in the main repo are still tracked
await fs.writeFile(mainFile, "Modified content in main repo")
const mainCheckpoint = await service.saveCheckpoint("Main repo checkpoint")
expect(mainCheckpoint?.commit).toBeTruthy()

// Clean up.
vitest.restoreAllMocks()
Expand Down
Loading