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

Glob: do not skip over backslashes under Windows #197

Merged
merged 2 commits into from
Aug 25, 2021
Merged

Glob: do not skip over backslashes under Windows #197

merged 2 commits into from
Aug 25, 2021

Conversation

nojb
Copy link
Contributor

@nojb nojb commented Aug 23, 2021

(The first commit is just to fix the build following #192)

Currently Glob skips over backslashes, interpreting the character following one "literally". This is wrong on Windows, where backslashes are perfectly good characters in pathnames. This commit makes the "skip over backslash" logic only apply when not on Windows.

Note that there may be other fixes necessary to work well under Windows, but this is already an improvement.

This PR fixes (at least in some cases) the issue reported in ocaml-ppx/ocamlformat#1773

@nojb
Copy link
Contributor Author

nojb commented Aug 23, 2021

Note that a better fix would be to add an argument to glob function specifying how to interpret backslashes, which would allow matching Windows paths under Unix and vice-versa. Opinions welcome.

@nojb
Copy link
Contributor Author

nojb commented Aug 25, 2021

Friendly ping

@rgrinberg rgrinberg merged commit 6a32b81 into ocaml:master Aug 25, 2021
@rgrinberg
Copy link
Member

Thanks!

@nojb nojb deleted the glob_backslash branch August 26, 2021 08:22
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Sep 9, 2021
CHANGES:

* Glob: add optional argument `?backslash_escapes` to control interpretation of
  backslashes (useful under Windows) (ocaml/ocaml-re#197, ocaml/ocaml-re#198)

* Restore accidentally deleted `*_seq` deprecated aliases.
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