Skip to content
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.

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

Add a ~p parameter to Patch.parse mimicking the behaviour of patch -p<num> #9

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

kit-ty-kate
Copy link
Collaborator

@kit-ty-kate kit-ty-kate commented Mar 20, 2024

PR on top of #20

This is required when dealing with both git and GNU diffs. The user of the library cannot know which name was there to begin with an cannot eliminate the prefix in the filenames accordingly.

Requiring a ~p parameter allows to give all the filenames cleanly and also to mimic the behaviour from GNU patch:

If the header is that of a context diff, patch takes the old and new file names in the header.
A name is ignored if it does not have enough slashes to satisfy the -pnum or --strip=num op‐
tion. The name /dev/null is also ignored.

@hannesm
Copy link
Owner

hannesm commented Mar 21, 2024

Thanks, this looks great. I force-pushed (rebased on main now that #7 is merged), and added a commit which fixes the tests.

The only question I have is about "rename_only", namely in test/git2.diff:

diff --git a/git2.old b/git2.new
similarity index 100%
rename from git2.old
rename to git2.new

And here our header / parser now returns git2.old / git2.new -- without the a and b. Is this a good solution? My worry is that, since now we support ~p and also remove the hard-coded "if diff is originating from git, drop the a/ / b/", we end up in an inconsistent state (in the rename_only case). Can we assume that rename from / rename to always originates from git, and add a a/ / b/ there? Or is there another path we should follow?

@hannesm
Copy link
Owner

hannesm commented Mar 21, 2024

I added another commit, please let me know if this works fine for you (esp. the rename case). I think this is good.

@kit-ty-kate kit-ty-kate marked this pull request as draft March 27, 2024 16:28
src/patch.ml Outdated Show resolved Hide resolved
@kit-ty-kate kit-ty-kate mentioned this pull request May 7, 2024
@kit-ty-kate kit-ty-kate marked this pull request as ready for review October 7, 2024 16:11
@kit-ty-kate kit-ty-kate requested a review from hannesm October 7, 2024 16:12
@kit-ty-kate kit-ty-kate changed the title Add a ~p parameter to Patch.to_diffs mimicking the behaviour of patch -p<num> Add a ~p parameter to Patch.parse mimicking the behaviour of patch -p<num> Oct 7, 2024
@hannesm
Copy link
Owner

hannesm commented Oct 7, 2024

thanks @kit-ty-kate, API-wise this looks fine to me, and is an appreciated cleanup.

@kit-ty-kate
Copy link
Collaborator Author

I fixed the behaviour with adjacent slashes (per GNU patch's behaviour) and added some tests. Merging

@kit-ty-kate kit-ty-kate merged commit c1a631f into hannesm:main Oct 7, 2024
1 check passed
@kit-ty-kate kit-ty-kate deleted the p1 branch October 7, 2024 16:45
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