From 4ee5334c109b605e7a9f5d66fac2e271143d829b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 11 Dec 2024 00:23:12 -0800 Subject: [PATCH] Refactor all compare routers --- models/repo/fork.go | 29 ++---- models/repo/fork_test.go | 4 +- routers/api/v1/repo/compare.go | 19 ---- routers/common/compare.go | 97 +++++++++++++++++-- routers/common/compare_test.go | 18 ++++ routers/web/repo/compare.go | 168 +++------------------------------ routers/web/repo/fork.go | 6 +- services/repository/fork.go | 4 +- 8 files changed, 141 insertions(+), 204 deletions(-) diff --git a/models/repo/fork.go b/models/repo/fork.go index 1c75e86458..b69c539ccc 100644 --- a/models/repo/fork.go +++ b/models/repo/fork.go @@ -21,15 +21,17 @@ func GetRepositoriesByForkID(ctx context.Context, forkID int64) ([]*Repository, } // GetForkedRepo checks if given user has already forked a repository with given ID. -func GetForkedRepo(ctx context.Context, ownerID, repoID int64) *Repository { +func GetForkedRepo(ctx context.Context, ownerID, repoID int64) (*Repository, error) { repo := new(Repository) - has, _ := db.GetEngine(ctx). + has, err := db.GetEngine(ctx). Where("owner_id=? AND fork_id=?", ownerID, repoID). Get(repo) - if has { - return repo + if err != nil { + return nil, err + } else if !has { + return nil, ErrRepoNotExist{ID: repoID} } - return nil + return repo, nil } // HasForkedRepo checks if given user has already forked a repository with given ID. @@ -41,19 +43,6 @@ func HasForkedRepo(ctx context.Context, ownerID, repoID int64) bool { return has } -// GetUserFork return user forked repository from this repository, if not forked return nil -func GetUserFork(ctx context.Context, repoID, userID int64) (*Repository, error) { - var forkedRepo Repository - has, err := db.GetEngine(ctx).Where("fork_id = ?", repoID).And("owner_id = ?", userID).Get(&forkedRepo) - if err != nil { - return nil, err - } - if !has { - return nil, nil - } - return &forkedRepo, nil -} - // IncrementRepoForkNum increment repository fork number func IncrementRepoForkNum(ctx context.Context, repoID int64) error { _, err := db.GetEngine(ctx).Exec("UPDATE `repository` SET num_forks=num_forks+1 WHERE id=?", repoID) @@ -87,8 +76,8 @@ func GetForksByUserAndOrgs(ctx context.Context, user *user_model.User, repo *Rep if user == nil { return repoList, nil } - forkedRepo, err := GetUserFork(ctx, repo.ID, user.ID) - if err != nil { + forkedRepo, err := GetForkedRepo(ctx, repo.ID, user.ID) + if err != nil && !IsErrRepoNotExist(err) { return repoList, err } if forkedRepo != nil { diff --git a/models/repo/fork_test.go b/models/repo/fork_test.go index e8dca204cc..7a15874329 100644 --- a/models/repo/fork_test.go +++ b/models/repo/fork_test.go @@ -20,14 +20,14 @@ func TestGetUserFork(t *testing.T) { repo, err := repo_model.GetRepositoryByID(db.DefaultContext, 10) assert.NoError(t, err) assert.NotNil(t, repo) - repo, err = repo_model.GetUserFork(db.DefaultContext, repo.ID, 13) + repo, err = repo_model.GetForkedRepo(db.DefaultContext, repo.ID, 13) assert.NoError(t, err) assert.NotNil(t, repo) repo, err = repo_model.GetRepositoryByID(db.DefaultContext, 9) assert.NoError(t, err) assert.NotNil(t, repo) - repo, err = repo_model.GetUserFork(db.DefaultContext, repo.ID, 13) + repo, err = repo_model.GetForkedRepo(db.DefaultContext, repo.ID, 13) assert.NoError(t, err) assert.Nil(t, repo) } diff --git a/routers/api/v1/repo/compare.go b/routers/api/v1/repo/compare.go index 6c4d428f22..56e00db943 100644 --- a/routers/api/v1/repo/compare.go +++ b/routers/api/v1/repo/compare.go @@ -105,25 +105,6 @@ func CompareDiff(ctx *context.APIContext) { ctx.Repo.PullRequest.SameRepo = ci.IsSameRepo() log.Trace("Repo path: %q, base branch: %q, head branch: %q", ctx.Repo.GitRepo.Path, ci.BaseOriRef, ci.HeadOriRef) - // Check if current user has fork of repository or in the same repository. - /*headRepo := repo_model.GetForkedRepo(ctx, ci.HeadUser.ID, ctx.Repo.Repository.ID) - if headRepo == nil && !ci.IsSameRepo() { - err := ctx.Repo.Repository.GetBaseRepo(ctx) - if err != nil { - ctx.Error(http.StatusInternalServerError, "GetBaseRepo", err) - return nil, nil, nil, "", "" - } - - // Check if baseRepo's base repository is the same as headUser's repository. - if baseRepo.BaseRepo == nil || baseRepo.BaseRepo.OwnerID != headUser.ID { - log.Trace("parseCompareInfo[%d]: does not have fork or in same repository", baseRepo.ID) - ctx.NotFound("GetBaseRepo") - return nil, nil, nil, "", "" - } - // Assign headRepo so it can be used below. - headRepo = baseRepo.BaseRepo - }*/ - ci.CompareInfo, err = ci.HeadGitRepo.GetCompareInfo(repo_model.RepoPath(baseRepo.Owner.Name, baseRepo.Name), ci.BaseOriRef, ci.HeadOriRef, false, false) if err != nil { ctx.Error(http.StatusInternalServerError, "GetCompareInfo", err) diff --git a/routers/common/compare.go b/routers/common/compare.go index d1b4327a1c..0c9d7295fb 100644 --- a/routers/common/compare.go +++ b/routers/common/compare.go @@ -53,7 +53,7 @@ func parseHead(head string) (string, string, string) { } ownerRepo := strings.SplitN(paths[0], "/", 2) if len(ownerRepo) == 1 { - return "", paths[0], paths[1] + return paths[0], "", paths[1] } return ownerRepo[0], ownerRepo[1], paths[1] } @@ -73,6 +73,7 @@ func parseCompareRouter(router string) (*CompareRouter, error) { HeadOriRef: headRef, HeadOwnerName: headOwnerName, HeadRepoName: headRepoName, + DotTimes: dotTimes, }, nil } else if len(parts) > 2 { return nil, util.NewSilentWrapErrorf(util.ErrInvalidArgument, "invalid compare router: %s", router) @@ -120,10 +121,10 @@ func (ci *CompareInfo) Close() { func detectFullRef(ctx context.Context, repoID int64, gitRepo *git.Repository, oriRef string) (git.RefName, bool, error) { b, err := git_model.GetBranch(ctx, repoID, oriRef) - if err != nil { + if err != nil && !git_model.IsErrBranchNotExist(err) { return "", false, err } - if !b.IsDeleted { + if b != nil && !b.IsDeleted { return git.RefNameFromBranch(oriRef), false, nil } @@ -142,6 +143,78 @@ func detectFullRef(ctx context.Context, repoID int64, gitRepo *git.Repository, o return git.RefName(commitObjectID.String()), true, nil } +func findHeadRepo(ctx context.Context, baseRepo *repo_model.Repository, headUserID int64) (*repo_model.Repository, error) { + if baseRepo.IsFork { + curRepo := baseRepo + for curRepo.OwnerID != headUserID { // We assume the fork deepth is not too deep. + if err := curRepo.GetBaseRepo(ctx); err != nil { + return nil, err + } + if curRepo.BaseRepo == nil { + return findHeadRepoFromRootBase(ctx, curRepo, headUserID, 3) + } + curRepo = curRepo.BaseRepo + } + return curRepo, nil + } + + return findHeadRepoFromRootBase(ctx, baseRepo, headUserID, 3) +} + +func findHeadRepoFromRootBase(ctx context.Context, baseRepo *repo_model.Repository, headUserID int64, traverseLevel int) (*repo_model.Repository, error) { + if traverseLevel == 0 { + return nil, nil + } + // test if we are lucky + repo, err := repo_model.GetForkedRepo(ctx, headUserID, baseRepo.ID) + if err == nil { + return repo, nil + } + if !repo_model.IsErrRepoNotExist(err) { + return nil, err + } + + firstLevelForkedRepo, err := repo_model.GetRepositoriesByForkID(ctx, baseRepo.ID) + if err != nil { + return nil, err + } + for _, repo := range firstLevelForkedRepo { + forked, err := findHeadRepoFromRootBase(ctx, repo, headUserID, traverseLevel-1) + if err != nil { + return nil, err + } + if forked != nil { + return forked, nil + } + } + return nil, nil +} + +// ParseComparePathParams Get compare 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.PathParam("*") +// 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 func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *repo_model.Repository, baseGitRepo *git.Repository) (*CompareInfo, error) { ci := &CompareInfo{} var err error @@ -159,12 +232,18 @@ func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *rep } if ci.IsSameRepo() { + ci.HeadOwnerName = baseRepo.Owner.Name + ci.HeadRepoName = baseRepo.Name ci.HeadUser = baseRepo.Owner ci.HeadRepo = baseRepo ci.HeadGitRepo = baseGitRepo } else { if ci.HeadOwnerName == baseRepo.Owner.Name { ci.HeadUser = baseRepo.Owner + if ci.HeadRepoName == "" { + ci.HeadRepoName = baseRepo.Name + ci.HeadRepo = baseRepo + } } else { ci.HeadUser, err = user_model.GetUserByName(ctx, ci.HeadOwnerName) if err != nil { @@ -172,9 +251,15 @@ func ParseComparePathParams(ctx context.Context, pathParam string, baseRepo *rep } } - ci.HeadRepo, err = repo_model.GetRepositoryByOwnerAndName(ctx, ci.HeadOwnerName, ci.HeadRepoName) - if err != nil { - return nil, err + if ci.HeadRepo == nil { + if ci.HeadRepoName != "" { + ci.HeadRepo, err = repo_model.GetRepositoryByOwnerAndName(ctx, ci.HeadOwnerName, ci.HeadRepoName) + } else { + ci.HeadRepo, err = findHeadRepo(ctx, baseRepo, ci.HeadUser.ID) + } + if err != nil { + return nil, err + } } ci.HeadRepo.Owner = ci.HeadUser ci.HeadGitRepo, err = gitrepo.OpenRepository(ctx, ci.HeadRepo) diff --git a/routers/common/compare_test.go b/routers/common/compare_test.go index 2341f80792..aa30389401 100644 --- a/routers/common/compare_test.go +++ b/routers/common/compare_test.go @@ -52,6 +52,7 @@ func TestCompareRouters(t *testing.T) { router: "develop", compareRouter: &CompareRouter{ HeadOriRef: "develop", + DotTimes: 3, }, }, { @@ -60,6 +61,7 @@ func TestCompareRouters(t *testing.T) { HeadOwnerName: "lunny", HeadRepoName: "forked_repo", HeadOriRef: "develop", + DotTimes: 3, }, }, { @@ -101,6 +103,22 @@ func TestCompareRouters(t *testing.T) { DotTimes: 3, }, }, + { + router: "teabot-patch-1...v0.0.1", + compareRouter: &CompareRouter{ + BaseOriRef: "teabot-patch-1", + HeadOriRef: "v0.0.1", + DotTimes: 3, + }, + }, + { + router: "teabot:feature1", + compareRouter: &CompareRouter{ + HeadOwnerName: "teabot", + HeadOriRef: "feature1", + DotTimes: 3, + }, + }, { router: "8eb19a5ae19abae15c0666d4ab98906139a7f439...283c030497b455ecfa759d4649f9f8b45158742e", compareRouter: &CompareRouter{ diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 4279cf9b3a..a6ce48506b 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -189,32 +189,6 @@ func setCsvCompareContext(ctx *context.Context) { // ParseCompareInfo parse compare info between two commit for preparing comparing references // Permission check for base repository's code read should be checked before invoking this function func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { - // 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.PathParam("*") - // 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 - fileOnly := ctx.FormBool("file-only") pathParam := ctx.PathParam("*") baseRepo := ctx.Repo.Repository @@ -250,90 +224,6 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { return nil } - ctx.Data["PageIsComparePull"] = ci.IsPull() && ctx.Repo.CanReadIssuesOrPulls(true) - ctx.Data["BaseName"] = baseRepo.OwnerName - ctx.Data["BaseBranch"] = ci.BaseOriRef - ctx.Data["HeadUser"] = ci.HeadUser - ctx.Data["HeadBranch"] = ci.HeadOriRef - ctx.Repo.PullRequest.SameRepo = ci.IsSameRepo() - - ctx.Data["BaseIsCommit"] = ci.IsBaseCommit - ctx.Data["BaseIsBranch"] = ci.BaseFullRef.IsBranch() - ctx.Data["BaseIsTag"] = ci.BaseFullRef.IsTag() - 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 !ci.IsSameRepo() && !has { - ctx.Data["PageIsComparePull"] = false - } - - ctx.Data["HeadRepo"] = ci.HeadRepo - ctx.Data["BaseCompareRepo"] = ctx.Repo.Repository - // If we're not merging from the same repo: if !ci.IsSameRepo() { // Assert ctx.Doer has permission to read headRepo's codes @@ -355,53 +245,23 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo { 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 - } + // TODO: prepareRepos, branches and tags for dropdowns - ctx.Data["RootRepoBranches"] = branches - ctx.Data["RootRepoTags"] = tags - } - } - } + ctx.Data["PageIsComparePull"] = ci.IsPull() && ctx.Repo.CanReadIssuesOrPulls(true) + ctx.Data["BaseName"] = baseRepo.OwnerName + ctx.Data["BaseBranch"] = ci.BaseOriRef + ctx.Data["HeadUser"] = ci.HeadUser + ctx.Data["HeadBranch"] = ci.HeadOriRef + ctx.Repo.PullRequest.SameRepo = ci.IsSameRepo() - // 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 - } - } - } + ctx.Data["BaseIsCommit"] = ci.IsBaseCommit + ctx.Data["BaseIsBranch"] = ci.BaseFullRef.IsBranch() + ctx.Data["BaseIsTag"] = ci.BaseFullRef.IsTag() + ctx.Data["IsPull"] = true + // ctx.Data["OwnForkRepo"] = ownForkRepo FIXME: This is not used + ctx.Data["HeadRepo"] = ci.HeadRepo + ctx.Data["BaseCompareRepo"] = ctx.Repo.Repository ctx.Data["HeadIsCommit"] = ci.IsHeadCommit ctx.Data["HeadIsBranch"] = ci.HeadFullRef.IsBranch() ctx.Data["HeadIsTag"] = ci.HeadFullRef.IsTag() diff --git a/routers/web/repo/fork.go b/routers/web/repo/fork.go index 27e42a8f98..c0d823c079 100644 --- a/routers/web/repo/fork.go +++ b/routers/web/repo/fork.go @@ -166,7 +166,11 @@ func ForkPost(ctx *context.Context) { ctx.RenderWithErr(ctx.Tr("repo.settings.new_owner_has_same_repo"), tplFork, &form) return } - repo := repo_model.GetForkedRepo(ctx, ctxUser.ID, traverseParentRepo.ID) + repo, err := repo_model.GetForkedRepo(ctx, ctxUser.ID, traverseParentRepo.ID) + if err != nil && !repo_model.IsErrRepoNotExist(err) { + ctx.ServerError("GetForkedRepo", err) + return + } if repo != nil { ctx.Redirect(ctxUser.HomeLink() + "/" + url.PathEscape(repo.Name)) return diff --git a/services/repository/fork.go b/services/repository/fork.go index bc4fdf8562..26057266c5 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -71,8 +71,8 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork } } - forkedRepo, err := repo_model.GetUserFork(ctx, opts.BaseRepo.ID, owner.ID) - if err != nil { + forkedRepo, err := repo_model.GetForkedRepo(ctx, opts.BaseRepo.ID, owner.ID) + if err != nil && !repo_model.IsErrRepoNotExist(err) { return nil, err } if forkedRepo != nil {