fix: address review feedback on classic diff fallback

- Drop the needless borrow at the format_diff_changes call site; with
  warnings = "deny" this was failing cargo clippy --all-targets and
  blocking the clippy job that build, test and release depend on.
- Keep the original both-files dump as the timer.track baseline. The
  synthesized classic diff is empty for identical files, which recorded
  input_tokens = 0 and reported ~0% instead of ~46% in rtk gain --history.
- Un-gate render_file_diff: fold it into render_diff rather than compiling
  a production helper only under cfg(test).
- Update the scripts/test-all.sh assertion to match the real output.
This commit is contained in:
Matt Van Horn
2026-08-31 06:16:41 -07:00
committed by Nicolas Le Cam
parent 1246ce335f
commit d1a0d9c2c1
2 changed files with 15 additions and 35 deletions
+1 -1
View File
@@ -540,7 +540,7 @@ section "Diff"
assert_ok "rtk diff identical files" rtk diff Cargo.toml Cargo.toml
assert_fails "rtk diff differing files" rtk diff Cargo.toml LICENSE
assert_contains "rtk diff shows changes" "added" rtk diff Cargo.toml LICENSE
assert_contains "rtk diff shows classic changes" "^< " bash -c 'rtk diff "$1" "$2"; [[ $? -eq 1 ]]' _ Cargo.toml LICENSE
# ── 37. Wc ────────────────────────────────────────────
+14 -34
View File
@@ -20,31 +20,22 @@ pub fn run(file1: &Path, file2: &Path, verbose: u8) -> Result<i32> {
let lines1: Vec<&str> = content1.lines().collect();
let lines2: Vec<&str> = content2.lines().collect();
let diff = compute_diff(&lines1, &lines2);
let raw = format_classic_diff(&diff);
let fallback = format_classic_diff(&diff);
let tracking_baseline = format!("{}\n---\n{}", content1, content2);
let (rtk, exit_code) = render_diff(file1, file2, &diff);
let shown = select_file_diff_output(&diff, &raw, &rtk);
let shown = select_file_diff_output(&diff, &fallback, &rtk);
print!("{}", shown);
timer.track(
&format!("diff {} {}", file1.display(), file2.display()),
"rtk diff",
&raw,
&tracking_baseline,
shown,
);
Ok(exit_code)
}
/// Renders the condensed file comparison and returns it with the
/// diff-convention exit code (0 = identical, 1 = differences found).
#[cfg(test)]
fn render_file_diff(file1: &Path, file2: &Path, content1: &str, content2: &str) -> (String, i32) {
let lines1: Vec<&str> = content1.lines().collect();
let lines2: Vec<&str> = content2.lines().collect();
let diff = compute_diff(&lines1, &lines2);
render_diff(file1, file2, &diff)
}
fn render_diff(file1: &Path, file2: &Path, diff: &DiffResult) -> (String, i32) {
if diff.changes.is_empty() {
return ("[ok] Files are identical\n".to_string(), 0);
@@ -56,7 +47,7 @@ fn render_diff(file1: &Path, file2: &Path, diff: &DiffResult) -> (String, i32) {
" +{} added, -{} removed, ~{} modified\n\n",
diff.added, diff.removed, diff.modified
));
rtk.push_str(&format_diff_changes(&diff));
rtk.push_str(&format_diff_changes(diff));
(rtk, 1)
}
@@ -444,18 +435,14 @@ mod tests {
assert!(result.changes.is_empty());
}
// --- render_file_diff (issue #2364 regression) ---
// --- render_diff (issue #2364 regression) ---
#[test]
fn test_render_modified_only_yaml_not_identical() {
// "a: 1" vs "a: 2" is classified as modified (similarity > 0.5);
// the identical check must not ignore modified-only diffs.
let (out, code) = render_file_diff(
Path::new("one.yaml"),
Path::new("two.yaml"),
"a: 1\n",
"a: 2\n",
);
let diff = compute_diff(&["a: 1"], &["a: 2"]);
let (out, code) = render_diff(Path::new("one.yaml"), Path::new("two.yaml"), &diff);
assert!(
!out.contains("identical"),
"modified-only diff reported as identical:\n{}",
@@ -469,12 +456,8 @@ mod tests {
#[test]
fn test_render_modified_only_json_not_identical() {
let (out, code) = render_file_diff(
Path::new("j1.json"),
Path::new("j2.json"),
"{\"a\": 1}\n",
"{\"a\": 2}\n",
);
let diff = compute_diff(&["{\"a\": 1}"], &["{\"a\": 2}"]);
let (out, code) = render_diff(Path::new("j1.json"), Path::new("j2.json"), &diff);
assert!(
!out.contains("identical"),
"modified-only diff reported as identical:\n{}",
@@ -485,19 +468,16 @@ mod tests {
#[test]
fn test_render_identical_files_exit_zero() {
let (out, code) = render_file_diff(
Path::new("a.yaml"),
Path::new("b.yaml"),
"a: 1\nb: 2\n",
"a: 1\nb: 2\n",
);
let diff = compute_diff(&["a: 1", "b: 2"], &["a: 1", "b: 2"]);
let (out, code) = render_diff(Path::new("a.yaml"), Path::new("b.yaml"), &diff);
assert!(out.contains("[ok] Files are identical"));
assert_eq!(code, 0);
}
#[test]
fn test_render_added_removed_exit_one() {
let (out, code) = render_file_diff(Path::new("t1.txt"), Path::new("t2.txt"), "x\n", "y\n");
let diff = compute_diff(&["x"], &["y"]);
let (out, code) = render_diff(Path::new("t1.txt"), Path::new("t2.txt"), &diff);
assert!(out.contains("+1 added, -1 removed"));
assert_eq!(code, 1);
}