Closed twrecked closed 4 years ago
It's about several years since I last touched this function, so I'm a bit fuzzy on how strict we should be here. I just remember that the original intent was to be err on the conservative side.
Some things I notice:
%r{\A\*([^*]+)\z}
) you propose is going to match things with slashes as well; eg. *a/b/c
would match that; */foo
would also. Despite the comment saying that it matches *something (match file with extension without dot at any level)
; there's nothing in there that stops it from matching a dot as far as I can see.Regexp.escape($~[1]) + '\z'
uses the first capturing group (ie. [^*]+
), which will have grabbed "anything but a star, repeated", at the end of the string; so the result can effectively match "pretty much anything at all", which might be a bit too general.'(\A|/)'
at the start like most of the other branches do, the resulting pattern won't be anchored to the start of the path or a /
within the path. So, if your 'wildignore'
is *~
, our regex ends up being /~\z/
, which will work for your use case, obviously, but may not work so well for other patterns that end up matching this elsif
. For example, if my 'wildignore'
is *snapshot
, I might expect that to ignore files like my-snapshot
but not something/snapshot
, but the resulting regex (/snapshot\z/
) will ignore both.If your goal is to ignore files of the form *~
(ie. ending with a tilde) or really any *suffix
, I would have expected something like this:
if pattern.match(%r{\A\*\([^*/.]+)\z})
# *something (match file with suffix at any level)
'(\A|/)[^/]+' + Regexp.escape($~[1]) + '\z'
(Typed in GitHub, so may be wrong.)
Your are absolutely correct, I really just wanted to trap *something.
I'll tighten it up and test it again.
Thanks for your input. And thanks for the plugin.
I just pushed a new commit that fixes the issues you brought up.
*
that is not followed by .
it also can't include any /
or *
characters./
in front of it.Thanks @twrecked. I think this is probably all right now. I'm going to merge it with this additional tweak on top:
diff --git a/ruby/command-t/lib/command-t/vim.rb b/ruby/command-t/lib/command-t/vim.rb
index d8bcffe..f01bda4 100644
--- a/ruby/command-t/lib/command-t/vim.rb
+++ b/ruby/command-t/lib/command-t/vim.rb
@@ -81,7 +81,7 @@ def wildignore_to_regexp(str)
'\.' + Regexp.escape($~[1]) + '\z'
elsif pattern.match(%r{\A\*([^/*.][^/*]*)\z})
# *something (match file with extension without front dot at any level)
- '[^/]+' + Regexp.escape($~[1]) + '\z'
+ '(\A|/)' + Regexp.escape($~[1]) + '\z'
elsif pattern.match(%r{\A\*/([^/]+)/\*\z})
# */something/* (match directories at any level)
'(\A|/)' + Regexp.escape($~[1]) + '/.+'
In part for consistency, but also because I think [^/]+
is a bit less precise that the (\A|/)
that the other branches use. [^/]+
says "match 1 or more non-slash things" then the rest of the pattern, which means your asking the program to match something without being fully explicit about what that "thing" is; it will work in practice most of the time, I think, but it's still not as direct and literal a translation of your intent as saying "match the pattern at the beginning or after a slash", which is what the other branches do.
Ok, tested it a little bit more and what I said in the last comment isn't quite accurate — it needs to be something more like what I originally suggested (ie. '(\A|/)[^/]+'
). Having said that, I think we can go simpler than that. I'll push a commit that explains it.
This fix lets wildcards without a period - ie,
*~
, Vim's default back up extension - to be hidden from file lists.