Improve code diff highlight, fix incorrect rendered diff result

[wxiaoguang]

Try to fix: #19508

• Diff highlighting doesn't always include all changes #19508

Some tests are removed, because these were only for broken diff results. This new method doesn't generate these broken results.

Edge cases are cleared, and added more related tests.

Screenshots

Before:

After:

[silverwind]

I think we can remove the <span class="line"><span class="cl"> wrapping of each diff line like I did in #19967 but for regular file view. They seem pointless and less HTML output will always render faster.

[silverwind]

Seems fine to me to close the syntax class before the added/removed block, I think this also matches GH rendering. Gave it a few more tests, renderings look fine.

[wxiaoguang]

I think we can remove the <span class="line"><span class="cl"> wrapping of each diff line like I did in #19967 but for regular file view. They seem pointless and less HTML output will always render faster.

Yup, I also think removing them is good. Updated (and now the code still support to add them back -- if it's needed)

[wxiaoguang]

Changes in 90c6440

1. split the code into a new go file
2. use Plane 16: Supplementary Private Use Area B (U+100000..U+10FFFD) , 64000
3. handle html entities &...;
4. add comment It totally depends on Chroma's valid HTML output and its structure, do not use these functions for other purposes.
5. more tests.

TODO comment: when placeholders are exhausted, the html entites must be outputted as something (at the moment, 0xFFFD). Since there are 64000 placeholders now, this case could be very rare, although still not ideal. In 4afb1cc , the users will always see the different outputs, even if it's not ideal, better ugly than mistake.

[zeripath]

This isn't right.

The return from diffMatchPatch.DiffCleanupEfficiency is []*Diff.

Each *Diff represents an addition, unchanged or deletion section.

There is no guarantee that these sections have to be balanced individually.

The slice has to be balanced across the addition+unchanged or deletion+unchanged subsets all together.

Therefore you can have:

+ <span class="function">
- <span class="variable">
  amIaFunctionOrAVariable
+ ()
  </span>

Isn't this code going to result in:

+ <span class="function"></span>
- <span class="variable"></span>
  amIaFunctionOrAVariable
+ ()
  </span>

?

[wxiaoguang]

In this case, the highlight is just lost, but the adding/removing tags are still correct. All tags are still balanced, and the last </span> won't be there, it should have been removed. https://github.com/go-gitea/gitea/pull/19958/files#diff-ee527c0c674cb10b21deadb5c08b523a8ff14727234ce3f752685c51d5e46b19R194-R200

I do not think there is a clear and easy way to make highlight still work perfectly in all cases, just better than before.

[wxiaoguang]

See the demo in my last commit 9cf358b

It's guaranteed that tags in these sections are balanced individually.

[wxiaoguang]

@zeripath ping. I think it is right. Do you have a real sample to break the code?

[wxiaoguang]

@zeripath ping

[silverwind]

Seems to work well enough. I think we should land this PR and #19967 to streamline the code/diff renderings. After those are landed, I'd like to upgrade chroma to latest version.

[wxiaoguang]

Since there is no new suggestion since 14 days ago (with 3 pings), I will merge this PR.

I have confidence that this PR is better than before (stable behavior, no broken tags). And I will fix any bug in my code in the first time, as always.

[zeripath]

I suggest once this PR has completed being built we check results of looking at:

https://try.gitea.io/arandomer/pathological/commit/6a27858c792f20ec5789d5487362b74b9ac46411

and the parent commits to see whether anything like the problems I suggested might be occuring.

I am suspicious that there will be multiple issues resulting from this PR.

[wxiaoguang]

Now try.gitea.io is using this PR.

[zeripath]

Aha I've made the pathological case:

https://try.gitea.io/arandomer/pathological/commit/f98651b39614be44b15686165446502d6ce89c92

Look at the fmt.Println(variableNoFunction() + "a") line and the func variableOrFunction() string { line

[wxiaoguang]

It's not easy (or nearly impossible) to make highlight diff work perfectly when doing post-highlight diff (#19958 (comment)), and some highlights may be lost or incorrect in some cases, but it won't affect the readers to see the correct diff result (#19958 (comment)). The new code is just better than before, not perfect (#19958 (comment)).

And IMO your cases are the expected outputs for this PR, and I would say that's still clear to reviewers.

• For the first diff, I think readers can easily see that the ( is moved.
• For the second diff, the same, the readers can easily see that how the function name and arguments are changed.

In any case, the tags are balanced correctly (compared to issue #19508, the old code was totally broken and misled readers)

---

Update: if you meant that the problem is that the diff result for ab() to a(b) is inconsistent, then this is still the problem cased by the result generated by diff module for the highlighted code, then it goes back to the impossible task at the moment -- unless introduce some totally new diff algorithm. Again, this PR could be better than or equivalent to before for all cases, it doesn't generate misleading diff results (eg: #19508), while it doesn't mean to be the perfect solution or solve the diff problem fundamentally.

[silverwind]

FYI, git's diff.algorithm also influences diff highlight. I'm a fan of patience as it generally produces better diffs than the myers default, it's worth testing with:

[diff]
	algorithm = patience

[zeripath]

I'm sorry that both my comments here have been negative - This PR is a great idea using the unused characters to do this and the it does represent a significant improvement.

However the problem I've pointed out is real and it will appear in real life.

There are two solutions to this:

1. The Add/delete section expands to include the semantic change.
   • e.g. in the change fmt.Println(variableNoFunction() + "a") to fmt.Println(variable(NoFunction) + "a")
   • the add delete components are variableNoFunction( and variable(NoFunction
2. The semantic meaning crosses over the diff boundary.
   • Here one makes no attempt to fix up the elements until rendering.
   • When rendering you keep a stack and as you cross an add/delete/unchanged boundary you close all the tags before reopening them on the other side.

We need to choose one of these. I think if I interpret your replies correctly you think 2. is the right thing and it's probably the simplest thing to do. 1. is perhaps clearer to the user is closer to the patience algorithm and what we had before but honestly I don't want to argue.

[wxiaoguang]

Agree that 1 is clearer. Introducing a new "semantic change expanding" algorithm might make the result much better.

It's worth to try (I will try it if I have time later, and PRs are also welcome, let's make the code more right together).

I will open a new issue for this task.

---

Update:

• [Proposal] Future improvments for code hightlight diff #20467