diff --git a/Cargo.lock b/Cargo.lock index d50177c148..b7fea112e1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9013,11 +9013,12 @@ checksum = "09e54e57b4c48b40f7aec75635392b12b3421fa26fe8b4332e63138ed278459c" [[package]] name = "imara-diff" -version = "0.1.8" +version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "17d34b7d42178945f775e84bc4c36dde7c1c6cdfea656d3354d009056f2bb3d2" +checksum = "2f01d462f766df78ab820dd06f5eb700233c51f0f4c2e846520eaf4ba6aa5c5c" dependencies = [ "hashbrown 0.15.5", + "memchr", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index e7d5de4f02..9d0b9da182 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -644,7 +644,7 @@ image = { version = "0.25.1", default-features = false, features = [ "tiff", "webp", ] } -imara-diff = "0.1.8" +imara-diff = "0.2.0" indexmap = { version = "2.7.0", features = ["serde"] } indoc = "2" inventory = "0.3.19" diff --git a/crates/buffer_diff/src/buffer_diff.rs b/crates/buffer_diff/src/buffer_diff.rs index 5cba96c5a9..dad94c4b01 100644 --- a/crates/buffer_diff/src/buffer_diff.rs +++ b/crates/buffer_diff/src/buffer_diff.rs @@ -1,5 +1,5 @@ use gpui::{App, AppContext as _, Context, Entity, EventEmitter, Task}; -use imara_diff::{Algorithm, Sink, intern::InternedInput, sources::lines_with_terminator}; +use imara_diff::{Algorithm, Diff, InternedInput, sources::lines}; use language::{ Capability, DiffOptions, Language, LanguageName, LanguageRegistry, language_settings::LanguageSettings, word_diff_ranges, @@ -1192,13 +1192,20 @@ fn compute_hunks( return tree; } - let input = InternedInput::new( - lines_with_terminator(diff_base.as_ref()), - lines_with_terminator(buffer_text.as_str()), - ); - let sink = HunkSink::new(&diff_base, &diff_base_rope, buffer, diff_options.as_ref()); - let hunks = imara_diff::diff(Algorithm::Histogram, &input, sink); - for hunk in hunks { + let input = InternedInput::new(lines(diff_base.as_ref()), lines(buffer_text.as_str())); + let mut diff = Diff::compute(Algorithm::Histogram, &input); + // Canonicalize the placement of ambiguous hunks (git's slider/indent + // heuristic). Without this, diffs of the same buffer against different + // base texts (e.g. HEAD vs index) can anchor the same logical change at + // different rows, and code that correlates hunks across those diffs + // misbehaves: hunks render as staged when they aren't, and staging or + // unstaging them corrupts the index. + diff.postprocess_lines(&input); + let mut sink = HunkSink::new(&diff_base, &diff_base_rope, buffer, diff_options.as_ref()); + for hunk in diff.hunks() { + sink.process_change(hunk.before, hunk.after); + } + for hunk in sink.finish() { tree.push(hunk, buffer); } } else { @@ -1244,7 +1251,7 @@ impl<'a> HunkSink<'a> { fn compute_line_offsets(text: &str) -> Vec { let mut offsets = vec![0]; let mut offset = 0; - for line in lines_with_terminator(text) { + for line in lines(text) { offset += line.len(); offsets.push(offset); } @@ -1252,9 +1259,7 @@ impl<'a> HunkSink<'a> { } } -impl Sink for HunkSink<'_> { - type Out = Vec; - +impl HunkSink<'_> { fn process_change(&mut self, before: Range, after: Range) { let old_start = before.start as usize; let old_end = before.end as usize; @@ -1321,7 +1326,7 @@ impl Sink for HunkSink<'_> { }); } - fn finish(self) -> Self::Out { + fn finish(self) -> Vec { self.hunks } } diff --git a/crates/edit_prediction_metrics/src/reversal.rs b/crates/edit_prediction_metrics/src/reversal.rs index 80fdc40d53..00929a2744 100644 --- a/crates/edit_prediction_metrics/src/reversal.rs +++ b/crates/edit_prediction_metrics/src/reversal.rs @@ -4,21 +4,14 @@ use std::path::Path; use std::sync::Arc; use crate::tokenize::tokenize; -use imara_diff::{ - Algorithm, diff, - intern::{InternedInput, Token}, - sources::lines_with_terminator, -}; +use imara_diff::{Algorithm, Diff, InternedInput, Token, sources::lines}; use zeta_prompt::udiff::apply_diff_to_string; fn text_diff(old_text: &str, new_text: &str) -> Vec<(Range, Arc)> { let empty: Arc = Arc::default(); let mut edits = Vec::new(); let mut hunk_input = InternedInput::default(); - let input = InternedInput::new( - lines_with_terminator(old_text), - lines_with_terminator(new_text), - ); + let input = InternedInput::new(lines(old_text), lines(new_text)); diff_internal(&input, &mut |old_byte_range, new_byte_range, @@ -104,35 +97,34 @@ fn diff_internal( let mut old_token_ix = 0; let mut new_token_ix = 0; - diff( - Algorithm::Histogram, - input, - |old_tokens: Range, new_tokens: Range| { - old_offset += token_len( - input, - &input.before[old_token_ix as usize..old_tokens.start as usize], - ); - new_offset += token_len( - input, - &input.after[new_token_ix as usize..new_tokens.start as usize], - ); - let old_len = token_len( - input, - &input.before[old_tokens.start as usize..old_tokens.end as usize], - ); - let new_len = token_len( - input, - &input.after[new_tokens.start as usize..new_tokens.end as usize], - ); - let old_byte_range = old_offset..old_offset + old_len; - let new_byte_range = new_offset..new_offset + new_len; - old_token_ix = old_tokens.end; - new_token_ix = new_tokens.end; - old_offset = old_byte_range.end; - new_offset = new_byte_range.end; - on_change(old_byte_range, new_byte_range, old_tokens, new_tokens); - }, - ); + let diff = Diff::compute(Algorithm::Histogram, input); + for hunk in diff.hunks() { + let old_tokens = hunk.before; + let new_tokens = hunk.after; + old_offset += token_len( + input, + &input.before[old_token_ix as usize..old_tokens.start as usize], + ); + new_offset += token_len( + input, + &input.after[new_token_ix as usize..new_tokens.start as usize], + ); + let old_len = token_len( + input, + &input.before[old_tokens.start as usize..old_tokens.end as usize], + ); + let new_len = token_len( + input, + &input.after[new_tokens.start as usize..new_tokens.end as usize], + ); + let old_byte_range = old_offset..old_offset + old_len; + let new_byte_range = new_offset..new_offset + new_len; + old_token_ix = old_tokens.end; + new_token_ix = new_tokens.end; + old_offset = old_byte_range.end; + new_offset = new_byte_range.end; + on_change(old_byte_range, new_byte_range, old_tokens, new_tokens); + } } fn tokenize_chars(text: &str) -> impl Iterator { diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 8f5cda4b65..a5045a07f6 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -12791,9 +12791,9 @@ async fn test_fold_function_bodies(cx: &mut TestAppContext) { fn b() { c(); } - - - - // this is another uncommitted comment + - // this is another uncommitted comment + - fn d() { // e // f diff --git a/crates/language/src/text_diff.rs b/crates/language/src/text_diff.rs index 69d241cabd..4e8239672b 100644 --- a/crates/language/src/text_diff.rs +++ b/crates/language/src/text_diff.rs @@ -1,10 +1,6 @@ use crate::{CharClassifier, CharKind, CharScopeContext, LanguageScope}; use anyhow::{Context, anyhow}; -use imara_diff::{ - Algorithm, Sink, diff, - intern::{InternedInput, Interner, Token}, - sources::lines_with_terminator, -}; +use imara_diff::{Algorithm, Diff, InternedInput, Interner, Token, sources::lines}; use std::{fmt::Write, iter, ops::Range, sync::Arc}; const MAX_WORD_DIFF_LEN: usize = 512; @@ -36,12 +32,17 @@ pub fn unified_diff_with_context( new_start_line: u32, context_lines: u32, ) -> String { - let input = InternedInput::new(old_text, new_text); - diff( - Algorithm::Histogram, - &input, - OffsetUnifiedDiffBuilder::new(&input, old_start_line, new_start_line, context_lines), - ) + // The builder appends its own line terminators, so tokenize without them. + let mut input = InternedInput::default(); + input.update_before(old_text.lines()); + input.update_after(new_text.lines()); + let diff = Diff::compute(Algorithm::Histogram, &input); + let mut builder = + OffsetUnifiedDiffBuilder::new(&input, old_start_line, new_start_line, context_lines); + for hunk in diff.hunks() { + builder.process_change(hunk.before, hunk.after); + } + builder.finish() } /// A unified diff builder that applies line number offsets to hunk headers. @@ -126,9 +127,7 @@ impl<'a> OffsetUnifiedDiffBuilder<'a> { } } -impl Sink for OffsetUnifiedDiffBuilder<'_> { - type Out = String; - +impl OffsetUnifiedDiffBuilder<'_> { fn process_change(&mut self, before: Range, after: Range) { if before.start - self.pos > self.context_lines * 2 { self.flush(); @@ -148,7 +147,7 @@ impl Sink for OffsetUnifiedDiffBuilder<'_> { self.print_tokens(&self.after[after.start as usize..after.end as usize], '+'); } - fn finish(mut self) -> Self::Out { + fn finish(mut self) -> String { self.flush(); self.dst } @@ -158,10 +157,7 @@ impl Sink for OffsetUnifiedDiffBuilder<'_> { /// ranges. pub fn line_diff(old_text: &str, new_text: &str) -> Vec<(Range, Range)> { let mut edits = Vec::new(); - let input = InternedInput::new( - lines_with_terminator(old_text), - lines_with_terminator(new_text), - ); + let input = InternedInput::new(lines(old_text), lines(new_text)); diff_internal(&input, &mut |_, _, old_rows, new_rows| { edits.push((old_rows, new_rows)); }); @@ -264,10 +260,7 @@ pub fn text_diff_with_options( let empty: Arc = Arc::default(); let mut edits = Vec::new(); let mut hunk_input = InternedInput::default(); - let input = InternedInput::new( - lines_with_terminator(old_text), - lines_with_terminator(new_text), - ); + let input = InternedInput::new(lines(old_text), lines(new_text)); diff_internal(&input, &mut |old_byte_range, new_byte_range, old_rows, @@ -349,35 +342,34 @@ fn diff_internal( let mut new_offset = 0; let mut old_token_ix = 0; let mut new_token_ix = 0; - diff( - Algorithm::Histogram, - input, - |old_tokens: Range, new_tokens: Range| { - old_offset += token_len( - input, - &input.before[old_token_ix as usize..old_tokens.start as usize], - ); - new_offset += token_len( - input, - &input.after[new_token_ix as usize..new_tokens.start as usize], - ); - let old_len = token_len( - input, - &input.before[old_tokens.start as usize..old_tokens.end as usize], - ); - let new_len = token_len( - input, - &input.after[new_tokens.start as usize..new_tokens.end as usize], - ); - let old_byte_range = old_offset..old_offset + old_len; - let new_byte_range = new_offset..new_offset + new_len; - old_token_ix = old_tokens.end; - new_token_ix = new_tokens.end; - old_offset = old_byte_range.end; - new_offset = new_byte_range.end; - on_change(old_byte_range, new_byte_range, old_tokens, new_tokens); - }, - ); + let diff = Diff::compute(Algorithm::Histogram, input); + for hunk in diff.hunks() { + let old_tokens = hunk.before; + let new_tokens = hunk.after; + old_offset += token_len( + input, + &input.before[old_token_ix as usize..old_tokens.start as usize], + ); + new_offset += token_len( + input, + &input.after[new_token_ix as usize..new_tokens.start as usize], + ); + let old_len = token_len( + input, + &input.before[old_tokens.start as usize..old_tokens.end as usize], + ); + let new_len = token_len( + input, + &input.after[new_tokens.start as usize..new_tokens.end as usize], + ); + let old_byte_range = old_offset..old_offset + old_len; + let new_byte_range = new_offset..new_offset + new_len; + old_token_ix = old_tokens.end; + new_token_ix = new_tokens.end; + old_offset = old_byte_range.end; + new_offset = new_byte_range.end; + on_change(old_byte_range, new_byte_range, old_tokens, new_tokens); + } } fn tokenize_chars(text: &str) -> impl Iterator { diff --git a/crates/project/tests/integration/project_tests.rs b/crates/project/tests/integration/project_tests.rs index bf90db372e..420af4aaed 100644 --- a/crates/project/tests/integration/project_tests.rs +++ b/crates/project/tests/integration/project_tests.rs @@ -16081,3 +16081,201 @@ mod disable_ai_settings_tests { }); } } + +/// Regression test for https://github.com/zed-industries/zed/issues/60424. +/// +/// The committed text contains repeated `end\n\n` line runs, so the deletion +/// hunks have ambiguous placements. Before hunk placement was canonicalized, +/// the uncommitted diff (HEAD vs worktree) and the recomputed unstaged diff +/// (index vs worktree) could anchor the same logical deletion at different +/// rows after a partial stage. The mismatched hunk then rendered as staged +/// while git still had it unstaged, and unstaging it inserted a duplicate +/// copy of its contents into the index on every click. +#[gpui::test] +async fn test_staging_hunks_with_ambiguous_placement(cx: &mut gpui::TestAppContext) { + init_test(cx); + + let committed_contents = concat!( + "function is_empty(str)\n", + " assert(type(str) == string)\n", + " return str == nil or str == \"\"\n", + "end\n", + "\n", + "function string:starts_with(str)\n", + " return self:sub(1, #str) == str\n", + "end\n", + "\n", + "function string:ends_with(str)\n", + " return self:sub(-#str) == str\n", + "end\n", + "\n", + "function table:append(other)\n", + " for _, val in ipairs(other) do table.insert(self, val) end\n", + "end\n", + "\n", + "function table:contains(elem)\n", + " assert(type(self) == \"table\")\n", + "\n", + " for _, val in ipairs(self) do\n", + " if val == elem then return true end\n", + " end\n", + " return false\n", + "end\n", + "\n", + "function table:keys()\n", + " local keys = {}\n", + " for key, _ in pairs(self) do\n", + " table.insert(keys, key)\n", + " end\n", + " return keys\n", + "end\n", + "\n", + "function has_class(el, class) return el.classes[0] == class end\n", + ) + .to_string(); + let file_contents = concat!( + "function table:append(other)\n", + " for _, val in ipairs(other) do table.insert(self, val) end\n", + "end\n", + "\n", + "function table:keys()\n", + " local keys = {}\n", + " for key, _ in pairs(self) do\n", + " table.insert(keys, key)\n", + " end\n", + " return keys\n", + "end\n", + "\n", + "function has_class(el, class) return el.classes:includes(class) end\n", + ) + .to_string(); + + let fs = FakeFs::new(cx.background_executor.clone()); + fs.insert_tree( + "/dir", + json!({ + ".git": {}, + "test.txt": file_contents.clone() + }), + ) + .await; + fs.set_head_and_index_for_repo( + path!("/dir/.git").as_ref(), + &[("test.txt", committed_contents.clone())], + ); + + let project = Project::test(fs.clone(), ["/dir".as_ref()], cx).await; + let buffer = project + .update(cx, |project, cx| { + project.open_local_buffer("/dir/test.txt", cx) + }) + .await + .unwrap(); + let snapshot = buffer.read_with(cx, |buffer, _| buffer.snapshot()); + let uncommitted_diff = project + .update(cx, |project, cx| { + project.open_uncommitted_diff(buffer.clone(), cx) + }) + .await + .unwrap(); + + fn hunk_states( + uncommitted_diff: &Entity, + snapshot: &language::BufferSnapshot, + cx: &mut gpui::TestAppContext, + ) -> Vec<(Range, DiffHunkSecondaryStatus)> { + uncommitted_diff.read_with(cx, |diff, cx| { + diff.snapshot(cx) + .hunks(&snapshot.text) + .map(|hunk| (hunk.range.clone(), hunk.secondary_status)) + .collect() + }) + } + + fn index_text(fs: &FakeFs) -> String { + fs.with_git_state(path!("/dir/.git").as_ref(), false, |state| { + state + .index_contents + .get(&repo_path("test.txt")) + .cloned() + .expect("file is present in the index") + }) + .unwrap() + } + + use DiffHunkSecondaryStatus::*; + let row = |start: u32, end: u32| Point::new(start, 0)..Point::new(end, 0); + + // Two deletions and one modification, all unstaged. + assert_eq!( + hunk_states(&uncommitted_diff, &snapshot, cx), + vec![ + (row(0, 0), HasSecondaryHunk), + (row(4, 4), HasSecondaryHunk), + (row(12, 13), HasSecondaryHunk), + ] + ); + + // Stage the first deletion (the three string functions). + let hunks = uncommitted_diff.read_with(cx, |diff, cx| { + diff.snapshot(cx).hunks(&snapshot.text).collect::>() + }); + project + .update(cx, |project, cx| { + let unstaged_diff = uncommitted_diff.read(cx).secondary_diff().unwrap(); + project.stage_hunks( + buffer.clone(), + unstaged_diff, + vec![hunks[0].buffer_range.clone()], + cx, + ) + }) + .unwrap(); + cx.run_until_parked(); + + // Only the first hunk is staged; the other hunks must not be affected, + // even though the unstaged diff was recomputed against a new index text + // in which the remaining deletion has an ambiguous placement. + assert_eq!( + hunk_states(&uncommitted_diff, &snapshot, cx), + vec![ + (row(0, 0), NoSecondaryHunk), + (row(4, 4), HasSecondaryHunk), + (row(12, 13), HasSecondaryHunk), + ] + ); + let table_contains_offset = committed_contents + .find("function table:append") + .expect("committed text contains table:append"); + let expected_index = &committed_contents[table_contains_offset..]; + assert_eq!(index_text(&fs), expected_index); + + // Unstaging the second deletion is a no-op, since it isn't staged. Before + // the fix, each such request inserted another copy of the deleted content + // into the index. + for _ in 0..3 { + let hunks = uncommitted_diff.read_with(cx, |diff, cx| { + diff.snapshot(cx).hunks(&snapshot.text).collect::>() + }); + project + .update(cx, |project, cx| { + project.unstage_uncommitted_hunks( + buffer.clone(), + uncommitted_diff.clone(), + vec![hunks[1].buffer_range.clone()], + cx, + ) + }) + .unwrap(); + cx.run_until_parked(); + } + assert_eq!(index_text(&fs), expected_index); + assert_eq!( + hunk_states(&uncommitted_diff, &snapshot, cx), + vec![ + (row(0, 0), NoSecondaryHunk), + (row(4, 4), HasSecondaryHunk), + (row(12, 13), HasSecondaryHunk), + ] + ); +} diff --git a/crates/zeta_prompt/src/udiff.rs b/crates/zeta_prompt/src/udiff.rs index 7b4f01e9ad..3df1109bf2 100644 --- a/crates/zeta_prompt/src/udiff.rs +++ b/crates/zeta_prompt/src/udiff.rs @@ -6,10 +6,7 @@ use std::{ }; use anyhow::{Context as _, Result, anyhow}; -use imara_diff::{ - Algorithm, Sink, diff, - intern::{InternedInput, Interner, Token}, -}; +use imara_diff::{Algorithm, Diff, InternedInput, Interner, Token}; pub fn strip_diff_path_prefix<'a>(diff: &'a str, prefix: &str) -> Cow<'a, str> { if prefix.is_empty() { @@ -216,12 +213,17 @@ pub fn unified_diff_with_context( new_start_line: u32, context_lines: u32, ) -> String { - let input = InternedInput::new(old_text, new_text); - diff( - Algorithm::Histogram, - &input, - OffsetUnifiedDiffBuilder::new(&input, old_start_line, new_start_line, context_lines), - ) + // The builder appends its own line terminators, so tokenize without them. + let mut input = InternedInput::default(); + input.update_before(old_text.lines()); + input.update_after(new_text.lines()); + let diff = Diff::compute(Algorithm::Histogram, &input); + let mut builder = + OffsetUnifiedDiffBuilder::new(&input, old_start_line, new_start_line, context_lines); + for hunk in diff.hunks() { + builder.process_change(hunk.before, hunk.after); + } + builder.finish() } struct OffsetUnifiedDiffBuilder<'a> { @@ -302,9 +304,7 @@ impl<'a> OffsetUnifiedDiffBuilder<'a> { } } -impl Sink for OffsetUnifiedDiffBuilder<'_> { - type Out = String; - +impl OffsetUnifiedDiffBuilder<'_> { fn process_change(&mut self, before: Range, after: Range) { if before.start - self.pos > self.context_lines * 2 { self.flush(); @@ -325,7 +325,7 @@ impl Sink for OffsetUnifiedDiffBuilder<'_> { self.print_tokens(&self.after[after.start as usize..after.end as usize], '+'); } - fn finish(mut self) -> Self::Out { + fn finish(mut self) -> String { self.flush(); self.dst }