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

Support patches without a prefix by determining the prefix depth #10885

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

Leonidas-from-XIV
Copy link
Collaborator

This reads the prefix depth from the file. We currently only support only 1 but this adds support for 0 which the regex already supported.

For arbitrary depth prefixes this would require a more complex parsing regex but also these are presumably rather rare in the real world.

@Leonidas-from-XIV Leonidas-from-XIV changed the title fix: Support patches without a prefix by determining the prefix depth Support patches without a prefix by determining the prefix depth Sep 5, 2024
let* new_file = Re.Group.get_opt group 2 in
let* old_prefix = Re.Group.get_opt group 1 in
let* old_file = Re.Group.get_opt group 2 in
let* new_prefix = Re.Group.get_opt group 3 in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a prefix always a relative path?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question and I never thought about it so tried what the tools do. diff -u will gladly generate a patch with an absolute path:

--- /tmp/diffy/old	2024-09-06 09:48:02.146670822 +0200
+++ /tmp/diffy/new	2024-09-06 09:48:10.369718133 +0200
@@ -1 +1 @@
-This is wrong.
+This is right.

However if I have the files at the locations and use patch -p0 it will refuse to apply the patch:

$ patch -p0 < absolute.patch
Ignoring potentially dangerous file name /tmp/diffy/old
Ignoring potentially dangerous file name /tmp/diffy/new
can't find file to patch at input line 3
Perhaps you used the wrong -p or --strip option?

If I instead call it from /tmp/diffy and strip the 2 components it will still not do it correctly:

$ patch -p2 < absolute.patch
can't find file to patch at input line 3
Perhaps you used the wrong -p or --strip option?

However, if I turn the absolute path /tmp/diffy/... into a relative tmp/diffy/... it applies the patch correctly with -p2 so I assume that patch can only handle relative paths.

In our case the only prefixes that the regex can ever match are "", "a/" and "b/" but the calculation function is a little bit more robust.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then I think we can assume only a relative path is valid then. what about a trailing slash on the relative path, is that accepted? Also if trailing slashes are allowed, how is just / interpreted?

Reason why I'm asking is that I'm not sure using string operations on slashes is the robust approach here. I would much prefer to see things converted to Path.Local.t and operated on using path functions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason why I'm asking is that I'm not sure using string operations on slashes is the robust approach here. I would much prefer to see things converted to Path.Local.t and operated on using path functions.

Yes you're right, that is nicer. I've converted it to use Path.Local functions, there are some pretty useful ones to deal with this exact case. I've moved the handling of the prefix from the regex to the OCaml code, that way the prefix handling is in one place.

Signed-off-by: Marek Kubica <marek@tarides.com>
@Leonidas-from-XIV Leonidas-from-XIV merged commit b3bd89f into ocaml:main Sep 9, 2024
27 checks passed
@Leonidas-from-XIV Leonidas-from-XIV deleted the prefixless-patches branch September 9, 2024 13:42
anmonteiro pushed a commit to anmonteiro/dune that referenced this pull request Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants