Glob: Make double asterisk match zero or more directories

[eWert-Online]

Fixes #185

I don't know, if this is a good implementation for the fix (I rather doubt it) but maybe it can be used as a starting point to further improve on.

As I already noted in the issue, the problem was, that the parser saw the /'s as explicitly set characters that needed to be present.
I am now checking, if the double asterisk has leading or trailing slashes and ignore them if there are any.
If the trailing slash is in the last place, I do not ignore it, so the test on line 94 does not fail.

If you have any suggestions for an implementation that is easier to understand / read, I would be happy to hear it 😄

[rgrinberg]

What about the case where ** is part of a path element? E.g. foo/**bar. If I understand the spec correctly, this should be treated as *? I think we want to test this behavior.

[rgrinberg]

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] = /

[eWert-Online]

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.

[rgrinberg]

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?

[eWert-Online]

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".

[rgrinberg]

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.

[eWert-Online]

What do you think of read_ahead?

[eWert-Online]

What about the case where ** is part of a path element? E.g. foo/**bar. If I understand the spec correctly, this should be treated as *? I think we want to test this behavior.

I think this behaviour is already tested here:

ocaml-re/lib_test/test_glob.ml

Line 99 in 690a002

assert (re_match (glob ~anchored "foo/**bar") "foo/far/oofbar");

...or do you mean another test like the following (which also passes currently)

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

[rgrinberg]

Thanks!

[nojb]

The project does not compile anymore after this PR (raise_no_trace should be raise_notrace).