writable_search.vim icon indicating copy to clipboard operation
writable_search.vim copied to clipboard

Plugin breaks if the filename contains a number range

Open pkrumins opened this issue 5 years ago • 17 comments

Hi Andrew,

Thanks for creating this great plugin!

I was just using it on a file with a filename readme-2000-2010-2.md and the plugin broke. It thinks that all edits should happen on line 2000.

I experimented some more and it works if the filename is readme-2000.md but breaks again if it's readme-2000-2010.md.

Also, if the pathname contains a range but the filename does not, then sometimes it breaks the same way. At the moment, it looks like it breaks if the pathname contains a range only if it's using git grep.

Apparently, if there's something that looks like a number range in the pathname or filename, then something goes wrong.

I hope you can replicate it and fix it. Thanks once again.

Peter

pkrumins avatar Feb 24 '20 21:02 pkrumins

Ah, parsing edge cases. The plugin tries to match grep output to figure out which part is a filename and which is a line number. I suppose it's unsurprising that it doesn't work that well for all cases.

I'll see what I can do to fix the situation. I've got some ideas, but I'll need to experiment.

AndrewRadev avatar Feb 25 '20 20:02 AndrewRadev

I've pushed some commits that should fix the issue. Essentially, the problem is that filename:42:line text is unambiguous, but file:name:42:line text is not so much. What I ended up doing is checking the possible combinations of : delimiters (and also - delimiters) and confirming that the parsed file exists, that it has a line that matches the parsed line, and that the contents are the correct ones.

Performance-wise, this shouldn't be a problem, although I admit it feels a bit like overkill. But there's no reliable way that I can see to get grep output in a more machine-readable way. Ripgrep has a --json flag that I should really play around with.

In any case, could you test it out now and confirm it works correctly?

AndrewRadev avatar Mar 22 '20 09:03 AndrewRadev

Hello Andrew,

Thanks for your work! I just started testing the new release but ran into something that looks like a completely new bug that wasn't there before.

Here's a screenshot that shows the new issue:

image

Here are the steps that I took that led to this new issue.

  1. I upgraded the previous writable-search version that I had cloned from this git repository on Feb 24 to today's master.

  2. To quickly create a directory with number ranges for testing, I copied writable-search directory to /tmp/writable-search-git-2020-04-16 and opened the README.md file in vim. When I ran the :WritableSearch the command, I got the error in the screenshot.

  3. Just to verify that it's a new issue in today's master, I put back the Feb 24 version and this issue was not present (but the previous issue is still present.)

Here are 5 commands for quick copy/pasting that instantly lead to this new issue:

cd /tmp
git clone '[email protected]:AndrewRadev/writable_search.vim.git'
mv 'writable_search.vim' 'writable-search-git-2020-04-16'
vim writable-search-git-2020-04-16/README.md
:WritableSearch the

I'll be happy to do more testing at any time, just let me know.

Peter

pkrumins avatar Apr 16 '20 01:04 pkrumins

Ah, I seem to have missed a particular case to test. Thanks for the detailed report. Could you try the most recent master?

AndrewRadev avatar Apr 16 '20 07:04 AndrewRadev

Hi Andrew,

I just tested today's master (head 5dd96a52) but unfortunately another error has come up. I took the same steps to test it as the last time and got this error:

image

For your convenience, here are the commands that I ran:

cd /tmp
git clone '[email protected]:AndrewRadev/writable_search.vim.git'
mv 'writable_search.vim' 'writable-search-git-2020-04-21'
vim writable-search-git-2020-04-21/README.md
:WritableSearch the

I'll be happy to test again, just let me know when.

Peter

pkrumins avatar Apr 21 '20 02:04 pkrumins

Ah, yes, seems like I had managed to fix it only for a specific case, my bad. I've pushed another attempt, please try again.

AndrewRadev avatar Apr 21 '20 08:04 AndrewRadev

Hi Andrew,

I just tried again with the latest commit 5644382 but now a new error has come up:

image

I used the exact same 5 steps as in the previous message. I'll be happy to test again, just let me know, and thanks so much for all this effort!

Peter

pkrumins avatar Apr 21 '20 21:04 pkrumins

That's strange, I can't replicate the error with these steps. Could you let me know what your Vim version is, and ideally share your .vimrc? There might be some configuration I'm accidentally relying on.

AndrewRadev avatar Apr 22 '20 05:04 AndrewRadev

Hi Andrew,

I use the following vim version (default vim install on Debian 9.8 Stretch):

VIM - Vi IMproved 8.0 (2016 Sep 12, compiled Jun 21 2019 04:10:35)
Included patches: 1-197, 322, 377-378, 550, 649, 651, 703, 706-707
Extra patches: 8.1.1401, 8.1.1382, 8.1.1368, 8.1.1367, 8.1.1366, 8.1.1365, 8.1.1046, 8.1.0613, 8.1.0547, 8.1.0546, 8.1.0544, 8.1.0540, 8.1.0539, 8.1.0538, 8.1.0506, 8.1.0208, 8.1.0206, 8.1.0205, 8.1.0189, 8.1.0177, 8.1.0067, 8.1.0066

