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

Multiline comments are not highlighted correctly #23176

Open
drsybren opened this issue Feb 27, 2023 · 12 comments
Open

Multiline comments are not highlighted correctly #23176

drsybren opened this issue Feb 27, 2023 · 12 comments
Labels

Comments

@drsybren
Copy link
Contributor

Description

Multi-line comments are not highlighted correctly in diff views.

Actual behaviour

Example: commit aa72e3abf9644de3c0478692d7bb357d1c7297e2 in Blender

image

Another example in Go, to show that it's not limited to C/C++: https://try.gitea.io/drSybren/skyfill/commit/aa87c1c50db6dfd97ab53905097e43fce3c5568e

Expected behaviour

Expected would be to see the highlighting like done when viewing the file regularly (of course with red/green background for the added/removed lines in the diff):

image

Gitea Version

1.19.0+dev-649-g2093ca517

Can you reproduce the bug on the Gitea demo site?

Yes

Log Gist

No response

Screenshots

No response

Git Version

No response

Operating System

No response

How are you running Gitea?

Self-built from Git

Database

PostgreSQL

@wxiaoguang
Copy link
Contributor

I guess it's caused by that Gitea renders the diff page line by line, then highlight doesn't work (the comments are processed line by line). That's why the comment highlight works for whole file rendering, but doesn't work for the diff/compare page.

Maybe related to:

@a1012112796
Copy link
Member

a1012112796 commented Feb 28, 2023

looks that's because gitea hasn't checking language by file name and content like file view. but it's sound not a easy job (how to get file content ? single file name always not enough for checking language). I'd like tag this issue as kind/enhancement instead of kind/bug .

ref:

if language, has := attrs["linguist-language"]; has && language != "unspecified" && language != "" {
diffFile.Language = language
} else if language, has := attrs["gitlab-language"]; has && language != "unspecified" && language != "" {
diffFile.Language = language
}

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 28, 2023

Not only a "language" problem, but the highlighter need all the lines together to do highlight correctly. Otherwise when the highlighter sees the second line of the comment without /*, it doesn't know it's in the comment context.

@drsybren
Copy link
Contributor Author

drsybren commented Mar 3, 2023

I'd like tag this issue as kind/enhancement instead of kind/bug .

I would still argue this is a bug, as the highlighting is clearly wrong.

@silverwind
Copy link
Member

silverwind commented Mar 3, 2023

Yes, the main reason is diff highlighting is line-by-line so all multiline stuff breaks. We should instead feed both the "before" and "after" documents separately to chroma and then use those highlighted lines to replace the lines with highlighted ones in the diff.

I'm pretty sure this issue is duplicate.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 4, 2023

This problem is quite challenging. Because at the moment many Diff Lines are provided by git commands without highlight. If we want to highlight the file first then do the diff to get Diff Lines, then the diff code should be (nearly) totally rewritten.

There could be a intermediate solution: git provides the diff lines -> gitea parses the diff lines -> git merge (join) the diff lines (update: for every hunk, but not join all chunks) as a temp file content and highlight it -> then gitea splits the highlighted content into lines as diff lines.

Correct me if I am wrong.

@lunny
Copy link
Member

lunny commented Mar 4, 2023

Could we highlight every section?
Storing the diff as a patch file may be useful for a second time CPU reduction.

@lunny
Copy link
Member

lunny commented Mar 4, 2023

I mean the struct of DiffLineSectionInfo

@silverwind
Copy link
Member

silverwind commented Mar 4, 2023

Could we highlight every section?

If you mean the diff hunks: It may be an improvement, but will not completely fix the issue. A complete fix can only to be to highlight the whole "before" and "after" files and map back the highlighted lines into the diff lines.

Imagine a file that only consists of one big muliline string, spanning hundres of lines. git diff will give a context of +/- 3 lines to a one-line edit. Highlighting a hunk in the middle will still be incorrect.

Also, highlighting diff hunks may result in even more broken cases because added/removed lines are intermingled, so it will definitely confuse chroma.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 4, 2023

Could we highlight every section?

That's my "an intermediate solution" above. However it is only an intermediate solution, you would still lose the correct context because the diff hunk may only contain a few lines, there might be no /* in the hunk.

@silverwind
Copy link
Member

I doubt highlighting hunks as a whole will improve the situation much, I fear it may only make it worse.

@wxiaoguang
Copy link
Contributor

hunks as a whole ... I fear it may only make it worse.

Yup, if the a hunk only contains /* but doesn't contain */, the remaining hunks are all rendered as "comment", not good.

a1012112796 added a commit to a1012112796/gitea that referenced this issue Apr 25, 2023
related go-gitea#23176

try fix it with @silverwind solution in
 go-gitea#23176 (comment)

looks the result is good. but looks it will
cost to much memorry when the file is large.
maybe limt the max file size?

Signed-off-by: a1012112796 <1012112796@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants