nacitar / a.vim

Alternate Files quickly (.c --> .h etc)
http://www.vim.org/scripts/script.php?script_id=31
19 stars 8 forks source link

:A fails when previously-opened alternate not co-located and filename is suffix of another alternate #8

Open brombres opened 6 months ago

brombres commented 6 months ago

Using a.vim from commit f3cfbbf:

Steps to Reproduce

brombres commented 6 months ago

Here's a fix. I changed line 679 as follows.

OLD:

let bufName = bufname(bufFilename)

NEW:

let bufName = bufname("\\(^\\|\\/\\)".bufFilename)

Description of the fix for anyone interested: bufname() is a built-in Vimscript function that attempts to locate a buffer using the specified filename as a pattern. I've added (^|/) to the beginning to force the match to be at the beginning of the filename or after a / path separator. That is escaped in magic regex mode as \(^\|\/\), and each \ is escaped again when written as a literal string.

. is the original string concatenation operator; the more modern .. could be used instead.

nacitar commented 6 months ago

Just to be clear, this isn't my project and I haven't even bothered to try to read through and understand the code really. I could, but I haven't had the interest. This repository is just from me finding out that github had a network/fork graph and I was able to see all the forks people had made and what changes they had.... and I just merged all of those forks into this (with some care checking if it made sense to do so) because there doesn't seem to be an actual maintainer for this anymore.

That said, I saw your bug ticket and was planning on (if my mood permitted) making some time this weekend to possibly look into this given that a number of people seem to be using my repository now (to my surprise).

Looking at your change though, I question if it's correct. I haven't tested it, but I figured I'd simply ask you about it.

Your new code contains this: \\(^\\|\\/\\)

If in vim I run: :echom "\\(^\\|\\/\\)"

It gives me this: \(^\|\/\)

That regular expression doesn't seem to my (non-vim-regex) eyes to be what you described.

That looks like you're matching a literal lparen, literal pipe, literal rparen, and are unnecessarily escaping a /.

The regex you mentioned in your comment though, does seem to logically make sense to solve the problem (assuming linux filesystem, as vim would naturally encounter): (^|/)

So if you can explain to me how your regex is equivalent to that, or link me to something that explains it well enough for me to understand as someone who is an experienced developer that just hasn't done much in vimscript, then all it will take for me to incorporate your change is for you to make a PR and I'll gladly accept it.

Apparently enough people are using this fork now that it leads me to worry about breaking things... so just taking some care.

Thanks for following up, too.

brombres commented 5 months ago

RE: maintainer - yeah, I found your repo via a comment on the original repo. I've been using Vi(m) for 34 years and yet a.vim is one of only two plugins I use (greplace.vim being the other), so I appreciate you maintaining it in some form.

Vim operates in "magic" regex mode by default. I don't know why it's called that because it doesn't seem all that magical to me. But it basically means that (...|...) groupings must be escaped to \(...\|...\), along with / as \/. :help magic describes magic mode, and :help bufname mentions that bufname() implicitly works in magic mode.

You can test the validity of the magic search pattern using a standard search within Vim. For example, if the current text buffer contains this:

AlphaBeta.h
Beta.cpp
include/Beta.h

If we want to check and see if Beta.h exists, the search /Beta.h (where the leading / is the actual Vim search command) finds a match on the line AlphaBeta.h as well as the line include/Beta.h, which is the problem with how a.vim's existing bufname() search is working - there are multiple matches so it returns empty string.

Then the following search will match Beta.h but not AlphaBeta.h: /\(^\|\/\)Beta.h. This is the magic-escaped form of /(^|/)Beta.h, but the unescaped form doesn't work. If you're not convinced, try recreating the simple directory structure from my first comment and you'll see that only the escaped (well, double-escaped) version works right.

Today I tested the fix on Windows and Linux as well as macOS. Turns out it doesn't work for Windows because of Windows' backslashes. I thought it would be easy enough to expand the pattern, but that wasn't the case.

In theory we just need to match ^, /, and \, which would be (^|/|\\) in non-magic mode. Magic mode escapes would be \(^\|\/\|\\\) (the \\ in magic mode stays as \\) and then the string-escaped version of the magic-escaped version would be \\(^\\|\\/\\|\\\\\\). Problem is , that still works fine on macOS and Linux but not on Windows! On Windows, the \\\\ needs to be just \\ (\\(^\\|\\/\\|\\\\)), but that somehow breaks functionality on macOS + Linux.

I also tried using [\x5c] instead of \, but that didn't work at all.

My solution was to match beginning-of-word or not-(any valid filename character). It's kludgy but I can't see another way:

let bufName = bufname("\\(^\\|[^-+A-Za-z0-9_ ,!#$%&'();=@^`]\\)".bufFilename)

There are a couple of valid filename characters I couldn't get to work right in the regex, especially . (period). That's fine; it just means that a.vim would be unable to distinguish between OtherFolder/.Beta.h and OtherFolder/Beta.h. It currently can't distinguish between them anyway, so there's no loss of functionality.

I'll continue to use this proposed update for a few days to make sure everything's working nicely and then I'll submit a PR.

Thanks!