Skip to content

YETUS-983. test-patch confused by multiple commits that include rename#384

Open
ndimiduk wants to merge 3 commits intoapache:mainfrom
ndimiduk:yetus-983-diff-preference
Open

YETUS-983. test-patch confused by multiple commits that include rename#384
ndimiduk wants to merge 3 commits intoapache:mainfrom
ndimiduk:yetus-983-diff-preference

Conversation

@ndimiduk
Copy link
Copy Markdown
Member

@ndimiduk ndimiduk commented May 8, 2026

Flip dryrun_both_files to prefer cumulative .diff over per-commit .patch. The cumulative diff represents the PR's net change, avoiding stale files left on disk when format-patch stanzas add then rename/delete the same file.

Generate the local diff with git diff --binary to preserve binary file handling that GitHub's API .diff endpoint strips. Fall back to the API .diff when local generation fails.

Flip dryrun_both_files to prefer cumulative .diff over per-commit
.patch. The cumulative diff represents the PR's net change, avoiding
stale files left on disk when format-patch stanzas add then
rename/delete the same file.

Generate the local diff with git diff --binary to preserve binary
file handling that GitHub's API .diff endpoint strips. Fall back to
the API .diff when local generation fails.
@ndimiduk
Copy link
Copy Markdown
Member Author

ndimiduk commented May 8, 2026

Okay I think I tracked this down. Looks like git apply processes per-commit format-patch stanzas sequentially against the working tree, but each stanza's precondition check assumes the file is in its pre-stanza state — not the state left by earlier stanzas. When a file is created in commit 1, modified in commit 2, and deleted in commit 3, the file persists on disk despite the apply claiming success. git apply --check (dry-run) also passes, because it validates stanzas independently rather than simulating the cumulative effect (using MacOS git 2.50.1). I added a bats test that I think reproduces.

The cumulative git diff output doesn't have this problem — it represents the net change from base to HEAD as a single stanza per file. A file that was added and later deleted simply doesn't appear. A file that was added and later renamed appears only at its final path.

In the patch I swap the logic of dryrun_both_files to prefer the cumulative .diff over the per-commit .patch. The .patch is kept as a fallback.

@ndimiduk ndimiduk force-pushed the yetus-983-diff-preference branch 3 times, most recently from d96abd1 to cc0929f Compare May 8, 2026 09:49
@ndimiduk ndimiduk force-pushed the yetus-983-diff-preference branch from cc0929f to 6a8a8ce Compare May 8, 2026 10:01
@aw-was-here
Copy link
Copy Markdown
Contributor

If I remember correctly, the problem was that the diff format didn't support binary files so tests and the like would be missing content.

@ndimiduk
Copy link
Copy Markdown
Member Author

ndimiduk commented May 8, 2026

If I remember correctly, the problem was that the diff format didn't support binary files so tests and the like would be missing content.

That's what I thought. Looks like the world has changed since then, at least, I think that's what my new test is showing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants