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

feat(api): implement branch/commit comparison API #30349

Merged
merged 13 commits into from Apr 16, 2024
Merged

Conversation

appleboy
Copy link
Member

@appleboy appleboy commented Apr 9, 2024

  • 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)

GitHub API https://docs.github.com/en/rest/commits/commits?apiVersion=2022-11-28#compare-two-commits

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 9, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 9, 2024
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels Apr 9, 2024
@lunny
Copy link
Member

lunny commented Apr 9, 2024

Sorry, the code looks like duplicated to previous blocks.

@appleboy
Copy link
Member Author

appleboy commented Apr 9, 2024

@lunny Yes, so I remove some unused logic for API

@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 9, 2024
@appleboy appleboy force-pushed the compare branch 2 times, most recently from 6a38ce9 to 3238464 Compare April 9, 2024 13:36
@appleboy appleboy changed the title [WIP]: feat(api): implement branch/commit comparison API feat(api): implement branch/commit comparison API Apr 9, 2024
@appleboy appleboy added this to the 1.22.0 milestone Apr 9, 2024
@lunny
Copy link
Member

lunny commented Apr 10, 2024

It's better to have an integration test for the API.

@lunny lunny modified the milestones: 1.22.0, 1.23.0 Apr 10, 2024
- 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 <appleboy.tw@gmail.com>
- 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 <appleboy.tw@gmail.com>
Signed-off-by: appleboy <appleboy.tw@gmail.com>
Signed-off-by: appleboy <appleboy.tw@gmail.com>
Signed-off-by: appleboy <appleboy.tw@gmail.com>
Signed-off-by: appleboy <appleboy.tw@gmail.com>
Signed-off-by: appleboy <appleboy.tw@gmail.com>
@appleboy
Copy link
Member Author

Signed-off-by: appleboy <appleboy.tw@gmail.com>
@appleboy
Copy link
Member Author

Added permission check 6ce81db (#30349)

routers/api/v1/api.go Outdated Show resolved Hide resolved
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@appleboy appleboy requested a review from wolfogre April 14, 2024 13:32
@wolfogre
Copy link
Member

wolfogre commented Apr 15, 2024

Sorry, I cannot give my approval since the ParseCompareInfo function is almost copied from

func ParseCompareInfo(ctx *context.Context) *CompareInfo {
baseRepo := ctx.Repo.Repository
ci := &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 branch>...[<head repo>:]<head branch>
// 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
ctx.Data["PageIsComparePull"] = false
} else {
infos = []string{baseRepo.DefaultBranch, infoPath}
}
}
}
ctx.Data["BaseName"] = baseRepo.OwnerName
ci.BaseBranch = infos[0]
ctx.Data["BaseBranch"] = ci.BaseBranch
// 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.Data["HeadUser"] = ci.HeadUser
ctx.Data["HeadBranch"] = ci.HeadBranch
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()
ctx.Data["BaseBranch"] = ci.BaseBranch
baseIsCommit = true
} 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
}
}
ctx.Data["BaseIsCommit"] = baseIsCommit
ctx.Data["BaseIsBranch"] = baseIsBranch
ctx.Data["BaseIsTag"] = baseIsTag
ctx.Data["IsPull"] = true
// 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
ctx.Data["OwnForkRepo"] = ownForkRepo
}
}
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. Otherwise if we're not the same repo and haven't found a repo give up
if !isSameRepo && !has {
ctx.Data["PageIsComparePull"] = false
}
// 8. 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
}
ctx.Data["HeadRepo"] = ci.HeadRepo
ctx.Data["BaseCompareRepo"] = ctx.Repo.Repository
// 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
}
ctx.Data["CanWriteToHeadRepo"] = permHead.CanWrite(unit.TypeCode)
}
// If we have a rootRepo and it's different from:
// 1. the computed base
// 2. the computed head
// then get the branches of it
if rootRepo != nil &&
rootRepo.ID != ci.HeadRepo.ID &&
rootRepo.ID != baseRepo.ID {
canRead := access_model.CheckRepoUnitUser(ctx, rootRepo, ctx.Doer, unit.TypeCode)
if canRead {
ctx.Data["RootRepo"] = rootRepo
if !fileOnly {
branches, tags, err := getBranchesAndTagsForRepo(ctx, rootRepo)
if err != nil {
ctx.ServerError("GetBranchesForRepo", err)
return nil
}
ctx.Data["RootRepoBranches"] = branches
ctx.Data["RootRepoTags"] = tags
}
}
}
// If we have a ownForkRepo and it's different from:
// 1. The computed base
// 2. The computed head
// 3. The rootRepo (if we have one)
// then get the branches from it.
if ownForkRepo != nil &&
ownForkRepo.ID != ci.HeadRepo.ID &&
ownForkRepo.ID != baseRepo.ID &&
(rootRepo == nil || ownForkRepo.ID != rootRepo.ID) {
canRead := access_model.CheckRepoUnitUser(ctx, ownForkRepo, ctx.Doer, unit.TypeCode)
if canRead {
ctx.Data["OwnForkRepo"] = ownForkRepo
if !fileOnly {
branches, tags, err := getBranchesAndTagsForRepo(ctx, ownForkRepo)
if err != nil {
ctx.ServerError("GetBranchesForRepo", err)
return nil
}
ctx.Data["OwnForkRepoBranches"] = branches
ctx.Data["OwnForkRepoTags"] = tags
}
}
}
// 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
headIsCommit = true
} else {
ctx.NotFound("IsRefExist", nil)
return nil
}
}
ctx.Data["HeadIsCommit"] = headIsCommit
ctx.Data["HeadIsBranch"] = headIsBranch
ctx.Data["HeadIsTag"] = headIsTag
// Treat as pull request if both references are branches
if ctx.Data["PageIsComparePull"] == nil {
ctx.Data["PageIsComparePull"] = headIsBranch && baseIsBranch
}
if ctx.Data["PageIsComparePull"] == true && !permBase.CanReadIssuesOrPulls(true) {
if log.IsTrace() {
log.Trace("Permission Denied: User: %-v cannot create/read pull requests in Repo: %-v\nUser in baseRepo has Permissions: %-+v",
ctx.Doer,
baseRepo,
permBase)
}
ctx.NotFound("ParseCompareInfo", 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
}
if ci.DirectComparison {
ctx.Data["BeforeCommitID"] = ci.CompareInfo.BaseCommitID
} else {
ctx.Data["BeforeCommitID"] = ci.CompareInfo.MergeBase
}
return ci
}

