-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: allow Git repositories in non-Git workspaces for checkpointing #8166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -162,6 +162,20 @@ export abstract class ShadowCheckpointService extends EventEmitter { | |
|
||
private async getNestedGitRepository(): Promise<string | null> { | ||
try { | ||
// First check if the workspace root itself is a Git repository | ||
const workspaceIsGitRepo = await fileExistsAtPath(path.join(this.workspaceDir, ".git")) | ||
|
||
// If the workspace root is not a Git repository, then any Git repositories | ||
// in subdirectories are not "nested" - they're just regular repositories | ||
// that happen to be in the workspace. This is a valid use case. | ||
if (!workspaceIsGitRepo) { | ||
this.log( | ||
`[${this.constructor.name}#getNestedGitRepository] workspace is not a git repository, allowing child repositories`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor suggestion: Could this log message be more descriptive? Something like:
This would make it clearer that we're intentionally skipping the check rather than just "allowing" repositories. |
||
) | ||
return null | ||
} | ||
|
||
// The workspace IS a Git repository, so now we need to check for nested repos | ||
// Find all .git/HEAD files that are not at the root level. | ||
const args = ["--files", "--hidden", "--follow", "-g", "**/.git/HEAD", this.workspaceDir] | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ import { EventEmitter } from "events" | |
|
||
import { simpleGit, SimpleGit } from "simple-git" | ||
|
||
import { fileExistsAtPath } from "../../../utils/fs" | ||
import * as fsUtils from "../../../utils/fs" | ||
import * as fileSearch from "../../../services/search/file-search" | ||
|
||
import { RepoPerTaskCheckpointService } from "../RepoPerTaskCheckpointService" | ||
|
@@ -415,7 +415,7 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( | |
const nestedGitDir = path.join(nestedRepoPath, ".git") | ||
const headFile = path.join(nestedGitDir, "HEAD") | ||
await fs.writeFile(headFile, "HEAD") | ||
expect(await fileExistsAtPath(nestedGitDir)).toBe(true) | ||
expect(await fsUtils.fileExistsAtPath(nestedGitDir)).toBe(true) | ||
|
||
vitest.spyOn(fileSearch, "executeRipgrep").mockImplementation(({ args }) => { | ||
const searchPattern = args[4] | ||
|
@@ -483,6 +483,78 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( | |
await fs.rm(shadowDir, { recursive: true, force: true }) | ||
await fs.rm(workspaceDir, { recursive: true, force: true }) | ||
}) | ||
|
||
it("allows git repositories in non-git workspace (issue #8164)", async () => { | ||
// This test addresses the specific issue where a workspace that is NOT a git repository | ||
// contains cloned git repositories as subdirectories. This should be allowed. | ||
|
||
const shadowDir = path.join(tmpDir, `${prefix}-non-git-workspace-${Date.now()}`) | ||
const workspaceDir = path.join(tmpDir, `workspace-non-git-${Date.now()}`) | ||
|
||
// Create a workspace directory WITHOUT initializing it as a git repo | ||
await fs.mkdir(workspaceDir, { recursive: true }) | ||
|
||
// Create a cloned repository inside the workspace (simulating the user's scenario) | ||
const clonedRepoPath = path.join(workspaceDir, "cloned-repository") | ||
await fs.mkdir(clonedRepoPath, { recursive: true }) | ||
const clonedGit = simpleGit(clonedRepoPath) | ||
await clonedGit.init() | ||
await clonedGit.addConfig("user.name", "Roo Code") | ||
await clonedGit.addConfig("user.email", "[email protected]") | ||
|
||
// Add a file to the cloned repo | ||
const clonedFile = path.join(clonedRepoPath, "cloned-file.txt") | ||
await fs.writeFile(clonedFile, "Content in cloned repo") | ||
await clonedGit.add(".") | ||
await clonedGit.commit("Initial commit in cloned repo") | ||
|
||
// Create a regular file in the workspace root | ||
const workspaceFile = path.join(workspaceDir, "workspace-file.txt") | ||
await fs.writeFile(workspaceFile, "Content in workspace") | ||
|
||
// Mock executeRipgrep to return the cloned repo's .git/HEAD | ||
vitest.spyOn(fileSearch, "executeRipgrep").mockImplementation(({ args }) => { | ||
const searchPattern = args[4] | ||
|
||
if (searchPattern.includes(".git/HEAD")) { | ||
// Return the HEAD file path for the cloned repository | ||
const headFilePath = path.join(path.relative(workspaceDir, clonedRepoPath), ".git", "HEAD") | ||
return Promise.resolve([ | ||
{ | ||
path: headFilePath, | ||
type: "file", | ||
label: "HEAD", | ||
}, | ||
]) | ||
} else { | ||
return Promise.resolve([]) | ||
} | ||
}) | ||
|
||
// Mock fileExistsAtPath to return false for workspace/.git (workspace is not a git repo) | ||
vitest.spyOn(fsUtils, "fileExistsAtPath").mockImplementation((filePath: string) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The mock implementation is clever, but could we make it more explicit about what we're testing? Consider adding a comment explaining why we're only mocking the workspace/.git path and using real implementation for others to maintain test integrity. |
||
if (filePath === path.join(workspaceDir, ".git")) { | ||
return Promise.resolve(false) // Workspace is NOT a git repo | ||
} | ||
// For other paths, use the real implementation | ||
return fs | ||
.access(filePath) | ||
.then(() => true) | ||
.catch(() => false) | ||
}) | ||
|
||
const service = new klass(taskId, shadowDir, workspaceDir, () => {}) | ||
|
||
// This should NOT throw an error because the workspace is not a git repository, | ||
// so the cloned repository is not considered "nested" | ||
await expect(service.initShadowGit()).resolves.not.toThrow() | ||
expect(service.isInitialized).toBe(true) | ||
|
||
// Clean up | ||
vitest.restoreAllMocks() | ||
await fs.rm(shadowDir, { recursive: true, force: true }) | ||
await fs.rm(workspaceDir, { recursive: true, force: true }) | ||
}) | ||
}) | ||
|
||
describe(`${klass.name}#events`, () => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this check robust enough? The
.git
path could also be a file (in the case of Git worktrees or submodules pointing to.git/worktrees/...
). Should we consider checking if it's actually a directory?Or perhaps we could use the simpleGit library to check if it's a valid repo?