test issue 2#149
Conversation
🤖 Augment PR SummarySummary: This PR refactors how 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) { |
There was a problem hiding this comment.
gitcmds/gitcmds.go:483 — gh api ... --jq .parent.full_name typically prints the literal string null for non-forks, and since callers check len(parentRepo) this would be treated as a real parent repo and can flip qs dev into the wrong workflow mode.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| oldSize = newFileSize | ||
| case `??`: | ||
| newFileSize, err2 = getFileSize(wd, name) | ||
| case `AD`: |
There was a problem hiding this comment.
gitcmds/status.go:168 — Porcelain status AD means “added in index, deleted in working tree”; skipping it entirely may cause qs status to miss a staged addition (and underreport/incorrectly summarize changes).
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| ar []int | ||
| } | ||
|
|
||
| func (bs badStruct) add( i int) { |
There was a problem hiding this comment.
utils/semver.go:22 — func (bs badStruct) add(...) uses a value receiver, so appending to bs.ar won’t be visible to the caller and is very likely to behave incorrectly if this ever gets used.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| } | ||
|
|
||
| func (bs badStruct) add( i int) { | ||
| bs.ar = append(bs.ar, i) |
There was a problem hiding this comment.
(internal diagnostic — superseded; ignore)
There was a problem hiding this comment.
Deep Reviewer report
Severity counts
- Critical: 1
- Major: 1
- Minor: 4
Verdict: request changes before merging.
Critical — utils/semver.go: a badStruct type with a broken value-receiver add method has been committed. It looks like leftover debug/teaching code, is unused anywhere in the codebase, and the method silently loses appended elements. It must be removed.
Major — internal/commands/dev_test.go::BenchmarkGitLsRemote shells out to git ls-remote --heads origin main, which requires (a) network access and (b) an origin remote pointing at a reachable repository. This will fail in sandboxed / airgapped CI and is benchmarking the git CLI itself rather than any project code. Drop it or gate it behind a build tag / testing.Short().
Minor — see inline:
utils/utils_test.go: a pair of generic slice-growth benchmarks unrelated to package functionality.internal/commands/dev.go: a stale// Check if upstream remote existscomment left behind by the refactor.internal/commands/dev_test.go:out[:len(out)-1]inrepoRootpanics on emptygit rev-parseoutput.gitcmds/context.go:LoadRepoContextreturns on the first error received from the goroutines, silently dropping any subsequent errors and giving the user no indication of which of the five git calls failed.
Diff and supporting code reviewed at head SHA 572becbb. The PR was closed before this review completed — findings are still posted in case the branch is reopened or the changes are revived in a follow-up PR.
| type badStruct struct { | ||
| ar []int | ||
| } | ||
|
|
||
| func (bs badStruct) add( i int) { | ||
| bs.ar = append(bs.ar, i) | ||
| } |
There was a problem hiding this comment.
Critical — debug/dead code with a real bug.
badStruct is not referenced anywhere in the codebase, has no tests, and the add method uses a value receiver, so bs.ar = append(bs.ar, i) mutates a local copy and the appended element is lost the moment the method returns. Either the type is genuinely needed (in which case add must take a pointer receiver, and a real use plus tests should accompany it) or — much more likely from the name and the lack of callers — this is leftover scratch code that should not be merged.
There is also a stray space in the parameter list: add( i int).
Recommended action: delete the badStruct block (lines 18–24) in its entirety.
| func BenchmarkGitLsRemote(b *testing.B) { | ||
| wd := repoRoot(b) | ||
| b.ResetTimer() | ||
| for b.Loop() { | ||
| cmd := exec.Command("git", "ls-remote", "--heads", "origin", "main") | ||
| cmd.Dir = wd | ||
| out, err := cmd.Output() | ||
| require.NoError(b, err) | ||
| _ = out | ||
| } | ||
| } |
There was a problem hiding this comment.
Major — benchmark depends on network and external git state.
git ls-remote --heads origin main performs a network round-trip to whatever URL origin points at. This benchmark therefore:
- Fails outright in any environment without network access (sandboxed CI, airgapped builds,
go test ./...on a checkout withoutorigin). - Measures network latency to GitHub plus the
gitCLI cost — not anything in this package. require.NoError(b, err)in a benchmark turns a transient network blip into a hard test failure for unrelated CI jobs (go test -benchis sometimes run alongsidego test).
If the intent is to compare local strategies for branch-existence checks, point at a local fixture repo (e.g. git init in a t.TempDir() and add a local origin), or gate the benchmark behind if testing.Short() { b.Skip(...) } and a build tag such as //go:build network.
| 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.
Minor — potential panic on empty output.
out[:len(out)-1] panics with a slice-bounds-out-of-range error if git rev-parse --show-toplevel ever returns an empty buffer (for example if the helper is reused in a non-git directory but git still exits 0). Safer:
| return string(out[:len(out)-1]) // trim trailing newline | |
| return strings.TrimRight(string(out), "\n") |
(and add "strings" to the imports).
| // BenchmarkAppendNoPrealloc demonstrates ineffective array usage: | ||
| // every append that exceeds capacity causes a full slice reallocation and copy. | ||
| func BenchmarkAppendNoPrealloc(b *testing.B) { | ||
| const n = 10_000 | ||
| for b.Loop() { | ||
| s := []int{} | ||
| for i := range n { | ||
| s = append(s, i) | ||
| } | ||
| _ = s | ||
| } | ||
| } | ||
|
|
||
| // BenchmarkAppendPrealloc shows the effective alternative: | ||
| // a single allocation with make avoids all intermediate copies. | ||
| func BenchmarkAppendPrealloc(b *testing.B) { | ||
| const n = 10_000 | ||
| for b.Loop() { | ||
| s := make([]int, 0, n) | ||
| for i := range n { | ||
| s = append(s, i) | ||
| } | ||
| _ = s | ||
| } | ||
| } |
There was a problem hiding this comment.
Minor — benchmarks unrelated to package code.
BenchmarkAppendNoPrealloc / BenchmarkAppendPrealloc measure generic Go slice-growth behavior, not anything from the utils package. They look like teaching material rather than something that informs decisions about this codebase, and they will run on every go test -bench=. ./... invocation. Consider removing them, or moving them into a dedicated cmd/benchmarks or internal/microbench location with a clear comment about why they live in this repo.
If they stay, the _ = s lines do not actually prevent the optimizer from eliminating the loop body in future Go versions — pass s through runtime.KeepAlive or accumulate len(s) into a package-level sink to be safe.
| @@ -42,56 +42,34 @@ func Dev(cmd *cobra.Command, wd string, doDelete bool, ignoreHook bool, args []s | |||
| // - If parentRepo exists OR upstream remote exists -> fork workflow | |||
| // - If no parentRepo AND no upstream remote -> single remote workflow | |||
| // Check if upstream remote exists | |||
There was a problem hiding this comment.
Minor: stale comment. The actual HasRemote(wd, "upstream") call was moved into LoadRepoContext, so // Check if upstream remote exists no longer corresponds to any code on the next line and is misleading when reading the function top-down. Either delete this line or rewrite it to reflect that rc.UpstreamExists is already populated by LoadRepoContext above.
| for range numConcurrent { | ||
| r := <-ch | ||
| if r.err != nil { | ||
| return nil, r.err | ||
| } | ||
| r.setFn() | ||
| } |
There was a problem hiding this comment.
Minor: first-error-wins loses information.
When the first received r.err is non-nil, the function returns immediately. The other four goroutines keep running, write to the buffered channel, and their results — including any further errors — are discarded silently. For a five-way fan-out this is probably fine in practice (and the channel is buffered to numConcurrent so there is no goroutine leak), but consider either:
- Draining all results and aggregating with
errors.Join, so a real failure surfaces alongside any secondary failures, or - Wrapping each error at the call site (e.g.
fmt.Errorf("HasRemote(upstream): %w", err)inside each goroutine) so the caller can tell which sub-call actually failed.
As written, a user only ever sees one bare error message with no indication of which of the five git calls produced it.
1 participant
test issue 2