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

Let wildcards without a period work. #369

Closed
wants to merge 2 commits into from
Closed

Conversation

twrecked
Copy link
Contributor

@twrecked twrecked commented May 1, 2020

This fix lets wildcards without a period - ie, *~, Vim's default back up extension - to be hidden from file lists.

@wincent
Copy link
Owner

wincent commented May 2, 2020

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:

  • The pattern (%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.
  • The regex built using 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.
  • Because you're not using '(\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.)

@twrecked
Copy link
Contributor Author

twrecked commented May 2, 2020

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.

Stop regex including *./ characters. Anchor search for narrower results.
@twrecked
Copy link
Contributor Author

twrecked commented May 4, 2020

I just pushed a new commit that fixes the issues you brought up.

  • The pattern now matches * that is not followed by . it also can't include any / or * characters.
  • The ignore match now has to have at least one character that isn't a / in front of it.

@wincent
Copy link
Owner

wincent commented May 9, 2020

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.

@wincent
Copy link
Owner

wincent commented May 9, 2020

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.

@wincent wincent closed this in d044bac May 9, 2020
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