pull request review · captured revision

Unified diff in the switch picker

Worktrunk PR #3865 adds a unified-diff preview for the switch picker. The captured revision is awaiting a merge decision; its exact 46-file patch remains available below.

Pull request description and checks

Review finding

The current revision is safe to approve with two follow-ups. PreparedDiff owns each complete Git diff invocation; TempIndex includes untracked files without changing the real index; and the picker loads off-screen worktrees only when selected.

Review found sparse-checkout divergence, a shared temporary directory, a symlink race while copying the index, and stale prewarming claims. The current revision corrects all four. Two follow-ups remain: selected-row demand latency has no benchmark, and a pre-existing wt step commit --dry-run gap can still stage its own temporary-index artifact when TMPDIR sits inside the worktree.

Change surface · 46 files, 191 hunks

Change surface

Risk is concentrated in the Git repository layer and picker runtime. Tests, documentation, and benchmarks account for most of the remaining files.

Files: 46
Hunks: 191
Patch lines: 4,126
Patch bytes: 214 KB
Commits: 6
Changed lines: 2,268

Changed paths grouped by implementation area.
Review unit	Files	Change	Review focus
1 · Git repository layer	4	`+312 −30`	Untracked files enter while the real index remains byte-identical.
2 · Picker runtime	17	`+823 −466`	Off-screen worktrees load on demand without breaking tab or row identity.
3 · Integration tests	8	`+308 −92`	Tests observe index bytes and filesystem residue.
4 · Other runtime	6	`+42 −61`	Prior diff callers move to the shared abstraction.
5 · Docs and shipped skills	9	`+69 −42`	The Git 2.34 floor is discoverable in every published copy.
6 · Benchmarks	2	`+13 −10`	Prewarm cost moves to selected-row demand.

Path hierarchy · 46 files

max-sixty/worktrunk #3865
  src/
    commands/picker/  17 files  +823 −466
    git/repository/    4 files  +312 −30
    other              6 files  +42  −61
  tests/               8 files  +308 −92
  docs + shipped skills
                       9 files  +69  −42
  benches/             2 files  +13  −10

What changes at runtime

The behavior view keeps the picker path fixed and changes the default preview in place. The call view is a scoped static analysis of the orchestration file, so every source coordinate leads back to the exact patch.

One picker path, changes in place

− old default unchanged boundary + new path

flowchart LR
  Row[Selected row] --> Mode[Preview mode]
  Mode -. old default .-> Working["− working-tree diff"]
  Mode --> Unified["+ unified diff"]
  Mode --> Tabs[Working / committed tabs]
  Unified --> Prepared[PreparedDiff]
  Prepared --> History[Committed changes]
  Prepared --> Temp[TempIndex]
  Temp --> Staged[Staged]
  Temp --> Unstaged[Unstaged]
  Temp --> Untracked[Untracked]
  classDef removed fill:var(--del-tint),stroke:var(--danger),color:var(--danger)
  classDef added fill:var(--ok-tint),stroke:var(--ok),color:var(--ok-ink)
  class Working removed
  class Unified,Prepared,Temp added

One unchanged root, replacement calls in context

This is the exact semantic output of calldiff@0.5.0 diff … --entry compute_combined_diff --locs --max-depth 1. The stable compute_combined_diff root keeps its neighboring calls while direct Git command assembly disappears and PreparedDiff calls appear in the same branch. Comment on any row, or follow a location into the authoritative patch.

The orchestration hunk the picker uses

This exact hunk from src/summary.rs replaces direct Git argument assembly with the repository’s PreparedDiff API.

diff --git a/src/summary.rs b/src/summary.rs
--- a/src/summary.rs
+++ b/src/summary.rs
@@ -258,16 +256,14 @@ pub(crate) fn compute_combined_diff(
 
     // Working tree diff: uncommitted changes
     if let Some(wt_path) = worktree_path {
-        let path = wt_path.display().to_string();
-        if let Ok(wt_stat) = repo.run_command(&["-C", &path, "diff", "HEAD", "--stat"])
+        let prepared = repo.worktree_at(wt_path).prepare_diff(["HEAD"]);
+        if let Ok(wt_stat) = prepared.capture(["--stat"])
             && !wt_stat.trim().is_empty()
         {
             stat.push_str(&wt_stat);
         }
-        let mut wt_diff_args = vec!["-C", &path];
-        wt_diff_args.extend(DIFF_PREFIX_OVERRIDES);
-        wt_diff_args.extend(["diff", "HEAD"]);
-        if let Ok(wt_diff) = repo.run_command(&wt_diff_args)
+        if let Ok(wt_diff) =
+            prepared.capture_with_git_options(DIFF_PREFIX_OVERRIDES, std::iter::empty::<&str>())
             && !wt_diff.trim().is_empty()
         {
             diff.push_str(&wt_diff);

Review invariants

The implementation is expected to preserve these three properties.

assert_eq!(repo.read_index_bytes(&worktree)?, index_before_preview, "preview preparation rewrote {worktree:?}");
assert!(temp_index.is_operation_scoped());
assert!(offscreen_worktree_preview.is_demand_loaded());

A clean status is insufficient: preview preparation must preserve the real index byte for byte.

Random, exclusive temporary files prevent concurrent previews from sharing or replacing one another’s index.

Moving off-screen worktrees out of prewarm protects startup, but leaves selected-row latency as a separate measurement question.

Review history · four fixes in review

Review history

PR opened The first revision introduced the unified picker tab, shared diff preparation, and broad integration coverage.

First review withheld approval It found divergent sparse handling, a predictable shared temp directory, startup-cost uncertainty, and documentation drift.

Temp indexes became random, exclusive files Sparse registration and worktree-local temp-directory exclusion gained focused tests.

The index copy kept ownership of its pathname Writing through the still-open file closed the symlink race, and off-screen worktrees moved out of eager prewarm.

Agent review recommends approval with follow-ups The shared TempIndex path and orchestration prose converged; selected-row latency and dry-run temp artifacts remained visible notes.

Exact patch · 46 files

GitHub’s captured patch for revision 9ca8f78b. All 46 files start closed and remain independently commentable.

Evidence matrix · four proven claims, two gaps

Evidence and remaining gaps

What the final revision demonstrates and what remains unmeasured.
Claim	Evidence	Status	Review reading
Unified and subsidiary tabs show the intended changes	`unified_diff_is_net_change_and_subsidiary_diffs_include_untracked`	passed	The same test requires the real index to remain byte-identical.
Sparse checkouts retain out-of-cone untracked files	`test_picker_dry_run_includes_untracked_outside_sparse_checkout`	passed	Exercises the regression the first review found.
Worktree-local temp directories leave no index artifacts	`test_picker_dry_run_tempdir_inside_worktree_has_no_index_artifacts`	passed	Also makes glob escaping load-bearing through a bracketed path.
Off-screen worktrees are demand-loaded	`initial_precompute_skips_offscreen_worktrees_but_warms_branches`	passed	Proves scheduling shape, not selected-row latency.
Selected-row demand remains fast	No latency benchmark	gap	The correctness test allows up to 60 seconds and cannot answer this.
Commit dry-run excludes its own temp index under local TMPDIR	No focused regression in this PR	gap	The captured hunk moves the existing staging path onto `TempIndex` without changing its `git add` semantics; a reviewer reproduced the extra index and lock entries with plain Git.

Exact PreparedDiff source at the current revision

This 169-line repository slice contains the source-owning abstraction and all three preparation entry points.