Skip to content

Full-file syntax highlighting for diff pages #33766

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

Merged
merged 31 commits into from
Mar 9, 2025

Conversation

dfirebaugh
Copy link
Contributor

@dfirebaugh dfirebaugh commented Mar 2, 2025

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
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Mar 2, 2025
@wxiaoguang wxiaoguang marked this pull request as draft March 2, 2025 03:13
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

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
Copy link
Contributor

wxiaoguang commented Mar 2, 2025

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
Copy link
Member

silverwind commented Mar 2, 2025

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
@lunny
Copy link
Member

lunny commented Mar 8, 2025

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
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
26 checks passed
@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)
@wxiaoguang
Copy link
Contributor

Follow up fix: Fix material icon & diff highlight #33844

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 <[email protected]>

(cherry picked from commit 3f1f808)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files 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