patch_set: accept CRLF line endings in all patch syntax

.gitattributes

@@ -0,0 +1,2 @@
+# The CRLF compat fixtures must keep their line endings byte for byte.
+tests/compat/*/crlf_*/** -text

src/binary/mod.rs

@@ -15,6 +15,9 @@ use alloc::vec::Vec;
 use core::fmt;
 use core::ops::Range;
 
+use crate::utils::LineIter;
+use crate::utils::Text;
+
 /// Cap preallocation when the size comes from untrusted input.
 ///
 /// This prevents instant OOM from a bogus header.
@@ -394,16 +397,9 @@ impl<'a> BinaryParser<'a> {
     }
 
     fn next_line(&mut self) -> Option<&'a [u8]> {
-        let rest = &self.input[self.offset..];
-        if rest.is_empty() {
-            return None;
-        }
-        let (line, skip) = match rest.iter().position(|&b| b == b'\n') {
-            Some(pos) => (&rest[..pos], pos + 1),
-            None => (rest, rest.len()),
-        };
-        self.offset += skip;
-        Some(line.strip_suffix(b"\r").unwrap_or(line))
+        let line = LineIter::new(&self.input[self.offset..]).next()?;
+        self.offset += line.len();
+        Some(line.strip_line_ending().unwrap_or(line))
     }
 }
 
@@ -474,10 +470,7 @@ fn parse_binary_block<'a>(parser: &mut BinaryParser<'a>) -> Option<BinaryBlock<'
 
     // Slice the data lines, stripping the final line ending.
     let data = &parser.input[data_start..data_end];
