[Feat] Add gmc wt share discover subcommand#58
Conversation
…eable files Signed-off-by: samzong <[email protected]>
![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 a new discover command to the worktree share utility, allowing users to automatically find and configure common files (like .env) or directories (like node_modules) for sharing across worktrees using either copy or symlink strategies. The implementation includes a discovery engine that scans the main worktree and a batch addition mechanism. Feedback focuses on improving the robustness of the CLI by removing a redundant --dry-run flag, refactoring duplicated logic in the discovery loops, ensuring deterministic behavior when selecting a worktree to scan, and improving error handling within the test suite.
| {Path: ".env", Strategy: StrategyCopy}, | ||
| }, | ||
| } | ||
| data, _ := yaml.Marshal(&cfg) |
There was a problem hiding this comment.
The error returned by yaml.Marshal is being ignored. In tests, it's important to check for all potential errors to ensure the test setup is correct and the test is reliable. Please handle this error, for example by using require.NoError(t, err).
| data, _ := yaml.Marshal(&cfg) | |
| data, err := yaml.Marshal(&cfg) | |
| require.NoError(t, err) |
| _ = wtShareAddCmd.RegisterFlagCompletionFunc("strategy", completeStrategies) | ||
|
|
||
| wtShareDiscoverCmd.Flags().BoolVar(&discoverAuto, "auto", false, "Actually add discovered items and sync") | ||
| wtShareDiscoverCmd.Flags().Bool("dry-run", true, "Preview mode (default behavior)") |
There was a problem hiding this comment.
The --dry-run flag is defined here but its value is never used in the command's logic. The dry-run behavior is instead controlled by the absence of the --auto flag. This can be confusing for users who might expect --dry-run=false to have an effect.
Since the Long description for the command already clearly states that dry-run is the default behavior, consider removing this flag to avoid confusion and simplify the command's interface.
| for _, candidate := range copyCandidates { | ||
| if existing[candidate] { | ||
| continue | ||
| } | ||
| if _, statErr := os.Stat(filepath.Join(mainPath, candidate)); statErr == nil { | ||
| results = append(results, DiscoverResult{ | ||
| Path: candidate, | ||
| Strategy: StrategyCopy, | ||
| Reason: "config/env file — isolated copy per worktree", | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| for _, candidate := range linkCandidates { | ||
| if existing[candidate] { | ||
| continue | ||
| } | ||
| if _, statErr := os.Stat(filepath.Join(mainPath, candidate)); statErr == nil { | ||
| results = append(results, DiscoverResult{ | ||
| Path: candidate, | ||
| Strategy: StrategySymlink, | ||
| Reason: "dependency/cache directory — shared symlink saves disk", | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The loops for copyCandidates and linkCandidates are nearly identical, leading to code duplication. To improve maintainability, this can be refactored.
Consider creating a helper function that takes a list of candidates and a strategy, or restructuring the candidate data into a single slice of structs that includes the strategy, and then iterating over it once.
| for _, wt := range worktrees { | ||
| if !wt.IsBare && filepath.Base(wt.Path) != ".bare" { | ||
| fmt.Fprintf(os.Stderr, "Warning: no main/master branch found, using worktree %q\n", filepath.Base(wt.Path)) | ||
| return wt.Path, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The fallback logic for finding a worktree (when no main or master branch is found) is non-deterministic. If multiple non-bare worktrees exist, the one chosen depends on the order returned by c.List(), which is not guaranteed to be stable. This could lead to unpredictable behavior on different runs.
To make the behavior predictable, you could sort the worktrees slice by path before this loop.
| oldCwd, _ := os.Getwd() | ||
| require.NoError(t, os.Chdir(repoDir)) | ||
| defer func() { _ = os.Chdir(oldCwd) }() |
There was a problem hiding this comment.
Errors from os.Getwd() and os.Chdir() in the deferred function are being ignored. This pattern is repeated in other tests in this file. It's a good practice to handle these errors to make tests more robust and prevent subtle failures.
Consider changing this to:
oldCwd, err := os.Getwd()
require.NoError(t, err)
require.NoError(t, os.Chdir(repoDir))
defer func() {
require.NoError(t, os.Chdir(oldCwd))
}()| oldCwd, _ := os.Getwd() | |
| require.NoError(t, os.Chdir(repoDir)) | |
| defer func() { _ = os.Chdir(oldCwd) }() | |
| t.Cleanup(func() { require.NoError(t, os.Chdir(oldCwd)) }) |
1 participant
What's changed?
gmc wt share discoversubcommand that scans the main worktree for files that should be shared across worktrees.env*,.claude/*,.serena/*) and link (node_modules,.venv,vendor,__pycache__) strategies--autoflag to batch-add discovered resources and sync in one shot--dry-runflag (default) for preview modeAddDiscoveredResourcesbatch method — single config load/save cycleWhy
skills/gmc/scripts/wt-share-discover.shwith native Go