feat(worktree): Git-first discovery + shared config in git common dir#48
feat(worktree): Git-first discovery + shared config in git common dir#48
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the flexibility and compatibility of gmc's worktree management and shared resource features. It shifts the core logic to be "Git-first" for worktree discovery, allowing gmc wt commands to function seamlessly within any Git repository, not just those following the .bare pattern. Furthermore, it centralizes shared configuration within the Git common directory, improving consistency and supporting diverse repository layouts, while also refining how shared resources are managed and synchronized across worktrees. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the worktree management functionality, primarily by enabling git-first discovery (removing the hard requirement for a .bare repository layout for read operations) and relocating the shared configuration to the git common directory. This makes the feature more flexible and compatible with existing non-bare repositories. The changes are well-structured and include new tests and documentation. I've identified a couple of high-severity issues regarding ambiguous worktree resolution by basename, which could lead to unpredictable behavior in environments with similarly named worktrees. I also found a minor logging issue where a verbose warning was not being displayed. Addressing these points will enhance the robustness of this new feature.
| if filepath.Base(wt.Path) == worktreeName { | ||
| return wt.Path, nil | ||
| } |
There was a problem hiding this comment.
Matching worktrees by basename can be ambiguous and lead to unpredictable behavior. If a user has multiple worktrees with the same basename in different locations (e.g., ~/dev/project1/my-feature and ~/dev/project2/my-feature which are both worktrees of the same repo), this logic will match the first one it finds in the list from c.List(). The order is not guaranteed. This could result in operations being performed on the wrong worktree.
Consider detecting this ambiguity. You could collect all matches and if there's more than one, return an error asking the user to provide a more specific, unique path.
| for _, wt := range worktrees { | ||
| if filepath.Base(wt.Path) != parts[0] { | ||
| continue | ||
| } |
There was a problem hiding this comment.
This logic for resolving legacy shared resource paths (e.g., worktree-name/path/to/file) has the same ambiguity as in resolveWorktreePath. It matches the source worktree by its basename (filepath.Base(wt.Path)).
If multiple worktrees share the same basename, this could resolve the source path from the wrong worktree, leading to incorrect files being synced. This is especially risky as it could overwrite files with incorrect content.
It would be safer to detect when parts[0] matches multiple worktree basenames and return an error to the user, prompting for a more specific configuration.
| var report Report | ||
| report.Warn(fmt.Sprintf("Skipping %s: source worktree is target", res.Path)) |
There was a problem hiding this comment.
This warning message will never be displayed. A new local report variable is created, a warning is added to it, and then the variable goes out of scope. The Warn method appends to the report's internal slice, but since the report is never used, the warning is lost.
If the intention is to log a verbose message, you should use a mechanism that ensures the message is displayed, such as writing directly to os.Stderr.
| var report Report | |
| report.Warn(fmt.Sprintf("Skipping %s: source worktree is target", res.Path)) | |
| report.Warn(fmt.Sprintf("Skipping %s: source worktree is target", res.Path)) |
There was a problem hiding this comment.
Pull request overview
This PR updates gmc wt to discover worktrees using Git metadata (working in non-bare repos/worktrees) and relocates the shared-resource config to the repository’s git common dir so wt share works across both bare and non-bare layouts.
Changes:
- Add Git-first worktree discovery and adjust display naming to prefer repo-relative paths (with sane fallback for external worktrees).
- Move shared-resource config to
<git-common-dir>/gmc-share.ymland update sync to target real worktree paths from Git metadata. - Add non-bare repo/worktree test coverage for
wtdefault output and shared-resource sync/config resolution.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/worktree/worktree.go | Adds GetGitCommonDir/GetRepoRoot and aligns “root” resolution with Git metadata. |
| internal/worktree/resource.go | Moves shared config into git common dir, normalizes shared paths, and syncs to real worktree paths. |
| internal/worktree/resource_nonbare_test.go | Tests config resolution and sync behavior from a non-bare linked worktree. |
| cmd/worktree.go | Makes default wt output work in non-bare repos and improves worktree name display. |
| cmd/worktree_switch.go | Removes bare-only gating so switching works in non-bare repos too. |
| cmd/worktree_share.go | Updates help text and removes legacy auto-prepend behavior in interactive add. |
| cmd/worktree_test.go | Tests default wt output from a non-bare linked worktree. |
| docs/plans/2026-03-14-worktree-discovery-share-common-dir-design.md | Documents design decisions and verification plan. |
| README.md | Documents Git-first discovery and new shared-config location/behavior. |
| func (c *Client) resolveWorktreePath(worktreeName string) (string, error) { | ||
| if worktreeName == "" { | ||
| return "", errors.New("worktree name cannot be empty") | ||
| } | ||
|
|
||
| repoRoot, _ := c.GetRepoRoot() | ||
| worktrees, err := c.List() | ||
| if err != nil { | ||
| if repoRoot != "" { | ||
| return filepath.Join(repoRoot, worktreeName), nil | ||
| } | ||
| return "", err | ||
| } | ||
| for _, wt := range worktrees { | ||
| if wt.Path == worktreeName { | ||
| return wt.Path, nil | ||
| } | ||
| if filepath.Base(wt.Path) == worktreeName { | ||
| return wt.Path, nil | ||
| } | ||
| if repoRoot != "" { | ||
| if rel, relErr := filepath.Rel(repoRoot, wt.Path); relErr == nil && rel == worktreeName { | ||
| return wt.Path, nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if repoRoot != "" { | ||
| return filepath.Join(repoRoot, worktreeName), nil | ||
| } | ||
| return worktreeName, nil | ||
| } |
| var report Report | ||
| report.Warn(fmt.Sprintf("Skipping %s: source worktree is target", res.Path)) |
| @@ -192,7 +216,7 @@ func (c *Client) AddSharedResource(path string, strategy ResourceStrategy) (Repo | |||
|
|
|||
| if !found { | |||
| cfg.Resources = append(cfg.Resources, SharedResource{ | |||
| Path: path, | |||
| Path: normalizedPath, | |||
| Strategy: strategy, | |||
| }) | |||
| } | |||
| @@ -201,7 +225,7 @@ func (c *Client) AddSharedResource(path string, strategy ResourceStrategy) (Repo | |||
| return report, err | |||
| } | |||
|
|
|||
| report.Info(fmt.Sprintf("Updated shared resource: %s (%s)", path, strategy)) | |||
| report.Info(fmt.Sprintf("Updated shared resource: %s (%s)", normalizedPath, strategy)) | |||
| return report, nil | |||
| configPath := filepath.Join(commonDir, sharedConfigName) | ||
| legacyCandidates := []string{ | ||
| filepath.Join(commonDir, legacySharedConfigYML), | ||
| filepath.Join(commonDir, legacySharedConfigYAML), | ||
| } | ||
|
|
||
| configPath := filepath.Join(root, ".gmc-shared.yml") | ||
| if _, err := os.Stat(configPath); os.IsNotExist(err) { | ||
| yamlPath := filepath.Join(root, ".gmc-shared.yaml") | ||
| if _, err := os.Stat(yamlPath); err == nil { | ||
| configPath = yamlPath | ||
| for _, candidate := range legacyCandidates { | ||
| if _, statErr := os.Stat(candidate); statErr == nil { | ||
| configPath = candidate | ||
| break | ||
| } | ||
| } | ||
| } |
| commonDir = root | ||
| } else { | ||
| return nil, "", err | ||
| } | ||
| } | ||
|
|
| func (c *Client) NormalizeSharedResourcePath(path string) (string, error) { | ||
| trimmed := strings.TrimSpace(path) | ||
| if trimmed == "" { | ||
| return "", errors.New("shared resource path cannot be empty") | ||
| } | ||
|
|
||
| if filepath.IsAbs(trimmed) { | ||
| currentRoot := c.currentTopLevel() | ||
| if currentRoot != "" { | ||
| rel, err := filepath.Rel(currentRoot, trimmed) | ||
| if err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { | ||
| trimmed = rel | ||
| } | ||
| } | ||
| } | ||
|
|
||
| trimmed = filepath.Clean(trimmed) | ||
| if trimmed == "." { | ||
| return "", errors.New("shared resource path cannot be '.'") | ||
| } | ||
| if strings.HasPrefix(trimmed, ".."+string(filepath.Separator)) || trimmed == ".." { | ||
| return "", fmt.Errorf("shared resource path must stay within the worktree: %s", path) | ||
| } | ||
| return trimmed, nil |
|
/gemini review Addressed the previous review concerns in this update:
Tests: |
|
@copilot-pull-request-reviewer please re-review the latest changes. Addressed the shared-path resolution and compatibility issues from the previous pass, and |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the worktree functionality by removing the hard dependency on the .bare repository layout, making it usable with standard Git repositories that use worktrees. The changes are well-structured, moving the shared configuration to the git common directory (.git/ or .bare/) with backward compatibility, and resolving worktree paths directly from Git metadata. The introduction of new tests for non-bare repository scenarios is a great addition that increases confidence in these changes. I have a couple of suggestions for code simplification and to improve robustness.
| if err != nil { | ||
| if repoRoot != "" { | ||
| candidate := filepath.Join(repoRoot, worktreeName) | ||
| if info, statErr := os.Stat(candidate); statErr == nil && info.IsDir() { | ||
| return candidate, nil | ||
| } | ||
| } | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
The fallback logic for when c.List() fails is risky. c.List() should be the source of truth for what Git considers a worktree. If it fails, falling back to a filesystem check with os.Stat could lead to operations on a directory that isn't a valid worktree from Git's perspective. This might hide underlying repository state issues and cause unexpected behavior. It would be safer to propagate the error from c.List() directly to ensure the caller is aware of the problem.
if err != nil {
return "", err
}| if filepath.Base(commonDir) == ".bare" { | ||
| return filepath.Dir(commonDir), nil | ||
| } | ||
| return filepath.Dir(commonDir), nil |
There was a problem hiding this comment.
This if condition is redundant. Since GetGitCommonDir returns an absolute path, filepath.Dir(commonDir) correctly returns the parent directory (the repository root) for both .bare and .git common directories. You can simplify the code by removing this if block.
return filepath.Dir(commonDir), nil
Summary
Implements two improvements to
gmc wt:1. Git-first worktree discovery
gmc wtandgmc wt switchno longer reject non-bare repositories.bareremains the preferred layout forgmc wt clone / init, but it is no longer a hard gate for read operations2. Shared config moves to git common dir
<git-common-dir>/gmc-share.yml(e.g..git/gmc-share.ymlor.bare/gmc-share.yml).gmc-shared.yml/.gmc-shared.yamlare still read as fallback for backward compatibilitywt.Path), no longer constructed by string concatenationSaveSharedConfigcreates the parent directory if needed3. Display name fix
wt listnow prefer repo-relative paths; external worktrees fall back to basename instead of raw absolute pathsTests added
cmd/worktree_test.go: defaultwtoutput works from a non-bare repointernal/worktree/resource_nonbare_test.go: shared config path resolves to git common dir; sync writes to real worktree paths, not fabricated directoriesChecklist
go test ./...all green.bareinit/clone behaviour unchanged