Skip to content

Preserve line endings when applying patches #30882

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
charliemarsh-oai wants to merge 3 commits into
base: main
Choose a base branch
from
+287 −87
Open

Preserve line endings when applying patches #30882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
37e9c5f
Preserve line endings when applying patches
charliemarsh-oai Jul 2, 2026
4919933
Document line-ending preservation
charliemarsh-oai Jul 2, 2026
ecd347e
Support CR line endings in apply_patch
charliemarsh-oai Jul 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 42 additions & 49 deletions codex-rs/apply-patch/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ mod parser;
mod seek_sequence;
mod standalone_executable;
mod streaming_parser;
mod text_file;

use std::collections::HashMap;
use std::io;
Expand All @@ -23,6 +24,8 @@ pub use parser::UpdateFileChunk;
pub use parser::parse_patch;
use similar::TextDiff;
pub use streaming_parser::StreamingPatchParser;
use text_file::Replacement;
use text_file::SourceFile;
use thiserror::Error;

pub use invocation::maybe_parse_apply_patch_verified;
Expand Down Expand Up @@ -688,22 +691,13 @@ async fn derive_new_contents_from_chunks(
})
})?;

let mut original_lines: Vec<String> = original_contents.split('\n').map(String::from).collect();

// Drop the trailing empty element that results from the final newline so
// that line counts match the behaviour of standard `diff`.
if original_lines.last().is_some_and(String::is_empty) {
original_lines.pop();
}
let mut source_file = SourceFile::parse(&original_contents);
let original_lines = source_file.line_texts();

