Skip to content

perf(gitpr): fetch the merge plan in one request, and show progress - #181

Merged
MCamner merged 1 commit into
mainfrom
feat/merge-safe-progress
Aug 8, 2026
Merged

perf(gitpr): fetch the merge plan in one request, and show progress#181
MCamner merged 1 commit into
mainfrom
feat/merge-safe-progress

Conversation

@MCamner

@MCamner MCamner commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Follow-up to #180: wire ui_spinner into the PR-merge tool. Measuring the calls first changed what the fix should be.

The wait was nine round trips, not one slow one

gitpr-merge-safe.sh read nine PR fields with nine separate gh pr view calls, one per field:

9 separate pr_field calls: 3.44s
1 batched call:            0.47s

A spinner makes a wait legible. Not waiting is better. So the batching is the fix and the spinner covers what is left. Two more redundant calls went with it: a separate gh pr view --json number existence check that said what the plan read already says by failing, and gh pr checks running twice — once to print, once to decide.

Measured end to end against live gh: the whole plan phase now takes 0.99s, spinner included.

The delimiter is load-bearing

The batched fields are joined on U+001F, not a tab. Tab counts as IFS whitespace, so read collapses a run of them and one empty field shifts every later field left:

$ printf 'a\tb\t\tc\n' | { IFS=$'\t' read -r w x y z; echo "w=[$w] x=[$x] y=[$y] z=[$z]"; }
w=[a] x=[b] y=[c] z=[]        # <- c landed in y, z is gone

reviewDecision is empty on any PR nobody reviewed, which is most of them — so a tab-separated read would have printed the URL as the review decision. Confirmed against live gh on #180, where reviewDecision is in fact empty:

  Review:   none
  URL:      https://github.com/MCamner/macos-scripts/pull/180

join also refuses to join null, so nulls map to "" first.

Sourcing

The git menu launches this script as its own process (( cd "$CURRENT_REPO" && "$merge_script" )), it never sources it — so the script loads mq-ui.sh itself and falls back to a no-op ui_spinner when the library is unreachable. A missing spinner must not cost you the merge tool. Step 6 pins that.

Test coverage

First coverage this script has ever had; gitmerge-safe-smoke.sh covers the local-merge sibling, not this one. It matters here more than most — this is the one script in the repo that can change a remote branch.

The fake gh records every invocation, so "one call, not nine" is asserted against behaviour rather than against how the source happens to read. Also pinned: the no-TTY guardrail, that the confirmation can be declined, and that declining means gh pr merge was never called.

$ MACOS_SCRIPTS_HOME="$PWD" MQ_NO_TUI=1 ./tools/scripts/test-all.sh
[PASS] Shell lint passed at warning severity (197 files)
[PASS] All selftest checks passed.

🤖 Generated with Claude Code

Wrapping the gh calls in ui_spinner was the ask; measuring them first changed
what the fix should be. Reading the nine fields of the merge plan took nine
`gh pr view` calls — 3.44s measured, all of it silent — against 0.47s for one
batched read. A spinner makes a wait legible, but not waiting is better, so
the batching is the fix and the spinner covers what is left.

Also folded in: the separate `gh pr view --json number` existence check was a
tenth call saying what the plan read already says by failing, and `gh pr
checks` ran twice, once to print and once to decide.

The batched fields are joined on U+001F rather than a tab. Tab counts as IFS
whitespace, so `read` collapses a run of them and one empty field shifts every
later field left — and reviewDecision is empty on any PR nobody reviewed,
which is most of them. A tab-separated read would have printed the URL as the
review decision. Verified against live gh on #180, where reviewDecision is in
fact empty: "Review: none" and the URL both land correctly.

The script is launched as its own process by the git menu, so it sources
mq-ui.sh itself and falls back to a no-op ui_spinner when the library is not
reachable. A missing spinner must not cost you the merge tool.

First test coverage this script has had — gitmerge-safe-smoke.sh covers the
local-merge sibling, not this one. The fake gh records every invocation, so
"one call, not nine" is asserted against behaviour rather than against how the
source reads, and the no-TTY guardrail and the declined confirmation are
pinned too.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@MCamner
MCamner merged commit 253f2f2 into main Aug 8, 2026
3 checks passed
@MCamner
MCamner deleted the feat/merge-safe-progress branch August 8, 2026 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant