Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow vimfiles to work with native macOS vim diffopt #213

Merged
merged 1 commit into from
Nov 4, 2021

Conversation

dtshepherd
Copy link
Contributor

Tested on MacOS BigSur and Catalina. Also, tested on Ubuntu 20.04 with custom built (buildtool) vim:

$ vim --version
VIM - Vi IMproved 8.2 (2019 Dec 12, compiled Feb  5 2021 16:21:06)
Included patches: 1-2467
Compiled by [email protected]
Huge version without GUI.  Features included (+) or not (-):
+acl               -farsi             +mouse_sgr         +tag_binary
+arabic            +file_in_path      -mouse_sysmouse    -tag_old_static
+autocmd           +find_in_path      +mouse_urxvt       -tag_any_white
+autochdir         +float             +mouse_xterm       +tcl
-autoservername    +folding           +multi_byte        +termguicolors
-balloon_eval      -footer            +multi_lang        +terminal
+balloon_eval_term +fork()            -mzscheme          +terminfo
-browse            +gettext           +netbeans_intg     +termresponse
++builtin_terms    -hangul_input      +num64             +textobjects
+byte_offset       +iconv             +packages          +textprop
+channel           +insert_expand     +path_extra        +timers
+cindent           +ipv6              +perl              +title
-clientserver      +job               +persistent_undo   -toolbar
-clipboard         +jumplist          +popupwin          +user_commands
+cmdline_compl     +keymap            +postscript        +vartabs
+cmdline_hist      +lambda            +printer           +vertsplit
+cmdline_info      +langmap           +profile           +virtualedit
+comments          +libcall           +python            +visual
+conceal           +linebreak         -python3           +visualextra
+cryptv            +lispindent        +quickfix          +viminfo
+cscope            +listcmds          +reltime           +vreplace
+cursorbind        +localmap          +rightleft         +wildignore
+cursorshape       -lua               +ruby              +wildmenu
+dialog_con        +menu              +scrollbind        +windows
+diff              +mksession         +signs             +writebackup
+digraphs          +modify_fname      +smartindent       -X11
-dnd               +mouse             -sound             -xfontset
-ebcdic            -mouseshape        +spell             -xim
+emacs_tags        +mouse_dec         +startuptime       -xpm
+eval              +mouse_gpm         +statusline        -xsmp
+ex_extra          -mouse_jsbterm     -sun_workshop      -xterm_clipboard
+extra_search      +mouse_netterm     +syntax            -xterm_save
   system vimrc file: "$VIM/vimrc"
     user vimrc file: "$HOME/.vimrc"
 2nd user vimrc file: "~/.vim/vimrc"
      user exrc file: "$HOME/.exrc"
       defaults file: "$VIMRUNTIME/defaults.vim"
  fall-back for $VIM: "/usr/local/share/vim"
Compilation: gcc -c -I. -Iproto -DHAVE_CONFIG_H -g -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1 
Linking: gcc -L. -fstack-protector-strong -rdynamic -Wl,-export-dynamic -L/home/linuxbrew/.linuxbrew/opt/libyaml/lib -Wl,-rpath,/home/linuxbrew/.linuxbrew/opt/libyaml/lib -L/home/linuxbrew/.linuxbrew/opt/[email protected]/lib -Wl,-rpath,/home/linuxbrew/.linuxbrew/opt/[email protected]/lib -L/home/linuxbrew/.linuxbrew/opt/readline/lib -Wl,-rpath,/home/linuxbrew/.linuxbrew/opt/readline/lib -Wl,-E -Wl,-rpath,/home/linuxbrew/.linuxbrew/Cellar/perl/5.32.1/lib/perl5/5.32.1/x86_64-linux-thread-multi/CORE -L/usr/local/lib -Wl,--as-needed -o vim -lm -ltinfo -lgpm -ldl -Wl,-E -Wl,-rpath,/home/linuxbrew/.linuxbrew/Cellar/perl/5.32.1/lib/perl5/5.32.1/x86_64-linux-thread-multi/CORE -fstack-protector-strong -L/usr/local/lib -L/home/linuxbrew/.linuxbrew/Cellar/perl/5.32.1/lib/perl5/5.32.1/x86_64-linux-thread-multi/CORE -lperl -lpthread -lnsl -ldl -lm -lcrypt -lutil -lc -L/usr/lib/python2.7/config-x86_64-linux-gnu -lpython2.7 -lpthread -ldl -lutil -lm -Xlinker -export-dynamic -Wl,-O1 -Wl,-Bsymbolic-functions -L/home/linuxbrew/.linuxbrew/Cellar/tcl-tk/8.6.11/lib -ltcl8.6 -ldl -lz -lpthread -lm -Wl,-rpath,/home/linuxbrew/.linuxbrew/Cellar/ruby/3.0.0_1/lib -L/home/linuxbrew/.linuxbrew/Cellar/ruby/3.0.0_1/lib -lruby -lm 

vimrc Outdated
if has('mac') && $VIM == '/usr/share/vim'
" Built-in vim doesn't support diffopt internal options
" ref: https://github.com/thoughtbot/dotfiles/issues/655#issuecomment-605019271
set diffopt-=internal
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must come before setting +vertical or throws errors.

