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

Decouple diff stats query from actual diffing #33810

Merged
merged 6 commits into from
Mar 8, 2025

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Mar 6, 2025

The diff stats are no longer part of the diff generation.
Use GetDiffShortStat instead to get the total number of changed files, added lines, and deleted lines.
As such, gitdiff.GetDiff can be simplified:
It should not do more than expected.

And do not run "git diff --shortstat" for pull list. Fix #31492

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 6, 2025
@github-actions github-actions bot added 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 labels Mar 6, 2025
@wxiaoguang wxiaoguang force-pushed the refactor-git-diff branch 2 times, most recently from 372b48b to 9974e14 Compare March 6, 2025 05:02
func (diffFile *DiffFile) GetTailSection(gitRepo *git.Repository, leftCommit, rightCommit *git.Commit) *DiffSection {
if len(diffFile.Sections) == 0 || diffFile.Type != DiffFileChange || diffFile.IsBin || diffFile.IsLFSFile {
func (diffFile *DiffFile) GetTailSection(leftCommit, rightCommit *git.Commit) *DiffSection {
if len(diffFile.Sections) == 0 || leftCommit == nil || diffFile.Type != DiffFileChange || diffFile.IsBin || diffFile.IsLFSFile {
Copy link
Member

Choose a reason for hiding this comment

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

Wait, that seems strange:
What does the commit being missing have to do with whether we have to create the expand button?
These two things seem unrelated to me…

Copy link
Member

Choose a reason for hiding this comment

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

And if we do need it, wouldn't we need the check rightCommit == null as well?

Copy link
Contributor Author

@wxiaoguang wxiaoguang Mar 6, 2025

Choose a reason for hiding this comment

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

It's not strange (hmm, maybe strange), actually it doesn't need leftCommit == nil because DiffFileChange already implies leftCommit != nil.

This change just clarifies it.

And, rightCommit can never be nil

@delvh delvh changed the title Refactor git diff Decouple diff stats query from actual diffing Mar 6, 2025
@wxiaoguang
Copy link
Contributor Author

Suggested changes are done in 58d24ab

@wxiaoguang wxiaoguang force-pushed the refactor-git-diff branch 4 times, most recently from 553c108 to c6a07ee Compare March 7, 2025 01:34
@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 7, 2025
@GiteaBot GiteaBot removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 8, 2025
@GiteaBot GiteaBot added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Mar 8, 2025
@wxiaoguang wxiaoguang merged commit 6422f05 into go-gitea:main Mar 8, 2025
26 checks passed
@GiteaBot GiteaBot added this to the 1.24.0 milestone Mar 8, 2025
@wxiaoguang wxiaoguang deleted the refactor-git-diff branch March 8, 2025 09:36
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
The diff stats are no longer part of the diff generation.
Use `GetDiffShortStat` instead to get the total number of changed files,
added lines, and deleted lines.
As such, `gitdiff.GetDiff` can be simplified:
It should not do more than expected.

And do not run "git diff --shortstat" for pull list. Fix go-gitea#31492

(cherry picked from commit 6422f05)
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

repos/pulls api take long time for large repos
5 participants