Skip to content

Full-file syntax highlighting for diff pages#33766

Merged
wxiaoguang merged 31 commits into
go-gitea:mainfrom
dfirebaugh:git-diff-highlights
Mar 9, 2025
Merged

Full-file syntax highlighting for diff pages#33766
wxiaoguang merged 31 commits into
go-gitea:mainfrom
dfirebaugh:git-diff-highlights

Conversation

@dfirebaugh

@dfirebaugh dfirebaugh commented Mar 2, 2025

Copy link
Copy Markdown
Contributor

Fix #33358, fix #21970

This adds a step in the GitDiff that does syntax highlighting for the entire file and then only references lines from that syntax highlighted code. This allows things like multi-line comments to be syntax highlighted correctly.

image (4)
image (3)
image (2)
image (1)
image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 2, 2025
Comment thread services/gitdiff/highlightdiff_test.go Outdated
@wxiaoguang wxiaoguang marked this pull request as draft March 2, 2025 03:13

@wxiaoguang wxiaoguang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTML encoding should be correctly handled, for example: <>

See new comment below #33766 (comment)

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 2, 2025
@wxiaoguang

This comment was marked as outdated.

@pull-request-size pull-request-size Bot added size/M and removed size/L labels Mar 2, 2025
… since we dont actually need to highlight the code here

- adding newline in testcase
@pull-request-size pull-request-size Bot added size/L and removed size/M labels Mar 2, 2025
@wxiaoguang

wxiaoguang commented Mar 2, 2025

Copy link
Copy Markdown
Contributor

Thank you very much. I can understand most of changes now. The idea is feasible, while there are still some details:

  1. Some of the old tests should be kept, for example: diff.Text = "OC", I do not see why it should be removed. It is used to make sure the recoverOneDiff works correctly.
  2. Some of the old tests do not fit the new code:
    • If a test doesn't fit (the case won't happen), feel free to remove it.
    • New tests should cover the new behavior since the behavior of diffWithHighlight has changed, it only accepts HTML content
  3. Some variable types should be changed to match the new behavior, use template.HTML
  4. Some edge cases, for example, what if a file is large (Mega-bytes)? It would bloat the Gitea's memory and make it OOM.
    • I think we need to have a limit, if a file is too large, fall back to the "hunk block" diff but not the "full file" diff.

@silverwind

silverwind commented Mar 2, 2025

Copy link
Copy Markdown
Member

I think we need to have a limit, if a file is too large, fall back to the "hunk block" diff but not the "full file" diff.

Sounds acceptable to have a certain byte limit above which to fall back to hunk-based diff. Maybe 1-5MB or so.

- adding conditions for when full file based highlighting should occur
- falling back to hunk based highlighting in certain conditions
- reverting original hunk-based highlighting tests
- updating types to express intent better and to be more accurate i.e. `template.HTML` instead of string where appropriate
@wxiaoguang wxiaoguang added type/enhancement An improvement of existing functionality type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Mar 8, 2025
@wxiaoguang wxiaoguang changed the title Fix #33358 - Syntax highlighting should be full-file only Full-file syntax highlighting for diff pages Mar 8, 2025
@wxiaoguang wxiaoguang added type/enhancement An improvement of existing functionality and removed type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/enhancement An improvement of existing functionality labels Mar 8, 2025
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 8, 2025
@wxiaoguang wxiaoguang removed their assignment Mar 8, 2025
Comment thread services/gitdiff/gitdiff.go Outdated
@lunny

lunny commented Mar 8, 2025

Copy link
Copy Markdown
Member

When expend more codes, it result in 500

Render failed, failed to render template: repo/diff/blob_excerpt, error: template error: builtin(static):repo/diff/blob_excerpt:83:20 : executing "repo/diff/blob_excerpt" at <$.section.GetComputedInlineDiffFor>: error calling GetComputedInlineDiffFor: runtime error: invalid memory address or nil pointer dereference
----------------------------------------------------------------------
		{{$inlineDiff := $.section.GetComputedInlineDiffFor $line ctx.Locale}}
		                  ^
----------------------------------------------------------------------

@wxiaoguang

Copy link
Copy Markdown
Contributor

When expend more codes, it result in 500

Fixed in b2dbc78 and added more tests

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 9, 2025
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 9, 2025
@wxiaoguang wxiaoguang merged commit 3f1f808 into go-gitea:main Mar 9, 2025
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 9, 2025
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 10, 2025
* giteaofficial/main:
  Move notifywatch to service layer (go-gitea#33825)
  [skip ci] Updated translations via Crowdin
  Only keep popular licenses (go-gitea#33832)
  Removing unwanted ui container (go-gitea#33833)
  Full-file syntax highlighting for diff pages (go-gitea#33766)
  Improve theme display (go-gitea#30671)
  Decouple context from repository related structs (go-gitea#33823)
  Improve log format (go-gitea#33814)
  Decouple diff stats query from actual diffing (go-gitea#33810)
  Add global lock for migrations to make upgrade more safe with multiple replications (go-gitea#33706)
  Do not show passkey on http sites (go-gitea#33820)
hiifong pushed a commit to hiifong/gitea that referenced this pull request Mar 10, 2025
Fix go-gitea#33358, fix go-gitea#21970

This adds a step in the `GitDiffForRender` that does syntax highlighting for the
entire file and then only references lines from that syntax highlighted
code. This allows things like multi-line comments to be syntax
highlighted correctly.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

(cherry picked from commit 3f1f808)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jun 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Syntax highlighting should be full-file only PHP Syntax Highlighting does not working in review mode

7 participants