Skip to content

Commit 691b316

Browse files
committed
fix(patch_set): accept CRLF line endings in git extended headers
GitHeader::parse extracts header values (file modes, rename/copy paths, the diff --git line) via strip_line_ending, which stripped only the trailing \n. In a patch file saved with CRLF line endings (e.g. on Windows) every extracted value therefore kept its trailing \r, and parsing a mode header failed outright: error parsing patches at byte 0: invalid file mode: 100644\r Strip a trailing \r as well, mirroring how GNU patch strips trailing CRs from CRLF patches (the behavior the TODO on strip_line_ending already pointed at). This only affects git extended header lines; hunk content is handled by the patch parser itself. Downstream report: pnpm/pnpm#16641, where a CRLF patch file that creates a new file fails with ERR_PNPM_INVALID_PATCH on pnpm 12 (pnpm parses patches with diffy's PatchSet::parse). Tests: crlf_new_file_with_content and crlf_mode_only_change fail on the unpatched parser with InvalidFileMode("100644\r") and pass with this change; the full suite (131 lib tests) passes.
1 parent 4aa5870 commit 691b316

2 files changed

Lines changed: 58 additions & 5 deletions

File tree

‎src/patch_set/parse.rs‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -821,12 +821,17 @@ fn extract_file_op_unidiff<'a, T: Text + ?Sized>(
821821
}
822822
}
823823

824-
/// Strips the trailing `\n` from a line yielded by [`Text::lines`].
824+
/// Strips the trailing line ending from a line yielded by [`Text::lines`].
825825
///
826826
/// [`Text::lines`] includes line endings; strip for matching.
827827
fn strip_line_ending<T: Text + ?Sized>(line: &T) -> &T {
828-
// TODO: GNU patch strips trailing CRs from CRLF patches automatically.
829-
// We should consider adding compat tests for GNU patch.
830-
// And `git apply` seems to reject. Worth adding tests as well.
831-
line.strip_suffix("\n").unwrap_or(line)
828+
// GNU patch strips trailing CRs from CRLF patches automatically.
829+
// Mirror that for header lines so a patch file saved with CRLF line
830+
// endings (e.g. on Windows) parses the same as its LF equivalent:
831+
// without this, extracted values such as the file mode keep their
832+
// trailing `\r` and fail to parse (`invalid file mode: 100644\r`).
833+
// Note `git apply` seems to reject CRLF patches. Worth adding tests
834+
// for both behaviors as well.
835+
let line = line.strip_suffix("\n").unwrap_or(line);
836+
line.strip_suffix("\r").unwrap_or(line)
832837
}

‎src/patch_set/tests.rs‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -759,6 +759,54 @@ index 0000000..abc1234
759759
);
760760
}
761761

762+
/// New file with content, with CRLF line endings throughout (e.g. a
763+
/// patch file saved on Windows). The extended header values must not
764+
/// keep the trailing `\r`: `new file mode 100644\r` previously failed
765+
/// with `invalid file mode: 100644\r`.
766+
#[test]
767+
fn crlf_new_file_with_content() {
768+
let input = "\
769+
diff --git a/new.rs b/new.rs
770+
new file mode 100644
771+
index 0000000..abc1234
772+
--- /dev/null
773+
+++ b/new.rs
774+
@@ -0,0 +1 @@
775+
+hello
776+
"
777+
.replace('\n', "\r\n");
778+
let patches = parse_gitdiff(&input);
779+
assert_eq!(patches.len(), 1);
780+
assert!(patches[0].operation().is_create());
781+
assert_eq!(
782+
patches[0].new_mode(),
783+
Some(&super::super::FileMode::Regular),
784+
);
785+
}
786+
787+
/// Mode-only change with CRLF line endings: both `old mode` and
788+
/// `new mode` values must parse despite the trailing `\r`.
789+
#[test]
790+
fn crlf_mode_only_change() {
791+
let input = "\
792+
diff --git a/script.sh b/script.sh
793+
old mode 100644
794+
new mode 100755
795+
"
796+
.replace('\n', "\r\n");
797+
let patches = parse_gitdiff(&input);
798+
assert_eq!(patches.len(), 1);
799+
assert!(patches[0].operation().is_modify());
800+
assert_eq!(
801+
patches[0].old_mode(),
802+
Some(&super::super::FileMode::Regular),
803+
);
804+
assert_eq!(
805+
patches[0].new_mode(),
806+
Some(&super::super::FileMode::Executable),
807+
);
808+
}
809+
762810
/// `diff --git` line with no-prefix paths (`git diff --no-prefix`).
763811
/// Fallback path parsing works when ---/+++ are absent.
764812
#[test]

0 commit comments

Comments
 (0)