Skip to content

Allow compare page to look up base, head, own-fork, forkbase-of-head #11327

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.

Sign up for GitHub

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

Merged
merged 17 commits into from
May 12, 2020
Merged

Allow compare page to look up base, head, own-fork, forkbase-of-head #11327

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
a1a63dc
Allow compare page to look up base, head, own-fork, forkbase-of-head
zeripath May 7, 2020
438ee22
as per @guillep2k
zeripath May 9, 2020
787ce45
Merge remote-tracking branch 'origin/master' into fix-7759-again
zeripath May 9, 2020
a15c5d9
Update routers/repo/compare.go
zeripath May 9, 2020
4237bff
Merge remote-tracking branch 'origin/master' into fix-7759-again
zeripath May 10, 2020
5cddd62
as per @guillep2k
zeripath May 10, 2020
8a36c25
Rationalise the names a little
zeripath May 10, 2020
0379878
Rationalise the names a little (2)
zeripath May 10, 2020
91e9cfc
Fix 500 with fork of fork
zeripath May 10, 2020
770fda4
Prevent 500 on compare different trees
zeripath May 10, 2020
f0c3b61
dotdotdot is perfectly valid in both usernames and repo names
zeripath May 10, 2020
354078c
ensure we can set the head and base repos too
zeripath May 10, 2020
5fa17a9
ensure we can set the head and base repos too (2)
zeripath May 10, 2020
e9741ea
fix lint
zeripath May 10, 2020
4d2347b
only set headRepo == baseRepo if isSameRepo
zeripath May 11, 2020
3e7c347
Merge branch 'master' into fix-7759-again
zeripath May 11, 2020
377989c
Merge branch 'master' into fix-7759-again
zeripath May 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions modules/git/commit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand Down
1 change: 0 additions & 1 deletion modules/git/repo_compare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
221 changes: 181 additions & 40 deletions routers/repo/compare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 branch>...[<head repo>:]<head branch>
// 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

Expand All @@ -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, "", ""
Expand Down Expand Up @@ -139,28 +186,92 @@ 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, "", ""
}
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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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
func getBranchesForRepo(user *models.User, repo *models.Repository) (bool, []string, error) {
perm, err := models.GetUserRepoPermission(repo, user)
if err != nil {
return false, nil, err
}
if err := repo.GetBaseRepo(); err != nil {
return err
if !perm.CanRead(models.UnitTypeCode) {
return false, nil, nil
}

baseGitRepo, err := git.OpenRepository(repo.BaseRepo.RepoPath())
gitRepo, err := git.OpenRepository(repo.RepoPath())
if err != nil {
return err
return false, nil, err
}
defer baseGitRepo.Close()
defer gitRepo.Close()

ctx.Data["BaseRepoBranches"], err = baseGitRepo.GetBranches()
branches, err := gitRepo.GetBranches()
if err != nil {
return err
return false, nil, err
}
return nil
return true, branches, nil
}

// CompareDiff show different from one commit to another commit
Expand All @@ -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
Expand Down
35 changes: 30 additions & 5 deletions templates/repo/diff/compare.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,21 @@
</div>
<div class="scrolling menu">
{{range .Branches}}
<div class="item {{if eq $.BaseBranch .}}selected{{end}}" data-url="{{$.RepoLink}}/compare/{{EscapePound .}}...{{if not $.PullRequestCtx.SameRepo}}{{$.HeadUser.Name}}:{{end}}{{EscapePound $.HeadBranch}}">{{$.BaseName}}:{{.}}</div>
<div class="item {{if eq $.BaseBranch .}}selected{{end}}" data-url="{{$.RepoLink}}/compare/{{EscapePound .}}...{{if not $.PullRequestCtx.SameRepo}}{{$.HeadUser.Name}}/{{$.HeadRepo.Name}}:{{end}}{{EscapePound $.HeadBranch}}">{{$.BaseName}}:{{.}}</div>
{{end}}
{{if .Repository.IsFork}}
{{range .BaseRepoBranches}}
<div class="item" data-url="{{$.PullRequestCtx.BaseRepo.Link}}/compare/{{EscapePound .}}...{{$.HeadUser.Name}}:{{EscapePound $.HeadBranch}}">{{$.PullRequestCtx.BaseRepo.OwnerName}}:{{.}}</div>
{{if not .PullRequestCtx.SameRepo}}
{{range .HeadBranches}}
<div class="item" data-url="{{$.HeadRepo.Link}}/compare/{{EscapePound .}}...{{$.HeadUser.Name}}/{{$.HeadRepo.Name}}:{{EscapePound $.HeadBranch}}">{{$.HeadUser.Name}}:{{.}}</div>
{{end}}
{{end}}
{{if .OwnForkRepo}}
{{range .OwnForkRepoBranches}}
<div class="item" data-url="{{$.OwnForkRepo.Link}}/compare/{{EscapePound .}}...{{$.HeadUser.Name}}/{{$.HeadRepo.Name}}:{{EscapePound $.HeadBranch}}">{{$.OwnForkRepo.OwnerName}}:{{.}}</div>
{{end}}
{{end}}
{{if .RootRepo}}
{{range .RootRepoBranches}}
<div class="item" data-url="{{$.RootRepo.Link}}/compare/{{EscapePound .}}...{{$.HeadUser.Name}}/{{$.HeadRepo.Name}}:{{EscapePound $.HeadBranch}}">{{$.RootRepo.OwnerName}}:{{.}}</div>
{{end}}
{{end}}
</div>
Expand All @@ -49,7 +59,22 @@
</div>
<div class="scrolling menu">
{{range .HeadBranches}}
<div class="{{if eq $.HeadBranch .}}selected{{end}} item" data-url="{{$.RepoLink}}/compare/{{EscapePound $.BaseBranch}}...{{if not $.PullRequestCtx.SameRepo}}{{$.HeadUser.Name}}:{{end}}{{EscapePound .}}">{{$.HeadUser.Name}}:{{.}}</div>
<div class="{{if eq $.HeadBranch .}}selected{{end}} item" data-url="{{$.RepoLink}}/compare/{{EscapePound $.BaseBranch}}...{{if not $.PullRequestCtx.SameRepo}}{{$.HeadUser.Name}}/{{$.HeadRepo.Name}}:{{end}}{{EscapePound .}}">{{$.HeadUser.Name}}:{{.}}</div>
{{end}}
{{if not .PullRequestCtx.SameRepo}}
{{range .Branches}}
<div class="item" data-url="{{$.RepoLink}}/compare/{{EscapePound $.BaseBranch}}...{{$.BaseName}}/{{$.Repository.Name}}:{{EscapePound .}}">{{$.BaseName}}:{{.}}</div>
{{end}}
{{end}}
{{if .OwnForkRepo}}
{{range .OwnForkRepoBranches}}
<div class="item" data-url="{{$.RepoLink}}/compare/{{EscapePound $.BaseBranch}}...{{$.OwnForkRepo.OwnerName}}/{{$.OwnForkRepo.Name}}:{{EscapePound .}}">{{$.OwnForkRepo.OwnerName}}:{{.}}</div>
{{end}}
{{end}}
{{if .RootRepo}}
{{range .RootRepoBranches}}
<div class="item" data-url="{{$.RepoLink}}/compare/{{EscapePound $.BaseBranch}}...{{$.RootRepo.OwnerName}}/{{$.RootRepo.Name}}:{{EscapePound .}}">{{$.RootRepo.OwnerName}}:{{.}}</div>
{{end}}
{{end}}
</div>
</div>
Expand Down