Refactor compare router param parse and fix bugs #36105

lunny · 2025-12-08T06:14:40Z

Fix #36116
Fix #35912
Fix #20906

routers/common/compare_test.go

Co-authored-by: techknowlogick <[email protected]> Signed-off-by: Lunny Xiao <[email protected]>

a1012112796 · 2025-12-10T01:49:50Z

please have a look at #36116

…lunny/refactor_compare

lunny · 2025-12-11T03:08:39Z

please have a look at #36116

This has been included and it's ready to review now.

wxiaoguang · 2025-12-12T02:49:47Z

models/user/user.go

+
+func GetUserOrOrgByName(ctx context.Context, name string) (*User, error) {
+	var u User
+	has, err := db.GetEngine(ctx).Where("name = ?", name).Get(&u)


Why not lower_name

There is no existing similar function?

6453e4a There is no existing function that can retrieve a user or an organization.

wxiaoguang · 2025-12-12T02:50:45Z

routers/api/v1/repo/pull.go

+					return nil, nil
+				}
+				if headRepo == nil {
+					ctx.HTTPError(http.StatusBadRequest, "The user "+headOwner.Name+" does not have a fork of the base repository")


Why sometimes APIError sometimes HTTPError?

wxiaoguang · 2025-12-12T02:51:16Z

routers/web/repo/compare.go

+					} else {
+						ctx.ServerError("GetRepositoryByName", err)
+					}
+					return nil


Why duplicate the code with API?

wxiaoguang · 2025-12-12T02:54:11Z

routers/api/v1/repo/pull.go

+		} else {
+			headOwner, err = user_model.GetUserOrOrgByName(ctx, compareReq.HeadOwner)
+			if err != nil {
+				if user_model.IsErrUserNotExist(err) {


GetUserOrOrgByName doesn't return errors for IsErrUserNotExist

wxiaoguang · 2025-12-12T02:54:45Z

Is it safe to backport a refactoring PR with so many logic changes?

lunny · 2025-12-12T03:21:31Z

Is it safe to backport a refactoring PR with so many logic changes?

This will be partially backport. Only bugfixes will be backport.

silverwind · 2025-12-12T23:52:06Z

routers/common/compare.go

+	if traverseLevel == 0 {
+		return nil, nil
+	}
+	// test if we are lucky


describe what "lucky" means?

This comment could be removed. It seems meaningless.

Then I wonder why you added it in first place :)

9868b2f I honestly can’t remember why I did that.

silverwind · 2025-12-12T23:54:17Z

routers/common/compare.go

+func GetHeadOwnerAndRepo(ctx context.Context, baseRepo *repo_model.Repository, compareReq *CompareRouterReq) (headOwner *user_model.User, headRepo *repo_model.Repository, err error) {
+	if compareReq.HeadOwner == "" {
+		if compareReq.HeadRepoName != "" { // unsupported syntax
+			return nil, nil, nil


Can you avoid returning no result and no error at the same time? I thought we had the nilnil linter to detect such scenarios.

Seems nilnil it is currently not enabled and I guess there may be many violations in the code base. We should still strive to enable it because such returns are ambiguous and very unclear to readers.

A function should either be successful or failure, not something in between the two.

FYI I did a test run of nilnil with below settings, there are currently 287 violations in the code base but I feel I'm not qualified to fix them. What we could do is add // nolint to them all and gradually fix them, forbiding any new errors to be introduced.

nilnil: only-two: false detect-opposite: true

Many many times, people don't learn from lessons.

Move git config/remote to gitrepo package and add global lock to resolve possible conflict when updating repository git config file #35151 (comment)

Don't display error log when .git-blame-ignore-revs doesn't exist #34457 (comment)

…lunny/refactor_compare

Refactor compare router param parse

7fe0840

lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Dec 8, 2025

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 8, 2025

github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels Dec 8, 2025

lunny added 3 commits December 7, 2025 22:55

Refactor compare router param parse

3c51f38

Fix test

a2c8e9c

Merge branch 'main' into lunny/refactor_compare

229b20b

techknowlogick reviewed Dec 8, 2025

View reviewed changes

routers/common/compare_test.go Outdated Show resolved Hide resolved

Update routers/common/compare_test.go

44925e3

Co-authored-by: techknowlogick <[email protected]> Signed-off-by: Lunny Xiao <[email protected]>

lunny added 6 commits December 10, 2025 11:07

Fix

8a83a64

Merge branch 'main' into lunny/refactor_compare

eaf8dff

Merge branch 'lunny/refactor_compare' of github.com:lunny/gitea into …

0166880

…lunny/refactor_compare

Fix bug

dcb8e47

Merge branch 'main' into lunny/refactor_compare

e5dbcbd

Fix bug and add test

aebb29c

lunny marked this pull request as ready for review December 10, 2025 23:25

lunny added 2 commits December 10, 2025 15:51

remove unnecessary code

c3c4e3c

Remove unnecessary permission check

220a908

lunny changed the title ~~Refactor compare router param parse~~ Refactor compare router param parse and fix bugs Dec 11, 2025

lunny added the backport/v1.25 label Dec 11, 2025

Prevent to create duplicated pull request

3bd496b

github-actions bot added the modifies/translation label Dec 11, 2025

Merge branch 'main' into lunny/refactor_compare

6ed0c75

lunny added the type/bug label Dec 11, 2025

Zettat123 approved these changes Dec 11, 2025

View reviewed changes

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 Dec 11, 2025

kerwin612 approved these changes Dec 12, 2025

View reviewed changes

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 Dec 12, 2025

wxiaoguang reviewed Dec 12, 2025

View reviewed changes

wxiaoguang marked this pull request as draft December 12, 2025 02:54

wxiaoguang removed the backport/v1.25 label Dec 12, 2025

wxiaoguang requested review from Zettat123 and kerwin612 December 12, 2025 02:54

lunny added 2 commits December 11, 2025 19:17

some improvements

6453e4a

Merge branch 'main' into lunny/refactor_compare

64d6010

GiteaBot removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Dec 12, 2025

remove duplicated code

eb40447

lunny marked this pull request as ready for review December 12, 2025 17:36

Merge branch 'main' into lunny/refactor_compare

56e2b67

lunny mentioned this pull request Dec 12, 2025

Fix bugs when comparing and creating pull request #36144

Open

lunny added this to the 1.26.0 milestone Dec 12, 2025

lunny added backport/done All backports for this PR have been created backport/v1.25 labels Dec 12, 2025

silverwind reviewed Dec 12, 2025

View reviewed changes

lunny added 2 commits December 12, 2025 18:38

Don't return nilnil

9868b2f

Merge branch 'lunny/refactor_compare' of github.com:lunny/gitea into …

7ceeab1

…lunny/refactor_compare

Uh oh!

Refactor compare router param parse and fix bugs #36105

Are you sure you want to change the base?

Refactor compare router param parse and fix bugs #36105

Conversation

lunny commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

a1012112796 commented Dec 10, 2025

Uh oh!

lunny commented Dec 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lunny Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wxiaoguang commented Dec 12, 2025

Uh oh!

lunny commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

lunny commented Dec 8, 2025 •

edited

Loading

lunny Dec 12, 2025 •

edited

Loading

lunny commented Dec 12, 2025 •

edited

Loading

silverwind Dec 12, 2025 •

edited

Loading

silverwind Dec 12, 2025 •

edited

Loading