mhinz / vim-signify

:heavy_plus_sign: Show a diff using Vim its sign column.
MIT License
2.69k stars 102 forks source link

No Gutter #87

Closed Alexander-Shukaev closed 10 years ago

Alexander-Shukaev commented 10 years ago

Well, nothing more to say, but I see no gutter with signs. Plugin is loaded. Git is in place.

I'm on Windows for the record. By the way, interestingly enough, GitGutter does not display anything for me either.

justinmk commented 10 years ago

In vim, if you run :!git --version does it say "command not found"?

When you installed msysgit, did you enable the option to put git in your cmd.exe path?

justinmk commented 10 years ago

Also, if you run vim from with msysgit bash, does the gutter appear for git-tracked files with modifications?

The gutter won't appear if the file has no changes. If you make a change, and save it, the gutter should then appear.

Alexander-Shukaev commented 10 years ago

Sure, I know how to read manuals, and I wouldn't create an issue then. Nothing appears, even when I save it. As I said before, I can access Git from Vim (it is in PATH), I'm not a newbie, you know ;).

justinmk commented 10 years ago

Ok. Here is a manual you should read: http://www.catb.org/esr/faqs/smart-questions.html

Your original question contained almost no information, so I tried to extract some details to eliminate unknowns. In particular, no one knows whether you're a newbie or not.

mhinz commented 10 years ago

Hmm. Try opening a file with changes and do this:

:!cd %:h && git diff --no-color --no-ext-diff -U0 -- thisfile.vim

Does this output a patch file? (If not, try without the -- and if that isn't working as well, test if && is the culprit.)

EDIT:

BTW, what type is shown if you run :SyDebug in that buffer?

Alexander-Shukaev commented 10 years ago

If I understood the test case correctly, then here is what worked for me:

:!cd %:h && git diff --no-color --no-ext-diff -U0 -- %

it does show the diff in the crappy cmd.exe window.

:SyDebug of course returns git as type.

mhinz commented 10 years ago

Looks good. Perhaps another plugin is interfering.

1) Could you try disabling all plugins except Sy and try again?

2) Do you have any Sy options set? If so, comment them out for the tests.

BTW, there will be vimproc support soon (aka no cmd.exe window anymore).

Alexander-Shukaev commented 10 years ago
  1. As I thought, this has no effect;
  2. I have already unset them for testing purposes.

To conclude, something really goes wrong in the plugin itself.

justinmk commented 10 years ago

@Haroogan Did you try from within msysgit bash's vim, instead of gvim?

mhinz commented 10 years ago

@bling Any ideas on this issue?

bling commented 10 years ago

@Haroogan which version of vim are you using? cygwin, gvim, patch number, etc. also, if you can post a minimal vimrc that reproduces on your computer i can quickly test here.

Alexander-Shukaev commented 10 years ago
VIM - Vi IMproved 7.4 (2013 Aug 10, compiled Sep 10 2013 20:35:10)
MS-Windows 64-bit GUI version with OLE support
Included patches: 1-20
Compiled by Haroogan <Haroogan@gmail.com>
Huge version with GUI.  Features included (+) or not (-):
+acl                +ex_extra           +multi_byte_ime/dyn +tag_old_static
+arabic             +extra_search       +multi_lang         -tag_any_white
+autocmd            +farsi              -mzscheme           -tcl
+balloon_eval       +file_in_path       -netbeans_intg      -tgetent
+browse             +find_in_path       +ole                -termresponse
++builtin_terms     +float              +path_extra         +textobjects
+byte_offset        +folding            -perl               +title
+cindent            -footer             +persistent_undo    +toolbar
+clientserver       +gettext/dyn        -postscript         +user_commands
+clipboard          -hangul_input       +printer            +vertsplit
+cmdline_compl      +iconv/dyn          +profile            +virtualedit
+cmdline_hist       +insert_expand      +python/dyn         +visual
+cmdline_info       +jumplist           +python3/dyn        +visualextra
+comments           +keymap             +quickfix           +viminfo
+conceal            +langmap            +reltime            +vreplace
+cryptv             +libcall            +rightleft          +wildignore
+cscope             +linebreak          -ruby               +wildmenu
+cursorbind         +lispindent         +scrollbind         +windows
+cursorshape        +listcmds           +signs              +writebackup
+dialog_con_gui     +localmap           +smartindent        -xfontset
+diff               -lua                -sniff              -xim
+digraphs           +menu               +startuptime        -xterm_save
-dnd                +mksession          +statusline         +xpm_w32
-ebcdic             +modify_fname       -sun_workshop       
+emacs_tags         +mouse              +syntax             
+eval               +mouseshape         +tag_binary         

It's not Cygwin, it's native Vim for Windows which I build myself.

Why do you need vimrc? There is nothing to configure. Just launch bare Vim with only this plugin installed, and it will either work or not work.

bling commented 10 years ago

@Haroogan providing a vimrc saves us a ton a time because it rules out every possible permutation of plugins and settings you might have. gvim7.4/vim-signify/win7 works fine over here. see if one of these builds makes a difference.

Alexander-Shukaev commented 10 years ago

First of all, I want to make a clue - I don't believe that trying various builds that differ within a few non relevant patches is going to make any difference. But in order to prove that, I did the following test. I'd love you to do absolutely the same and tell me about your results. Let's go:

  1. Take the build you linked;
  2. No vimrc at all, just bare Vim;
  3. Only 1 plugin, i.e. signify;
  4. Start gvim.exe --cmd "set nocompatible" (as this plugin needs it);
  5. Test git availability: :echo system('git') (passed);
  6. See if signify is loaded: :echo g:loaded_signify (passed);
  7. Open the test file;
  8. Debug: :SyDebug (passed);
  9. Final test: :!cd %:h && git diff --no-color --no-ext-diff -U0 -- %:t (passed);

And after all of these passed tests with absolutely virgin Vim (no configuration bloat, no other plugins to conflict), I'm sorry, but I still see NO signification. Q.E.D.

I believe that it works for you, but it does not for me, and there should definitely be a reason why. Looking forward to your feedback.

mhinz commented 10 years ago

