✨ feat(worktree): add sync command and --sync option#34
Conversation
Signed-off-by: samzong <samzong.lu@gmail.com>
Summary of ChangesHello @samzong, 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 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 a sync command for worktrees and a --sync option for the add command, which is a valuable feature for keeping base branches up-to-date. The implementation is generally solid and includes good test coverage for the new functionality. However, I've identified a few areas for improvement. There's an inconsistency in how origin and upstream remotes are prioritized, which could affect fork-based workflows. Additionally, there are a couple of opportunities to improve code organization and avoid state mutation for better maintainability. My detailed comments below provide specific suggestions to address these points.
| if ref := c.gitSymbolicRef(repoDir, "refs/remotes/origin/HEAD"); ref != "" { | ||
| return ref, nil | ||
| } | ||
| if ref := c.gitSymbolicRef(repoDir, "refs/remotes/origin/HEAD"); ref != "" { | ||
| if ref := c.gitSymbolicRef(repoDir, "refs/remotes/upstream/HEAD"); ref != "" { | ||
| return ref, nil | ||
| } |
There was a problem hiding this comment.
The logic for resolving the base branch now prefers origin over upstream. In a typical forked repository workflow, upstream represents the main source of truth, and developers sync from it. The selectSyncRemote function in sync.go correctly prefers upstream. To maintain consistency and support forked workflows correctly, upstream should be checked before origin when determining the base branch.
I suggest swapping the order of these checks.
| if ref := c.gitSymbolicRef(repoDir, "refs/remotes/origin/HEAD"); ref != "" { | |
| return ref, nil | |
| } | |
| if ref := c.gitSymbolicRef(repoDir, "refs/remotes/origin/HEAD"); ref != "" { | |
| if ref := c.gitSymbolicRef(repoDir, "refs/remotes/upstream/HEAD"); ref != "" { | |
| return ref, nil | |
| } | |
| if ref := c.gitSymbolicRef(repoDir, "refs/remotes/upstream/HEAD"); ref != "" { | |
| return ref, nil | |
| } | |
| if ref := c.gitSymbolicRef(repoDir, "refs/remotes/origin/HEAD"); ref != "" { | |
| return ref, nil | |
| } |
| if ref := c.gitSymbolicRef(repoDir, "refs/remotes/origin/HEAD"); ref != "" { | ||
| return ref, nil | ||
| } | ||
| if ref := c.gitSymbolicRef(repoDir, "refs/remotes/upstream/HEAD"); ref != "" { | ||
| return ref, nil | ||
| } |
There was a problem hiding this comment.
This logic prefers origin over upstream when resolving the base branch name. However, selectSyncRemote prefers upstream when selecting a remote to fetch from. In a standard forked repository workflow, upstream is the canonical source of truth. For consistency, upstream should be preferred here as well when determining the base branch name.
I recommend swapping the order of these checks to prioritize upstream.
| if ref := c.gitSymbolicRef(repoDir, "refs/remotes/origin/HEAD"); ref != "" { | |
| return ref, nil | |
| } | |
| if ref := c.gitSymbolicRef(repoDir, "refs/remotes/upstream/HEAD"); ref != "" { | |
| return ref, nil | |
| } | |
| if ref := c.gitSymbolicRef(repoDir, "refs/remotes/upstream/HEAD"); ref != "" { | |
| return ref, nil | |
| } | |
| if ref := c.gitSymbolicRef(repoDir, "refs/remotes/origin/HEAD"); ref != "" { | |
| return ref, nil | |
| } |
| wtAddSync bool | ||
| wtSyncBase string | ||
| wtSyncDryRun bool | ||
| ) | ||
|
|
||
| var wtSyncCmd = &cobra.Command{ | ||
| Use: "sync", | ||
| Short: "Sync the base branch used for worktrees", | ||
| Long: `Sync the base branch used for worktrees. | ||
|
|
||
| This updates the base branch using fast-forward only and optionally | ||
| updates the base worktree when it's clean.`, | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| wtClient := worktree.NewClient(worktree.Options{Verbose: verbose}) | ||
| return runWorktreeSync(wtClient) | ||
| }, | ||
| } | ||
|
|
||
| func init() { | ||
| wtCmd.AddCommand(wtSyncCmd) | ||
| wtAddCmd.Flags().BoolVar(&wtAddSync, "sync", false, "Sync base branch before creating worktree") |
There was a problem hiding this comment.
The wtAddSync flag variable is defined here, and the flag is registered for wtAddCmd in this file's init function. This is confusing because the flag belongs to the add command, which is defined in cmd/worktree.go. For better code organization and maintainability, flag variables and their registrations should be co-located with the command they belong to. I recommend moving the wtAddSync variable definition and the wtAddCmd.Flags().BoolVar(...) call to cmd/worktree.go.
| origDir := c.runner.Dir | ||
| if repoDir != "" { | ||
| c.runner.Dir = repoDir | ||
| defer func() { | ||
| c.runner.Dir = origDir | ||
| }() | ||
| } |
There was a problem hiding this comment.
The Sync function temporarily mutates the c.runner.Dir field. This is not thread-safe and can lead to subtle bugs if the Client is ever used concurrently. A better approach would be to avoid mutating the client's state. Since other git commands in this function use the -C flag to specify the directory, consider refactoring c.List() to also accept a directory parameter, which would make this state mutation unnecessary.
1 participant
No description provided.