mcchrish / nnn.vim

File manager for vim/neovim powered by n³
BSD 2-Clause "Simplified" License
646 stars 25 forks source link

Error when exiting nnn picker without choosing a file. #167

Open Oxore opened 1 year ago

Oxore commented 1 year ago

Describe the bug

I get multiple Neovim errors when I exit nnn picker without choosing a file, i.e. by pressing q.

To Reproduce

Minimal configuration:

call plug#begin('~/.local/share/nvim/site/plugged')
Plug 'mcchrish/nnn.vim'
call plug#end()

Steps to reproduce:

  1. Open Neovim nvim -u testrc.vim testrc.vim.
  2. Run command :NnnPicker %:p:h. This is recommended way in the readme to make nnn start in the current file's directory.
  3. Press q on the opened nnn picker window.
  4. See error. You may want to press Enter a couple of times to see all errors.

After this the behavior is proper: Neovim shows previously opened file, no problem. Error messages are the problem basically.

The text of the error:

Error detected while processing function <SNR>45_callback:
line   11:
E684: list index out of range: 1
Press ENTER or type command to continue
Error detected while processing function <SNR>45_callback:
line   12:
E121: Undefined variable: lastd
Press ENTER or type command to continue
Error detected while processing function <SNR>45_callback:
line   12:
E116: Invalid arguments for function fnameescape
Press ENTER or type command to continue

Expected behavior

Close nnn picker window by q key hit without errors and show previously opened file.

Screenshots

2023-08-19_21:05:49_950x370 2023-08-19_21:05:52_950x370 2023-08-19_21:05:56_950x370

Environment:

Additional context

The %:p:h argument is actually not relevant. Starting :NnnPicker without arguments will lead to errors too when NNN closed by q. I found it after I already took screenshots and decided to leave "steps to reproduce" section as is.

Oxore commented 1 year ago

The following change seems to be a fix for the issue, because the .lastd file contains string cd '/path/to/file' and not cd "/path/to/file":

diff --git a/autoload/nnn.vim b/autoload/nnn.vim
index 487d716..78592ae 100644
--- a/autoload/nnn.vim
+++ b/autoload/nnn.vim
@@ -235,7 +235,7 @@ function! s:create_on_exit_callback(opts)
         let fname = s:nnn_conf_dir.'/.lastd'
         if !empty(glob(fname))
             let firstline = readfile(fname)[0]
-            let lastd = split(firstline, '"')[1]
+            let lastd = split(firstline, "'")[1]
             execute 'cd' fnameescape(lastd)
             call delete(fnameescape(fname))
         endif

But I'm not sure that it does not break anything else. Going to investigate further and test it more, but later.

N-R-K commented 1 year ago

Since https://github.com/jarun/nnn/commit/57882ffab713e66690d29714a7fbea2cb2e61a5f nnn already shell escapes the filename. So I suspect that fnameescape call might not be correct anymore as well.

Oxore commented 1 year ago

That commit may be the reason why this issue arose at all. @N-R-K thank you for sharing. A fix for this issue should probably be inclusive for both single quote and double quote cases, because not everyone is using the newest nnn version.

Also splitting string by quotation marks may not work for paths that contain ' and " anyway.