Skip to content

Move repository sub menu data loading to the shared file and only display license on repository home page #32686

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

Closed
wants to merge 7 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Dec 1, 2024

#24872 introduce display license in code, branch, commits and tags page. But it's unnecessary to load licenses data for branches or commit list pages.

This PR will keep license display only for repository home page. It also extracts licenses loading from RepoAssignment middleware to a shared/license.go file.

This will avoid loading commits/branches/tags/license number or information in issues/pulls/releases and other unrelated pages. It at least reduced 3 database queries for every page of these features.

@lunny lunny added type/refactoring Existing code has been cleaned up. There should be no new functionality. performance/speed performance issues with slow downs labels Dec 1, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 1, 2024
@lunny lunny added skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. and removed size/M labels Dec 1, 2024
@lunny lunny added this to the 1.23.0 milestone Dec 1, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Dec 1, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 1, 2024

Think about another case: "main" branch's license is MIT and "feature/other" branch/tag's license is Apache2, then the license stored in database is always MIT, if you show the license on "feature/other" branch/tag/release page, then it confuses users.


Overall the optimization should be like this: only show License on the repo's home page. Then the code could also be simplified, no need to introduce any more tricks.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 1, 2024
@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Dec 1, 2024
@lunny
Copy link
Member Author

lunny commented Dec 1, 2024

Think about another case: "main" branch's license is MIT and "feature/other" branch/tag's license is Apache2, then the license stored in database is always MIT, if you show the license on "feature/other" branch/tag/release page, then it confuses users.

Overall the optimization should be like this: only show License on the repo's home page. Then the code could also be simplified, no need to introduce any more tricks.

I also find the LICENSE may be different between different branches. I will not change the previous design in this PR. 9ec4bdd Please review again, removed the permission check.

@wxiaoguang
Copy link
Contributor

I also find the LICENSE may be different between different branches. I will not change the previous design in this PR. 9ec4bdd Please review again, removed the permission check.

I do not understand why "you will not change the previous License design in this PR" but you changed the template and might break the design of "Add tags list for repos whose release setting is disabled (#23465)"

There are indeed simple approaches, why make it so complex?

@lunny lunny force-pushed the lunny/move_license_sub_menu branch from 984c0ae to 51f4ad8 Compare December 1, 2024 05:51
@pull-request-size pull-request-size bot added size/M and removed size/L labels Dec 1, 2024
@github-actions github-actions bot removed the modifies/templates This PR modifies the template files label Dec 1, 2024
@lunny lunny changed the title Move licenses renders to shared file to avoid unnecesary loading Only display license on repository home page and move licenses data loading to shared file Dec 1, 2024
return false
}
ctx.Data["DetectedRepoLicenses"] = repoLicenses.StringList()
ctx.Data["LicenseFileName"] = repo_service.LicenseFileName
Copy link
Member

Choose a reason for hiding this comment

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

I know, this code was simply moved.
But… why are we storing request-specific data inside a global variable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot answer the question. I guess it may allow a different license file name in the future. Maybe @yp05327 can answer.

Copy link
Contributor

Choose a reason for hiding this comment

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

LicenseFileName in context data is used for the link.
image
At first, I reused the code of detecting README file, so the license file name is not a specific value, but according to the review of #24872 (comment) and #24872 (comment), I changed it into a specific value.
But I think is not enough, e.g. LICENSES or license is not supported, and if it is necessary, we can improve it in the future.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 1, 2024
@lunny lunny changed the title Only display license on repository home page and move licenses data loading to shared file move repository sub menu data loading to the shared file and only display license on repository home page Dec 1, 2024
@lunny lunny changed the title move repository sub menu data loading to the shared file and only display license on repository home page Move repository sub menu data loading to the shared file and only display license on repository home page Dec 1, 2024
@yp05327
Copy link
Contributor

yp05327 commented Dec 2, 2024

only show License on the repo's home page

This is the second step in my plan, it will be/maybe done in #32213

@yp05327
Copy link
Contributor

yp05327 commented Dec 2, 2024

Can this PR fix 500 error in https://gitea.com/gitea/docs/tags?
image
I can not access the logs, but it looks like it is caused by license. (sorry, my bad)

I got the error in my dev environment.
image

@yp05327
Copy link
Contributor

yp05327 commented Dec 3, 2024

It is safe to do some changes of preparing license related data, as it is newly added, no other places start using them, but why you touch others like branch/tag/commit?
I can image your reason, but is it really safe? I don't think so.
By my experience, the global variables of branch/tag/commit are deeply used in many places, simply remove them will cause bugs.
So I tried to find an example, then I got this:
image
How to reproduce? See:
image

Advice:
Only do some changes for license data (only preparing data in home page instead of in repoAssignment), it is a good change, and will fix the bug mentioned above.
You can finish it in this PR or do it directly in #32213.

@lunny lunny modified the milestones: 1.23.0, 1.24.0 Dec 4, 2024
@yp05327
Copy link
Contributor

yp05327 commented Dec 6, 2024

Most necessary changes have been implemented in #32213

@lunny lunny closed this Dec 6, 2024
@lunny lunny deleted the lunny/move_license_sub_menu branch December 6, 2024 20:17
@lunny lunny removed this from the 1.24.0 milestone Dec 6, 2024
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Mar 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/go Pull requests that update Go code performance/speed performance issues with slow downs skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants