test issue 1#148
Conversation
🤖 Augment PR SummarySummary: This PR refactors Changes:
Technical Notes: 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| // getParentRepoByOrgRepo fetches the parent repository full name via the GitHub API. | ||
| func getParentRepoByOrgRepo(wd, org, repo string) (string, error) { | ||
| stdout, stderr, err := new(exec.PipedExec). | ||
| Command("gh", "api", "repos/"+org+slash+repo, "--jq", ".parent.full_name"). |
There was a problem hiding this comment.
gitcmds/gitcmds.go:485 — gh api --jq '.parent.full_name' can output the literal null when the repo isn’t a fork, so callers that use len(parentRepo)==0 may mis-detect fork vs single-remote mode. Consider normalizing this helper’s output (e.g., treat null/whitespace as empty) before returning.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| b.Helper() | ||
| out, err := exec.Command("git", "rev-parse", "--show-toplevel").Output() | ||
| require.NoError(b, err) | ||
| return string(out[:len(out)-1]) // trim trailing newline |
There was a problem hiding this comment.
internal/commands/dev_test.go:27 — out[:len(out)-1] assumes a trailing \n and can mis-trim on \r\n (and can panic if output is unexpectedly empty). Consider trimming newlines/whitespace more defensively when computing the repo root for this benchmark.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Deep review of PR #148.
The main change introduces LoadRepoContext (gitcmds/context.go) which fans out five independent local-git lookups in parallel and then makes one gh api call that needs Org/Repo from phase 1. The optimization is sound: error propagation is correct (errors from any goroutine surface and the partially-populated RepoContext is discarded), and the caller in internal/commands/dev.go is refactored cleanly to use the cached values. Behavior is preserved relative to the previous serial implementation. Two ancillary changes (gitcmds/status.go adds an AD case; gitcmds/types.go adds the RepoContext struct) are coherent with that goal.
Findings:
- 0 blockers
- 1 medium: fragile concurrency-count constant in
LoadRepoContext— silent result loss or goroutine leak if the goroutine count andnumConcurrentever drift - 3 low: unrelated network-dependent benchmark in
dev_test.go; unsafe trailing-newline slice in the same file; narrowAD-only handling instatus.go(sibling*Dcodes still error) - 1 nit: stray blank line in
utils/semver.go
Verdict: safe to merge after the medium-severity concurrency comment is addressed; the rest are quality improvements.
| type result struct { | ||
| setFn func() | ||
| err error | ||
| } |
There was a problem hiding this comment.
The buffered channel capacity and the receive-loop count are both hard-coded to numConcurrent = 5 and must match the number of goroutines launched below. If a future change adds a 6th goroutine without bumping this constant, the 6th send blocks forever after the loop has drained the first 5 (goroutine leak). If a goroutine is removed without decrementing it, the main goroutine blocks forever waiting for a result that never arrives (deadlock).
Consider building the work list as a []func() error (or a slice of result-producing closures) and deriving both the channel capacity and the loop bound from len(tasks) so the count cannot drift. Alternatively, use golang.org/x/sync/errgroup to manage the goroutines and propagate the first error.
| oldSize = newFileSize | ||
| case `??`: | ||
| newFileSize, err2 = getFileSize(wd, name) | ||
| case `AD`: |
There was a problem hiding this comment.
case `AD`:
// file was added, then removed -> consider it never existed
continueThe AD handling is correct, but the fix is narrow: other valid two-letter codes from git status --porcelain for a worktree-deleted entry — RD (renamed in index, deleted in worktree) and CD (copied in index, deleted in worktree) — still fall through to default and return unknown file status %s for file %s, which would surface as an error from Status() for users in those states.
If this scenario is reachable in practice, consider handling the worktree-deleted family generically (e.g. any XD where X is in AMRC) rather than enumerating AD only. If you intentionally want to defer those, a brief comment to that effect would help.
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func BenchmarkGitLsRemote(b *testing.B) { |
There was a problem hiding this comment.
Two concerns with this benchmark:
- It shells out to
git ls-remote --heads origin mainon every iteration, so it is I/O- and network-bound rather than measuring anything in this package, and will fail outright in offline environments or in any clone that does not have anoriginremote with amainbranch (both are realistic in CI). Consider gating withtesting.Short()/ explicit skip when the prerequisites are absent. - The file is named
dev_test.goand lives ininternal/commands, but nothing here exercisesDev()or any symbol from the package. If the benchmark was kept only for ad-hoc profiling, consider removing it or relocating it to a clearly-scoped file (for examplegit_perf_test.goin an appropriate package).
| out, err := exec.Command("git", "rev-parse", "--show-toplevel").Output() | ||
| require.NoError(b, err) | ||
| return string(out[:len(out)-1]) // trim trailing newline | ||
| } |
There was a problem hiding this comment.
return string(out[:len(out)-1]) // trim trailing newlineIf out is ever empty (len(out) == 0), this expression evaluates to out[:-1] and panics with slice bounds out of range. require.NoError only guards against a non-nil error, not against an empty-but-successful stdout. A safer, more portable form (also handles \r\n if the command ever emits CRLF):
| } | |
| return strings.TrimRight(string(out), "\r\n") |
(remember to add "strings" to the import block).
| writeFilePermission = 0644 | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Stray empty line — no behavior change, but worth dropping to keep the file tidy.
1 participant
test issue 1