Skip to content

Commit 4956373

Browse files
kylehgcKuSh
authored andcommitted
test(diff): pin both flush sites with a two-file fixture; move instead of clone
Amendment from self-review of the original commit: - The regression test used a single-file diff, but condense_unified_diff flushes a file's changes at two separate sites — once per `+++` for the preceding file, once after the loop for the last one. A single-file fixture only ever reaches the second: mutation testing showed the in-loop site could be reverted to the indenting form with the whole suite still green. The fixture now carries two files and fails against that mutant. - The anti-indent assertion was a blanket "no line starts with a space", which the ` ... +N more` trailer makes false for any diff with more than ten changes. Scoped to change lines. - The flush loops now move the lines (`result.append(&mut changes)`) instead of cloning each one; the mid-loop `changes.clear()` stays for the branch-not-taken path. The `Vec<String>` annotation is dropped — inference comes from `changes.push(line.to_string())`, so it was never needed (the original commit message's claim about the removed `format!` was wrong; `format!` accepts any Display and contributes no type information). Gate: cargo fmt --all / clippy --all-targets / test --all all exit 0.
1 parent ca85483 commit 4956373

1 file changed

Lines changed: 26 additions & 15 deletions

File tree

‎src/cmds/git/diff_cmd.rs‎

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ fn condense_unified_diff(diff: &str) -> String {
164164
let mut current_file = String::new();
165165
let mut added = 0;
166166
let mut removed = 0;
167-
let mut changes: Vec<String> = Vec::new();
167+
let mut changes = Vec::new();
168168

169169
// Never truncate diff content — users make decisions based on this data.
170170
// Only strip diff metadata (headers, @@ hunks); all +/- lines shown in full.
@@ -173,10 +173,8 @@ fn condense_unified_diff(diff: &str) -> String {
173173
if line.starts_with("+++ ") {
174174
if !current_file.is_empty() && (added > 0 || removed > 0) {
175175
result.push(format!("[file] {} (+{} -{})", current_file, added, removed));
176-
for c in &changes {
177-
// Column 0: anchored greps (`^[+-]`) must match these.
178-
result.push(c.clone());
179-
}
176+
// Column 0: anchored greps (`^[+-]`) must match these.
177+
result.append(&mut changes);
180178
}
181179
current_file = line
182180
.trim_start_matches("+++ ")
@@ -198,10 +196,8 @@ fn condense_unified_diff(diff: &str) -> String {
198196
// Last file
199197
if !current_file.is_empty() && (added > 0 || removed > 0) {
200198
result.push(format!("[file] {} (+{} -{})", current_file, added, removed));
201-
for c in &changes {
202-
// Column 0: anchored greps (`^[+-]`) must match these.
203-
result.push(c.clone());
204-
}
199+
// Column 0: anchored greps (`^[+-]`) must match these.
200+
result.append(&mut changes);
205201
}
206202

207203
result.join("\n")
@@ -398,14 +394,29 @@ diff --git a/b.rs b/b.rs
398394

399395
#[test]
400396
fn test_condense_unified_diff_markers_at_column_0() {
401-
// Same silent-false-negative class as compact_diff (#118 / upstream
402-
// #3646): indented markers make anchored greps (`^[+-]`) match nothing.
403-
let diff = "diff --git a/f.rs b/f.rs\n--- a/f.rs\n+++ b/f.rs\n@@ -1,2 +1,2 @@\n-old line\n+new line\n";
397+
// Same silent-false-negative class as compact_diff (#3646): indented
398+
// markers make anchored greps (`^[+-]`) match nothing.
399+
//
400+
// Two files on purpose. A file's changes are flushed at two separate
401+
// sites: once per `+++` for the preceding file, once after the loop for
402+
// the last one. A single-file fixture only ever reaches the second, so
403+
// the first could be reverted with the whole suite still green.
404+
let diff = "diff --git a/a.rs b/a.rs\n--- a/a.rs\n+++ b/a.rs\n@@ -1 +1 @@\n-fn old() {}\n+fn new() {}\ndiff --git a/b.rs b/b.rs\n--- a/b.rs\n+++ b/b.rs\n@@ -1 +1 @@\n-let x = 1;\n+let x = 2;\n";
404405
let result = condense_unified_diff(diff);
405-
assert!(result.lines().any(|l| l == "-old line"), "got:\n{}", result);
406-
assert!(result.lines().any(|l| l == "+new line"), "got:\n{}", result);
406+
for want in ["-fn old() {}", "+fn new() {}", "-let x = 1;", "+let x = 2;"] {
407+
assert!(
408+
result.lines().any(|l| l == want),
409+
"missing {want:?} at column 0 in:\n{}",
410+
result
411+
);
412+
}
413+
// Scoped to change lines: the ` ... +N more` trailer is legitimately
414+
// indented, so a blanket "no line starts with a space" would be false
415+
// for any diff with more than ten changes.
407416
assert!(
408-
!result.lines().any(|l| l.starts_with(' ')),
417+
!result
418+
.lines()
419+
.any(|l| l.starts_with(" +") || l.starts_with(" -")),
409420
"change lines must not be indented:\n{}",
410421
result
411422
);

0 commit comments

Comments
 (0)