It contains complex logic and detailed comments which make me believe it shouldn't be maintained in two places.

@appleboy
Copy link
Member Author

@wolfogre Yes, in the next phase I will refactor the ParseCompareInfo from route/web and route/api.

- 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 <appleboy.tw@gmail.com>
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 15, 2024
@appleboy
Copy link
Member Author

@wolfogre @lunny See the latest commit. 94746ec

@GiteaBot 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 Apr 16, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 16, 2024
@lunny lunny merged commit c70e442 into go-gitea:main Apr 16, 2024
26 checks passed
@lunny lunny modified the milestones: 1.22.0, 1.23.0 Apr 16, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 16, 2024
@appleboy appleboy deleted the compare branch April 16, 2024 03:45
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 17, 2024
* giteaofficial/main:
  Reduce unnecessary database queries on actions table (go-gitea#30509)
  [skip ci] Updated translations via Crowdin
  Tweak and fix toggle checkboxes (go-gitea#30527)
  Tweak repo buttons on mobile and labeled button border-radius (go-gitea#30503)
  Fix long branch name overflows (go-gitea#30345)
  Update API to return 'source_id' for users (go-gitea#29718)
  Allow `preferred_username` as username source for OIDC (go-gitea#30454)
  Fix empty field `login_name` in API response JSON when creating user (go-gitea#30511)
  feat(api): implement branch/commit comparison API (go-gitea#30349)
silverwind pushed a commit that referenced this pull request Apr 21, 2024
- Update branch existence check to also include tag existence check
- Adjust error message for branch/tag existence check

ref: #30349

---------

Signed-off-by: appleboy <appleboy.tw@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Giteabot <teabot@gitea.io>
@lunny lunny mentioned this pull request Apr 22, 2024
lunny added a commit that referenced this pull request Apr 23, 2024
The swagger format on #30349 is not right. This PR will fix it.
caarlos0 added a commit to goreleaser/goreleaser that referenced this pull request Apr 23, 2024
- Add `strings` package import to `gitea.go`
- Implement `Changelog` function in `gitea.go`
- Update `useGitea` constant in `changelog.go`
- Add test for `useGitea` in `changelog_test.go`
- Update `changelog.md` with information about `gitea` customization

ref:

* Server API: go-gitea/gitea#30349
* SDK: https://gitea.com/gitea/go-sdk/pulls/659

---------

Signed-off-by: appleboy <appleboy.tw@gmail.com>
Co-authored-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
meschbach pushed a commit to meschbach/gitea-sdk that referenced this pull request Apr 24, 2024
See the API: go-gitea/gitea#30349

- Add a new file `repo_compare.go` with package `gitea` and `Compare` struct
- Implement `CompareCommits` method in `Client` struct in `repo_compare.go`
- Add `version1_22_0` constant in `version.go`

Signed-off-by: appleboy <appleboy.tw@gmail.com>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Reviewed-on: https://gitea.com/gitea/go-sdk/pulls/659
Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: appleboy <appleboy.tw@gmail.com>
Co-committed-by: appleboy <appleboy.tw@gmail.com>
@wxiaoguang wxiaoguang modified the milestones: 1.23.0, 1.22.0 Apr 27, 2024
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this pull request Apr 30, 2024
- Update branch existence check to also include tag existence check
- Adjust error message for branch/tag existence check

ref: go-gitea/gitea#30349

---------

Signed-off-by: appleboy <appleboy.tw@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Giteabot <teabot@gitea.io>
(cherry picked from commit 6459c50278906893f3cbc2bf3e52eff65e739b37)
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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants