perf: --single-branch upstream clone (follow-up to #19 review) - #20
Conversation
… for out-of-branch refs Addresses the review suggestion on #19: the CI clone is ephemeral, so fetching the full ref namespace every run roughly doubles clone time for nothing — the script targets origin/HEAD or a release tag, both reachable from the default branch. Goes one step beyond the suggestion: with --single-branch, a --ref that is NOT reachable from the default branch (e.g. a manual run against a feature branch) would no longer resolve, so rev-parse now falls back to an explicit fetch of that ref before failing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Same hardening as go-agent#2 review nit: '--' terminator on the fallback fetch and --end-of-options on rev-parse so a manual --ref value starting with '-' cannot be parsed as an option. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@devin please review this PR — head is |
There was a problem hiding this comment.
Perry's Review
Verdict: 💬 Comments / questions
Re-review on 7e51d31 (previous review was a COMMENT on a81fec6). The maintainer app is not installed on this repo, so I cannot post an APPROVE. This is an LGTM — the code is correct and CI is green.
Review details
Change summary
Two targeted changes to the upstream wrapper script:
-
--single-branch on fresh clone — In CI, the upstream checkout is ephemeral (not persisted between runs), so the existing-clone path (fetch with all tags) is never taken. Fetching the full ref namespace every run roughly doubles clone time for nothing, since the script only targets origin/HEAD or a release tag reachable from the default branch. --single-branch fetches only the default branch and its reachable history (including reachable tags).
-
Fallback fetch for unresolvable refs — With --single-branch, a ref not reachable from the default branch (e.g. a manual --ref against a feature branch or an unreachable tag) won't be in the local clone. The new if-guard checks resolution with rev-parse --quiet --verify --end-of-options, and falls back to an explicit git fetch origin plus FETCH_HEAD. The --end-of-options / -- pair prevents a ref starting with a dash from being parsed as an option (CI refs start with @, but manual runs take anything).
Analysis
Verified empirically with a test repo:
- --single-branch fetches tags reachable from the default branch (e.g. v1.0.0 on main resolves locally).
- Tags NOT reachable from the default branch (e.g. v2.0.0 on a feature branch) do NOT resolve — correctly triggers the fallback fetch, which then resolves via FETCH_HEAD.
- origin/HEAD resolves in a --single-branch clone (the else branch for no --ref).
- The existing-clone path (local dev) is unaffected: it still does a full fetch with all tags, so the fallback is a no-op there.
Error handling is correct under set -euo pipefail: if the fallback fetch fails (ref truly doesn't exist on the remote), the script exits cleanly with a git error message.
CI
Both checks pass: check (19s), verify-port (22s).
|
@devin review — second request. Slack pings at ~10:52 and ~10:56 CDT and a GitHub mention at ~11:19 have all gone unanswered on this PR, and Perry has since reviewed the current head. Flagging explicitly rather than letting it sit silent. |
|
Correction to my earlier ping — withdrawing the escalation. Devin is not unresponsive here; it is not installed on this repo. Checked the last 20 PRs in each: zero Devin reviews on That makes Perry-at-head the complete automated coverage available here, not a partial pass waiting on a second reviewer. Apologies for the noise from the repeat If Devin coverage on these repos is wanted, that is a one-time GitHub App install rather than anything to do on this PR. |
Follow-up to the review feedback on #19, which merged before the fix landed on the branch.
scripts/upstreamnow clones the upstream reference--single-branch: the CI clone is ephemeral, so fetching the full ref namespace every run roughly doubled clone time for nothing.--single-branch, a--refnot reachable from the default branch (e.g. a manual run against a feature branch) would stop resolving —rev-parsenow falls back to an explicitgit fetch origin <ref>before failing.🤖 Generated with Claude Code