From 69a343401fe2a6bb7fc5993c4e428b2053157751 Mon Sep 17 00:00:00 2001 From: Xin Date: Tue, 6 Oct 2026 21:38:44 +0100 Subject: [PATCH] feat(branch): compare two branches without checking either out `diffs branch BASE HEAD` reviews HEAD against BASE (like `git diff BASE...HEAD`) without HEAD being the checked-out branch. - git::branch_diff takes an optional head ref; --include-dirty conflicts with it (CLI) and is a 400 from /api/branch-diff. - /api/blob accepts rev= with path, so pure renames load from the reviewed branch instead of the working tree. - Local comments accept an optional branch scope (?branch=), so threads on a non-checked-out branch are filed under it. - Web forwards ?head= and keeps it when switching base. --- README.md | 3 +- src/cli.rs | 95 +++++--- src/comments.rs | 223 ++++++++++++------- src/git.rs | 71 +++++- src/server.rs | 92 ++++++-- tests/diff_parity.rs | 25 ++- web/src/components/DiffView.tsx | 42 +++- web/src/components/Home.tsx | 2 +- web/src/components/diff-view/DiffToolbar.tsx | 31 ++- 9 files changed, 429 insertions(+), 155 deletions(-) diff --git a/README.md b/README.md index 5487109..eb2c836 100644 --- a/README.md +++ b/README.md @@ -43,11 +43,12 @@ diffs pr 123 # PR in the current repo diffs pr org/repo/pull/123 # PR in any repo ``` -Review the current branch against a base locally: +Review a branch against a base locally (the current branch unless a head is given): ```sh diffs branch # infers the base diffs branch main # explicit base +diffs branch main feature # compare two branches without checking out either diffs branch --include-dirty # include staged, unstaged, and untracked changes ``` diff --git a/src/cli.rs b/src/cli.rs index 3b9565b..f2419ed 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -75,6 +75,9 @@ Examples: # Compare against an explicit base diffs branch main + # Compare two branches without checking either out + diffs branch main feature/login + # Include uncommitted changes diffs branch --include-dirty "; @@ -157,14 +160,17 @@ enum Command { #[command(flatten)] serve: ServeFlags, }, - #[command(about = "Review commits on the current branch against a base")] + #[command(about = "Review commits on a branch against a base")] #[command(after_help = BRANCH_AFTER_HELP)] Branch { /// Base ref to compare against (inferred from the PR or default branch when omitted) #[arg(value_name = "BASE")] base: Option, + /// Branch to review (defaults to the checked-out branch) + #[arg(value_name = "HEAD", requires = "base")] + head: Option, /// Include staged, unstaged, and untracked changes - #[arg(long)] + #[arg(long, conflicts_with = "head")] include_dirty: bool, #[command(flatten)] serve: ServeFlags, @@ -257,6 +263,7 @@ pub async fn run(started: Instant) -> anyhow::Result<()> { } Some(Command::Branch { base, + head, include_dirty, serve, }) => { @@ -265,7 +272,15 @@ pub async fn run(started: Instant) -> anyhow::Result<()> { // "could not infer base ref"). resolve_repo_root_or_help(&cli.dir)?; let base = resolve_branch_base(base, &cli.dir).await?; - let target = branch_target(&base, include_dirty); + let head = head + .map(|value| value.trim().to_string()) + .filter(|value| !value.is_empty()); + if let Some(head) = &head + && !git::ref_exists(&cli.dir, head) + { + bail!("unknown head ref: {head}"); + } + let target = branch_target(&base, head.as_deref(), include_dirty); run_server_target(&cli.dir, &serve, gh_host(None), &target, true, started).await } Some(Command::Comments(command)) => run_comments(&cli.dir, command), @@ -521,8 +536,11 @@ fn target_label(target_path: &str, cwd: &std::path::Path) -> String { }; } if target_path.starts_with("/branch") { - let base = branch_base_from_target(target_path); - let head = git::branch(cwd); + let base = target_query_param(target_path, "base"); + let mut head = target_query_param(target_path, "head"); + if head.is_empty() { + head = git::branch(cwd); + } let head = if head.is_empty() { "HEAD" } else { &head }; return if base.is_empty() { format!("{head} branch diff") @@ -537,12 +555,12 @@ fn target_label(target_path: &str, cwd: &std::path::Path) -> String { target_path.to_string() } -fn branch_base_from_target(target_path: &str) -> String { +fn target_query_param(target_path: &str, name: &str) -> String { let Some((_, query)) = target_path.split_once('?') else { return String::new(); }; url::form_urlencoded::parse(query.as_bytes()) - .find(|(key, _)| key == "base") + .find(|(key, _)| key == name) .map(|(_, value)| value.into_owned()) .unwrap_or_default() } @@ -615,9 +633,12 @@ async fn resolve_branch_base(base: Option, dir: &PathBuf) -> anyhow::Res bail!("could not infer base ref; pass one explicitly, e.g. `diffs branch main`") } -fn branch_target(base: &str, include_dirty: bool) -> String { +fn branch_target(base: &str, head: Option<&str>, include_dirty: bool) -> String { let mut params = url::form_urlencoded::Serializer::new(String::new()); params.append_pair("base", base); + if let Some(head) = head { + params.append_pair("head", head); + } if include_dirty { params.append_pair("dirty", "1"); } @@ -650,7 +671,7 @@ fn run_comments(dir: &PathBuf, command: CommentsCommand) -> anyhow::Result<()> { let store = comments::Store::new(dir)?; match command.command { CommentSubcommand::List => { - let threads = store.list()?; + let threads = store.list(None)?; if command.json { print_json(&serde_json::json!({ "threads": threads }))?; } else { @@ -666,15 +687,18 @@ fn run_comments(dir: &PathBuf, command: CommentsCommand) -> anyhow::Result<()> { body, author, } => { - let thread = store.add_thread(comments::AddThreadInput { - path, - side, - line, - end_line: end_line.unwrap_or_default(), - end_side, - body: body_from_flag(body)?, - author, - })?; + let thread = store.add_thread( + None, + comments::AddThreadInput { + path, + side, + line, + end_line: end_line.unwrap_or_default(), + end_side, + body: body_from_flag(body)?, + author, + }, + )?; print_thread_result(&thread, command.json)?; } CommentSubcommand::Reply { @@ -683,6 +707,7 @@ fn run_comments(dir: &PathBuf, command: CommentsCommand) -> anyhow::Result<()> { author, } => { let thread = store.add_reply( + None, &thread_id, comments::AddReplyInput { body: body_from_flag(body)?, @@ -692,11 +717,11 @@ fn run_comments(dir: &PathBuf, command: CommentsCommand) -> anyhow::Result<()> { print_thread_result(&thread, command.json)?; } CommentSubcommand::Resolve { thread_id } => { - let thread = store.resolve(&thread_id)?; + let thread = store.resolve(None, &thread_id)?; print_thread_result(&thread, command.json)?; } CommentSubcommand::Reopen { thread_id } => { - let thread = store.reopen(&thread_id)?; + let thread = store.reopen(None, &thread_id)?; print_thread_result(&thread, command.json)?; } } @@ -794,25 +819,37 @@ mod tests { } #[test] - fn branch_target_encodes_base_and_dirty() { + fn branch_target_encodes_base_head_and_dirty() { assert_eq!( - branch_target("origin/main", false), + branch_target("origin/main", None, false), "/branch?base=origin%2Fmain" ); assert_eq!( - branch_target("origin/main", true), + branch_target("origin/main", None, true), "/branch?base=origin%2Fmain&dirty=1" ); + assert_eq!( + branch_target("main", Some("feature/x"), false), + "/branch?base=main&head=feature%2Fx" + ); } #[test] - fn branch_base_from_target_decodes_base() { + fn target_query_param_decodes_values() { assert_eq!( - branch_base_from_target("/branch?base=origin%2Fmain"), + target_query_param("/branch?base=origin%2Fmain", "base"), "origin/main" ); - assert_eq!(branch_base_from_target("/branch?base=main&dirty=1"), "main"); - assert_eq!(branch_base_from_target("/local"), ""); + assert_eq!( + target_query_param("/branch?base=main&dirty=1", "base"), + "main" + ); + assert_eq!( + target_query_param("/branch?base=main&head=feature%2Fx", "head"), + "feature/x" + ); + assert_eq!(target_query_param("/branch?base=main", "head"), ""); + assert_eq!(target_query_param("/local", "base"), ""); } #[test] @@ -832,6 +869,10 @@ mod tests { target_label("/branch?base=origin%2Fmain", dir.path()), "feature/startup -> origin/main" ); + assert_eq!( + target_label("/branch?base=main&head=other", dir.path()), + "other -> main" + ); assert_eq!( target_label("/org/repo/pull/123", dir.path()), "GitHub PR org/repo#123" diff --git a/src/comments.rs b/src/comments.rs index 646cd2a..f3453e9 100644 --- a/src/comments.rs +++ b/src/comments.rs @@ -114,7 +114,12 @@ impl Store { &self.root } - pub fn branch(&self) -> String { + /// The branch threads are scoped to: `scope` when given (reviewing a branch + /// that isn't checked out), otherwise the checked-out branch. + pub fn branch(&self, scope: Option<&str>) -> String { + if let Some(scope) = scope.map(str::trim).filter(|s| !s.is_empty()) { + return scope.to_string(); + } let branch = git::branch(&self.root); if branch.is_empty() { "local".to_string() @@ -123,10 +128,10 @@ impl Store { } } - pub fn list(&self) -> Result> { + pub fn list(&self, scope: Option<&str>) -> Result> { let _guard = self.lock.lock().expect("comment store lock poisoned"); let file = self.load()?; - let branch = self.branch(); + let branch = self.branch(scope); Ok(file .threads .into_iter() @@ -134,7 +139,7 @@ impl Store { .collect()) } - pub fn add_thread(&self, input: AddThreadInput) -> Result { + pub fn add_thread(&self, scope: Option<&str>, input: AddThreadInput) -> Result { let clean = clean_thread_input(input.clone())?; let author = clean_author(&self.root, input.author); let now = Utc::now(); @@ -167,20 +172,25 @@ impl Store { } let _guard = self.lock.lock().expect("comment store lock poisoned"); - thread.branch = self.branch(); + thread.branch = self.branch(scope); let mut file = self.load()?; file.threads.push(thread.clone()); self.save(file)?; Ok(thread) } - pub fn add_reply(&self, thread_id: &str, input: AddReplyInput) -> Result { + pub fn add_reply( + &self, + scope: Option<&str>, + thread_id: &str, + input: AddReplyInput, + ) -> Result { let body = input.body.trim().to_string(); if body.is_empty() { return validation("body is required"); } let author = clean_author(&self.root, input.author); - self.update_thread(thread_id, |thread, now| { + self.update_thread(scope, thread_id, |thread, now| { thread.comments.push(Comment { id: new_id("cmt"), author, @@ -191,22 +201,22 @@ impl Store { }) } - pub fn resolve(&self, thread_id: &str) -> Result { - self.set_status(thread_id, "resolved") + pub fn resolve(&self, scope: Option<&str>, thread_id: &str) -> Result { + self.set_status(scope, thread_id, "resolved") } - pub fn reopen(&self, thread_id: &str) -> Result { - self.set_status(thread_id, "open") + pub fn reopen(&self, scope: Option<&str>, thread_id: &str) -> Result { + self.set_status(scope, thread_id, "open") } - pub fn delete(&self, thread_id: &str) -> Result<()> { + pub fn delete(&self, scope: Option<&str>, thread_id: &str) -> Result<()> { let thread_id = thread_id.trim(); if thread_id.is_empty() { return validation("thread id is required"); } let _guard = self.lock.lock().expect("comment store lock poisoned"); let mut file = self.load()?; - let branch = self.branch(); + let branch = self.branch(scope); let original_len = file.threads.len(); file.threads .retain(|thread| thread.id != thread_id || thread.branch != branch); @@ -216,8 +226,8 @@ impl Store { self.save(file) } - fn set_status(&self, thread_id: &str, status: &str) -> Result { - self.update_thread(thread_id, |thread, now| { + fn set_status(&self, scope: Option<&str>, thread_id: &str, status: &str) -> Result { + self.update_thread(scope, thread_id, |thread, now| { thread.status = status.to_string(); thread.updated_at = now; }) @@ -225,6 +235,7 @@ impl Store { fn update_thread( &self, + scope: Option<&str>, thread_id: &str, update: impl FnOnce(&mut Thread, DateTime), ) -> Result { @@ -234,7 +245,7 @@ impl Store { } let _guard = self.lock.lock().expect("comment store lock poisoned"); let mut file = self.load()?; - let branch = self.branch(); + let branch = self.branch(scope); for thread in &mut file.threads { if thread.id != thread_id || thread.branch != branch { continue; @@ -526,14 +537,17 @@ mod tests { let store = Store::new(dir.path()).unwrap(); let thread = store - .add_thread(AddThreadInput { - path: "web/src/App.tsx".into(), - line: 42, - end_line: 45, - side: "additions".into(), - body: "Check this".into(), - ..Default::default() - }) + .add_thread( + None, + AddThreadInput { + path: "web/src/App.tsx".into(), + line: 42, + end_line: 45, + side: "additions".into(), + body: "Check this".into(), + ..Default::default() + }, + ) .unwrap(); assert!(!thread.id.is_empty()); assert_eq!(thread.provider, "local"); @@ -550,6 +564,7 @@ mod tests { let thread = store .add_reply( + None, &thread.id, AddReplyInput { body: "Reply".into(), @@ -561,11 +576,11 @@ mod tests { assert_eq!(thread.comments[1].body, "Reply"); assert_eq!(thread.comments[1].author, "agent"); - assert_eq!(store.resolve(&thread.id).unwrap().status, "resolved"); - assert_eq!(store.reopen(&thread.id).unwrap().status, "open"); + assert_eq!(store.resolve(None, &thread.id).unwrap().status, "resolved"); + assert_eq!(store.reopen(None, &thread.id).unwrap().status, "open"); - store.delete(&thread.id).unwrap(); - assert!(store.list().unwrap().is_empty()); + store.delete(None, &thread.id).unwrap(); + assert!(store.list(None).unwrap().is_empty()); } #[test] @@ -573,25 +588,31 @@ mod tests { let dir = new_repo(); let store = Store::new(dir.path()).unwrap(); store - .add_thread(AddThreadInput { - path: "a.go".into(), - line: 1, - body: "main".into(), - ..Default::default() - }) + .add_thread( + None, + AddThreadInput { + path: "a.go".into(), + line: 1, + body: "main".into(), + ..Default::default() + }, + ) .unwrap(); run_git(dir.path(), &["checkout", "-b", "feature/comments"]); store - .add_thread(AddThreadInput { - path: "b.go".into(), - line: 1, - body: "feature".into(), - ..Default::default() - }) + .add_thread( + None, + AddThreadInput { + path: "b.go".into(), + line: 1, + body: "feature".into(), + ..Default::default() + }, + ) .unwrap(); - let threads = store.list().unwrap(); + let threads = store.list(None).unwrap(); assert_eq!(threads.len(), 1); assert_eq!(threads[0].path, "b.go"); } @@ -601,12 +622,15 @@ mod tests { let dir = new_repo(); let store = Store::new(dir.path()).unwrap(); let thread = store - .add_thread(AddThreadInput { - path: "a.go".into(), - line: 1, - body: "main".into(), - ..Default::default() - }) + .add_thread( + None, + AddThreadInput { + path: "a.go".into(), + line: 1, + body: "main".into(), + ..Default::default() + }, + ) .unwrap(); assert_eq!(thread.branch, "main"); @@ -614,11 +638,11 @@ mod tests { // must not touch a different branch's threads. run_git(dir.path(), &["checkout", "-b", "feature/comments"]); assert!(matches!( - store.delete(&thread.id).unwrap_err(), + store.delete(None, &thread.id).unwrap_err(), CommentError::NotFound )); assert!(matches!( - store.resolve(&thread.id).unwrap_err(), + store.resolve(None, &thread.id).unwrap_err(), CommentError::NotFound )); @@ -626,7 +650,7 @@ mod tests { // `main` is unborn here (no commit), so re-point HEAD via symbolic-ref; // `checkout` would fail on a ref that never existed. run_git(dir.path(), &["symbolic-ref", "HEAD", "refs/heads/main"]); - let listed = store.list().unwrap(); + let listed = store.list(None).unwrap(); assert_eq!(listed.len(), 1); assert_eq!(listed[0].id, thread.id); @@ -642,19 +666,53 @@ mod tests { let dir = new_repo(); let store = Store::new(dir.path()).unwrap(); let thread = store - .add_thread(AddThreadInput { - path: "a.go".into(), - line: 1, - body: "main".into(), - ..Default::default() - }) + .add_thread( + None, + AddThreadInput { + path: "a.go".into(), + line: 1, + body: "main".into(), + ..Default::default() + }, + ) .unwrap(); run_git(dir.path(), &["checkout", "-b", "feature/comments"]); - let err = store.resolve(&thread.id).unwrap_err(); + let err = store.resolve(None, &thread.id).unwrap_err(); assert!(matches!(err, CommentError::NotFound)); } + #[test] + fn explicit_scope_overrides_checked_out_branch() { + let dir = new_repo(); + let store = Store::new(dir.path()).unwrap(); + // Checked out on main, reviewing feature/x: the thread belongs to it. + let thread = store + .add_thread( + Some("feature/x"), + AddThreadInput { + path: "a.go".into(), + line: 1, + body: "on feature".into(), + ..Default::default() + }, + ) + .unwrap(); + assert_eq!(thread.branch, "feature/x"); + assert!(store.list(None).unwrap().is_empty()); + assert_eq!(store.list(Some("feature/x")).unwrap().len(), 1); + assert!(matches!( + store.resolve(None, &thread.id).unwrap_err(), + CommentError::NotFound + )); + assert_eq!( + store.resolve(Some("feature/x"), &thread.id).unwrap().status, + "resolved" + ); + store.delete(Some("feature/x"), &thread.id).unwrap(); + assert!(store.list(Some("feature/x")).unwrap().is_empty()); + } + #[test] fn store_keeps_concurrent_adds() { let dir = new_repo(); @@ -665,17 +723,20 @@ mod tests { let store = store.clone(); scope.spawn(move || { store - .add_thread(AddThreadInput { - path: format!("file-{i:02}.go"), - line: 1, - body: "body".into(), - ..Default::default() - }) + .add_thread( + None, + AddThreadInput { + path: format!("file-{i:02}.go"), + line: 1, + body: "body".into(), + ..Default::default() + }, + ) .unwrap(); }); } }); - assert_eq!(store.list().unwrap().len(), COUNT); + assert_eq!(store.list(None).unwrap().len(), COUNT); } #[test] @@ -683,13 +744,16 @@ mod tests { let dir = new_repo(); let store = Store::new(dir.path()).unwrap(); let thread = store - .add_thread(AddThreadInput { - path: "a.go".into(), - line: 1, - body: "b".into(), - author: " carol ".into(), - ..Default::default() - }) + .add_thread( + None, + AddThreadInput { + path: "a.go".into(), + line: 1, + body: "b".into(), + author: " carol ".into(), + ..Default::default() + }, + ) .unwrap(); assert_eq!(thread.comments[0].author, "carol"); } @@ -699,17 +763,20 @@ mod tests { let dir = new_repo(); let store = Store::new(dir.path()).unwrap(); let created = store - .add_thread(AddThreadInput { - path: "a.go".into(), - line: 1, - body: "b".into(), - ..Default::default() - }) + .add_thread( + None, + AddThreadInput { + path: "a.go".into(), + line: 1, + body: "b".into(), + ..Default::default() + }, + ) .unwrap(); // Re-open the store and reload from disk: timestamps must survive the // JSON (RFC3339) serialize/parse cycle exactly. - let reloaded = Store::new(dir.path()).unwrap().list().unwrap(); + let reloaded = Store::new(dir.path()).unwrap().list(None).unwrap(); assert_eq!(reloaded.len(), 1); assert_eq!(reloaded[0].created_at, created.created_at); assert_eq!(reloaded[0].updated_at, created.updated_at); diff --git a/src/git.rs b/src/git.rs index 25e8215..f130807 100644 --- a/src/git.rs +++ b/src/git.rs @@ -25,6 +25,8 @@ pub enum GitError { BlobNotFound, #[error("file not found")] FileNotFound, + #[error("uncommitted changes can only be included for the checked-out branch")] + DirtyWithHead, } pub type Result = std::result::Result; @@ -210,9 +212,23 @@ pub fn local_diff(cwd: impl AsRef) -> Result { } } -pub fn branch_diff(cwd: impl AsRef, base: &str, include_dirty: bool) -> Result { +/// Diffs `head` (the checked-out HEAD when `None`) against its merge-base with +/// `base`, like `git diff base...head`. `include_dirty` diffs the merge-base +/// against the working tree instead, so it requires `head` to be `None`. +pub fn branch_diff( + cwd: impl AsRef, + base: &str, + head: Option<&str>, + include_dirty: bool, +) -> Result { + if include_dirty && head.is_some() { + return Err(GitError::DirtyWithHead); + } let repo = discover(cwd)?; - let head = repo.head()?.peel_to_commit()?; + let head = match head { + Some(head) => repo.revparse_single(head)?.peel_to_commit()?, + None => repo.head()?.peel_to_commit()?, + }; let base_commit = repo.revparse_single(base)?.peel_to_commit()?; let merge_base = repo.merge_base(base_commit.id(), head.id())?; if include_dirty { @@ -407,6 +423,24 @@ pub fn read_blob(cwd: impl AsRef, oid: &str) -> Result> { Ok(blob.content().to_vec()) } +/// Reads `rel_path` as committed at `rev` (e.g. a branch that isn't checked +/// out), so callers don't depend on the working tree. +pub fn read_rev_file(cwd: impl AsRef, rev: &str, rel_path: &str) -> Result> { + if !is_safe_repo_path(rel_path) { + return Err(GitError::InvalidRepoPath); + } + let repo = discover(cwd)?; + let tree = repo.revparse_single(rev)?.peel_to_commit()?.tree()?; + let entry = tree + .get_path(Path::new(rel_path)) + .map_err(|_| GitError::FileNotFound)?; + let blob = entry + .to_object(&repo)? + .peel_to_blob() + .map_err(|_| GitError::FileNotFound)?; + Ok(blob.content().to_vec()) +} + /// Whether `path` is a safe repository-relative path for a worktree file /// read: non-empty, relative (no leading `/`), no backslashes, and no empty, /// `.`, or `..` segments. Does not check the filesystem; callers must still @@ -512,6 +546,39 @@ mod tests { ); } + #[test] + fn read_rev_file_reads_committed_content_not_worktree() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + git(root, &["init", "-b", "main"]); + git(root, &["config", "user.email", "diffs@example.com"]); + git(root, &["config", "user.name", "Diffs Test"]); + fs::write(root.join("a.txt"), "main\n").unwrap(); + git(root, &["add", "."]); + git(root, &["commit", "-m", "initial"]); + git(root, &["checkout", "-q", "-b", "feature"]); + fs::create_dir(root.join("dir")).unwrap(); + fs::write(root.join("dir/b.txt"), "feature\n").unwrap(); + git(root, &["add", "."]); + git(root, &["commit", "-m", "feature"]); + git(root, &["checkout", "-q", "main"]); + fs::write(root.join("a.txt"), "dirty\n").unwrap(); + + assert_eq!( + read_rev_file(root, "feature", "dir/b.txt").unwrap(), + b"feature\n" + ); + assert_eq!(read_rev_file(root, "feature", "a.txt").unwrap(), b"main\n"); + assert!(matches!( + read_rev_file(root, "main", "dir/b.txt"), + Err(GitError::FileNotFound) + )); + assert!(matches!( + read_rev_file(root, "feature", "../x"), + Err(GitError::InvalidRepoPath) + )); + } + #[test] fn changed_files_labels_deletions_and_renames() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/server.rs b/src/server.rs index fe50ea2..9c149d9 100644 --- a/src/server.rs +++ b/src/server.rs @@ -96,6 +96,7 @@ struct BranchesResponse { #[derive(Debug, Deserialize)] struct BranchDiffQuery { base: Option, + head: Option, dirty: Option, } @@ -104,12 +105,16 @@ struct CommentTargetQuery { org: Option, repo: Option, number: Option, + /// Local comments only: scope to this branch instead of the checked-out one. + branch: Option, } #[derive(Debug, Deserialize)] struct BlobQuery { oid: Option, path: Option, + /// With `path`: read the file as committed at this ref. + rev: Option, worktree: Option, } @@ -294,8 +299,23 @@ async fn handle_branch_diff( format!("invalid base ref: {base:?}"), ); } - match git::branch_diff(&state.cwd, &base, dirty_enabled(query.dirty.as_deref())) { + let head = query + .head + .as_deref() + .map(str::trim) + .filter(|v| !v.is_empty()); + if let Some(head) = head + && !is_safe_ref_arg(head) + { + return error( + StatusCode::BAD_REQUEST, + format!("invalid head ref: {head:?}"), + ); + } + let include_dirty = dirty_enabled(query.dirty.as_deref()); + match git::branch_diff(&state.cwd, &base, head, include_dirty) { Ok(patch) => text(patch), + Err(err @ git::GitError::DirtyWithHead) => error(StatusCode::BAD_REQUEST, err), Err(err) => error(StatusCode::BAD_GATEWAY, err), } } @@ -367,11 +387,11 @@ async fn handle_list_comments( Err(err) => error(StatusCode::BAD_GATEWAY, err), } } - CommentScope::Local => { + CommentScope::Local(scope) => { let Some(store) = state.comments else { return comments_unavailable(); }; - match store.list() { + match store.list(scope.as_deref()) { Ok(threads) => { (StatusCode::OK, Json(json!({ "threads": threads }))).into_response() } @@ -402,11 +422,11 @@ async fn handle_add_comment( Err(err) => error(StatusCode::BAD_GATEWAY, err), } } - CommentScope::Local => { + CommentScope::Local(scope) => { let Some(store) = state.comments else { return comments_unavailable(); }; - match store.add_thread(input) { + match store.add_thread(scope.as_deref(), input) { Ok(thread) => (StatusCode::CREATED, Json(thread)).into_response(), Err(err) => comment_error(err), } @@ -425,11 +445,11 @@ async fn handle_delete_comment( StatusCode::BAD_REQUEST, "deleting GitHub comments is not supported", ), - CommentScope::Local => { + CommentScope::Local(scope) => { let Some(store) = state.comments else { return comments_unavailable(); }; - match store.delete(&thread_id) { + match store.delete(scope.as_deref(), &thread_id) { Ok(()) => StatusCode::NO_CONTENT.into_response(), Err(err) => comment_error(err), } @@ -456,9 +476,9 @@ async fn handle_reply_comment( ) .await, ), - CommentScope::Local => { - write_thread_or_error(state.comments, |store| store.add_reply(&thread_id, input)) - } + CommentScope::Local(scope) => write_thread_or_error(state.comments, |store| { + store.add_reply(scope.as_deref(), &thread_id, input) + }), } } @@ -497,11 +517,11 @@ async fn set_resolved( ) .await, ), - CommentScope::Local => write_thread_or_error(state.comments, |store| { + CommentScope::Local(scope) => write_thread_or_error(state.comments, |store| { if resolved { - store.resolve(&thread_id) + store.resolve(scope.as_deref(), &thread_id) } else { - store.reopen(&thread_id) + store.reopen(scope.as_deref(), &thread_id) } }), } @@ -551,12 +571,18 @@ async fn handle_blob(State(state): State, Query(query): Query blob_response(bytes), @@ -733,20 +759,26 @@ struct PullTarget { } enum CommentScope { - Local, + /// Local comments, scoped to the given branch or else the checked-out one. + Local(Option), Pull(PullTarget), Invalid, } -/// Empty org/repo/number means local comments; otherwise the trio must pass the -/// same validators as the PR routes, or the request is rejected with 400 -/// (`Invalid`). +/// Empty org/repo/number means local comments (optionally scoped by a `branch` +/// that must be a safe ref); otherwise the trio must pass the same validators +/// as the PR routes, or the request is rejected with 400 (`Invalid`). fn comment_scope(target: &CommentTargetQuery) -> CommentScope { let org = target.org.as_deref().unwrap_or_default(); let repo = target.repo.as_deref().unwrap_or_default(); let number = target.number.as_deref().unwrap_or_default(); if org.is_empty() && repo.is_empty() && number.is_empty() { - return CommentScope::Local; + let branch = target.branch.as_deref().map(str::trim).unwrap_or_default(); + return match branch { + "" => CommentScope::Local(None), + b if is_safe_ref_arg(b) => CommentScope::Local(Some(b.to_string())), + _ => CommentScope::Invalid, + }; } if safe_path_part(org) && safe_path_part(repo) && pull_number(number) { CommentScope::Pull(PullTarget { @@ -1152,13 +1184,32 @@ mod tests { org: None, repo: None, number: None, + branch: None, + }; + assert!(matches!(comment_scope(&local), CommentScope::Local(None))); + + let scoped = CommentTargetQuery { + branch: Some("feature/x".into()), + ..local + }; + match comment_scope(&scoped) { + CommentScope::Local(Some(branch)) => assert_eq!(branch, "feature/x"), + _ => panic!("expected branch-scoped local scope"), + } + + let bad_branch = CommentTargetQuery { + org: None, + repo: None, + number: None, + branch: Some("a..b".into()), }; - assert!(matches!(comment_scope(&local), CommentScope::Local)); + assert!(matches!(comment_scope(&bad_branch), CommentScope::Invalid)); let pull = CommentTargetQuery { org: Some("org".into()), repo: Some("repo".into()), number: Some("123".into()), + branch: None, }; match comment_scope(&pull) { CommentScope::Pull(t) => assert_eq!( @@ -1172,6 +1223,7 @@ mod tests { org: Some("../bad".into()), repo: Some("repo".into()), number: Some("123".into()), + branch: None, }; assert!(matches!(comment_scope(&invalid), CommentScope::Invalid)); } diff --git a/tests/diff_parity.rs b/tests/diff_parity.rs index bd5b13c..7a051a9 100644 --- a/tests/diff_parity.rs +++ b/tests/diff_parity.rs @@ -113,7 +113,7 @@ fn git_local_patch_no_head(dir: &Path) -> String { } /// Reconstructs `branchDiff` (three-dot) reference. -fn git_branch_patch(dir: &Path, base: &str) -> String { +fn git_branch_patch(dir: &Path, base: &str, head: &str) -> String { git_output( dir, &[ @@ -121,7 +121,7 @@ fn git_branch_patch(dir: &Path, base: &str) -> String { "--no-ext-diff", "--patch", "--submodule=diff", - &format!("{base}...HEAD"), + &format!("{base}...{head}"), "--", ], ) @@ -252,9 +252,24 @@ fn branch_diff_three_dot_matches_git() { git(dir, &["commit", "-am", "main moves on"]); git(dir, &["checkout", "-q", "feature"]); - let rust_patch = diffs::git::branch_diff(dir, "main", false).unwrap(); - let git_patch = git_branch_patch(dir, "main"); + let rust_patch = diffs::git::branch_diff(dir, "main", None, false).unwrap(); + let git_patch = git_branch_patch(dir, "main", "HEAD"); assert_per_file_parity(&rust_patch, &git_patch); + + // An explicit head reviews that branch even when neither side is checked + // out, and ignores the (dirty) working tree of whatever is. + git(dir, &["checkout", "-q", "-b", "elsewhere", "main"]); + fs::write(dir.join("base.txt"), "dirty on elsewhere\n").unwrap(); + let rust_patch = diffs::git::branch_diff(dir, "main", Some("feature"), false).unwrap(); + let git_patch = git_branch_patch(dir, "main", "feature"); + assert_per_file_parity(&rust_patch, &git_patch); + assert!(rust_patch.contains("feature.txt"), "{rust_patch}"); + assert!(!rust_patch.contains("dirty on elsewhere"), "{rust_patch}"); + + assert!(matches!( + diffs::git::branch_diff(dir, "main", Some("feature"), true), + Err(diffs::git::GitError::DirtyWithHead) + )); } #[test] @@ -277,7 +292,7 @@ fn branch_diff_include_dirty_matches_git() { fs::write(dir.join("base.txt"), "base dirty edit\n").unwrap(); fs::write(dir.join("untracked.txt"), "loose\n").unwrap(); - let rust_patch = diffs::git::branch_diff(dir, "main", true).unwrap(); + let rust_patch = diffs::git::branch_diff(dir, "main", None, true).unwrap(); let git_patch = git_branch_dirty_patch(dir, "main"); assert_per_file_parity(&rust_patch, &git_patch); } diff --git a/web/src/components/DiffView.tsx b/web/src/components/DiffView.tsx index 8cbbe59..7bba27d 100644 --- a/web/src/components/DiffView.tsx +++ b/web/src/components/DiffView.tsx @@ -288,6 +288,8 @@ const fetchBlob = (oid: string, name: string) => fetchFile(`/api/blob?oid=${encodeURIComponent(oid)}`, name, `blob:${oid}`); const fetchWorktree = (path: string) => fetchFile(`/api/blob?${new URLSearchParams({ path, worktree: "1" })}`, path); +const fetchRevFile = (rev: string, path: string) => + fetchFile(`/api/blob?${new URLSearchParams({ path, rev })}`, path); export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" } = {}) { const { org, repo, number } = useParams<{ @@ -297,6 +299,8 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" }>(); const [searchParams] = useSearchParams(); const baseRef = source === "branch" ? (searchParams.get("base") ?? "") : ""; + // Explicit branch to review; empty means the checked-out branch. + const headRef = source === "branch" ? (searchParams.get("head") ?? "") : ""; const includeDirty = source === "branch" && searchParams.get("dirty") === "1"; const [appColorScheme, setAppColorScheme] = useState(() => initialColorScheme()); @@ -355,7 +359,7 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" const sessionKey = isLocal ? "local" : isBranch - ? `branch:${baseRef}` + ? `branch:${baseRef}${headRef ? `...${headRef}` : ""}` : `pr:${org}/${repo}/${number}`; const scrollStorageKey = `diffs-scroll:${sessionKey}`; const collapsedStorageKey = `diffs-collapsed:${sessionKey}`; @@ -364,16 +368,19 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" const prUrl = org && repo && number ? `https://${config.githubHost}/${org}/${repo}/pull/${number}` : ""; const prQuery = hasPr ? new URLSearchParams({ org, repo, number }).toString() : ""; + // Local threads are scoped to the reviewed branch, not the checked-out one. + const localCommentQuery = headRef ? `?branch=${encodeURIComponent(headRef)}` : ""; const commentsEndpoint = usesLocalStore - ? "/api/comments" + ? `/api/comments${localCommentQuery}` : hasPr ? `/api/comments?${prQuery}` : null; const pullRequestInfoEndpoint = hasPr ? `/api/pull/${[org, repo, number].map(encodeURIComponent).join("/")}` : null; + const headLabel = headRef || config.gitBranch.trim(); const baseTitle = isBranch - ? `${baseRef || "base"} ← ${config.gitBranch.trim() || "HEAD"}` + ? `${baseRef || "base"} ← ${headLabel || "HEAD"}` : isLocal ? localRepoTitle(config.cwd, config.gitBranch) : org && repo && number @@ -436,7 +443,11 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" return; } const endpoint = isBranch - ? `/api/branch-diff?base=${encodeURIComponent(baseRef)}${includeDirty ? "&dirty=1" : ""}` + ? `/api/branch-diff?${new URLSearchParams({ + base: baseRef, + ...(headRef ? { head: headRef } : {}), + ...(includeDirty ? { dirty: "1" } : {}), + })}` : isLocal ? "/api/local-diff" : `/api/patch/${org}/${repo}/${number}`; @@ -478,7 +489,7 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" eventSource?.close(); if (fallbackInterval != null) window.clearInterval(fallbackInterval); }; - }, [isLocal, isBranch, usesLocalStore, baseRef, includeDirty, org, repo, number]); + }, [isLocal, isBranch, usesLocalStore, baseRef, headRef, includeDirty, org, repo, number]); useEffect(() => { if (pullRequestInfoEndpoint == null) return; @@ -973,7 +984,9 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" return; } if (!usesLocalStore) return; - apiFetch(`/api/comments/${encodeURIComponent(thread.id)}`, { method: "DELETE" }) + apiFetch(`/api/comments/${encodeURIComponent(thread.id)}${localCommentQuery}`, { + method: "DELETE", + }) .then(() => { setCommentThreads((prev) => prev.filter((current) => current.id !== thread.id)); }) @@ -981,7 +994,7 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" console.error("Failed to delete comment:", err); }); }, - [usesLocalStore], + [usesLocalStore, localCommentQuery], ); useEffect(() => { @@ -1045,9 +1058,13 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" return { oldFile, newFile }; } // Pure renames carry no `index` line (and thus no object ids); their - // content is unchanged, so read the new path from the worktree. + // content is unchanged, so read the new path from the worktree, or from + // the reviewed branch when it isn't the checked-out one. if (fileDiff.type === "rename-pure") - return { oldFile: null, newFile: await fetchWorktree(newName) }; + return { + oldFile: null, + newFile: await (headRef ? fetchRevFile(headRef, newName) : fetchWorktree(newName)), + }; const { prevObjectId, newObjectId } = fileDiff; if (isZeroOid(prevObjectId)) throw new Error(`missing prevObjectId for ${fileDiff.name}`); const [oldFile, newFile] = await Promise.all([ @@ -1059,7 +1076,7 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" ]); return { oldFile, newFile }; }, - [usesLocalStore, isLocal, hasPr, pullRequestInfoEndpoint], + [usesLocalStore, isLocal, hasPr, pullRequestInfoEndpoint, headRef], ); // Only local diffs edit the worktree. Pure renames have no rows to edit. @@ -1185,7 +1202,7 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" if (exporting || visibleFiles.length === 0) return; setExporting(true); try { - const subtitle = isLocal ? config.cwd : isBranch ? config.gitBranch.trim() : prUrl; + const subtitle = isLocal ? config.cwd : isBranch ? headLabel : prUrl; await exportDiffToHtml({ files: visibleFiles, options: codeViewOptions, @@ -1208,7 +1225,7 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" isLocal, isBranch, config.cwd, - config.gitBranch, + headLabel, config.codeFontFamily, config.uiFontFamily, prUrl, @@ -1418,6 +1435,7 @@ export function DiffView({ source = "pr" }: { source?: "pr" | "local" | "branch" config={config} isLocal={usesLocalStore} baseRef={isBranch ? baseRef : undefined} + headRef={isBranch ? headRef : undefined} includeDirty={isBranch ? includeDirty : false} onSidebarToggle={openSidebar} onSubmitPendingComments={submitPendingComments} diff --git a/web/src/components/Home.tsx b/web/src/components/Home.tsx index 6c99219..3165a90 100644 --- a/web/src/components/Home.tsx +++ b/web/src/components/Home.tsx @@ -81,7 +81,7 @@ export function NotFound() { codeLabel="Available routes" code={`/ /local -/branch?base= +/branch?base=[&head=] /:org/:repo/pull/:number`} >