Does :echo system('cd <directory> && git diff --no-color --no-ext-diff -U0 <file> looks suspicious somehow? (Containing control sequences or anything.)

bling commented 10 years ago

@Haroogan the point of specifying builds and a vimrc is so we don't have to guess what you have and i can try out your configuration in 30 seconds, not back and forth in this issue for days.

i went over your steps and everything works, including seeing signs.

how did you install vim-signify? did you just extract all the files into the vim installation?

Alexander-Shukaev commented 10 years ago

@mhinz: I think, I've nailed it: if I call:

:echo system('cd %:h && git diff --no-color --no-ext-diff -U0 -- %:t')

I get:

E484: Can't open file D:\Users\Haroogan\AppData\Local\Temp\VIoBA81.tmp

I have no idea what's his problem to create this temporary file. And, no, it's not permissions, so forget about it. There is some other reason for it... Any ideas?

@bling: Yes, I simply extracted them into the Vim installation that you've linked.

mhinz commented 10 years ago

Hmm, I have no idea about Windows, but does cmd.exe understand %:h? Because it shouldn't get expanded in the Vim sense, e.g. on Linux it would try to change to the actual directory %:h. (Same with %:t.)

If you type the directory and file out, do you get the same error? That would suggest that it's a path escaping problem. Then again, only calling :echo system() shouldn't lead Vim to try opening a file (E484 is obviously an error thrown by Vim, not the cmd.exe).

Alexander-Shukaev commented 10 years ago

Sure, typing it manually works fine. As I have quite some experience in writing Vim plugins as well on both Windows and Unix platforms, and I also have plugins which deal with invocation of external tools and supply paths, I'd like to recommend the following:

let l:dir_path  = shellescape(expand('%:h', 1))
let l:file_name = shellescape(expand('%:t', 1))

system('cd ' . l:dir_path . '&& git diff --no-color --no-ext-diff -U0 -- ' . l:file_name)
mhinz commented 10 years ago

The argument to system() gets shellescape()d already. If it wouldn't, it would break for much more people.

Although I don't think it's the problem, you could try altering autoload/sy/util.vim from..

function! sy#util#escape(path) abort
  if exists('+shellslash')
    let old_ssl = &shellslash
    set noshellslash
  endif

  let path = shellescape(a:path)

  if exists('old_ssl')
    let &shellslash = old_ssl
  endif

  return path
endfunction

..to..

function! sy#util#escape(path) abort
  "if exists('+shellslash')
  " let old_ssl = &shellslash
  "  set noshellslash
  "endif

  let path = shellescape(a:path)

  "if exists('old_ssl')
  "  let &shellslash = old_ssl
  "endif

  return path
endfunction

Otherwise I'm running out of ideas.

Alexander-Shukaev commented 10 years ago

Line 68 in repo.vim change from:

  let diff = system('cd '. sy#util#escape(fnamemodify(a:path, ':h')) .' && git diff --no-color --no-ext-diff -U0 '. diffoptions .' -- '. sy#util#escape(a:path))

to:

  let diff = system('cd '. sy#util#escape(fnamemodify(a:path, ':h')) .' && git diff --no-color --no-ext-diff -U0 '. diffoptions .' -- '. sy#util#escape(fnamemodify(a:path, ':t')))

and it works instantly. Your variant (absolute path to directory and absolute path to file) returns empty diff, but the call to git succeeds, thats why I had no errors and no signs: the diff was always empty, but the call always succeeded.

Anyway, absolute path does not make any sense since we have already changed to the repository. Relative path works just fine, and I suggest to change it not only for Windows, but for all platforms.

Finally, playing with shellslash and your other function sy#util#separator are really bad ideas in general for the following reasons:

  1. If shell does not refer to a Unix shell. For instance, when one is on Windows and one didn't change shell, then it certainly refers to cmd.exe by default. Setting shellslash in this case is devastating because, for example, shellescape() will enclose its arguments in single quotes ' which are not recognized by cmd.exe as one could expect.
  2. If you want to enforce all path separators into Windows ones (or Unix ones, if run on Vim for Unix) in some string, say path, then all you have to do is call expand(path, 1), and that's it! You don't have to reinvent the wheel with your own path conversion functions because the cross-platform solution is already given to you.

So I suggest to rethink, clean up redundant functionality, and redesign a bit. Looking forward to the upcoming fix ;).

mhinz commented 10 years ago

And I suggest using a less arogant tone. Seriously, it's not useful.

This plugin has to work for lots of different version control systems. Thus I have to find ways to make them all play together without adding too much VCS-specific code for all kinds of corner cases. Using a relative path would break other VCS and first converting a path to absolute and later back to relative doesn't make sense.

If I understand you correctly, the problem seems to happen when you provide 'git diff' with an absolute path although you're in the directory of the file already. Is that correct? Which git program do you use? (Cygwin? msys git?) Because if the current way would be a problem in general, it wouldn't be working for any Windows users in the first place.

Alexander-Shukaev commented 10 years ago

Hey, calm down there, do you even know what the word "arrogant" means? If not, then go ask someone or consult the dictionary. All the points that I've listed have well-thought reasoning behind them. Whether to consider them or not is your own responsibility, and I don't really care about it that much. I'm sitting here for the 3rd day and spending my time to help you resolve these corner cases, so, at least, let's respect each other's time and effort. If you think that anyone who gives you recommendations and/or suggestions is "arrogant" because one knows something that you didn't, then the discussion of this issue would be over right here.

Regarding the issue, "without adding too much VCS-specific code" is weak argument because sy#repo#get_diff_git() is already a VCS-specific function. "Using a relative path would break other VCS" - nobody told you to do that, it's just stupid, we're discussing Git. "first converting a path to absolute and later back to relative doesn't make sense" - it does make sense because, in fact, it's not converting, it's simply splitting the path into 2 parts: the directory where it's located (to change into it) and its name, but even the function interface stays intact, i.e. you just send a path to the file to it (as you did before), and what happens inside of it - is an implementation detail. And, YES, that would be a VCS-specific action to do for Git. And tell me for which users exactly would this approach break the usage of Git with your plugin?

Finally, I'm using Git which comes with MSYS2, the bleeding-edge Unix utilities natively built for Windows (no Cygwin dependencies, and NO, it's not old crappy MSYS, hence the 2 in the end). They probably rely on msysgit project to build Git supplied in it. Current Git version is 1.8.4.

justinmk commented 10 years ago

Classic.

The reason fnamemodify(a:path, ':t') "fixes" the issue is almost certainly because msys2 is choking on the Windows-style path that is set here:

https://github.com/mhinz/vim-signify/blob/50496311c5e05e430cff407c8250792078ed2fff/plugin/signify.vim#L18

But fnamemodify(a:path, ':t') doesn't actually fix anything, it just hides the fact that the shell is choking on the path sent from vim-signify.

resolve(expand('<afile>:p')) returns a path like this:

\Users\foo\Desktop\baz.txt

In msys2 mintty, if you try this, what happens?

cd '\Users'

msysgit bash handles that correctly (it changes to /c/Users/). Note that the single-quotes are required, of course. If that works correctly in msys2 mintty, then it is clear that sy#util#escape() is indeed the problem. shellescape is probably not correct for msys2.

I think the easiest solution is to wrap the path in single-quotes for any shell other than cmd.exe, rather than trying to satisfy shellescape().

What is the value of :echo &shell in your gvim?

I'm using Git which comes with MSYS2

You seriously didn't think that was worth mentioning 3 days ago? msys2 is in alpha. 90% of Windows git-users run vanilla msysgit.

mhinz commented 10 years ago

I can't test it anyway, so I welcome any patches that provide a better fix than the fnamemodify() one. :)

justinmk commented 10 years ago

@mhinz Well, @Haroogan said the change worked for him, so it is changing directory correctly. I just mean that sy#util#escape(a:path) is returning something that breaks somewhere in the pipeline from Vim to msys2.

msysgit has some extra hacks on top of cygwin that make it more forgiving than vanilla cygwin/mintty, so I am guessing msys2 is not as forgiving.

Your commit seems fine, but I think sy#util#escape(a:path) might deserve a closer look in case it causes other problems.

Alexander-Shukaev commented 10 years ago

@mhinz: I don't know if you were distracted or simply made a typo, but this commit is incorrect as it should be fnamemodify(a:path, ':t') and not fnamemodify(a:path, '%:t') (look again at my previous post)! Please, amend that. In any case, thanks for accepting the proposal, it was definitely the right choice, as now we don't care whether Git guys !@#$ed up, msysgit guys !@#$ed up, or MSYS2 guys !@#$%ed up, whoever it was, good for them, hopefully they learn something new after that... Also, please, I kind of beg you to give additional thought about what I said about shellslash tampering and functions like sy#util#separator, this is very important, and can bite you in the ass when you least expect it to. Overall, thanks for collaboration, and useful plugin, wish you good luck with further development! :+1:

@justinmk: I'd like to defend MSYS2. First of all, it's neither alpha nor beta, it's release already. They simply didn't change the directory on the repository so it still contains the "alpha" word (which is stupid, I know). Secondly, I've just searched web for this issue and there are people who experience the same while not sitting on MSYS2. This proves that I'm definitely not the first one to experience that, and I can assure you that I'm not the last one. In that sense, this change is just an amazing solution which not only doesn't break anything, but actually seems very natural to me (why would you use absolute path, if you're already there?). By the way it does not work even from bash (not only cmd.exe). Also, tried all the combinations of path styles and neither works. What a crap, guess, we just have to live with that...

mhinz commented 10 years ago

Argh, apparently I was still thinking in expand() terms..

justinmk commented 10 years ago

@Haroogan

MSYS2. First of all, it's neither alpha nor beta

Good to know. I will probably try it out, seems interesting.

By the way it does not work even from bash (not only cmd.exe). Also, tried all the combinations of path styles and neither works. What a crap, guess, we just have to live with that..

Do you mean cd '\Users' does not work msys2? Or what was the result of that?

Alexander-Shukaev commented 10 years ago

@justinmk: No, no. Of course cd \Users works everywhere. I meant that invocation of git diff with absolute path to file or even relative path to file (which contains not only name, but at least one directory in it) does not work, no matter what shell you use or which path style. So it's a bug in Git itself, or maybe it was built improperly.