[vim/vim] feat(diff): merge adjacent blocks using inline:word using configurable wordgaps option (PR #19098)
Harsh Kapse
Problem:
When using 'diffopt=inline:word', lines were excessively fragmented with punctuation creating separate highlight blocks, making it harder to read the diffs.
84319.png (view on web)Solution:
This PR replaces the old behavior of fragmentation in between words separated by 'spaces' or 'punctuation' to joining those words together and add a new wordgap option for 'diffopt' to configure this behavior.
Which when enabled inline:word is enabled the default value of wordgap:2 is used to provide continuous block of diff.
- This creates more readable diffs and closely matches GitHub's own diff display.
set diffopt=internal,filler,inline:word,wordgap:0 (old behavior)
84319.png (view on web)
set diffopt=internal,filler,inline:word,wordgap:2 (new default behavior)
82737.png (view on web)
set diffopt=internal,filler,inline:word,wordgap:5 (max)
23810.png (view on web)
Backwards Compatibility
- The new behavior is default with configurable wordgap: option: wordgap:0 to wordgap:5, where wordgap:2 is default
- Old behavior of inline:word can still be able to be used, using wordgap:0
Usage
Enable with:
:set diffopt+=inline:word,wordgap:(0-5)
Implementation
- Added
diff_word_gapvariable (default: 2) - Parsing in
diffopt_changed()forwordgap:Nsyntax - Function
diff_refine_inline_word_highlight()merges adjacent blocks
Testing
- Added tests in
test_diffmode.vim - Tested with punctuation, whitespace, multi-byte chars
Related Work
This feature is also being contributed to Neovim in parallel:
- Neovim PR: neovim/neovim#37085
The Neovim PR contains additional comprehensive tests and discussion about
the feature design.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/19098
Commit Summary
- 9a188c4 diff: add wordgap option for inline word diff
- 2586325 runtime(doc): document wordgap option for diffopt
File Changes
(8 files)
- M runtime/doc/options.txt (11)
- M runtime/doc/tags (1)
- M src/diff.c (143)
- A src/testdir/dumps/Test_diff_inline_wordgap_01.dump (20)
- A src/testdir/dumps/Test_diff_inline_wordgap_02.dump (20)
- A src/testdir/dumps/Test_diff_inline_wordgap_03.dump (20)
- A src/testdir/dumps/Test_diff_inline_wordgap_04.dump (20)
- M src/testdir/test_diffmode.vim (58)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Harsh Kapse
HarshK97 left a comment (vim/vim#19098)@chrisbra @ychin sorry for inconvenience, I had 2 branch's for this while porting it, and deleted the wrong one
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
zeertzjq
@zeertzjq commented on this pull request.
> + wordgap:{n} When highlighting inline differences with
+ 'diffopt' set to "inline:word", consider words
+ to be different only if they are separated
+ by at least {n} changed characters. This
+ helps avoid highlighting small changes
+ within words that are mostly the same.
+ The default setting is "wordgap:2".
+ The max setting is "wordgap:5".
The indent here is inconsistent. Try selecting these lines in Visual mode and running :retab!.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Harsh Kapse
@HarshK97 pushed 1 commit.
- 4166d80 runtime(doc): proper indentation for documentation of 'wordgap'
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Harsh Kapse
@HarshK97 pushed 4 commits.
- 5a83925 diff: add wordgap option for inline word diff
- bdba94e runtime(doc): document wordgap option for diffopt
- 4ddc292 runtime(doc): proper indentation for documentation of 'wordgap'
- ee9fd2c test(diff): added entry for wordgap in "Test all possible values" alongside other options
You are receiving this because you are subscribed to this thread.
Harsh Kapse
HarshK97 left a comment (vim/vim#19098)Hey @zeertzjq, can you rerun the CI tests, as the tests failed due to
- Packages no able to install due to being in middle of mirror sync
- build failure in if_perl.xs which was already fixed in aa58f1f
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
zeertzjq
zeertzjq left a comment (vim/vim#19098)Rebase
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Harsh Kapse
@HarshK97 pushed 6 commits.
- 38aca78 diff: add wordgap option for inline word diff
- f421a0c runtime(doc): document wordgap option for diffopt
- 9e7a75f runtime(doc): proper indentation for documentation of 'wordgap'
- c8aca36 test(diff): added entry for wordgap in "Test all possible values" alongside other options
- 121d399 completion(diff): added wordgap option to optionstr.c to provide completion
- e6e1097 test(diff): added compresenhive tests to represent the new behavior
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#19098)Sorry for the slow response.
Personally, I kind of dread adding yet more options to diffopt itself, which is getting very heavily bloated. This is especially true for someoption:<number> style options which I think could be a bit unwieldy when it's added to the defaults. Vim doesn't really give you a way to remove the default in your vimrc via set-=, because Vim doesn't have a syntax to do something like set diffopt-=wordgaps:*. Instead, you have to know the exact tuning number used in the default.
And more options in general just makes it harder for a user to learn and I think we are approaching the point where diffopt is getting quite long and complicated (I do recognize that I was the last person to add the last two new options to diffopt…). When I was implementing inline:char I intentionally didn't add any tunable settings to the character refine step (which this PR based the word refine step on) for this reason.
If we want this, I wonder if it makes sense to just not expose this as an option (for now). I think sometimes too many options could be counter-productive and sets up long term support burdens, and lots of "why not expose this implementation detail too" requests. Most diff tools ultimately just pick some heuristics that they think is good for most people and go with them, and only expose high level options (e.g. algorithm type, whitespace handling, etc). But yes this will result in a slight change in behavior which may not please everyone, but at least this will force us to try to make sure the new behavior works well rather than just letting the user figure it out.
Otherwise I'm leaning on setting up another option along the lines of diffinlinewordgap to make it easier to use and tune. The main concern I still have is that exposing options like this is going to limit future changes we may want to make to the refinement step where we massage the output to look nice.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Harsh Kapse
HarshK97 left a comment (vim/vim#19098)Personally, I kind of dread adding yet more options to
diffoptitself, which is getting very heavily bloated. This is especially true forsomeoption:<number>style options which I think could be a bit unwieldy when it's added to the defaults. Vim doesn't really give you a way to remove the default in your vimrc viaset-=, because Vim doesn't have a syntax to do something likeset diffopt-=wordgaps:*. Instead, you have to know the exact tuning number used in the default.
I get your concern, and I will remove the wordgap option, just a question, what should be heuristic for inline:word be for the number of gaps merged? Default 2? or 5 suggested by Lewis in Neovim PR or something in between?
For now I will use 5, let me know if I have to make any changes
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
