[vim/vim] diff: Update diffopt defaults to use indent-heuristic and inline:char (PR #18255)
Yee Cheng Chin
The default diff options have not been updated much despite new functionality having been added to Vim.
-
indent-heurstic: This has been enabled by default in Git since 33de716 in 2017. Given that Vim uses xdiff from Git, it makes sense to track the default configuration from Git.
-
inline:char: This turns on character-wise inline highlighting which is generally much better than the default inline:simple. It has been implemented since #16881 and we have not seen reports of any issues with it, and it has received good feedbacks.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/18255
Commit Summary
- 703fd59 diff: Update diffopt defaults to use indent-heuristic and inline:char
File Changes
(3 files)
- M runtime/doc/options.txt (2)
- M src/optiondefs.h (2)
- M src/testdir/test_diffmode.vim (18)
Patch Links:
—
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#18255)You need to update the initial value of diff_flags in diff.c too.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#18255)This proposes two separate new defaults for the diff options. I'm happy to separate them out if we only want one and not the other.
Some additional thoughts below.
indent-heuristic:
- Just for more background info (in case people are interested): I think indent-heuristic was added in Git in git/git@433860f in 2016, which comes with benchmarks and justifications for the change. Then as mentioned above it was added as a default behavior in Git in 2017. It adds a slight performance cost as Git needs to slide up and down to find the best alignment but from experience Git/Xdiff is still consistently one of the fastest diff engine out there compared to a lot of other diff tools (with the default indent heuristic turned on).
inline:char:
- This change will make diff slightly slower because the inline diff is now doing more work. In reality I don't think it matters much because we only calculate inline diff for the blocks on screen and will really be a noticeable regression if you have way a really long line. Either way in the worst case scenario the entire file is one single line so you are really just processing the same file twice, so there's an upper bound to how bad it could be in the worst case possible.
- It does make interactive editing a bit slower when you have diff mode on. I personally don't see this as a very common use case and from personal testing (since I have been using this exclusively as my main diff tool since the change was merged) it's mostly unnoticeable unless you have a really long chunk, but it may depend on the user. The user can always turn off inline highlight when they want to edit, of course.
- The main reason for turning this on as default is that I think this feature is a pretty big improvement over
inline:simpleand is much more of how a user expects a standard diff tool to work. We keep the old method around for backwards compatibility but I think it makes sense to use the newer method by default. - I'm proposing using
inline:charoverinline:wordbecause it's the more flexible option. With word-base diff'ing it may lead to over-highlighting if words are not properly set up or you are editing a long string where the substring difference matters. Char-based diff is more of an almost strictly better version ofinline:simple.
—
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.
- dcc8b57 Make sure to set initial diff flags/algorithm to new defaults
—
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#18255)You need to update the initial value of
diff_flagsin diff.c too.
Done. Forgot to do that.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#18255)Thanks, makes sense. Let me await updated CI runs.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#18255)It looks like some screendump tests need to be regenerated. However the failure in test_diff_with_cursorline looks a bit stringe. It doesn't seem to use the DiffAdd highlighting for the last line and highlights the final bar different than expected?
Also is it just me or is the cursorline highlighting barely noticing?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#18255)I just fixed the tests to use old defaults, by setting them to diffopt=internal,filler, since that's what the tests were originally written against. I already did this for some other tests but had to fix 3 more tests after I fixed the issue where I forgot to set the initial diff_flags/diff_algorithm.
The cursorline is not visible in the above screenshot because inline:char does multi-line comparison, whereas inline:simple considers "baz" and "bar" to be completely different here since they are on different lines in the same diff block.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#18255)I just fixed the tests to use old defaults, by setting them to diffopt=internal,filler, since that's what the tests were originally written against. I already did this for some other tests but have to fix 3 more tests here after I fixed the issue where I forgot to update the initial diff_flags/diff_algorithm. I think this is better than regenerating the terminal dumps because it preserves the original intent of each test case.
Yes thanks makes senes. But wouldn't it also make sense to have those tests run under the new defaults? I am afraid we are now testing something that is no longer in so much use once we have changed the default. And I think we should make sure similar tests are run with the new defaults. Or do you think the existing tests that you provided in v9.1.1243 covers this?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#18255)The way I look at it is that every feature should be tested and works, regardless of whether it's the default. Because the old tests were written against the old defaults (no indent-heuristic, inline:simple), there may be some hidden code coverage that's depending on triggering certain behaviors in those test cases that I don't really want to have to scrub through the code to find out. My opinion is if we think there are gaps in testing for indent-heuristics or inline:char, we should make sure those are tested and add new tests for those. I do believe I added enough regression tests for inline:char in the PR that added it that there shouldn't be any noticeable gaps in behavior though, which is why I don't propose adding more tests here. My goal here is to preserve the existing code coverage that should in theory already be testing with / without these features.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#18255)Ok, despite what I wrote above, I made some changes so the new defaults will now actually be checked in tests.
This required re-generating a couple screen dumps for Test_diff_cursorline and Test_diff_screen so they will enforce that the new defaults are actually in place and works correctly.
—
Reply to this email directly, view it on GitHub.
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.
Christian Brabandt
chrisbra left a comment (vim/vim#18255)Alright thanks. I had to re-generate the screendump Test_smooth_diff_change_line_3.dump and update test_scroll_opt.vim. So I included both versions of the test with old and new diffopt value.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#18255)Ok thanks. I must have missed that.
Feel free to ping me if there are issues with the new defaults. I think inline:char is better but it does work a little differently so there could be questions about it.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Shane-XB-Qian
Shane-XB-Qian left a comment (vim/vim#18255)not sure indent-heuristic impact for now,
but inline:char was really a good idea to be added as default, or ok to change this default?
if no inline:char, the diffAdd was displayed as above, which seems more make sense to me.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#18255)I think it's a matter of taste, but inline:char uses the whole block for comparison and will therefore compare texts from different lines within the same diff chunk for comparison. This allows for diff highlighting like this:
In the original inline:simple setting it would just look like the following as it doesn't consider multiple lines and just arbitrarily consider any additional lines are "TextAdd":
In the example you are using, the texts in the other lines are being compared with each other, hence they are considered to be "changed" rather than "added". The new system doesn't use number of lines to decide what is "added" and what is "changed" within each block and therefore just because the lines are "extra" on one side doesn't automatically mark them as "TextAdd".
inline:char does try to notice the most obvious case when some text are appended at the end within a single diff chunk, and mark them as "TextAdd", but it's not going to work for every single case. An example of a modified version of your example:
It is possible to add some more heuristics to decide when to break up a diff chunk to mark the trailing changes as "added" rather than "changed". It's something that I have thought about and we could add that. It's not going to be perfect though because there is no right answer as to the "correct" result.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Shane-XB-Qian
Shane-XB-Qian left a comment (vim/vim#18255)but since the situation is like this,
as usual (like many prev suggestion i gave to others cases, just some suggestion, but no expect would be followed), i may suggest:
not adding `inline:char` to default value of `diffopt` (vs indent-heuristics maybe ok), till you has confidence no surprise to users.
--
shane.xb.qian
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yee Cheng Chin
ychin left a comment (vim/vim#18255)My opinion is that diff results are never going to be completely backwards compatible in terms of creating identical behaviors, e.g. when Vim updated the diff engine to xdiff, or when maybe Git changes how the algorithm works (that said Git is usually quite conservative in doing so). The other main reason I proposed the change for using inline:char is that the old highlight mode (inline:simple) was/is a frequently complained part of Vim's diff feature, meaning that the stickiness of the reliance of the old feature may be less.
That said I can see how some may see this as a surprise, since inline:char inherently works over multiple lines rather than individual lines. Due to how this feature works it is impossible for this change to lead to "no surprises" because the point is that it leads to better diff highlight results. Changing defaults in Vim does seem to be one of those touchy topics that immediately draws out a lot of discussion, and it depends on what philosophy we want to adopt on new features and default behavior. I will leave it to @chrisbra to decide if we want to keep it or not.
As for indent-heuristic, I highly doubt it will be an issue. As I mentioned it has been the default in Git shortly after it was implemented years ago. Most people never noticed, other than the diff results being a little better aligned.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.