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.

Sign up for GitHub

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

Jump to bottom

Glob: Make double asterisk match zero or more directories #192

Merged
merged 4 commits into from
Jul 28, 2021
Merged

Glob: Make double asterisk match zero or more directories #192

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
690a002
Glob: match zero or more directories
eWert-Online Jul 7, 2021
b415281
rewrite with new lookahead function
eWert-Online Jul 28, 2021
04d5c84
change name to read_ahead
eWert-Online Jul 28, 2021
c4180d7
Update CHANGES
rgrinberg Jul 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions lib/glob.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,23 +75,30 @@ let of_string ~double_asterisk s : t =
in

let piece () =
if read '*'
then if double_asterisk && read '*'
then ManyMany
else Many
if read '/' then
if read '*' then
if double_asterisk && read '*' then
if eos () then [ManyMany; Exactly('/')]
else [ManyMany]
else [Many; Exactly('/')]
else [Exactly('/')]
else if read '*'
then if double_asterisk && read '*'
then [ManyMany]
else [Many]
Copy link
Member

Choose a reason for hiding this comment

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

Indeed this is confusing. How about we just keep one clause for double asterisk and detect the trailing ** case by checking that s.[!i - 2] = /

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is possible, because then the / is already added as an Exactly('/') piece.
The idea is, to not add the / as a piece, when a ** follows.

Copy link
Member

@rgrinberg rgrinberg Jul 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Alright, but I still think what is problematic is that we are spreading the double asterisk clauses across so many branches. How about if we introduce the following primitive in addition to read:

(** [read_prefix prefix] will attempt to read [prefix] and will return [true] if it was successful.
   If it fails, it will not increment the read index. *)
val read_prefix : string -> bool

Then the code can be written as:

if double_asterisk && read_prefix "/**" then
   ..
else if read_prefix "**" then ..
 (* etc *)

Do you think this will help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, this is much cleaner now!

I decided to name the function lookahead instead of read_prefix, as the word "prefix" for me suggests looking back from the current position and not ahead.
Would be fine with renaming it though, if you don't like "lookahead".

Copy link
Member

Choose a reason for hiding this comment

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

The problem with the name lookahead is that it is reserved for peeking into available characters but not consuming it. Your lookahead function does the consuming. read_prefix isn't that great either, so feel free to come up with another name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of read_ahead?

else if read '?'
then One
then [One]
else if not (read '[')
then Exactly (char ())
then [Exactly (char ())]
else if read '^' || read '!'
then Any_but (enclosed ())
else Any_of (enclosed ())
then [Any_but (enclosed ())]
else [Any_of (enclosed ())]
in

let rec loop pieces =
if eos ()
then List.rev pieces
else loop (piece () :: pieces)
else loop (piece () @ pieces)
in

loop []
Expand Down
10 changes: 10 additions & 0 deletions lib_test/test_glob.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,16 @@ let _ =
assert (re_match (glob ~anchored "/**") "//foo");
assert (re_match (glob ~anchored "**") "foo//bar");

assert (re_match (glob ~anchored "foo/bar/**/*.ml") "foo/bar/baz/foobar.ml");
assert (re_match (glob ~anchored "foo/bar/**/*.ml") "foo/bar/foobar.ml");

assert (re_match (glob ~anchored "foo/**/bar/**/baz") "foo/bar/baz");
assert (re_match (glob ~anchored "foo/**/bar/**/baz") "foo/bar/x/y/z/baz");
assert (re_match (glob ~anchored "foo/**/bar/**/baz") "foo/x/y/z/bar/baz");
assert (re_match (glob ~anchored "foo/**/bar/**/baz") "foo/bar/x/bar/x/baz");
assert (re_mismatch (glob ~anchored "foo/**/bar/**/baz") "foo/bar/../x/baz");
assert (re_mismatch (glob ~anchored "foo/**/bar/**/baz") "foo/bar/./x/baz");

(* Interaction with [~period] *)
let period = true in
assert (re_mismatch (glob ~anchored ~period "**") ".foobar");
Expand Down