forked from cline/cline
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Clarify checkpointing git phrasing #8392
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
Closed
jfcantu
wants to merge
6
commits into
RooCodeInc:main
from
jfcantu:clarify-checkpointing-git-phrasing
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
e072c22
Eliminate term "nested" Git repositories to reduce confusion, clarifi…
jfcantu 97e324a
Merge branch 'main' of https://github.com/jfcantu/Roo-Code into clari…
jfcantu ba30ca7
fix regex match for modified error message
jfcantu 439a0a2
Eliminate term "nested" Git repositories to reduce confusion, clarifi…
jfcantu 37bedd0
fix regex match for modified error message
jfcantu 3e1b9fc
Merge branch 'clarify-checkpointing-git-phrasing' of https://github.c…
jfcantu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,17 +70,17 @@ export abstract class ShadowCheckpointService extends EventEmitter { | |
| throw new Error("Shadow git repo already initialized") | ||
| } | ||
|
|
||
| const nestedGitPath = await this.getNestedGitRepository() | ||
| const childGitPath = await this.getChildGitRepository() | ||
|
|
||
| if (nestedGitPath) { | ||
| if (childGitPath) { | ||
| // 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 }) | ||
| const relativePath = path.relative(this.workspaceDir, childGitPath) | ||
| const message = t("common:errors.child_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.", | ||
| `Checkpoints are disabled because a Git repository was detected below the workspace root at: ${relativePath}. ` + | ||
| `To use checkpoints, please remove or relocate this git repository, or open a Git repository as the workspace root.`, | ||
| ) | ||
| } | ||
|
|
||
|
|
@@ -160,17 +160,17 @@ export abstract class ShadowCheckpointService extends EventEmitter { | |
| } | ||
| } | ||
|
|
||
| private async getNestedGitRepository(): Promise<string | null> { | ||
| private async getChildGitRepository(): Promise<string | null> { | ||
| try { | ||
| // Find all .git/HEAD files that are not at the root level. | ||
| const args = ["--files", "--hidden", "--follow", "-g", "**/.git/HEAD", this.workspaceDir] | ||
|
|
||
| const gitPaths = await executeRipgrep({ args, workspacePath: this.workspaceDir }) | ||
|
|
||
| // Filter to only include nested git directories (not the root .git). | ||
| // Filter to only include child git directories (not the root .git). | ||
| // Since we're searching for HEAD files, we expect type to be "file" | ||
| const nestedGitPaths = gitPaths.filter(({ type, path: filePath }) => { | ||
| // Check if it's a file and is a nested .git/HEAD (not at root) | ||
| const childGitPaths = gitPaths.filter(({ type, path: filePath }) => { | ||
| // Check if it's a file and is a child .git/HEAD (not at root) | ||
| if (type !== "file") return false | ||
|
|
||
| // Ensure it's a .git/HEAD file and not the root one | ||
|
|
@@ -182,10 +182,10 @@ export abstract class ShadowCheckpointService extends EventEmitter { | |
| ) | ||
| }) | ||
|
|
||
| if (nestedGitPaths.length > 0) { | ||
| // Get the first nested git repository path | ||
| if (childGitPaths.length > 0) { | ||
| // Get the first child git repository path | ||
| // Remove .git/HEAD from the path to get the repository directory | ||
| const headPath = nestedGitPaths[0].path | ||
| const headPath = childGitPaths[0].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 | ||
|
|
@@ -195,18 +195,18 @@ export abstract class ShadowCheckpointService extends EventEmitter { | |
| const absolutePath = path.join(this.workspaceDir, repoDir) | ||
|
|
||
| this.log( | ||
| `[${this.constructor.name}#getNestedGitRepository] found ${nestedGitPaths.length} nested git repositories, first at: ${repoDir}`, | ||
| `[${this.constructor.name}#getChildGitRepository] found ${childGitPaths.length} child git repositories, first at: ${repoDir}`, | ||
| ) | ||
| return absolutePath | ||
| } | ||
|
|
||
| return null | ||
| } 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}#getChildGitRepository] failed to check for child 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. | ||
| // If we can't check, assume there are no child repos to avoid blocking the feature. | ||
| return null | ||
| } | ||
| } | ||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -378,11 +378,11 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( | |
| }) | ||
| }) | ||
|
|
||
| describe(`${klass.name}#hasNestedGitRepositories`, () => { | ||
| it("throws error when nested git repositories are detected during initialization", async () => { | ||
| describe(`${klass.name}#hasChildGitRepositories`, () => { | ||
| it("throws error when child git repositories are detected during initialization", 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()}`) | ||
| const shadowDir = path.join(tmpDir, `${prefix}-child-git-${Date.now()}`) | ||
| const workspaceDir = path.join(tmpDir, `workspace-child-git-${Date.now()}`) | ||
|
|
||
| // Create a primary workspace repo. | ||
| await fs.mkdir(workspaceDir, { recursive: true }) | ||
|
|
@@ -391,38 +391,38 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( | |
| await mainGit.addConfig("user.name", "Roo Code") | ||
| await mainGit.addConfig("user.email", "[email protected]") | ||
|
|
||
| // Create a nested repo inside the workspace. | ||
| const nestedRepoPath = path.join(workspaceDir, "nested-project") | ||
| await fs.mkdir(nestedRepoPath, { recursive: true }) | ||
| const nestedGit = simpleGit(nestedRepoPath) | ||
| await nestedGit.init() | ||
| await nestedGit.addConfig("user.name", "Roo Code") | ||
| await nestedGit.addConfig("user.email", "[email protected]") | ||
| // Create a child repo inside the workspace. | ||
| const childRepoPath = path.join(workspaceDir, "nested-project") | ||
| await fs.mkdir(childRepoPath, { recursive: true }) | ||
| const childGit = simpleGit(childRepoPath) | ||
| await childGit.init() | ||
| await childGit.addConfig("user.name", "Roo Code") | ||
| await childGit.addConfig("user.email", "[email protected]") | ||
|
|
||
| // Add a file to the nested repo. | ||
| const nestedFile = path.join(nestedRepoPath, "nested-file.txt") | ||
| await fs.writeFile(nestedFile, "Content in nested repo") | ||
| await nestedGit.add(".") | ||
| await nestedGit.commit("Initial commit in nested repo") | ||
| // Add a file to the child repo. | ||
| const childFile = path.join(childRepoPath, "nested-file.txt") | ||
| await fs.writeFile(childFile, "Content in child repo") | ||
| await childGit.add(".") | ||
| await childGit.commit("Initial commit in child repo") | ||
|
|
||
| // Create a test file in the main workspace. | ||
| const mainFile = path.join(workspaceDir, "main-file.txt") | ||
| await fs.writeFile(mainFile, "Content in main repo") | ||
| await mainGit.add(".") | ||
| await mainGit.commit("Initial commit in main repo") | ||
|
|
||
| // Confirm nested git directory exists before initialization. | ||
| const nestedGitDir = path.join(nestedRepoPath, ".git") | ||
| const headFile = path.join(nestedGitDir, "HEAD") | ||
| // Confirm child git directory exists before initialization. | ||
| const childGitDir = path.join(childRepoPath, ".git") | ||
| const headFile = path.join(childGitDir, "HEAD") | ||
| await fs.writeFile(headFile, "HEAD") | ||
| expect(await fileExistsAtPath(nestedGitDir)).toBe(true) | ||
| expect(await fileExistsAtPath(childGitDir)).toBe(true) | ||
|
|
||
| vitest.spyOn(fileSearch, "executeRipgrep").mockImplementation(({ args }) => { | ||
| const searchPattern = args[4] | ||
|
|
||
| if (searchPattern.includes(".git/HEAD")) { | ||
| // Return the HEAD file path, not the .git directory | ||
| const headFilePath = path.join(path.relative(workspaceDir, nestedGitDir), "HEAD") | ||
| const headFilePath = path.join(path.relative(workspaceDir, childGitDir), "HEAD") | ||
| return Promise.resolve([ | ||
| { | ||
| path: headFilePath, | ||
|
|
@@ -437,10 +437,10 @@ 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 | ||
| // Verify that initialization throws an error when child git repos are detected | ||
| // The error message now includes the specific path of the child repository | ||
| await expect(service.initShadowGit()).rejects.toThrowError( | ||
| /Checkpoints are disabled because a nested git repository was detected at:/, | ||
| /Checkpoints are disabled because a Git repository was detected below the workspace root at: .*?\. To use checkpoints, please remove or relocate this git repository, or open a Git repository as the workspace root\./, | ||
| ) | ||
|
|
||
| // Clean up. | ||
|
|
@@ -449,12 +449,12 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( | |
| await fs.rm(workspaceDir, { recursive: true, force: true }) | ||
| }) | ||
|
|
||
| it("succeeds when no nested git repositories are detected", async () => { | ||
| it("succeeds when no child git repositories are detected", async () => { | ||
| // Create a new temporary workspace and service for this test. | ||
| const shadowDir = path.join(tmpDir, `${prefix}-no-nested-git-${Date.now()}`) | ||
| const workspaceDir = path.join(tmpDir, `workspace-no-nested-git-${Date.now()}`) | ||
| const shadowDir = path.join(tmpDir, `${prefix}-no-child-git-${Date.now()}`) | ||
| const workspaceDir = path.join(tmpDir, `workspace-no-child-git-${Date.now()}`) | ||
|
|
||
| // Create a primary workspace repo without any nested repos. | ||
| // Create a primary workspace repo without any child repos. | ||
| await fs.mkdir(workspaceDir, { recursive: true }) | ||
| const mainGit = simpleGit(workspaceDir) | ||
| await mainGit.init() | ||
|
|
@@ -468,13 +468,13 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( | |
| await mainGit.commit("Initial commit in main repo") | ||
|
|
||
| vitest.spyOn(fileSearch, "executeRipgrep").mockImplementation(() => { | ||
| // Return empty array to simulate no nested git repos found | ||
| // Return empty array to simulate no child git repos found | ||
| return Promise.resolve([]) | ||
| }) | ||
|
|
||
| const service = new klass(taskId, shadowDir, workspaceDir, () => {}) | ||
|
|
||
| // Verify that initialization succeeds when no nested git repos are detected | ||
| // Verify that initialization succeeds when no child git repos are detected | ||
| await expect(service.initShadowGit()).resolves.not.toThrow() | ||
| expect(service.isInitialized).toBe(true) | ||
|
|
||
|
|
||
Oops, something went wrong.
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.
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.
Typographical consistency: The string uses "Git repository" earlier but refers to it as "git repository" later on this line. Consider using a consistent capitalization (e.g., "Git repository") throughout.