Add gw status and prune commands for worktree management#3
Add gw status and prune commands for worktree management#3
Conversation
- Add GitHub.find_prs_by_branches to batch fetch PR info - Add Repository.full_name to parse from remote URL - Add gw status: show worktrees with PR status, title, and colored state - Add gw prune: remove MERGED/CLOSED worktrees with --dry-run and --merged options - Fix Worktree.list to get actual branch names from worktrees
There was a problem hiding this comment.
Pull request overview
This PR adds worktree management commands to track GitHub PR status and clean up completed work. It introduces gw status to display PR states for all worktrees and gw prune to remove worktrees associated with merged or closed PRs.
Changes:
- Enhanced worktree listing to use
git worktree listinstead of directory scanning for better accuracy - Added GitHub API integration to fetch PR information in batch for efficiency
- Implemented status display with color-coded PR states and summary statistics
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| gw-rb/lib/gw/worktree.rb | Modified list method to use git commands for accurate worktree enumeration |
| gw-rb/lib/gw/repository.rb | Converted full_name to dynamic method that parses GitHub URL from remote origin |
| gw-rb/lib/gw/github.rb | Added PR lookup methods with batch fetching for performance |
| gw-rb/lib/gw/cli.rb | Implemented status and prune commands with formatting helpers and interactive confirmation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| new(repository, branch) | ||
| end.select(&:exist?) | ||
| # Use git worktree list to get actual branch names | ||
| output = `git -C #{repository.bare_path} worktree list --porcelain` |
There was a problem hiding this comment.
The bare_path is interpolated into a shell command without escaping. While bare_path is constructed from Config.core_dir and name, if either contains special shell characters, this could cause command failures or security issues. Consider using Shellwords.escape or similar escaping mechanism to safely handle paths with special characters.
| return @_full_name if @_full_name | ||
|
|
||
| @_full_name ||= begin | ||
| remote_url = `git -C #{bare_path} remote get-url origin 2>/dev/null`.strip |
There was a problem hiding this comment.
The bare_path is interpolated into a shell command without escaping. If the path contains special shell characters, this could cause command failures or security issues. Consider using Shellwords.escape or similar escaping mechanism to safely handle paths with special characters.
| # Fetch all open and closed PRs | ||
| %w[open closed].each do |state| | ||
| prs = client.pull_requests(full_name, state: state, per_page: 100) | ||
| prs.each do |pr| | ||
| branch = pr.head.ref | ||
| next unless branches.include?(branch) | ||
|
|
||
| result[branch] = { | ||
| number: pr.number, | ||
| state: pr.merged_at ? "MERGED" : pr.state.upcase, | ||
| title: pr.title, | ||
| url: pr.html_url | ||
| } |
There was a problem hiding this comment.
The find_prs_by_branches method only fetches the first 100 PRs per state. If a repository has more than 100 open or closed PRs, some worktrees might not get their PR status correctly identified. Consider implementing pagination to fetch all PRs or document this limitation.
| # Fetch all open and closed PRs | |
| %w[open closed].each do |state| | |
| prs = client.pull_requests(full_name, state: state, per_page: 100) | |
| prs.each do |pr| | |
| branch = pr.head.ref | |
| next unless branches.include?(branch) | |
| result[branch] = { | |
| number: pr.number, | |
| state: pr.merged_at ? "MERGED" : pr.state.upcase, | |
| title: pr.title, | |
| url: pr.html_url | |
| } | |
| # Fetch all open and closed PRs, handling pagination | |
| %w[open closed].each do |state| | |
| page = 1 | |
| loop do | |
| prs = client.pull_requests(full_name, state: state, per_page: 100, page: page) | |
| break if prs.empty? | |
| prs.each do |pr| | |
| branch = pr.head.ref | |
| next unless branches.include?(branch) | |
| result[branch] = { | |
| number: pr.number, | |
| state: pr.merged_at ? "MERGED" : pr.state.upcase, | |
| title: pr.title, | |
| url: pr.html_url | |
| } | |
| end | |
| break if prs.length < 100 | |
| page += 1 |
| # Find PR by branch name | ||
| # Returns { number:, state:, title:, url: } or nil | ||
| def find_pr_by_branch(full_name, branch) | ||
| prs = client.pull_requests(full_name, state: "all", head: "#{full_name.split("/").first}:#{branch}") | ||
| return nil if prs.empty? | ||
|
|
||
| pr = prs.first | ||
| { | ||
| number: pr.number, | ||
| state: pr.state.upcase, | ||
| merged: pr.merged_at ? true : false, | ||
| title: pr.title, | ||
| url: pr.html_url | ||
| } | ||
| rescue Octokit::Error | ||
| nil | ||
| end |
There was a problem hiding this comment.
The find_pr_by_branch method is defined but appears to be unused in the codebase. The feature uses find_prs_by_branches instead. Consider removing this method to reduce code maintenance burden, or add a comment explaining when this single-PR lookup might be needed.
| end | ||
|
|
||
| def color_padding(state) | ||
| %w[OPEN MERGED CLOSED].include?(state) ? 9 : 0 |
There was a problem hiding this comment.
The color_padding helper method uses a magic number 9 to account for ANSI color code characters. This value is fragile and could break if color codes change. Consider using a more explicit calculation based on the actual color codes used (e.g., measuring "\e[32m".length + "\e[0m".length), or adding a comment explaining where the value 9 comes from.
| %w[OPEN MERGED CLOSED].include?(state) ? 9 : 0 | |
| return 0 unless state.is_a?(String) | |
| colorize_state(state).length - state.length |
| { | ||
| number: pr.number, | ||
| state: pr.state.upcase, | ||
| merged: pr.merged_at ? true : false, |
There was a problem hiding this comment.
The merged field is calculated using a ternary operator that returns true/false explicitly. This is redundant since the expression pr.merged_at ? true : false can be simplified to !!pr.merged_at or pr.merged_at.nil? == false. However, note that this field is defined but never used in the return hash, making it unnecessary code.
| merged: pr.merged_at ? true : false, | |
| merged: !!pr.merged_at, |
| return nil if remote_url.empty? | ||
|
|
||
| # Parse GitHub URL: https://github.com/owner/repo.git or git@github.com:owner/repo.git | ||
| if remote_url =~ %r{github\.com[/:]([^/]+)/([^/]+?)(?:\.git)?$} |
There was a problem hiding this comment.
The regex pattern does not account for all valid GitHub URL formats. For example, it won't match URLs with .git extensions in SSH format that might have additional path segments. Additionally, the regex allows any characters in the owner and repo name groups which could capture invalid repository identifiers. Consider tightening the regex to match valid GitHub username/repository patterns (alphanumeric, hyphens, underscores) to avoid false positives.
| if remote_url =~ %r{github\.com[/:]([^/]+)/([^/]+?)(?:\.git)?$} | |
| if remote_url =~ %r{\Agithub\.com[/:]([A-Za-z0-9_-]+)/([A-Za-z0-9_.-]+?)(?:\.git)?(?:/.*)?\z} |
| repo_name = @argv[1] | ||
| dry_run = @argv.include?("--dry-run") | ||
| merged_only = @argv.include?("--merged") | ||
|
|
There was a problem hiding this comment.
The argument parsing for flags like --dry-run and --merged does not validate their position or prevent the repo_name from being mistaken for a flag. If a user runs 'gw prune --dry-run', the repo_name would be set to '--dry-run' instead of showing a proper error. Consider checking that repo_name doesn't start with '--' or parsing flags more robustly.
| repo_name = @argv[1] | |
| dry_run = @argv.include?("--dry-run") | |
| merged_only = @argv.include?("--merged") | |
| args = @argv[1..] || [] | |
| repo_name = nil | |
| dry_run = false | |
| merged_only = false | |
| args.each do |arg| | |
| case arg | |
| when "--dry-run" | |
| dry_run = true | |
| when "--merged" | |
| merged_only = true | |
| else | |
| if arg.start_with?("--") | |
| puts "Error: unknown option '#{arg}'" | |
| puts "Usage: gw prune <repo> [--dry-run] [--merged]" | |
| exit 1 | |
| elsif repo_name | |
| puts "Error: multiple repository names provided" | |
| puts "Usage: gw prune <repo> [--dry-run] [--merged]" | |
| exit 1 | |
| else | |
| repo_name = arg | |
| end | |
| end | |
| end |
| gw remove <repo>/<branch> Remove worktree | ||
| gw list [repo] List worktrees | ||
| gw status <repo> Show worktrees with PR status | ||
| gw prune <repo> [--dry-run] [--merged] Remove merged/closed worktrees |
There was a problem hiding this comment.
The usage description has extra whitespace before "Remove" that creates inconsistent spacing in the command list. The other commands don't have this leading space before their descriptions.
| gw prune <repo> [--dry-run] [--merged] Remove merged/closed worktrees | |
| gw prune <repo> [--dry-run] [--merged] Remove merged/closed worktrees |
| output = `git -C #{repository.bare_path} worktree list --porcelain` | ||
| return [] if output.empty? | ||
|
|
||
| output.scan(/worktree (.+)/).map do |paths| |
There was a problem hiding this comment.
The regex pattern /worktree (.+)/ is too greedy and will capture everything after 'worktree ' including potential newlines in the path. While git output is typically well-formed, this could lead to incorrect parsing if paths contain unusual characters. Consider using /worktree (.+?)$/ with multiline mode or /worktree ([^\n]+)/ to only capture until the end of the line.
| output.scan(/worktree (.+)/).map do |paths| | |
| output.scan(/worktree ([^\n]+)/).map do |paths| |
Summary
gw statuscommand to show worktrees with their GitHub PR statusgw prunecommand to remove MERGED/CLOSED worktrees--dry-runflag for previewing what will be removed--mergedflag to only remove MERGED worktrees (keep CLOSED)Features
gw status
Shows a table of all worktrees with:
gw prune
Removes worktrees with MERGED or CLOSED PRs:
--dry-runto preview--mergedto only remove MERGED (not CLOSED)Example