feat: worktree 支持设置全局目录 #15
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for configuring a global directory for git worktrees, addressing the issue where worktrees were always created as subdirectories of projects, leading to cluttered global search results.
Changes:
- Added global worktree configuration with a customizable base directory and directory naming pattern
- Implemented security checks to prevent worktree creation in sensitive system directories
- Updated the UI to allow users to choose between project-level and global worktree locations
- Added database schema changes to persist worktree base paths per project
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/path.go | New file implementing security checks for sensitive system directories |
| utils/app_config.go | Added WorktreeConfig struct and atomic configuration update mechanism with mutex protection |
| service/worktree_service.go | Extended worktree service to support global directory creation with pattern expansion and path sanitization |
| service/worktree_service_test.go | Added tests for worktree persistence and security validation |
| service/branch_service.go | Updated to use new CreateWorktreeOptions struct |
| model/queries/project.sql | Added ProjectUpdateWorktreeBasePath query |
| model/project.sql_gen.go | Generated code for new database query |
| model/db_gen.go | Updated query initialization and cleanup |
| api/worktree.go | Extended API to accept location and global base directory override parameters |
| api/system.go | Added endpoints for worktree settings management with atomic config updates |
| api/api.go | Updated worktree route registration to pass config |
| ui/src/views/GeneralSettings.vue | Added UI for global worktree configuration |
| ui/src/types/models.ts | Added WorktreeConfig TypeScript interface |
| ui/src/stores/project.ts | Updated store to handle new worktree creation parameters |
| ui/src/i18n/locales/zh-CN.ts | Added Chinese translations for new features |
| ui/src/i18n/locales/en-US.ts | Added English translations for new features |
| ui/src/components/worktree/WorktreeCreateDialog.vue | Enhanced dialog to support location selection and global directory override |
| ui/src/api/project.ts | Updated API client with new parameters |
| ui/components.d.ts | Auto-generated component type declarations |
| static/index.html | Auto-generated build artifact with updated asset hashes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func sanitizePathSegment(input string) string { | ||
| trimmed := strings.TrimSpace(input) | ||
| replacer := strings.NewReplacer( | ||
| "/", "_", | ||
| "\\", "_", | ||
| ":", "_", | ||
| "*", "_", | ||
| "?", "_", | ||
| "<", "_", | ||
| ">", "_", | ||
| "|", "_", | ||
| ) | ||
| return replacer.Replace(trimmed) | ||
| } |
There was a problem hiding this comment.
The sanitizePathSegment function does not check for dangerous directory names like "." or ".." before sanitization. If the input is "." or "..", it will be sanitized to "." or ".." respectively (since these don't contain any special characters), which could still cause path traversal issues. Add checks similar to those in sanitizeBranchName to reject these dangerous values.
| function isGlobalWorktreeBasePath(projectPath: string, worktreeBasePath: string) { | ||
| if (!looksLikeAbsPath(worktreeBasePath)) { | ||
| return false; | ||
| } | ||
| const projectNorm = normalizePath(projectPath); | ||
| const baseNorm = normalizePath(worktreeBasePath); | ||
| return !baseNorm.startsWith(projectNorm + '/'); | ||
| } |
There was a problem hiding this comment.
The isGlobalWorktreeBasePath function uses a simple string prefix check to determine if a path is inside the project directory. This approach is case-sensitive on Windows but the check doesn't account for this. Windows paths are case-insensitive, so "C:/Project" and "c:/project" should be considered the same. Convert both paths to lowercase on Windows before comparison to ensure correct behavior.
| func sanitizePathSegment(input string) string { | ||
| trimmed := strings.TrimSpace(input) | ||
| replacer := strings.NewReplacer( | ||
| "/", "_", | ||
| "\\", "_", | ||
| ":", "_", | ||
| "*", "_", | ||
| "?", "_", | ||
| "<", "_", | ||
| ">", "_", | ||
| "|", "_", | ||
| ) | ||
| return replacer.Replace(trimmed) | ||
| } |
There was a problem hiding this comment.
The sanitizePathSegment function is not covered by any test cases. Given that this function is critical for security (it's used in pattern expansion for worktree directory names), add comprehensive test coverage that includes edge cases like ".", "..", empty strings, and strings with special characters to ensure it properly handles all potentially dangerous inputs.
| </n-form-item> | ||
| <n-form-item :label="t('settings.worktreeGlobalDirNamePattern')"> | ||
| <n-space vertical size="small" style="width: 100%"> | ||
| <n-input v-model:value="worktreeSettingsForm.globalDirNamePattern" /> |
There was a problem hiding this comment.
The globalDirNamePattern input in the settings UI has no client-side validation. Consider adding validation to check for invalid patterns (e.g., patterns containing path separators "/" or "\", or patterns that would result in dangerous directory names like "." or "..") before sending to the server. This provides better user experience with immediate feedback.
| <n-input v-model:value="worktreeSettingsForm.globalDirNamePattern" /> | |
| <n-input | |
| v-model:value="worktreeSettingsForm.globalDirNamePattern" | |
| :input-props="{ | |
| pattern: '^(?!\\.{1,2}$)(?!.*[\\\\/]).*$', | |
| title: t('settings.worktreeGlobalDirNamePatternTip') | |
| }" | |
| /> |
| } | ||
| if pattern == "" { | ||
| return nil, huma.Error400BadRequest("globalDirNamePattern is required") | ||
| } |
There was a problem hiding this comment.
The API endpoint for updating worktree settings validates that the pattern is not empty, but doesn't validate the actual content of the pattern. Consider adding validation to check for invalid patterns (e.g., patterns containing path separators "/" or "\") to provide clearer error messages to users before attempting to use the pattern.
| } | |
| } | |
| // Disallow path separators in the directory name pattern to avoid unintended paths. | |
| if strings.ContainsAny(pattern, "/\\") { | |
| return nil, huma.Error400BadRequest("globalDirNamePattern cannot contain path separators ('/' or '\\')") | |
| } |
| "C:\\Windows", "C:\\Program Files", "C:\\Program Files (x86)", | ||
| "C:\\ProgramData", "C:\\System Volume Information", |
There was a problem hiding this comment.
The Windows system directories are hard-coded with the C: drive. This will not catch sensitive directories on other drives (e.g., D:\Windows, E:\Program Files). Consider making the check more flexible by checking the directory names without the drive letter, or by iterating through all available drives.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| func UpdateConfig(config *AppConfig, modifier func(*AppConfig)) error { | ||
| configMu.Lock() | ||
| defer configMu.Unlock() | ||
|
|
||
| // 在锁内应用修改 | ||
| modifier(config) | ||
|
|
||
| // 持久化到磁盘 | ||
| if activeConfigPath == "" { | ||
| dataDir := GetDataDir() | ||
| activeConfigPath = fmt.Sprintf("%s/config.yaml", dataDir) | ||
| } | ||
| return writeConfigToPathLocked(config, activeConfigPath) |
There was a problem hiding this comment.
The UpdateConfig function modifies the config pointer directly within the lock, which means the config object passed in is mutated. This could lead to unexpected behavior if the caller continues to use the original config object after calling UpdateConfig. Consider either: 1) Making a deep copy before modification, or 2) Documenting this behavior clearly in the function documentation to warn callers about the in-place mutation.
|
这个copilot review要怎么说呢？是等待修改后合入还是如何，是否有经过自测？ |
copilot 的 agent review 不太会用, 本地跑过了, 怎么说呢, 痛点是解决了, 交互可以考虑后面优化 |
那我就跳过copilot的问题先合并了，我看看适配一下新的codex然后发一个新版 |
2 participants
解决 worktree 总是在 项目的子目录中， 导致 全局搜索看到一堆子目录里面的文件