From 0198bbedc193fd1c5733fede11a57b7900cf6c8c Mon Sep 17 00:00:00 2001 From: zeripath Date: Tue, 12 May 2020 06:52:46 +0100 Subject: [PATCH] Allow compare page to look up base, head, own-fork, forkbase-of-head (#11327) * Allow compare page to look up base, head, own-fork, forkbase-of-head Signed-off-by: Andrew Thornton * as per @guillep2k Signed-off-by: Andrew Thornton * Update routers/repo/compare.go * as per @guillep2k Signed-off-by: Andrew Thornton * Rationalise the names a little Signed-off-by: Andrew Thornton * Rationalise the names a little (2) Signed-off-by: Andrew Thornton * Fix 500 with fork of fork Signed-off-by: Andrew Thornton * Prevent 500 on compare different trees Signed-off-by: Andrew Thornton * dotdotdot is perfectly valid in both usernames and repo names Signed-off-by: Andrew Thornton * ensure we can set the head and base repos too Signed-off-by: Andrew Thornton * ensure we can set the head and base repos too (2) Signed-off-by: Andrew Thornton * fix lint Signed-off-by: Andrew Thornton * only set headRepo == baseRepo if isSameRepo Signed-off-by: Andrew Thornton --- modules/git/commit.go | 4 - modules/git/repo_compare.go | 1 - routers/repo/compare.go | 227 +++++++++++++++++++++++++------ templates/repo/diff/compare.tmpl | 35 ++++- 4 files changed, 214 insertions(+), 53 deletions(-) diff --git a/modules/git/commit.go b/modules/git/commit.go index e65782912fea..5e492e27eac4 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -529,10 +529,6 @@ func GetCommitFileStatus(repoPath, commitID string) (*CommitFileStatus, error) { // GetFullCommitID returns full length (40) of commit ID by given short SHA in a repository. func GetFullCommitID(repoPath, shortID string) (string, error) { - if len(shortID) >= 40 { - return shortID, nil - } - commitID, err := NewCommand("rev-parse", shortID).RunInDir(repoPath) if err != nil { if strings.Contains(err.Error(), "exit status 128") { diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 683ac7a7ee42..5bc7f9ca5a6a 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -89,7 +89,6 @@ func (repo *Repository) GetCompareInfo(basePath, baseBranch, headBranch string) return nil, err } compareInfo.NumFiles = len(strings.Split(stdout, "\n")) - 1 - return compareInfo, nil } diff --git a/routers/repo/compare.go b/routers/repo/compare.go index 622c911bbe4d..29a543e67f6e 100644 --- a/routers/repo/compare.go +++ b/routers/repo/compare.go @@ -71,25 +71,45 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, * baseRepo := ctx.Repo.Repository // 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} + // + // 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 ( headUser *models.User + headRepo *models.Repository headBranch string isSameRepo bool infoPath string err error ) infoPath = ctx.Params("*") - infos := strings.Split(infoPath, "...") + infos := strings.SplitN(infoPath, "...", 2) if len(infos) != 2 { log.Trace("ParseCompareInfo[%d]: not enough compared branches information %s", baseRepo.ID, infos) ctx.NotFound("CompareAndPullRequest", nil) return nil, nil, nil, nil, "", "" } + ctx.Data["BaseName"] = baseRepo.OwnerName baseBranch := infos[0] ctx.Data["BaseBranch"] = baseBranch @@ -101,17 +121,44 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, * headBranch = headInfos[0] } else if len(headInfos) == 2 { - headUser, err = models.GetUserByName(headInfos[0]) - if err != nil { - if models.IsErrUserNotExist(err) { - ctx.NotFound("GetUserByName", nil) - } else { - ctx.ServerError("GetUserByName", err) + headInfosSplit := strings.Split(headInfos[0], "/") + if len(headInfosSplit) == 1 { + headUser, err = models.GetUserByName(headInfos[0]) + if err != nil { + if models.IsErrUserNotExist(err) { + ctx.NotFound("GetUserByName", nil) + } else { + ctx.ServerError("GetUserByName", err) + } + return nil, nil, nil, nil, "", "" } - return nil, nil, nil, nil, "", "" + headBranch = headInfos[1] + isSameRepo = headUser.ID == ctx.Repo.Owner.ID + if isSameRepo { + headRepo = baseRepo + } + } else { + headRepo, err = models.GetRepositoryByOwnerAndName(headInfosSplit[0], headInfosSplit[1]) + if err != nil { + if models.IsErrRepoNotExist(err) { + ctx.NotFound("GetRepositoryByOwnerAndName", nil) + } else { + ctx.ServerError("GetRepositoryByOwnerAndName", err) + } + return nil, nil, nil, nil, "", "" + } + if err := headRepo.GetOwner(); err != nil { + if models.IsErrUserNotExist(err) { + ctx.NotFound("GetUserByName", nil) + } else { + ctx.ServerError("GetUserByName", err) + } + return nil, nil, nil, nil, "", "" + } + headBranch = headInfos[1] + headUser = headRepo.Owner + isSameRepo = headRepo.ID == ctx.Repo.Repository.ID } - headBranch = headInfos[1] - isSameRepo = headUser.ID == ctx.Repo.Owner.ID } else { ctx.NotFound("CompareAndPullRequest", nil) return nil, nil, nil, nil, "", "" @@ -139,20 +186,80 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, * ctx.Data["BaseIsBranch"] = baseIsBranch ctx.Data["BaseIsTag"] = baseIsTag - // Check if current user has fork of repository or in the same repository. - headRepo, has := models.HasForkedRepo(headUser.ID, baseRepo.ID) - if !has && !isSameRepo { + // 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 *models.Repository + if baseRepo.IsFork { + err = baseRepo.GetBaseRepo() + if err != nil { + if !models.IsErrRepoNotExist(err) { + ctx.ServerError("Unable to find root repo", err) + return nil, nil, nil, 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 *models.Repository + if baseRepo.OwnerID != ctx.User.ID { + repo, has := models.HasForkedRepo(ctx.User.ID, baseRepo.ID) + if has { + ownForkRepo = repo + ctx.Data["OwnForkRepo"] = ownForkRepo + } + } + + has := 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 == headUser.ID { + headRepo = rootRepo + has = true + } + + // 4. If the ctx.User has their own fork of the baseRepo and the headUser is the ctx.User + // set the headRepo to the ownFork + if !has && ownForkRepo != nil && ownForkRepo.OwnerID == headUser.ID { + headRepo = ownForkRepo + has = true + } + + // 5. If the headOwner has a fork of the baseRepo - use that + if !has { + headRepo, has = models.HasForkedRepo(headUser.ID, baseRepo.ID) + } + + // 6. If the baseRepo is a fork and the headUser has a fork of that use that + if !has && baseRepo.IsFork { + headRepo, has = models.HasForkedRepo(headUser.ID, baseRepo.ForkID) + } + + // 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 var headGitRepo *git.Repository if isSameRepo { headRepo = ctx.Repo.Repository headGitRepo = ctx.Repo.GitRepo - ctx.Data["BaseName"] = headUser.Name - } else { - headGitRepo, err = git.OpenRepository(models.RepoPath(headUser.Name, headRepo.Name)) - ctx.Data["BaseName"] = baseRepo.OwnerName + } else if has { + headGitRepo, err = git.OpenRepository(headRepo.RepoPath()) if err != nil { ctx.ServerError("OpenRepository", err) return nil, nil, nil, nil, "", "" @@ -160,7 +267,11 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, * defer headGitRepo.Close() } - // user should have permission to read baseRepo's codes and pulls, NOT headRepo's + ctx.Data["HeadRepo"] = headRepo + + // Now we need to assert that the ctx.User has permission to read + // the baseRepo's code and pulls + // (NOT headRepo's) permBase, err := models.GetUserRepoPermission(baseRepo, ctx.User) if err != nil { ctx.ServerError("GetUserRepoPermission", err) @@ -177,8 +288,9 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, * return nil, nil, nil, nil, "", "" } + // If we're not merging from the same repo: if !isSameRepo { - // user should have permission to read headrepo's codes + // Assert ctx.User has permission to read headRepo's codes permHead, err := models.GetUserRepoPermission(headRepo, ctx.User) if err != nil { ctx.ServerError("GetUserRepoPermission", err) @@ -196,6 +308,44 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, * } } + // 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 != headRepo.ID && + rootRepo.ID != baseRepo.ID { + perm, branches, err := getBranchesForRepo(ctx.User, rootRepo) + if err != nil { + ctx.ServerError("GetBranchesForRepo", err) + return nil, nil, nil, nil, "", "" + } + if perm { + ctx.Data["RootRepo"] = rootRepo + ctx.Data["RootRepoBranches"] = branches + } + } + + // If we have a ownForkRepo and it's different from: + // 1. The computed base + // 2. The computed hea + // 3. The rootRepo (if we have one) + // then get the branches from it. + if ownForkRepo != nil && + ownForkRepo.ID != headRepo.ID && + ownForkRepo.ID != baseRepo.ID && + (rootRepo == nil || ownForkRepo.ID != rootRepo.ID) { + perm, branches, err := getBranchesForRepo(ctx.User, ownForkRepo) + if err != nil { + ctx.ServerError("GetBranchesForRepo", err) + return nil, nil, nil, nil, "", "" + } + if perm { + ctx.Data["OwnForkRepo"] = ownForkRepo + ctx.Data["OwnForkRepoBranches"] = branches + } + } + // Check if head branch is valid. headIsCommit := headGitRepo.IsCommitExist(headBranch) headIsBranch := headGitRepo.IsBranchExist(headBranch) @@ -343,28 +493,25 @@ func PrepareCompareDiff( return false } -// parseBaseRepoInfo parse base repository if current repo is forked. -// The "base" here means the repository where current repo forks from, -// not the repository fetch from current URL. -func parseBaseRepoInfo(ctx *context.Context, repo *models.Repository) error { - if !repo.IsFork { - return nil - } - if err := repo.GetBaseRepo(); err != nil { - return err - } - - baseGitRepo, err := git.OpenRepository(repo.BaseRepo.RepoPath()) +func getBranchesForRepo(user *models.User, repo *models.Repository) (bool, []string, error) { + perm, err := models.GetUserRepoPermission(repo, user) if err != nil { - return err + return false, nil, err } - defer baseGitRepo.Close() - - ctx.Data["BaseRepoBranches"], err = baseGitRepo.GetBranches() + if !perm.CanRead(models.UnitTypeCode) { + return false, nil, nil + } + gitRepo, err := git.OpenRepository(repo.RepoPath()) if err != nil { - return err + return false, nil, err } - return nil + defer gitRepo.Close() + + branches, err := gitRepo.GetBranches() + if err != nil { + return false, nil, err + } + return true, branches, nil } // CompareDiff show different from one commit to another commit @@ -375,12 +522,6 @@ func CompareDiff(ctx *context.Context) { } defer headGitRepo.Close() - var err error - if err = parseBaseRepoInfo(ctx, headRepo); err != nil { - ctx.ServerError("parseBaseRepoInfo", err) - return - } - nothingToCompare := PrepareCompareDiff(ctx, headUser, headRepo, headGitRepo, compareInfo, baseBranch, headBranch) if ctx.Written() { return diff --git a/templates/repo/diff/compare.tmpl b/templates/repo/diff/compare.tmpl index 35a37ab76177..1859af68d0ff 100644 --- a/templates/repo/diff/compare.tmpl +++ b/templates/repo/diff/compare.tmpl @@ -26,11 +26,21 @@
{{range .Branches}} -
{{$.BaseName}}:{{.}}
+
{{$.BaseName}}:{{.}}
{{end}} - {{if .Repository.IsFork}} - {{range .BaseRepoBranches}} -
{{$.PullRequestCtx.BaseRepo.OwnerName}}:{{.}}
+ {{if not .PullRequestCtx.SameRepo}} + {{range .HeadBranches}} +
{{$.HeadUser.Name}}:{{.}}
+ {{end}} + {{end}} + {{if .OwnForkRepo}} + {{range .OwnForkRepoBranches}} +
{{$.OwnForkRepo.OwnerName}}:{{.}}
+ {{end}} + {{end}} + {{if .RootRepo}} + {{range .RootRepoBranches}} +
{{$.RootRepo.OwnerName}}:{{.}}
{{end}} {{end}}
@@ -49,7 +59,22 @@
{{range .HeadBranches}} -
{{$.HeadUser.Name}}:{{.}}
+
{{$.HeadUser.Name}}:{{.}}
+ {{end}} + {{if not .PullRequestCtx.SameRepo}} + {{range .Branches}} +
{{$.BaseName}}:{{.}}
+ {{end}} + {{end}} + {{if .OwnForkRepo}} + {{range .OwnForkRepoBranches}} +
{{$.OwnForkRepo.OwnerName}}:{{.}}
+ {{end}} + {{end}} + {{if .RootRepo}} + {{range .RootRepoBranches}} +
{{$.RootRepo.OwnerName}}:{{.}}
+ {{end}} {{end}}