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

terminal/launchers/gitpr-merge-safe.sh

@@ -31,6 +31,21 @@ require_cmd() {
 require_cmd git
 require_cmd gh
 
+# ui_spinner, so the gh round trips below read as work rather than as a hang.
+# This script runs as its own process (the git menu launches it, never sources
+# it), so it has to load the UI library itself — and degrade to a no-op if the
+# library is not there, because a missing spinner must not cost you the merge
+# tool. MQ_UI_LIB is an override for tests.
+_mq_ui_lib="${MQ_UI_LIB:-$(cd "$(dirname "${BASH_SOURCE[0]}")/../../ui/terminal-ui" 2>/dev/null && pwd)/mq-ui.sh}"
+if [[ -f "$_mq_ui_lib" ]]; then
+  # shellcheck source=../../ui/terminal-ui/mq-ui.sh
+  source "$_mq_ui_lib"
+fi
+if ! declare -f ui_spinner >/dev/null; then
+  ui_spinner() { local _label="$1"; shift; "$@"; }
+fi
+unset _mq_ui_lib
+
 git rev-parse --is-inside-work-tree >/dev/null 2>&1 || die "Not inside a Git repository."
 
 # Class C interactive action: refuse before any network operation if there is no
@@ -57,16 +72,21 @@ pr_number="${1:-}"
 
 if [[ -z "$pr_number" ]]; then
   # Try the PR whose head is the current branch.
-  pr_number="$(gh pr view --json number --jq '.number' 2>/dev/null || true)"
+  pr_number="$(ui_spinner "Looking for a PR on $current_branch" \
+    gh pr view --json number --jq '.number' 2>/dev/null || true)"
 fi
 
 if [[ -z "$pr_number" ]]; then
   yellow "No PR argument and no PR for the current branch. Open pull requests:"
   echo
-  gh pr list --state open --limit 30 \
-    --json number,title,headRefName,baseRefName \
-    --template '{{range .}}{{printf "  %v) #%v  %s  (%s -> %s)\n" .number .number .title .headRefName .baseRefName}}{{end}}' \
+  # Captured rather than streamed, so the spinner has somewhere to sit while
+  # the list is fetched and the list still prints in one piece afterwards.
+  open_prs="$(ui_spinner "Listing open pull requests" \
+    gh pr list --state open --limit 30 \
+      --json number,title,headRefName,baseRefName \
+      --template '{{range .}}{{printf "  %v) #%v  %s  (%s -> %s)\n" .number .number .title .headRefName .baseRefName}}{{end}}')" \
     || die "Could not list pull requests."
+  printf '%s\n' "$open_prs"
   echo
   read -r -p "PR number to merge (or q to quit): " pr_number
   [[ "${pr_number:-}" =~ ^[Qq]$ ]] && exit 0
@@ -77,23 +97,28 @@ fi
 # ------------------------
 # SHOW MERGE PLAN
 # ------------------------
-gh pr view "$pr_number" --json number >/dev/null 2>&1 || die "Could not read PR #$pr_number."
-
-# Read one field at a time via gh's built-in --jq, so we do not depend on a
-# standalone jq binary being installed.
-pr_field() {
-  gh pr view "$pr_number" --json "$1" --jq ".$1" 2>/dev/null || true
-}
-
-title="$(pr_field title)"
-state="$(pr_field state)"
-is_draft="$(pr_field isDraft)"
-base_ref="$(pr_field baseRefName)"
-head_ref="$(pr_field headRefName)"
-mergeable="$(pr_field mergeable)"
-merge_state="$(pr_field mergeStateStatus)"
-review_decision="$(pr_field reviewDecision)"
-pr_url="$(pr_field url)"
+# One request for the whole plan, still through gh's built-in --jq so there is
+# no dependency on a standalone jq binary. This used to be nine `gh pr view`
+# calls, one per field, which measured 3.44s of silence against 0.47s for the
+# batched read. The separate existence check was a tenth call saying what this
+# one already says by failing.
+#
+# Fields are joined on U+001F, not on a tab: tab counts as IFS whitespace, so
+# `read` collapses a run of them and an empty field shifts every later field
+# left. reviewDecision is empty on any PR nobody has reviewed — which is most
+# of them — so a tab-separated read would have silently swapped the review
+# decision for the URL. Nulls are mapped to "" for the same reason: jq's join
+# refuses to join null.
+pr_plan="$(ui_spinner "Reading PR #$pr_number" \
+  gh pr view "$pr_number" \
+    --json title,state,isDraft,baseRefName,headRefName,mergeable,mergeStateStatus,reviewDecision,url \
+    --jq '[.title,.state,.isDraft,.baseRefName,.headRefName,.mergeable,.mergeStateStatus,.reviewDecision,.url]
+          | map(if . == null then "" else tostring end) | join("\u001f")' \
+    2>/dev/null)" || die "Could not read PR #$pr_number."
+[[ -n "$pr_plan" ]] || die "Could not read PR #$pr_number."
+
+IFS=$'\037' read -r title state is_draft base_ref head_ref \
+  mergeable merge_state review_decision pr_url <<<"$pr_plan"
 
 echo
 blue "Merge plan"
