diff --git a/.config/jp/tools/src/git/hunk.rs b/.config/jp/tools/src/git/hunk.rs index 737326f6..a5240d52 100644 --- a/.config/jp/tools/src/git/hunk.rs +++ b/.config/jp/tools/src/git/hunk.rs @@ -61,6 +61,83 @@ pub fn diff_header(diff_stdout: &str) -> Option<&str> { diff_stdout.split_once("\n@@ ").map(|(header, _)| header) } +/// Parsed counts and start positions extracted from a `@@ ... @@` header. +#[derive(Debug, Clone, Copy)] +pub struct HunkCounts { + pub old_start: usize, + pub old_count: usize, + pub new_count: usize, +} + +/// Parse the `-OS,N` and `+Y,M` ranges from a hunk header line. +/// +/// Accepts both forms with explicit count (`-5,2`) and the implicit +/// single-line form (`-5`, equivalent to `-5,1`). Anything after the second +/// `@@` is ignored. +pub fn parse_hunk_counts(header_line: &str) -> Option { + let mut parts = header_line.split_whitespace(); + parts.next()?; // leading `@@` + let old_part = parts.next()?.strip_prefix('-')?; + let new_part = parts.next()?.strip_prefix('+')?; + + let (old_start, old_count) = parse_range(old_part)?; + let (_new_start, new_count) = parse_range(new_part)?; + + Some(HunkCounts { + old_start, + old_count, + new_count, + }) +} + +fn parse_range(s: &str) -> Option<(usize, usize)> { + let mut it = s.split(','); + let start: usize = it.next()?.parse().ok()?; + let count: usize = it.next().map_or(Some(1), |c| c.parse().ok())?; + Some((start, count)) +} + +/// Rewrite a hunk's header to use a canonical `+Y` line number. +/// +/// Hunks emitted by `git diff-files --unified=0` carry `+Y` values that +/// reflect the cumulative line shift of every preceding unstaged hunk in +/// the same file. When staging only a subset of those hunks, the `+Y` of +/// each selected hunk must be recomputed: `git apply --cached +/// --unidiff-zero` positions changes by `+Y`, so a stale offset places +/// the patch at the wrong line. +/// +/// `cumulative_offset` is the net `(additions - removals)` line change +/// produced by preceding *selected* hunks in the assembled patch, so that +/// the rewritten `+Y` reflects the source state at the point this hunk +/// is applied. +#[allow(clippy::cast_possible_wrap, clippy::cast_sign_loss)] +pub fn rewrite_hunk_y(hunk: &str, cumulative_offset: isize) -> Option<(String, HunkCounts)> { + let (header_line, body) = hunk.split_once('\n')?; + let counts = parse_hunk_counts(header_line)?; + + // Canonical `+Y` assuming the hunk applies at its old position to the + // source state implied by `cumulative_offset`. + let canonical_y = if counts.old_count > 0 { + counts.old_start as isize + } else { + counts.old_start as isize + 1 + }; + let new_y = (canonical_y + cumulative_offset).max(0) as usize; + + // Preserve any trailing context after the second `@@`. + let trailing = header_line + .splitn(4, ' ') + .nth(3) + .map_or(String::new(), |t| format!(" {t}")); + + let new_header = format!( + "@@ -{},{} +{},{} @@{}", + counts.old_start, counts.old_count, new_y, counts.new_count, trailing + ); + + Some((format!("{new_header}\n{body}"), counts)) +} + #[cfg(test)] #[path = "hunk_tests.rs"] mod tests; diff --git a/.config/jp/tools/src/git/stage_patch.rs b/.config/jp/tools/src/git/stage_patch.rs index 2f70a70c..8d39cc1b 100644 --- a/.config/jp/tools/src/git/stage_patch.rs +++ b/.config/jp/tools/src/git/stage_patch.rs @@ -6,7 +6,7 @@ use serde_json::{Map, Value}; use super::{ apply::apply_patch_to_index, - hunk::{diff_header, hunk_id, split_hunks}, + hunk::{diff_header, hunk_id, rewrite_hunk_y, split_hunks}, }; use crate::util::{ OneOrMany, ToolResult, @@ -189,13 +189,30 @@ fn build_file_patch( // Deduplicate requested IDs and emit hunks in file order to satisfy // `git apply`'s monotonic-line requirement for multi-hunk patches. + // + // Each selected hunk's `+Y` is recomputed: the working-tree diff bakes + // in the cumulative line shift of every preceding unstaged hunk, but a + // partial stage skips some of those shifts. We track the net effect of + // preceding *selected* hunks and rewrite each header accordingly so + // `git apply --cached --unidiff-zero` lands the change at the right + // line. let requested: std::collections::HashSet<&str> = requested_ids.iter().map(String::as_str).collect(); - let selected: Vec<&str> = available_hunks - .iter() - .filter(|h| requested.contains(hunk_id(h).as_str())) - .map(String::as_str) - .collect(); + + let mut cumulative_offset: isize = 0; + let mut selected: Vec = vec![]; + for hunk in &available_hunks { + if !requested.contains(hunk_id(hunk).as_str()) { + continue; + } + let (rewritten, counts) = rewrite_hunk_y(hunk, cumulative_offset) + .ok_or_else(|| format!("Malformed hunk header in diff: {hunk}"))?; + #[allow(clippy::cast_possible_wrap)] + { + cumulative_offset += counts.new_count as isize - counts.old_count as isize; + } + selected.push(rewritten); + } // Ensure the patch ends with a newline. Hunks emitted by `split_hunks` // may have lost their trailing newline during splitting (the newline diff --git a/.config/jp/tools/src/git/stage_patch_lines.rs b/.config/jp/tools/src/git/stage_patch_lines.rs index ca2b37d1..91105de8 100644 --- a/.config/jp/tools/src/git/stage_patch_lines.rs +++ b/.config/jp/tools/src/git/stage_patch_lines.rs @@ -56,10 +56,13 @@ enum DiffLineKind { } /// Parsed hunk header coordinates. +/// +/// Only `old_start` is tracked: `new_start` from the original full-diff hunk +/// header is intentionally discarded, since `build_sub_hunk` recomputes the +/// `+Y` line number from scratch (see the offset-correction comment there). #[derive(Debug)] struct HunkHeader { old_start: usize, - new_start: usize, } fn parse_hunk_header(header: &str) -> Result { @@ -68,20 +71,11 @@ fn parse_hunk_header(header: &str) -> Result { let old_part = parts .get(1) .ok_or("Invalid hunk header: missing old range")?; - let new_part = parts - .get(2) - .ok_or("Invalid hunk header: missing new range")?; let old_range = old_part.trim_start_matches('-'); let (old_start, _old_count) = parse_range(old_range)?; - let new_range = new_part.trim_start_matches('+'); - let (new_start, _new_count) = parse_range(new_range)?; - - Ok(HunkHeader { - old_start, - new_start, - }) + Ok(HunkHeader { old_start }) } fn parse_range(range: &str) -> Result<(usize, usize), String> { @@ -239,13 +233,26 @@ fn build_sub_hunk(hunk: &str, selected: &[usize]) -> Result { body.push('\n'); } - // Preserve the offset between old and new positions from the original - // hunk header. This is critical for pure insertions where new_start - // differs from old_start (e.g. @@ -3,0 +4,1 @@ inserts AFTER line 3). - #[allow(clippy::cast_possible_wrap, clippy::cast_sign_loss)] - let new_start = { - let offset = header.new_start as isize - header.old_start as isize; - (old_start as isize + offset).max(0) as usize + // Compute `new_start` from the sub-hunk's own contents, NOT by + // preserving the offset of the original full-diff hunk header. + // + // The header we received comes from `git diff-files --unified=0` against + // the entire working tree, so its `+Y` reflects the cumulative line + // shift of every unstaged hunk earlier in the file. Applying a sub-hunk + // in isolation against a clean index via `git apply --cached + // --unidiff-zero` uses `+Y` to position the change — carrying that + // cumulative offset over would land the patch at the wrong line + // whenever other unstaged hunks (not part of this staging) shifted + // line numbers. + let new_start = if found_removal { + // Replacement or pure removal: new content starts at the same + // position as the first removed line. + old_start + } else { + // Pure insertion: the first new line lands at `old_start + 1`. For + // `old_start = 0` (insertion at file start) this naturally yields + // `1`. + old_start.saturating_add(1) }; let hunk_header = format!("@@ -{old_start},{removals} +{new_start},{additions} @@"); diff --git a/.config/jp/tools/src/git/stage_patch_lines_tests.rs b/.config/jp/tools/src/git/stage_patch_lines_tests.rs index 70482297..61a601ad 100644 --- a/.config/jp/tools/src/git/stage_patch_lines_tests.rs +++ b/.config/jp/tools/src/git/stage_patch_lines_tests.rs @@ -7,28 +7,24 @@ use crate::util::runner::MockProcessRunner; fn parse_hunk_header_simple() { let h = parse_hunk_header("@@ -5 +5 @@").unwrap(); assert_eq!(h.old_start, 5); - assert_eq!(h.new_start, 5); } #[test] fn parse_hunk_header_with_counts() { let h = parse_hunk_header("@@ -5,3 +5,2 @@").unwrap(); assert_eq!(h.old_start, 5); - assert_eq!(h.new_start, 5); } #[test] fn parse_hunk_header_zero_count() { let h = parse_hunk_header("@@ -5,0 +5,3 @@").unwrap(); assert_eq!(h.old_start, 5); - assert_eq!(h.new_start, 5); } #[test] fn parse_hunk_header_different_old_new_start() { let h = parse_hunk_header("@@ -3,0 +4,1 @@").unwrap(); assert_eq!(h.old_start, 3); - assert_eq!(h.new_start, 4); } #[test] diff --git a/.config/jp/tools/tests/git_tests.rs b/.config/jp/tools/tests/git_tests.rs index ab8e6ad9..587ce09d 100644 --- a/.config/jp/tools/tests/git_tests.rs +++ b/.config/jp/tools/tests/git_tests.rs @@ -125,6 +125,16 @@ fn commit_then_modify(root: &Utf8Path, path: &str, original: &str, modified: &st fs::write(root.join(path), modified).unwrap(); } +/// 20-line fixture used by the offset-correction regression tests. +fn lines_1_to_20() -> String { + let mut s = String::new(); + for i in 1..=20 { + use std::fmt::Write as _; + writeln!(s, "line{i}").unwrap(); + } + s +} + /// Extract content-addressed patch IDs from a `git_list_patches` XML response. fn extract_patch_ids(xml: &str) -> Vec { xml.lines() @@ -1341,6 +1351,103 @@ async fn sequential_staging_across_tools() { assert_eq!(staged_content(&root, "mix.rs"), "A\nb\nc\nd\nE\n"); } +#[tokio::test] +async fn stage_patch_addition_with_unrelated_unstaged_removal() { + if !has_git() { + return; + } + + let (_dir, root) = init_repo(); + let original = lines_1_to_20(); + let modified: String = { + let mut lines: Vec = original.lines().map(String::from).collect(); + for _ in 0..9 { + lines.remove(2); + } + lines.insert(9, "INSERTED".to_string()); + lines.join("\n") + "\n" + }; + + fs::write(root.join("sp.rs"), &original).unwrap(); + git(&root, &["add", "sp.rs"]); + git(&root, &["commit", "-m", "init sp"]); + fs::write(root.join("sp.rs"), &modified).unwrap(); + + let ids = patch_ids(&root, "sp.rs").await; + assert_eq!(ids.len(), 2, "expected 2 hunks, got {}", ids.len()); + + // Stage only the addition hunk (the second one). + run_ok( + ctx(&root), + tool_with_answers( + "git_stage_patch", + &json!({"patches": [{"path": "sp.rs", "ids": [&ids[1]]}]}), + &json!({"stage_changes": true}), + ), + ) + .await; + + let expected: String = { + let mut lines: Vec = original.lines().map(String::from).collect(); + lines.insert(18, "INSERTED".to_string()); + lines.join("\n") + "\n" + }; + assert_eq!(staged_content(&root, "sp.rs"), expected); +} + +#[tokio::test] +async fn stage_patch_lines_addition_with_unrelated_unstaged_removal() { + if !has_git() { + return; + } + + let (_dir, root) = init_repo(); + + // Original file: 20 lines. + let original = lines_1_to_20(); + + // Working tree: remove lines 3-11 (9 lines) AND insert "INSERTED" after + // old line 18. Two distant unstaged hunks in a single file. + let modified: String = { + let mut lines: Vec = original.lines().map(String::from).collect(); + for _ in 0..9 { + lines.remove(2); + } + // After removals: indices 0..=10 hold line1, line2, line12..line20. + // Old line 18 is now at index 8. Insert "INSERTED" right after it. + lines.insert(9, "INSERTED".to_string()); + lines.join("\n") + "\n" + }; + + fs::write(root.join("two_hunk.rs"), &original).unwrap(); + git(&root, &["add", "two_hunk.rs"]); + git(&root, &["commit", "-m", "init two_hunk"]); + fs::write(root.join("two_hunk.rs"), &modified).unwrap(); + + // Expect two distinct hunks in the listing. + let ids = patch_ids(&root, "two_hunk.rs").await; + assert_eq!(ids.len(), 2, "expected 2 hunks, got {}", ids.len()); + + // Stage just the insertion hunk's only line. + run_ok( + ctx(&root), + tool( + "git_stage_patch_lines", + &json!({"path": "two_hunk.rs", "patch_id": &ids[1], "lines": [0]}), + ), + ) + .await; + + // The index should match HEAD plus exactly one inserted line after + // old line 18 — the unrelated removal must remain unstaged. + let expected: String = { + let mut lines: Vec = original.lines().map(String::from).collect(); + lines.insert(18, "INSERTED".to_string()); + lines.join("\n") + "\n" + }; + assert_eq!(staged_content(&root, "two_hunk.rs"), expected); +} + #[tokio::test] async fn stage_patch_multiple_files_single_call() { if !has_git() {