airblade / vim-gitgutter

A Vim plugin which shows git diff markers in the sign column and stages/previews/undoes hunks and partial hunks.
MIT License
8.35k stars 296 forks source link

Other plugins may make the preview window read-only, causing problems in hunk-preview #713

Closed adah1972 closed 4 years ago

adah1972 commented 4 years ago

When some other plugins open a read-only preview window (like YouCompleteMe), <Leader>hp could lead to a lot of Vim error messages about the window being read-only.

Not sure whether this is the best method, but I could fix the issue with the following simple patch:

diff --git a/autoload/gitgutter/hunk.vim b/autoload/gitgutter/hunk.vim
index 0460248..fbf5d42 100644
--- a/autoload/gitgutter/hunk.vim
+++ b/autoload/gitgutter/hunk.vim
@@ -200,6 +200,7 @@ endfunction

 function! s:hunk_op(op, ...)
+  pclose
   let bufnr = bufnr('')

   if s:in_hunk_preview_window()

What is the latest commit SHA in your installed vim-gitgutter?

e31e9bb

What vim/nvim version are you on?

Vim 8.2, several different patched versions

airblade commented 4 years ago

This line is supposed to handle this situation:

https://github.com/airblade/vim-gitgutter/blob/e31e9bb35f7346caab4fcf972d44449bdfd3230a/autoload/gitgutter/hunk.vim#L454

But it's only called for a new preview window.

Please can you try this patch?

diff --git i/autoload/gitgutter/hunk.vim w/autoload/gitgutter/hunk.vim
index 0460248..b6da1db 100644
--- i/autoload/gitgutter/hunk.vim
+++ w/autoload/gitgutter/hunk.vim
@@ -449,10 +449,10 @@ function! s:open_hunk_preview_window()
       let s:preview_bufnr = bufnr('')
     endif
     set previewwindow
-    setlocal filetype=diff buftype=acwrite bufhidden=delete
-    " Reset some defaults in case someone else has changed them.
-    setlocal noreadonly modifiable noswapfile
   endif
+  setlocal filetype=diff buftype=acwrite bufhidden=delete
+  " Reset some defaults in case someone else has changed them.
+  setlocal noreadonly modifiable noswapfile
 endfunction
adah1972 commented 4 years ago

Before the change, I got this error:

Error detected while processing function gitgutter#hunk#preview[3]..<SNR>126_hunk_op[
62]..<SNR>126_preview[6]..<SNR>126_populate_hunk_preview_window:
line   44:
E21: Cannot make changes, 'modifiable' is off:     %delete _
line   45:
E21: Cannot make changes, 'modifiable' is off
E21: Cannot make changes, 'modifiable' is off
Error detected while processing function gitgutter#hunk#preview[3]..<SNR>126_hunk_op[
62]..<SNR>126_preview[7]..<SNR>126_enable_staging_from_hunk_preview_window:
line    4:
E680: <buffer=0>: invalid buffer number

After moving setlocal down, I still got the last error:

Error detected while processing function gitgutter#hunk#preview[3]..<SNR>126_hunk_op[
62]..<SNR>126_preview[7]..<SNR>126_enable_staging_from_hunk_preview_window:
line    4:
E680: <buffer=0>: invalid buffer number
airblade commented 4 years ago

The remaining error is from this line:

https://github.com/airblade/vim-gitgutter/blob/e31e9bb35f7346caab4fcf972d44449bdfd3230a/autoload/gitgutter/hunk.vim#L529

I think the solution is similar to above:

diff --git i/autoload/gitgutter/hunk.vim w/autoload/gitgutter/hunk.vim
index 0460248..6ac4e65 100644
--- i/autoload/gitgutter/hunk.vim
+++ w/autoload/gitgutter/hunk.vim
@@ -443,16 +443,16 @@ function! s:open_hunk_preview_window()
   if !&previewwindow
     noautocmd execute g:gitgutter_preview_win_location &previewheight 'new gitgutter://hunk-preview'
     doautocmd WinEnter
-    if exists('*win_getid')
-      let s:winid = win_getid()
-    else
-      let s:preview_bufnr = bufnr('')
-    endif
     set previewwindow
-    setlocal filetype=diff buftype=acwrite bufhidden=delete
-    " Reset some defaults in case someone else has changed them.
-    setlocal noreadonly modifiable noswapfile
   endif
+  if exists('*win_getid')
+    let s:winid = win_getid()
+  else
+    let s:preview_bufnr = bufnr('')
+  endif
+  setlocal filetype=diff buftype=acwrite bufhidden=delete
+  " Reset some defaults in case someone else has changed them.
+  setlocal noreadonly modifiable noswapfile
 endfunction
adah1972 commented 4 years ago

Yup, this could solve the problem.

From the user's point view, this change keeps the position of the original preview window. My previous use of pclose makes the gitgutter preview window appear from its usual position—the bottom in my case.

Either can work. I actually prefer my change (close the old top preview window and open the gitgutter preview window from the bottom), but I do not have a strong opinion.

airblade commented 4 years ago

Thanks for the feedback.

I don't feel strongly about it either. I thought it would be slightly better to reuse an existing preview window to avoid the window jumping around.