I just disabled all plugins except writable-search and here's a list of all default system plugins that were loaded:

  1: /usr/share/vim/vimrc
  2: /usr/share/vim/vim80/debian.vim
  3: ~/.vimrc
  4: /usr/share/vim/vim80/filetype.vim
  5: /usr/share/vim/vim80/ftplugin.vim
  6: /usr/share/vim/vim80/indent.vim
  7: /usr/share/vim/vim80/syntax/syntax.vim
  8: /usr/share/vim/vim80/syntax/synload.vim
  9: /usr/share/vim/vim80/syntax/syncolor.vim
 10: /usr/share/vim/vim80/plugin/getscriptPlugin.vim
 11: /usr/share/vim/vim80/plugin/gzip.vim
 12: /usr/share/vim/vim80/plugin/logiPat.vim
 13: /usr/share/vim/vim80/plugin/manpager.vim
 14: /usr/share/vim/vim80/plugin/matchparen.vim
 15: /usr/share/vim/vim80/plugin/netrwPlugin.vim
 16: /usr/share/vim/vim80/plugin/rrhelper.vim
 17: /usr/share/vim/vim80/plugin/spellfile.vim
 18: /usr/share/vim/vim80/plugin/tarPlugin.vim
 19: /usr/share/vim/vim80/plugin/tohtml.vim
 20: /usr/share/vim/vim80/plugin/vimballPlugin.vim
 21: /usr/share/vim/vim80/plugin/zipPlugin.vim
 22: ~/.vim/pack/writable-search/start/writable_search.vim/plugin/writable_search.vim

My .vimrc is here: https://github.com/pkrumins/dotfiles/blob/master/.vimrc

Please let me know if you need any other extra information.

Peter

pkrumins avatar Apr 23 '20 07:04 pkrumins

Hm, I think I maybe sort of understand what might be the problem. The core of it is that running the command includes stderr, which means diagnostic messages also get placed in the buffer, like "permission denied". I think the reason I'm not getting the error with my normal Vim setup is because my backend is the ack.vim plugin which might do its own filtering.

I'll see about ignoring stderr, but I'm wondering if it would work for you if you cd-ed into the writable-search-git-2020-04-21 directory before running the command. So, running these steps:

cd /tmp
git clone '[email protected]:AndrewRadev/writable_search.vim.git'
mv 'writable_search.vim' 'writable-search-git-2020-04-21'
vim writable-search-git-2020-04-21/README.md
:WritableSearch the

Either way, I do need to fix the stderr issue, just wondering if this is the core of the problem or if there's something else still.

AndrewRadev avatar Apr 23 '20 10:04 AndrewRadev

Update: The stderr issue should be fixed, so your unmodified test might work now as well.

AndrewRadev avatar Apr 23 '20 10:04 AndrewRadev

Hi Andrew,

Thanks for figuring this out! I just tried the latest master at d6a408 and it's working well. The original issue and other issues are gone.

Just one final question:

When I run the :WritableSearch the command, then make some test changes, then :wq! the results tab, I end up with about 15 open buffers.

Here's a screenshot that lists the open buffers after I :wq! exited the results tab:

image

It looks like these are all buffers that have the matches. Is that the expected behavior? I don't remember if it left all buffers with matches open before.

Peter

pkrumins avatar Apr 26 '20 06:04 pkrumins

Yes, this has probably always been the case. To update the buffers, the plugin switches to them, applies the update, then switches back. Which is why it temporarily sets bufhidden to hide for the proxy buffer before jumping to the other ones.

I've applied a fix, since it shouldn't be necessary to hide the original buffers. Let me know if it looks okay now.

AndrewRadev avatar Apr 26 '20 09:04 AndrewRadev

Hi Andrew,

I just tested the new version (master f537c7) and it's almost perfect now except for one new issue related to buffers.

Here are the steps to reproduce it:

cd /tmp
echo "hello the world" > a
echo "bye the world" > b
echo "adfasdf the world" > c

git clone '[email protected]:AndrewRadev/writable_search.vim.git'
mv 'writable_search.vim' 'writable-search-git-2020-04-29'
vim a b c writable-search-git-2020-04-29/README.md
:WritableSearch the

Now change some "the" in some of files to something else like "moo".

Now :wq! to save the changes.

What will happen now, only a file (the first argument to vim) will stay open but the buffer of this file will have some weird property. If you now open any new file or create a fresh new buffer via :enew, then a file/buffer will disappear and this error will appear when you try to switch to a again:

image

I'll be happy to test again at any time.

Peter

pkrumins avatar Apr 29 '20 08:04 pkrumins

Hm, I see, the problem is that I was too eager to mark buffers as deletable when switching back to the result buffer. I added a check for whether the buffer already exists or we're loading it just for the replacement. Could you check now?

AndrewRadev avatar May 03 '20 09:05 AndrewRadev

Hi Andrew,

I just got to testing the latest changes and it looks like it's finally working without errors. Really great!

There are just two more tiny issues which I don't think are related to this ticket but it would be nice to fix them as well.

The first one is this – after doing a :w! in WritableSearch buffer, it shows this message "No lines in buffer". Like this:

image

The second is this one – it shows a lot of grep "No such device or address" messages right after running ":WritableSearch foo". Like this:

image

Both of these issues can be reproduced with the latest master and with all plugins disabled (only writable_search plugin is active).

I'll be happy to test these last two issues at any time.

Peter

pkrumins avatar Jun 05 '20 03:06 pkrumins

I'm not sure where the first message is coming from, but adding a silent to the update calls seems to avoid it. I've also made a change to how reading the output happens, so both issues should be fixed now. Give it a try and let me know if it works and whether I've broken anything with the update -- doesn't seem like it to me, at least.

AndrewRadev avatar Jun 05 '20 19:06 AndrewRadev