feat: add zero changes push and pr subcommands, and extra repo-info metrics#391
feat: add zero changes push and pr subcommands, and extra repo-info metrics#391KunjShah95 wants to merge 1 commit into
Conversation
WalkthroughThis PR adds a ChangesPush and PR CLI commands
Estimated code review effort: 3 (Moderate) | ~30 minutes Repo-info commit/branch/tag counts
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as runChangesPR
participant Push as deps.pushChanges
participant Git
participant Gh as gh CLI
User->>CLI: zero changes pr --fill
CLI->>Push: pushChanges(PushOptions)
Push->>Git: rev-parse --abbrev-ref HEAD
Push->>Git: git config branch.<branch>.remote
Push->>Git: git push -u remote branch
Git-->>Push: output
Push-->>CLI: PushResult{Remote, Branch, Output}
CLI->>Gh: exec.CommandContext(gh pr create ...)
Gh-->>CLI: stdout/stderr
CLI-->>User: JSON or trimmed output
Possibly related PRs
Suggested reviewers: Given this PR runs 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
please rebase to main and fix conflicts @KunjShah95 |
KunjShah95
force-pushed
the
feat-changes-commands-and-repoinfo
branch
from
10b30be to
2e338f6
Compare
KunjShah95
force-pushed
the
feat-changes-commands-and-repoinfo
branch
from
2e338f6 to
567ff2c
Compare
|
@kevincodex1 do check it out now |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
internal/repoinfo/repoinfo.go (1)
240-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate non-empty-line-counting logic between branch and tag counting.
The two blocks are structurally identical loops; extracting a small
countNonEmptyLines(s string) inthelper would remove the duplication.♻️ Suggested refactor
+func countNonEmptyLines(s string) int { + count := 0 + for _, line := range strings.Split(s, "\n") { + if strings.TrimSpace(line) != "" { + count++ + } + } + return count +} + ... - if branches, err := run(ctx, dir, "branch", "-a"); err == nil { - lines := strings.Split(branches, "\n") - count := 0 - for _, line := range lines { - if strings.TrimSpace(line) != "" { - count++ - } - } - info.BranchCount = count - } - if tags, err := run(ctx, dir, "tag"); err == nil { - lines := strings.Split(tags, "\n") - count := 0 - for _, line := range lines { - if strings.TrimSpace(line) != "" { - count++ - } - } - info.TagCount = count - } + if branches, err := run(ctx, dir, "branch", "-a"); err == nil { + info.BranchCount = countNonEmptyLines(branches) + } + if tags, err := run(ctx, dir, "tag"); err == nil { + info.TagCount = countNonEmptyLines(tags) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/repoinfo/repoinfo.go` around lines 240 - 259, The branch and tag counting blocks in repoinfoInfo use the same non-empty-line counting logic twice, so extract that shared loop into a small helper like countNonEmptyLines and reuse it from the existing run(ctx, dir, "branch", "-a") and run(ctx, dir, "tag") paths. Keep the helper near the repo info counting code so BranchCount and TagCount are set from the same centralized logic and the repeated strings.Split/strings.TrimSpace/count pattern is removed.internal/repoinfo/repoinfo_test.go (1)
46-47: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't cover the symbolic-ref/remote-tracking overcount edge case.
The canned
branchoutput only exercises the simple local+one-remote case. Given theBranchCountovercounting concern flagged in repoinfo.go, consider adding a case with aremotes/origin/HEAD -> origin/mainline to lock in correct behavior once fixed.Also applies to: 90-98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/repoinfo/repoinfo_test.go` around lines 46 - 47, The repoinfo test fixture only covers a simple local branch plus one remote branch, so it misses the symbolic-ref overcount case in BranchCount. Update the canned branch output used by the repoinfo_test.go cases to include a remotes/origin/HEAD -> origin/main line, and add/adjust the assertion around BranchCount so it verifies the symbolic-ref is not counted as an extra branch. Use the existing repoinfo/BranchCount test setup to locate and expand the case.internal/cli/workflows.go (1)
478-480: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
zero changes prcannot force-push or target a non-default remote.
--remote/--forceare rejected for theprcommand (Line 478-480), sorunChangesPR's implicit push (Line 845-847) always does a plaingit push -u <upstream-or-origin> <branch>. After a rebase/amend, the auto-push will fail and the user has to fall back tozero changes push --forcefirst, then re-runpr— somewhat defeating the "push and create a PR in one step" convenience this command is meant to provide. Consider allowing--remote/--forceforprtoo and threading them into thezerogit.PushOptions.Also applies to: 845-847
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/workflows.go` around lines 478 - 480, The pr workflow currently rejects --remote and --force in the option validation and then runChangesPR always does a plain push, which blocks force-pushes or non-default remotes after rebases. Update the command handling in parse/workflows around the options check and runChangesPR so pr accepts these flags and passes them through to zerogit.PushOptions for the implicit push, keeping the existing push behavior as the default when flags are not set.internal/zerogit/zerogit_test.go (1)
525-549: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only covers the happy path with an existing upstream remote.
Missing coverage for:
Force/DryRunflag →--force-with-lease/--dry-runargs, and the fallback-to-originbranch whenbranch.<name>.remotelookup fails/returns empty (theelsebranch at zerogit.go Line 571-573 is never exercised).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/zerogit/zerogit_test.go` around lines 525 - 549, The current TestPushBranchesToRemote only verifies the happy path with an existing upstream remote, so expand it to cover the missing Push behavior. Add cases around Push/PushOptions and fakeRunner to assert that setting Force and DryRun results in the expected git push args (--force-with-lease and --dry-run), and add a scenario where the branch.<name>.remote lookup returns empty or fails so the fallback-to-origin path in Push is exercised and verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/workflows.go`:
- Around line 828-877: `runChangesPR` invokes `gh pr create` directly, unlike
the other injectable side effects in `appDeps`, which makes PR creation logic
hard to unit test. Add a `createPR` dependency to `appDeps` (similar to
`inspectChanges`, `commitChanges`, and `pushChanges`), move the
`exec.CommandContext` logic behind that method, and have `runChangesPR` call the
injected helper so tests can verify `--fill`, `--draft`, `--title`, and `--body`
argument construction without requiring the `gh` binary.
- Around line 817-826: The new handlers in runChangesPush and runChangesPR are
ignoring stdout write failures, which is inconsistent with the rest of
workflows.go. Update the fmt.Fprintf/fmt.Fprintln calls in those handlers to
check their returned errors and return exitCrash on failure, matching the
pattern used by the other subcommand handlers in this file; use the existing
runChangesPush and runChangesPR functions as the places to fix.
- Around line 844-891: The pr creation flow in `runChangesPr` is emitting
human-readable status lines before the `options.json` branch, which makes
`--json` output invalid. Move or gate the `fmt.Fprintln`/`fmt.Fprintf` status
messages so they only run in the non-JSON path, matching the pattern used by
`runChangesPush`, and ensure `writePrettyJSON` is the only thing written to
stdout when `options.json` is true.
In `@internal/repoinfo/repoinfo.go`:
- Around line 57-59: The new repo count metrics are using plain ints, so
git-command failures are hidden as 0 instead of “unknown.” Update the repo info
model in repoinfo.go so BranchCount, CommitCount, and TagCount follow the same
optional pattern as AgeDays, Contributors90d, and CommitVelocity30d, then adjust
the count-gathering logic to leave them nil on command failure and have
formatRepoInfo in repoinfo.go render those fields only when present. Ensure the
existing helpers that run rev-list, branch, and tag map failures to nil rather
than zero so users can distinguish failure from an empty repository.
- Around line 240-249: BranchCount is inflated because repoinfo.go counts every
non-empty line from run(ctx, dir, "branch", "-a"), including symbolic HEAD refs
and remote-tracking duplicates. Update the branch-counting logic in the repoinfo
collection code to ignore the remotes/.../HEAD -> ... line and dedupe local
branches against their remote-tracking counterparts before assigning
info.BranchCount. Use the existing run call and the count-building block in the
repoinfo function as the place to apply the filtering.
In `@internal/zerogit/zerogit.go`:
- Around line 559-565: The branch resolution in PushResult handling accepts the
literal "HEAD" from gitOutput when HEAD is detached, which later causes a bad
git push. Update the branch lookup logic where options.Branch is trimmed and
resolved via "rev-parse --abbrev-ref HEAD" to detect the detached-HEAD case
explicitly; if the resolved value is "HEAD" (or otherwise not a real branch),
return a clear error like “cannot push: not currently on a branch” before
attempting the push.
---
Nitpick comments:
In `@internal/cli/workflows.go`:
- Around line 478-480: The pr workflow currently rejects --remote and --force in
the option validation and then runChangesPR always does a plain push, which
blocks force-pushes or non-default remotes after rebases. Update the command
handling in parse/workflows around the options check and runChangesPR so pr
accepts these flags and passes them through to zerogit.PushOptions for the
implicit push, keeping the existing push behavior as the default when flags are
not set.
In `@internal/repoinfo/repoinfo_test.go`:
- Around line 46-47: The repoinfo test fixture only covers a simple local branch
plus one remote branch, so it misses the symbolic-ref overcount case in
BranchCount. Update the canned branch output used by the repoinfo_test.go cases
to include a remotes/origin/HEAD -> origin/main line, and add/adjust the
assertion around BranchCount so it verifies the symbolic-ref is not counted as
an extra branch. Use the existing repoinfo/BranchCount test setup to locate and
expand the case.
In `@internal/repoinfo/repoinfo.go`:
- Around line 240-259: The branch and tag counting blocks in repoinfoInfo use
the same non-empty-line counting logic twice, so extract that shared loop into a
small helper like countNonEmptyLines and reuse it from the existing run(ctx,
dir, "branch", "-a") and run(ctx, dir, "tag") paths. Keep the helper near the
repo info counting code so BranchCount and TagCount are set from the same
centralized logic and the repeated strings.Split/strings.TrimSpace/count pattern
is removed.
In `@internal/zerogit/zerogit_test.go`:
- Around line 525-549: The current TestPushBranchesToRemote only verifies the
happy path with an existing upstream remote, so expand it to cover the missing
Push behavior. Add cases around Push/PushOptions and fakeRunner to assert that
setting Force and DryRun results in the expected git push args
(--force-with-lease and --dry-run), and add a scenario where the
branch.<name>.remote lookup returns empty or fails so the fallback-to-origin
path in Push is exercised and verified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a17244d4-b307-492b-b498-9a379a39b12d
📒 Files selected for processing (9)
internal/cli/app.gointernal/cli/repoinfo.gointernal/cli/repoinfo_test.gointernal/cli/workflow_test.gointernal/cli/workflows.gointernal/repoinfo/repoinfo.gointernal/repoinfo/repoinfo_test.gointernal/zerogit/zerogit.gointernal/zerogit/zerogit_test.go
| dryRunStr := "" | ||
| if options.dryRun { | ||
| dryRunStr = " (dry run)" | ||
| } | ||
| fmt.Fprintf(stdout, "Pushed branch %s to remote %s%s\n", result.Branch, result.Remote, dryRunStr) | ||
| if result.Output != "" { | ||
| fmt.Fprintln(stdout, result.Output) | ||
| } | ||
| return exitSuccess | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
New handlers ignore Fprintf/Fprintln write errors.
Every other subcommand handler in this file (e.g. Lines 204, 225) checks the error from stdout writes and returns exitCrash on failure. runChangesPush (Lines 821-824) and runChangesPR (Lines 844, 851, 890) do not, so a broken pipe or write failure silently falls through to exitSuccess. Minor in practice, but inconsistent with the file's established convention.
Also applies to: 844-891
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cli/workflows.go` around lines 817 - 826, The new handlers in
runChangesPush and runChangesPR are ignoring stdout write failures, which is
inconsistent with the rest of workflows.go. Update the fmt.Fprintf/fmt.Fprintln
calls in those handlers to check their returned errors and return exitCrash on
failure, matching the pattern used by the other subcommand handlers in this
file; use the existing runChangesPush and runChangesPR functions as the places
to fix.
| func runChangesPR(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { | ||
| options, help, err := parseChangesArgs(args, "pr") | ||
| if err != nil { | ||
| return writeExecUsageError(stderr, err.Error()) | ||
| } | ||
| if help { | ||
| if err := writeChangesHelp(stdout); err != nil { | ||
| return exitCrash | ||
| } | ||
| return exitSuccess | ||
| } | ||
| workspaceRoot, err := resolveWorkspaceRoot(options.cwd, deps) | ||
| if err != nil { | ||
| return writeExecUsageError(stderr, err.Error()) | ||
| } | ||
|
|
||
| fmt.Fprintln(stdout, "Pushing current branch to set upstream...") | ||
| pushResult, err := deps.pushChanges(context.Background(), zerogit.PushOptions{ | ||
| Cwd: workspaceRoot, | ||
| }) | ||
| if err != nil { | ||
| return writeExecUsageError(stderr, fmt.Sprintf("auto-push failed: %v", err)) | ||
| } | ||
| fmt.Fprintf(stdout, "Pushed branch %s to remote %s\n", pushResult.Branch, pushResult.Remote) | ||
|
|
||
| prArgs := []string{"pr", "create"} | ||
| if options.fill { | ||
| prArgs = append(prArgs, "--fill") | ||
| } | ||
| if options.draft { | ||
| prArgs = append(prArgs, "--draft") | ||
| } | ||
| if options.title != "" { | ||
| prArgs = append(prArgs, "--title", options.title) | ||
| } | ||
| if options.body != "" { | ||
| prArgs = append(prArgs, "--body", options.body) | ||
| } | ||
|
|
||
| cmd := exec.CommandContext(context.Background(), "gh", prArgs...) | ||
| cmd.Dir = workspaceRoot | ||
| var ghStdout, ghStderr bytes.Buffer | ||
| cmd.Stdout = &ghStdout | ||
| cmd.Stderr = &ghStderr | ||
|
|
||
| err = cmd.Run() | ||
| if err != nil { | ||
| return writeExecUsageError(stderr, fmt.Sprintf("gh pr create failed: %s\n%s", err.Error(), ghStderr.String())) | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
gh pr create invocation is not injectable, unlike every other external side effect in this file.
inspectChanges, commitChanges, and now pushChanges are all dependency-injected via appDeps for testability. runChangesPR instead calls exec.CommandContext(context.Background(), "gh", prArgs...) directly. This is a real gap: there is no way to unit-test PR-argument construction (--fill/--draft/--title/--body) or the resulting JSON/text output without actually invoking the gh binary — and indeed workflow_test.go has no TestRunChangesPR at all, unlike the sibling TestRunChangesPush. Following the established pattern (a createPR func(...) (...) field on appDeps, defaulted to a real implementation) would make this feature testable and consistent with the rest of the codebase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cli/workflows.go` around lines 828 - 877, `runChangesPR` invokes `gh
pr create` directly, unlike the other injectable side effects in `appDeps`,
which makes PR creation logic hard to unit test. Add a `createPR` dependency to
`appDeps` (similar to `inspectChanges`, `commitChanges`, and `pushChanges`),
move the `exec.CommandContext` logic behind that method, and have `runChangesPR`
call the injected helper so tests can verify `--fill`, `--draft`, `--title`, and
`--body` argument construction without requiring the `gh` binary.
| fmt.Fprintln(stdout, "Pushing current branch to set upstream...") | ||
| pushResult, err := deps.pushChanges(context.Background(), zerogit.PushOptions{ | ||
| Cwd: workspaceRoot, | ||
| }) | ||
| if err != nil { | ||
| return writeExecUsageError(stderr, fmt.Sprintf("auto-push failed: %v", err)) | ||
| } | ||
| fmt.Fprintf(stdout, "Pushed branch %s to remote %s\n", pushResult.Branch, pushResult.Remote) | ||
|
|
||
| prArgs := []string{"pr", "create"} | ||
| if options.fill { | ||
| prArgs = append(prArgs, "--fill") | ||
| } | ||
| if options.draft { | ||
| prArgs = append(prArgs, "--draft") | ||
| } | ||
| if options.title != "" { | ||
| prArgs = append(prArgs, "--title", options.title) | ||
| } | ||
| if options.body != "" { | ||
| prArgs = append(prArgs, "--body", options.body) | ||
| } | ||
|
|
||
| cmd := exec.CommandContext(context.Background(), "gh", prArgs...) | ||
| cmd.Dir = workspaceRoot | ||
| var ghStdout, ghStderr bytes.Buffer | ||
| cmd.Stdout = &ghStdout | ||
| cmd.Stderr = &ghStderr | ||
|
|
||
| err = cmd.Run() | ||
| if err != nil { | ||
| return writeExecUsageError(stderr, fmt.Sprintf("gh pr create failed: %s\n%s", err.Error(), ghStderr.String())) | ||
| } | ||
|
|
||
| if options.json { | ||
| res := map[string]string{ | ||
| "branch": pushResult.Branch, | ||
| "remote": pushResult.Remote, | ||
| "output": strings.TrimSpace(ghStdout.String()), | ||
| } | ||
| if err := writePrettyJSON(stdout, res); err != nil { | ||
| return exitCrash | ||
| } | ||
| return exitSuccess | ||
| } | ||
|
|
||
| fmt.Fprintln(stdout, strings.TrimSpace(ghStdout.String())) | ||
| return exitSuccess |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--json output for zero changes pr is not valid JSON.
Lines 844 and 851 unconditionally write plain-text status lines to stdout before the options.json check at Line 878. When --json is passed, stdout will contain "Pushing current branch to set upstream..." and "Pushed branch ... to remote ..." followed by the actual JSON blob — breaking any machine parsing of the output. Compare with runChangesPush, which correctly defers all text output until after the JSON branch is handled.
🐛 Proposed fix
- fmt.Fprintln(stdout, "Pushing current branch to set upstream...")
+ if !options.json {
+ fmt.Fprintln(stdout, "Pushing current branch to set upstream...")
+ }
pushResult, err := deps.pushChanges(context.Background(), zerogit.PushOptions{
Cwd: workspaceRoot,
})
if err != nil {
return writeExecUsageError(stderr, fmt.Sprintf("auto-push failed: %v", err))
}
- fmt.Fprintf(stdout, "Pushed branch %s to remote %s\n", pushResult.Branch, pushResult.Remote)
+ if !options.json {
+ fmt.Fprintf(stdout, "Pushed branch %s to remote %s\n", pushResult.Branch, pushResult.Remote)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fmt.Fprintln(stdout, "Pushing current branch to set upstream...") | |
| pushResult, err := deps.pushChanges(context.Background(), zerogit.PushOptions{ | |
| Cwd: workspaceRoot, | |
| }) | |
| if err != nil { | |
| return writeExecUsageError(stderr, fmt.Sprintf("auto-push failed: %v", err)) | |
| } | |
| fmt.Fprintf(stdout, "Pushed branch %s to remote %s\n", pushResult.Branch, pushResult.Remote) | |
| prArgs := []string{"pr", "create"} | |
| if options.fill { | |
| prArgs = append(prArgs, "--fill") | |
| } | |
| if options.draft { | |
| prArgs = append(prArgs, "--draft") | |
| } | |
| if options.title != "" { | |
| prArgs = append(prArgs, "--title", options.title) | |
| } | |
| if options.body != "" { | |
| prArgs = append(prArgs, "--body", options.body) | |
| } | |
| cmd := exec.CommandContext(context.Background(), "gh", prArgs...) | |
| cmd.Dir = workspaceRoot | |
| var ghStdout, ghStderr bytes.Buffer | |
| cmd.Stdout = &ghStdout | |
| cmd.Stderr = &ghStderr | |
| err = cmd.Run() | |
| if err != nil { | |
| return writeExecUsageError(stderr, fmt.Sprintf("gh pr create failed: %s\n%s", err.Error(), ghStderr.String())) | |
| } | |
| if options.json { | |
| res := map[string]string{ | |
| "branch": pushResult.Branch, | |
| "remote": pushResult.Remote, | |
| "output": strings.TrimSpace(ghStdout.String()), | |
| } | |
| if err := writePrettyJSON(stdout, res); err != nil { | |
| return exitCrash | |
| } | |
| return exitSuccess | |
| } | |
| fmt.Fprintln(stdout, strings.TrimSpace(ghStdout.String())) | |
| return exitSuccess | |
| if !options.json { | |
| fmt.Fprintln(stdout, "Pushing current branch to set upstream...") | |
| } | |
| pushResult, err := deps.pushChanges(context.Background(), zerogit.PushOptions{ | |
| Cwd: workspaceRoot, | |
| }) | |
| if err != nil { | |
| return writeExecUsageError(stderr, fmt.Sprintf("auto-push failed: %v", err)) | |
| } | |
| if !options.json { | |
| fmt.Fprintf(stdout, "Pushed branch %s to remote %s\n", pushResult.Branch, pushResult.Remote) | |
| } | |
| prArgs := []string{"pr", "create"} | |
| if options.fill { | |
| prArgs = append(prArgs, "--fill") | |
| } | |
| if options.draft { | |
| prArgs = append(prArgs, "--draft") | |
| } | |
| if options.title != "" { | |
| prArgs = append(prArgs, "--title", options.title) | |
| } | |
| if options.body != "" { | |
| prArgs = append(prArgs, "--body", options.body) | |
| } | |
| cmd := exec.CommandContext(context.Background(), "gh", prArgs...) | |
| cmd.Dir = workspaceRoot | |
| var ghStdout, ghStderr bytes.Buffer | |
| cmd.Stdout = &ghStdout | |
| cmd.Stderr = &ghStderr | |
| err = cmd.Run() | |
| if err != nil { | |
| return writeExecUsageError(stderr, fmt.Sprintf("gh pr create failed: %s\n%s", err.Error(), ghStderr.String())) | |
| } | |
| if options.json { | |
| res := map[string]string{ | |
| "branch": pushResult.Branch, | |
| "remote": pushResult.Remote, | |
| "output": strings.TrimSpace(ghStdout.String()), | |
| } | |
| if err := writePrettyJSON(stdout, res); err != nil { | |
| return exitCrash | |
| } | |
| return exitSuccess | |
| } | |
| fmt.Fprintln(stdout, strings.TrimSpace(ghStdout.String())) | |
| return exitSuccess |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cli/workflows.go` around lines 844 - 891, The pr creation flow in
`runChangesPr` is emitting human-readable status lines before the `options.json`
branch, which makes `--json` output invalid. Move or gate the
`fmt.Fprintln`/`fmt.Fprintf` status messages so they only run in the non-JSON
path, matching the pattern used by `runChangesPush`, and ensure
`writePrettyJSON` is the only thing written to stdout when `options.json` is
true.
| BranchCount int `json:"branchCount"` | ||
| CommitCount int `json:"commitCount"` | ||
| TagCount int `json:"tagCount"` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
New count fields silently show 0 on git-command failure instead of surfacing "unknown".
BranchCount/CommitCount/TagCount are plain int, unlike the existing optional metrics (AgeDays, Contributors90d, CommitVelocity30d) which are *int and only populated when the corresponding git command succeeds. If rev-list, branch, or tag fail here, the field silently stays 0, and formatRepoInfo (internal/cli/repoinfo.go, lines 91-93) prints e.g. Commits: 0 unconditionally — indistinguishable from a genuinely empty repo. Consider following the same *int + nil-check pattern used for the other soft-fail metrics.
Also applies to: 235-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/repoinfo/repoinfo.go` around lines 57 - 59, The new repo count
metrics are using plain ints, so git-command failures are hidden as 0 instead of
“unknown.” Update the repo info model in repoinfo.go so BranchCount,
CommitCount, and TagCount follow the same optional pattern as AgeDays,
Contributors90d, and CommitVelocity30d, then adjust the count-gathering logic to
leave them nil on command failure and have formatRepoInfo in repoinfo.go render
those fields only when present. Ensure the existing helpers that run rev-list,
branch, and tag map failures to nil rather than zero so users can distinguish
failure from an empty repository.
| if branches, err := run(ctx, dir, "branch", "-a"); err == nil { | ||
| lines := strings.Split(branches, "\n") | ||
| count := 0 | ||
| for _, line := range lines { | ||
| if strings.TrimSpace(line) != "" { | ||
| count++ | ||
| } | ||
| } | ||
| info.BranchCount = count | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
BranchCount overcounts: includes remote-tracking branches and the symbolic HEAD ref line.
git branch -a output typically looks like:
* main
remotes/origin/HEAD -> origin/main
remotes/origin/main
Counting every non-empty trimmed line here treats the remotes/origin/HEAD -> origin/main symbolic pointer as a branch, and separately counts a local branch and its remote-tracking counterpart as two branches, both of which inflate the reported count beyond what a user would expect as "number of branches."
🔧 Suggested fix: filter symbolic refs and dedupe local vs. remote-tracking names
if branches, err := run(ctx, dir, "branch", "-a"); err == nil {
- lines := strings.Split(branches, "\n")
- count := 0
- for _, line := range lines {
- if strings.TrimSpace(line) != "" {
- count++
- }
- }
- info.BranchCount = count
+ names := map[string]bool{}
+ for _, line := range strings.Split(branches, "\n") {
+ line = strings.TrimSpace(strings.TrimPrefix(strings.TrimSpace(line), "* "))
+ if line == "" || strings.Contains(line, "->") {
+ continue
+ }
+ names[strings.TrimPrefix(line, "remotes/")] = true
+ }
+ info.BranchCount = len(names)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if branches, err := run(ctx, dir, "branch", "-a"); err == nil { | |
| lines := strings.Split(branches, "\n") | |
| count := 0 | |
| for _, line := range lines { | |
| if strings.TrimSpace(line) != "" { | |
| count++ | |
| } | |
| } | |
| info.BranchCount = count | |
| } | |
| if branches, err := run(ctx, dir, "branch", "-a"); err == nil { | |
| names := map[string]bool{} | |
| for _, line := range strings.Split(branches, "\n") { | |
| line = strings.TrimSpace(strings.TrimPrefix(strings.TrimSpace(line), "* ")) | |
| if line == "" || strings.Contains(line, "->") { | |
| continue | |
| } | |
| names[strings.TrimPrefix(line, "remotes/")] = true | |
| } | |
| info.BranchCount = len(names) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/repoinfo/repoinfo.go` around lines 240 - 249, BranchCount is
inflated because repoinfo.go counts every non-empty line from run(ctx, dir,
"branch", "-a"), including symbolic HEAD refs and remote-tracking duplicates.
Update the branch-counting logic in the repoinfo collection code to ignore the
remotes/.../HEAD -> ... line and dedupe local branches against their
remote-tracking counterparts before assigning info.BranchCount. Use the existing
run call and the count-building block in the repoinfo function as the place to
apply the filtering.
| branch := strings.TrimSpace(options.Branch) | ||
| if branch == "" { | ||
| branch, err = gitOutput(ctx, runGit, root, "rev-parse", "--abbrev-ref", "HEAD") | ||
| if err != nil { | ||
| return PushResult{}, fmt.Errorf("resolve current branch: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Detached HEAD resolves to literal branch name "HEAD".
When options.Branch is empty and HEAD is detached, rev-parse --abbrev-ref HEAD returns the literal string "HEAD", which then gets pushed as git push -u <remote> HEAD. Git will reject this with a fairly opaque error, wrapped generically as "push: ...". Consider detecting this case up front and returning a clear, actionable error (e.g. "cannot push: not currently on a branch").
💡 Proposed fix
branch := strings.TrimSpace(options.Branch)
if branch == "" {
branch, err = gitOutput(ctx, runGit, root, "rev-parse", "--abbrev-ref", "HEAD")
if err != nil {
return PushResult{}, fmt.Errorf("resolve current branch: %w", err)
}
+ if branch == "HEAD" {
+ return PushResult{}, fmt.Errorf("cannot push: HEAD is detached, not on a branch")
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| branch := strings.TrimSpace(options.Branch) | |
| if branch == "" { | |
| branch, err = gitOutput(ctx, runGit, root, "rev-parse", "--abbrev-ref", "HEAD") | |
| if err != nil { | |
| return PushResult{}, fmt.Errorf("resolve current branch: %w", err) | |
| } | |
| } | |
| branch := strings.TrimSpace(options.Branch) | |
| if branch == "" { | |
| branch, err = gitOutput(ctx, runGit, root, "rev-parse", "--abbrev-ref", "HEAD") | |
| if err != nil { | |
| return PushResult{}, fmt.Errorf("resolve current branch: %w", err) | |
| } | |
| if branch == "HEAD" { | |
| return PushResult{}, fmt.Errorf("cannot push: HEAD is detached, not on a branch") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/zerogit/zerogit.go` around lines 559 - 565, The branch resolution in
PushResult handling accepts the literal "HEAD" from gitOutput when HEAD is
detached, which later causes a bad git push. Update the branch lookup logic
where options.Branch is trimmed and resolved via "rev-parse --abbrev-ref HEAD"
to detect the detached-HEAD case explicitly; if the resolved value is "HEAD" (or
otherwise not a real branch), return a clear error like “cannot push: not
currently on a branch” before attempting the push.
This PR adds the push and pr subcommands to zero changes command suite, allowing developers to push changes and create GitHub PRs directly. It also adds BranchCount, CommitCount, and TagCount metrics to the zero repo-info command.
Summary by CodeRabbit
New Features
changes pushandchanges prworkflows to the CLI.Bug Fixes
Tests