bling / fzf.el

A front-end for fzf
GNU General Public License v3.0
364 stars 49 forks source link

Fix files with colons #51

Closed neojski closed 1 year ago

neojski commented 4 years ago

fzf/start was not working for files that had colons in them because it was incorrectly applying split on : to make fzf-git-grep work. This change moves the fzf-git-grep-related code to the fzf-git-grep instead of polluting fzf/start. As a side-effect of this change fzf/start now works for files with : in the name.

pierre-rouleau commented 1 year ago

@neojski Question: are the files names with colon in them files in Windows or also in Unix systems? If the files with embedded colons are in Unix, what type of files were they?

neojski commented 1 year ago

The issue is about files with colon in the name not in the contents.

You can just create such file with touch 'test:file'

pierre-rouleau commented 1 year ago

@neojski Sorry I was not clear enough. I understood that you meant colon inside the name of the file. I am just curious as to why you would use a file name with an embedded colon. At first I though it might have been a Windows drive specifier like in C:/Program File but you not referring to Windows. Historically several ASCII printable characters have been avoided in file names as they were previously not allowed in some OS (like ':' in Windows file names) and because lots of software had problem dealing with them. Space character and several Unicode characters are affected. Therefore my question as to why you're interested by using an ASCII colon character in a file name, just to see if there are use trends for it.

Of course that is orthogonal to the fact that fzf.el does not deal with it as it expects it not be present and uses a generally non-problematic convention.

neojski commented 1 year ago

I work with files that were not necessarily created by me. Let's say a massive monorepo that has all sorts of files with colons, spaces, or non-ascii characters.

I know a workaround for this problem would be to rename those files but that may have far reaching consequences, e.g. I'd have to convince the owners of those files that a tool I use doesn't work with colons and so they should change their filename naming conventions. That would be much harder than fixing fzf.el.

It would be preferable if fzf.el supported all character permissible in the Unix filenames.

pierre-rouleau commented 1 year ago

@neojski I agree that fzf.el should allow their support. I'm still curious about the type of files where the names have ':' in them, but would understand if you can't disclose this.

pierre-rouleau commented 1 year ago

@neojski If you don't mind, I will integrate your ideas inside my fork to be able to bring these into the main. Being able to search for any Unix file is important.

neojski commented 1 year ago

The examples I know of aren't actually enlightening or interesting. It's some proprietary config files where someone chose : as a delimiter for a system name that consists of a few components, e.g. europe:instance:primary.config

Yeah, definitely don't mind if this gets solved in your fork! Appreciate you looking into this.

pierre-rouleau commented 1 year ago

@neojski I am getting ready to push a PR that solves the issue you identified.

To support the ability to search for files that have a name embedding colons I replaced the file name/line number extraction logic with Emacs Lisp regexp and match group. The extraction is identified by a 3-member list that contains a regexp and 2 integers identifying the match group inside the regexp for extracting the file name and the line number.

I have pushed the code inside my fork. I'm going to add ability to override the extracted file name validation as well.

If you have time to take a look to see if that solves your problem it's be nice. I tested the code with a file that had colon and was able to handle that fine.

Thanks

pierre-rouleau commented 1 year ago

@neojski Can you confirm that the latest code solves your problem with file that have colons in their names? I did not use the exact same code you used but I believe it addresses your issue.

neojski commented 1 year ago

Looks good, thank you!