@@ -113,9 +138,18 @@ if [[ "$is_draft" == "true" ]]; then
   die "PR #$pr_number is a draft. Mark it ready for review first."
 fi
 
-# CI / check summary.
+# CI / check summary. Fetched once and reused below: this used to run twice,
+# once to print and once to decide, paying for the same round trip either way.
+checks_rc=0
+checks_output="$(ui_spinner "Reading checks for #$pr_number" \
+  gh pr checks "$pr_number" 2>/dev/null)" || checks_rc=$?
+
 yellow "Checks:"
-gh pr checks "$pr_number" 2>/dev/null || yellow "  (no checks reported)"
+if [[ -n "$checks_output" ]]; then
+  printf '%s\n' "$checks_output"
+else
+  yellow "  (no checks reported)"
+fi
 echo
 
 # ------------------------
@@ -134,7 +168,7 @@ if [[ "$review_decision" == "CHANGES_REQUESTED" ]]; then
   yellow "Review decision is CHANGES_REQUESTED."
   risky=1
 fi
-if ! gh pr checks "$pr_number" >/dev/null 2>&1; then
+if [[ "$checks_rc" -ne 0 ]]; then
   yellow "One or more checks are failing or pending."
   risky=1
 fi

tests/gitpr-merge-safe-smoke.sh

