Add worktree subcommand
#32
Conversation
Signed-off-by: Paulo Gomes <pjbgf@linux.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a new worktree command to the go-git CLI that enables management of Git worktrees with three subcommands: add, list, and remove. The implementation aims to align output formatting with upstream Git for the list command and supports testing the go-git linked worktree functionality.
- Adds comprehensive worktree management capabilities (add, list, remove)
- Updates go-git and related dependencies to versions with worktree support
- Aligns list command output format with upstream Git
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| cmd/gogit/worktree.go | New file implementing worktree subcommands with add, list, and remove functionality |
| go.mod | Updates go-git, go-billy, and go-git-fixtures dependencies to versions supporting worktree features |
| go.sum | Corresponding checksum updates for the new dependency versions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if worktreeAddCommit != "" { | ||
| hash := plumbing.NewHash(worktreeAddCommit) | ||
| if !hash.IsZero() { | ||
| opts = append(opts, worktree.WithCommit(hash)) | ||
| } |
There was a problem hiding this comment.
No validation is performed on the commit hash provided by the user. If the user provides an invalid hash string, plumbing.NewHash() will silently return a zero hash, and the WithCommit option won't be added. This could lead to unexpected behavior where the user thinks they're checking out a specific commit but they're not.
| return fmt.Sprint("[", name.Short(), "]") | ||
| } | ||
|
|
||
| return fmt.Sprint("(detached ", string(ref.Name()), ")") |
There was a problem hiding this comment.
The fmt.Sprint call is unnecessarily verbose. Using fmt.Sprintf with proper formatting would be more idiomatic and clearer.
| return fmt.Sprint("[", name.Short(), "]") | |
| } | |
| return fmt.Sprint("(detached ", string(ref.Name()), ")") | |
| return fmt.Sprintf("[%s]", name.Short()) | |
| } | |
| return fmt.Sprintf("(detached %s)", string(ref.Name())) |
| return fmt.Sprint("[", name.Short(), "]") | ||
| } | ||
|
|
||
| return fmt.Sprint("(detached ", string(ref.Name()), ")") |
There was a problem hiding this comment.
The fmt.Sprint call is unnecessarily verbose. Using fmt.Sprintf with proper formatting would be more idiomatic and clearer.
| return fmt.Sprint("[", name.Short(), "]") | |
| } | |
| return fmt.Sprint("(detached ", string(ref.Name()), ")") | |
| return fmt.Sprintf("[%s]", name.Short()) | |
| } | |
| return fmt.Sprintf("(detached %s)", ref.Name()) |
| continue | ||
| } | ||
|
|
||
| wtPath := filepath.Dir(string(gitdirData[:len(gitdirData)-1])) |
There was a problem hiding this comment.
Potential index out of bounds error when gitdirData is empty. The code subtracts 1 from the length without checking if the data has at least one byte. This could panic if the gitdir file is empty or only contains a newline character.
| r, err := w.Open(wt) | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| ref, err := r.Head() |
There was a problem hiding this comment.
Variable shadowing issue: the variable 'r' is being redeclared here, shadowing the repository variable 'r' from line 89. This makes the outer 'r' variable inaccessible within this scope and could lead to confusion about which repository is being used.
| r, err := w.Open(wt) | |
| if err != nil { | |
| continue | |
| } | |
| ref, err := r.Head() | |
| wtRepo, err := w.Open(wt) | |
| if err != nil { | |
| continue | |
| } | |
| ref, err := wtRepo.Head() |
| ref, err := r.Head() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get HEAD: %w", err) | ||
| } | ||
|
|
||
| fmt.Fprintf(cmd.OutOrStdout(), "%-30s %s %s\n", wtPath, ref.Hash().String()[:7], refName(ref)) |
There was a problem hiding this comment.
Variable shadowing issue: the variable 'ref' is being redeclared here, shadowing the HEAD reference 'ref' from line 104. This makes the outer 'ref' variable inaccessible within this scope and could lead to confusion.
| ref, err := r.Head() | |
| if err != nil { | |
| return fmt.Errorf("failed to get HEAD: %w", err) | |
| } | |
| fmt.Fprintf(cmd.OutOrStdout(), "%-30s %s %s\n", wtPath, ref.Hash().String()[:7], refName(ref)) | |
| wtRef, err := r.Head() | |
| if err != nil { | |
| return fmt.Errorf("failed to get HEAD: %w", err) | |
| } | |
| fmt.Fprintf(cmd.OutOrStdout(), "%-30s %s %s\n", wtPath, wtRef.Hash().String()[:7], refName(wtRef)) |
|
|
||
| ref, err := r.Head() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get HEAD: %w", err) |
There was a problem hiding this comment.
Inconsistent error handling in the loop. While errors from os.ReadFile, w.Init, and w.Open are silently ignored with 'continue', an error from r.Head() returns immediately with an error. This inconsistency could make the command fail unexpectedly instead of just skipping problematic worktrees like the earlier error cases.
| return fmt.Errorf("failed to get HEAD: %w", err) | |
| continue |
Adds a new
worktreecommand with sub commandadd,removeandlist.The output of
listwas aligned with upstream for the supported cases (e.g. we currently don't supportprune):go-git/cli
upstream git
This command was largely made to test the go-git linked worktree implementation.