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 highlights:

Binary diffs are always pared, and emit PatchKind::Binary marker. Callers decide
how to handle.
API are mostly generic over T: Text, since hunks and filename may contain non-UTF8 bytes. This shares the same limitation of Patch — if you use a PatchSet::parse for Patch<'_, str>, you cannot have non-UTF8 filename in diff --git extended header.

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-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

Following YAGNI, I removed skip and fail options. The new revision will always parse binary patches. If users don't want to parse binary patches, they shouldn't have generated binary patch with --binary in the first place. Anyway, this can be a future feature request and is not too hard.

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.

#65 has addressed and #66 fixes #64

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.

This one is interesting. If we have diff --git a/x b/x c/x, git-apply failed to apply

git diff header lacks filename information
when removing N leading pathname component(s)"

Also in https://git-scm.com/docs/diff-format#generate_patch_text_with_p:

The a/ and b/ filenames are the same unless rename/copy is involved.

This kinda tells git-apply's path resolution is strip-level-aware, unlike ours that picks the first one. I'll mark this as incompat in our compat tests. We can decide the actual behavior later.

Added tests

compat::git::fail_ambiguous_suffix_tie

compat::git::path_ambiguous_suffix

Though this is still not complete compatible with git-apply. Our parser is more lenient.

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?

The / boundary only matters for partial suffixes. Let me enhance the doc here.

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.

I misunderstood your comment. It should be fine with mbox stream concatenation, as we ignore trailing garbage when hunk is satisfied

Tests added. Working as expected. The behavior matches both reference tools (patches applied)

compat::git::format_patch_mbox

compat::gnu_patch::format_patch_mbox

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"

Tested added:

patch_set::tests::patchset_gitdiff::rename_both_quoted

patch_set::tests::patchset_gitdiff::rename_quoted_to_unquoted

patch_set::tests::patchset_gitdiff::rename_unquoted_to_quoted

patch_set::tests::patchset_gitdiff::path_quoted_with_escaped_quote (different location than rename)

compat::git::path_quoted_inner_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?

Comment added. Thanks for calling it out!

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 · 2026-04-17T05:04:34Z

This is ready for review.

Because we merged #66 so this is kind a huge rewrite, so I had no choice but rewrote the history. Each review comment should be addressed already. Most of the code logic didn't change.

weihanglo · 2026-04-17T05:17:55Z

+fn junk_between_hunks() {
+    Case::git("junk_between_hunks")
+        .strip(1)
+        .expect_compat(false)


This is incompatible with git-apply (diffy being more permissive)

weihanglo · 2026-04-17T05:19:30Z

+    Case::git("fail_ambiguous_suffix_tie")
+        .strip(1)
+        .expect_success(true)
+        .expect_compat(false)


We discussed this incompat in #59 (comment)

* 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.

bmwill#62 optimized it

bmwill · 2026-04-18T14:13:17Z

Awesome thank you so much!

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

Comment thread tests/replay.rs Outdated

weihanglo mentioned this pull request Apr 13, 2026

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

Merged

weihanglo force-pushed the gitdiff branch 3 times, most recently 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

Comment thread src/patch_set/parse.rs Outdated

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

weihanglo mentioned this pull request Apr 17, 2026

feat(patch_set): PatchSet::parse_bytes for raw byte input #64

Closed

weihanglo force-pushed the gitdiff branch 3 times, most recently from 5ce67af to 0a3c53d Compare April 17, 2026 04:50

weihanglo marked this pull request as ready for review April 17, 2026 05:04

weihanglo commented Apr 17, 2026

View reviewed changes

weihanglo force-pushed the gitdiff branch from 0a3c53d to 56939c5 Compare April 17, 2026 14:09

weihanglo added 2 commits April 17, 2026 10:18

feat(patch_set): git diff parsing/application

da27abf

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

test(compat): add git apply compatibility tests

da4cf13

Compat test for also `git apply`.

weihanglo added 3 commits April 17, 2026 10:18

test(replay): add gitdiff mode support

d4ff209

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

dfbf385

chore(ci): we can now replay more commits!

065d801

bmwill#62 optimized it

weihanglo force-pushed the gitdiff branch from 56939c5 to 065d801 Compare April 17, 2026 14:18

bmwill merged commit 52ee463 into bmwill:master Apr 18, 2026
18 checks passed

weihanglo deleted the gitdiff branch April 18, 2026 14:47

		// 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!

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!

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!

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!

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!

weihanglo commented Apr 16, 2026

Uh oh!

weihanglo commented Apr 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmwill commented Apr 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

weihanglo commented Apr 12, 2026 •

edited

Loading

weihanglo Apr 15, 2026 •

edited

Loading