-    let data = data
-        .strip_suffix(b"\r\n".as_slice())
-        .or_else(|| data.strip_suffix(b"\n".as_slice()))
-        .unwrap_or(data);
+    let data = data.strip_line_ending().unwrap_or(data);
 
     Some(BinaryBlock {
         kind,
@@ -503,8 +496,8 @@ fn decode_base85_lines(data: &[u8]) -> Result<Vec<u8>, BinaryPatchParseError> {
     // A rough estimate: In Base85, 5 chars -> 4 bytes
     let mut result = Vec::with_capacity(data.len() * 4 / 5);
 
-    for line in data.split(|&b| b == b'\n') {
-        let line = line.strip_suffix(b"\r".as_slice()).unwrap_or(line);
+    for line in LineIter::new(data) {
+        let line = line.strip_line_ending().unwrap_or(line);
         if line.is_empty() {
             continue;
         }

src/patch/format.rs

@@ -237,14 +237,20 @@ impl<T: AsRef<[u8]> + ?Sized> HunkDisplay<'_, T> {
             if self.f.with_color {
                 write!(w, "{}", style::FUNCTION_CONTEXT)?;
             }
-            write!(w, " ")?;
             w.write_all(ctx.as_ref())?;
             #[cfg(feature = "color")]
             if self.f.with_color {
                 write!(w, "{:#}", style::FUNCTION_CONTEXT)?;
             }
         }
-        writeln!(w)?;
+        // The function context keeps the line ending it was parsed with.
+        let has_line_ending = self
+            .hunk
+            .function_context
+            .is_some_and(|ctx| ctx.as_ref().ends_with(b"\n"));
+        if !has_line_ending {
+            writeln!(w)?;
+        }
 
         for line in &self.hunk.lines {
             self.f.write_line_into(line, &mut w)?;
@@ -272,13 +278,20 @@ impl Display for HunkDisplay<'_, str> {
             if self.f.with_color {
                 write!(f, "{}", style::FUNCTION_CONTEXT)?;
             }
-            write!(f, " {ctx}")?;
+            write!(f, "{ctx}")?;
             #[cfg(feature = "color")]
             if self.f.with_color {
                 write!(f, "{:#}", style::FUNCTION_CONTEXT)?;
             }
         }
-        writeln!(f)?;
+        // The function context keeps the line ending it was parsed with.
+        let has_line_ending = self
+            .hunk
+            .function_context
+            .is_some_and(|ctx| ctx.ends_with('\n'));
+        if !has_line_ending {
+            writeln!(f)?;
+        }
 
         for line in &self.hunk.lines {
             write!(f, "{}", self.f.fmt_line(line))?;

src/patch/mod.rs

@@ -302,7 +302,13 @@ impl<'a, T: ?Sized> Hunk<'a, T> {
         self.new_range
     }
 
-    /// Returns the function context (if any) for the hunk
+    /// Returns the function context (if any) for the hunk.
+    ///
+    /// This is the text after the closing `@@` of the hunk header, such as
+    /// the enclosing function's signature added by `diff -p` or `git diff`.
+    /// It excludes the space before it but keeps the hunk header's line
+    /// ending (`\n` or `\r\n`), if there is one, so formatting the hunk
+    /// writes the header back unchanged.
     pub fn function_context(&self) -> Option<&T> {
         self.function_context
     }
@@ -390,6 +396,12 @@ impl fmt::Display for HunkRange {
 ///
 /// A `Line` contains the terminating newline character `\n` unless it is the final
 /// line in the file and the file does not end with a newline character.
+///
+/// Line endings are kept exactly as they appear in the patch: a line parsed
+/// from a patch with CRLF line endings ends in `\r\n`, so applying the patch
+/// only matches lines that also end in `\r\n`. This matches `git apply`,
+/// which treats the `\r` as part of the line's content. (GNU patch instead
+/// strips the `\r` from every line of a patch with CRLF line endings.)
 #[derive(Debug, PartialEq, Eq)]
 pub enum Line<'a, T: ?Sized> {
     /// A line providing context in the diff which is present in both the old and new file

src/patch/parse.rs

@@ -178,16 +178,11 @@ fn parse_filename<'a, T: Text + ?Sized>(prefix: &str, line: &'a T) -> Result<Cow
         .strip_prefix(prefix)
         .ok_or(ParsePatchErrorKind::InvalidFilename)?;
 
-    let filename = if let Some((filename, _)) = line.split_at_exclusive("\t") {
-        filename
-    } else if let Some((filename, _)) = line.split_at_exclusive("\n") {
-        // Accept CRLF line endings, as produced by tools on Windows.
-        // A carriage return is never part of the filename itself: it has
-        // to be quoted when unquoted filenames are used, and is written
-        // as the `\r` escape inside a quoted one. See `byte_needs_quoting`.
-        filename.strip_suffix("\r").unwrap_or(filename)
-    } else {
-        return Err(ParsePatchErrorKind::FilenameUnterminated.into());
+    let filename = match line.split_at_exclusive("\t") {
+        Some((filename, _)) => filename,
+        None => line
+            .strip_line_ending()
+            .ok_or(ParsePatchErrorKind::FilenameUnterminated)?,
     };
 
     let filename = escaped_filename(filename)?;

src/patch/tests.rs

@@ -804,3 +804,110 @@ mod error_display {
         assert_eq!(err.kind, ParsePatchErrorKind::InvalidHunkHeader);
     }
 }
+
+/// A patch written by `diff -u -p` (GNU diffutils 3.12). `git diff` writes the
+/// same hunk header: one space after the closing `@@`, then the enclosing
+/// function.
+const FUNCTION_CONTEXT_PATCH: &str = "\
+--- a/short.c
++++ b/short.c
+@@ -4,6 +4,6 @@ int main(void)
+     line3;
+     line4;
+     line5;
+-    line6;
++    CHANGED;
+     line7;
+     line8;
+";
+
+// The function context excludes the space before it but keeps the line
+// ending, so formatting writes the hunk header back unchanged.
+#[test]
+fn function_context_roundtrip() {
+    let s = FUNCTION_CONTEXT_PATCH;
+    let p = parse(s).unwrap();
+    assert_eq!(p.hunks()[0].function_context(), Some("int main(void)\n"));
+    assert_eq!(p.to_string(), s);
+
+    let b = parse_bytes(s.as_ref()).unwrap();
+    assert_eq!(
+        b.hunks()[0].function_context(),
+        Some(&b"int main(void)\n"[..])
+    );
+    assert_eq!(b.to_bytes(), s.as_bytes());
+}
+
+// A CRLF hunk header keeps its `\r\n` through a round trip. The formatter
+// writes its own `---`/`+++` lines, which end in `\n`.
+#[test]
+fn function_context_crlf() {
+    let s = "\
+--- a/f\r
++++ b/f\r
+@@ -1 +1 @@ fn foo()\r
+-old\r
++new\r
+";
+    let p = parse(s).unwrap();
+    assert_eq!(p.hunks()[0].function_context(), Some("fn foo()\r\n"));
+    assert_eq!(
+        p.to_string(),
+        "--- a/f\n+++ b/f\n@@ -1 +1 @@ fn foo()\r\n-old\r\n+new\r\n"
+    );
+}
+
+// With nothing after the closing `@@` there is no function context. With only
+// a space after it, the function context is just the line ending, and
+// formatting keeps the space.
+#[test]
+fn function_context_absent_or_empty() {
+    let absent = "--- a/f\n+++ b/f\n@@ -1 +1 @@\n-old\n+new\n";
+    let p = parse(absent).unwrap();
+    assert_eq!(p.hunks()[0].function_context(), None);
+    assert_eq!(p.to_string(), absent);
+
+    let empty = "--- a/f\n+++ b/f\n@@ -1 +1 @@ \n-old\n+new\n";
+    let p = parse(empty).unwrap();
+    assert_eq!(p.hunks()[0].function_context(), Some("\n"));
+    assert_eq!(p.to_string(), empty);
+}
+
+// A hunk header at the end of the input has no line ending, so formatting
+// adds one.
+#[test]
+fn function_context_without_line_ending() {
+    let s = "--- a/f\n+++ b/f\n@@ -0,0 +0,0 @@ fn foo()";
+    let p = parse(s).unwrap();
+    assert_eq!(p.hunks()[0].function_context(), Some("fn foo()"));
+    assert_eq!(p.to_string(), format!("{s}\n"));
+}
+
+// Color only wraps parts of the output in escape sequences.
+#[cfg(feature = "color")]
+#[test]
+fn function_context_with_color() {
+    use super::PatchFormatter;
+
+    fn strip_escapes(s: &str) -> alloc::string::String {
+        let mut out = alloc::string::String::new();
+        let mut rest = s;
+        while let Some(start) = rest.find("\x1b[") {
+            out.push_str(&rest[..start]);
+            let len = rest[start..].find('m').expect("unterminated escape");
+            rest = &rest[start + len + 1..];
+        }
+        out.push_str(rest);
+        out
+    }
+
+    let s = FUNCTION_CONTEXT_PATCH;
+    let f = PatchFormatter::new().with_color();
+    let p = parse(s).unwrap();
+    assert_eq!(strip_escapes(&f.fmt_patch(&p).to_string()), s);
+
+    let b = parse_bytes(s.as_ref()).unwrap();
+    let mut bytes = alloc::vec::Vec::new();
+    f.write_patch_into(&b, &mut bytes).unwrap();
+    assert_eq!(strip_escapes(core::str::from_utf8(&bytes).unwrap()), s);
+}

src/patch_set/parse.rs

@@ -24,13 +24,20 @@ const MODIFIED_PREFIX: &str = "+++ ";
 /// Path used to indicate file creation or deletion.
 const DEV_NULL: &str = "/dev/null";
 
-/// Separator between commit message and patch in git format-patch output.
-const EMAIL_PREAMBLE_SEPARATOR: &str = "\n---\n";
+/// Line separating the commit message from the patch in git format-patch
+/// output.
+const EMAIL_PREAMBLE_SEPARATOR: &str = "---";
 
 /// Streaming iterator for parsing patches one at a time.
 ///
 /// Created by [`PatchSet::parse`] or [`PatchSet::parse_bytes`].
 ///
+/// # Line endings
+///
+/// Lines of patch syntax, such as headers and separators, may end in either
+/// `\n` or `\r\n`. Hunk lines keep their line endings as content; see
+/// [`Line`](crate::Line).
+///
 /// # Example
 ///
 /// ```
@@ -194,14 +201,17 @@ fn find_patch_start<T: Text + ?Sized>(input: &T) -> Option<usize> {
 
 /// Strips email preamble (headers and commit message) from `git format-patch` output.
 ///
-/// Returns the content after the first `\n---\n` separator.
+/// Returns the content after the first `---` separator line.
 ///
 /// ## Observed git behavior
 ///
 /// `git mailinfo` (used by `git am`) uses the first `---` line
 /// as the separator between commit message and patch content.
 /// It does not check if `diff --git` follows or there are more `---` lines.
 ///
+/// `git mailsplit` (also used by `git am`) strips the `\r` from lines ending
+/// in `\r\n` unless `--keep-cr` is given, so a `---\r\n` line separates too.
+///
 /// From [`git format-patch`] manpage:
 ///
 /// > The log message and the patch are separated by a line with a three-dash line.
@@ -213,13 +223,18 @@ fn strip_email_preamble<T: Text + ?Sized>(input: &T) -> &T {
         return input;
     }
 
-    match input.find(EMAIL_PREAMBLE_SEPARATOR) {
-        Some(pos) => {
-            let (_, rest) = input.split_at(pos + EMAIL_PREAMBLE_SEPARATOR.len());
-            rest
+    let mut offset = 0;
+    for line in input.lines() {
+        offset += line.len();
+        let is_separator = line
+            .strip_line_ending()
+            .is_some_and(|line| line.as_bytes() == EMAIL_PREAMBLE_SEPARATOR.as_bytes());
+        if is_separator {
+            let (_, rest) = input.split_at(offset);
+            return rest;
         }
-        None => input,
     }
+    input
 }
 
 fn next_gitdiff_patch<'a, T: Text + ?Sized>(
@@ -423,7 +438,7 @@ impl<'a, T: Text + ?Sized> GitHeader<'a, T> {
         let mut consumed = 0;
 
         for line in input.lines() {
-            let trimmed = strip_line_ending(line);
+            let trimmed = line.strip_line_ending().unwrap_or(line);
 
             if let Some(rest) = trimmed.strip_prefix("diff --git ") {
                 // Only accept the first `diff --git` line.
@@ -820,13 +835,3 @@ fn extract_file_op_unidiff<'a, T: Text + ?Sized>(
         }
     }
 }
-
-/// Strips the trailing `\n` from a line yielded by [`Text::lines`].
-///
-/// [`Text::lines`] includes line endings; strip for matching.
-fn strip_line_ending<T: Text + ?Sized>(line: &T) -> &T {
-    // TODO: GNU patch strips trailing CRs from CRLF patches automatically.
-    // We should consider adding compat tests for GNU patch.
-    // And `git apply` seems to reject. Worth adding tests as well.
-    line.strip_suffix("\n").unwrap_or(line)
-}