airblade/vim-gitgutter

Added files don't show any signs when using a specific commit/branch as diff base

fvictorio opened this issue · 11 comments

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

44dbd57dd19e8ec58b2a50c787c8ae7bb231c145

What vim/nvim version are you on?

Vim 8.2


Problem

I normally do something like let g:gitgutter_diff_base="main" when reviewing a pull request. This works great except for added files. I would expect them to show the + sign in each line, but nothing is shown.

This is different from #461, which is about new files. The answer there is that git doesn't know about those files, so nothing is shown. I agree with that. But in this scenario, if you do git diff main -- some-new-file, you do get a diff with all the lines added.

Possible cause

I tried to investigate this a bit, and I found that one of the commands that end up being executed is something like:

git show main:some-new-file

which produces a fatal: path 'some-new-file' exists on disk, but not in 'main' error.

I agree with everything you wrote. I'm trying to think of a way around this given that the git show main:some-new-file is part of a compound command.

The command that's executed is something like:

cd /path/to/repo &&
  (git show main:some/file > a && git diff -- a b | grep '^@@ ' || exit 0)

– where b is a temporary file holding the buffer contents.

I don't want to execute a separate call to determine whether some/file exists on the main branch, because each external call adds overhead and slows things down.

I can't get too clever with the shell command because it has to work on Windows too. So I doubt whether we could do something like ... (git show main:some/file || echo '') > a ....

The only thing I can think of is to update the async job's stderr handlers to look for git's error message, and then either return a fake hunk header or call a new function to mark every line as added. And similarly for the synchronous diff's output.

So I doubt whether we could do something like ... (git show main:some/file || echo '') > a ....

That seems like the most straightforward solution though, and the command already has parentheses and ||, so maybe it's worth trying? Happy to help anyway I can. I don't use Windows but I can try it in some VM or something.

FWIW, this seems to work:

diff --git a/autoload/gitgutter/diff.vim b/autoload/gitgutter/diff.vim
index 794cd14..5c93fa6 100644
--- a/autoload/gitgutter/diff.vim
+++ b/autoload/gitgutter/diff.vim
@@ -124,7 +124,7 @@ function! gitgutter#diff#run_diff(bufnr, from, preserve_full_diff) abort
 
     " Write file from index to temporary file.
     let index_name = gitgutter#utility#get_diff_base(a:bufnr).':'.gitgutter#utility#repo_path(a:bufnr, 1)
-    let cmd .= g:gitgutter_git_executable.' '.g:gitgutter_git_args.' --no-pager show '.index_name.' > '.from_file.' && '
+    let cmd .= '('.g:gitgutter_git_executable.' '.g:gitgutter_git_args.' --no-pager show '.index_name.' || echo -n "")'.' > '.from_file.' && '
 
   elseif a:from ==# 'working_tree'
     let from_file = gitgutter#utility#repo_path(a:bufnr, 1)

I had to use echo -n because otherwise the temp file has a newline, which causes the first sign in the buffer to be a ~ instead of a +

Thanks for trying it out. And good to hear that it seems to work.

I'm wary about tinkering with the overall command because it took so long to find a form which works everywhere. However, as you say, it already has parentheses and ||.

I had to use echo -n because otherwise the temp file has a newline, which causes the first sign in the buffer to be a ~ instead of a +

Good point. In fact I think printf is much more consistent than echo. Would you mind trying that in a Windows VM?

diff --git i/autoload/gitgutter/diff.vim w/autoload/gitgutter/diff.vim
index 794cd14..faf0098 100644
--- i/autoload/gitgutter/diff.vim
+++ w/autoload/gitgutter/diff.vim
@@ -124,7 +124,7 @@ function! gitgutter#diff#run_diff(bufnr, from, preserve_full_diff) abort
 
     " Write file from index to temporary file.
     let index_name = gitgutter#utility#get_diff_base(a:bufnr).':'.gitgutter#utility#repo_path(a:bufnr, 1)
-    let cmd .= g:gitgutter_git_executable.' '.g:gitgutter_git_args.' --no-pager show '.index_name.' > '.from_file.' && '
+    let cmd .= '('.g:gitgutter_git_executable.' '.g:gitgutter_git_args.' --no-pager show '.index_name.' || printf "") > '.from_file.' && '
 
   elseif a:from ==# 'working_tree'
     let from_file = gitgutter#utility#repo_path(a:bufnr, 1)

Ok I was finally able to try this after installing vim and git in my wife's laptop and dealing with the fact that not all computers have caps lock mapped to ctrl, ANYWAY:

First thing, printf doesn't seem to exist in cmd or powershell, and I confirmed that using it in the command makes the plugin stop working on Windows.

echo does work, but -n is not a valid flag in windows (and "" is printed verbatim). This means that when you go to a new file and preview the hunk, it will tell you that the previous version had -n "" as its content. Not great.

So I tried using just echo on its own, but that prints ECHO is on., which is even worse. Apparently you can use echo: to print an empty line, but that will obviously not work on linux.

So I'm not sure what's the best course of action here. I guess we could use gitgutter#utility#windows and change the command based on that, but that function is barely used in the codebase, so I guess you try to avoid those kinds of fixes.

Windows is a pain! And there are several shell variations: cmd, powershell, cygwin, bash installed by the git installer, etc.

We could continue with this approach and change the command to echo:, perhaps using gitgutter#utility#windows() && s:dos_shell() (or simply make dos_shell() into gitgutter#utility#dos_shell() and use that alone).

I'd almost prefer to say that the first time a buffer is diffed, if g:gitgutter_diff_base is not empty we make an extra system call to see if the file exists in the diff base, and cache the result in a buffer variable. If the file doesn't exist in the diff base, we change the overall command to git diff <g:gitgutter_diff_base> -- <file>. But that isn't quite what we want because gitgutter shows the diff between the buffer contents and the diff base, not the working tree and the diff base. But the only way cross-platform way to diff the buffer contents is to write it to a temp file, and then we have to do git diff -- a b and we're back where we started.

We could continue with this approach and change the command to echo:, perhaps using gitgutter#utility#windows() && s:dos_shell() (or simply make dos_shell() into gitgutter#utility#dos_shell() and use that alone).

Makes sense, I'll give it a try and then test it in as many windows shells as I can (if you have a full list somewhere, let me know).

Actually I think I have a way to avoid all that.

The first time a buffer is diffed, if g:gitgutter_diff_base is not empty we make an extra system call to see if the file exists in the diff base, and cache the result in a buffer variable. If the file doesn't exist in the diff base, we skip running the diff commands and instead return a fake hunk header showing that the whole file is new.

Ah, that makes sense. It seems a bit out of my depth, but I can give it a try if you don't have time to do it yourself. And if you do it, I'm happy to help with the manual testing on windows (well, not happy, but, you know).

I'll try to get to it over the next day or two.