feat(patch_set): support git diff application by weihanglo · Pull Request #59 · bmwill/diffy

weihanglo · 2026-04-12T16:43:54Z

Add git diff output parsing and application support. Some hightlights:

The first few refactor commits add parse options for Patch, making skipping preamble and rejecting hunks configurable. Optional preamble skipping is required for git diff parsing because git diff patch may have only git headers without any unidiff portion, followed by the next diff --git header. If we still skip preamble, the next diff --git will be swallowed.
Binary patch is not yet supported, though we have skip and fail options which user can configure to either skip binary patch, or fail on encountering a binary patch. I plan to add a Keep enum variant when we add the support the binary patch parsing/application. (See https://github.com/weihanglo/diffy/blob/4fc7c4f0e4bc6db55da6a8eb92f4cdf20113225c/src/patches/parse.rs#L172-L197)

weihanglo · 2026-04-12T16:45:13Z

+            }
+        };
+
+        // FIXME: error spans point at `diff --git` line, not the specific offending line


Leave it for future, I am lazy again to write new code 🙇🏾‍♂️.

Yeah if there are explicit limitation or things that we know will be addressed in the future as long as we document the issue i'm fine with avoiding scope bloat on individual PRs

weihanglo · 2026-04-12T21:11:25Z

+            // Can't use `--numstat` for GitDiff: it shows `-\t-\t` for both
+            // actual binary diffs AND pure binary renames (100% similarity).
+            // Parser correctly handles pure renames (rename headers, no binary content).
+            //
+            // Use `--raw` for total count, subtract actual binary markers from diff.
+            //
+            // TODO: once `--binary` is passed above, count ALL `--raw`
+            // entries — every file will have patch data (delta, literal, or text).
+            let raw = git(repo, &["diff", "--raw", parent, child]);
+            let total = raw.lines().filter(|l| !l.is_empty()).count();
+            let type_changes = count_type_changes(&raw);
+            let binary = diff_output
+                .lines()
+                .filter(|l| l.starts_with("Binary files ") || l.starts_with("GIT binary patch"))
+                .count();
+            skipped += binary;
+            total - binary + type_changes


This is a partial port of https://github.com/weihanglo/diffy/blob/e50f9286b95671b0f131fb5ac4f5725933e78232/tests/replay.rs#L292-L304. We will remove the binary filter eventually to avoid duplicate the parsing logic in test when we add binary patch support.

`.lines()` strips line endings, so callers tracking byte offsets need to re-add the `\r\n` or `\n` length manually. Extract the repeated inline pattern into a reusable helper.

* Parse `diff --git` extended headers * split multi-file git diffs at `diff --git` boundaries

Compat test for also `git apply`.

Unlike unidiff, gitdiff produces patches for empty file creations/deletions (`0\t0` in numstat) because they carry `diff --git` + extended headers even without hunks. Binary files (`-\t-\t`) are skipped in gitdiff mode for now.

weihanglo · 2026-04-15T03:02:17Z

+    /// Skip binary diffs silently.
+    pub fn skip_binary(mut self) -> Self {
+        self.binary = Binary::Skip;
+        self
+    }


This is not good actually. We may want a binary marker like patch so people explicitly know they are skipping something

weihanglo · 2026-04-15T03:10:50Z

+/// * `a/<path> b/<path>` (default prefix)
+/// * `<path> <path>` (`git diff --no-prefix`)
+/// * `<src-prefix><path> <dst-prefix><path>` (custom prefix)
+/// * `"<prefix><path>" "<prefix><path>"` (quoted, with escapes)


We should add more compat tests around this logic.

weihanglo · 2026-04-15T03:14:28Z

