From e8e7bf8fc37e8ee1aecc3b7cb907a987f0880715 Mon Sep 17 00:00:00 2001 From: MonteYin Date: Thu, 26 Mar 2026 09:28:33 +0800 Subject: [PATCH] GTST-19 refactor: improve coding style with pedantic lints, shared helpers, and dedup - Add rustfmt.toml, clippy.toml, rust-toolchain.toml for consistent tooling - Enable clippy::pedantic in Cargo.toml [lints] and fix all warnings - Extract shared git2 helpers into git/mod.rs (diff_opts, delta_path, hunk_header, is_binary_delta) - Add LineTag::from_origin() and PROTOCOL_VERSION constant to models.rs - Extract dispatch() helper in protocol.rs to reduce match arm duplication - Deduplicate test helpers into tests/common/mod.rs - Bump version to 0.1.2 --- .github/workflows/ci.yml | 2 +- .gitignore | 3 + Cargo.toml | 19 ++- clippy.toml | 3 + rust-toolchain.toml | 3 + rustfmt.toml | 3 + src/git/diff.rs | 84 ++++--------- src/git/mod.rs | 45 +++++++ src/git/stage.rs | 260 +++++++++++---------------------------- src/git/status.rs | 22 +--- src/main.rs | 5 +- src/models.rs | 97 +++++---------- src/output.rs | 34 ++--- src/protocol.rs | 48 ++++---- tests/cli.rs | 143 ++++++--------------- tests/common/mod.rs | 37 ++++++ tests/edge_cases.rs | 120 +++++------------- tests/protocol.rs | 58 ++------- 18 files changed, 348 insertions(+), 638 deletions(-) create mode 100644 clippy.toml create mode 100644 rust-toolchain.toml create mode 100644 rustfmt.toml create mode 100644 tests/common/mod.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fc55928..9d8a94d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,7 +20,7 @@ jobs: components: clippy - uses: Swatinem/rust-cache@v2 - run: cargo check --all-targets - - run: cargo clippy --all-targets -- -D warnings + - run: cargo clippy --all-targets test: name: Test diff --git a/.gitignore b/.gitignore index e666f00..c6a5096 100644 --- a/.gitignore +++ b/.gitignore @@ -21,6 +21,9 @@ CLAUDE.md # Test test-folder/ +# Local docs +gotchas.md + # Local config .env *.local diff --git a/Cargo.toml b/Cargo.toml index af3ab24..f48e9b1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "gitsift" -version = "0.1.1" +version = "0.1.2" edition = "2024" description = "Git hunk sifter for code agents" license = "MIT" @@ -21,3 +21,20 @@ similar = { version = "2", features = ["text"] } assert_cmd = "2" predicates = "3" tempfile = "3" + +[lints.rust] +unsafe_code = "forbid" + +[lints.clippy] +pedantic = { level = "warn", priority = -1 } +# Re-allow noisy pedantic lints +module_name_repetitions = "allow" +missing_errors_doc = "allow" +must_use_candidate = "allow" +missing_panics_doc = "allow" +similar_names = "allow" +unnecessary_wraps = "allow" +# Enforce specific lints +cloned_instead_of_copied = "deny" +redundant_closure_for_method_calls = "deny" +unnested_or_patterns = "deny" diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 0000000..7b2c5e0 --- /dev/null +++ b/clippy.toml @@ -0,0 +1,3 @@ +too-many-arguments-threshold = 7 +too-many-lines-threshold = 250 +cognitive-complexity-threshold = 30 diff --git a/rust-toolchain.toml b/rust-toolchain.toml new file mode 100644 index 0000000..73cb934 --- /dev/null +++ b/rust-toolchain.toml @@ -0,0 +1,3 @@ +[toolchain] +channel = "stable" +components = ["rustfmt", "clippy"] diff --git a/rustfmt.toml b/rustfmt.toml new file mode 100644 index 0000000..38cba21 --- /dev/null +++ b/rustfmt.toml @@ -0,0 +1,3 @@ +edition = "2024" +max_width = 100 +use_small_heuristics = "Max" diff --git a/src/git/diff.rs b/src/git/diff.rs index 23af06a..bed5cf2 100644 --- a/src/git/diff.rs +++ b/src/git/diff.rs @@ -1,10 +1,11 @@ use anyhow::{Context, Result}; -use git2::{Delta, DiffOptions, Repository}; +use git2::{Delta, Repository}; use std::cell::RefCell; use std::collections::hash_map::DefaultHasher; use std::hash::{Hash, Hasher}; use std::path::Path; +use super::{delta_path, hunk_header, is_binary_delta}; use crate::models::{DiffOutput, FileChange, FileStatus, Hunk, HunkLine, LineTag}; /// Generate a stable hunk ID from file path, old start line, and header. @@ -20,7 +21,7 @@ pub fn hunk_id(file_path: &str, old_start: u32, header: &str) -> String { format!("{:016x}", hasher.finish()) } -/// Map git2 Delta to our FileStatus. +/// Map git2 Delta to our `FileStatus`. fn delta_to_status(delta: Delta) -> FileStatus { match delta { Delta::Added | Delta::Untracked => FileStatus::Added, @@ -30,7 +31,7 @@ fn delta_to_status(delta: Delta) -> FileStatus { } } -/// Mutable state shared across git2 foreach callbacks via RefCell. +/// Mutable state shared across git2 foreach callbacks via `RefCell`. struct DiffState { files: Vec, current_file: Option, @@ -39,11 +40,7 @@ struct DiffState { impl DiffState { fn new() -> Self { - Self { - files: Vec::new(), - current_file: None, - current_hunk: None, - } + Self { files: Vec::new(), current_file: None, current_hunk: None } } /// Flush current hunk into current file, then flush current file into files list. @@ -76,18 +73,13 @@ impl DiffState { pub fn diff_unstaged(repo_path: &Path, file_filter: Option<&str>) -> Result { let repo = Repository::open(repo_path).context("failed to open git repository")?; - let mut opts = DiffOptions::new(); - opts.context_lines(3); - opts.include_untracked(true); - opts.show_untracked_content(true); - + let mut opts = super::diff_opts_with_untracked(); if let Some(filter) = file_filter { opts.pathspec(filter); } - let diff = repo - .diff_index_to_workdir(None, Some(&mut opts)) - .context("failed to generate diff")?; + let diff = + repo.diff_index_to_workdir(None, Some(&mut opts)).context("failed to generate diff")?; let state = RefCell::new(DiffState::new()); @@ -96,18 +88,13 @@ pub fn diff_unstaged(repo_path: &Path, file_filter: Option<&str>) -> Result) -> Result) -> Result' => LineTag::Insert, - '-' | '<' => LineTag::Delete, - _ => LineTag::Equal, - }; + let tag = LineTag::from_origin(line.origin()); let content = String::from_utf8_lossy(line.content()).into_owned(); @@ -193,8 +172,7 @@ mod tests { index.write().unwrap(); let tree_oid = index.write_tree().unwrap(); let tree = repo.find_tree(tree_oid).unwrap(); - repo.commit(Some("HEAD"), &sig, &sig, "initial", &tree, &[]) - .unwrap(); + repo.commit(Some("HEAD"), &sig, &sig, "initial", &tree, &[]).unwrap(); } (dir, repo) @@ -208,19 +186,15 @@ mod tests { let tree_oid = index.write_tree().unwrap(); let tree = repo.find_tree(tree_oid).unwrap(); let head = repo.head().unwrap().peel_to_commit().unwrap(); - repo.commit(Some("HEAD"), &sig, &sig, msg, &tree, &[&head]) - .unwrap(); + repo.commit(Some("HEAD"), &sig, &sig, msg, &tree, &[&head]).unwrap(); } #[test] fn diff_modified_file() { let (dir, _repo) = setup_repo(); - fs::write( - dir.path().join("hello.txt"), - "line 1\nline 2 modified\nline 3\nline 4\n", - ) - .unwrap(); + fs::write(dir.path().join("hello.txt"), "line 1\nline 2 modified\nline 3\nline 4\n") + .unwrap(); let output = diff_unstaged(dir.path(), None).unwrap(); assert_eq!(output.files.len(), 1); @@ -252,10 +226,7 @@ mod tests { // Must have hunks with actual content (not empty) assert!(!new_file.hunks.is_empty(), "added file must have hunks"); assert!( - new_file.hunks[0] - .lines - .iter() - .any(|l| l.tag == LineTag::Insert), + new_file.hunks[0].lines.iter().any(|l| l.tag == LineTag::Insert), "added file hunk must have insert lines" ); } @@ -335,11 +306,7 @@ mod tests { let output = diff_unstaged(dir.path(), None).unwrap(); let big_file = output.files.iter().find(|f| f.path == "big.txt").unwrap(); - assert!( - big_file.hunks.len() >= 2, - "expected 2+ hunks, got {}", - big_file.hunks.len() - ); + assert!(big_file.hunks.len() >= 2, "expected 2+ hunks, got {}", big_file.hunks.len()); // Each hunk should have a unique ID let ids: Vec<&str> = big_file.hunks.iter().map(|h| h.id.as_str()).collect(); @@ -361,11 +328,7 @@ mod tests { fn diff_line_numbers_correct() { let (dir, _repo) = setup_repo(); - fs::write( - dir.path().join("hello.txt"), - "line 1\nINSERTED\nline 2\nline 3\n", - ) - .unwrap(); + fs::write(dir.path().join("hello.txt"), "line 1\nINSERTED\nline 2\nline 3\n").unwrap(); let output = diff_unstaged(dir.path(), None).unwrap(); let hunk = &output.files[0].hunks[0]; @@ -376,10 +339,7 @@ mod tests { .find(|l| l.tag == LineTag::Insert && l.content.contains("INSERTED")) .expect("should find inserted line"); - assert!( - inserted.old_lineno.is_none(), - "insert has no old line number" - ); + assert!(inserted.old_lineno.is_none(), "insert has no old line number"); assert!(inserted.new_lineno.is_some(), "insert has new line number"); } } diff --git a/src/git/mod.rs b/src/git/mod.rs index c3d4caf..dcab348 100644 --- a/src/git/mod.rs +++ b/src/git/mod.rs @@ -1,3 +1,48 @@ +use git2::DiffOptions; + pub mod diff; pub mod stage; pub mod status; + +/// Default context lines for all diffs. +const CONTEXT_LINES: u32 = 3; + +/// Create `DiffOptions` including untracked file content. +/// +/// Used for: diff, status, and hunk metadata scanning. +pub fn diff_opts_with_untracked() -> DiffOptions { + let mut opts = DiffOptions::new(); + opts.context_lines(CONTEXT_LINES); + opts.include_untracked(true); + opts.show_untracked_content(true); + opts +} + +/// Create `DiffOptions` for tracked files only. +/// +/// Used for: applying hunks to the index (untracked files handled separately). +pub fn diff_opts_tracked_only() -> DiffOptions { + let mut opts = DiffOptions::new(); + opts.context_lines(CONTEXT_LINES); + opts +} + +/// Extract the file path from a `DiffDelta`, preferring `new_file`. +pub fn delta_path(delta: &git2::DiffDelta) -> String { + delta + .new_file() + .path() + .or_else(|| delta.old_file().path()) + .map(|p| p.to_string_lossy().into_owned()) + .unwrap_or_default() +} + +/// Extract and clean the header string from a git2 `DiffHunk`. +pub fn hunk_header(hunk: &git2::DiffHunk<'_>) -> String { + String::from_utf8_lossy(hunk.header()).trim().to_string() +} + +/// Check if a `DiffDelta` represents a binary file. +pub fn is_binary_delta(delta: &git2::DiffDelta) -> bool { + delta.new_file().is_binary() || delta.old_file().is_binary() +} diff --git a/src/git/stage.rs b/src/git/stage.rs index 1f36d34..37bcc36 100644 --- a/src/git/stage.rs +++ b/src/git/stage.rs @@ -1,29 +1,21 @@ use anyhow::{Context, Result}; -use git2::{ApplyLocation, ApplyOptions, Delta, Diff, DiffOptions, Repository}; +use git2::{ApplyLocation, ApplyOptions, Delta, Diff, Repository}; use std::cell::RefCell; use std::collections::{HashMap, HashSet}; use std::path::Path; use super::diff::hunk_id; +use super::{delta_path, hunk_header, is_binary_delta}; #[cfg(test)] use crate::models::LineSelection; use crate::models::{HunkLine, LineTag, StageRequest, StageResult}; -/// Check if a git2 DiffDelta represents a binary file. -fn is_binary_delta(delta: &git2::DiffDelta) -> bool { - delta.new_file().is_binary() || delta.old_file().is_binary() -} - -/// Scan the diff and return a map of hunk_id → (file_path, is_untracked). +/// Scan the diff and return a map of `hunk_id` → (`file_path`, `is_untracked`). fn scan_hunk_metadata(repo: &Repository) -> Result> { - let mut opts = DiffOptions::new(); - opts.context_lines(3); - opts.include_untracked(true); - opts.show_untracked_content(true); + let mut opts = super::diff_opts_with_untracked(); - let diff = repo - .diff_index_to_workdir(None, Some(&mut opts)) - .context("failed to generate diff")?; + let diff = + repo.diff_index_to_workdir(None, Some(&mut opts)).context("failed to generate diff")?; let metadata: RefCell> = RefCell::new(HashMap::new()); let current: RefCell<(String, bool)> = RefCell::new((String::new(), false)); @@ -34,12 +26,7 @@ fn scan_hunk_metadata(repo: &Repository) -> Result Result) -> String { + use std::fmt::Write; + let mut patch_lines: Vec = Vec::new(); let mut old_count: u32 = 0; let mut new_count: u32 = 0; @@ -132,18 +121,13 @@ fn reconstruct_patch(hunk: &RawHunk, selected_indices: &HashSet) -> Strin } } - let header = format!( - "@@ -{},{} +{},{} @@", - hunk.old_start, old_count, hunk.old_start, new_count - ); + let header = + format!("@@ -{},{} +{},{} @@", hunk.old_start, old_count, hunk.old_start, new_count); let mut patch = String::new(); - patch.push_str(&format!( - "diff --git a/{} b/{}\n", - hunk.file_path, hunk.file_path - )); - patch.push_str(&format!("--- a/{}\n", hunk.file_path)); - patch.push_str(&format!("+++ b/{}\n", hunk.file_path)); + let _ = writeln!(patch, "diff --git a/{0} b/{0}", hunk.file_path); + let _ = writeln!(patch, "--- a/{}", hunk.file_path); + let _ = writeln!(patch, "+++ b/{}", hunk.file_path); patch.push_str(&header); patch.push('\n'); for line in &patch_lines { @@ -158,14 +142,10 @@ fn reconstruct_patch(hunk: &RawHunk, selected_indices: &HashSet) -> Strin /// Collect all hunk data from the unstaged diff, keyed by hunk ID. fn collect_hunks(repo: &Repository) -> Result> { - let mut opts = DiffOptions::new(); - opts.context_lines(3); - opts.include_untracked(true); - opts.show_untracked_content(true); + let mut opts = super::diff_opts_with_untracked(); - let diff = repo - .diff_index_to_workdir(None, Some(&mut opts)) - .context("failed to generate diff")?; + let diff = + repo.diff_index_to_workdir(None, Some(&mut opts)).context("failed to generate diff")?; let state = RefCell::new(CollectState { hunks: HashMap::new(), @@ -180,12 +160,7 @@ fn collect_hunks(repo: &Repository) -> Result> { s.current_path.clear(); s.current_hunk_id = None; } else { - s.current_path = delta - .new_file() - .path() - .or_else(|| delta.old_file().path()) - .map(|p| p.to_string_lossy().into_owned()) - .unwrap_or_default(); + s.current_path = delta_path(&delta); s.current_hunk_id = None; } true @@ -193,7 +168,7 @@ fn collect_hunks(repo: &Repository) -> Result> { None, Some(&mut |_delta, hunk| { let mut s = state.borrow_mut(); - let header = String::from_utf8_lossy(hunk.header()).trim().to_string(); + let header = hunk_header(&hunk); let id = hunk_id(&s.current_path, hunk.old_start(), &header); let raw = RawHunk { file_path: s.current_path.clone(), @@ -206,11 +181,7 @@ fn collect_hunks(repo: &Repository) -> Result> { }), Some(&mut |_delta, _hunk, line| { let mut s = state.borrow_mut(); - let tag = match line.origin() { - '+' | '>' => LineTag::Insert, - '-' | '<' => LineTag::Delete, - _ => LineTag::Equal, - }; + let tag = LineTag::from_origin(line.origin()); let content = String::from_utf8_lossy(line.content()).into_owned(); let hunk_line = HunkLine { tag, @@ -267,7 +238,7 @@ pub fn stage_selection(repo_path: &Path, request: &StageRequest) -> Result = request.hunk_ids.iter().map(|s| s.as_str()).collect(); + let unique_requested: HashSet<&str> = request.hunk_ids.iter().map(String::as_str).collect(); // Separate untracked file hunks from tracked file hunks. let mut untracked_paths: Vec = Vec::new(); @@ -300,8 +271,7 @@ pub fn stage_selection(repo_path: &Path, request: &StageRequest) -> Result Result Result d, - None => return false, + let Some(d) = delta else { + return false; }; if is_binary_delta(&d) { return false; } - let path = d - .new_file() - .path() - .or_else(|| d.old_file().path()) - .map(|p| p.to_string_lossy().into_owned()) - .unwrap_or_default(); - *current_path.borrow_mut() = path; + *current_path.borrow_mut() = delta_path(&d); true }); apply_opts.hunk_callback(|hunk| { - let hunk = match hunk { - Some(h) => h, - None => return false, + let Some(hunk) = hunk else { + return false; }; let path = current_path.borrow(); - let header = String::from_utf8_lossy(hunk.header()).trim().to_string(); + let header = hunk_header(&hunk); let id = hunk_id(&path, hunk.old_start(), &header); valid_ids.contains(&id) }); @@ -413,12 +369,9 @@ pub fn stage_selection(repo_path: &Path, request: &StageRequest) -> Result = sel.line_indices.iter().copied().collect(); // Validate: at least one selected index is a change line (not context) - let has_change = selected.iter().any(|&i| { - raw.lines - .get(i) - .map(|l| l.tag != LineTag::Equal) - .unwrap_or(false) - }); + let has_change = selected + .iter() + .any(|&i| raw.lines.get(i).is_some_and(|l| l.tag != LineTag::Equal)); if !has_change { errors.push(format!("no change lines selected for hunk {}", sel.hunk_id)); failed += 1; @@ -428,13 +381,14 @@ pub fn stage_selection(repo_path: &Path, request: &StageRequest) -> Result { - repo.apply(&patch_diff, ApplyLocation::Index, None) - .with_context(|| { + repo.apply(&patch_diff, ApplyLocation::Index, None).with_context( + || { format!( "failed to apply line selection for hunk {}", sel.hunk_id ) - })?; + }, + )?; staged += 1; } Err(e) => { @@ -450,11 +404,7 @@ pub fn stage_selection(repo_path: &Path, request: &StageRequest) -> Result= 2); let first_id = output.files[0].hunks[0].id.clone(); - let request = StageRequest { - hunk_ids: vec![first_id], - line_selections: vec![], - }; + let request = StageRequest { hunk_ids: vec![first_id], line_selections: vec![] }; let result = stage_selection(dir.path(), &request).unwrap(); assert_eq!(result.staged, 1); assert_eq!(result.failed, 0); @@ -579,10 +516,7 @@ mod tests { let output = diff_unstaged(dir.path(), None).unwrap(); let all_ids: Vec = output.files[0].hunks.iter().map(|h| h.id.clone()).collect(); - let request = StageRequest { - hunk_ids: all_ids.clone(), - line_selections: vec![], - }; + let request = StageRequest { hunk_ids: all_ids.clone(), line_selections: vec![] }; let result = stage_selection(dir.path(), &request).unwrap(); assert_eq!(result.staged, all_ids.len()); assert_eq!(count_staged_hunks(&repo), all_ids.len()); @@ -594,10 +528,8 @@ mod tests { #[test] fn stage_invalid_hunk_id() { let (dir, repo) = setup_two_hunk_repo(); - let request = StageRequest { - hunk_ids: vec!["nonexistent_id".into()], - line_selections: vec![], - }; + let request = + StageRequest { hunk_ids: vec!["nonexistent_id".into()], line_selections: vec![] }; let result = stage_selection(dir.path(), &request).unwrap(); assert_eq!(result.staged, 0); assert_eq!(result.failed, 1); @@ -611,10 +543,8 @@ mod tests { let output = diff_unstaged(dir.path(), None).unwrap(); let valid_id = output.files[0].hunks[0].id.clone(); - let request = StageRequest { - hunk_ids: vec![valid_id, "bad_id".into()], - line_selections: vec![], - }; + let request = + StageRequest { hunk_ids: vec![valid_id, "bad_id".into()], line_selections: vec![] }; let result = stage_selection(dir.path(), &request).unwrap(); assert_eq!(result.staged, 1); assert_eq!(result.failed, 1); @@ -627,10 +557,8 @@ mod tests { let output = diff_unstaged(dir.path(), None).unwrap(); let first_id = output.files[0].hunks[0].id.clone(); - let request = StageRequest { - hunk_ids: vec![first_id.clone(), first_id], - line_selections: vec![], - }; + let request = + StageRequest { hunk_ids: vec![first_id.clone(), first_id], line_selections: vec![] }; let result = stage_selection(dir.path(), &request).unwrap(); assert_eq!(result.staged, 1); assert_eq!(result.failed, 0); @@ -640,10 +568,7 @@ mod tests { #[test] fn stage_empty_request() { let (dir, _repo) = setup_two_hunk_repo(); - let request = StageRequest { - hunk_ids: vec![], - line_selections: vec![], - }; + let request = StageRequest { hunk_ids: vec![], line_selections: vec![] }; let result = stage_selection(dir.path(), &request).unwrap(); assert_eq!(result.staged, 0); assert!(result.errors[0].contains("no hunk IDs")); @@ -656,20 +581,14 @@ mod tests { let first_id = output.files[0].hunks[0].id.clone(); let original_count = output.total_hunks; - let request = StageRequest { - hunk_ids: vec![first_id.clone()], - line_selections: vec![], - }; + let request = StageRequest { hunk_ids: vec![first_id.clone()], line_selections: vec![] }; stage_selection(dir.path(), &request).unwrap(); let after = diff_unstaged(dir.path(), None).unwrap(); assert_eq!(after.total_hunks, original_count - 1); - let remaining_ids: Vec<&str> = after - .files - .iter() - .flat_map(|f| f.hunks.iter().map(|h| h.id.as_str())) - .collect(); + let remaining_ids: Vec<&str> = + after.files.iter().flat_map(|f| f.hunks.iter().map(|h| h.id.as_str())).collect(); assert!(!remaining_ids.contains(&first_id.as_str())); } @@ -702,11 +621,7 @@ mod tests { let output = diff_unstaged(dir.path(), None).unwrap(); let hunk = &output.files[0].hunks[0]; - let insert_idx = hunk - .lines - .iter() - .position(|l| l.tag == LineTag::Insert) - .unwrap(); + let insert_idx = hunk.lines.iter().position(|l| l.tag == LineTag::Insert).unwrap(); let request = StageRequest { hunk_ids: vec![], @@ -732,16 +647,8 @@ mod tests { let output = diff_unstaged(dir.path(), None).unwrap(); let hunk = &output.files[0].hunks[0]; - let delete_idx = hunk - .lines - .iter() - .position(|l| l.tag == LineTag::Delete) - .unwrap(); - let insert_idx = hunk - .lines - .iter() - .position(|l| l.tag == LineTag::Insert) - .unwrap(); + let delete_idx = hunk.lines.iter().position(|l| l.tag == LineTag::Delete).unwrap(); + let insert_idx = hunk.lines.iter().position(|l| l.tag == LineTag::Insert).unwrap(); let request = StageRequest { hunk_ids: vec![], @@ -856,10 +763,7 @@ mod tests { stage_selection(dir.path(), &request).unwrap(); let remaining = diff_unstaged(dir.path(), None).unwrap(); - assert!( - remaining.total_hunks > 0, - "should still have unstaged changes" - ); + assert!(remaining.total_hunks > 0, "should still have unstaged changes"); } #[test] @@ -931,19 +835,10 @@ mod tests { // Should be able to stage tracked file hunks despite untracked file let output = diff_unstaged(dir.path(), None).unwrap(); - let tracked_hunk = output - .files - .iter() - .find(|f| f.path == "file.txt") - .unwrap() - .hunks[0] - .id - .clone(); + let tracked_hunk = + output.files.iter().find(|f| f.path == "file.txt").unwrap().hunks[0].id.clone(); - let request = StageRequest { - hunk_ids: vec![tracked_hunk], - line_selections: vec![], - }; + let request = StageRequest { hunk_ids: vec![tracked_hunk], line_selections: vec![] }; let result = stage_selection(dir.path(), &request).unwrap(); assert_eq!(result.staged, 1); assert_eq!(result.failed, 0); @@ -974,10 +869,7 @@ mod tests { let new_hunk_id = new_file.unwrap().hunks[0].id.clone(); // Should be able to stage the untracked file's hunk - let request = StageRequest { - hunk_ids: vec![new_hunk_id], - line_selections: vec![], - }; + let request = StageRequest { hunk_ids: vec![new_hunk_id], line_selections: vec![] }; let result = stage_selection(dir.path(), &request).unwrap(); assert_eq!(result.staged, 1); assert_eq!(result.failed, 0); @@ -995,28 +887,14 @@ mod tests { fs::write(dir.path().join("new.txt"), "new content\n").unwrap(); let output = diff_unstaged(dir.path(), None).unwrap(); - let tracked_id = output - .files - .iter() - .find(|f| f.path == "file.txt") - .unwrap() - .hunks[0] - .id - .clone(); - let untracked_id = output - .files - .iter() - .find(|f| f.path == "new.txt") - .unwrap() - .hunks[0] - .id - .clone(); + let tracked_id = + output.files.iter().find(|f| f.path == "file.txt").unwrap().hunks[0].id.clone(); + let untracked_id = + output.files.iter().find(|f| f.path == "new.txt").unwrap().hunks[0].id.clone(); // Stage both in one request - let request = StageRequest { - hunk_ids: vec![tracked_id, untracked_id], - line_selections: vec![], - }; + let request = + StageRequest { hunk_ids: vec![tracked_id, untracked_id], line_selections: vec![] }; let result = stage_selection(dir.path(), &request).unwrap(); assert_eq!(result.staged, 2); assert_eq!(result.failed, 0); diff --git a/src/git/status.rs b/src/git/status.rs index cccb057..3673ff4 100644 --- a/src/git/status.rs +++ b/src/git/status.rs @@ -1,5 +1,5 @@ use anyhow::{Context, Result}; -use git2::{DiffOptions, Repository}; +use git2::Repository; use std::path::Path; use crate::models::StatusSummary; @@ -30,23 +30,17 @@ pub fn get_status(repo_path: &Path) -> Result { }; // Unstaged: diff between index and working directory - let mut opts = DiffOptions::new(); - opts.include_untracked(true); - opts.show_untracked_content(true); + let mut opts = super::diff_opts_with_untracked(); let unstaged_diff = repo .diff_index_to_workdir(None, Some(&mut opts)) .context("failed to diff index to workdir")?; let (unstaged_files, unstaged_hunks) = count_files_and_hunks(&unstaged_diff)?; - Ok(StatusSummary { - staged_files, - unstaged_files, - staged_hunks, - unstaged_hunks, - }) + Ok(StatusSummary { staged_files, unstaged_files, staged_hunks, unstaged_hunks }) } +/// Count distinct files and hunks in a git2 `Diff`. fn count_files_and_hunks(diff: &git2::Diff) -> Result<(usize, usize)> { let mut files = 0usize; let mut hunks = 0usize; @@ -91,8 +85,7 @@ mod tests { index.write().unwrap(); let tree_oid = index.write_tree().unwrap(); let tree = repo.find_tree(tree_oid).unwrap(); - repo.commit(Some("HEAD"), &sig, &sig, "initial", &tree, &[]) - .unwrap(); + repo.commit(Some("HEAD"), &sig, &sig, "initial", &tree, &[]).unwrap(); } (dir, repo) @@ -131,10 +124,7 @@ mod tests { let hunk_id = output.files[0].hunks[0].id.clone(); stage_selection( dir.path(), - &StageRequest { - hunk_ids: vec![hunk_id], - line_selections: vec![], - }, + &StageRequest { hunk_ids: vec![hunk_id], line_selections: vec![] }, ) .unwrap(); diff --git a/src/main.rs b/src/main.rs index 2b86732..87d0cde 100644 --- a/src/main.rs +++ b/src/main.rs @@ -16,10 +16,7 @@ fn main() -> Result<()> { let diff = git::diff::diff_unstaged(&cli.repo, file.as_ref().and_then(|f| f.to_str()))?; output::print_diff(&diff, &cli.format); } - Commands::Stage { - hunk_ids, - from_stdin, - } => { + Commands::Stage { hunk_ids, from_stdin } => { let request = if *from_stdin { serde_json::from_reader(std::io::stdin())? } else { diff --git a/src/models.rs b/src/models.rs index ce42193..90fea07 100644 --- a/src/models.rs +++ b/src/models.rs @@ -19,6 +19,17 @@ pub enum LineTag { Delete, } +impl LineTag { + /// Convert a git2 line origin character to a `LineTag`. + pub fn from_origin(origin: char) -> Self { + match origin { + '+' | '>' => Self::Insert, + '-' | '<' => Self::Delete, + _ => Self::Equal, + } + } +} + /// A diff hunk with stable ID for agent reference. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct Hunk { @@ -84,6 +95,9 @@ pub struct StageResult { pub errors: Vec, } +/// Protocol response version. Increment on breaking schema changes. +pub const PROTOCOL_VERSION: u8 = 1; + /// Generic JSON response envelope. #[derive(Debug, PartialEq, Serialize, Deserialize)] pub struct Response { @@ -97,23 +111,13 @@ pub struct Response { impl Response { pub fn success(data: T) -> Self { - Self { - version: 1, - ok: true, - data: Some(data), - error: None, - } + Self { version: PROTOCOL_VERSION, ok: true, data: Some(data), error: None } } } impl Response<()> { pub fn error(msg: impl Into) -> Self { - Self { - version: 1, - ok: false, - data: None, - error: Some(msg.into()), - } + Self { version: PROTOCOL_VERSION, ok: false, data: None, error: Some(msg.into()) } } } @@ -198,36 +202,18 @@ mod tests { #[test] fn linetag_json_values() { assert_eq!(serde_json::to_string(&LineTag::Equal).unwrap(), "\"equal\""); - assert_eq!( - serde_json::to_string(&LineTag::Insert).unwrap(), - "\"insert\"" - ); - assert_eq!( - serde_json::to_string(&LineTag::Delete).unwrap(), - "\"delete\"" - ); + assert_eq!(serde_json::to_string(&LineTag::Insert).unwrap(), "\"insert\""); + assert_eq!(serde_json::to_string(&LineTag::Delete).unwrap(), "\"delete\""); } // --- FileStatus JSON values --- #[test] fn filestatus_json_values() { - assert_eq!( - serde_json::to_string(&FileStatus::Modified).unwrap(), - "\"modified\"" - ); - assert_eq!( - serde_json::to_string(&FileStatus::Added).unwrap(), - "\"added\"" - ); - assert_eq!( - serde_json::to_string(&FileStatus::Deleted).unwrap(), - "\"deleted\"" - ); - assert_eq!( - serde_json::to_string(&FileStatus::Renamed).unwrap(), - "\"renamed\"" - ); + assert_eq!(serde_json::to_string(&FileStatus::Modified).unwrap(), "\"modified\""); + assert_eq!(serde_json::to_string(&FileStatus::Added).unwrap(), "\"added\""); + assert_eq!(serde_json::to_string(&FileStatus::Deleted).unwrap(), "\"deleted\""); + assert_eq!(serde_json::to_string(&FileStatus::Renamed).unwrap(), "\"renamed\""); } // --- Hunk round-trip --- @@ -287,10 +273,7 @@ mod tests { #[test] fn diff_output_empty() { - let output = DiffOutput { - files: vec![], - total_hunks: 0, - }; + let output = DiffOutput { files: vec![], total_hunks: 0 }; let json = serde_json::to_string(&output).unwrap(); assert_eq!(json, r#"{"files":[],"total_hunks":0}"#); } @@ -299,10 +282,8 @@ mod tests { #[test] fn stage_request_hunk_ids_roundtrip() { - let req = StageRequest { - hunk_ids: vec!["abc".into(), "def".into()], - line_selections: vec![], - }; + let req = + StageRequest { hunk_ids: vec!["abc".into(), "def".into()], line_selections: vec![] }; let json = serde_json::to_string(&req).unwrap(); let back: StageRequest = serde_json::from_str(&json).unwrap(); assert_eq!(req, back); @@ -334,11 +315,7 @@ mod tests { #[test] fn stage_result_success_roundtrip() { - let result = StageResult { - staged: 3, - failed: 0, - errors: vec![], - }; + let result = StageResult { staged: 3, failed: 0, errors: vec![] }; let json = serde_json::to_string(&result).unwrap(); // errors should be omitted when empty assert!(!json.contains("errors")); @@ -363,10 +340,7 @@ mod tests { #[test] fn response_success_roundtrip() { - let resp = Response::success(DiffOutput { - files: vec![], - total_hunks: 0, - }); + let resp = Response::success(DiffOutput { files: vec![], total_hunks: 0 }); let json = serde_json::to_string(&resp).unwrap(); // Should not contain "error" key assert!(!json.contains("\"error\"")); @@ -386,11 +360,7 @@ mod tests { #[test] fn response_schema_shape() { - let resp = Response::success(StageResult { - staged: 1, - failed: 0, - errors: vec![], - }); + let resp = Response::success(StageResult { staged: 1, failed: 0, errors: vec![] }); let val: serde_json::Value = serde_json::to_value(&resp).unwrap(); assert_eq!(val["version"], 1); assert_eq!(val["ok"], true); @@ -453,16 +423,9 @@ mod tests { #[test] fn protocol_request_roundtrip() { let requests = vec![ - ProtocolRequest::Diff { - params: DiffParams { - file: Some("test.rs".into()), - }, - }, + ProtocolRequest::Diff { params: DiffParams { file: Some("test.rs".into()) } }, ProtocolRequest::Stage { - params: StageRequest { - hunk_ids: vec!["id1".into()], - line_selections: vec![], - }, + params: StageRequest { hunk_ids: vec!["id1".into()], line_selections: vec![] }, }, ProtocolRequest::Status, ]; diff --git a/src/output.rs b/src/output.rs index 7441293..7e7c5f5 100644 --- a/src/output.rs +++ b/src/output.rs @@ -3,7 +3,7 @@ use crate::models::{DiffOutput, LineTag, Response, StageResult, StatusSummary}; use std::io::IsTerminal; /// Resolve Auto format to Json or Human based on terminal detection. -pub fn resolve_format(format: &OutputFormat) -> OutputFormat { +fn resolve_format(format: &OutputFormat) -> OutputFormat { match format { OutputFormat::Auto => { if std::io::stdout().is_terminal() { @@ -16,13 +16,16 @@ pub fn resolve_format(format: &OutputFormat) -> OutputFormat { } } +/// Print a value as a JSON `Response::success` envelope. +fn print_json(data: &impl serde::Serialize) { + let resp = Response::success(data); + println!("{}", serde_json::to_string(&resp).unwrap()); +} + /// Print diff output in the requested format. pub fn print_diff(output: &DiffOutput, format: &OutputFormat) { match resolve_format(format) { - OutputFormat::Json => { - let resp = Response::success(output); - println!("{}", serde_json::to_string(&resp).unwrap()); - } + OutputFormat::Json => print_json(output), OutputFormat::Human => { if output.files.is_empty() { println!("No unstaged changes."); @@ -46,11 +49,7 @@ pub fn print_diff(output: &DiffOutput, format: &OutputFormat) { } println!(); } - println!( - "Total: {} file(s), {} hunk(s)", - output.files.len(), - output.total_hunks - ); + println!("Total: {} file(s), {} hunk(s)", output.files.len(), output.total_hunks); } OutputFormat::Auto => unreachable!("resolve_format always resolves Auto"), } @@ -59,10 +58,7 @@ pub fn print_diff(output: &DiffOutput, format: &OutputFormat) { /// Print stage result. pub fn print_stage_result(result: &StageResult, format: &OutputFormat) { match resolve_format(format) { - OutputFormat::Json => { - let resp = Response::success(result); - println!("{}", serde_json::to_string(&resp).unwrap()); - } + OutputFormat::Json => print_json(result), OutputFormat::Human => { println!("Staged: {}, Failed: {}", result.staged, result.failed); for err in &result.errors { @@ -76,15 +72,9 @@ pub fn print_stage_result(result: &StageResult, format: &OutputFormat) { /// Print status summary. pub fn print_status(status: &StatusSummary, format: &OutputFormat) { match resolve_format(format) { - OutputFormat::Json => { - let resp = Response::success(status); - println!("{}", serde_json::to_string(&resp).unwrap()); - } + OutputFormat::Json => print_json(status), OutputFormat::Human => { - println!( - "Staged: {} file(s), {} hunk(s)", - status.staged_files, status.staged_hunks - ); + println!("Staged: {} file(s), {} hunk(s)", status.staged_files, status.staged_hunks); println!( "Unstaged: {} file(s), {} hunk(s)", status.unstaged_files, status.unstaged_hunks diff --git a/src/protocol.rs b/src/protocol.rs index fef17f3..8bdb703 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -16,14 +16,11 @@ fn write_response(out: &mut impl Write, resp: &Response) out.flush() } -/// Read a single line from the reader, respecting MAX_LINE_LENGTH. +/// Read a single line from the reader, respecting `MAX_LINE_LENGTH`. /// Returns None on EOF, Some(Err) on read error, Some(Ok(bytes)) on success. fn read_line_bytes(reader: &mut impl BufRead) -> Option>> { let mut buf = Vec::new(); - match reader - .take(MAX_LINE_LENGTH as u64) - .read_until(b'\n', &mut buf) - { + match reader.take(MAX_LINE_LENGTH as u64).read_until(b'\n', &mut buf) { Ok(0) => None, // EOF Ok(_) => { // Strip trailing newline @@ -39,6 +36,17 @@ fn read_line_bytes(reader: &mut impl BufRead) -> Option>> { } } +/// Write a success or error response from a `Result`, converting errors to error envelopes. +fn dispatch( + out: &mut impl Write, + result: anyhow::Result, +) -> io::Result<()> { + match result { + Ok(data) => write_response(out, &Response::success(data)), + Err(e) => write_response(out, &Response::<()>::error(format!("{e}"))), + } +} + /// Enter the JSON-lines protocol loop: read requests from stdin, write responses to stdout. /// /// Each line on stdin is a JSON request with a `"method"` field. @@ -66,20 +74,17 @@ pub fn run_protocol(repo_path: &Path) -> Result<()> { }; // Skip empty lines - if bytes.iter().all(|b| b.is_ascii_whitespace()) { + if bytes.iter().all(u8::is_ascii_whitespace) { continue; } // Parse as UTF-8, return error response for invalid encoding - let line = match String::from_utf8(bytes) { - Ok(s) => s, - Err(_) => { - let resp = Response::<()>::error("invalid UTF-8 input"); - if write_response(&mut out, &resp).is_err() { - break; - } - continue; + let Ok(line) = String::from_utf8(bytes) else { + let resp = Response::<()>::error("invalid UTF-8 input"); + if write_response(&mut out, &resp).is_err() { + break; } + continue; }; let request = match serde_json::from_str::(&line) { @@ -95,21 +100,12 @@ pub fn run_protocol(repo_path: &Path) -> Result<()> { let write_ok = match request { ProtocolRequest::Diff { params } => { - match git::diff::diff_unstaged(repo_path, params.file.as_deref()) { - Ok(diff) => write_response(&mut out, &Response::success(diff)), - Err(e) => write_response(&mut out, &Response::<()>::error(format!("{e}"))), - } + dispatch(&mut out, git::diff::diff_unstaged(repo_path, params.file.as_deref())) } ProtocolRequest::Stage { params } => { - match git::stage::stage_selection(repo_path, ¶ms) { - Ok(result) => write_response(&mut out, &Response::success(result)), - Err(e) => write_response(&mut out, &Response::<()>::error(format!("{e}"))), - } + dispatch(&mut out, git::stage::stage_selection(repo_path, ¶ms)) } - ProtocolRequest::Status => match git::status::get_status(repo_path) { - Ok(status) => write_response(&mut out, &Response::success(status)), - Err(e) => write_response(&mut out, &Response::<()>::error(format!("{e}"))), - }, + ProtocolRequest::Status => dispatch(&mut out, git::status::get_status(repo_path)), }; if write_ok.is_err() { diff --git a/tests/cli.rs b/tests/cli.rs index 4a56e51..9d35e52 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -1,33 +1,7 @@ -use assert_cmd::Command; -use git2::{Repository, Signature}; -use std::fs; -use std::path::Path; -use tempfile::TempDir; - -/// Create a temp repo with an initial commit and return dir + path. -fn setup_repo() -> TempDir { - let dir = TempDir::new().unwrap(); - let repo = Repository::init(dir.path()).unwrap(); - let sig = Signature::now("test", "test@test.com").unwrap(); - - fs::write(dir.path().join("hello.txt"), "line 1\nline 2\nline 3\n").unwrap(); - - { - let mut index = repo.index().unwrap(); - index.add_path(Path::new("hello.txt")).unwrap(); - index.write().unwrap(); - let tree_oid = index.write_tree().unwrap(); - let tree = repo.find_tree(tree_oid).unwrap(); - repo.commit(Some("HEAD"), &sig, &sig, "initial", &tree, &[]) - .unwrap(); - } - - dir -} +mod common; -fn gitsift() -> Command { - Command::cargo_bin("gitsift").unwrap() -} +use common::{gitsift, parse_json, setup_repo}; +use std::fs; // ===== diff subcommand ===== @@ -36,16 +10,12 @@ fn diff_json_output_valid() { let dir = setup_repo(); fs::write(dir.path().join("hello.txt"), "line 1\nchanged\nline 3\n").unwrap(); - let output = gitsift() - .args(["diff", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let output = + gitsift().args(["diff", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); assert!(output.status.success()); - let stdout = String::from_utf8(output.stdout).unwrap(); - let val: serde_json::Value = serde_json::from_str(&stdout).unwrap(); + let val = parse_json(&output.stdout); assert_eq!(val["ok"], true); assert_eq!(val["version"], 1); assert!(val["data"]["files"].is_array()); @@ -56,15 +26,11 @@ fn diff_json_output_valid() { fn diff_json_no_changes() { let dir = setup_repo(); - let output = gitsift() - .args(["diff", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let output = + gitsift().args(["diff", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); assert!(output.status.success()); - let stdout = String::from_utf8(output.stdout).unwrap(); - let val: serde_json::Value = serde_json::from_str(&stdout).unwrap(); + let val = parse_json(&output.stdout); assert_eq!(val["data"]["total_hunks"], 0); } @@ -73,17 +39,14 @@ fn diff_human_output_contains_hunk_ids() { let dir = setup_repo(); fs::write(dir.path().join("hello.txt"), "changed\n").unwrap(); - let output = gitsift() - .args(["diff", "--format", "human", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let output = + gitsift().args(["diff", "--format", "human", "--repo"]).arg(dir.path()).output().unwrap(); assert!(output.status.success()); let stdout = String::from_utf8(output.stdout).unwrap(); // Human output shows hunk IDs in brackets and @@ headers assert!(stdout.contains("@@")); - assert!(stdout.contains("[")); + assert!(stdout.contains('[')); } #[test] @@ -99,8 +62,7 @@ fn diff_file_filter() { .unwrap(); assert!(output.status.success()); - let stdout = String::from_utf8(output.stdout).unwrap(); - let val: serde_json::Value = serde_json::from_str(&stdout).unwrap(); + let val = parse_json(&output.stdout); let files = val["data"]["files"].as_array().unwrap(); assert_eq!(files.len(), 1); assert_eq!(files[0]["path"], "hello.txt"); @@ -126,16 +88,10 @@ fn stage_hunk_by_id() { fs::write(dir.path().join("hello.txt"), "changed\n").unwrap(); // Get hunk ID from diff - let diff_output = gitsift() - .args(["diff", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); - let diff_val: serde_json::Value = - serde_json::from_str(&String::from_utf8(diff_output.stdout).unwrap()).unwrap(); - let hunk_id = diff_val["data"]["files"][0]["hunks"][0]["id"] - .as_str() - .unwrap(); + let diff_output = + gitsift().args(["diff", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); + let diff_val = parse_json(&diff_output.stdout); + let hunk_id = diff_val["data"]["files"][0]["hunks"][0]["id"].as_str().unwrap(); // Stage it let stage_output = gitsift() @@ -145,20 +101,15 @@ fn stage_hunk_by_id() { .unwrap(); assert!(stage_output.status.success()); - let stage_val: serde_json::Value = - serde_json::from_str(&String::from_utf8(stage_output.stdout).unwrap()).unwrap(); + let stage_val = parse_json(&stage_output.stdout); assert_eq!(stage_val["ok"], true); assert_eq!(stage_val["data"]["staged"], 1); assert_eq!(stage_val["data"]["failed"], 0); // Verify status shows staged - let status_output = gitsift() - .args(["status", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); - let status_val: serde_json::Value = - serde_json::from_str(&String::from_utf8(status_output.stdout).unwrap()).unwrap(); + let status_output = + gitsift().args(["status", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); + let status_val = parse_json(&status_output.stdout); assert_eq!(status_val["data"]["staged_hunks"], 1); assert_eq!(status_val["data"]["unstaged_hunks"], 0); } @@ -175,8 +126,7 @@ fn stage_invalid_id() { .unwrap(); assert!(output.status.success()); - let val: serde_json::Value = - serde_json::from_str(&String::from_utf8(output.stdout).unwrap()).unwrap(); + let val = parse_json(&output.stdout); assert_eq!(val["data"]["staged"], 0); assert_eq!(val["data"]["failed"], 1); } @@ -188,15 +138,11 @@ fn status_json_output() { let dir = setup_repo(); fs::write(dir.path().join("hello.txt"), "changed\n").unwrap(); - let output = gitsift() - .args(["status", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let output = + gitsift().args(["status", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); assert!(output.status.success()); - let val: serde_json::Value = - serde_json::from_str(&String::from_utf8(output.stdout).unwrap()).unwrap(); + let val = parse_json(&output.stdout); assert_eq!(val["ok"], true); assert_eq!(val["data"]["unstaged_files"], 1); assert_eq!(val["data"]["staged_files"], 0); @@ -207,11 +153,8 @@ fn status_human_output() { let dir = setup_repo(); fs::write(dir.path().join("hello.txt"), "changed\n").unwrap(); - let output = gitsift() - .args(["status", "--format", "human", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let output = + gitsift().args(["status", "--format", "human", "--repo"]).arg(dir.path()).output().unwrap(); assert!(output.status.success()); let stdout = String::from_utf8(output.stdout).unwrap(); @@ -237,21 +180,13 @@ fn help_shows_subcommands() { #[test] fn full_workflow_diff_stage_status() { let dir = setup_repo(); - fs::write( - dir.path().join("hello.txt"), - "line 1\nchanged\nline 3\nnew line\n", - ) - .unwrap(); + fs::write(dir.path().join("hello.txt"), "line 1\nchanged\nline 3\nnew line\n").unwrap(); // 1. Diff - let diff_out = gitsift() - .args(["diff", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let diff_out = + gitsift().args(["diff", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); assert!(diff_out.status.success()); - let diff_val: serde_json::Value = - serde_json::from_str(&String::from_utf8(diff_out.stdout).unwrap()).unwrap(); + let diff_val = parse_json(&diff_out.stdout); let hunks = diff_val["data"]["files"][0]["hunks"].as_array().unwrap(); assert!(!hunks.is_empty()); let hunk_id = hunks[0]["id"].as_str().unwrap(); @@ -263,18 +198,13 @@ fn full_workflow_diff_stage_status() { .output() .unwrap(); assert!(stage_out.status.success()); - let stage_val: serde_json::Value = - serde_json::from_str(&String::from_utf8(stage_out.stdout).unwrap()).unwrap(); + let stage_val = parse_json(&stage_out.stdout); assert_eq!(stage_val["data"]["staged"], 1); // 3. Status — should show staged - let status_out = gitsift() - .args(["status", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); - let status_val: serde_json::Value = - serde_json::from_str(&String::from_utf8(status_out.stdout).unwrap()).unwrap(); + let status_out = + gitsift().args(["status", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); + let status_val = parse_json(&status_out.stdout); assert!(status_val["data"]["staged_hunks"].as_u64().unwrap() > 0); // 4. Diff again — should show no remaining unstaged hunks for this file @@ -283,7 +213,6 @@ fn full_workflow_diff_stage_status() { .arg(dir.path()) .output() .unwrap(); - let diff2_val: serde_json::Value = - serde_json::from_str(&String::from_utf8(diff2_out.stdout).unwrap()).unwrap(); + let diff2_val = parse_json(&diff2_out.stdout); assert_eq!(diff2_val["data"]["total_hunks"], 0); } diff --git a/tests/common/mod.rs b/tests/common/mod.rs new file mode 100644 index 0000000..5bf6ee9 --- /dev/null +++ b/tests/common/mod.rs @@ -0,0 +1,37 @@ +#![allow(dead_code)] + +use assert_cmd::Command; +use git2::{Repository, Signature}; +use std::fs; +use std::path::Path; +use tempfile::TempDir; + +/// Create a temp repo with an initial commit containing `hello.txt`. +pub fn setup_repo() -> TempDir { + let dir = TempDir::new().unwrap(); + let repo = Repository::init(dir.path()).unwrap(); + let sig = Signature::now("test", "test@test.com").unwrap(); + + fs::write(dir.path().join("hello.txt"), "line 1\nline 2\nline 3\n").unwrap(); + + { + let mut index = repo.index().unwrap(); + index.add_path(Path::new("hello.txt")).unwrap(); + index.write().unwrap(); + let tree_oid = index.write_tree().unwrap(); + let tree = repo.find_tree(tree_oid).unwrap(); + repo.commit(Some("HEAD"), &sig, &sig, "initial", &tree, &[]).unwrap(); + } + + dir +} + +/// Build a `Command` for the gitsift binary. +pub fn gitsift() -> Command { + Command::cargo_bin("gitsift").unwrap() +} + +/// Parse stdout bytes as a single JSON value. +pub fn parse_json(stdout: &[u8]) -> serde_json::Value { + serde_json::from_slice(stdout).unwrap() +} diff --git a/tests/edge_cases.rs b/tests/edge_cases.rs index b1da72a..779fbf3 100644 --- a/tests/edge_cases.rs +++ b/tests/edge_cases.rs @@ -1,40 +1,14 @@ //! Edge case and cross-cutting integration tests for gitsift. //! Covers scenarios not tested in individual module tests. -use assert_cmd::Command; +mod common; + +use common::{gitsift, parse_json, setup_repo}; use git2::{Repository, Signature}; use std::fs; use std::path::Path; use tempfile::TempDir; -fn setup_repo() -> TempDir { - let dir = TempDir::new().unwrap(); - let repo = Repository::init(dir.path()).unwrap(); - let sig = Signature::now("test", "test@test.com").unwrap(); - - fs::write(dir.path().join("hello.txt"), "line 1\nline 2\nline 3\n").unwrap(); - - { - let mut index = repo.index().unwrap(); - index.add_path(Path::new("hello.txt")).unwrap(); - index.write().unwrap(); - let tree_oid = index.write_tree().unwrap(); - let tree = repo.find_tree(tree_oid).unwrap(); - repo.commit(Some("HEAD"), &sig, &sig, "initial", &tree, &[]) - .unwrap(); - } - - dir -} - -fn gitsift() -> Command { - Command::cargo_bin("gitsift").unwrap() -} - -fn parse_json(stdout: &[u8]) -> serde_json::Value { - serde_json::from_slice(stdout).unwrap() -} - // ===== Binary files ===== #[test] @@ -44,11 +18,8 @@ fn binary_file_skipped_in_diff() { // Create a binary file (contains null bytes) fs::write(dir.path().join("image.png"), b"\x89PNG\r\n\x1a\n\x00\x00").unwrap(); - let output = gitsift() - .args(["diff", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let output = + gitsift().args(["diff", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); assert!(output.status.success()); let val = parse_json(&output.stdout); @@ -97,22 +68,16 @@ fn deleted_file_diff_and_status() { fs::remove_file(dir.path().join("hello.txt")).unwrap(); // Diff should show deleted file - let diff_out = gitsift() - .args(["diff", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let diff_out = + gitsift().args(["diff", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); let diff_val = parse_json(&diff_out.stdout); let file = &diff_val["data"]["files"][0]; assert_eq!(file["status"], "deleted"); assert_eq!(file["path"], "hello.txt"); // Status should show 1 unstaged file - let status_out = gitsift() - .args(["status", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let status_out = + gitsift().args(["status", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); let status_val = parse_json(&status_out.stdout); assert_eq!(status_val["data"]["unstaged_files"], 1); } @@ -125,11 +90,8 @@ fn new_file_diff_shows_added() { fs::write(dir.path().join("new_feature.rs"), "fn main() {}\n").unwrap(); - let output = gitsift() - .args(["diff", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let output = + gitsift().args(["diff", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); let val = parse_json(&output.stdout); let new_file = val["data"]["files"] .as_array() @@ -152,15 +114,10 @@ fn stage_from_stdin_json() { fs::write(dir.path().join("hello.txt"), "changed\n").unwrap(); // Get hunk ID - let diff_out = gitsift() - .args(["diff", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let diff_out = + gitsift().args(["diff", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); let diff_val = parse_json(&diff_out.stdout); - let hunk_id = diff_val["data"]["files"][0]["hunks"][0]["id"] - .as_str() - .unwrap(); + let hunk_id = diff_val["data"]["files"][0]["hunks"][0]["id"].as_str().unwrap(); // Stage via --from-stdin with JSON payload let stdin_json = format!(r#"{{"hunk_ids": ["{hunk_id}"]}}"#); @@ -193,8 +150,7 @@ fn multi_file_selective_staging() { let tree_oid = index.write_tree().unwrap(); let tree = repo.find_tree(tree_oid).unwrap(); let head = repo.head().unwrap().peel_to_commit().unwrap(); - repo.commit(Some("HEAD"), &sig, &sig, "add second", &tree, &[&head]) - .unwrap(); + repo.commit(Some("HEAD"), &sig, &sig, "add second", &tree, &[&head]).unwrap(); } // Modify both files @@ -202,11 +158,8 @@ fn multi_file_selective_staging() { fs::write(dir.path().join("second.txt"), "second changed\n").unwrap(); // Get diff — should have 2 files - let diff_out = gitsift() - .args(["diff", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let diff_out = + gitsift().args(["diff", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); let diff_val = parse_json(&diff_out.stdout); let files = diff_val["data"]["files"].as_array().unwrap(); assert_eq!(files.len(), 2); @@ -224,21 +177,15 @@ fn multi_file_selective_staging() { assert_eq!(stage_val["data"]["staged"], 1); // Status: 1 staged (hello.txt), 1 unstaged (second.txt) - let status_out = gitsift() - .args(["status", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let status_out = + gitsift().args(["status", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); let status_val = parse_json(&status_out.stdout); assert_eq!(status_val["data"]["staged_files"], 1); assert_eq!(status_val["data"]["unstaged_files"], 1); // Diff should only show second.txt remaining - let diff2_out = gitsift() - .args(["diff", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let diff2_out = + gitsift().args(["diff", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); let diff2_val = parse_json(&diff2_out.stdout); let remaining = diff2_val["data"]["files"].as_array().unwrap(); assert_eq!(remaining.len(), 1); @@ -265,9 +212,7 @@ fn protocol_session_diff_stage_status_diff() { .filter(|l| !l.is_empty()) .map(|l| serde_json::from_str(l).unwrap()) .collect(); - let hunk_id = diff_lines[0]["data"]["files"][0]["hunks"][0]["id"] - .as_str() - .unwrap(); + let hunk_id = diff_lines[0]["data"]["files"][0]["hunks"][0]["id"].as_str().unwrap(); // Step 2: stage via protocol let stage_stdin = @@ -326,21 +271,15 @@ fn empty_repo_all_commands_work() { let _repo = Repository::init(dir.path()).unwrap(); // diff - let diff_out = gitsift() - .args(["diff", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let diff_out = + gitsift().args(["diff", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); assert!(diff_out.status.success()); let diff_val = parse_json(&diff_out.stdout); assert_eq!(diff_val["data"]["total_hunks"], 0); // status - let status_out = gitsift() - .args(["status", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let status_out = + gitsift().args(["status", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); assert!(status_out.status.success()); let status_val = parse_json(&status_out.stdout); assert_eq!(status_val["data"]["staged_files"], 0); @@ -353,11 +292,8 @@ fn empty_repo_all_commands_work() { fn clean_repo_diff_returns_empty() { let dir = setup_repo(); - let output = gitsift() - .args(["diff", "--format", "json", "--repo"]) - .arg(dir.path()) - .output() - .unwrap(); + let output = + gitsift().args(["diff", "--format", "json", "--repo"]).arg(dir.path()).output().unwrap(); assert!(output.status.success()); let val = parse_json(&output.stdout); assert_eq!(val["data"]["total_hunks"], 0); diff --git a/tests/protocol.rs b/tests/protocol.rs index 670e00d..6f5cfdd 100644 --- a/tests/protocol.rs +++ b/tests/protocol.rs @@ -1,32 +1,7 @@ -use assert_cmd::Command; -use git2::{Repository, Signature}; -use std::fs; -use std::path::Path; -use tempfile::TempDir; - -fn setup_repo() -> TempDir { - let dir = TempDir::new().unwrap(); - let repo = Repository::init(dir.path()).unwrap(); - let sig = Signature::now("test", "test@test.com").unwrap(); - - fs::write(dir.path().join("hello.txt"), "line 1\nline 2\nline 3\n").unwrap(); - - { - let mut index = repo.index().unwrap(); - index.add_path(Path::new("hello.txt")).unwrap(); - index.write().unwrap(); - let tree_oid = index.write_tree().unwrap(); - let tree = repo.find_tree(tree_oid).unwrap(); - repo.commit(Some("HEAD"), &sig, &sig, "initial", &tree, &[]) - .unwrap(); - } +mod common; - dir -} - -fn gitsift() -> Command { - Command::cargo_bin("gitsift").unwrap() -} +use common::{gitsift, setup_repo}; +use std::fs; /// Parse each line of stdout as a JSON value. fn parse_response_lines(stdout: &str) -> Vec { @@ -112,12 +87,7 @@ fn protocol_invalid_json() { let responses = parse_response_lines(&String::from_utf8(output.stdout).unwrap()); assert_eq!(responses.len(), 1); assert_eq!(responses[0]["ok"], false); - assert!( - responses[0]["error"] - .as_str() - .unwrap() - .contains("invalid request") - ); + assert!(responses[0]["error"].as_str().unwrap().contains("invalid request")); } #[test] @@ -168,12 +138,8 @@ fn protocol_multiple_requests() { .join("\n") + "\n"; - let output = gitsift() - .args(["protocol", "--repo"]) - .arg(dir.path()) - .write_stdin(stdin) - .output() - .unwrap(); + let output = + gitsift().args(["protocol", "--repo"]).arg(dir.path()).write_stdin(stdin).output().unwrap(); assert!(output.status.success()); let responses = parse_response_lines(&String::from_utf8(output.stdout).unwrap()); @@ -197,9 +163,7 @@ fn protocol_full_workflow_diff_stage_status() { .output() .unwrap(); let diff_resp = parse_response_lines(&String::from_utf8(diff_output.stdout).unwrap()); - let hunk_id = diff_resp[0]["data"]["files"][0]["hunks"][0]["id"] - .as_str() - .unwrap(); + let hunk_id = diff_resp[0]["data"]["files"][0]["hunks"][0]["id"].as_str().unwrap(); // Step 2: stage that hunk let stage_stdin = @@ -233,12 +197,8 @@ fn protocol_each_response_is_one_line() { let stdin = "{\"method\": \"diff\"}\n{\"method\": \"status\"}\n"; - let output = gitsift() - .args(["protocol", "--repo"]) - .arg(dir.path()) - .write_stdin(stdin) - .output() - .unwrap(); + let output = + gitsift().args(["protocol", "--repo"]).arg(dir.path()).write_stdin(stdin).output().unwrap(); let stdout = String::from_utf8(output.stdout).unwrap(); let non_empty_lines: Vec<&str> = stdout.lines().filter(|l| !l.trim().is_empty()).collect();