diff --git a/web/src/components/diff-view/DiffToolbar.tsx b/web/src/components/diff-view/DiffToolbar.tsx index 1774172..1eed1eb 100644 --- a/web/src/components/diff-view/DiffToolbar.tsx +++ b/web/src/components/diff-view/DiffToolbar.tsx @@ -73,14 +73,23 @@ function BranchChip({ label, title }: { label: string; title: string }) { ); } -function branchDiffHref(base: string, includeDirty: boolean) { +function branchDiffHref(base: string, head: string, includeDirty: boolean) { const params = new URLSearchParams(); params.set("base", base); + if (head) params.set("head", head); if (includeDirty) params.set("dirty", "1"); return `/branch?${params.toString()}`; } -function BaseBranchSwitcher({ baseRef, includeDirty }: { baseRef: string; includeDirty: boolean }) { +function BaseBranchSwitcher({ + baseRef, + headRef, + includeDirty, +}: { + baseRef: string; + headRef: string; + includeDirty: boolean; +}) { const [branches, setBranches] = useState(null); const [loading, setLoading] = useState(false); @@ -129,7 +138,7 @@ function BaseBranchSwitcher({ baseRef, includeDirty }: { baseRef: string; includ return ( } + render={} className={selected ? "font-medium" : undefined} aria-current={selected ? "true" : undefined} > @@ -215,6 +224,7 @@ export function DiffToolbar({ config, isLocal, baseRef, + headRef = "", includeDirty = false, onSidebarToggle, onSubmitPendingComments, @@ -234,6 +244,7 @@ export function DiffToolbar({ config: AppConfig; isLocal: boolean; baseRef?: string; + headRef?: string; includeDirty?: boolean; onSidebarToggle: () => void; onSubmitPendingComments: () => void; @@ -250,6 +261,7 @@ export function DiffToolbar({ submittingPendingComments: boolean; }) { const remoteTitle = pullRequestTitle(prUrl); + const branchLabel = headRef || config.gitBranch.trim(); const baseBranch = pullRequestInfo?.baseBranch ?? ""; const headBranch = pullRequestInfo?.headBranch ?? ""; const createdAt = pullRequestInfo ? formatPullRequestDate(pullRequestInfo.createdAt) : ""; @@ -273,18 +285,19 @@ export function DiffToolbar({ {baseRef && baseRef.trim() !== "" && ( <> - + )} - {config.gitBranch.trim() !== "" && ( - + {branchLabel !== "" && ( + )} ) : (