+        return Ok(FileOperation::Rename {
+            from: Cow::Borrowed(from),
+            to: Cow::Borrowed(to),
+        });
+    }
+    if let (Some(from), Some(to)) = (header.copy_from, header.copy_to) {
+        return Ok(FileOperation::Copy {
+            from: Cow::Borrowed(from),
+            to: Cow::Borrowed(to),


We might return the quoted filename. This need to be fixed.

weihanglo · 2026-04-15T03:32:10Z

+fn extract_file_op_gitdiff<'a>(
+    header: &GitHeader<'a>,
+    patch: &Patch<'a, str>,
+) -> Result<FileOperation<'a>, PatchSetParseError> {


Pre-existing issue: This FileOperation preserves the raw path with prefix unstripped, e.g., a/src/lib.rs and b/src/lib.rs. I personally think this is the right chose on syntactic level and consumer should know their patch better than us. However, the API doc should call this out more explicitly.

Also note that we have yet supported non-UTF8 path even in #64.

Also note that we have yet supported non-UTF8 path

This was one other thing i was going to mention. How do you want to handle that? punt on that for a follow up PR?

Can also cherry pick whatever in #64, if that is preferred.

bmwill · 2026-04-15T19:27:26Z

+                Binary::Skip => {
+                    return self.next_gitdiff_patch();
+                }


This is recursive and some inputs could trivially overflow the stack. This should hopefully be fairly easy to restructure into a looping construct instead which can avoid the possibility of blowing the stack.

Or better, instead of silent skip, we always emit a binary marker patch.

#59 (comment)

bmwill · 2026-04-15T19:33:46Z

+        }
+        // Select split with longest common path suffix (matches Git behavior)
+        if let Some(path) = longest_common_path_suffix(left, right) {
+            if path.len() > longest_path.len() {


If there are multiple splits with the same length what does git do in those situations? as-is this will prefer the first one we encounter.

bmwill · 2026-04-15T19:36:14Z

+/// Path component boundary means:
+///
+/// * At `/` character (e.g., `foo/bar.rs` vs `fooo/bar.rs` → `bar.rs`)
+/// * Or the entire string is identical


The "entire string is identical" case takes care of when a and b have no/s correct?

bmwill · 2026-04-15T19:48:58Z

        // Strip email preamble once at construction
        let input = strip_email_preamble(input);


Not new in this change but I was just wondering how we would properly handle mbox streams (a concatenation of a bunch of email patchsets). Maybe worth adding a comment to comeback and address in a followup?

I remembered last time I tried, GNU patch and git apply both failed on this case. We now have compat test infra we can verify with some tests.

bmwill · 2026-04-15T19:50:26Z

+}
+
+/// See [`parse_diff_git_path`].
+fn parse_quoted_diff_git_path(line: &str) -> Option<(Cow<'_, str>, Cow<'_, str>)> {


Can we add a test for quote-within-quote eg diff --git "a/with\"quote"

bmwill · 2026-04-15T19:53:16Z

+        let end = loop {
+            match bytes.get(i)? {
+                b'"' => break i + 1,
+                b'\\' => i += 2,


Do we need to be concerned about full octal awareness here? If we don't then maybe we should add a comment indicating why its ok?

Octal escape \377 decodes to 0xFF, which is not valid UTF-8. When parsing into `Patch<'_, str>`, `convert_cow_to_str` panics via `unwrap()` instead of returning a parse error. This documents the pre-existing bug that the reviewer flagged: bmwill#59 (comment)

Octal escape \377 decodes to 0xFF, which is not valid UTF-8. When parsing into `Patch<'_, str>`, `convert_cow_to_str` panics via `unwrap()` instead of returning a parse error. See bmwill#59 (comment)

weihanglo · 2026-04-16T01:14:21Z

Just put this on hold and created #65 and #66 for refactoring/improving the existing stuff.

Octal escape \377 decodes to 0xFF, which is not valid UTF-8. When parsing into `Patch<'_, str>`, `convert_cow_to_str` panics via `unwrap()` instead of returning a parse error. See #59 (comment)

weihanglo force-pushed the gitdiff branch from b460c48 to 9ce8e55 Compare April 12, 2026 16:53

weihanglo commented Apr 12, 2026

View reviewed changes

weihanglo force-pushed the gitdiff branch 2 times, most recently from ffb8730 to 2515dd3 Compare April 12, 2026 21:08

weihanglo commented Apr 12, 2026

View reviewed changes

weihanglo mentioned this pull request Apr 13, 2026

feat(binary): add git binary patch support #61

Draft

weihanglo added 4 commits April 13, 2026 15:05

refactor(patch): ParseOpts for configurable parsing behavior

e5e4e84

refactor(patch): make preamble skipping configurable

1a83fe9

refactor(patch): organize item order

d1ec236

refactor(patch): wire up ParseOpts for byte parsing

fe9b1b3

weihanglo force-pushed the gitdiff branch 2 times, most recently from eaa47ff to 49376b2 Compare April 13, 2026 19:24

weihanglo added 6 commits April 14, 2026 00:15

refactor(patch): deduplicate parse_one logic

f4e6b2b

refactor(patch_set): extract line_ending_len helper

edf93e5

`.lines()` strips line endings, so callers tracking byte offsets need to re-add the `\r\n` or `\n` length manually. Extract the repeated inline pattern into a reusable helper.

feat(patch_set): support git diff application

90861e0

* Parse `diff --git` extended headers * split multi-file git diffs at `diff --git` boundaries

test(compat): add git apply compatibility tests

95836ec

Compat test for also `git apply`.

test(replay): add gitdiff mode support

ff392af

Unlike unidiff, gitdiff produces patches for empty file creations/deletions (`0\t0` in numstat) because they carry `diff --git` + extended headers even without hunks. Binary files (`-\t-\t`) are skipped in gitdiff mode for now.

chore(ci): run gitdiff also in replay workflow

e5895d9

weihanglo force-pushed the gitdiff branch from 49376b2 to e5895d9 Compare April 14, 2026 05:00

weihanglo commented Apr 15, 2026

View reviewed changes

bmwill reviewed Apr 15, 2026

View reviewed changes

weihanglo mentioned this pull request Apr 15, 2026

fix(patch): on non-UTF8 filename, error rather than panic #65

Merged

weihanglo marked this pull request as draft April 15, 2026 23:11

		// Strip email preamble once at construction
		let input = strip_email_preamble(input);

Conversation

weihanglo commented Apr 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weihanglo Apr 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weihanglo Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

weihanglo commented Apr 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

weihanglo commented Apr 12, 2026 •

edited

Loading

weihanglo Apr 12, 2026 •

edited

Loading

weihanglo Apr 15, 2026 •

edited

Loading