From 3bc63a87bb082ea32f8a40559b46c1c1bd48a84e Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Tue, 28 Jul 2026 15:26:13 +0200 Subject: [PATCH] fix: detect already-applied diffs via post-image matching The reverse round-trip heuristic in is_diff_applied_with_config misclassified deletion-only patches as already applied: reversing a deletion yields an insertion diff, which applies to the *un*patched content just as well (duplicating the still-present lines) and round-trips cleanly back to the input. Combined with fuzzy per-line matching this caused rattler-build to skip valid patches (prefix-dev/rattler-build#2693). Instead, consider a diff already applied only when every hunk's post-image (context + inserted lines) occurs contiguously in the base image, compared strictly (whitespace/case normalization per config, but no similarity threshold and no fuzz). This mirrors GNU patch's 'previously applied' detection and has neither the forward-apply nor the reverse-round-trip failure mode. Co-Authored-By: Claude Fable 5 --- src/apply.rs | 179 ++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 164 insertions(+), 15 deletions(-) diff --git a/src/apply.rs b/src/apply.rs index a1edf3b..9a22ac1 100644 --- a/src/apply.rs +++ b/src/apply.rs @@ -367,15 +367,24 @@ pub fn apply_bytes(base_image: &[u8], patch: &Diff<'_, [u8]>) -> ApplyResult, config: &ApplyConfig, ) -> bool { - // Reverse round-trip: reversing an already-applied diff must produce a - // *different* pre-image that, patched forward again, reproduces the input. - match apply_bytes_with_config(base_image, &diff.reverse(), config) { - Ok((pre, _)) if pre != base_image => apply_bytes_with_config(&pre, diff, config) - .map(|(re, _)| re == base_image) - .unwrap_or(false), - _ => false, + let hunks = diff.hunks(); + if hunks.is_empty() { + return false; + } + + let image: Vec<(&[u8], Option)> = LineIter::new(base_image).collect(); + + hunks + .iter() + .all(|hunk| find_post_image_position(&image, hunk, config).is_some()) +} + +/// Search `image` for a position where `hunk`'s post-image (context plus +/// inserted lines) occurs contiguously, comparing lines strictly (equality +/// modulo the whitespace/case normalization from `config`, without any +/// similarity threshold). Returns `None` for hunks with an empty post-image. +fn find_post_image_position( + image: &[(&[u8], Option)], + hunk: &Hunk<'_, [u8]>, + config: &ApplyConfig, +) -> Option { + let post_image_lines: Vec<_> = post_image(hunk.lines()).collect(); + if post_image_lines.is_empty() { + return None; } + + let match_at = |pos: usize| -> bool { + image + .get(pos..pos + post_image_lines.len()) + .is_some_and(|window| { + post_image_lines + .iter() + .zip(window) + .all(|(post_line, image_line)| { + post_line.0.similarity(image_line.0, config) >= 1.0 + }) + }) + }; + + // Start at the position the hunk header points at and search outward, like + // the pre-image search in `find_position`. + let pos = std::cmp::min(hunk.new_range().start().saturating_sub(1), image.len()); + let backward = (0..pos).rev(); + let forward = pos + 1..image.len(); + + iter::once(pos) + .chain(interleave(backward, forward)) + .find(|&pos| match_at(pos)) } /// The outcome of attempting to apply a diff with [`apply_bytes_reporting`]. @@ -440,8 +489,8 @@ pub enum ApplyOutcome { /// This is a convenience wrapper around [`is_diff_applied_with_config`] and /// [`apply_bytes_with_config`] that answers "apply this, but if it is already /// applied tell me so instead of applying it again". It checks for the -/// already-applied case *first* (via a reverse round-trip) and only forward -/// applies when the diff is genuinely not yet applied. This ordering is +/// already-applied case *first* (by locating each hunk's post-image) and only +/// forward applies when the diff is genuinely not yet applied. This ordering is /// necessary under fuzzy matching, where a forward apply of an already-applied /// diff is unreliable — it may fail, or it may succeed while wrongly /// re-applying the change (e.g. duplicating an inserted line). @@ -1304,6 +1353,106 @@ mod test { )); } + #[test] + fn test_is_diff_applied_pure_deletion() { + // A hunk that only deletes lines. This is the case most likely to fool + // a reverse-round-trip check: the reversed diff is a pure insertion, + // which also applies to the *un*patched content (duplicating the + // still-present lines) and round-trips cleanly. + // https://github.com/prefix-dev/rattler-build/issues/2693 + let patch = "\ +--- a/CMakeLists.txt ++++ b/CMakeLists.txt +@@ -1,7 +1,4 @@ + else() +- if (CMAKE_BUILD_TYPE MATCHES Debug) +- add_compile_options(/MTd) +- endif() + add_compile_options(/utf-8) + add_compile_definitions(-D_CRT_SECURE_NO_WARNINGS=1) + endif() +"; + let diff = Diff::from_bytes(patch.as_bytes()).unwrap(); + let config = fuzzy_config(); + + let pre = b"\ +else() + if (CMAKE_BUILD_TYPE MATCHES Debug) + add_compile_options(/MTd) + endif() + add_compile_options(/utf-8) + add_compile_definitions(-D_CRT_SECURE_NO_WARNINGS=1) +endif() +"; + let post = b"\ +else() + add_compile_options(/utf-8) + add_compile_definitions(-D_CRT_SECURE_NO_WARNINGS=1) +endif() +"; + + // The unpatched content still contains the lines to delete: NOT applied. + assert!(!is_diff_applied_with_config(pre, &diff, &config)); + // The patched content no longer contains them: already applied. + assert!(is_diff_applied_with_config(post, &diff, &config)); + + // And the reporting wrapper must actually apply it, not skip it. + match apply_bytes_reporting(pre, &diff, &config) { + ApplyOutcome::Applied(content, _) => assert_eq!(content, post), + other => panic!("expected Applied, got {other:?}"), + } + match apply_bytes_reporting(post, &diff, &config) { + ApplyOutcome::AlreadyApplied(content) => assert_eq!(content, post), + other => panic!("expected AlreadyApplied, got {other:?}"), + } + } + + #[test] + fn test_deletion_patch_with_stale_context_is_not_already_applied() { + // Deletion-only hunk whose trailing context does not match the file + // (the patch was written against a newer upstream). GNU patch rejects + // this; it must be reported as Failed, never as AlreadyApplied. + // Distilled from the libddwaf win-rt.patch case in + // https://github.com/prefix-dev/rattler-build/issues/2693 + let patch = "\ +--- a/CMakeLists.txt ++++ b/CMakeLists.txt +@@ -1,10 +1,4 @@ + else() +- if (CMAKE_BUILD_TYPE MATCHES Debug) +- add_compile_options(/MTd) +- else() +- add_compile_options(/MT) +- endif() +- + add_compile_options(/utf-8) + add_compile_definitions(-D_CRT_SECURE_NO_WARNINGS=1) + endif() +"; + let diff = Diff::from_bytes(patch.as_bytes()).unwrap(); + let config = fuzzy_config(); + + // The actual file has no `add_compile_options(/utf-8)` line, so the + // hunk's context can never fully match. + let base = b"\ +else() + if (CMAKE_BUILD_TYPE MATCHES Debug) + add_compile_options(/MTd) + else() + add_compile_options(/MT) + endif() + + add_compile_definitions(-D_CRT_SECURE_NO_WARNINGS=1) +endif() +"; + + assert!(!is_diff_applied_with_config(base, &diff, &config)); + match apply_bytes_reporting(base, &diff, &config) { + ApplyOutcome::Failed(_) => {} + other => panic!("expected Failed, got {other:?}"), + } + } + #[test] fn test_apply_bytes_reporting_outcomes() { let patch = "\