From 884f8ad100ba118f39f11c3d04d589713f225096 Mon Sep 17 00:00:00 2001 From: karitham Date: Mon, 22 Jun 2026 11:25:31 +0200 Subject: [PATCH] jj-review: refactor --- modules/dev/tools/scripts/jj-review.nu | 286 +++++++++++++------------ 1 file changed, 153 insertions(+), 133 deletions(-) diff --git a/modules/dev/tools/scripts/jj-review.nu b/modules/dev/tools/scripts/jj-review.nu index 223a5ba..10cb22f 100755 --- a/modules/dev/tools/scripts/jj-review.nu +++ b/modules/dev/tools/scripts/jj-review.nu @@ -1,101 +1,159 @@ #!/usr/bin/env -S nu --no-config-file +# # jj-review: PR review workflow with jj, prr, and hx. # Requires: jj, gh, git, prr, hx (helix). - -# Squash the PR commits onto the target branch in a new jj change. Returns -# the change-ids of the previous working copy and the new review change so -# the caller can restore @ after helix closes. # -# The original PR commits and their bookmarks are preserved by duplicating -# the source range first and squashing only the duplicates: a plain -# `jj squash` of the source range would abandon the originals and, per the -# jj docs, "When a commit has been abandoned, all associated bookmarks will -# be deleted." The duplicates have no bookmarks, so abandoning them is -# harmless. +# The review change remains in the log after the script exits; revisit +# it with `jj edit `. + +# run-capture executes a closure and returns its {exit_code, stdout, stderr} record. +def run-capture [code: closure]: nothing -> record { + do $code | complete +} + +# run-or-error executes a closure, returning trimmed stdout or raising an +# error annotated with label and the command's stderr. +def run-or-error [label: string, code: closure]: nothing -> string { + let result = (run-capture $code) + if $result.exit_code != 0 { + return (error make {msg: $"($label) failed: ($result.stderr | str trim)"}) + } + $result.stdout | str trim +} + +# PR_FIELDS is the JSON contract with `gh pr view`. Keep narrow. +const PR_FIELDS = [ + "url" + "number" + "title" + "baseRefName" + "headRefName" + "isCrossRepository" + "headRepository" + "headRepositoryOwner" + "headRefOid" + "baseRefOid" +] + +# fetch-pr-metadata fetches PR metadata via `gh pr view` and parses it into a record. +def fetch-pr-metadata [url: string]: nothing -> record { + run-or-error "gh pr view" { + gh pr view $url --json ($PR_FIELDS | str join ",") + } | from json +} + +# derive-pr-revs returns the base/head revsets for a PR. base_override +# of null falls back to `@`. For cross-repo PRs the +# head uses the fork owner's name, not remote. +def derive-pr-revs [meta: record, base_override, remote: string]: nothing -> record { + let base_rev = ( + if $base_override == null { + $"($meta.baseRefName)@($remote)" + } else { + $"($base_override)" + } + ) + let head_remote = ( + if $meta.isCrossRepository { $meta.headRepositoryOwner.login } else { $remote } + ) + let head_rev = $"($meta.headRefName)@($head_remote)" + + {base_rev: $base_rev, head_rev: $head_rev, head_remote: $head_remote} +} + +# ensure-fork-fetched adds the fork as a remote and fetches it. No-op for same-repo PRs. +def ensure-fork-fetched [meta: record]: nothing -> nothing { + if not $meta.isCrossRepository { + return + } + let owner = $meta.headRepositoryOwner.login + let repo_url = $meta.headRepository.url + let existing = git remote | lines + if not ($owner in $existing) { + run-or-error "jj git remote add" { jj git remote add $owner $repo_url } | ignore + } + print $"Fetching fork ($owner)..." + run-or-error "jj git fetch" { jj git fetch --remote $owner } | ignore +} + +# current-change-id returns the change-id of @ as a short string. +def current-change-id []: nothing -> string { + jj log -r @ -T change_id --no-graph --limit 1 | str trim +} + +# squash-review-change duplicates the PR range and squashes the duplicates +# onto base in a new change. The duplicates have no bookmarks, so abandoning +# them is harmless; the originals (with their bookmarks) stay intact. +# --ignore-immutable is required because the duplicates inherit the original +# PR authors and match strict `immutable_heads` revsets like `(trunk().. & ~mine())`. def squash-review-change [meta: record, base_rev: string, head_rev: string]: nothing -> record { - # Capture the user's current working copy so we can restore it after review. - let prev_wc = jj log -r @ -T change_id --no-graph --limit 1 | str trim + let prev_wc = (current-change-id) - # Create a new empty working copy on top of base. jj new $base_rev - - # Duplicate the PR commits and insert them between base and @. Originals - # (with their bookmarks) stay intact; the duplicates have no bookmarks. jj duplicate -B @ -r $"($base_rev)..($head_rev)" - - # Squash the duplicates into @. They get abandoned, but since they had - # no bookmarks nothing is lost. - # - # --ignore-immutable is required because the duplicates inherit the - # original PR authors and therefore match a strict `immutable_heads` - # revset like `(trunk().. & ~mine())`. The originals (with their - # bookmarks) are still untouched — only the temporary duplicates we - # just created are rewritten. jj squash --ignore-immutable -m $"Squashed PR #($meta.number) for review.\njj-review: head=($meta.headRefOid) base=($meta.baseRefOid)" -t @ -f $"($base_rev)..@-" - # Capture the new review change-id so the caller can print it for the user. - let review_wc = jj log -r @ -T change_id --no-graph --limit 1 | str trim - - {prev_wc: $prev_wc, review_wc: $review_wc} + { + prev_wc: $prev_wc + review_wc: (current-change-id) + } } -# Discover an existing review's path by opening it in a no-op editor. -# More reliable than reconstructing the path because prr uses its own -# configured workdir and local config. -def existing-prr-path [url: string]: nothing -> string { - let edit = do { prr edit $url } | complete - if $edit.exit_code != 0 { - return (error make {msg: $"prr edit failed: ($edit.stderr)"}) +# prr-edit-path discovers the path of an existing review via `prr edit`, +# which opens the configured no-op editor. +def prr-edit-path [url: string]: nothing -> string { + let result = (run-capture { prr edit $url }) + if $result.exit_code != 0 { + return (error make {msg: $"prr edit failed: ($result.stderr | str trim)"}) } - let path = $edit.stdout | str trim + let path = $result.stdout | str trim if $path == "" { return (error make {msg: "prr edit returned an empty path"}) } $path } -# Return the path to the prr review file, downloading it if necessary. -# If the review already exists with unsubmitted changes, re-use the existing -# file instead of failing. -def fetch-prr-path [url: string]: nothing -> string { - let get = do { prr get $url } | complete - if $get.exit_code == 0 { - let path = $get.stdout | str trim - if $path != "" { - return $path - } +# prr-get-or-existing runs `prr get`, falling back to `prr edit` for the +# unsubmitted-changes case. +def prr-get-or-existing [url: string]: nothing -> string { + let result = (run-capture { prr get $url }) + if $result.exit_code == 0 and ($result.stdout | str trim) != "" { + return ($result.stdout | str trim) } - - if ($get.stderr | str contains "unsubmitted changes") { - return (existing-prr-path $url) + if ($result.stderr | str contains "unsubmitted changes") { + return (prr-edit-path $url) } - - return (error make {msg: $"prr get failed: ($get.stderr)"}) + return (error make {msg: $"prr get failed: ($result.stderr | str trim)"}) } -# Return the jj workspace root so helix can run from the repo regardless of -# where this script was invoked. -def jj-repo-root []: nothing -> string { - let root = do { jj root } | complete - if $root.exit_code != 0 { - error make {msg: $"jj root failed: ($root.stderr)"} +# resolve-prr-path returns the path to the prr review file, retrying +# against meta.url when the input differs (e.g. a PR number). +def resolve-prr-path [url: string, meta: record, prr_result: record]: nothing -> string { + if $prr_result.exit_code == 0 and ($prr_result.stdout | str trim) != "" { + return ($prr_result.stdout | str trim) + } + if ($prr_result.stderr | str contains "unsubmitted changes") { + return (prr-edit-path $url) + } + if $meta.url != $url { + return (prr-get-or-existing $meta.url) } - $root.stdout | str trim + return (error make {msg: $"prr get failed: ($prr_result.stderr | str trim)"}) } -# Pick the first modified or added file from the squashed PR for the left -# pane. Deleted files are skipped because they cannot be opened as review -# context. Returns { path, temp } where temp is true for fallback empty files. +# left-pane-file returns the first modified/added file from the squashed +# diff, or a temp file if none exists. The caller cleans up temp files +# (see temp: true). def left-pane-file []: nothing -> record { - let summary = do { jj diff --summary } | complete + let summary = (run-capture { jj diff --summary }) if $summary.exit_code != 0 { return { path: (mktemp) temp: true } } - - let files = ($summary.stdout + let files = ( + $summary.stdout | lines | parse "{status} {path}" | where status in [M A] @@ -109,100 +167,62 @@ def left-pane-file []: nothing -> record { {path: $files.0.path, temp: false} } -# Open helix with a changed file on the left and the review file on the right. -# Uses the repo root as the working directory so relative paths from jj diff -# resolve correctly and the file picker starts in the repo. +# jj-repo-root returns the jj workspace root. +def jj-repo-root []: nothing -> string { + run-or-error "jj root" { jj root } +} + +# open-helix-review opens helix with the left pane on a changed file (or a +# temp file) and the right pane on the prr review file. The working directory +# is the repo root so relative paths from `jj diff` resolve. def open-helix-review [prr_file: string]: nothing -> nothing { if not ($prr_file | path exists) { - error make {msg: $"prr review file not found: ($prr_file)"} + return (error make {msg: $"prr review file not found: ($prr_file)"}) } - let left = (left-pane-file) let repo_root = (jj-repo-root) - hx --working-dir $repo_root --vsplit $left.path $prr_file - if $left.temp { - rm $left.path + try { + hx --working-dir $repo_root --vsplit $left.path $prr_file + } finally { + if $left.temp { + rm $left.path + } } } def main [ url: string --base: string # jj revset to squash onto (default: @<--remote>) - --remote: string = "origin" # remote to use for branch refs (default: origin) + --remote: string = "origin" # remote to use for base/head branch refs ]: nothing -> nothing { - - # Phase 1: Run three independent network operations in parallel. - # - gh pr view: PR metadata (needed for squash, cross-repo fetch) - # - prr get: review file download (needed for helix) - # - jj git fetch --remote: base remote refs (needed for squash) - # All three depend only on the input URL, not on each other. - # --keep-order pins results to input order: $p.0=gh, $p.1=prr, $p.2=fetch. print $"Fetching PR metadata, review file, and ($remote) refs for ($url)..." let p = ( - ["gh" "prr" $remote] + [gh prr fetch] | par-each --keep-order { |tag| - if $tag == "gh" { - do { gh pr view $url --json "url,number,title,baseRefName,headRefName,isCrossRepository,headRepository,headRepositoryOwner,headRefOid,baseRefOid" } | complete - } else if $tag == "prr" { - do { prr get $url } | complete - } else { - do { jj git fetch --remote $tag } | complete + match $tag { + gh => (run-capture { gh pr view $url --json ($PR_FIELDS | str join ",") }), + prr => (run-capture { prr get $url }), + _ => (run-capture { jj git fetch --remote $remote }), } } ) - let gh_result = $p.0 - if $gh_result.exit_code != 0 { - error make {msg: $"gh pr view failed: ($gh_result.stderr)"} + if $p.0.exit_code != 0 { + return (error make {msg: $"gh pr view failed: ($p.0.stderr | str trim)"}) } - let meta = $gh_result.stdout | str trim | from json + let meta = $p.0.stdout | str trim | from json + let revs = (derive-pr-revs $meta $base $remote) - # Use remote branch refs (e.g. main@origin) so the script works in - # setups where the relevant branches are only present as remote - # tracking refs. The head remote is the fork owner for cross-repo - # PRs; otherwise it matches --remote. - let base_rev = (if $base == null { $"($meta.baseRefName)@($remote)" } else { $base }) - let head_remote = ( - if $meta.isCrossRepository { $meta.headRepositoryOwner.login } else { $remote } - ) - let head_rev = $"($meta.headRefName)@($head_remote)" + ensure-fork-fetched $meta - # Cross-repo fetch if needed (depends on metadata from gh). - if $meta.isCrossRepository { - let owner = $meta.headRepositoryOwner.login - let repo_url = $meta.headRepository.url - let existing = git remote | lines - if not ($owner in $existing) { - jj git remote add $owner $repo_url - } - print $"Fetching fork ($owner)..." - jj git fetch --remote $owner - } - - print $"PR #($meta.number): squashing ($base_rev)..($head_rev)..." - let ids = (squash-review-change $meta $base_rev $head_rev) - - # Process prr result. If prr get failed with the input URL, retry with - # the canonical URL from gh metadata (handles non-URL inputs like PR - # numbers that gh accepts but prr does not). - let prr_result = $p.1 - let prr_file = ( - if $prr_result.exit_code == 0 and ($prr_result.stdout | str trim) != "" { - $prr_result.stdout | str trim - } else if ($prr_result.stderr | str contains "unsubmitted changes") { - existing-prr-path $url - } else if $meta.url != $url { - fetch-prr-path $meta.url - } else { - error make {msg: $"prr get failed: ($prr_result.stderr)"} - } - ) + print $"PR #($meta.number): squashing ($revs.base_rev)..($revs.head_rev)..." + let ids = (squash-review-change $meta $revs.base_rev $revs.head_rev) + + let prr_file = (resolve-prr-path $url $meta $p.1) print "Opening helix for review..." open-helix-review $prr_file - # Restore the user's working copy so the script is idempotent across - # runs. The review change remains in the log for later reference. jj edit $ids.prev_wc print $"Review change: ($ids.review_wc). Run 'jj edit ($ids.review_wc)' to revisit." } -- 2.51.2