@drmikehenry
Copy link
Owner

The linked article
thoughtbot/dotfiles#655 (which itself points to
https://www.micahsmith.com/blog/2019/11/fixing-vim-invalid-argument-diffopt-iwhite/)
is interesting; it's not often you come across a default value that isn't valid.

I saw that the line set diffopt+=algorithm:histogram is bypassed for Mac in this pull request; was that because algorithm:histogram is also invalid (or perhaps valid-but-meaningless because internal isn't valid)?
I'd have been tempted to fix this via a stand-alone "if mac then remove internal" clause before the existing manipulation of diffopt, unless it's also bad to set algorithm:histogram on the Mac. I don't have a Mac to test with, so I can't easily try out any of this myself.

@dtshepherd
Copy link
Contributor Author

Those other two options also throw errors sadly:

$ vim
Error detected while processing /Users/dshepherd/.vimrc[1]../Users/dshepherd/.dotfiles/vim/vim.symlink/vimrc:
line  999:
E474: Invalid argument: diffopt+=indent-heuristic
Press ENTER or type command to continue
$ vim
Error detected while processing /Users/dshepherd/.vimrc[1]../Users/dshepherd/.dotfiles/vim/vim.symlink/vimrc:
line 1000:
E474: Invalid argument: diffopt+=algorithm:histogram

I was pretty sure I tested that, but thanks for keeping me honest :)

@drmikehenry
Copy link
Owner

Was this with the bogus internal option still in diffopt? A test without the + in += would be more convincing:

set diffopt=indent-heuristic
set diffopt=algorithm:histogram

@dtshepherd
Copy link
Contributor Author

Here's the full list supported according to :help diffopt from macOS running Big Sur. I haven't tried on Monterey yet, but I imagine it will show the same results since Catalina also showed the same list.

'diffopt' 'dip'         string  (default "internal,filler,closeoff")             
                        global                                                   
                        {not available when compiled without the |+diff|         
                        feature}                                                 
        Option settings for diff mode.  It can consist of the following items.   
        All are optional.  Items must be separated by a comma.                   
                                                                                 
                filler          Show filler lines, to keep the text              
                                synchronized with a window that has inserted     
                                lines at the same position.  Mostly useful       
                                when windows are side-by-side and 'scrollbind'   
                                is set.                                          
                                                                                 
                context:{n}     Use a context of {n} lines between a change      
                                and a fold that contains unchanged lines.        
                                When omitted a context of six lines is used.     
                                When using zero the context is actually one,     
                                since folds require a line in between, also      
                                for a deleted line.                              
                                See |fold-diff|.                                 
                                                                                 
                iblank          Ignore changes where lines are all blank.  Adds  
                                the "-B" flag to the "diff" command if           
                                'diffexpr' is empty.  Check the documentation    
                                of the "diff" command for what this does         
                                exactly.                                         
                                NOTE: the diff windows will get out of sync,     
                                because no differences between blank lines are   
                                taken into account.                              
                                                                                 
                icase           Ignore changes in case of text.  "a" and "A"     
                                are considered the same.  Adds the "-i" flag     
                                to the "diff" command if 'diffexpr' is empty.    
                                                                                 
                iwhite          Ignore changes in amount of white space.  Adds   
                                the "-b" flag to the "diff" command if           
                                'diffexpr' is empty.  Check the documentation    
                                of the "diff" command for what this does         
                                exactly.  It should ignore adding trailing       
                                white space, but not leading white space.        
                                                                                 
                iwhiteall       Ignore all white space changes.  Adds            
                                the "-w" flag to the "diff" command if           
                                'diffexpr' is empty.  Check the documentation    
                                of the "diff" command for what this does         
                                exactly.                                         
                                                                                 
                iwhiteeol       Ignore white space changes at end of line.       
                                Adds the "-Z" flag to the "diff" command if      
                                'diffexpr' is empty.  Check the documentation    
                                of the "diff" command for what this does         
                                exactly.                                         
                                                                                 
                horizontal      Start diff mode with horizontal splits (unless   
                                explicitly specified otherwise).                 
                                                                                 
                vertical        Start diff mode with vertical splits (unless     
                                explicitly specified otherwise).                 
                                                                                 
                closeoff        When a window is closed where 'diff' is set      
                                and there is only one window remaining in the    
                                same tab page with 'diff' set, execute           
                                `:diffoff` in that window.  This undoes a        
                                `:diffsplit` command.                            
                                                                                 
                hiddenoff       Do not use diff mode for a buffer when it        
                                becomes hidden.                                  
                                                                                 
                foldcolumn:{n}  Set the 'foldcolumn' option to {n} when          
                                starting diff mode.  Without this 2 is used. 

@dtshepherd
Copy link
Contributor Author

No I made sure internal wasn't there for the test. But here's with just an =:

$ vim
Error detected while processing /Users/dshepherd/.vimrc[1]../Users/dshepherd/.dotfiles/vim/vim.symlink/vimrc:
line  988:
E474: Invalid argument: diffopt=indent-heuristic
line  989:
E474: Invalid argument: diffopt=algorithm:histogram
Press ENTER or type command to continue

@drmikehenry
Copy link
Owner

That's a helpful list. Compared to my build of Vim on ubuntu, these are missing in the Mac list:

followwrap
internal
indent-heuristic
algorithm:{text}

I don't see any of these options being used in the rest of Vimfiles anywhere (plugins or whatnot).

My other question has to do with the meaning of this test:

if has('mac') && $VIM == '/usr/share/vim'

Is this trying to detect a build of Vim that ships with OS X (vs. a self-compiled one that presumably supports diffopt=internal)? It seems a bit fiddly, though perhaps that's the best we can do. I haven't spent time trying to detect Mac-specific stuff like this; but elsewhere we've got code like this:

if has("mac") || has("macunix")

I'm not clear on the distinction between mac and macunix here, and whether we need both or just has('mac').

Also, I'm curious how the following test passes on Mac's Vim without the internal diff feature being present:

if v:version > 801 || (v:version == 801 && has('patch-8.1.0360'))

Did Apple patch out the feature? It doesn't appear to be configurable in the Vim source (diff.c). What Vim version does Mac's Vim report?

If we can't rely on the v:version detection because Apple has patched Vim, then we'll have to detect this somehow. Maybe the test above is the right idea, or maybe we could try to catch the E474: Invalid argument or something (which seems more directly what we're trying to test for, and more likely to be correct in the future if/when Apple starts supporting the diffopt=internal feature).

@drmikehenry
Copy link
Owner

Maybe something like this would detect the ability to use diffopt=internal. How does it behave on Mac Vim? It seems to behave as expected on my Ubuntu system. HasInternalDiff() would need to be called before any attempts to adjust diffopt; that function has the side-effect of removing internal from diffopt if it's not supported, making future diffopt twiddles safe.

function! HasInternalDiff()
    let parts = split(&diffopt, ',')
    let using_internal = (count(parts, 'internal') == 1)
    try
        set diffopt+=internal
        if !using_internal
            set diffopt-=internal
        endif
        return 1
    catch /E474:/
        set diffopt-=internal
        return 0
    endtry
endfunction

if HasInternalDiff()
    " Use the new internal diff feature with options:
    " - indent-heuristic: uses indentation to improve diffs.
    " - algorithm:histogram: an improved patience algorithm as used in Git.
    set diffopt+=internal
    set diffopt+=indent-heuristic
    set diffopt+=algorithm:histogram
endif

set diffopt+=vertical

@dtshepherd
Copy link
Contributor Author

Yeah, admittedly, I didn't like that $VIM check either. I stole it from the site I linked to and I wanted to make sure I didn't exclude a custom vim build on a Mac. Turns out the has() gets very confusing for mac too: https://github.com/vim/vim/blob/master/src/evalfunc.c#L4841-L4868.

In any case, I like your suggestion better, so let me give that a shot now and report back.

As an aside, I think Apple patched it out due to GPL issues: agude/dotfiles#2 (comment). But that's just trusting some random person on the internets opinion. It somewhat makes sense, but annoying to say the least...

Or any other vim build that doesnt support "diffopt=internal"
@dtshepherd
Copy link
Contributor Author

Ok that worked and was a much cleaner approach. I just force pushed a change adopting it.

Tested on MacOS Catalina and Big Sur, and on my Ubuntu 20.04 machine.

@drmikehenry drmikehenry merged commit f126c5f into drmikehenry:main Nov 4, 2021
@drmikehenry
Copy link
Owner

Sold; thanks for tracking this down :-)

@dtshepherd
Copy link
Contributor Author

Thanks Mike!

@craigmac
Copy link

FWIW, this bug just bit me on Monterey 12.3.1 as well, and it's still the same.

@dtshepherd
Copy link
Contributor Author

As-in, it is still broken?

@craigmac
Copy link

Still broken, yep. GPL most likely reason that makes sense, and why we'll never see a fix. vim from source it is, then.

@dtshepherd
Copy link
Contributor Author

Hmm, I wonder why this was working fine for me. I've been working on Linux mostly lately, I'll check my Mac this weekend and see what happens. I think the fix @drmikehenry suggested and merged worked around the limitation.

@craigmac
Copy link

You can work around it sort-of with that code yes, and you can get 'vanilla' diff, but you'll never be able to do set diffopt+=algorithm:patience for example, because it depends on xdiff lib which they cut out.

@jszakmeister
Copy link
Contributor

@craigmac I'm not sure I understand the complaint. This wasn't about trying to make something that isn't supported work. It was about gracefully adding in bits that would work. In this case, the internal option, and the fix does the trick--under Monterey too. Correct me if I'm wrong, I think you're just saying that Mac's implementation is still broken rather than the fix for the pull request, correct?

@craigmac
Copy link

You are correct. The issue is that macOS shipped Vim has the necessary patch level and feature that is supposed to support the new xdiff backed options algorithm:patience and indent-heuristic, yet it those appear in the diffopt we get errors. I filed an issue with Vim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants