[vim/vim] WIP: Improve diff inline highlighting using per-character/word diff (PR #16881)
Yee Cheng Chin
Problem: Diff mode's inline highlighting is lackluster. It only performs a line-by-line comparison, and calculates a single shortest range within a line that could encompass all the changes. In lines with multiple changes, or those that span multiple lines, this approach tends to end up highlighting much more than necessary.
Solution: Implement new inline highlighting modes by doing per-character or per-word diff within the diff block, and highlight only the relevant parts.
This change introduces a new diffopt option "inline:". Setting to "none" will disable all inline highlighting, "simple" (the default) will use the old behavior, "char" / "word" will perform a character/word-wise diff of the texts within each diff block and only highlight the differences.
The new char/word inline diff only use the internal xdiff, and will respect diff options such as algorithm choice, icase, and misc iwhite options. indent-heuristics is always on to perform better sliding.
For character highlight, a post-process of the diff results is first applied before we show the highlight. This is because a naive diff will create a result with a lot of small diff chunks and gaps, due to the repetitive nature of individual characters. The post-process is a heuristic-based refinement that attempts to merge adjacent diff blocks if they are separated by a short gap (1-3 characters), and can be further tuned in the future for better results. This process results in more characters than necessary being highlighted but overall less visual noise.
For word highlight, always use first buffer's iskeyword definition. Otherwise if each buffer has different iskeyword settings we would not be able to group words properly.
The char/word diffing is always per-diff block, not per line, meaning that changes that span multiple lines will show up correctly. Added/removed newlines are not shown by default, but if the user has 'list' set (with "eol" listchar defined), the eol character will be be highlighted correctly for the specific newline characters.
Also, add a new "DiffTextAdd" highlight group linked to "DiffText" by default. It allows color schemes to use different colors for texts that have been added within a line versus modified.
This doesn't interact with linematch perfectly currently. The linematch feature splits up diff blocks into multiple smaller blocks for better visual matching, which makes inline highlight less useful especially for multi-line change (e.g. a line is broken into two lines). This could be addressed in the future.
As a side change, this also removes the bounds checking introduced to diff_read() as they were added to mask existing logic bugs that were properly fixed in #16768.
Notes
Marking this as draft/WIP as I am still fixing up a couple minor issues, and need to write a fair amount of tests to cover this feature. That said, the feature should be fully usable (other than diff_hlID() not working with this yet), and I would love it if people could give it a try and provide feedbacks. Some extra notes below for those interested:
Some examples of how it looks like:
inline:simple(old behavior)inline:charinline:char,algorithm:patience,icaseinline:word
More complicated multi-line change (the diff comes from this PR itself):
image.png (view on web)Color schemes can define DiffTextAdd to highlight added texts differently from changed texts:
Name
Every diff program seems to call this feature something different. I mostly settled on "inline highlight" but open to other suggestions.
character diff
As mentioned above, I added a refinement post-process step to clean up the diffs. Just to show the problem and the before/after (first image is a naive implementation, and second is the one with a post-process step to merge small gaps):
image.png (view on web) image.png (view on web)From surveying other popular diff programs, the ones that use character diff for inline highlighting (e.g. Meld, VSCode) all have to do similar work, via pre/post-processing to massage the diff results a little bit. VSCode for example does quite a bit of post-processing (merging small gaps, extending small diffs to word boundaries, etc), and I tried to just keep this minimal and go for the largest bang for the buck to make the result looks decent without over-complicating the implementation (implemented in diff_refine_inline_char_highlight()).
Sliders
Another issue is sliders, where you could slide a diff left or right due to repeating texts. To fix this, I'm always forcing on indent-heuristics to piggyback on it for free white-space sliding alignment. FWIW I think indent-heuristics should be in the default diffopt setting value (since Git has had it on by default for a long time) but that's another discussion. Without / with indent-heuristics:
Without indent-heuristics (you can see how they don't align at whitespace boundaries):
image.png (view on web)
With indent-heuristics forced on:
image.png (view on web)
Note that this is not perfect. Since we are hijacking a feature designed for line diffing (where xdiff uses the white space and indentation of lines as a heuristics) rather than per-character diff, this doesn't help solve sliders at symbols (ideally the highlight should border the "+" sign):
image.png (view on web)If we want to fix this we should probably modify xdiff to add a new heuristics mode that's aware that it's doing character diffing and has scoring designed for intraline diff (e.g. VSCode has a scoring system ranked from whitespace, symbols, keywords, etc).
Word diff
This basically just splits the diff block along word difference instead of character. For the most part it works fine. As mentioned I'm defining a word based on the first buffer's iskeyword setting to have a consistent definition. Some other programs that use word diff (e.g. diffchar.vim or Git's own word-diff feature) allows you to define words in different ways or via a regex pattern. For first pass I would rather just keep it simple and just provide a single inline:word option for now. We could always add a new inline:pattern etc setting later as it's extensible.
Newline
As noted above, newlines can be diffed as well, but it will only show up when list option is set.
I was debating back and forth how to show this information. VSCode for example highlights the entire rest of the line in the changed color, which I find to be too much. Meld essentially automatically shows the equivalent of listchar only where a change happens. I think this could be an interesting future extension, something along the lines of set diffopt+=auto-list where we automatically show list only when there is a diff highlight.
There's also an issue here where more than half of the color schemes (including bundled ones) I tried tend to not work well when list is on during diffing. I am still not sure whether this is because of how Vim combines the highlight attributes or it's the color schemes' fault. This is usually particularly problematic if the color scheme uses gui=inverse for NonText group. That's why all the screenshots I took were using the third-party "iceberg" color scheme as it works the best in this situation.
linematch
Unfortunately this PR doesn't work perfectly with linematch as mentioned above. Here's a showcase of the issue:
linematch off:
image.png (view on web)
linematch on:
image.png (view on web)
Note how in the linematch example, the code splits the diff block into two, and therefore inline highlighting fails to recognize that the first part of the texts ("another line of text…" corresponds to the other buffer) and it's marked as changed instead. I have some ideas how to fix it but for scope issues decided not to tackle it for now.
Anticipated FAQs
Why not just take Git's word-diff code when we use xdiff already?
Word diff is an application layer feature in Git, not part of the xdiff library. It does something similar to this PR but is specific to Git. It isn't something we can just take.
Why not a plugin?
There's an existing plugin (diffchar.vim) that already does something similar. It does interesting things but it's limited by Vim's API. For example, it does not do live update when typing, does not handle multi-line blocks, and does not support multi-buffer diff'ing (this PR handles it fine as it uses Vim's builtin diff block merging in diff_read). Vim also has more internal knowledge and can control how the rendering is done instead of a plugin needing to bend over backwards to get this to work.
I also think this feature is a basic requirement for a competent diff program and should be part of the overall design. We should not force users to install a plugin to get access to such features.
It does have some interesting features, such as the ability to highlight corresponding texts in the other buffer when cursor is on a highlighted chunk; and for chunks where there's an added piece of text (meaning there's no corresponding texts in the other buffer), use underline in neighbor texts to highlight where it is inserted in the other buffer. I decided not to clone all of them in this PR.
I do think later on Vim could expose more APIs both for accessing the diff highlight information, and/or provide ways to modify it. E.g. If another diff plugin like Difftastic (which does syntax diffing using treesitter) wants to hook into Vim/Neovim, it should have an API where it could define where exactly the highlight should be. We could add something like set diffopt+=inline:expr.
Relevant links
neovim/neovim#29549
neovim/neovim#3433
https://github.com/rickhowe/diffchar.vim
There are further diff changes / improvements I would like to work on, but probably will wait to see how this PR goes first. Future potential options include making this work better with linematch, move detection, expose xdiff's --anchored feature, better API integration, etc.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/16881
Commit Summary
- 47f892e Improve diff inline highlighting using per-character/word diff
File Changes
(13 files)
- M runtime/doc/diff.txt (29)
- M runtime/doc/options.txt (20)
- M runtime/doc/syntax.txt (3)
- M runtime/doc/tags (1)
- M src/change.c (3)
- M src/diff.c (746)
- M src/drawline.c (52)
- M src/highlight.c (1)
- M src/optiondefs.h (2)
- M src/optionstr.c (15)
- M src/proto/diff.pro (4)
- M src/structs.h (38)
- M src/vim.h (5)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
@ychin pushed 1 commit.
- 623a6eb Improve diff inline highlighting using per-character/word diff
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
@ychin pushed 4 commits.
- 69981bb Fix normal diff icase handling to correctly handle composing chars
- 282afcd Fix inline diff to not assume first buffer is populated
- 8c1e8e6 Fix icase logic to handle different byte lengths correctly
- 5bca11a Add tests for inline highlight
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
@ychin pushed 10 commits.
- 8a7dad2 Improve diff inline highlighting using per-character/word diff
- becd6d2 Fix multi-buffer diff to work
- 841714b Fix multi-buffer diff to only use the non-empty blocks
- 177e230 Fix diff_hlID() to work with inline diff
- 3fef300 Fix diff_hlID to respect diff flags change
- a2309e9 Fix test_options to pass
- dc2cdc2 Fix normal diff icase handling to correctly handle composing chars
- 1a831fb Fix inline diff to not assume first buffer is populated
- c522d67 Fix icase logic to handle different byte lengths correctly
- 8c33618 Add tests for inline highlight
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#16881)Ok this PR should be ready. Let me know if there are any feedbacks or thoughts. Implemented tests and previously the PR was also crashing in Linux / Windows builds which are also now fixed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
zeertzjq
@zeertzjq commented on this pull request.
> @@ -4392,7 +4407,7 @@ A jump table for the options with a short description can be found at |Q_op|. v:Visual,V:VisualNOS,w:WarningMsg, W:WildMenu,f:Folded,F:FoldColumn, A:DiffAdd,C:DiffChange,D:DiffDelete, - T:DiffText,>:SignColumn,-:Conceal, + T:DiffText,E:DiffTextAdd,>:SignColumn,-:Conceal,
This line is too long
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
zeertzjq
@zeertzjq commented on this pull request.
In src/testdir/test_diffmode.vim:
> @@ -2350,6 +2373,162 @@ func Test_diff_topline_noscroll() call StopVimInTerminal(buf) endfunc +" Test inline highlighting which shows what's different within each diff block +funct Test_diff_inline()⬇️ Suggested change
-funct Test_diff_inline() +func Test_diff_inline()
In src/testdir/test_diffmode.vim:
> + call VerifyInternal(buf, "Test_diff_inline_multiline_04", " diffopt+=inline:word,iwhiteeol") + call VerifyInternal(buf, "Test_diff_inline_multiline_05", " diffopt+=inline:char,iwhiteall") + call VerifyInternal(buf, "Test_diff_inline_multiline_06", " diffopt+=inline:word,iwhiteall") + + " newline should be highlighted too when 'list' is set + call term_sendkeys(buf, ":windo set list\<CR>") + call VerifyInternal(buf, "Test_diff_inline_multiline_07", " diffopt+=inline:char") + call VerifyInternal(buf, "Test_diff_inline_multiline_08", " diffopt+=inline:char,iwhite") + call VerifyInternal(buf, "Test_diff_inline_multiline_09", " diffopt+=inline:char,iwhiteeol") + call VerifyInternal(buf, "Test_diff_inline_multiline_10", " diffopt+=inline:char,iwhiteall") + call term_sendkeys(buf, ":windo set nolist\<CR>") + + call StopVimInTerminal(buf) +endfunc + +funct Test_diff_inline_multibuffer()⬇️ Suggested change
-funct Test_diff_inline_multibuffer() +func Test_diff_inline_multibuffer()
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
> + last character that is different (searching + from the end of the line). The text in + between is highlighted. This means that parts + in the middle that are still the same are + highlighted anyway. The 'diffopt' flags + "iwhite" and "icase" are used here. + With `inline:` set to "char" or "word", Vim + uses the internal diff library to perform a + detailed diff between the changed blocks and + highlight the exact difference between the + two. Will respect any 'diffopt' flag that + affects internal diff. +|hl-DiffTextAdd| DiffTextAdd Added text inside a Changed line. Similar to + DiffText, but used when there is no + corresponding text in other buffers. Will not + be used when `inline:` is set to "simple".⬇️ Suggested change
- be used when `inline:` is set to "simple".. + be used when `inline:` is set to "simple" or "none".
> + behavior depends on the `inline:` setting in + 'diffopt'. + With `inline:` set to "simple", Vim finds the + first character that is different, and the + last character that is different (searching + from the end of the line). The text in + between is highlighted. This means that parts + in the middle that are still the same are + highlighted anyway. The 'diffopt' flags + "iwhite" and "icase" are used here. + With `inline:` set to "char" or "word", Vim + uses the internal diff library to perform a + detailed diff between the changed blocks and + highlight the exact difference between the + two. Will respect any 'diffopt' flag that + affects internal diff.⬇️ Suggested change
- affects internal diff. + affects internal diff. Not used when `inline:` set to "none".
> @@ -2975,6 +2975,21 @@ A jump table for the options with a short description can be found at |Q_op|.
Use the indent heuristic for the internal
diff library.
+ inline:{text} Highlight inline differences within a change.
+ See |view-diffs|. Supported values are:
+
+ none Do not perform inline highlighting.
+ simple Highlight from first different
+ character to the last one in each
+ line. This is the default if nothing
+ is set.
Hm, there doesn't seem to be a difference between, :set diffopt=internal,inline:simple and :set diffopt=internal. In other words, do we need the subvalue simple, since the same can be achieved when not setting inline at all?
In src/drawline.c:
> @@ -1223,6 +1223,9 @@ win_line(
#ifdef FEAT_DIFF
int change_start = MAXCOL; // first col of changed area
int change_end = -1; // last col of changed area
+ diffline_T line_changes;
+ CLEAR_FIELD(line_changes);
That looks a bit strange between all the declarations, should this be moved to (above line 1481) instead?
In src/drawline.c:
> @@ -2438,13 +2460,30 @@ win_line(
#ifdef FEAT_DIFF
if (wlv.diff_hlf != (hlf_T)0)
{
+ if (line_changes.num_changes > 0
+ && change_index >= 0
+ && change_index < line_changes.num_changes - 1)
+ {
+ if (ptr - line >= line_changes.changes[change_index+1].dc_start[line_changes.bufidx])
+ {
we can remove the braces here
In src/diff.c:
> @@ -579,6 +591,12 @@ diff_alloc_new(tabpage_T *tp, diff_T *dprev, diff_T *dp)
tp->tp_first_diff = dnew;
else
dprev->df_next = dnew;
+
+ CLEAR_FIELD(dnew->df_lnum);
+ CLEAR_FIELD(dnew->df_count);
perhaps use ALLOC_CLEAR_ONE(diff_T) above and then get rid for CLEAR_FIELD here?
In src/diff.c:
> + diffline->changes = &simple_diffline_change;
+ diffline->num_changes = 1;
+ diffline->bufidx = idx;
+ diffline->lineoff = lnum - dp->df_lnum[idx];
+
+ simple_diffline_change.dc_start[idx] = change_start;
+ simple_diffline_change.dc_end[idx] = change_end;
+ simple_diffline_change.dc_start_lnum_off[idx] = off;
+ simple_diffline_change.dc_end_lnum_off[idx] = off;
+ return ret;
+ }
+
+ // Use inline diff algorithm.
+ // The diff changes are usually cached so we check that first.
+ if (!dp->has_changes)
+ {
can leave out the braces
In src/diff.c:
> + if (idx == DB_COUNT) // cannot happen
+ return FALSE;
+
+ // search for a change that includes "lnum" in the list of diffblocks.
+ FOR_ALL_DIFFBLOCKS_IN_TAB(curtab, dp)
+ if (lnum <= dp->df_lnum[idx] + dp->df_count[idx])
+ break;
+ if (dp->is_linematched)
+ {
+ while (dp && dp->df_next
+ && lnum == dp->df_count[idx] + dp->df_lnum[idx]
+ && dp->df_next->df_lnum[idx] == lnum)
+ dp = dp->df_next;
+ }
+ if (dp == NULL || diff_check_sanity(curtab, dp) == FAIL)
+ {
can leave out the braces
In src/diff.c:
> + if (lnum <= dp->df_lnum[idx] + dp->df_count[idx])
+ break;
+ if (dp->is_linematched)
+ {
+ while (dp && dp->df_next
+ && lnum == dp->df_count[idx] + dp->df_lnum[idx]
+ && dp->df_next->df_lnum[idx] == lnum)
+ dp = dp->df_next;
+ }
+ if (dp == NULL || diff_check_sanity(curtab, dp) == FAIL)
+ {
+ return FALSE;
+ }
+
+ if (lnum - dp->df_lnum[idx] > INT_MAX)
+ {
same here
In src/diff.c:
> + FOR_ALL_DIFFBLOCKS_IN_TAB(curtab, dp)
+ if (lnum <= dp->df_lnum[idx] + dp->df_count[idx])
+ break;
+ if (dp->is_linematched)
+ {
+ while (dp && dp->df_next
+ && lnum == dp->df_count[idx] + dp->df_lnum[idx]
+ && dp->df_next->df_lnum[idx] == lnum)
+ dp = dp->df_next;
+ }
+ if (dp == NULL || diff_check_sanity(curtab, dp) == FAIL)
+ {
+ return FALSE;
+ }
+
+ if (lnum - dp->df_lnum[idx] > INT_MAX)
MAXLNUM?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
@ychin commented on this pull request.
> + last character that is different (searching + from the end of the line). The text in + between is highlighted. This means that parts + in the middle that are still the same are + highlighted anyway. The 'diffopt' flags + "iwhite" and "icase" are used here. + With `inline:` set to "char" or "word", Vim + uses the internal diff library to perform a + detailed diff between the changed blocks and + highlight the exact difference between the + two. Will respect any 'diffopt' flag that + affects internal diff. +|hl-DiffTextAdd| DiffTextAdd Added text inside a Changed line. Similar to + DiffText, but used when there is no + corresponding text in other buffers. Will not + be used when `inline:` is set to "simple".
I don't think this is correct. If inline:none is set, both DiffTextAdd and DiffText are not going to be used. Only DiffAdd and DiffChange will be used.
Note that DiffTextAdd is a new highlight group and not the same as DiffAdd. This is intentional because in some colorschemes it would not work well if we just use DiffAdd within a changed line.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
@ychin commented on this pull request.
In src/diff.c:
> @@ -579,6 +591,12 @@ diff_alloc_new(tabpage_T *tp, diff_T *dprev, diff_T *dp)
tp->tp_first_diff = dnew;
else
dprev->df_next = dnew;
+
+ CLEAR_FIELD(dnew->df_lnum);
+ CLEAR_FIELD(dnew->df_count);
Oh I didn't see that we could do that. Will do.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
@ychin pushed 1 commit.
- 22f01db Misc code cleanups per review feedbacks
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
> + if (lnum <= dp->df_lnum[idx] + dp->df_count[idx])
+ break;
+ if (dp->is_linematched)
+ {
+ while (dp && dp->df_next
+ && lnum == dp->df_count[idx] + dp->df_lnum[idx]
+ && dp->df_next->df_lnum[idx] == lnum)
+ dp = dp->df_next;
+ }
+ if (dp == NULL || diff_check_sanity(curtab, dp) == FAIL)
+ {
+ return FALSE;
+ }
+
+ if (lnum - dp->df_lnum[idx] > INT_MAX)
+ {
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
> + if (idx == DB_COUNT) // cannot happen
+ return FALSE;
+
+ // search for a change that includes "lnum" in the list of diffblocks.
+ FOR_ALL_DIFFBLOCKS_IN_TAB(curtab, dp)
+ if (lnum <= dp->df_lnum[idx] + dp->df_count[idx])
+ break;
+ if (dp->is_linematched)
+ {
+ while (dp && dp->df_next
+ && lnum == dp->df_count[idx] + dp->df_lnum[idx]
+ && dp->df_next->df_lnum[idx] == lnum)
+ dp = dp->df_next;
+ }
+ if (dp == NULL || diff_check_sanity(curtab, dp) == FAIL)
+ {
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
> + diffline->changes = &simple_diffline_change;
+ diffline->num_changes = 1;
+ diffline->bufidx = idx;
+ diffline->lineoff = lnum - dp->df_lnum[idx];
+
+ simple_diffline_change.dc_start[idx] = change_start;
+ simple_diffline_change.dc_end[idx] = change_end;
+ simple_diffline_change.dc_start_lnum_off[idx] = off;
+ simple_diffline_change.dc_end_lnum_off[idx] = off;
+ return ret;
+ }
+
+ // Use inline diff algorithm.
+ // The diff changes are usually cached so we check that first.
+ if (!dp->has_changes)
+ {
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
> @@ -579,6 +591,12 @@ diff_alloc_new(tabpage_T *tp, diff_T *dprev, diff_T *dp)
tp->tp_first_diff = dnew;
else
dprev->df_next = dnew;
+
+ CLEAR_FIELD(dnew->df_lnum);
+ CLEAR_FIELD(dnew->df_count);
Done.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
@ychin commented on this pull request.
In src/drawline.c:
> @@ -2438,13 +2460,30 @@ win_line(
#ifdef FEAT_DIFF
if (wlv.diff_hlf != (hlf_T)0)
{
+ if (line_changes.num_changes > 0
+ && change_index >= 0
+ && change_index < line_changes.num_changes - 1)
+ {
+ if (ptr - line >= line_changes.changes[change_index+1].dc_start[line_changes.bufidx])
+ {
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
@ychin commented on this pull request.
In src/diff.c:
> + FOR_ALL_DIFFBLOCKS_IN_TAB(curtab, dp)
+ if (lnum <= dp->df_lnum[idx] + dp->df_count[idx])
+ break;
+ if (dp->is_linematched)
+ {
+ while (dp && dp->df_next
+ && lnum == dp->df_count[idx] + dp->df_lnum[idx]
+ && dp->df_next->df_lnum[idx] == lnum)
+ dp = dp->df_next;
+ }
+ if (dp == NULL || diff_check_sanity(curtab, dp) == FAIL)
+ {
+ return FALSE;
+ }
+
+ if (lnum - dp->df_lnum[idx] > INT_MAX)
The reason for using INT_MAX is that off is an int, not linenr_T, in the original code for diff_find_change(). I guess I could have refactored off to use that instead, but just didn't feel like changing it. I could change it to linenr_T if you want.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
@ychin commented on this pull request.
In src/drawline.c:
> @@ -1223,6 +1223,9 @@ win_line(
#ifdef FEAT_DIFF
int change_start = MAXCOL; // first col of changed area
int change_end = -1; // last col of changed area
+ diffline_T line_changes;
+ CLEAR_FIELD(line_changes);
DONE
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
@ychin commented on this pull request.
> + behavior depends on the `inline:` setting in + 'diffopt'. + With `inline:` set to "simple", Vim finds the + first character that is different, and the + last character that is different (searching + from the end of the line). The text in + between is highlighted. This means that parts + in the middle that are still the same are + highlighted anyway. The 'diffopt' flags + "iwhite" and "icase" are used here. + With `inline:` set to "char" or "word", Vim + uses the internal diff library to perform a + detailed diff between the changed blocks and + highlight the exact difference between the + two. Will respect any 'diffopt' flag that + affects internal diff.
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
> + last character that is different (searching + from the end of the line). The text in + between is highlighted. This means that parts + in the middle that are still the same are + highlighted anyway. The 'diffopt' flags + "iwhite" and "icase" are used here. + With `inline:` set to "char" or "word", Vim + uses the internal diff library to perform a + detailed diff between the changed blocks and + highlight the exact difference between the + two. Will respect any 'diffopt' flag that + affects internal diff. +|hl-DiffTextAdd| DiffTextAdd Added text inside a Changed line. Similar to + DiffText, but used when there is no + corresponding text in other buffers. Will not + be used when `inline:` is set to "simple".
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
@ychin commented on this pull request.
> @@ -2975,6 +2975,21 @@ A jump table for the options with a short description can be found at |Q_op|.
Use the indent heuristic for the internal
diff library.
+ inline:{text} Highlight inline differences within a change.
+ See |view-diffs|. Supported values are:
+
+ none Do not perform inline highlighting.
+ simple Highlight from first different
+ character to the last one in each
+ line. This is the default if nothing
+ is set.
I feel like for a list of options like that, it's best for symmetry reasons to have all the options available. It's easier to parse in the docs and in auto-complete. It also makes it easier to change the defaults in the future without screwing people who explicitly set their inline: values.
This is also similar to how algorithm: is implemented. algorithm:myers is the default, but we allow the user to explicitly set it, while it's also ok to not set the algorithm in diffopt, since this makes it easier on the user ergonomics to not having to specify an overly long verbose diffopt.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
> @@ -2975,6 +2975,21 @@ A jump table for the options with a short description can be found at |Q_op|.
Use the indent heuristic for the internal
diff library.
+ inline:{text} Highlight inline differences within a change.
+ See |view-diffs|. Supported values are:
+
+ none Do not perform inline highlighting.
+ simple Highlight from first different
+ character to the last one in each
+ line. This is the default if nothing
+ is set.
Yeah, I came to the same conclusion when looking at the algorithm suboption. Makes sense. Thanks.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
In src/diff.c:
> + FOR_ALL_DIFFBLOCKS_IN_TAB(curtab, dp)
+ if (lnum <= dp->df_lnum[idx] + dp->df_count[idx])
+ break;
+ if (dp->is_linematched)
+ {
+ while (dp && dp->df_next
+ && lnum == dp->df_count[idx] + dp->df_lnum[idx]
+ && dp->df_next->df_lnum[idx] == lnum)
+ dp = dp->df_next;
+ }
+ if (dp == NULL || diff_check_sanity(curtab, dp) == FAIL)
+ {
+ return FALSE;
+ }
+
+ if (lnum - dp->df_lnum[idx] > INT_MAX)
I think this is fine, thanks. Is this something we should document, that it is limited to INT_MAX changes per line?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
@ychin commented on this pull request.
In src/diff.c:
> + FOR_ALL_DIFFBLOCKS_IN_TAB(curtab, dp)
+ if (lnum <= dp->df_lnum[idx] + dp->df_count[idx])
+ break;
+ if (dp->is_linematched)
+ {
+ while (dp && dp->df_next
+ && lnum == dp->df_count[idx] + dp->df_lnum[idx]
+ && dp->df_next->df_lnum[idx] == lnum)
+ dp = dp->df_next;
+ }
+ if (dp == NULL || diff_check_sanity(curtab, dp) == FAIL)
+ {
+ return FALSE;
+ }
+
+ if (lnum - dp->df_lnum[idx] > INT_MAX)
Hmm, do we really need to document this? That's a lot of lines... I feel like this is more a failsafe than a realistic user scenarios of a diff user.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
@chrisbra commented on this pull request.
In src/diff.c:
> + FOR_ALL_DIFFBLOCKS_IN_TAB(curtab, dp)
+ if (lnum <= dp->df_lnum[idx] + dp->df_count[idx])
+ break;
+ if (dp->is_linematched)
+ {
+ while (dp && dp->df_next
+ && lnum == dp->df_count[idx] + dp->df_lnum[idx]
+ && dp->df_next->df_lnum[idx] == lnum)
+ dp = dp->df_next;
+ }
+ if (dp == NULL || diff_check_sanity(curtab, dp) == FAIL)
+ {
+ return FALSE;
+ }
+
+ if (lnum - dp->df_lnum[idx] > INT_MAX)
Yeah, agree. It's an implementation detail. Let's keep it like this for now. Thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#16881)While not strictly required, does it make sense, to change the default diffopt to include inline:simple ? Since that technically does not change the default behaviour, it may makes it a bit easier to notice that there is this great new improvement?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#16881)While not strictly required, does it make sense, to change the default
diffoptto includeinline:simple? Since that technically does not change the default behaviour, it may makes it a bit easier to notice that there is this great new improvement?
I did wonder about that. I think one issue is that right now the option parsing in this PR is a little strict (probably too strict) and will throw an error if you have multiple inline: values. So if we add a new default value it would make it annoying to do set diffopt+=inline:char in your vimrc since you have to remove the original one first. That said, we could and probably should relax that requirement a bit and always just use the last value, similar to algorithm: for consistency. We just need to specify that behavior in docs. This way diffopt=inline:simple,inline:char would still work and just use char-diff.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Nick Jensen
nickspoons left a comment (vim/vim#16881)Is it possible to update the set ...+= mechanism to replace an existing inline:... when a new one is added? This would be useful with algorithm: too.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#16881)Is it possible to update the
set ...+=mechanism to replace an existinginline:...when a new one is added? This would be useful withalgorithm:too.
This is an interesting thought. This is really a global change though as it applies to the behavior of :set command. We currently define some settings with a P_COLON flag to indicate an option that has a colon syntax, mostly used for auto-complete. This applies to 'diffopt' but also other settings like 'highlight', 'completeopt', 'guifont', and 'wildmode'. I think this substitute behavior actually makes sense for most of these options, and should be done globally. E.g. it doesn't make sense that you can define set mopt=hit-enter,history:100,history:1000 or set previewpopup=height:10,height:100. I think there are a fair bit of nuances here with some options though, like wildmode. It's probably best to wait till a later change to do something like this.
Another corollary is that it may make sense to make set diffopt-=inline: just remove whatever inline option we have set (similarly for set diffopt-=algorithm: or other options as discussed above).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
@ychin pushed 1 commit.
- 6a7e42e Add 'inline:simple' to defaults, and allow multiple 'inline:' in diffopt
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#16881)Ok, I just pushed a change to just add inline:simple to the defaults, and allow for duplicates. Feels to me being relaxed in parsing is probably better and allows for more experimentation and flexibility for the user. Also consistent with how diffopt=algorithm: is parsed as mentioned above.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#16881)thanks all!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Maxim Kim
habamax left a comment (vim/vim#16881)@ychin thank you!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#16881)Unfortunately, this introduced a few Coverity warnings:
23. Condition !(diff_flags & (98304 /* 0x8000 | 0x10000 */)), taking true branch.
3587 if (!(diff_flags & ALL_INLINE_DIFF) || diff_internal_failed())
3588 {
3589 // Use simple algorithm
3590 int change_start = MAXCOL; // first col of changed area
3591 int change_end = -1; // last col of changed area
3592 int ret;
3593
3594 ret = diff_find_change_simple(wp, lnum, dp, idx, &change_start, &change_end);
3595
3596 // convert from inclusive end to exclusive end per diffline's contract
3597 change_end += 1;
3598
3599 // Create a mock diffline struct. We always only have one so no need to
3600 // allocate memory.
24. assignment: Assigning: idx = diff_buf_idx(wp->w_buffer). The value of idx is now between 1 and 8 (inclusive).
3601 idx = diff_buf_idx(wp->w_buffer);;
3602 CLEAR_FIELD(simple_diffline_change);
3603 diffline->changes = &simple_diffline_change;
3604 diffline->num_changes = 1;
3605 diffline->bufidx = idx;
CID 1645489: (#1 of 1): Out-of-bounds read (OVERRUN)
25. overrun-local: Overrunning array dp->df_lnum of 8 8-byte elements at element index 8 (byte offset 71) using index idx (which evaluates to 8).
3606 diffline->lineoff = lnum - dp->df_lnum[idx];
3607
CID 1645487:Out-of-bounds write (OVERRUN) [ "select issue" ]
3608 simple_diffline_change.dc_start[idx] = change_start;
CID 1645490:Out-of-bounds write (OVERRUN) [ "select issue" ]
3609 simple_diffline_change.dc_end[idx] = change_end;
CID 1645488:Out-of-bounds write (OVERRUN) [ "select issue" ]
3610 simple_diffline_change.dc_start_lnum_off[idx] = off;
CID 1645486:Out-of-bounds write (OVERRUN) [ "select issue" ]
3611 simple_diffline_change.dc_end_lnum_off[idx] = off;
3612 return ret;
3613 }Can we validate, that idx does not overrun? Oh, and there seems to be a duplicate semicolon on line 3601.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#16881)This was definitely a typo I missed, but is benign.
At the top of the function we already do this to make sure it's not out of bounds:
int diff_find_change( win_T *wp, linenr_T lnum, diffline_T *diffline) { diff_T *dp; int idx; int off; idx = diff_buf_idx(wp->w_buffer);
if (idx == DB_COUNT) // cannot happen
return FALSE;This code was in the original diff_find_change as well. Then within the simple algorithm if block I somehow did the exact same thing again (with the extra semicolon). It will give us the same result, but we also don't need to do this. I think it was a result of bad copy-paste and I didn't notice it on review. Will submit a PR to fix it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#16881)#16988 will fix this. Let me know if that satisfies Coverity (Is there a public dashboard?)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
zeertzjq
zeertzjq left a comment (vim/vim#16881)Coverity can only run once per day :(
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
rickhowe
rickhowe left a comment (vim/vim#16881)FYI: This might be included in your ToDo list, but word-wise inline diff does not recognize character class as a word boundary in a multibyte string. Actually 'w' and 'b' cursor move command and '<' and '>' regexp recognize the word boundary.
:echo split('差分モード用のオプション設定', '\<\|\>')
['差分', 'モード', '用', 'の', 'オプション', '設定']
In mbyte.c, dbcs_class() and utf_class() are implemented to support them.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#16881)I just tested and you are right. I'll submit a fix.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#16881)FWIW iskeyword's documentation seems a little misleading / confusing. Multi-byte words are completely hard-coded based on Unicode class rules, and completely unaffected by iskeyword settings. The description of @ seems to be a little ambiguous:
'isfname' for a description of the format of this option. For '@' characters above 255 check the "word" character class (any character that is not white space or punctuation).
It does mean if you use word diff right now it's impossible to customize how it groups words for say Japanese or Chinese (which is specifically a problem since all the characters in a whole sentence get lumped into a single "word"). I guess that's why inline:char exists (which IMO is still the better setting than inline:word for most use cases), and in the future we can implement inline:pattern to support custom regex. Right now inline:word should obey the same rule that Vim uses for words.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#16881)I filed #17050 to fix this. The proposed solution is to simply treat all non-alphanumeric multi-byte characters as non-word in the context of word diff. It's slightly inconsistent with how Vim word motions work but I think this works best in this context. This mostly means emojis and CJK characters will treat each character as an individual word.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.