History

add optional detailed report to slider test

This is the first commit in my quest to bring `gix-diff` closer to `git diff`. Before making any changes, I want to first add proper diagnostics, in order to be able to split the larger problem of "diffs don't match" into smaller problems of "diffs don't match for this specific reason" and "there's these specific reasons that can cause a mismatch". The individual reasons can then be tackled independently.

Christoph Rüßler committed 2 months ago

Commit 7192d9a1 parent fbebed73 files · +371 −42Browse files

Unified · Split

3 files
  1. gix-diff/tests/README.md +6 −1
  2. gix-diff/tests/diff/blob/slider.rs +359 −37
  3. tests/it/src/commands/create_diff_cases.rs +6 −4
gix-diff/tests/README.md+6−1
@@ -23,4 +23,9 @@ cargo run --package internal-tools -- \ · 22 unmodified lines
23 --destination-dir gix-diff/tests/fixtures/23 --destination-dir gix-diff/tests/fixtures/
24```24```
25 25
26Finally, run `cargo test -p gix-diff sliders -- --nocapture` to execute the actual tests to compare.26Finally, run `cargo test -p gix-diff slider::baseline -- --nocapture` to execute the actual tests to compare.
27
28The generated fixture includes Git baselines with and without the indent heuristic. The regular test compares
29`gix-diff` with slider heuristics to the primary Git baselines, while the additional `*.no-indent.baseline`
30files are used for diagnostics. To print a coarse mismatch report instead of failing on the first mismatch, run
31`GIX_DIFF_SLIDER_REPORT=1 cargo test -p gix-diff slider::baseline -- --nocapture`.
gix-diff/tests/diff/blob/slider.rs+359−37
@@ -1,16 +1,18 @@
1use std::{collections::BTreeMap, hash::Hash, path::Path};
2
3use gix_diff::blob::{self, Algorithm, InternedInput, diff_with_slider_heuristics};
1use gix_object::bstr::ByteSlice;4use gix_object::bstr::ByteSlice;
2use pretty_assertions::StrComparison;5use pretty_assertions::StrComparison;
3 6
4#[test]7#[test]
5fn baseline() -> gix_testtools::Result {8fn baseline() -> gix_testtools::Result {
6 use gix_diff::blob::{self, Algorithm, InternedInput, diff_with_slider_heuristics};
7
8 let worktree_path = crate::scripted_fixture_read_only("make_diff_for_sliders_repo.sh")?;9 let worktree_path = crate::scripted_fixture_read_only("make_diff_for_sliders_repo.sh")?;
9 let asset_dir = worktree_path.join("assets");10 let asset_dir = worktree_path.join("assets");
10 11
11 let dir = std::fs::read_dir(&worktree_path)?;12 let dir = std::fs::read_dir(&worktree_path)?;
13 let should_print_extended_report = std::env::var_os("GIX_DIFF_SLIDER_REPORT").is_some();
12 14
13 let mut diffs = Vec::new();15 let mut cases = Vec::new();
14 16
15 for entry in dir {17 for entry in dir {
16 let entry = entry?;18 let entry = entry?;
@@ -28,44 +30,130 @@ fn baseline() -> gix_testtools::Result { · 11 unmodified lines
28 old_data.to_str().expect("BUG: we don't have non-ascii here"),30 old_data.to_str().expect("BUG: we don't have non-ascii here"),
29 new_data.to_str().expect("BUG: we don't have non-ascii here"),31 new_data.to_str().expect("BUG: we don't have non-ascii here"),
30 );32 );
31 let diff = diff_with_slider_heuristics(
32 match algorithm {
33 Algorithm::Myers => Algorithm::Myers,
34 Algorithm::Histogram => Algorithm::Histogram,
35 Algorithm::MyersMinimal => Algorithm::MyersMinimal,
36 },
37 &input,
38 );
39 33
40 let actual = blob::UnifiedDiff::new(34 let gix_no_postprocess = {
41 &diff,35 let diff = blob::Diff::compute(algorithm, &input);
42 &input,36 render_unidiff(&diff, &input)?
43 blob::unified_diff::ConsumeBinaryHunk::new(String::new(), "\n"),37 };
44 blob::unified_diff::ContextSize::symmetrical(3),38
45 )39 let gix_postprocess_no_heuristic = {
46 .consume()?;40 let mut diff = blob::Diff::compute(algorithm, &input);
41 diff.postprocess_no_heuristic(&input);
42 render_unidiff(&diff, &input)?
43 };
44
45 let gix_postprocess_slider_heuristics = {
46 let diff = diff_with_slider_heuristics(algorithm, &input);
47 render_unidiff(&diff, &input)?
48 };
47 49
48 let baseline_path = worktree_path.join(&file_name);50 let baseline_path = worktree_path.join(&file_name);
49 let baseline = std::fs::read(baseline_path)?;51 let baseline = std::fs::read(baseline_path)?;
50 let baseline = crate::blob::skip_header_and_fold_to_unidiff(&baseline);52 let baseline = crate::blob::skip_header_and_fold_to_unidiff(&baseline);
53 let git_no_indent_heuristic = if should_print_extended_report {
54 read_no_indent_baseline(&worktree_path, &file_name)?
55 } else {
56 None
57 };
51 58
52 let actual_matches_baseline = actual == baseline;59 cases.push(Case {
53 diffs.push((actual, baseline, actual_matches_baseline, file_name));60 file_name,
61 algorithm,
62 git_postprocess_indent_heuristic: baseline,
63 git_no_indent_heuristic,
64 gix_no_postprocess,
65 gix_postprocess_no_heuristic,
66 gix_postprocess_slider_heuristics,
67 });
54 }68 }
55 69
56 if diffs.is_empty() {70 if cases.is_empty() {
57 eprintln!("Slider baseline isn't setup - look at ./gix-diff/tests/README.md for instructions");71 eprintln!("Slider baseline isn't set up – see ./gix-diff/tests/README.md for instructions");
72 }
73
74 if should_print_extended_report {
75 print_extended_report(&cases);
76 } else {
77 assert_diffs(&cases);
58 }78 }
59 79
60 assert_diffs(&diffs);
61 Ok(())80 Ok(())
62}81}
63 82
64fn assert_diffs(diffs: &[(String, String, bool, String)]) {83struct Case {
65 let total_diffs = diffs.len();84 file_name: String,
66 let matching_diffs = diffs85 algorithm: Algorithm,
86 git_postprocess_indent_heuristic: String,
87 git_no_indent_heuristic: Option<String>,
88 gix_no_postprocess: String,
89 gix_postprocess_no_heuristic: String,
90 gix_postprocess_slider_heuristics: String,
91}
92
93#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
94enum Classification {
95 Exact,
96 NoPostprocessMatchesGitDefault,
97 PostprocessNoHeuristicMatchesGitDefault,
98 PostprocessNoHeuristicMatchesGitNoIndent,
99 LikelySliderOnlyMismatch,
100 OtherMismatch,
101}
102
103impl Case {
104 fn classify(&self) -> Classification {
105 if self.gix_postprocess_slider_heuristics == self.git_postprocess_indent_heuristic {
106 Classification::Exact
107 } else if self.gix_no_postprocess == self.git_postprocess_indent_heuristic {
108 Classification::NoPostprocessMatchesGitDefault
109 } else if self.gix_postprocess_no_heuristic == self.git_postprocess_indent_heuristic {
110 Classification::PostprocessNoHeuristicMatchesGitDefault
111 } else if self
112 .git_no_indent_heuristic
113 .as_ref()
114 .is_some_and(|git_no_indent_heuristic| &self.gix_postprocess_no_heuristic == git_no_indent_heuristic)
115 {
116 Classification::PostprocessNoHeuristicMatchesGitNoIndent
117 } else if has_same_changed_line_sequence(
118 &self.gix_postprocess_slider_heuristics,
119 &self.git_postprocess_indent_heuristic,
120 ) {
121 Classification::LikelySliderOnlyMismatch
122 } else {
123 Classification::OtherMismatch
124 }
125 }
126}
127
128fn render_unidiff<T: AsRef<[u8]> + Hash + Eq>(diff: &blob::Diff, input: &InternedInput<T>) -> std::io::Result<String> {
129 blob::UnifiedDiff::new(
130 diff,
131 input,
132 blob::unified_diff::ConsumeBinaryHunk::new(String::new(), "\n"),
133 blob::unified_diff::ContextSize::symmetrical(3),
134 )
135 .consume()
136}
137
138fn read_no_indent_baseline(worktree_path: &Path, primary_file_name: &str) -> std::io::Result<Option<String>> {
139 let Some(stem) = primary_file_name.strip_suffix(".baseline") else {
140 return Ok(None);
141 };
142
143 let path = worktree_path.join(format!("{stem}.no-indent.baseline"));
144 let baseline = match std::fs::read(path) {
145 Ok(baseline) => baseline,
146 Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(None),
147 Err(err) => return Err(err),
148 };
149 Ok(Some(super::skip_header_and_fold_to_unidiff(&baseline)))
150}
151
152fn assert_diffs(cases: &[Case]) {
153 let total_diffs = cases.len();
154 let matching_diffs = cases
67 .iter()155 .iter()
68 .filter(|(_, _, actual_matches_baseline, _)| *actual_matches_baseline)156 .filter(|case| case.gix_postprocess_slider_heuristics == case.git_postprocess_indent_heuristic)
69 .count();157 .count();
70 158
71 assert_eq!(159 assert_eq!(
@@ -76,20 +164,253 @@ fn assert_diffs(diffs: &[(String, String, bool, String)]) { · 4 unmodified lines
76 total_diffs,164 total_diffs,
77 ((matching_diffs as f32) / (total_diffs as f32) * 100.0),165 ((matching_diffs as f32) / (total_diffs as f32) * 100.0),
78 {166 {
79 let first_non_matching_diff = diffs167 let first_non_matching_diff = cases
80 .iter()168 .iter()
81 .find(|(_, _, actual_matches_baseline, _)| !actual_matches_baseline)169 .find(|case| case.gix_postprocess_slider_heuristics != case.git_postprocess_indent_heuristic)
82 .expect("at least one non-matching diff to be there");170 .expect("at least one non-matching diff to be there");
83 171
84 format!(172 format!(
85 "affected baseline: `{}`\n\n{}",173 "affected baseline: `{}`\n\n{}",
86 first_non_matching_diff.3,174 first_non_matching_diff.file_name,
87 StrComparison::new(&first_non_matching_diff.0, &first_non_matching_diff.1)175 StrComparison::new(
176 &first_non_matching_diff.gix_postprocess_slider_heuristics,
177 &first_non_matching_diff.git_postprocess_indent_heuristic
178 )
88 )179 )
89 }180 }
90 );181 );
91}182}
92 183
184// This function intentionally only is capable to detect a subset of diffs that differ by slider
185// placement only. It can only detect sliders that have added/deleted lines in the exact same
186// order. I plan on adding more accurate classification in the future, though.
187fn has_same_changed_line_sequence(lhs: &str, rhs: &str) -> bool {
188 fn changed_lines(diff: &str) -> Vec<&str> {
189 diff.lines()
190 .filter(|line| line.starts_with('+') || line.starts_with('-'))
191 .collect()
192 }
193
194 changed_lines(lhs) == changed_lines(rhs)
195}
196
197fn slider_detail(gix: &str, git: &str) -> String {
198 let gix_hunks = parse_hunks(gix);
199 let git_hunks = parse_hunks(git);
200
201 if gix_hunks.is_empty() || git_hunks.is_empty() {
202 return format!(
203 "diagnostic-error/unparsed-or-empty-hunks/gix-{}/git-{}",
204 gix_hunks.len(),
205 git_hunks.len()
206 );
207 }
208
209 if gix_hunks.len() != git_hunks.len() {
210 return format!(
211 "multi-hunk/different-hunk-count/gix-{}/git-{}",
212 gix_hunks.len(),
213 git_hunks.len()
214 );
215 }
216
217 // At this point, we know that `gix_hunks.len() == git_hunks.len()`.
218 if gix_hunks.len() > 1 {
219 let directions: Vec<_> = gix_hunks
220 .iter()
221 .zip(&git_hunks)
222 .map(|(gix_hunk, git_hunk)| movement_direction(gix_hunk, git_hunk))
223 .collect();
224 let same_direction = directions
225 .first()
226 .is_some_and(|first| directions.iter().all(|direction| direction == first));
227 let direction = if same_direction {
228 "same-direction"
229 } else {
230 "mixed-direction"
231 };
232
233 return format!("multi-hunk/same-count/{direction}");
234 }
235
236 let gix_hunk = &gix_hunks[0];
237 let git_hunk = &git_hunks[0];
238
239 let gix_is_empty = gix_hunk.removed == 0 && gix_hunk.added == 0;
240 let git_is_empty = git_hunk.removed == 0 && git_hunk.added == 0;
241
242 match (gix_is_empty, git_is_empty) {
243 (true, true) => return "diagnostic-error/empty-hunks/both".to_owned(),
244 (true, false) => return "diagnostic-error/empty-hunks/gix".to_owned(),
245 (false, true) => return "diagnostic-error/empty-hunks/git".to_owned(),
246 (false, false) => {}
247 }
248
249 let kind = match (gix_hunk.removed > 0, gix_hunk.added > 0) {
250 (false, true) => "pure-insertion",
251 (true, false) => "pure-deletion",
252 (true, true) => "modification",
253 (false, false) => unreachable!("BUG: we verified that both `added` and `removed` are non-zero"),
254 };
255 let direction = movement_direction(gix_hunk, git_hunk);
256 let distance = movement_distance_bucket(gix_hunk, git_hunk);
257
258 format!("single-hunk/{kind}/{direction}/{distance}")
259}
260
261struct ParsedHunk {
262 before_start: u32,
263 after_start: u32,
264 removed: usize,
265 added: usize,
266}
267
268fn parse_hunks(diff: &str) -> Vec<ParsedHunk> {
269 let mut hunks = Vec::<ParsedHunk>::new();
270
271 for line in diff.lines() {
272 if line.starts_with("@@ ") {
273 if let Some((before_start, after_start)) = parse_hunk_header(line) {
274 hunks.push(ParsedHunk {
275 before_start,
276 after_start,
277 removed: 0,
278 added: 0,
279 });
280 }
281 } else if let Some(hunk) = hunks.last_mut() {
282 if line.starts_with('-') {
283 hunk.removed += 1;
284 } else if line.starts_with('+') {
285 hunk.added += 1;
286 }
287 }
288 }
289
290 hunks
291}
292
293fn parse_hunk_header(line: &str) -> Option<(u32, u32)> {
294 // Supported forms include both `@@ -10,3 +10,4 @@` and `@@ -10 +10 @@`.
295 let line = line.strip_prefix("@@ ")?;
296 let (header, _section) = line.split_once(" @@")?;
297
298 let mut parts = header.split_ascii_whitespace();
299 let before = parts.next()?.strip_prefix('-')?;
300 let after = parts.next()?.strip_prefix('+')?;
301
302 Some((parse_range_start(before)?, parse_range_start(after)?))
303}
304
305fn parse_range_start(range: &str) -> Option<u32> {
306 let start = range.split_once(',').map_or(range, |(start, _len)| start);
307
308 start.parse().ok()
309}
310
311fn movement_direction(gix_hunk: &ParsedHunk, git_hunk: &ParsedHunk) -> &'static str {
312 match gix_hunk.before_start.cmp(&git_hunk.before_start) {
313 std::cmp::Ordering::Less => "gix-before-start-earlier",
314 std::cmp::Ordering::Greater => "gix-before-start-later",
315 std::cmp::Ordering::Equal => match gix_hunk.after_start.cmp(&git_hunk.after_start) {
316 std::cmp::Ordering::Less => "same-before-start/gix-after-start-earlier",
317 std::cmp::Ordering::Greater => "same-before-start/gix-after-start-later",
318 std::cmp::Ordering::Equal => "same-before-start",
319 },
320 }
321}
322
323fn movement_distance_bucket(gix_hunk: &ParsedHunk, git_hunk: &ParsedHunk) -> &'static str {
324 let before_delta = gix_hunk.before_start.abs_diff(git_hunk.before_start);
325 let after_delta = gix_hunk.after_start.abs_diff(git_hunk.after_start);
326
327 match before_delta.max(after_delta) {
328 0 => "0",
329 1 => "1",
330 2..=5 => "2-5",
331 6..=20 => "6-20",
332 _ => ">20",
333 }
334}
335
336fn print_extended_report(cases: &[Case]) {
337 let mut counts = BTreeMap::<Classification, usize>::new();
338 let mut likely_slider_only_counts = BTreeMap::<String, usize>::new();
339
340 for case in cases {
341 let classification = case.classify();
342 *counts.entry(classification).or_default() += 1;
343
344 if classification == Classification::LikelySliderOnlyMismatch {
345 *likely_slider_only_counts
346 .entry(slider_detail(
347 &case.gix_postprocess_slider_heuristics,
348 &case.git_postprocess_indent_heuristic,
349 ))
350 .or_default() += 1;
351 }
352 }
353
354 eprintln!("slider report");
355 eprintln!();
356
357 if cases.is_empty() {
358 eprintln!("no cases to report");
359
360 return;
361 }
362
363 eprintln!("total cases: {}", cases.len());
364 eprintln!(
365 "git no-indent baselines: {}",
366 cases
367 .iter()
368 .filter(|case| case.git_no_indent_heuristic.is_some())
369 .count()
370 );
371 eprintln!();
372
373 for (classification, count) in counts {
374 let percentage = count as f32 / cases.len() as f32 * 100.0;
375
376 eprintln!("{classification:?}: {count} [{percentage:.2}%]");
377 }
378
379 eprintln!();
380 eprintln!("likely slider-only details");
381 eprintln!();
382
383 let likely_slider_only_total: usize = likely_slider_only_counts.values().sum();
384
385 if likely_slider_only_total == 0 {
386 eprintln!("no likely slider-only mismatches");
387 } else {
388 let mut details: Vec<_> = likely_slider_only_counts.into_iter().collect();
389 details.sort_by(|(lhs_detail, lhs_count), (rhs_detail, rhs_count)| {
390 rhs_count.cmp(lhs_count).then_with(|| lhs_detail.cmp(rhs_detail))
391 });
392
393 for (detail, count) in details {
394 let slider_only_percentage = count as f32 / likely_slider_only_total as f32 * 100.0;
395 let total_percentage = count as f32 / cases.len() as f32 * 100.0;
396
397 eprintln!("{detail}: {count} [{slider_only_percentage:.2}% slider-only, {total_percentage:.2}% total]");
398 }
399 }
400
401 eprintln!();
402 eprintln!("first non-matching cases");
403 eprintln!();
404
405 for case in cases
406 .iter()
407 .filter(|case| case.gix_postprocess_slider_heuristics != case.git_postprocess_indent_heuristic)
408 .take(20)
409 {
410 eprintln!("{} {:?} {:?}", case.file_name, case.algorithm, case.classify());
411 }
412}
413
93mod baseline {414mod baseline {
94 use gix_diff::blob::Algorithm;415 use gix_diff::blob::Algorithm;
95 use std::ffi::OsStr;416 use std::ffi::OsStr;
@@ -102,17 +423,18 @@ mod baseline { · 6 unmodified lines
102 pub new_data: Vec<u8>,423 pub new_data: Vec<u8>,
103 }424 }
104 425
105 /// Returns `None` if the file isn't a baseline entry.426 /// Returns `None` if the file isn't a primary baseline entry.
106 pub fn parse_dir_entry(asset_dir: &Path, file_name: &OsStr) -> std::io::Result<Option<DirEntry>> {427 pub fn parse_dir_entry(asset_dir: &Path, file_name: &OsStr) -> std::io::Result<Option<DirEntry>> {
107 let file_name = file_name.to_str().expect("ascii filename").to_owned();428 let file_name = file_name.to_str().expect("ascii filename").to_owned();
108 429
109 if !file_name.ends_with(".baseline") {430 let Some(stem) = file_name.strip_suffix(".baseline") else {
110 return Ok(None);431 return Ok(None);
111 }432 };
112 433
113 let parts: Vec<_> = file_name.split('.').collect();434 let parts: Vec<_> = stem.split('.').collect();
114 let [name, algorithm, ..] = parts[..] else {435 let [name, algorithm] = parts[..] else {
115 unreachable!("BUG: Need file named '<name>.<algorithm>'")436 // Additional baselines like `<name>.<algorithm>.no-indent.baseline` are consumed separately.
437 return Ok(None);
116 };438 };
117 let algorithm = match algorithm {439 let algorithm = match algorithm {
118 "myers" => Algorithm::Myers,440 "myers" => Algorithm::Myers,
tests/it/src/commands/create_diff_cases.rs+6−4
@@ -85,10 +85,12 @@ mkdir -p {asset_dir} · 84 unmodified lines
85 }85 }
86 86
87 blocks.push(format!(87 blocks.push(format!(
88 r#"git -c diff.algorithm=myers diff --no-index "$ROOT/{asset_dir}/{old_blob_id}.blob" "$ROOT/{asset_dir}/{new_blob_id}.blob" > {old_blob_id}-{new_blob_id}.myers.baseline || true88 r#"git -c diff.algorithm=myers diff --no-index --no-ext-diff --no-color --indent-heuristic "$ROOT/{asset_dir}/{old_blob_id}.blob" "$ROOT/{asset_dir}/{new_blob_id}.blob" > {old_blob_id}-{new_blob_id}.myers.baseline || true
89git -c diff.algorithm=histogram diff --no-index "$ROOT/{asset_dir}/{old_blob_id}.blob" "$ROOT/{asset_dir}/{new_blob_id}.blob" > {old_blob_id}-{new_blob_id}.histogram.baseline || true89git -c diff.algorithm=myers diff --no-index --no-ext-diff --no-color --no-indent-heuristic "$ROOT/{asset_dir}/{old_blob_id}.blob" "$ROOT/{asset_dir}/{new_blob_id}.blob" > {old_blob_id}-{new_blob_id}.myers.no-indent.baseline || true
90cp "$ROOT/{asset_dir}/{old_blob_id}.blob" assets/90git -c diff.algorithm=histogram diff --no-index --no-ext-diff --no-color --indent-heuristic "$ROOT/{asset_dir}/{old_blob_id}.blob" "$ROOT/{asset_dir}/{new_blob_id}.blob" > {old_blob_id}-{new_blob_id}.histogram.baseline || true
91cp "$ROOT/{asset_dir}/{new_blob_id}.blob" assets/91git -c diff.algorithm=histogram diff --no-index --no-ext-diff --no-color --no-indent-heuristic "$ROOT/{asset_dir}/{old_blob_id}.blob" "$ROOT/{asset_dir}/{new_blob_id}.blob" > {old_blob_id}-{new_blob_id}.histogram.no-indent.baseline || true
92cp "$ROOT/{asset_dir}/{old_blob_id}.blob" {asset_dir}/
93cp "$ROOT/{asset_dir}/{new_blob_id}.blob" {asset_dir}/
92"#94"#
93 ));95 ));
94 }96 }

Keyboard shortcuts

General

Show keyboard shortcuts
?
Focus the current search
/

Issues and pull requests

Open filters
f

Issues, pull requests and Inbox

Select next item
j↓
Select previous item
k↑
Open selected item
Enter
Dismiss filters or clear selection
Esc

Repository code

Go to file
t