let path_text = path.inferred_native_path_string();
let replacements = compute_replacements(&original_lines, &path_text, chunks)?;
let new_lines = apply_replacements(original_lines, &replacements);
let mut new_lines = new_lines;
if !new_lines.last().is_some_and(String::is_empty) {
new_lines.push(String::new());
}
let new_contents = new_lines.join("\n");
source_file.apply_replacements(&replacements);
let new_contents = source_file.into_contents();
Ok(AppliedPatch {
original_contents,
new_contents,
Expand All @@ -717,8 +711,8 @@ fn compute_replacements(
original_lines: &[String],
path: &str,
chunks: &[UpdateFileChunk],
) -> std::result::Result<Vec<(usize, usize, Vec<String>)>, ApplyPatchError> {
let mut replacements: Vec<(usize, usize, Vec<String>)> = Vec::new();
) -> std::result::Result<Vec<Replacement>, ApplyPatchError> {
let mut replacements: Vec<Replacement> = Vec::new();
let mut line_index: usize = 0;

for chunk in chunks {
Expand Down Expand Up @@ -756,11 +750,11 @@ fn compute_replacements(
// Attempt to locate the `old_lines` verbatim within the file. In many
// real‑world diffs the last element of `old_lines` is an *empty* string
// representing the terminating newline of the region being replaced.
// This sentinel is not present in `original_lines` because we strip the
// trailing empty slice emitted by `split('\n')`. If a direct search
// fails and the pattern ends with an empty string, retry without that
// final element so that modifications touching the end‑of‑file can be
// located reliably.
// This sentinel is not present in `original_lines` because `SourceFile`
// stores the terminator on the preceding line rather than as an extra
// trailing element. If a direct search fails and the pattern ends with
// an empty string, retry without that final element so modifications
// touching the end‑of‑file can be located reliably.

let mut pattern: &[String] = &chunk.old_lines;
let mut found =
Expand All @@ -785,7 +779,34 @@ fn compute_replacements(
}

if let Some(start_idx) = found {
replacements.push((start_idx, pattern.len(), new_slice.to_vec()));
// Context lines occur in both sides of a patch chunk. Keep those
// original lines in place so their exact contents and terminators
// survive, especially when the file has mixed line endings.
let mut old_start = 0;
let mut new_start = 0;
for &(old_context, new_context) in &chunk.context_line_indices {
// A trailing empty context line can be removed from `pattern`
// and `new_slice` above when it represents the final newline.
if old_context >= pattern.len() || new_context >= new_slice.len() {
break;
}
if old_start != old_context || new_start != new_context {
replacements.push((
start_idx + old_start,
old_context - old_start,
new_slice[new_start..new_context].to_vec(),
));
}
old_start = old_context + 1;
new_start = new_context + 1;
}
if old_start != pattern.len() || new_start != new_slice.len() {
replacements.push((
start_idx + old_start,
pattern.len() - old_start,
new_slice[new_start..].to_vec(),
));
}
line_index = start_idx + pattern.len();
} else {
return Err(ApplyPatchError::ComputeReplacements(format!(
Expand All @@ -801,34 +822,6 @@ fn compute_replacements(
Ok(replacements)
}

/// Apply the `(start_index, old_len, new_lines)` replacements to `original_lines`,
/// returning the modified file contents as a vector of lines.
fn apply_replacements(
mut lines: Vec<String>,
replacements: &[(usize, usize, Vec<String>)],
) -> Vec<String> {
// We must apply replacements in descending order so that earlier replacements
// don't shift the positions of later ones.
for (start_idx, old_len, new_segment) in replacements.iter().rev() {
let start_idx = *start_idx;
let old_len = *old_len;

// Remove old lines.
for _ in 0..old_len {
if start_idx < lines.len() {
lines.remove(start_idx);
}
}

// Insert new lines.
for (offset, new_line) in new_segment.iter().enumerate() {
lines.insert(start_idx + offset, new_line.clone());
}
}

lines
}

/// Intended result of a file update for apply_patch.
#[derive(Debug, Eq, PartialEq)]
pub struct ApplyPatchFileUpdate {
Expand Down
23 changes: 22 additions & 1 deletion codex-rs/apply-patch/src/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ impl Hunk {
#[cfg(test)]
use Hunk::*;

#[derive(Debug, PartialEq, Clone)]
#[derive(Debug, Default, PartialEq, Clone)]
pub struct UpdateFileChunk {
/// A single line of context used to narrow down the position of the chunk
/// (this is usually a class, method, or function definition.)
Expand All @@ -122,11 +122,26 @@ pub struct UpdateFileChunk {
pub old_lines: Vec<String>,
pub new_lines: Vec<String>,

/// Pairs of indices into `old_lines` and `new_lines` that identify lines
/// parsed as context rather than inferred to be equal by their contents.
pub(crate) context_line_indices: Vec<(usize, usize)>,

/// If set to true, `old_lines` must occur at the end of the source file.
/// (Tolerance around trailing newlines should be encouraged.)
pub is_end_of_file: bool,
}

impl UpdateFileChunk {
/// Adds a context line to both sides while recording its corresponding
/// indices so it remains distinguishable from identical changed lines.
pub(crate) fn push_context_line(&mut self, line: String) {
self.context_line_indices
.push((self.old_lines.len(), self.new_lines.len()));
self.old_lines.push(line.clone());
self.new_lines.push(line);
}
}

pub fn parse_patch(patch: &str) -> Result<ApplyPatchArgs, ParseError> {
let mode = if PARSE_IN_STRICT_MODE {
ParseMode::Strict
Expand Down Expand Up @@ -345,6 +360,7 @@ fn test_parse_patch() {
change_context: Some("def f():".to_string()),
old_lines: vec![" pass".to_string()],
new_lines: vec![" return 123".to_string()],
context_line_indices: vec![],
is_end_of_file: false
}]
}
Expand Down Expand Up @@ -372,6 +388,7 @@ fn test_parse_patch() {
change_context: None,
old_lines: vec![],
new_lines: vec!["line".to_string()],
context_line_indices: vec![],
is_end_of_file: false
}],
},
Expand Down Expand Up @@ -402,6 +419,7 @@ fn test_parse_patch() {
change_context: None,
old_lines: vec!["import foo".to_string()],
new_lines: vec!["import foo".to_string(), "bar".to_string()],
context_line_indices: vec![(0, 0)],
is_end_of_file: false,
}],
}]
Expand All @@ -422,6 +440,7 @@ fn test_parse_patch_preserves_end_of_file_marker() {
change_context: None,
old_lines: Vec::new(),
new_lines: vec!["quux".to_string()],
context_line_indices: vec![],
is_end_of_file: true,
}],
}],
Expand Down Expand Up @@ -470,6 +489,7 @@ fn test_parse_patch_accepts_relative_and_absolute_hunk_paths() {
change_context: None,
old_lines: vec!["old".to_string()],
new_lines: vec!["new".to_string()],
context_line_indices: vec![],
is_end_of_file: false
}]
},
Expand Down Expand Up @@ -548,6 +568,7 @@ fn test_parse_patch_lenient() {
change_context: None,
old_lines: vec!["import foo".to_string()],
new_lines: vec!["import foo".to_string(), "bar".to_string()],
context_line_indices: vec![(0, 0)],
is_end_of_file: false,
}],
}];
Expand Down
54 changes: 17 additions & 37 deletions codex-rs/apply-patch/src/streaming_parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,22 +274,15 @@ impl StreamingPatchParser {
}

if update_line == EMPTY_CHANGE_CONTEXT_MARKER {
chunks.push(UpdateFileChunk {
change_context: None,
old_lines: Vec::new(),
new_lines: Vec::new(),
is_end_of_file: false,
});
chunks.push(UpdateFileChunk::default());
self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number };
return Ok(());
}

