diff --git a/.beads/.gitignore b/.beads/.gitignore index e50ad2e..d3a474d 100644 --- a/.beads/.gitignore +++ b/.beads/.gitignore @@ -72,9 +72,9 @@ backup/ db.sqlite bd.db issues.jsonl +interactions.jsonl !.gitignore !README.md !config.yaml !metadata.json -!interactions.jsonl diff --git a/.beads/interactions.jsonl b/.beads/interactions.jsonl deleted file mode 100644 index b7f911c..0000000 --- a/.beads/interactions.jsonl +++ /dev/null @@ -1 +0,0 @@ -{"id":"int-ab595789dc8f6cf0ef12855e7de8f614","kind":"field_change","created_at":"2026-08-30T19:08:28.153996Z","actor":"Muzaffar Mohammed","issue_id":"hunk-commit-6af","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"Fixed, but the trigger in the description was wrong and the fix is broader.\n\nDiscarding a marked symlink was already blocked, incidentally: checkReviewedFile\nreads the working copy for the staleness check, that read follows the link and\nreturns the target file's content, which never matches the patch — so it refused\nas `stale`. Accidental, not a guarantee.\n\nThe reachable path is an *unmarked* symlink anywhere in a jj review.\nrequiresWorkingCopyCheck is false for a `revert` disposition, so such a file is\nnever read and no staleness gate runs on it — yet it still becomes a `restore`\ninstruction for the helper script, whose `cp` resolves the destination. Marking\none unrelated hunk and staging writes through the link, outside the workspace.\nConfirmed by disabling the check and watching the copy land outside the repo.\n\nAlso found: the restore loop's `cp -p` has the same defect as the `>` redirect;\nthe description only named the redirect.\n\nFix — refusal at the boundary, not a guard at each write site:\n- src/patch/parse.ts: FilePatch.declaredModes, read from `new file mode`,\n `deleted file mode`, `old mode`, `new mode` and the trailing field on `index`.\n Both sides, since a symlink -> regular change names the symlink on the old\n side only while the working copy still holds the link.\n- src/patch/modes.ts (new): unsupportedModeReason, an allowlist of 100644/100755.\n- src/review/check.ts: new `unsupported-type` refusal, right after the path\n check — the one place both backends pass through, and it runs for every file,\n marked or not.\n\nDefence in depth:\n- src/discard/workingCopy.ts (new): the discard environment, extracted from\n index.ts so the integration test drives the real one; writes with O_NOFOLLOW.\n- src/jj/script.ts: unlink_if_symlink before both the redirect and the cp.\n A function rather than `[ -L x ] && rm`, which aborts under `set -eu`.\n\nNot done: no guard on symlinked parent directories. Neither a git index nor a\njj tree can hold `a` as a symlink and `a/b` at once, so it is not reachable\nfrom tracked content.\n\n20 tests added; the four integration tests were verified to fail with the\ncheck disabled. 201 pass, typecheck clean.\n"}} diff --git a/.claude/skills/hunkdiff-upgrade/SKILL.md b/.claude/skills/hunkdiff-upgrade/SKILL.md new file mode 100644 index 0000000..a68dcbb --- /dev/null +++ b/.claude/skills/hunkdiff-upgrade/SKILL.md @@ -0,0 +1,184 @@ +--- +name: hunkdiff-upgrade +description: Complete a hunkdiff dependency bump for hunk-commit — read the release notes, reconcile the declared extension API generation, the README, and the demo GIFs with what the new Hunk actually does. Use when a Renovate PR bumps hunkdiff, when the user asks whether a new Hunk release needs changes here, or after manually changing the hunkdiff version. +--- + +# Completing a hunkdiff bump + +Renovate announces the version bump; it cannot finish it. `renovate.json5` says +why in full — hunkdiff is imported almost entirely as `import type`, so it is +erased before runtime and only one test module actually evaluates it. Green CI +on a hunkdiff PR means almost nothing. + +**Work on Renovate's own branch.** Its cooldown is turned off precisely so that +branch arrives on release day, with `package.json` and `bun.lock` already +bumped. Add your reconciliation commits on top, so one PR carries the bump and +everything the bump implies — a reviewer sees the new version and the README +line it moved as one change. Do not open a competing PR and close Renovate's: +Renovate treats a closed PR as a decision and its PRs can regenerate, so that +path fights the bot instead of using it. + +```sh +git fetch origin && git switch renovate/hunkdiff-0.x # name varies +``` + +Two cautions once you are on it: + +- **Never tick the rebase/retry checkbox** in the PR body. That hands the + branch back to Renovate, which rebuilds it from its own commit — your work + is not on the version it rebuilds from. +- **Renovate may or may not keep updating a branch a human has pushed to** + (it detects modification, but confirm the behaviour for this install rather + than trusting this line). If a newer hunkdiff lands while the PR is open, + check that the branch is on the version you think it is before finishing. + +**Your job is the part CI cannot do:** read what changed in Hunk, and reconcile +this repository's *claims* about Hunk with what Hunk now is. + +## Ground rules + +- **Never open the TUI or `reload` the user's Hunk session.** Same rule as + `demo-gifs`; the only Hunk that may run is one VHS drives. +- **Never commit or push** when a human is driving. Stop at the working copy + and hand off — Renovate's branch is shared, with an open PR on it, so + pushing to it is the owner's call. + + The one exception is `.github/workflows/hunkdiff-upgrade.yml`, which runs + this skill unattended when Renovate opens the PR and grants the push in its + prompt. Push only when something has said so explicitly; the default is + still stop-and-report. +- **Never claim a release note says something you did not read.** If `gh` fails + or the release has no notes, say so and stop. Guessing at an API change is + worse than reporting that you could not check. + +## 1. Establish the range + +```sh +git diff origin/main -- package.json # on Renovate's branch +jj diff -r @ package.json # or, in jj +``` + +You need **old version → new version**, and every release in between. Renovate +skips versions, so 0.20.1 → 0.22.0 means reading three sets of notes, not one. + +hunkdiff is **0.x**: a minor bump is breaking by convention, and Hunk uses that +convention — extension API generations advance on minors. + +## 2. Read the release notes + +Notes live on the git tag, `v` prefixed, in the upstream repo: + +```sh +gh release view v0.21.0 --repo modem-dev/hunk --json body -q .body +gh release list --repo modem-dev/hunk --limit 15 +``` + +Skip prereleases (`v0.21.0-beta.1`) — npm dist-tags never ship them here. + +Two places in each body carry the signal: + +- the **`### For extension authors`** section — the curated summary; read it + whole, it is short. +- the collapsed commit list — grep it for `(extension`, `(extensions`, + `(api`. A breaking rename sometimes lands there without reaching the + curated section. + +A permalink form also exists (`https://hunk.dev/changelog/0.21/`), linked from +the bottom of each release body. Use it if `gh` is unavailable. + +## 3. Run the gates + +```sh +bun install +bun test +bun run typecheck +``` + +`bun test` covers `hunk.apiVersion` (see below). `tsc --noEmit` is the *only* +thing checking the four types this repo imports — +`ExtensionCommandContext`, `ExtensionDiffFile`, `ExtensionLineHighlight`, +`HunkExtensionAPI` (`index.ts`, `src/ui/session.ts`). Neither gate says +anything about runtime behaviour, wording, or keybindings. + +## 4. Reconcile every claim + +Work this table top to bottom. The first two rows a gate will fail on; the rest +drift **silently** and are the actual reason this skill exists. + +| Claim | Where | How to check | +| --- | --- | --- | +| Declared API generation | `package.json` → `hunk.apiVersion` | `bun test`; the value is `HUNK_EXTENSION_API_VERSION` from `hunkdiff/extension` | +| Imported type surface | `index.ts`, `src/ui/session.ts` | `bun run typecheck` | +| Minimum Hunk version + API generation, in prose | `README.md` → `## Requirements` | **Fix the numbers.** Nothing checks this line | +| Command ids, keybindings, config keys | `README.md` → the command table, `[extension.hunk-commit]` | Against the release notes only — read `hunk-extensions` (below) if an id scheme changed | +| Dialog and confirmation wording | `demo/*.tape`, `README.md` | Release notes; the tapes assert on screen text | +| The four demo GIFs | `demo/*.gif` | See *GIFs*, below | + +**On the Requirements line:** it states a minimum Hunk version *and* an API +generation, and both move together — mechanically, with no judgement call. + +The host refuses any extension declaring a generation **higher** than its own, +and `test/manifest.test.ts` pins our declaration to the installed hunkdiff's +`HUNK_EXTENSION_API_VERSION`. So raising the dependency raises what we declare, +which raises the oldest Hunk that will load us at all. It is not "the oldest +Hunk whose API we happen to use" — the declaration alone locks older hosts out. + +The minimum is therefore **the first Hunk release shipping the generation we +now declare**. Generations advance on minors, so it is normally the new minor +itself. Confirm rather than assume — read the constant out of the published +tarballs: + +```sh +for v in 0.20.1 0.21.0; do + curl -sL "$(npm view hunkdiff@$v dist.tarball)" \ + | tar -xzO package/dist/npm/extension/index.js \ + | grep -o "HUNK_EXTENSION_API_VERSION = [0-9]*" | head -1 | sed "s/^/$v: /" +done +``` + +Worked example: 0.19.0 ships generation 6, 0.20.0 and 0.20.1 ship 8, 0.21.0 +ships 16 — so the 0.20.1 → 0.21.0 bump moved the line from +"Hunk 0.20 or newer (extension API v8)" to "Hunk 0.21 or newer (extension +API v16)". Note 0.20.1 did *not* move it: patches keep the generation, so a +patch bump leaves this line alone. + +## 5. Deeper API questions + +hunkdiff ships its own authoring map at +`node_modules/hunkdiff/skills/hunk-extensions/SKILL.md`, and it is versioned +with the package — so after `bun install` it already describes the *new* API. +Read it when the release notes mention an extension change you cannot map onto +this repo's code. Diffing it across the bump is the fastest way to see what +actually moved: + +```sh +git diff HEAD -- node_modules/hunkdiff/skills/ # if vendored; otherwise + # read the new one directly +``` + +## 6. GIFs + +If wording, a keybinding, a dialog, or Hunk's rendering changed, the committed +GIFs are now lying. **Do not re-record them here** — hand off to the +`demo-gifs` skill, which owns `demo/record.sh` and the tape inventory. Your +output is the *decision*: which GIFs are stale and which release note makes +them stale. + +If nothing visible changed, say so explicitly. Re-recording four GIFs for a +no-op churns binary files in git for nothing. + +## Handoff + +Report, in this order: + +1. **Versions** — old → new, and every release read in between. +2. **What the notes say for extension authors** — the relevant lines, quoted. +3. **Gates** — `bun test` and `bun run typecheck`, pass or fail with output. +4. **Files changed**, and the claim each change reconciles. +5. **GIFs** — stale (which, and why) or unaffected. +6. **Unresolved** — anything the notes imply but you could not verify without + running Hunk. This section is the point of the handoff; an empty one should + be rare on a minor bump. + +Then stop. Suggest the commit — Conventional Commits, `chore(deps):` or +`docs:` depending on what dominates — and let the user run it. diff --git a/.github/workflows/hunkdiff-upgrade.yml b/.github/workflows/hunkdiff-upgrade.yml new file mode 100644 index 0000000..38616ac --- /dev/null +++ b/.github/workflows/hunkdiff-upgrade.yml @@ -0,0 +1,76 @@ +name: hunkdiff upgrade + +# Renovate's hunkdiff PR is deliberately exempt from the cooldown in +# renovate.json5, so this branch appears on release day. It arrives with +# package.json and bun.lock bumped and nothing else — the README's stated +# minimum Hunk version, the declared API generation, and the demo GIFs are all +# still describing the old Hunk. This job adds that reconciliation on top of +# Renovate's own commit, so one PR carries the bump and everything it implies. +# +# `opened` and `reopened` only, never `synchronize`: this job pushes to the +# branch it was triggered by, and synchronize would make that a loop. +on: + pull_request: + types: [opened, reopened] + +permissions: + contents: write # push the reconciliation commit onto Renovate's branch + pull-requests: write # leave the summary where the reviewer will read it + +concurrency: + group: hunkdiff-upgrade-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + reconcile: + # Renovate branches live in this repository, not a fork, so secrets are + # available and the push below can reach the branch. + if: >- + github.event.pull_request.user.login == 'renovate[bot]' && + contains(github.event.pull_request.title, 'hunkdiff') + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + with: + # The default merge ref is detached — there is nothing to push from. + # Full history because the skill diffs the bump against main. + ref: ${{ github.head_ref }} + fetch-depth: 0 + + - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0 + + - run: bun install --frozen-lockfile + + - uses: anthropics/claude-code-action@fa2b2666b747000bf42767d1f332065b375e3c8f # v1.0.214 + with: + anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} + github_token: ${{ secrets.GITHUB_TOKEN }} + claude_args: | + --max-turns 40 + --allowedTools Bash,Edit,Read,Write,Glob,Grep + prompt: | + A Renovate PR has bumped the `hunkdiff` dependency on the branch + already checked out. Complete it by invoking the `hunkdiff-upgrade` + skill in `.claude/skills/` and following it exactly. + + Two adjustments to the skill's ground rules, for this run only: + + - The skill says never commit or push. Here you MAY commit and push + to this branch, because that is the whole point of this job. All + its other rules stand — in particular, never tick the rebase + checkbox, and never claim a release note you did not read. + - `jj` is NOT installed in this job, so the jj-backed test suites + self-skip. Do not report a green `bun test` as full coverage. The + main CI job runs the full suite with jj on this same commit; + defer to it and say so. + + Make one commit per concern, Conventional Commits style + (`docs:` for README claims, `chore(deps):` for anything about the + bump itself). Push them to this branch. + + Then post the skill's handoff report as a PR comment, so the + reviewer reads it next to the diff. If a demo GIF is stale, say + which and why — do NOT record it, since VHS and a terminal are not + available here. + + If the release notes cannot be read, push nothing and say so. diff --git a/README.md b/README.md index 788ff44..4adb67e 100644 --- a/README.md +++ b/README.md @@ -4,19 +4,15 @@ Mark hunks while you review in [Hunk](https://hunk.dev), and move them without leaving the review — into the git index, or into a commit: a git commit, or a [Jujutsu](https://github.com/jj-vcs/jj) revision. -Whichever you choose, **your files on disk never change** — the `git add -p` -shape, extended to committing, and to Jujutsu, which has no index. - ![Two hunks of a fix marked amber in a Hunk review, then C, then a one-line summary prompt, then the review reloads with those hunks gone and an unrelated typo left behind.](demo/hero.gif) ## Requirements -- Hunk 0.20 or newer (extension API v8) +- Hunk 0.21 or newer (extension API v16) - `git`, or `jj` — whichever the repository uses -- For jj only: a POSIX shell, which is how the selection reaches `jj`. - The git path has no such requirement and works on Windows. +- For jj only: a POSIX shell, which is how the selection reaches `jj` ## Install @@ -34,32 +30,22 @@ hunk diff --extension /path/to/hunk-commit Open a working-copy review (`hunk diff`), then: -| Key | Does | -| --- | ----------------------------------------------------------------------------------------- | -| `x` | Mark or unmark the hunk under the cursor — only needed to batch several | -| `X` | Mark or unmark the whole file — the only way to mark a binary or oversized file | -| `N` | Clear all marks | -| `S` | Stage the marked hunks, or the one under the cursor — git only | -| `C` | Commit the marked hunks, asking only for a summary | -| `B` | Commit them with a description as well — one more question | -| `F` | Add the marked hunks to a specific commit, selected from a list | -| `D` | Discard the marked hunks, or the one under the cursor — reverts them in your working copy | - -Marked lines are painted amber in the diff — a hue the diff's own green, red, -and neutral do not use. By default only the lines that will actually move are -marked; `context_marks` (below) can extend that to the context lines around -them, at the cost of making the marked region look larger than what moves, -since a hunk carries up to three context lines on each side. Every one of these -commands asks first — for a message, or for confirmation — names where the -hunks are going, and reloads the review afterwards, so what you see is what is -still unmoved. - -Commands are rebindable by id in Hunk's `[keybindings]` table. **The ids come -from the folder the extension is installed into**, so an install from a -repository named `hunk-commit` gives `hunk-commit.toggleHunk`, -`hunk-commit.toggleFile`, `hunk-commit.clearMarks`, `hunk-commit.stage`, -`hunk-commit.commit`, `hunk-commit.commitWithBody`, `hunk-commit.into`, and -`hunk-commit.discard`. +| Key | Does | +| --- | ---------------------------------------------------------------------------------------- | +| `x` | Mark or unmark the hunk under the cursor — only needed to batch several | +| `X` | Mark or unmark the whole file — the only way to mark a binary or oversized file | +| `N` | Clear all marks | +| `S` | Stage the marked hunks, or the one under the cursor — git only | +| `C` | Commit the marked hunks, asking only for a summary | +| `B` | Commit the marked hunks with a description | +| `F` | Add the marked hunks to a specific commit, selected from a list | +| `D` | Discard the marked hunks, or the one under the cursor — reverts them in the working copy | +| `L` | Show what is marked, in a pane beside the review | + +Commands are rebindable in Hunk's `[keybindings]` table as +`.` — an install from a repository named `hunk-commit` gives +`hunk-commit.toggleHunk`, `hunk-commit.stage`, and so on for every id in +`index.ts`. ### Config @@ -70,45 +56,29 @@ context_marks = "none" # how much of a marked hunk's context lines is marked: # "none" (default), "edge" (a thin rail), "full". ``` -There is no configured destination: `C` always makes a new commit and `F` -always asks which existing one, so where a hunk lands is decided in the review -rather than in a file. +Marked context is tinted rather than coloured, so its visibility depends on +your theme; `"edge"` reads almost anywhere, `"full"` can be near-invisible on a +dark one. ### Committing -`C` turns the marked hunks straight into a commit — a git commit on the current -branch, or a new Jujutsu revision — instead of leaving them staged. It asks one -question, the summary, and commits. `B` is the same command with a description: -Hunk's input dialog holds a single line, so a body costs a second question, and -only the key that promises one asks it. Cancelling any question abandons the -commit, and typing the message _is_ the confirmation, so nothing asks again -afterwards. - -In git it refuses, before asking anything, when something is already staged -(`git commit` would sweep it in) or when a rebase, merge, or cherry-pick is -half-finished. If a `pre-commit` hook rejects the commit, the marked hunks are -unstaged again, so the repository is exactly as it was. - ![Pressing B on a marked hunk: a dialog asking for the summary, a second dialog asking for an optional longer description, then the hunk committed and gone from the review.](demo/commit.gif) ### Putting hunks into an existing commit -`F` lists the commits you can still change and puts the marked hunks into the -one you pick. Only unpushed commits are offered — `@{upstream}..HEAD`, or the -recent history when the branch tracks nothing — so the picker cannot offer a -commit that someone else may already have. +`F` puts the marked hunks into a commit you pick. Only unpushed commits are +offered — `@{upstream}..HEAD`. -What happens next differs, and the confirmation says which: +What happens next differs: - **In Jujutsu it happens now.** `jj squash` moves the hunks into the revision, rebases its descendants, and records one operation that `jj undo` reverses. - **In git it is deferred.** A `fixup!` commit is added on top, naming the - target by its full hash — git matches a title _or_ a hash, and titles repeat. - Nothing is rewritten until you run the `git rebase --autosquash --autostash` - command the message gives you, at a moment you choose. `--autostash` is part - of it because the hunks you did not mark are still in your working tree. + target by hash rather than title, since titles repeat. Nothing is rewritten + until you run the `git rebase --autosquash --autostash` command the message + gives you — `--autostash` because your unmarked hunks are still in the tree. ![In a Jujutsu workspace, pressing F on a marked hunk: a picker of the commits that can still be changed, a confirmation naming the revision and the jj undo @@ -116,26 +86,13 @@ that reverses it, then the squash landing immediately.](demo/into.gif) ### Discarding -`D` is the opposite of `S`: instead of moving the marked hunks somewhere, it -reverts them in your working copy. The arithmetic is the same one staging -uses, run the other way round — staging keeps the marked hunks and reverts the -rest, discarding reverts the marked hunks and keeps the rest. - -The consequence is not the same, and the confirmation says so in different -words depending on where you are: - -- **In a Jujutsu workspace it is recoverable.** Loading a review runs - `jj diff`, which snapshots the working copy into the operation log, so the - state before the discard is already recorded. `jj undo` brings the changes - back. -- **In a git repository it is not.** Uncommitted text that gets overwritten - exists nowhere else — there is no stash, no dangling object, nothing to - recover from. This is the same finality as `git restore -p`. +`D` reverts the marked hunks in the working copy: -Discarding always asks first; there is no path that skips the confirmation. A -binary file is refused rather than guessed at, because the patch carries no -record of what it held before — revert those with `jj restore` or -`git restore`. +- **In Jujutsu it is recoverable.** Loading a review runs `jj diff`, which + snapshots the working copy into the operation log, so `jj undo` brings the + changes back. +- **In git it is not.** Overwritten uncommitted text exists nowhere else — no + stash, no dangling object. The same finality as `git restore -p`. ![Marking one hunk and pressing D in a git repository: a confirmation naming what is about to be lost, then the hunk reverting in the working copy while @@ -146,69 +103,16 @@ the unmarked changes stay.](demo/discard.gif) The marking half is shared. The applying half is not, because the two systems disagree about what staging _is_. -**Git** has an index, so staging is a patch applied to it. `hunk diff` in a git -repository is a bare `git diff` — the working tree against the index — so a -patch of the marked hunks applies exactly where it was measured, and composes -with whatever was staged before. Whole-file marks go through `git add` instead, -which handles binaries, renames, and mode changes natively. - -**Jujutsu** has no index, so staging is a rewrite — `jj split` to extract the -marked hunks into a new revision, or `jj squash` to fold them into one that -already exists. Either way sub-file selection goes through jj's diff editor: it -runs one with `$left` holding the target's content and `$right` holding -everything the source revision changed, and whatever `$right` contains when the -editor exits is what moves. So this extension rebuilds each partly marked file -by **reverting** its unmarked hunks, writes the results into a staging -directory, and points a generated merge-tool config at a small `sh` script that -copies that directory into `$right`. Both commands read the same two -directories, so the selection means the same thing to either. `jj` does the -rest — rewriting revisions, rebasing descendants, and recording one operation, -which **`jj undo` reverses completely**. - -The new revision's description is passed with `--message`, so jj never opens an -editor that would fight Hunk for the terminal. - -The difference shows up in one detail: in git, a file nobody marked needs no -instruction at all, because "not staged" is git's default state. In jj, a -revision has to be told what stays behind. - -Both paths use the same line arithmetic against the patch Hunk is already -showing, so there is no re-diffing and no fuzz, and all of it stays in -TypeScript where it is unit-tested. The shell script holds none of it. - -## What it refuses to do - -Staging is refused, with nothing written, when: - -- **the working copy moved on.** Every marked file is checked against the patch - the review was built from, line by line. Hunk reviews a snapshot, so a marked - hunk can stop meaning what it meant. A mismatch stops the whole operation. -- **the two parses disagree.** Hunk parses the patch to render it and assigns - the hunk indexes you mark; this extension parses it again to rebuild files. - If the two ever disagree about a hunk's extent, marking hunk 2 could stage - something else, so staging refuses instead. -- **the review is not a working-copy review**, the jj target is not a usable - revset, or a path cannot be expressed in the helper's manifests (paths - containing newlines). - -## Known limitations - -- **Working-copy reviews only.** Rebuilding a file reads it from the working - copy, so `hunk show ` and range diffs are not stageable. -- **No unstaging.** `hunk diff --staged` plus a reversed patch would give git - unstaging; it is not wired up. `U` is left free for it. -- **`S` is git's alone.** Jujutsu has no index, so there is nothing to stage - into: `C` and `F` are its two destinations. -- **Discarding is not a transaction.** Every file is checked before any file is - written, so a refusal changes nothing — but an error partway through the - writes leaves earlier files already reverted. -- **Nothing routes hunks automatically.** `jj absorb` and `git absorb` send each - hunk to the commit that last touched those lines; `F` always asks instead, so - no commit is rewritten on a guess. -- **Binary and oversized files are all-or-nothing** — Hunk shows no hunks for - them, so `X` is the only way to stage them. -- **Windows works for git, not jj.** The jj helper needs a POSIX shell; the - seam for a PowerShell version is `src/jj/script.ts`. +**Git** has an index, so staging is a patch applied to it, composing with +whatever was staged before. Whole-file marks go through `git add`, which +handles binaries, renames, and mode changes natively. + +**Jujutsu** has no index, so staging is a rewrite: `jj split` to extract the +marked hunks, or `jj squash` to fold them into an existing revision. Sub-file +selection goes through jj's diff editor, which this extension drives by +rebuilding each partly marked file with its unmarked hunks reverted. jj does +the rest, recording one operation that **`jj undo` reverses completely**. The +mechanics are in `src/jj/`. ## Development @@ -218,29 +122,18 @@ bun test # unit tests, plus integration tests when git / jj are on PATH bun run typecheck ``` -The integration tests build real git and jj repositories in temp directories -and drive the real binaries end to end; each suite skips itself when its binary -is missing. +Integration tests build real git and jj repositories in temp directories and +drive the real binaries; each suite skips itself when its binary is missing. ### Recording the demo GIFs -The GIFs above are recordings of the real extension, not mockups. Regenerate -one after changing a key, a prompt, or a confirmation: +The GIFs above are recordings of the real extension. Regenerate one after +changing a key, a prompt, or a confirmation: ```bash ./demo/record.sh hero # or: commit, into, discard ``` -It needs `vhs`, `ttyd`, and `ffmpeg` alongside the usual toolchain. Each run -builds a throwaway repository (`demo/fixture.ts`), installs this extension into -a config directory of its own — so no other extension, theme, or update notice -of yours reaches the frame — then records `demo/.tape` and checks the -result: a size budget, a length budget, and a last frame that differs from the -first, which is what catches a recording of a TUI that never took a keystroke. - -GIFs are binary, so a Hunk review cannot show you what changed. Watch the file -before committing it. - -## License - -MIT +It needs `vhs`, `ttyd`, and `ffmpeg`, builds a throwaway repository, and +verifies the result before it lets the run pass. GIFs are binary, so a Hunk +review cannot show you what changed — watch the file before committing it. diff --git a/bun.lock b/bun.lock index c520d30..d094532 100644 --- a/bun.lock +++ b/bun.lock @@ -6,7 +6,8 @@ "name": "hunk-jj-stage", "devDependencies": { "@types/bun": "^1.4.0", - "hunkdiff": "^0.20.0", + "@types/react": "^19.2.18", + "hunkdiff": "^0.21.0", "typescript": "^5.9.3", }, }, @@ -32,37 +33,13 @@ "@opentui/react": ["@opentui/react@0.5.8", "", { "dependencies": { "@opentui/core": "0.5.8", "react-reconciler": "^0.33.0" }, "peerDependencies": { "react": ">=19.2.0", "react-devtools-core": "^7.0.1", "ws": "^8.18.0" } }, "sha512-l3N/Kbg5V+OABp60A5iCfEj3WK5pKVUDmUrVPf/xgct9R7Xy2Jbw/0MoSFpgr3odq3CuWprYajnoLCR9f81BRQ=="], - "@oven/bun-darwin-aarch64": ["@oven/bun-darwin-aarch64@1.4.0", "", { "os": "darwin", "cpu": "arm64" }, "sha512-GCpf8QuFLsyioVawP5HrMxA1ZRBlu6Hq9RNnSc3UTUWAzIxBso9trjoZczw1HdgpqSssFkszfIV2zmOzFTjhkw=="], - - "@oven/bun-darwin-x64": ["@oven/bun-darwin-x64@1.4.0", "", { "os": "darwin", "cpu": "x64" }, "sha512-cIrhwOr0SPEraewznhC+c/k6TG8bwFn5uZ4EJuXwjiKJLcAF36q7/bGjWkeXSe48JwMcPRUR054JXF7+cRwSSA=="], - - "@oven/bun-freebsd-aarch64": ["@oven/bun-freebsd-aarch64@1.4.0", "", { "os": "freebsd", "cpu": "arm64" }, "sha512-09x7wnjMR6M5KGBDBhVl2CpfoCIQOkVDbPX2KfIhpXv4N6grbWE7dfLPw/Ydi9gaUMGhU7UKhoz444Nu6RCycA=="], - - "@oven/bun-freebsd-x64": ["@oven/bun-freebsd-x64@1.4.0", "", { "os": "freebsd", "cpu": "x64" }, "sha512-dRwzti/qJqV1HWplU27iUWUqp+f2DtFSf2yqQKSb+HH2dDOC//Uqd9u/A5h1DMsLszfP5OGP9UwQIKxVwFODaA=="], - - "@oven/bun-linux-aarch64": ["@oven/bun-linux-aarch64@1.4.0", "", { "os": "linux", "cpu": "arm64" }, "sha512-Y5yAtCbHK6JjprXEtkdklDQFPADgs+CkfcliyY5g4JJ8baGHyQSrfpSkX3XVJ2C+aBLsdwNDdW+oczMsAwx6uA=="], - - "@oven/bun-linux-aarch64-android": ["@oven/bun-linux-aarch64-android@1.4.0", "", { "os": "android", "cpu": "arm64" }, "sha512-HpPIxJfDNPBPhiBNMyZoo/dOLijARfsx5j72vNuLtaTvl0Hh7HUculxjsOQ2WSyGoCgqXMEr1Qqjab1im9u1RA=="], - - "@oven/bun-linux-aarch64-musl": ["@oven/bun-linux-aarch64-musl@1.4.0", "", { "os": "linux", "cpu": "arm64" }, "sha512-RUjAAkJ/CdNV++zVxyANWshPc73CECYsfhk0fWAkoJjtywxJ2BwXzI6nopBBDMfs0HS+fhRGn6zGwU8ccxLeJg=="], - - "@oven/bun-linux-x64": ["@oven/bun-linux-x64@1.4.0", "", { "os": "linux", "cpu": "x64" }, "sha512-Du44zebtPXJujvMLmtIxEQ6ykOhYt7L/Q+YIGVm+Yy+Pj/fpOnq60ggwIpKp/pGAFbYHNiTrA3JTjuZ9MTbZIg=="], - - "@oven/bun-linux-x64-android": ["@oven/bun-linux-x64-android@1.4.0", "", { "os": "android", "cpu": "x64" }, "sha512-u++KyLlfMn36yWz+AgJs+fZtS46UFDNpSSZhrcitkytONtNwq0X6Q9BDVEFXxYl/+Eec0xme1rb6MgW+U35WeA=="], - - "@oven/bun-linux-x64-musl": ["@oven/bun-linux-x64-musl@1.4.0", "", { "os": "linux", "cpu": "x64" }, "sha512-C1Dv+ISL8YKEKM9jAHzNifOcRUoziy6UMxh+yVXjUCP6QnbRhENDHLaIWWkQZJyBLTn0I3xozflorAlHiGzGqA=="], - - "@oven/bun-windows-aarch64": ["@oven/bun-windows-aarch64@1.4.0", "", { "os": "win32", "cpu": "arm64" }, "sha512-FBAYaQpJBP0asgqzL6NFUfjdQqsV+kvTpJ/eWxPKj+RcDgIfPSuE8kvQuPYu5pa8u8JTujYMjmuyvHxVuQsInA=="], - - "@oven/bun-windows-x64": ["@oven/bun-windows-x64@1.4.0", "", { "os": "win32", "cpu": "x64" }, "sha512-jRKv1NPLznMSZY5BEWciMF7zv0Tiyo2pQSxAJ3w+YWJ6y3VWNJQQQdLlV5Jx8lbOFDrJdrc9dD3GV17k3BP41A=="], - "@types/bun": ["@types/bun@1.4.0", "", { "dependencies": { "bun-types": "1.4.0" } }, "sha512-K+lZULY23vRgK/CfTjFIV+tyifaNdSMlPh9j+6mQ/cLfpOznLyAuzgV/JQysyECpkBQLVMSyvjlr2fBUSA9wFQ=="], "@types/node": ["@types/node@26.4.0", "", { "dependencies": { "undici-types": "~8.3.0" } }, "sha512-faiGnoIrLH/V8cibOMEAZ8pMw6oXqSukl29ra4mN8GdaB2ZewzeaLj+INpV5N+Z1eKWzY+IzaIZH2EIR6YZRNQ=="], - "ansi-regex": ["ansi-regex@6.3.0", "", {}, "sha512-WpDfL7NO6j7tH88IDBNVdUJxDh9nmCteAVW9dsep846XdwF4naCBK+/tGLX3KJgcpgMRXCFlTM2hKGoK9FsdrQ=="], + "@types/react": ["@types/react@19.2.18", "", { "dependencies": { "csstype": "^3.2.2" } }, "sha512-AnzbBERsrLKtk2XSfTbYRLjQPdy116Sty4q+T+Bp3IC4l6jNBvreVPAHmpq9qhXQM7CXZPjLVmGMw9sy+hxQ3w=="], - "bun": ["bun@1.4.0", "", { "optionalDependencies": { "@oven/bun-darwin-aarch64": "1.4.0", "@oven/bun-darwin-x64": "1.4.0", "@oven/bun-freebsd-aarch64": "1.4.0", "@oven/bun-freebsd-x64": "1.4.0", "@oven/bun-linux-aarch64": "1.4.0", "@oven/bun-linux-aarch64-android": "1.4.0", "@oven/bun-linux-aarch64-musl": "1.4.0", "@oven/bun-linux-x64": "1.4.0", "@oven/bun-linux-x64-android": "1.4.0", "@oven/bun-linux-x64-musl": "1.4.0", "@oven/bun-windows-aarch64": "1.4.0", "@oven/bun-windows-x64": "1.4.0" }, "os": [ "!aix", "!sunos", "!openbsd", ], "cpu": [ "x64", "arm64", ], "bin": { "bun": "bin/bun.exe", "bunx": "bin/bunx.exe" } }, "sha512-iRiFkc2W7UVpCyZXO9tod45TP9QCyN19fWqbpeN/jaM/K7uzeHYx/OSPsahMJazGKBgPsnxRt+4Jc43d8BcHZw=="], + "ansi-regex": ["ansi-regex@6.3.0", "", {}, "sha512-WpDfL7NO6j7tH88IDBNVdUJxDh9nmCteAVW9dsep846XdwF4naCBK+/tGLX3KJgcpgMRXCFlTM2hKGoK9FsdrQ=="], "bun-ffi-structs": ["bun-ffi-structs@0.3.1", "", { "peerDependencies": { "typescript": "^5" } }, "sha512-3gM7PpVWLyrwxWjcilSiGuhWanhZivvo6l0u573NziPH6f/gwk6McbaYgn7oJWov6pKGRTDbrg94W5DcJsKTtQ=="], @@ -72,23 +49,25 @@ "commander": ["commander@14.0.3", "", {}, "sha512-H+y0Jo/T1RZ9qPP4Eh1pkcQcLRglraJaSLoyOtHxu6AapkjWVCy2Sit1QQ4x3Dng8qDlSsZEet7g5Pq06MvTgw=="], + "csstype": ["csstype@3.2.3", "", {}, "sha512-z1HGKcYy2xA8AGQfwrn0PAy+PB7X/GSj3UVJW9qKyn43xWa+gl5nXmU4qqLMRzWVLFC8KusUX8T/0kCiOYpAIQ=="], + "diff": ["diff@8.0.4", "", {}, "sha512-DPi0FmjiSU5EvQV0++GFDOJ9ASQUVFh5kD+OzOnYdi7n3Wpm9hWWGfB/O2blfHcMVTL5WkQXSnRiK9makhrcnw=="], "emoji-regex": ["emoji-regex@10.6.0", "", {}, "sha512-toUI84YS5YmxW219erniWD0CIVOo46xGKColeNQRgOzDorgBi1v4D71/OFzgD9GO2UGKIv1C3Sp8DAn0+j5w7A=="], "get-east-asian-width": ["get-east-asian-width@1.6.0", "", {}, "sha512-QRbvDIbx6YklUe6RxeTeleMR0yv3cYH6PsPZHcnVn7xv7zO1BHN8r0XETu8n6Ye3Q+ahtSarc3WgtNWmehIBfA=="], - "hunkdiff": ["hunkdiff@0.20.0", "", { "dependencies": { "bun": "^1.3.14", "chokidar": "^4.0.3", "commander": "^14.0.3", "diff": "^8.0.3", "get-east-asian-width": "^1.5.0", "shell-quote": "1.9.0", "string-width": "^8.2.1", "zod": "^4.3.6" }, "optionalDependencies": { "hunkdiff-darwin-arm64": "0.20.0", "hunkdiff-darwin-x64": "0.20.0", "hunkdiff-linux-arm64": "0.20.0", "hunkdiff-linux-x64": "0.20.0", "hunkdiff-windows-x64": "0.20.0" }, "peerDependencies": { "@opentui/core": "^0.5.1", "@opentui/react": "^0.5.1", "@pierre/diffs": "1.3.5", "react": "^19.2.4" }, "optionalPeers": ["@pierre/diffs"], "bin": { "hunk": "bin/hunk.cjs", "hunkdiff": "bin/hunk.cjs" } }, "sha512-FfpgAOUEXb01dCMMsru8IJpC+y2KCJfn45E6hvzq5wKuFaYPTee3xU4rOYTgUUKO9ntRMhV4XlgSR1M/BiD33g=="], + "hunkdiff": ["hunkdiff@0.21.0", "", { "dependencies": { "chokidar": "^4.0.3", "commander": "^14.0.3", "diff": "^8.0.3", "get-east-asian-width": "^1.5.0", "shell-quote": "1.9.0", "string-width": "^8.2.1", "zod": "~4.4.3" }, "optionalDependencies": { "hunkdiff-darwin-arm64": "0.21.0", "hunkdiff-darwin-x64": "0.21.0", "hunkdiff-linux-arm64": "0.21.0", "hunkdiff-linux-x64": "0.21.0", "hunkdiff-windows-x64": "0.21.0" }, "peerDependencies": { "@opentui/core": "^0.5.6", "@opentui/react": "^0.5.6", "@pierre/diffs": "1.3.5", "react": "^19.2.4" }, "optionalPeers": ["@pierre/diffs"], "bin": { "hunk": "bin/hunk.cjs", "hunkdiff": "bin/hunk.cjs" } }, "sha512-s/+6G2s2T0sCxdvtRgCxOKtfzhAzXq+1lpFyKwgR8FQhYBxN7vU8UIlrWO6KP1u5JBwtFHaWLk3UFKFnveKt7g=="], - "hunkdiff-darwin-arm64": ["hunkdiff-darwin-arm64@0.20.0", "", { "os": "darwin", "cpu": "arm64", "bin": { "hunk": "bin/hunk" } }, "sha512-HDOeAIwbvma+VhDzy1rasDUvGTmKMxfBXfqyNaumbBdc8LIndNe/OkE0iIl969mzqsE/V32FqLmSezXCLH6aFQ=="], + "hunkdiff-darwin-arm64": ["hunkdiff-darwin-arm64@0.21.0", "", { "os": "darwin", "cpu": "arm64", "bin": { "hunk": "bin/hunk" } }, "sha512-LygRbyHDqpCzOaotS+A2gmRWxtgXGC8Q9ZkxmWH229KWQTp4SCjUBcR7IwXJj7jjsa8ycn5J/7RHsrLM0VeKSA=="], - "hunkdiff-darwin-x64": ["hunkdiff-darwin-x64@0.20.0", "", { "os": "darwin", "cpu": "x64", "bin": { "hunk": "bin/hunk" } }, "sha512-ssi3w1gffVwpJ5cyBkOn9WrySnUk7pfMaFwA4OALPf0pti/+ljoLEUleVnPxYzgP/nTUHX8Q6qCSG1LuDO8CMA=="], + "hunkdiff-darwin-x64": ["hunkdiff-darwin-x64@0.21.0", "", { "os": "darwin", "cpu": "x64", "bin": { "hunk": "bin/hunk" } }, "sha512-t1q5L3qKDvhxRjjLRO9v6p7SF241LRntFcgTVz+WVd1caQqnmbEFe0PEeA4lJxGhZRKsO4NYLv7vj+njimlvKw=="], - "hunkdiff-linux-arm64": ["hunkdiff-linux-arm64@0.20.0", "", { "os": "linux", "cpu": "arm64", "bin": { "hunk": "bin/hunk" } }, "sha512-m7ccTdWayHEiWYjWfG7q2YCHMN9J+0TrZrb6/yrmm5hSf9Sk96Ihzlp2+eviU60ME7SuuDDM1FH1X/wdE3j6wA=="], + "hunkdiff-linux-arm64": ["hunkdiff-linux-arm64@0.21.0", "", { "os": "linux", "cpu": "arm64", "bin": { "hunk": "bin/hunk" } }, "sha512-+r/HOb9KCapDZ89ON/ucikDyt7YWjfG79abZbB47U9+8Qu6vZdWzS/H/gXzjcsufBKrFluSpI0wV9BrbHzzy9Q=="], - "hunkdiff-linux-x64": ["hunkdiff-linux-x64@0.20.0", "", { "os": "linux", "cpu": "x64", "bin": { "hunk": "bin/hunk" } }, "sha512-GtZGqL5+rEIkICPGWipP4TQM53HzLRs4B8cbmXwVmTLRi/KuSz4DSsIfu3iJd8Z8/OBc5buskK+6+eQmnjPSjQ=="], + "hunkdiff-linux-x64": ["hunkdiff-linux-x64@0.21.0", "", { "os": "linux", "cpu": "x64", "bin": { "hunk": "bin/hunk" } }, "sha512-kzlCzt5vXp14XdbCfU1Q1weNtDjIq6B4pwpsIya+UVk7yvqWmiCzOuEG4oai371u/IJl9159j3BfSYzaC/SS1Q=="], - "hunkdiff-windows-x64": ["hunkdiff-windows-x64@0.20.0", "", { "os": "win32", "cpu": "x64", "bin": { "hunk": "bin/hunk.exe" } }, "sha512-yVzYboh4FNks+TGQ3acr21F7Kf7pNBerurQ2oOhFJJmDje9Q4+mhgcW/CDR8fGbL864iEuhoDSUs65PB3AUduA=="], + "hunkdiff-windows-x64": ["hunkdiff-windows-x64@0.21.0", "", { "os": "win32", "cpu": "x64", "bin": { "hunk": "bin/hunk.exe" } }, "sha512-mfUHj+Sy60isbgur/o7H350AEFgttGn4OnQHUFmfIJASbPecO4AWIePDKyyJnVzCXShPAwo188aKsJAuxWiAwg=="], "marked": ["marked@17.0.1", "", { "bin": { "marked": "bin/marked.js" } }, "sha512-boeBdiS0ghpWcSwoNm/jJBwdpFaMnZWRzjA6SkUMYb40SVaN1x7mmfGKp0jvexGcx+7y2La5zRZsYFZI6Qpypg=="], diff --git a/index.ts b/index.ts index 51338de..c54b3bd 100644 --- a/index.ts +++ b/index.ts @@ -13,6 +13,8 @@ import { createGitCommitBackend, findCommitBlocker } from "./src/git/commit"; import { autosquashCommand, createGitFixupBackend } from "./src/git/fixup"; import { listFixupTargets, type CommitChoice } from "./src/git/history"; import { createGit, type Git } from "./src/git/repository"; +import { reviewHasUncommittedWork } from "./src/review/provenance"; +import { createMarkedPane } from "./src/ui/markedPane"; import { createJjBackend } from "./src/jj/backend"; import { createJj, type Jj } from "./src/jj/repository"; import { listStagingTargets } from "./src/jj/revisions"; @@ -51,6 +53,20 @@ export default function activate(hunk: HunkExtensionAPI): void { highlight: ({ file }) => markHighlightsFor(file, session, hunk, contextMarks), }); + // Closed until asked for. The pane answers a question that only gets hard on + // a large review — "what exactly am I about to commit?" — and a reviewer + // marking two hunks in one file can already see the answer in the diff. + hunk.registerPane({ + id: "marked", + title: "Marked", + placement: "right", + // A fraction rather than a column count, so the list keeps its share of a + // wide terminal and still gives the diff room on a narrow one. The + // minimum is where a path stops being readable at all. + width: { preferred: 32, fraction: 0.22, min: 18, max: 48 }, + component: createMarkedPane(session.marks), + }); + hunk.registerCommand( { id: "toggleHunk", title: "Mark hunk", key: "x" }, (ctx) => { @@ -79,6 +95,16 @@ export default function activate(hunk: HunkExtensionAPI): void { }, ); + // Registering a pane does not show it, and this one has no `defaultOpen`: + // without a key it would be unreachable. `L` for list — `M` and `m` are + // Hunk's own menu bar and hunk headers, and `U` stays free for unstaging. + // Uppercase follows the convention here: lowercase `x` marks one hunk, and + // the capitals act on the set. + hunk.registerCommand( + { id: "toggleMarkedPane", title: "Show what is marked", key: "L" }, + (ctx) => ctx.panes.toggle("marked"), + ); + hunk.registerCommand({ id: "clearMarks", title: "Clear marks", key: "N" }, (ctx) => { session.marks.clear(); ctx.highlights.refresh(HIGHLIGHTER_ID); @@ -349,15 +375,19 @@ function markHighlightsFor( } try { - return buildMarkHighlights(parseFilePatch(file.patch), mark, contextMarks).map((highlight) => ({ - ...highlight, - // Amber, where the diff's own vocabulary is green, red, and neutral. A - // mark has to say "chosen", not "slightly lighter": the tones that only - // shift brightness disappear against an added line's green, and the two - // that carry meaning already — red for removed, near-white for the - // current search match — would either lie or flatten the diff's colours. - tone: "match" as const, - })); + // Tones come from the highlight itself, one per line, rather than being + // stamped on the whole set here. + // + // The lines that move take amber, where the diff's own vocabulary is + // green, red, and neutral: a mark has to say "chosen", and the two tones + // that carry meaning already — red for removed, near-white for the current + // search match — would either lie or flatten the diff's colours. + // + // Context takes `dim`, which the host added in API 16. Until then the only + // alternatives shifted brightness alone and vanished against an added + // line's green, so context had to borrow the amber and overstate the + // hunk's reach. It no longer does. + return buildMarkHighlights(parseFilePatch(file.patch), mark, contextMarks); } catch (error) { hunk.log(`Could not paint marks for ${file.path}: ${describe(error)}`); return null; @@ -391,6 +421,44 @@ function requireSelection(ctx: ExtensionCommandContext, session: ReviewSession): return selection; } +/** + * Refuse a review that holds no uncommitted work, reporting why. + * + * The guard every command here shares: they all treat the diff on screen as + * work that has not landed, and Hunk will just as happily show a commit + * (`hunk show`) or a comparison of two revisions (`hunk diff `). + * The extension cannot ask which of those it is looking at — the API hands it + * `sourceLabel` and `title`, both free-form display strings — so it asks the + * VCS what is uncommitted instead. + */ +async function requireWorkingCopy( + ctx: ExtensionCommandContext, + session: ReviewSession, + workspace: Workspace, +): Promise { + const run = + workspace.kind === "jj" + ? createJj({ root: workspace.root }).run + : createGit({ root: workspace.root }).run; + + const paths = session.reviewedFiles.map((file) => file.path); + + try { + if (await reviewHasUncommittedWork(paths, workspace, run)) { + return true; + } + } catch (error) { + // A VCS that cannot answer is not evidence of anything, so this reports + // the failure rather than silently letting the command through — the + // whole point of the check is that the dangerous case looks fine. + ctx.notify(messages.failed(describe(error)), "error"); + return false; + } + + ctx.notify(messages.notWorkingCopy, "warning"); + return false; +} + /** Resolve the workspace this review sits in, reporting when there is none. */ function requireWorkspace(ctx: ExtensionCommandContext): Workspace | null { const workspace = detectWorkspace(ctx.cwd); @@ -449,6 +517,10 @@ async function stage( return; } + if (!(await requireWorkingCopy(ctx, session, workspace))) { + return; + } + // Only the Jujutsu path needs a shell, to hand jj its selection. if (workspace.kind === "jj" && process.platform === "win32") { ctx.notify(messages.unsupportedPlatform, "error"); @@ -561,6 +633,10 @@ async function discard(ctx: ExtensionCommandContext, session: ReviewSession): Pr return; } + if (!(await requireWorkingCopy(ctx, session, workspace))) { + return; + } + const request = session.toStageRequest(selection.marks); const summary = session.summarise(selection.marks); diff --git a/package.json b/package.json index c0b0ab4..a83b079 100644 --- a/package.json +++ b/package.json @@ -8,7 +8,7 @@ "extensions": [ "./index.ts" ], - "apiVersion": 8 + "apiVersion": 16 }, "scripts": { "test": "bun test", @@ -16,7 +16,8 @@ }, "devDependencies": { "@types/bun": "^1.4.0", - "hunkdiff": "^0.20.0", + "@types/react": "^19.2.18", + "hunkdiff": "^0.21.0", "typescript": "^5.9.3" } } diff --git a/renovate.json5 b/renovate.json5 index e5d4d56..d166c73 100644 --- a/renovate.json5 +++ b/renovate.json5 @@ -52,18 +52,38 @@ automerge: true, }, - // hunkdiff is the one dependency CI cannot vouch for. It is imported only - // as `import type` (index.ts, src/ui/session.ts), so it is erased at compile - // time and never loaded at runtime — `bun test` passes identically on any - // version. Only `tsc --noEmit` covers it, and only for the 4 interfaces we - // import. It is also 0.x, where Renovate calls 0.20 -> 0.21 a "minor" even - // though that is breaking by convention. Green CI here means very little: - // read the changelog and load the extension in Hunk before merging. + // hunkdiff is the one dependency CI cannot vouch for. It is imported as + // `import type` in the extension itself (index.ts, src/ui/session.ts), so + // that much is erased at compile time and only `tsc --noEmit` covers it, + // for the 4 interfaces we name. The exception is test/manifest.test.ts, + // which imports HUNK_EXTENSION_API_VERSION as a *value* — that one module + // is really evaluated under `bun test`. Nothing else here changes + // behaviour with the version. It is also 0.x, where Renovate calls + // 0.20 -> 0.21 a "minor" even though that is breaking by convention, and + // where the extension API generation advances (0.20 declares 8, 0.21 + // declares 16) and drags our stated minimum Hunk version with it. Green CI + // means very little: read the release notes and load the extension in Hunk + // before merging. The `hunkdiff-upgrade` skill is that procedure. + // + // No automerge, and deliberately no cooldown either. The global 7-day + // minimumReleaseAge plus schedule:weekly put this PR 7-14 days behind the + // release, and every one of those days is spent not reading release notes + // we could have read on day one. Nothing merges itself here — the bump is + // finished by hand on a separate branch, and this PR serves mainly as the + // notification that a release happened. The cost we accept: CI runs + // `bun install` and evaluates that one module on the day the PR opens, + // rather than a week later. No trustedDependencies is set, so bun does not + // run install scripts. // // Listed last so it overrides the devDependencies rule above. { matchPackageNames: ["hunkdiff"], automerge: false, + // null opts out of the global cooldown, which also clears + // internalChecksFilter "strict" — with no pending age check there is + // nothing left for it to suppress. + minimumReleaseAge: null, + schedule: ["at any time"], }, ], } diff --git a/src/review/provenance.ts b/src/review/provenance.ts new file mode 100644 index 0000000..e571a30 --- /dev/null +++ b/src/review/provenance.ts @@ -0,0 +1,71 @@ +import type { Workspace } from "../workspace"; + +/** Runs one VCS command and returns its stdout. */ +export type RunVcs = (args: readonly string[]) => Promise; + +/** + * The paths the workspace currently reports as uncommitted. + * + * Untracked files count: a working-copy review includes them, so a review of + * nothing but a new file is still a review of uncommitted work. Jujutsu needs + * no equivalent question — it tracks new files into `@` on its own, so its + * diff already names them. + */ +async function uncommittedPaths(workspace: Workspace, run: RunVcs): Promise> { + const queries = + workspace.kind === "jj" + ? [["diff", "--name-only"]] + : [ + ["diff", "--name-only", "HEAD"], + ["ls-files", "--others", "--exclude-standard"], + ]; + + const outputs = await Promise.all(queries.map((args) => run(args))); + + return new Set( + outputs + .flatMap((output) => output.split("\n")) + .map((line) => line.trim()) + .filter((line) => line.length > 0), + ); +} + +/** + * True when the review on screen contains uncommitted work. + * + * Every command here rewrites the working copy or reads it as the source of a + * commit, so all of them assume the diff describes work that has not landed + * yet. Hunk does not promise that: `hunk show` reviews a commit, and + * `hunk diff ` compares two revisions, and neither has a working + * copy behind it. + * + * Usually that is caught anyway, further in, because the patch will not match + * what is on disk. The case that survives is a clean checkout: reviewing + * `HEAD~1 HEAD` there, the new side *is* what the files say, so every text + * check agrees and `D` edits the working copy — reverting lines the reviewer + * asked only to look at. The text is not lost, since the commit still holds + * it, but nobody asked for the edit, and an unnoticed one rides along into + * the next commit and undoes part of an earlier one. Content alone cannot + * tell a working copy from history that agrees with it, so this asks the VCS + * instead. + * + * Deliberately coarse. One question for the whole review, not one per file: + * a review holding any uncommitted change is a working-copy review, and the + * existing staleness and agreement checks are what catch a stale file inside + * it. The narrow answer would be to match the reviewed hunks against the + * working-copy diff, which means matching patches against a second source of + * truth to defend a case those checks already reach. + */ +export async function reviewHasUncommittedWork( + reviewedPaths: readonly string[], + workspace: Workspace, + run: RunVcs, +): Promise { + if (reviewedPaths.length === 0) { + return false; + } + + const uncommitted = await uncommittedPaths(workspace, run); + + return reviewedPaths.some((path) => uncommitted.has(path)); +} diff --git a/src/ui/highlights.ts b/src/ui/highlights.ts index b5103a4..2d5d481 100644 --- a/src/ui/highlights.ts +++ b/src/ui/highlights.ts @@ -4,11 +4,21 @@ import type { FileMark } from "../staging/plan"; /** How much of a marked hunk's context lines carries the mark. */ export type ContextMarks = "none" | "edge" | "full"; +/** + * How emphatically one painted line carries the mark. + * + * Named here rather than imported from the host so this module keeps knowing + * nothing about Hunk: the two values are spelled the same as the host's tones, + * and index.ts is where they meet its API. + */ +export type MarkTone = "match" | "dim"; + /** One painted line, in the shape Hunk's line highlighter contributes. */ export interface MarkHighlight { readonly side: "old" | "new"; readonly line: number; readonly range: readonly [number, number]; + readonly tone: MarkTone; } /** @@ -19,10 +29,12 @@ export interface MarkHighlight { * * Context lines are a judgement call, so they are a setting. Marking them says * how far the hunk reaches, which is real information — but a hunk carries up - * to three lines of context on each side, so it also makes the marked region - * look considerably larger than what will actually move. `"none"` is the - * default for that reason; `"edge"` draws a thin rail down the hunk, and - * `"full"` paints context exactly like the lines that move. + * to three lines of context on each side, so it also risks making the marked + * region look larger than what will actually move. They are painted `dim` + * rather than in the mark's own colour, which is what keeps that risk in + * check: the eye reads the amber as the change and the recessive tint as its + * extent, so `"edge"` draws a thin rail down the hunk and `"full"` traces the + * whole reach without either one competing with the lines that move. * * A context line is addressed on both sides, since it exists on both and a * split layout renders it twice. @@ -61,17 +73,17 @@ export function buildMarkHighlights( const range = [0, Math.max(line.text.length, 1)] as const; if (line.kind === "added") { - highlights.push({ side: "new", line: newLine, range }); + highlights.push({ side: "new", line: newLine, range, tone: "match" }); newLine += 1; } else if (line.kind === "removed") { - highlights.push({ side: "old", line: oldLine, range }); + highlights.push({ side: "old", line: oldLine, range, tone: "match" }); oldLine += 1; } else { if (contextMarks !== "none") { const width = contextMarks === "edge" ? Math.min(range[1], EDGE_WIDTH) : range[1]; const contextRange = [0, width] as const; - highlights.push({ side: "old", line: oldLine, range: contextRange }); - highlights.push({ side: "new", line: newLine, range: contextRange }); + highlights.push({ side: "old", line: oldLine, range: contextRange, tone: "dim" }); + highlights.push({ side: "new", line: newLine, range: contextRange, tone: "dim" }); } oldLine += 1; newLine += 1; diff --git a/src/ui/markedPane.tsx b/src/ui/markedPane.tsx new file mode 100644 index 0000000..8a861d3 --- /dev/null +++ b/src/ui/markedPane.tsx @@ -0,0 +1,64 @@ +import { useSyncExternalStore } from "react"; +import type { ExtensionPaneProps } from "hunkdiff/extension"; +import type { MarkStore } from "./marks"; +import { fitPath, markedFiles } from "./markedSet"; + +/** + * The marked set, listed beside the review. + * + * What it is for: marks are otherwise visible only as amber inside the diff, + * so a reviewer who marked hunks in three files eleven screens apart cannot + * see the set they are about to commit without scrolling back through all of + * it. The confirmation dialog counts the set but cannot name it. + * + * State lives in the MarkStore, not here. Panes unmount when closed, so a + * component holding the marks would lose them the moment the pane was hidden + * — `useSyncExternalStore` reads the same store the commands write to, and + * the store hands back a snapshot whose identity only changes when a mark + * does. + */ +export function createMarkedPane(marks: MarkStore) { + return function MarkedPane({ files, selectedFileId, width, theme }: ExtensionPaneProps) { + const snapshot = useSyncExternalStore( + (listener) => marks.subscribe(listener), + () => marks.snapshot(), + ); + + // The cursor comes from props rather than from a `hunk_viewed` + // subscription: Hunk re-renders the pane with the live selection, so an + // event handler would be fetching what is already in hand. + const marked = markedFiles(files, snapshot, selectedFileId); + const hunks = marked.reduce((total, file) => total + file.hunks, 0); + + if (marked.length === 0) { + return ( + + Nothing marked. + Press x on a hunk, or X for a file. + + ); + } + + return ( + + + {hunks} {hunks === 1 ? "hunk" : "hunks"} in {marked.length}{" "} + {marked.length === 1 ? "file" : "files"} + + {marked.map((file) => ( + + {/* The count is what the reviewer is checking, so it leads. A + whole-file mark says so rather than showing a number that + would be indistinguishable from marking every hunk by hand. */} + {file.whole ? "all" : String(file.hunks).padStart(3)}{" "} + {fitPath(file.path, Math.max(width - 8, 8))} + + ))} + + ); + }; +} diff --git a/src/ui/markedSet.ts b/src/ui/markedSet.ts new file mode 100644 index 0000000..7cf5b7b --- /dev/null +++ b/src/ui/markedSet.ts @@ -0,0 +1,79 @@ +import type { FileMark } from "../staging/plan"; + +/** One file of the review that carries at least one mark. */ +export interface MarkedFile { + readonly fileId: string; + readonly path: string; + /** Marked hunks, or every hunk when the whole file is marked. */ + readonly hunks: number; + readonly whole: boolean; + /** + * True for the file the review cursor is in. + * + * A long marked list is read against a diff that is scrolled somewhere + * inside it, and without this the two have nothing in common to look at: + * the list says what is marked, the diff says where you are, and neither + * says which row of the list you are standing on. + */ + readonly current: boolean; +} + +/** A file of the review, as summarising the marked set needs to see it. */ +export interface SummarisableFile { + readonly id: string; + readonly path: string; + readonly hunks?: readonly unknown[]; +} + +/** + * The marked set, in review order. + * + * Review order rather than mark order: the list is read against the diff + * beside it, so it has to agree with what scrolling would show. Remembering + * the order marks were made in would be a second, invisible ordering nobody + * asked about. + * + * A whole-file mark reports its file's hunk count, falling back to one for the + * files that have no hunks to count — binary and oversized ones, which can + * only ever be marked whole. + */ +export function markedFiles( + files: readonly SummarisableFile[], + marks: ReadonlyMap, + currentFileId: string | null = null, +): MarkedFile[] { + const marked: MarkedFile[] = []; + + for (const file of files) { + const mark = marks.get(file.id); + if (!mark) { + continue; + } + + marked.push({ + fileId: file.id, + path: file.path, + hunks: mark.kind === "whole" ? Math.max(file.hunks?.length ?? 0, 1) : mark.hunks.size, + whole: mark.kind === "whole", + current: file.id === currentFileId, + }); + } + + return marked; +} + +/** + * Shorten a path to fit a pane, keeping the end. + * + * The end is what identifies a file — `…/ui/markedSet.ts` still reads as one + * file, while a path cut at the front does not. An ellipsis says the front was + * dropped rather than leaving a plausible-looking relative path that is not + * the file's real one. + */ +export function fitPath(path: string, width: number): string { + if (width <= 1 || path.length <= width) { + return path; + } + + return `…${path.slice(-(width - 1))}`; +} diff --git a/src/ui/marks.ts b/src/ui/marks.ts index ef740f4..ce5e5d8 100644 --- a/src/ui/marks.ts +++ b/src/ui/marks.ts @@ -11,6 +11,19 @@ import type { FileMark } from "../staging/plan"; */ export class MarkStore { private readonly marks = new Map(); + private readonly listeners = new Set<() => void>(); + + /** + * The marks as one immutable value, stable until they change. + * + * Two callers want different things from this and both are served by the + * same object. A command captures the marks before opening a dialog and + * needs them not to move underneath it; the pane re-reads them constantly + * and needs to know cheaply whether anything happened. Rebuilding the map + * on every read would break the second — a fresh object every time reads as + * a change every time — so it is rebuilt only when a mark actually moves. + */ + private cached: ReadonlyMap = new Map(); /** Add or remove one hunk. Marking a hunk on a whole-file mark narrows it. */ toggleHunk(fileId: string, hunkIndex: number, hunkCount: number): void { @@ -41,6 +54,7 @@ export class MarkStore { clear(): void { this.marks.clear(); + this.changed(); } markFor(fileId: string): FileMark | undefined { @@ -62,7 +76,13 @@ export class MarkStore { /** The marks, keyed by file id, for handing to staging. */ snapshot(): ReadonlyMap { - return new Map(this.marks); + return this.cached; + } + + /** Watch for changes, for a surface that paints the marked set. */ + subscribe(listener: () => void): () => void { + this.listeners.add(listener); + return () => this.listeners.delete(listener); } private set(fileId: string, mark: FileMark | undefined): void { @@ -71,5 +91,15 @@ export class MarkStore { } else { this.marks.delete(fileId); } + + this.changed(); + } + + private changed(): void { + this.cached = new Map(this.marks); + + for (const listener of this.listeners) { + listener(); + } } } diff --git a/src/ui/messages.ts b/src/ui/messages.ts index bfd24ad..d7caaac 100644 --- a/src/ui/messages.ts +++ b/src/ui/messages.ts @@ -26,6 +26,10 @@ export const messages = { noWorkspace: "This review is not inside a git or Jujutsu workspace", + notWorkingCopy: + "Nothing in this review is uncommitted — these commands act on your working copy, " + + "and this looks like a review of committed history", + unsupportedPlatform: "Staging into Jujutsu needs a POSIX shell to hand jj the selection, which this platform does not provide", diff --git a/test/highlights.test.ts b/test/highlights.test.ts index cc07753..362eb90 100644 --- a/test/highlights.test.ts +++ b/test/highlights.test.ts @@ -22,36 +22,36 @@ describe("buildMarkHighlights", () => { test("marks only the lines that move by default", () => { expect(buildMarkHighlights(patch, { kind: "hunks", hunks: new Set([0]) })).toEqual([ - { side: "old", line: 2, range: [0, 4] }, - { side: "new", line: 2, range: [0, 4] }, + { side: "old", line: 2, range: [0, 4], tone: "match" }, + { side: "new", line: 2, range: [0, 4], tone: "match" }, ]); }); test("draws a rail down the hunk when context marks are set to edge", () => { expect(buildMarkHighlights(patch, { kind: "hunks", hunks: new Set([0]) }, "edge")).toEqual([ - { side: "old", line: 1, range: [0, 2] }, - { side: "new", line: 1, range: [0, 2] }, - { side: "old", line: 2, range: [0, 4] }, - { side: "new", line: 2, range: [0, 4] }, - { side: "old", line: 3, range: [0, 2] }, - { side: "new", line: 3, range: [0, 2] }, + { side: "old", line: 1, range: [0, 2], tone: "dim" }, + { side: "new", line: 1, range: [0, 2], tone: "dim" }, + { side: "old", line: 2, range: [0, 4], tone: "match" }, + { side: "new", line: 2, range: [0, 4], tone: "match" }, + { side: "old", line: 3, range: [0, 2], tone: "dim" }, + { side: "new", line: 3, range: [0, 2], tone: "dim" }, ]); }); test("paints context like the moving lines when set to full", () => { expect(buildMarkHighlights(patch, { kind: "hunks", hunks: new Set([0]) }, "full")).toEqual([ - { side: "old", line: 1, range: [0, 5] }, - { side: "new", line: 1, range: [0, 5] }, - { side: "old", line: 2, range: [0, 4] }, - { side: "new", line: 2, range: [0, 4] }, - { side: "old", line: 3, range: [0, 5] }, - { side: "new", line: 3, range: [0, 5] }, + { side: "old", line: 1, range: [0, 5], tone: "dim" }, + { side: "new", line: 1, range: [0, 5], tone: "dim" }, + { side: "old", line: 2, range: [0, 4], tone: "match" }, + { side: "new", line: 2, range: [0, 4], tone: "match" }, + { side: "old", line: 3, range: [0, 5], tone: "dim" }, + { side: "new", line: 3, range: [0, 5], tone: "dim" }, ]); }); test("tracks line numbers across context when a hunk only adds", () => { expect(buildMarkHighlights(patch, { kind: "hunks", hunks: new Set([1]) })).toEqual([ - { side: "new", line: 11, range: [0, 8] }, + { side: "new", line: 11, range: [0, 8], tone: "match" }, ]); }); @@ -59,6 +59,18 @@ describe("buildMarkHighlights", () => { expect(buildMarkHighlights(patch, { kind: "whole" })).toHaveLength(3); }); + test("separates the lines that move from the reach of the hunk", () => { + const tones = new Set( + buildMarkHighlights(patch, { kind: "hunks", hunks: new Set([0]) }, "full").map( + (highlight) => `${highlight.tone}:${highlight.range[1]}`, + ), + ); + + // The point of the split: whatever width context is given, it never + // carries the tone that means "this is what will move". + expect(tones).toEqual(new Set(["match:4", "dim:5"])); + }); + test("gives an empty line a range even though nothing can paint it", () => { const withBlank = parseFilePatch(`--- a/f +++ b/f @@ -68,7 +80,7 @@ describe("buildMarkHighlights", () => { `); expect(buildMarkHighlights(withBlank, { kind: "whole" })).toEqual([ - { side: "new", line: 2, range: [0, 1] }, + { side: "new", line: 2, range: [0, 1], tone: "match" }, ]); }); }); diff --git a/test/manifest.test.ts b/test/manifest.test.ts new file mode 100644 index 0000000..174b9f7 --- /dev/null +++ b/test/manifest.test.ts @@ -0,0 +1,18 @@ +import { describe, expect, test } from "bun:test"; +import { HUNK_EXTENSION_API_VERSION } from "hunkdiff/extension"; +import manifest from "../package.json"; + +describe("package.json", () => { + test("declares the API generation of the Hunk it is installed beside", () => { + // Drift here is silent: the host only refuses an extension declaring a + // generation HIGHER than its own, so a dependency bump that leaves + // `apiVersion` behind keeps loading and keeps working — right up until + // someone reaches for an API the declaration does not claim. A caret + // range means the bump arrives on its own, so this is what notices. + expect(manifest.hunk.apiVersion).toBe(HUNK_EXTENSION_API_VERSION); + }); + + test("points Hunk at the entry file this repository actually has", () => { + expect(manifest.hunk.extensions).toEqual(["./index.ts"]); + }); +}); diff --git a/test/markedSet.test.ts b/test/markedSet.test.ts new file mode 100644 index 0000000..41ee055 --- /dev/null +++ b/test/markedSet.test.ts @@ -0,0 +1,97 @@ +import { describe, expect, test } from "bun:test"; +import { fitPath, markedFiles } from "../src/ui/markedSet"; + +const files = [ + { id: "1", path: "src/a.ts", hunks: [{}, {}, {}] }, + { id: "2", path: "src/b.ts", hunks: [{}, {}] }, + { id: "3", path: "logo.png" }, +]; + +describe("markedFiles", () => { + test("lists nothing when nothing is marked", () => { + expect(markedFiles(files, new Map())).toEqual([]); + }); + + test("reports how many hunks each marked file carries", () => { + const marked = markedFiles( + files, + new Map([["1", { kind: "hunks", hunks: new Set([0, 2]) }]]), + ); + + expect(marked).toEqual([ + { fileId: "1", path: "src/a.ts", hunks: 2, whole: false, current: false }, + ]); + }); + + test("counts a whole-file mark as every hunk of the file", () => { + const marked = markedFiles(files, new Map([["2", { kind: "whole" }]])); + + expect(marked).toEqual([ + { fileId: "2", path: "src/b.ts", hunks: 2, whole: true, current: false }, + ]); + }); + + test("counts a file with no hunks as one, since it can only be marked whole", () => { + // Binary and oversized files: Hunk shows no hunks to mark, so X is the + // only way to include them and "0 hunks" would misreport a real choice. + const marked = markedFiles(files, new Map([["3", { kind: "whole" }]])); + + expect(marked[0]).toMatchObject({ path: "logo.png", hunks: 1, whole: true }); + }); + + test("follows review order, not the order the marks were made", () => { + const marks = new Map([ + ["2", { kind: "whole" }], + ["1", { kind: "whole" }], + ]); + + expect(markedFiles(files, marks).map((file) => file.path)).toEqual(["src/a.ts", "src/b.ts"]); + }); + + test("ignores a mark whose file left the review", () => { + expect(markedFiles(files, new Map([["gone", { kind: "whole" }]]))).toEqual([]); + }); +}); + +describe("markedFiles and the review cursor", () => { + const everything = new Map([ + ["1", { kind: "whole" }], + ["2", { kind: "whole" }], + ]); + + test("marks the row for the file the cursor is in", () => { + const marked = markedFiles(files, everything, "2"); + + expect(marked.map((file) => file.current)).toEqual([false, true]); + }); + + test("marks no row when the cursor is in an unmarked file", () => { + // Common while marking: you moved on to read the next file and have not + // decided about it yet. Nothing in the list should claim to be current. + const marked = markedFiles(files, everything, "3"); + + expect(marked.some((file) => file.current)).toBe(false); + }); + + test("marks no row when there is no cursor at all", () => { + expect(markedFiles(files, everything).some((file) => file.current)).toBe(false); + }); +}); + +describe("fitPath", () => { + test("leaves a path that already fits", () => { + expect(fitPath("src/a.ts", 20)).toBe("src/a.ts"); + }); + + test("keeps the end of a path too long to show", () => { + // The end identifies the file; a path cut at the front would read as a + // real relative path that does not exist. + expect(fitPath("very/deep/nested/file.ts", 12)).toBe("…ted/file.ts"); + }); + + test("never returns more characters than it was given", () => { + for (const width of [2, 5, 8, 13]) { + expect(fitPath("very/deep/nested/file.ts", width).length).toBe(width); + } + }); +}); diff --git a/test/marks.test.ts b/test/marks.test.ts index 8e5d99c..7d24ddc 100644 --- a/test/marks.test.ts +++ b/test/marks.test.ts @@ -60,3 +60,47 @@ describe("MarkStore", () => { expect(marks.isEmpty).toBe(true); }); }); + +describe("MarkStore subscriptions", () => { + test("notifies a listener when a mark changes", () => { + const store = new MarkStore(); + let calls = 0; + store.subscribe(() => { + calls += 1; + }); + + store.toggleHunk("a", 0, 3); + store.toggleFile("b"); + store.clear(); + + expect(calls).toBe(3); + }); + + test("hands back the same snapshot until a mark moves", () => { + // What useSyncExternalStore needs: a fresh object on every read would be + // an infinite render, and a stale one after a change would never repaint. + const store = new MarkStore(); + const before = store.snapshot(); + + expect(store.snapshot()).toBe(before); + + store.toggleHunk("a", 0, 3); + + expect(store.snapshot()).not.toBe(before); + expect(store.snapshot()).toBe(store.snapshot()); + }); + + test("stops notifying once unsubscribed", () => { + const store = new MarkStore(); + let calls = 0; + const unsubscribe = store.subscribe(() => { + calls += 1; + }); + + store.toggleFile("a"); + unsubscribe(); + store.toggleFile("b"); + + expect(calls).toBe(1); + }); +}); diff --git a/test/provenance.test.ts b/test/provenance.test.ts new file mode 100644 index 0000000..9c29eeb --- /dev/null +++ b/test/provenance.test.ts @@ -0,0 +1,108 @@ +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { createGit } from "../src/git/repository"; +import { reviewHasUncommittedWork } from "../src/review/provenance"; +import { createTestGitRepository, hasGit, type TestGitRepository } from "./support/gitRepo"; + +/** Records what was asked, and answers with canned stdout. */ +function fakeVcs(answers: Record) { + const asked: string[][] = []; + + return { + asked, + run: async (args: readonly string[]) => { + asked.push([...args]); + return answers[args.join(" ")] ?? ""; + }, + }; +} + +const git = { kind: "git", root: "/repo" } as const; +const jj = { kind: "jj", root: "/repo" } as const; + +describe("reviewHasUncommittedWork", () => { + test("sees a reviewed file that git reports as modified", async () => { + const vcs = fakeVcs({ "diff --name-only HEAD": "src/a.ts\nsrc/b.ts\n" }); + + expect(await reviewHasUncommittedWork(["src/b.ts"], git, vcs.run)).toBe(true); + }); + + test("counts an untracked file as uncommitted work", async () => { + // The case a name-only diff alone would miss: a review whose only content + // is a file git has never seen is still a review of the working copy. + const vcs = fakeVcs({ "ls-files --others --exclude-standard": "new.ts\n" }); + + expect(await reviewHasUncommittedWork(["new.ts"], git, vcs.run)).toBe(true); + }); + + test("refuses a review whose files are all committed", async () => { + const vcs = fakeVcs({ "diff --name-only HEAD": "somewhere/else.ts\n" }); + + expect(await reviewHasUncommittedWork(["src/a.ts"], git, vcs.run)).toBe(false); + }); + + test("refuses an empty review without asking the VCS anything", async () => { + const vcs = fakeVcs({}); + + expect(await reviewHasUncommittedWork([], git, vcs.run)).toBe(false); + expect(vcs.asked).toEqual([]); + }); + + test("asks jj one question, since it tracks new files itself", async () => { + const vcs = fakeVcs({ "diff --name-only": "src/a.ts\n" }); + + expect(await reviewHasUncommittedWork(["src/a.ts"], jj, vcs.run)).toBe(true); + expect(vcs.asked).toEqual([["diff", "--name-only"]]); + }); + + test("lets one uncommitted file carry the whole review", async () => { + // Deliberate: the check is per review, not per file. A stale file inside a + // working-copy review is what the staleness and agreement checks are for. + const vcs = fakeVcs({ "diff --name-only HEAD": "dirty.ts\n" }); + + expect(await reviewHasUncommittedWork(["committed.ts", "dirty.ts"], git, vcs.run)).toBe(true); + }); +}); + +describe("reviewHasUncommittedWork against a real repository", () => { + let repository: TestGitRepository; + let available = false; + + beforeAll(async () => { + available = await hasGit(); + if (available) { + repository = await createTestGitRepository(); + } + }); + + afterAll(async () => { + await repository?.dispose(); + }); + + test("refuses the clean checkout that every text check would pass", async () => { + if (!available) { + return; + } + + // The one case content cannot catch: reviewing `HEAD~1 HEAD` with nothing + // uncommitted. The new side is exactly what is on disk, so the patch + // matches the file perfectly — and discarding it would edit the working + // copy nobody asked to change. + await repository.write("a.ts", "one\n"); + await repository.git("add", "a.ts"); + await repository.git("commit", "-qm", "first"); + await repository.write("a.ts", "two\n"); + await repository.git("commit", "-qam", "second"); + + const run = createGit({ root: repository.root }).run; + + expect(await reviewHasUncommittedWork(["a.ts"], { kind: "git", root: repository.root }, run)).toBe( + false, + ); + + await repository.write("a.ts", "three\n"); + + expect(await reviewHasUncommittedWork(["a.ts"], { kind: "git", root: repository.root }, run)).toBe( + true, + ); + }); +}); diff --git a/tsconfig.json b/tsconfig.json index fea75a3..82ca274 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -11,7 +11,14 @@ "noFallthroughCasesInSwitch": true, "verbatimModuleSyntax": true, "noEmit": true, - "skipLibCheck": true + "skipLibCheck": true, + + // The marked-hunks pane is a React component rendered by Hunk's own + // OpenTUI runtime. `jsxImportSource` points at the host's renderer rather + // than React DOM, and nothing here bundles React: Hunk serves one copy to + // every extension, and a second would break hooks. + "jsx": "react-jsx", + "jsxImportSource": "@opentui/react" }, - "include": ["index.ts", "src/**/*.ts", "test/**/*.ts"] + "include": ["index.ts", "src/**/*.ts", "src/**/*.tsx", "test/**/*.ts"] }