-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: make nested git repository warning persistent with path info #7885
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
Conversation
- Changed from showWarningMessage to showErrorMessage for persistence - Modified getNestedGitRepository to return the specific path - Updated all i18n translations to include path parameter - Updated tests to match new error message format Fixes #7884
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.
Reviewing my own code is like looking in a mirror and realizing the mirror was right all along.
|
|
||
| if (nestedGitPaths.length > 0) { | ||
| // Get the first nested git repository path (remove .git/HEAD from the path) | ||
| const firstNestedPath = nestedGitPaths[0].path.replace(/[\/\\]\.git[\/\\]HEAD$/, "") |
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.
Consider adding a comment here explaining why we're stripping the .git/HEAD suffix from the path. Something like:
This would make the regex replacement purpose clearer for future maintainers.
| "Checkpoints are disabled because nested git repositories were detected in the workspace", | ||
| // 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:/, |
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.
Would it be helpful to add a more specific test that verifies the exact path is included in the error message? The current regex test confirms the format but doesn't verify the actual path. We could capture and assert the specific path like:
Checkpoints are disabled because a nested git repository was detected at: nested-project
- Fix path filtering to correctly expect 'file' type for HEAD files instead of 'folder' - Use Node's path module for robust cross-platform path manipulation - Update tests to match the corrected logic
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.
LGTM
Fixes #7884
Summary
Enhanced the nested Git repository warning for checkpoints to be persistent and include the specific offending path inline.
Changes
showWarningMessagetoshowErrorMessagefor persistent notificationshasNestedGitRepositoriesmethod togetNestedGitRepositoryto return the specific pathTesting
Screenshots
The warning now shows as an error notification (persistent) with the specific path:
"Checkpoints are disabled because a nested git repository was detected at: [path]. To use checkpoints, please remove or relocate this nested git repository."
Important
Enhances nested Git repository warning to be persistent and include specific path, updates translations and tests accordingly.
showWarningMessagetoshowErrorMessageinShadowCheckpointService.tsfor persistent notifications.getNestedGitRepository()to return the specific path of the nested repository.ShadowCheckpointService.spec.tsto match the new error message format.This description was created by
for 014858f. You can customize this summary. It will automatically update as commits are pushed.