if let Some(change_context) = update_line.strip_prefix(CHANGE_CONTEXT_MARKER) {
chunks.push(UpdateFileChunk {
change_context: Some(change_context.to_string()),
old_lines: Vec::new(),
new_lines: Vec::new(),
is_end_of_file: false,
..UpdateFileChunk::default()
});
self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number };
return Ok(());
Expand All @@ -313,46 +306,29 @@ impl StreamingPatchParser {

if line.is_empty() {
if chunks.is_empty() {
chunks.push(UpdateFileChunk {
change_context: None,
old_lines: Vec::new(),
new_lines: Vec::new(),
is_end_of_file: false,
});
chunks.push(UpdateFileChunk::default());
}
if let Some(chunk) = chunks.last_mut() {
chunk.old_lines.push(String::new());
chunk.new_lines.push(String::new());
chunk.push_context_line(String::new());
}
self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number };
return Ok(());
}

if let Some(line_to_add) = line.strip_prefix(' ') {
if chunks.is_empty() {
chunks.push(UpdateFileChunk {
change_context: None,
old_lines: Vec::new(),
new_lines: Vec::new(),
is_end_of_file: false,
});
chunks.push(UpdateFileChunk::default());
}
if let Some(chunk) = chunks.last_mut() {
chunk.old_lines.push(line_to_add.to_string());
chunk.new_lines.push(line_to_add.to_string());
chunk.push_context_line(line_to_add.to_string());
}
self.state.mode = StreamingParserMode::UpdateFile { hunk_line_number };
return Ok(());
}

if let Some(line_to_add) = line.strip_prefix('+') {
if chunks.is_empty() {
chunks.push(UpdateFileChunk {
change_context: None,
old_lines: Vec::new(),
new_lines: Vec::new(),
is_end_of_file: false,
});
chunks.push(UpdateFileChunk::default());
}
if let Some(chunk) = chunks.last_mut() {
chunk.new_lines.push(line_to_add.to_string());
Expand All @@ -363,12 +339,7 @@ impl StreamingPatchParser {

if let Some(line_to_remove) = line.strip_prefix('-') {
if chunks.is_empty() {
chunks.push(UpdateFileChunk {
change_context: None,
old_lines: Vec::new(),
new_lines: Vec::new(),
is_end_of_file: false,
});
chunks.push(UpdateFileChunk::default());
}
if let Some(chunk) = chunks.last_mut() {
chunk.old_lines.push(line_to_remove.to_string());
Expand Down Expand Up @@ -445,6 +416,7 @@ mod tests {
change_context: None,
old_lines: vec!["old".to_string()],
new_lines: vec!["new".to_string()],
context_line_indices: vec![],
is_end_of_file: false,
}],
}])
Expand Down Expand Up @@ -635,12 +607,14 @@ mod tests {
change_context: None,
old_lines: vec!["old a".to_string(), "*** Update File: b.txt".to_string()],
new_lines: vec!["new a".to_string(), "*** Update File: b.txt".to_string()],
context_line_indices: vec![(1, 1)],
is_end_of_file: false,
},
UpdateFileChunk {
change_context: None,
old_lines: vec!["old b".to_string()],
new_lines: vec!["new b".to_string()],
context_line_indices: vec![],
is_end_of_file: false,
},
],
Expand Down Expand Up @@ -680,6 +654,7 @@ mod tests {
String::new(),
"context after".to_string(),
],
context_line_indices: vec![(0, 0), (1, 1), (2, 2)],
is_end_of_file: false,
}],
}])
Expand All @@ -700,6 +675,7 @@ mod tests {
change_context: None,
old_lines: Vec::new(),
new_lines: vec!["quux".to_string()],
context_line_indices: vec![],
is_end_of_file: true,
}],
}])
Expand All @@ -718,6 +694,7 @@ mod tests {
change_context: None,
old_lines: vec!["old".to_string()],
new_lines: vec!["new".to_string()],
context_line_indices: vec![],
is_end_of_file: false,
}],
}])
Expand All @@ -733,6 +710,7 @@ mod tests {
change_context: None,
old_lines: vec!["old\r".to_string()],
new_lines: vec!["new".to_string()],
context_line_indices: vec![],
is_end_of_file: false,
}],
}])
Expand Down Expand Up @@ -769,6 +747,7 @@ mod tests {
change_context: None,
old_lines: vec!["old".to_string()],
new_lines: vec!["new".to_string()],
context_line_indices: vec![],
is_end_of_file: false,
}],
}])
Expand All @@ -782,6 +761,7 @@ mod tests {
change_context: None,
old_lines: vec!["old".to_string()],
new_lines: vec!["new".to_string()],
context_line_indices: vec![],
is_end_of_file: false,
}],
}])
Expand Down
Loading
Loading