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

fix highlight problem of git diff #24336

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

a1012112796
Copy link
Member

related #23176

try fix it with @silverwind solution in #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?

屏幕截图 2023-04-25 183456

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>
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 25, 2023
@GiteaBot GiteaBot added this to the 1.20.0 milestone Apr 25, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 25, 2023
@wxiaoguang
Copy link
Contributor

What's algorithm?

The current method supports to highlight deleted/added/changed part, could you show how your algorithm works?

image

@silverwind
Copy link
Member

silverwind commented Apr 25, 2023

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

Hmm yes, full file highlight is expensive. Might need a option for max line count after which to fall back to the cheap method. Kind of similar to git.MAX_GIT_DIFF_LINES but for the whole file, so the value can probably be larger, like start with 2000 or so.

@a1012112796
Copy link
Member Author

What's algorithm?

The current method supports to highlight deleted/added/changed part, could you show how your algorithm works?

image

no change about algorithm.

but render full file instead of render it line by line. ref: #23176 (comment)

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 26, 2023

no change about algorithm.

but render full file instead of render it line by line. ref: #23176 (comment)

I mean , does the old style diff still work? This feature is pretty useful.

image

@a1012112796
Copy link
Member Author

no change about algorithm.
but render full file instead of render it line by line. ref: #23176 (comment)

I mean , does the old style diff still work? This feature is pretty useful.

image

Oh, I see. sorry, I forgot it, will fix it later.

@a1012112796
Copy link
Member Author

@wxiaoguang fixed 4362fa6

image

Signed-off-by: a1012112796 <1012112796@qq.com>
@wxiaoguang
Copy link
Contributor

Haven't looked into details, some quick thoughts after a quick review:

  • why MAX_DIFF_HIGHLIGHT_FILE_SIZE=5k ? models/user/user.go is 37k
  • the name of MAX_DIFF_HIGHLIGHT_FILE_SIZE seems unclear, files exceed this limit also get hightlighted (line by line)
  • the "HighlightContent" could be dropped?
    • if the content is not highlighted, then highlight it , and set HasHighlightContent = true
  • there are too many arguments for diffWithHighlight, the logic doesn't seem easy to understand how these arguments affect each other
  • could there be some tests to cover the new logic?

@a1012112796 a1012112796 removed this from the 1.20.0 milestone Apr 27, 2023
@a1012112796
Copy link
Member Author

I'd like try some refactor to make highlight.File() using io.Reader directly at first. ref:

@a1012112796 a1012112796 marked this pull request as draft May 10, 2023 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants