tomtom / quickfixsigns_vim

Mark quickfix & location list items with signs
http://www.vim.org/scripts/script.php?script_id=2584
GNU General Public License v3.0
131 stars 13 forks source link

Support selecting a different revision to diff against #68

Closed vhda closed 7 years ago

vhda commented 7 years ago

Hi,

Forgot to warn you that I only tested git. For the remaining VCS I took the required arguments from online documentation. Do you have any way of testing this across all VCS?

Thanks!

tomtom commented 7 years ago

Do you have any way of testing this across all VCS?

Unfortunately no.

vhda commented 7 years ago

You might want to revert the merge until we are sure everything is correct. I'll try to test on other VCS in the meanwhile, as soon as I have some time.

tomtom commented 7 years ago

You might want to revert the merge until we are sure everything is correct. I'll try to test on other VCS in the meanwhile, as soon as I have some time.

Do you have a chance to test this in the foreseeable future? If not, I'd rather revert the g:quickfixsigns#vcsdiff#revision param with a more general g:quickfixsigns#vcsdiff#extra_args param that can be set to whatever you want.

Regards

blueyed commented 7 years ago

@vhda @tomtom This changes the default behavior, where you only see what is changed relative to the index (git diff), but now changes that are staged already (git add) show up in the signs still.

To get the old behavior I've tried let g:quickfixsigns#vcsdiff#revision_git = '', but it requires a patch:

diff --git i/autoload/quickfixsigns/vcsdiff.vim w/autoload/quickfixsigns/vcsdiff.vim
index cf1ffd1..ddb4ced 100644
--- i/autoload/quickfixsigns/vcsdiff.vim
+++ w/autoload/quickfixsigns/vcsdiff.vim
@@ -557,17 +557,11 @@ function! s:GetParam(name, type, default) abort "{{{3
     endif
     let gt = a:name .'_'. a:type
     if exists('g:'. gt)
-        let gtval = g:{gt}
-        if !empty(gtval)
-            return gtval
-        endif
+        return g:{gt}
     endif
     let g = a:name
     if exists('g:'. g)
-        let gval = g:{g}
-        if !empty(gval)
-            return gval
-        endif
+        return g:{g}
     endif
     return a:default
 endf

There is no non-empty value to get the old behavior, is there?

blueyed commented 7 years ago

@vhda What is your use case for this btw? I can see that is is nice for project specific settings (e.g. via https://github.com/embear/vim-localvimrc), but the initial PR was for the global setting only.

vhda commented 7 years ago

@blueyed I noticed that in the meanwhile and have been trying to come up with a different implementation of this feature to overcome that limitation. Give me a day or two more please.

I do frequent commits to keep track of daily updates and sometimes it is desirable to still be able to keep track of what changed during a certain feature implementation. Being able to decide which commit to diff against provides me with that visual aid.

vhda commented 7 years ago

Just noticed I already had a patch laying around :) Let me submit it so that you can try it yourself!

vhda commented 7 years ago

Please check my master branch. @tomtom had made some changes in the meanwhile and I had to rebase my work on top of his. I still don't like the current solution, because there's some code duplication...

blueyed commented 7 years ago

@vhda Please create a separate branch and a PR where we can discuss this.

tomtom commented 7 years ago

I can see that is is nice for project specific settings (e.g. via https://github.com/embear/vim-localvimrc), but the initial PR was for the global setting only.

Since https://github.com/tomtom/quickfixsigns_vim/commit/bc5bf54211776375b8559376bb0a09fbe05909da the behaviour can also be enabled per buffer, which also implies it can be enabled per project via some kind of project-local init file.

tomtom commented 7 years ago

@blueyed I hope https://github.com/tomtom/quickfixsigns_vim/commit/9e10e1527bbfa315bb315daff243329bd35b1af4 captures the gist of your proposal.

blueyed commented 7 years ago

@tomtom Thanks, that should work!