@@ -0,0 +1,131 @@
+#!/usr/bin/env bash
+# gitpr-merge-safe.sh closes a remote PR. Until now it had no coverage at all —
+# gitmerge-safe-smoke.sh tests the local-merge sibling, not this one.
+#
+# Two things are pinned here. The guardrails, because this is the one script in
+# the repo that can change a remote branch: no TTY means no merge, and the
+# final confirmation must be answerable with "no". And the shape of the plan
+# fetch, because the reason for touching this file was a wait: reading nine PR
+# fields took nine `gh pr view` calls, 3.44s measured, all of it silent. One
+# batched call measured 0.47s. A spinner makes a wait legible; not making the
+# wait is better, so the batching is the fix and the spinner covers what is
+# left.
+#
+# The fake gh records every invocation, so "one call, not nine" is asserted
+# against behaviour rather than against how the source happens to read.
+set -euo pipefail
+
+ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
+SCRIPT="$ROOT/terminal/launchers/gitpr-merge-safe.sh"
+TMP="$(mktemp -d)"
+trap 'rm -rf "$TMP"' EXIT
+
+echo "SMOKE: gitpr-merge-safe"
+
+echo "[1/6] the script exists and parses"
+test -x "$SCRIPT"
+bash -n "$SCRIPT"
+
+echo "[2/6] it refuses to merge without a TTY"
+out="$(cd "$ROOT" && "$SCRIPT" 999 squash </dev/null 2>&1 || true)"
+grep -q "interactive terminal" <<<"$out"
+
+# A throwaway repo and a fake gh, so nothing here can reach GitHub.
+REPO="$TMP/repo"
+mkdir -p "$REPO" "$TMP/bin"
+git -C "$REPO" init -q
+git -C "$REPO" -c user.email=t@t -c user.name=t commit -q --allow-empty -m init
+git -C "$REPO" branch -M feature-x
+
+export GH_CALLS="$TMP/gh-calls"
+: >"$GH_CALLS"
+
+cat >"$TMP/bin/gh" <<'STUB'
+#!/usr/bin/env bash
+printf '%s\n' "$*" >>"$GH_CALLS"
+case "$1 $2" in
+  "auth status") exit 0 ;;
+  "pr checks")   echo "build  pass  10s"; exit 0 ;;
+  "pr view")
+    # One batched read: every field the merge plan needs, joined on U+001F.
+    # reviewDecision is deliberately empty — that is the ordinary case (nobody
+    # reviewed it) and the one a tab-separated read would silently shift.
+    printf 'Add a thing\037OPEN\037false\037main\037feature-x\037MERGEABLE\037CLEAN\037\037https://example/pr/42\n'
+    exit 0
+    ;;
+esac
+exit 0
+STUB
+chmod +x "$TMP/bin/gh"
+
+# Answers the one confirmation with "n", so no merge is ever attempted.
+run_plan() {
+  PATH="$TMP/bin:$PATH" GH_CALLS="$GH_CALLS" python3 - "$SCRIPT" "$REPO" <<'PY'
+import os, pty, re, select, sys
+
+# pty.spawn's stdin_read callback fires whenever the master is writable, which
+# floods the script with answers. Drive it manually and reply only to an actual
+# prompt, so the transcript is the script's output and nothing else.
+script, repo = sys.argv[1], sys.argv[2]
+os.chdir(repo)
+PROMPT = re.compile(rb"\[y/N\]:\s*$", re.IGNORECASE)
+
+pid, fd = pty.fork()
+if pid == 0:
+    os.execv("/bin/bash", ["bash", script, "42", "squash"])
+
+seen, pending = bytearray(), bytearray()
+while True:
+    if not select.select([fd], [], [], 30)[0]:
+        break
+    try:
+        chunk = os.read(fd, 1024)
+    except OSError:
+        break
+    if not chunk:
+        break
+    seen.extend(chunk)
+    pending.extend(chunk)
+    if PROMPT.search(bytes(pending).rstrip()):
+        os.write(fd, b"n\n")
+        pending.clear()
+
+os.waitpid(pid, 0)
+sys.stdout.write(seen.decode(errors="replace"))
+PY
+}
+
+echo "[3/6] the merge plan renders and the confirmation can be declined"
+: >"$GH_CALLS"
+out="$(run_plan 2>&1)"
+grep -q "Merge plan" <<<"$out"
+grep -q "Add a thing" <<<"$out"
+grep -q "feature-x -> main" <<<"$out"
+grep -q "Merge cancelled" <<<"$out"
+# An empty reviewDecision must not drag the URL into its place.
+grep -q "Review:   none" <<<"$out"
+grep -q "URL:      https://example/pr/42" <<<"$out"
+
+echo "[4/6] declining means no merge was ever attempted"
+! grep -q "pr merge" "$GH_CALLS"
+
+echo "[5/6] the plan costs one gh pr view, not one per field"
+# Nine fields used to mean nine round trips. Anything above two here means the
+# per-field pattern has come back.
+views="$(grep -c "^pr view" "$GH_CALLS" || true)"
+test "$views" -le 2 || {
+  echo "FAIL: $views 'gh pr view' calls for one merge plan" >&2
+  cat "$GH_CALLS" >&2
+  exit 1
+}
+
+echo "[6/6] it still runs when the UI library is unreachable"
+# The script is launched as its own process from the git menu, so it sources
+# mq-ui.sh itself. A missing library must degrade to no spinner, not to a
+# broken merge tool.
+: >"$GH_CALLS"
+out="$(MQ_UI_LIB="$TMP/does-not-exist.sh" run_plan 2>&1)"
+grep -q "Merge plan" <<<"$out"
+grep -q "Merge cancelled" <<<"$out"
+
+echo "OK: gitpr-merge-safe smoke passed"

tests/manifest.tsv

@@ -97,3 +97,4 @@ menu-exit-contract-smoke.sh	active	-
 theme-manager-path-smoke.sh	active	-
 ui-spinner-smoke.sh	active	-
 mqobsidian-manifest-shell-parity-smoke.sh	active	-
+gitpr-merge-safe-smoke.sh	active	-

tools/scripts/test-all.sh

@@ -27,6 +27,7 @@ echo "== Running mqlaunch headless checks =="
 "$PROJECT_ROOT/tests/mq-stack-contract-smoke.sh"
 "$PROJECT_ROOT/tests/gitlaunch-menu-surface-smoke.sh"
 "$PROJECT_ROOT/tests/gitmerge-safe-smoke.sh"
+"$PROJECT_ROOT/tests/gitpr-merge-safe-smoke.sh"
 "$PROJECT_ROOT/tests/git-menu-surface-smoke.sh"
 "$PROJECT_ROOT/tests/git-restore-to-base-smoke.sh"
 "$PROJECT_ROOT/tests/dashboard-header-cache-smoke.sh"