From 074035168ea8b66dda7bbd303aeff8a5eff501f1 Mon Sep 17 00:00:00 2001 From: Bo-Yi Wu Date: Tue, 9 Apr 2024 14:34:34 +0800 Subject: [PATCH 01/11] feat: implement commit comparison API endpoint - Add new `Compare` struct to represent comparison between two commits - Introduce new API endpoint `/compare/*` to get commit comparison information - Create new file `repo_compare.go` with the `Compare` struct definition - Add new file `compare.go` in `routers/api/v1/repo` to handle comparison logic - Add new file `compare.go` in `routers/common` to define `CompareInfo` struct - Refactor `ParseCompareInfo` function to use `common.CompareInfo` struct - Update Swagger documentation to include the new API endpoint for commit comparison - Remove duplicate `CompareInfo` struct from `routers/web/repo/compare.go` - Adjust base path in Swagger template to be relative (`/api/v1`) Signed-off-by: Bo-Yi Wu --- modules/structs/repo_compare.go | 10 + routers/api/v1/api.go | 2 + routers/api/v1/repo/compare.go | 377 ++++++++++++++++++++++++++++++++ routers/common/compare.go | 21 ++ routers/web/repo/compare.go | 18 +- templates/swagger/v1_json.tmpl | 49 ++++- 6 files changed, 461 insertions(+), 16 deletions(-) create mode 100644 modules/structs/repo_compare.go create mode 100644 routers/api/v1/repo/compare.go create mode 100644 routers/common/compare.go diff --git a/modules/structs/repo_compare.go b/modules/structs/repo_compare.go new file mode 100644 index 0000000000000..8a12498705112 --- /dev/null +++ b/modules/structs/repo_compare.go @@ -0,0 +1,10 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package structs + +// Compare represents a comparison between two commits. +type Compare struct { + TotalCommits int `json:"total_commits"` // Total number of commits in the comparison. + Commits []*Commit `json:"commits"` // List of commits in the comparison. +} diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index e870378c4b247..b5ec07fc1d95f 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -1066,6 +1066,8 @@ func Routes() *web.Route { m.Post("/migrate", reqToken(), bind(api.MigrateRepoOptions{}), repo.Migrate) m.Group("/{username}/{reponame}", func() { + m.Get("/compare/*", repo.CompareDiff) + m.Combo("").Get(reqAnyRepoReader(), repo.Get). Delete(reqToken(), reqOwner(), repo.Delete). Patch(reqToken(), reqAdmin(), bind(api.EditRepoOption{}), repo.Edit) diff --git a/routers/api/v1/repo/compare.go b/routers/api/v1/repo/compare.go new file mode 100644 index 0000000000000..8cf6bf47d7bb1 --- /dev/null +++ b/routers/api/v1/repo/compare.go @@ -0,0 +1,377 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package repo + +import ( + "net/http" + "strings" + + access_model "code.gitea.io/gitea/models/perm/access" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" + "code.gitea.io/gitea/modules/log" + api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" + "code.gitea.io/gitea/routers/common" + "code.gitea.io/gitea/services/context" + "code.gitea.io/gitea/services/convert" +) + +// ParseCompareInfo parse compare info between two commit for preparing comparing references +func ParseCompareInfo(ctx *context.APIContext) *common.CompareInfo { + baseRepo := ctx.Repo.Repository + ci := &common.CompareInfo{} + + fileOnly := ctx.FormBool("file-only") + + // Get compared branches information + // A full compare url is of the form: + // + // 1. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headBranch} + // 2. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}:{:headBranch} + // 3. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}/{:headRepoName}:{:headBranch} + // 4. /{:baseOwner}/{:baseRepoName}/compare/{:headBranch} + // 5. /{:baseOwner}/{:baseRepoName}/compare/{:headOwner}:{:headBranch} + // 6. /{:baseOwner}/{:baseRepoName}/compare/{:headOwner}/{:headRepoName}:{:headBranch} + // + // Here we obtain the infoPath "{:baseBranch}...[{:headOwner}/{:headRepoName}:]{:headBranch}" as ctx.Params("*") + // with the :baseRepo in ctx.Repo. + // + // Note: Generally :headRepoName is not provided here - we are only passed :headOwner. + // + // How do we determine the :headRepo? + // + // 1. If :headOwner is not set then the :headRepo = :baseRepo + // 2. If :headOwner is set - then look for the fork of :baseRepo owned by :headOwner + // 3. But... :baseRepo could be a fork of :headOwner's repo - so check that + // 4. Now, :baseRepo and :headRepos could be forks of the same repo - so check that + // + // format: ...[:] + // base<-head: master...head:feature + // same repo: master...feature + + var ( + isSameRepo bool + infoPath string + err error + ) + + infoPath = ctx.Params("*") + var infos []string + if infoPath == "" { + infos = []string{baseRepo.DefaultBranch, baseRepo.DefaultBranch} + } else { + infos = strings.SplitN(infoPath, "...", 2) + if len(infos) != 2 { + if infos = strings.SplitN(infoPath, "..", 2); len(infos) == 2 { + ci.DirectComparison = true + } else { + infos = []string{baseRepo.DefaultBranch, infoPath} + } + } + } + + ci.BaseBranch = infos[0] + + // If there is no head repository, it means compare between same repository. + headInfos := strings.Split(infos[1], ":") + if len(headInfos) == 1 { + isSameRepo = true + ci.HeadUser = ctx.Repo.Owner + ci.HeadBranch = headInfos[0] + } else if len(headInfos) == 2 { + headInfosSplit := strings.Split(headInfos[0], "/") + if len(headInfosSplit) == 1 { + ci.HeadUser, err = user_model.GetUserByName(ctx, headInfos[0]) + if err != nil { + if user_model.IsErrUserNotExist(err) { + ctx.NotFound("GetUserByName", nil) + } else { + ctx.ServerError("GetUserByName", err) + } + return nil + } + ci.HeadBranch = headInfos[1] + isSameRepo = ci.HeadUser.ID == ctx.Repo.Owner.ID + if isSameRepo { + ci.HeadRepo = baseRepo + } + } else { + ci.HeadRepo, err = repo_model.GetRepositoryByOwnerAndName(ctx, headInfosSplit[0], headInfosSplit[1]) + if err != nil { + if repo_model.IsErrRepoNotExist(err) { + ctx.NotFound("GetRepositoryByOwnerAndName", nil) + } else { + ctx.ServerError("GetRepositoryByOwnerAndName", err) + } + return nil + } + if err := ci.HeadRepo.LoadOwner(ctx); err != nil { + if user_model.IsErrUserNotExist(err) { + ctx.NotFound("GetUserByName", nil) + } else { + ctx.ServerError("GetUserByName", err) + } + return nil + } + ci.HeadBranch = headInfos[1] + ci.HeadUser = ci.HeadRepo.Owner + isSameRepo = ci.HeadRepo.ID == ctx.Repo.Repository.ID + } + } else { + ctx.NotFound("CompareAndPullRequest", nil) + return nil + } + ctx.Repo.PullRequest.SameRepo = isSameRepo + + // Check if base branch is valid. + baseIsCommit := ctx.Repo.GitRepo.IsCommitExist(ci.BaseBranch) + baseIsBranch := ctx.Repo.GitRepo.IsBranchExist(ci.BaseBranch) + baseIsTag := ctx.Repo.GitRepo.IsTagExist(ci.BaseBranch) + + if !baseIsCommit && !baseIsBranch && !baseIsTag { + // Check if baseBranch is short sha commit hash + if baseCommit, _ := ctx.Repo.GitRepo.GetCommit(ci.BaseBranch); baseCommit != nil { + ci.BaseBranch = baseCommit.ID.String() + } else if ci.BaseBranch == ctx.Repo.GetObjectFormat().EmptyObjectID().String() { + if isSameRepo { + ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(ci.HeadBranch)) + } else { + ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(ci.HeadRepo.FullName()) + ":" + util.PathEscapeSegments(ci.HeadBranch)) + } + return nil + } else { + ctx.NotFound("IsRefExist", nil) + return nil + } + } + + // Now we have the repository that represents the base + + // The current base and head repositories and branches may not + // actually be the intended branches that the user wants to + // create a pull-request from - but also determining the head + // repo is difficult. + + // We will want therefore to offer a few repositories to set as + // our base and head + + // 1. First if the baseRepo is a fork get the "RootRepo" it was + // forked from + var rootRepo *repo_model.Repository + if baseRepo.IsFork { + err = baseRepo.GetBaseRepo(ctx) + if err != nil { + if !repo_model.IsErrRepoNotExist(err) { + ctx.ServerError("Unable to find root repo", err) + return nil + } + } else { + rootRepo = baseRepo.BaseRepo + } + } + + // 2. Now if the current user is not the owner of the baseRepo, + // check if they have a fork of the base repo and offer that as + // "OwnForkRepo" + var ownForkRepo *repo_model.Repository + if ctx.Doer != nil && baseRepo.OwnerID != ctx.Doer.ID { + repo := repo_model.GetForkedRepo(ctx, ctx.Doer.ID, baseRepo.ID) + if repo != nil { + ownForkRepo = repo + } + } + + has := ci.HeadRepo != nil + // 3. If the base is a forked from "RootRepo" and the owner of + // the "RootRepo" is the :headUser - set headRepo to that + if !has && rootRepo != nil && rootRepo.OwnerID == ci.HeadUser.ID { + ci.HeadRepo = rootRepo + has = true + } + + // 4. If the ctx.Doer has their own fork of the baseRepo and the headUser is the ctx.Doer + // set the headRepo to the ownFork + if !has && ownForkRepo != nil && ownForkRepo.OwnerID == ci.HeadUser.ID { + ci.HeadRepo = ownForkRepo + has = true + } + + // 5. If the headOwner has a fork of the baseRepo - use that + if !has { + ci.HeadRepo = repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, baseRepo.ID) + has = ci.HeadRepo != nil + } + + // 6. If the baseRepo is a fork and the headUser has a fork of that use that + if !has && baseRepo.IsFork { + ci.HeadRepo = repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, baseRepo.ForkID) + has = ci.HeadRepo != nil + } + + // 7. Finally open the git repo + if isSameRepo { + ci.HeadRepo = ctx.Repo.Repository + ci.HeadGitRepo = ctx.Repo.GitRepo + } else if has { + ci.HeadGitRepo, err = gitrepo.OpenRepository(ctx, ci.HeadRepo) + if err != nil { + ctx.ServerError("OpenRepository", err) + return nil + } + defer ci.HeadGitRepo.Close() + } else { + ctx.NotFound("ParseCompareInfo", nil) + return nil + } + + // Now we need to assert that the ctx.Doer has permission to read + // the baseRepo's code and pulls + // (NOT headRepo's) + permBase, err := access_model.GetUserRepoPermission(ctx, baseRepo, ctx.Doer) + if err != nil { + ctx.ServerError("GetUserRepoPermission", err) + return nil + } + if !permBase.CanRead(unit.TypeCode) { + if log.IsTrace() { + log.Trace("Permission Denied: User: %-v cannot read code in Repo: %-v\nUser in baseRepo has Permissions: %-+v", + ctx.Doer, + baseRepo, + permBase) + } + ctx.NotFound("ParseCompareInfo", nil) + return nil + } + + // If we're not merging from the same repo: + if !isSameRepo { + // Assert ctx.Doer has permission to read headRepo's codes + permHead, err := access_model.GetUserRepoPermission(ctx, ci.HeadRepo, ctx.Doer) + if err != nil { + ctx.ServerError("GetUserRepoPermission", err) + return nil + } + if !permHead.CanRead(unit.TypeCode) { + if log.IsTrace() { + log.Trace("Permission Denied: User: %-v cannot read code in Repo: %-v\nUser in headRepo has Permissions: %-+v", + ctx.Doer, + ci.HeadRepo, + permHead) + } + ctx.NotFound("ParseCompareInfo", nil) + return nil + } + } + + // Check if head branch is valid. + headIsCommit := ci.HeadGitRepo.IsCommitExist(ci.HeadBranch) + headIsBranch := ci.HeadGitRepo.IsBranchExist(ci.HeadBranch) + headIsTag := ci.HeadGitRepo.IsTagExist(ci.HeadBranch) + if !headIsCommit && !headIsBranch && !headIsTag { + // Check if headBranch is short sha commit hash + if headCommit, _ := ci.HeadGitRepo.GetCommit(ci.HeadBranch); headCommit != nil { + ci.HeadBranch = headCommit.ID.String() + ctx.Data["HeadBranch"] = ci.HeadBranch + } else { + ctx.NotFound("IsRefExist", nil) + return nil + } + } + + baseBranchRef := ci.BaseBranch + if baseIsBranch { + baseBranchRef = git.BranchPrefix + ci.BaseBranch + } else if baseIsTag { + baseBranchRef = git.TagPrefix + ci.BaseBranch + } + headBranchRef := ci.HeadBranch + if headIsBranch { + headBranchRef = git.BranchPrefix + ci.HeadBranch + } else if headIsTag { + headBranchRef = git.TagPrefix + ci.HeadBranch + } + + ci.CompareInfo, err = ci.HeadGitRepo.GetCompareInfo(baseRepo.RepoPath(), baseBranchRef, headBranchRef, ci.DirectComparison, fileOnly) + if err != nil { + ctx.ServerError("GetCompareInfo", err) + return nil + } + + return ci +} + +// CompareDiff compare two branches or commits +func CompareDiff(ctx *context.APIContext) { + // swagger:operation GET /repos/{owner}/{repo}/compare/{basehead} Get commit comparison information + // --- + // summary: Get commit comparison information + // produces: + // - application/json + // parameters: + // - name: owner + // in: path + // description: owner of the repo + // type: string + // required: true + // - name: repo + // in: path + // description: name of the repo + // type: string + // required: true + // - name: basehead + // in: path + // description: compare two branches or commits + // type: string + // required: true + // responses: + // "200": + // "$ref": "#/responses/Compare" + // "404": + // "$ref": "#/responses/notFound" + + if ctx.Repo.GitRepo == nil { + gitRepo, err := gitrepo.OpenRepository(ctx, ctx.Repo.Repository) + if err != nil { + ctx.Error(http.StatusInternalServerError, "OpenRepository", err) + return + } + ctx.Repo.GitRepo = gitRepo + defer gitRepo.Close() + } + + ci := ParseCompareInfo(ctx) + defer func() { + if ci != nil && ci.HeadGitRepo != nil { + ci.HeadGitRepo.Close() + } + }() + if ctx.Written() { + return + } + + apiCommits := make([]*api.Commit, 0, len(ci.CompareInfo.Commits)) + userCache := make(map[string]*user_model.User) + for i := 0; i < len(ci.CompareInfo.Commits); i++ { + apiCommit, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, ci.CompareInfo.Commits[i], userCache, + convert.ToCommitOptions{ + Stat: true, + Verification: ctx.FormBool("verification"), + Files: ctx.FormBool("files"), + }) + if err != nil { + ctx.ServerError("toCommit", err) + return + } + apiCommits = append(apiCommits, apiCommit) + } + + ctx.JSON(http.StatusOK, &api.Compare{ + TotalCommits: len(ci.CompareInfo.Commits), + Commits: apiCommits, + }) +} diff --git a/routers/common/compare.go b/routers/common/compare.go new file mode 100644 index 0000000000000..4d1cc2f0d8908 --- /dev/null +++ b/routers/common/compare.go @@ -0,0 +1,21 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package common + +import ( + repo_model "code.gitea.io/gitea/models/repo" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" +) + +// CompareInfo represents the collected results from ParseCompareInfo +type CompareInfo struct { + HeadUser *user_model.User + HeadRepo *repo_model.Repository + HeadGitRepo *git.Repository + CompareInfo *git.CompareInfo + BaseBranch string + HeadBranch string + DirectComparison bool +} diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index cfb0e859bd7c8..035a92f22830c 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -35,6 +35,7 @@ import ( api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/typesniffer" "code.gitea.io/gitea/modules/util" + "code.gitea.io/gitea/routers/common" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/context/upload" "code.gitea.io/gitea/services/gitdiff" @@ -185,21 +186,10 @@ func setCsvCompareContext(ctx *context.Context) { } } -// CompareInfo represents the collected results from ParseCompareInfo -type CompareInfo struct { - HeadUser *user_model.User - HeadRepo *repo_model.Repository - HeadGitRepo *git.Repository - CompareInfo *git.CompareInfo - BaseBranch string - HeadBranch string - DirectComparison bool -} - // ParseCompareInfo parse compare info between two commit for preparing comparing references -func ParseCompareInfo(ctx *context.Context) *CompareInfo { +func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { baseRepo := ctx.Repo.Repository - ci := &CompareInfo{} + ci := &common.CompareInfo{} fileOnly := ctx.FormBool("file-only") @@ -576,7 +566,7 @@ func ParseCompareInfo(ctx *context.Context) *CompareInfo { // PrepareCompareDiff renders compare diff page func PrepareCompareDiff( ctx *context.Context, - ci *CompareInfo, + ci *common.CompareInfo, whitespaceBehavior git.TrustedCmdArgs, ) bool { var ( diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index b5677c77e03c7..173a14221c5a1 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -21,7 +21,7 @@ }, "version": "{{AppVer | JSEscape}}" }, - "basePath": "{{AppSubUrl | JSEscape}}/api/v1", + "basePath": "/api/v1", "paths": { "/activitypub/user-id/{user-id}": { "get": { @@ -5340,6 +5340,51 @@ } } }, + "/repos/{owner}/{repo}/compare/{basehead}": { + "get": { + "produces": [ + "application/json" + ], + "tags": [ + "Get", + "commit", + "comparison" + ], + "summary": "Get commit comparison information", + "operationId": "information", + "parameters": [ + { + "type": "string", + "description": "owner of the repo", + "name": "owner", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "name of the repo", + "name": "repo", + "in": "path", + "required": true + }, + { + "type": "string", + "description": "compare two branches or commits", + "name": "basehead", + "in": "path", + "required": true + } + ], + "responses": { + "200": { + "$ref": "#/responses/Compare" + }, + "404": { + "$ref": "#/responses/notFound" + } + } + } + }, "/repos/{owner}/{repo}/contents": { "get": { "produces": [ @@ -25557,4 +25602,4 @@ "TOTPHeader": [] } ] -} +} \ No newline at end of file From b39e615a4cccf7dcd39132c61ff3ebb526c82992 Mon Sep 17 00:00:00 2001 From: Bo-Yi Wu Date: Tue, 9 Apr 2024 15:28:05 +0800 Subject: [PATCH 02/11] feat: refactor API comparison structures - Add a new `swaggerCompare` struct for API response in `repo.go` - Update the `basePath` in `v1_json.tmpl` to include `AppSubUrl` - Define a new `Compare` object in Swagger JSON template with properties `commits` and `total_commits` - Add a reference to the `Compare` definition in Swagger JSON template Signed-off-by: Bo-Yi Wu --- routers/api/v1/swagger/repo.go | 6 ++++++ templates/swagger/v1_json.tmpl | 27 ++++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/routers/api/v1/swagger/repo.go b/routers/api/v1/swagger/repo.go index 3e23aa4d5a5ac..c3219f28d6a86 100644 --- a/routers/api/v1/swagger/repo.go +++ b/routers/api/v1/swagger/repo.go @@ -414,3 +414,9 @@ type swaggerRepoNewIssuePinsAllowed struct { // in:body Body api.NewIssuePinsAllowed `json:"body"` } + +// swagger:response Compare +type swaggerCompare struct { + // in:body + Body api.Compare `json:"body"` +} diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 173a14221c5a1..b398b3446812e 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -21,7 +21,7 @@ }, "version": "{{AppVer | JSEscape}}" }, - "basePath": "/api/v1", + "basePath": "{{AppSubUrl | JSEscape}}/api/v1", "paths": { "/activitypub/user-id/{user-id}": { "get": { @@ -18762,6 +18762,25 @@ }, "x-go-package": "code.gitea.io/gitea/modules/structs" }, + "Compare": { + "type": "object", + "title": "Compare represents a comparison between two commits.", + "properties": { + "commits": { + "type": "array", + "items": { + "$ref": "#/definitions/Commit" + }, + "x-go-name": "Commits" + }, + "total_commits": { + "type": "integer", + "format": "int64", + "x-go-name": "TotalCommits" + } + }, + "x-go-package": "code.gitea.io/gitea/modules/structs" + }, "ContentsResponse": { "description": "ContentsResponse contains information about a repo's entry's (dir, file, symlink, submodule) metadata and content", "type": "object", @@ -24723,6 +24742,12 @@ } } }, + "Compare": { + "description": "", + "schema": { + "$ref": "#/definitions/Compare" + } + }, "ContentsListResponse": { "description": "ContentsListResponse", "schema": { From 6b8c322fa06fe588e78928e2c58613b1d8461e78 Mon Sep 17 00:00:00 2001 From: appleboy Date: Tue, 9 Apr 2024 21:36:11 +0800 Subject: [PATCH 03/11] docs: update Signed-off-by: appleboy --- templates/swagger/v1_json.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index b398b3446812e..254b7daf174dc 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -25627,4 +25627,4 @@ "TOTPHeader": [] } ] -} \ No newline at end of file +} From 1719ef55baa7e73eb7d3c48894aa026a3c906fed Mon Sep 17 00:00:00 2001 From: appleboy Date: Wed, 10 Apr 2024 21:24:51 +0800 Subject: [PATCH 04/11] test: add unit testing. --- tests/integration/api_repo_compare_test.go | 40 ++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 tests/integration/api_repo_compare_test.go diff --git a/tests/integration/api_repo_compare_test.go b/tests/integration/api_repo_compare_test.go new file mode 100644 index 0000000000000..e9abb42d8612d --- /dev/null +++ b/tests/integration/api_repo_compare_test.go @@ -0,0 +1,40 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "net/http" + "testing" + + auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/setting" + api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/tests" + "github.com/stretchr/testify/assert" +) + +func TestAPICompareTag(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + // Login as User2. + session := loginUser(t, user.Name) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) + + repoName := "repo1" + + req := NewRequestf(t, "GET", "/user2/%s/compare/v1.1...master", repoName). + AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + + var apiResp *api.Compare + DecodeJSON(t, resp, &apiResp) + + assert.Len(t, apiResp.TotalCommits, 1) + assert.Equal(t, "Initial commit", apiResp.Commits[0].RepoCommit.Message) + assert.Equal(t, "65f1bf27bc3bf70f64657658635e66094edbcb4d", apiResp.Commits[0].SHA) + assert.Equal(t, setting.AppURL+"api/v1/repos/user2/repo1/git/commits/65f1bf27bc3bf70f64657658635e66094edbcb4d", apiResp.Commits[0].URL) +} From e37ebf2c5c5d16f68bd0094a34fa6280125c65cf Mon Sep 17 00:00:00 2001 From: appleboy Date: Wed, 10 Apr 2024 21:29:46 +0800 Subject: [PATCH 05/11] chore: update Signed-off-by: appleboy --- tests/integration/api_repo_compare_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/api_repo_compare_test.go b/tests/integration/api_repo_compare_test.go index e9abb42d8612d..a0e6dcdc997cd 100644 --- a/tests/integration/api_repo_compare_test.go +++ b/tests/integration/api_repo_compare_test.go @@ -13,6 +13,7 @@ import ( "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/tests" + "github.com/stretchr/testify/assert" ) From ab5dcdafd2599f6f4c9372a8563cd9f9bbc8d4ee Mon Sep 17 00:00:00 2001 From: appleboy Date: Wed, 10 Apr 2024 21:41:40 +0800 Subject: [PATCH 06/11] chore: update Signed-off-by: appleboy --- tests/integration/api_repo_compare_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/api_repo_compare_test.go b/tests/integration/api_repo_compare_test.go index a0e6dcdc997cd..513ee628758a6 100644 --- a/tests/integration/api_repo_compare_test.go +++ b/tests/integration/api_repo_compare_test.go @@ -27,7 +27,7 @@ func TestAPICompareTag(t *testing.T) { repoName := "repo1" - req := NewRequestf(t, "GET", "/user2/%s/compare/v1.1...master", repoName). + req := NewRequestf(t, "GET", "/api/v1/repos/user2/%s/compare/v1.1...master", repoName). AddTokenAuth(token) resp := MakeRequest(t, req, http.StatusOK) From c2c3e59c9cebe6cacfba0332d29fd7f81837f2bd Mon Sep 17 00:00:00 2001 From: appleboy Date: Sat, 13 Apr 2024 19:48:15 +0800 Subject: [PATCH 07/11] update Signed-off-by: appleboy --- tests/integration/api_repo_compare_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/integration/api_repo_compare_test.go b/tests/integration/api_repo_compare_test.go index 513ee628758a6..af905d7e7ae67 100644 --- a/tests/integration/api_repo_compare_test.go +++ b/tests/integration/api_repo_compare_test.go @@ -4,13 +4,13 @@ package integration import ( + "log" "net/http" "testing" auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/tests" @@ -34,8 +34,7 @@ func TestAPICompareTag(t *testing.T) { var apiResp *api.Compare DecodeJSON(t, resp, &apiResp) + log.Printf("Total commits: %v", apiResp.TotalCommits) + log.Printf("Commits: %v", apiResp.Commits) assert.Len(t, apiResp.TotalCommits, 1) - assert.Equal(t, "Initial commit", apiResp.Commits[0].RepoCommit.Message) - assert.Equal(t, "65f1bf27bc3bf70f64657658635e66094edbcb4d", apiResp.Commits[0].SHA) - assert.Equal(t, setting.AppURL+"api/v1/repos/user2/repo1/git/commits/65f1bf27bc3bf70f64657658635e66094edbcb4d", apiResp.Commits[0].URL) } From a21234b4c6cddd563c906d46b4952c46205d005f Mon Sep 17 00:00:00 2001 From: appleboy Date: Sat, 13 Apr 2024 20:20:39 +0800 Subject: [PATCH 08/11] update Signed-off-by: appleboy --- tests/integration/api_repo_compare_test.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/tests/integration/api_repo_compare_test.go b/tests/integration/api_repo_compare_test.go index af905d7e7ae67..f3188eb49f621 100644 --- a/tests/integration/api_repo_compare_test.go +++ b/tests/integration/api_repo_compare_test.go @@ -4,7 +4,6 @@ package integration import ( - "log" "net/http" "testing" @@ -17,7 +16,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestAPICompareTag(t *testing.T) { +func TestAPICompareBranches(t *testing.T) { defer tests.PrepareTestEnv(t)() user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) @@ -25,16 +24,15 @@ func TestAPICompareTag(t *testing.T) { session := loginUser(t, user.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) - repoName := "repo1" + repoName := "repo20" - req := NewRequestf(t, "GET", "/api/v1/repos/user2/%s/compare/v1.1...master", repoName). + req := NewRequestf(t, "GET", "/api/v1/repos/user2/%s/compare/add-csv...remove-files-b", repoName). AddTokenAuth(token) resp := MakeRequest(t, req, http.StatusOK) var apiResp *api.Compare DecodeJSON(t, resp, &apiResp) - log.Printf("Total commits: %v", apiResp.TotalCommits) - log.Printf("Commits: %v", apiResp.Commits) - assert.Len(t, apiResp.TotalCommits, 1) + assert.Equal(t, 2, apiResp.TotalCommits) + assert.Len(t, apiResp.Commits, 2) } From 6ce81dba1b1c5eb716ee08c7557e2984a29bcc72 Mon Sep 17 00:00:00 2001 From: appleboy Date: Sat, 13 Apr 2024 21:34:14 +0800 Subject: [PATCH 09/11] chore: check permission Signed-off-by: appleboy --- routers/api/v1/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index b5ec07fc1d95f..bd4015898de6f 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -1066,7 +1066,7 @@ func Routes() *web.Route { m.Post("/migrate", reqToken(), bind(api.MigrateRepoOptions{}), repo.Migrate) m.Group("/{username}/{reponame}", func() { - m.Get("/compare/*", repo.CompareDiff) + m.Get("/compare/*", reqAnyRepoReader(), repo.CompareDiff) m.Combo("").Get(reqAnyRepoReader(), repo.Get). Delete(reqToken(), reqOwner(), repo.Delete). From 7d9723d1a39e0a0c4c87691c8130c5a17b66fc2e Mon Sep 17 00:00:00 2001 From: Bo-Yi Wu Date: Sat, 13 Apr 2024 23:34:07 +0800 Subject: [PATCH 10/11] Update api.go Co-authored-by: Lunny Xiao --- routers/api/v1/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index bd4015898de6f..1fc768296685f 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -1066,7 +1066,7 @@ func Routes() *web.Route { m.Post("/migrate", reqToken(), bind(api.MigrateRepoOptions{}), repo.Migrate) m.Group("/{username}/{reponame}", func() { - m.Get("/compare/*", reqAnyRepoReader(), repo.CompareDiff) + m.Get("/compare/*", reqRepoReader(unit.TypeCode), repo.CompareDiff) m.Combo("").Get(reqAnyRepoReader(), repo.Get). Delete(reqToken(), reqOwner(), repo.Delete). From 94746ecc56147f2265960d0c140cc314137f74df Mon Sep 17 00:00:00 2001 From: appleboy Date: Mon, 15 Apr 2024 21:08:03 +0800 Subject: [PATCH 11/11] refactor: simplify logic and improve variable clarity - Remove unused imports and functions - Refactor the `ParseCompareInfo` function to simplify the logic - Update variable names for clarity - Adjust the comparison of commits in the `CompareDiff` function Signed-off-by: appleboy --- routers/api/v1/repo/compare.go | 326 +++------------------------------ 1 file changed, 24 insertions(+), 302 deletions(-) diff --git a/routers/api/v1/repo/compare.go b/routers/api/v1/repo/compare.go index 8cf6bf47d7bb1..549b9b7fa91e8 100644 --- a/routers/api/v1/repo/compare.go +++ b/routers/api/v1/repo/compare.go @@ -7,304 +7,13 @@ import ( "net/http" "strings" - access_model "code.gitea.io/gitea/models/perm/access" - repo_model "code.gitea.io/gitea/models/repo" - "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" - "code.gitea.io/gitea/modules/log" api "code.gitea.io/gitea/modules/structs" - "code.gitea.io/gitea/modules/util" - "code.gitea.io/gitea/routers/common" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" ) -// ParseCompareInfo parse compare info between two commit for preparing comparing references -func ParseCompareInfo(ctx *context.APIContext) *common.CompareInfo { - baseRepo := ctx.Repo.Repository - ci := &common.CompareInfo{} - - fileOnly := ctx.FormBool("file-only") - - // Get compared branches information - // A full compare url is of the form: - // - // 1. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headBranch} - // 2. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}:{:headBranch} - // 3. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}/{:headRepoName}:{:headBranch} - // 4. /{:baseOwner}/{:baseRepoName}/compare/{:headBranch} - // 5. /{:baseOwner}/{:baseRepoName}/compare/{:headOwner}:{:headBranch} - // 6. /{:baseOwner}/{:baseRepoName}/compare/{:headOwner}/{:headRepoName}:{:headBranch} - // - // Here we obtain the infoPath "{:baseBranch}...[{:headOwner}/{:headRepoName}:]{:headBranch}" as ctx.Params("*") - // with the :baseRepo in ctx.Repo. - // - // Note: Generally :headRepoName is not provided here - we are only passed :headOwner. - // - // How do we determine the :headRepo? - // - // 1. If :headOwner is not set then the :headRepo = :baseRepo - // 2. If :headOwner is set - then look for the fork of :baseRepo owned by :headOwner - // 3. But... :baseRepo could be a fork of :headOwner's repo - so check that - // 4. Now, :baseRepo and :headRepos could be forks of the same repo - so check that - // - // format: ...[:] - // base<-head: master...head:feature - // same repo: master...feature - - var ( - isSameRepo bool - infoPath string - err error - ) - - infoPath = ctx.Params("*") - var infos []string - if infoPath == "" { - infos = []string{baseRepo.DefaultBranch, baseRepo.DefaultBranch} - } else { - infos = strings.SplitN(infoPath, "...", 2) - if len(infos) != 2 { - if infos = strings.SplitN(infoPath, "..", 2); len(infos) == 2 { - ci.DirectComparison = true - } else { - infos = []string{baseRepo.DefaultBranch, infoPath} - } - } - } - - ci.BaseBranch = infos[0] - - // If there is no head repository, it means compare between same repository. - headInfos := strings.Split(infos[1], ":") - if len(headInfos) == 1 { - isSameRepo = true - ci.HeadUser = ctx.Repo.Owner - ci.HeadBranch = headInfos[0] - } else if len(headInfos) == 2 { - headInfosSplit := strings.Split(headInfos[0], "/") - if len(headInfosSplit) == 1 { - ci.HeadUser, err = user_model.GetUserByName(ctx, headInfos[0]) - if err != nil { - if user_model.IsErrUserNotExist(err) { - ctx.NotFound("GetUserByName", nil) - } else { - ctx.ServerError("GetUserByName", err) - } - return nil - } - ci.HeadBranch = headInfos[1] - isSameRepo = ci.HeadUser.ID == ctx.Repo.Owner.ID - if isSameRepo { - ci.HeadRepo = baseRepo - } - } else { - ci.HeadRepo, err = repo_model.GetRepositoryByOwnerAndName(ctx, headInfosSplit[0], headInfosSplit[1]) - if err != nil { - if repo_model.IsErrRepoNotExist(err) { - ctx.NotFound("GetRepositoryByOwnerAndName", nil) - } else { - ctx.ServerError("GetRepositoryByOwnerAndName", err) - } - return nil - } - if err := ci.HeadRepo.LoadOwner(ctx); err != nil { - if user_model.IsErrUserNotExist(err) { - ctx.NotFound("GetUserByName", nil) - } else { - ctx.ServerError("GetUserByName", err) - } - return nil - } - ci.HeadBranch = headInfos[1] - ci.HeadUser = ci.HeadRepo.Owner - isSameRepo = ci.HeadRepo.ID == ctx.Repo.Repository.ID - } - } else { - ctx.NotFound("CompareAndPullRequest", nil) - return nil - } - ctx.Repo.PullRequest.SameRepo = isSameRepo - - // Check if base branch is valid. - baseIsCommit := ctx.Repo.GitRepo.IsCommitExist(ci.BaseBranch) - baseIsBranch := ctx.Repo.GitRepo.IsBranchExist(ci.BaseBranch) - baseIsTag := ctx.Repo.GitRepo.IsTagExist(ci.BaseBranch) - - if !baseIsCommit && !baseIsBranch && !baseIsTag { - // Check if baseBranch is short sha commit hash - if baseCommit, _ := ctx.Repo.GitRepo.GetCommit(ci.BaseBranch); baseCommit != nil { - ci.BaseBranch = baseCommit.ID.String() - } else if ci.BaseBranch == ctx.Repo.GetObjectFormat().EmptyObjectID().String() { - if isSameRepo { - ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(ci.HeadBranch)) - } else { - ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(ci.HeadRepo.FullName()) + ":" + util.PathEscapeSegments(ci.HeadBranch)) - } - return nil - } else { - ctx.NotFound("IsRefExist", nil) - return nil - } - } - - // Now we have the repository that represents the base - - // The current base and head repositories and branches may not - // actually be the intended branches that the user wants to - // create a pull-request from - but also determining the head - // repo is difficult. - - // We will want therefore to offer a few repositories to set as - // our base and head - - // 1. First if the baseRepo is a fork get the "RootRepo" it was - // forked from - var rootRepo *repo_model.Repository - if baseRepo.IsFork { - err = baseRepo.GetBaseRepo(ctx) - if err != nil { - if !repo_model.IsErrRepoNotExist(err) { - ctx.ServerError("Unable to find root repo", err) - return nil - } - } else { - rootRepo = baseRepo.BaseRepo - } - } - - // 2. Now if the current user is not the owner of the baseRepo, - // check if they have a fork of the base repo and offer that as - // "OwnForkRepo" - var ownForkRepo *repo_model.Repository - if ctx.Doer != nil && baseRepo.OwnerID != ctx.Doer.ID { - repo := repo_model.GetForkedRepo(ctx, ctx.Doer.ID, baseRepo.ID) - if repo != nil { - ownForkRepo = repo - } - } - - has := ci.HeadRepo != nil - // 3. If the base is a forked from "RootRepo" and the owner of - // the "RootRepo" is the :headUser - set headRepo to that - if !has && rootRepo != nil && rootRepo.OwnerID == ci.HeadUser.ID { - ci.HeadRepo = rootRepo - has = true - } - - // 4. If the ctx.Doer has their own fork of the baseRepo and the headUser is the ctx.Doer - // set the headRepo to the ownFork - if !has && ownForkRepo != nil && ownForkRepo.OwnerID == ci.HeadUser.ID { - ci.HeadRepo = ownForkRepo - has = true - } - - // 5. If the headOwner has a fork of the baseRepo - use that - if !has { - ci.HeadRepo = repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, baseRepo.ID) - has = ci.HeadRepo != nil - } - - // 6. If the baseRepo is a fork and the headUser has a fork of that use that - if !has && baseRepo.IsFork { - ci.HeadRepo = repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, baseRepo.ForkID) - has = ci.HeadRepo != nil - } - - // 7. Finally open the git repo - if isSameRepo { - ci.HeadRepo = ctx.Repo.Repository - ci.HeadGitRepo = ctx.Repo.GitRepo - } else if has { - ci.HeadGitRepo, err = gitrepo.OpenRepository(ctx, ci.HeadRepo) - if err != nil { - ctx.ServerError("OpenRepository", err) - return nil - } - defer ci.HeadGitRepo.Close() - } else { - ctx.NotFound("ParseCompareInfo", nil) - return nil - } - - // Now we need to assert that the ctx.Doer has permission to read - // the baseRepo's code and pulls - // (NOT headRepo's) - permBase, err := access_model.GetUserRepoPermission(ctx, baseRepo, ctx.Doer) - if err != nil { - ctx.ServerError("GetUserRepoPermission", err) - return nil - } - if !permBase.CanRead(unit.TypeCode) { - if log.IsTrace() { - log.Trace("Permission Denied: User: %-v cannot read code in Repo: %-v\nUser in baseRepo has Permissions: %-+v", - ctx.Doer, - baseRepo, - permBase) - } - ctx.NotFound("ParseCompareInfo", nil) - return nil - } - - // If we're not merging from the same repo: - if !isSameRepo { - // Assert ctx.Doer has permission to read headRepo's codes - permHead, err := access_model.GetUserRepoPermission(ctx, ci.HeadRepo, ctx.Doer) - if err != nil { - ctx.ServerError("GetUserRepoPermission", err) - return nil - } - if !permHead.CanRead(unit.TypeCode) { - if log.IsTrace() { - log.Trace("Permission Denied: User: %-v cannot read code in Repo: %-v\nUser in headRepo has Permissions: %-+v", - ctx.Doer, - ci.HeadRepo, - permHead) - } - ctx.NotFound("ParseCompareInfo", nil) - return nil - } - } - - // Check if head branch is valid. - headIsCommit := ci.HeadGitRepo.IsCommitExist(ci.HeadBranch) - headIsBranch := ci.HeadGitRepo.IsBranchExist(ci.HeadBranch) - headIsTag := ci.HeadGitRepo.IsTagExist(ci.HeadBranch) - if !headIsCommit && !headIsBranch && !headIsTag { - // Check if headBranch is short sha commit hash - if headCommit, _ := ci.HeadGitRepo.GetCommit(ci.HeadBranch); headCommit != nil { - ci.HeadBranch = headCommit.ID.String() - ctx.Data["HeadBranch"] = ci.HeadBranch - } else { - ctx.NotFound("IsRefExist", nil) - return nil - } - } - - baseBranchRef := ci.BaseBranch - if baseIsBranch { - baseBranchRef = git.BranchPrefix + ci.BaseBranch - } else if baseIsTag { - baseBranchRef = git.TagPrefix + ci.BaseBranch - } - headBranchRef := ci.HeadBranch - if headIsBranch { - headBranchRef = git.BranchPrefix + ci.HeadBranch - } else if headIsTag { - headBranchRef = git.TagPrefix + ci.HeadBranch - } - - ci.CompareInfo, err = ci.HeadGitRepo.GetCompareInfo(baseRepo.RepoPath(), baseBranchRef, headBranchRef, ci.DirectComparison, fileOnly) - if err != nil { - ctx.ServerError("GetCompareInfo", err) - return nil - } - - return ci -} - // CompareDiff compare two branches or commits func CompareDiff(ctx *context.APIContext) { // swagger:operation GET /repos/{owner}/{repo}/compare/{basehead} Get commit comparison information @@ -344,24 +53,37 @@ func CompareDiff(ctx *context.APIContext) { defer gitRepo.Close() } - ci := ParseCompareInfo(ctx) - defer func() { - if ci != nil && ci.HeadGitRepo != nil { - ci.HeadGitRepo.Close() + infoPath := ctx.Params("*") + infos := []string{ctx.Repo.Repository.DefaultBranch, ctx.Repo.Repository.DefaultBranch} + if infoPath != "" { + infos = strings.SplitN(infoPath, "...", 2) + if len(infos) != 2 { + if infos = strings.SplitN(infoPath, "..", 2); len(infos) != 2 { + infos = []string{ctx.Repo.Repository.DefaultBranch, infoPath} + } } - }() + } + + _, _, headGitRepo, ci, _, _ := parseCompareInfo(ctx, api.CreatePullRequestOption{ + Base: infos[0], + Head: infos[1], + }) if ctx.Written() { return } + defer headGitRepo.Close() + + verification := ctx.FormString("verification") == "" || ctx.FormBool("verification") + files := ctx.FormString("files") == "" || ctx.FormBool("files") - apiCommits := make([]*api.Commit, 0, len(ci.CompareInfo.Commits)) + apiCommits := make([]*api.Commit, 0, len(ci.Commits)) userCache := make(map[string]*user_model.User) - for i := 0; i < len(ci.CompareInfo.Commits); i++ { - apiCommit, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, ci.CompareInfo.Commits[i], userCache, + for i := 0; i < len(ci.Commits); i++ { + apiCommit, err := convert.ToCommit(ctx, ctx.Repo.Repository, ctx.Repo.GitRepo, ci.Commits[i], userCache, convert.ToCommitOptions{ Stat: true, - Verification: ctx.FormBool("verification"), - Files: ctx.FormBool("files"), + Verification: verification, + Files: files, }) if err != nil { ctx.ServerError("toCommit", err) @@ -371,7 +93,7 @@ func CompareDiff(ctx *context.APIContext) { } ctx.JSON(http.StatusOK, &api.Compare{ - TotalCommits: len(ci.CompareInfo.Commits), + TotalCommits: len(ci.Commits), Commits: apiCommits, }) }