File renames aren't handled properly about vim-gitgutter

[dimonomid]

One more time, thanks a lot for doing this @airblade. I'm honestly impressed by how dedicated you are to supporting this project, not many open source projects are maintained like that. 🙇

[airblade]

Thanks for the detailed report.

GitGutter handles file renames if done within Vim, via :file or :saveas (for example, see 68f16eb). However that doesn't help when renames are done outside Vim.

Without thinking too hard I first tried adding the -M flag to the git-diff command:

diff --git i/autoload/gitgutter/diff.vim w/autoload/gitgutter/diff.vim
index 010a17b..c56afd9 100644
--- i/autoload/gitgutter/diff.vim
+++ w/autoload/gitgutter/diff.vim
@@ -137,7 +137,7 @@ function! gitgutter#diff#run_diff(bufnr, from, preserve_full_diff) abort
     let cmd .= ' -c "diff.noprefix=false"'
     let cmd .= ' -c "core.safecrlf=false"'
   endif
-  let cmd .= ' diff --no-ext-diff --no-color -U0 '.g:gitgutter_diff_args.' -- '.from_file.' '.buff_file
+  let cmd .= ' diff --no-ext-diff --no-color -M -U0 '.g:gitgutter_diff_args.' -- '.from_file.' '.buff_file
 
   " Pipe git-diff output into grep.
   if !a:preserve_full_diff && !empty(g:gitgutter_grep)

Alas it didn't help.

GitGutter works by diffing the in-buffer file against a version of the file from the index (obtained via git show).

It looks a bit like this:

• Write buffer to temp file A
• Write base version of file to temp file B (git show SHA:file > B)
• Diff (git diff --no-index -- A B)

The git-show command needs the filename, for which the code uses the buffer's filename.

I suppose instead we want to use the original filename – but I don't know how to get that in a neat way which could be used in the diff command.

[airblade]

It looks like the way to find renames is:

git diff --diff-filter=R --name-status SHA

And then parse the output for the original and current filenames.

I can't really see how to graft that into the plugin's current combined show and diff command. In which case it would need to be a separate, earlier step. But running an extra system call might add too much overhead.

[dimonomid]

Yeah after your initial explanation I was also looking at getting the file renames from git diff and also found the --name-status, which is exactly what we need here.

I can't really see how to graft that into the plugin's current combined show and diff command. In which case it would need to be a separate, earlier step.

I'm a bit confused wdym by not seeing how to graft that into the current design; I think we just need to make one more (earlier) step, as you also mentioned. So instead of the 3 steps you outlined above, we'll have 4:

• Write buffer to temp file A
• new step: Collect all file renames using git diff --name-status ....
• Write base version of file to temp file B (git show SHA:file > B) (where file might be either the same filename as we have now, or it can be older, depending on the renames collected earlier)
• Diff (git diff --no-index -- A B)

But running an extra system call might add too much overhead.

Tbh my experience with git diff in general (and also some brief experiments with git diff --name-status I've done just now) show that it should be plenty fast. But, to avoid worrying too much about the potential performance regression, it can be hidden behind a flag which is disabled by default, and then over time we might consider switching it to enabled by default instead (or not; doesn't matter much, since whoever needs it can just have it enabled explicitly)

Also btw, I can see that the diffing part can be addressed by this, but there is also the quickfix issue: right now it uses the old filename with a weird suffix. Is this part also fixable?

[airblade]

It's not the overhead of git-diff I'm thinking of, it's the overhead of Vim calling out to an external process and passing data to and fro across the process boundary. It used to be significant, though no doubt it's much better these days.

However I realised that we don't need to get the list of renames on every diff. We can get it once per diff base and cache it. So I think inserting a new step will be fine. If anyone complains about slowness, I'll make it opt-out.

[airblade]

The quickfix code is implemented independently of the diffing (I didn't have much time available when I wrote it).

It sets the prefixes to a/ and b/ so the prefixes have known lengths. However the prefixes shouldn't appear in the filename in the quickfix list. In your case the offsets seem to have gone awry – I suspect the m in b/m comes from b/myfile.txt being truncated in the wrong place.

I'll tackle the main diff function first and then the quickfix.

[dimonomid]

However I realised that we don't need to get the list of renames on every diff. We can get it once per diff base and cache it.

Just to double check though: if I e.g. set a diff base to HEAD~ or whatever, so the gitgutter will get the list of renames, and then the file gets renamed outside of vim without changing the diff base, ideally we do want gitgutter to pick that up, right? So while I agree it doesn't have to be done on every diff, but it probably should be done on some occasions even when the diff base did not change. Perhaps it also needs to be done every time we open a new file in vim (to check if this file is a new rename by any chance), or something like that (you know better for sure, just wanted to double check that this scenario is gonna be covered)

I'll tackle the main diff function first and then the quickfix.

Thanks!

[airblade]

Does this branch do what you need, at least as far as the signs go (I'll tackle quickfix later)? Can you get it to give you wrong signs?

I haven't written any automated tests yet but, testing manually, it works for me.

[dimonomid]

I tested it on a couple repos, and yeah the signs for renamed files are correct from what I can tell. Thanks a lot!

After the quickfix part is addressed as well, it'll be just perfect, as I can do the whole review right in the editor then.

[dimonomid]

Actually I think there's a new issue: if I change g:gitgutter_diff_base to a non-existing revision, I get those exceptions from obtain_file_renames:

(sorry I keep forgetting how can I copy-paste this error message from gvim, such a poor UX, so posting screenshot)

Stepping into this non-existing revision might be easier than you think: if I work on some particular repo, and set g:gitgutter_diff_base to a specific hash, and then briefly open a file from some other repo, and boom, it points to a non-existing revision now. I think on master it doesn't error out like that.

[airblade]

Thanks for pointing that out. I updated the branch to handle an invalid diff base.

[airblade]

Done in f7b9766...6a95f1b.

[airblade]

Thank you!