linden123/githaven-fork
1
1
Fork 0
forked from Shiloh/githaven
Code Pull Requests Activity

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

Browse Source 
* Allow compare page to look up base, head, own-fork, forkbase-of-head

Signed-off-by: Andrew Thornton <art27@cantab.net>

* as per @guillep2k

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Update routers/repo/compare.go

* as per @guillep2k

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Rationalise the names a little

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Rationalise the names a little (2)

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Fix 500 with fork of fork

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Prevent 500 on compare different trees

Signed-off-by: Andrew Thornton <art27@cantab.net>

* dotdotdot is perfectly valid in both usernames and repo names

Signed-off-by: Andrew Thornton <art27@cantab.net>

* ensure we can set the head and base repos too

Signed-off-by: Andrew Thornton <art27@cantab.net>

* ensure we can set the head and base repos too (2)

Signed-off-by: Andrew Thornton <art27@cantab.net>

* fix lint

Signed-off-by: Andrew Thornton <art27@cantab.net>

* only set headRepo == baseRepo if isSameRepo

Signed-off-by: Andrew Thornton <art27@cantab.net>
This commit is contained in:
zeripath 2020-05-12 06:52:46 +01:00 committed by GitHub
parent a4c7ad99e5
commit 0198bbedc1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 214 additions and 53 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
modules/git/commit.go
View File

@ -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. // GetFullCommitID returns full length (40) of commit ID by given short SHA in a repository.
func GetFullCommitID(repoPath, shortID string) (string, error) { func GetFullCommitID(repoPath, shortID string) (string, error) {
if len(shortID) >= 40 {
return shortID, nil
}
commitID, err := NewCommand("rev-parse", shortID).RunInDir(repoPath) commitID, err := NewCommand("rev-parse", shortID).RunInDir(repoPath)
if err != nil { if err != nil {
if strings.Contains(err.Error(), "exit status 128") { if strings.Contains(err.Error(), "exit status 128") {

1
modules/git/repo_compare.go
View File

@ -89,7 +89,6 @@ func (repo *Repository) GetCompareInfo(basePath, baseBranch, headBranch string)
return nil, err return nil, err
} }
compareInfo.NumFiles = len(strings.Split(stdout, "\n")) - 1 compareInfo.NumFiles = len(strings.Split(stdout, "\n")) - 1
return compareInfo, nil return compareInfo, nil
} }

227
routers/repo/compare.go
View File

@ -71,25 +71,45 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
baseRepo := ctx.Repo.Repository baseRepo := ctx.Repo.Repository
// Get compared branches information // 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> // format: <base branch>...[<head repo>:]<head branch>
// base<-head: master...head:feature // base<-head: master...head:feature
// same repo: master...feature // same repo: master...feature
var ( var (
headUser *models.User headUser *models.User
headRepo *models.Repository
headBranch string headBranch string
isSameRepo bool isSameRepo bool
infoPath string infoPath string
err error err error
) )
infoPath = ctx.Params("*") infoPath = ctx.Params("*")
infos := strings.Split(infoPath, "...") infos := strings.SplitN(infoPath, "...", 2)
if len(infos) != 2 { if len(infos) != 2 {
log.Trace("ParseCompareInfo[%d]: not enough compared branches information %s", baseRepo.ID, infos) log.Trace("ParseCompareInfo[%d]: not enough compared branches information %s", baseRepo.ID, infos)
ctx.NotFound("CompareAndPullRequest", nil) ctx.NotFound("CompareAndPullRequest", nil)
return nil, nil, nil, nil, "", "" return nil, nil, nil, nil, "", ""
} }
ctx.Data["BaseName"] = baseRepo.OwnerName
baseBranch := infos[0] baseBranch := infos[0]
ctx.Data["BaseBranch"] = baseBranch ctx.Data["BaseBranch"] = baseBranch
@ -101,17 +121,44 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
headBranch = headInfos[0] headBranch = headInfos[0]
} else if len(headInfos) == 2 { } else if len(headInfos) == 2 {
headUser, err = models.GetUserByName(headInfos[0]) headInfosSplit := strings.Split(headInfos[0], "/")
if err != nil { if len(headInfosSplit) == 1 {
if models.IsErrUserNotExist(err) { headUser, err = models.GetUserByName(headInfos[0])
ctx.NotFound("GetUserByName", nil) if err != nil {
} else { if models.IsErrUserNotExist(err) {
ctx.ServerError("GetUserByName", 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 { } else {
ctx.NotFound("CompareAndPullRequest", nil) ctx.NotFound("CompareAndPullRequest", nil)
return nil, nil, nil, 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["BaseIsBranch"] = baseIsBranch
ctx.Data["BaseIsTag"] = baseIsTag ctx.Data["BaseIsTag"] = baseIsTag
// Check if current user has fork of repository or in the same repository. // Now we have the repository that represents the base
headRepo, has := models.HasForkedRepo(headUser.ID, baseRepo.ID)
if !has && !isSameRepo { // 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 ctx.Data["PageIsComparePull"] = false
} }
// 8. Finally open the git repo
var headGitRepo *git.Repository var headGitRepo *git.Repository
if isSameRepo { if isSameRepo {
headRepo = ctx.Repo.Repository headRepo = ctx.Repo.Repository
headGitRepo = ctx.Repo.GitRepo headGitRepo = ctx.Repo.GitRepo
ctx.Data["BaseName"] = headUser.Name } else if has {
} else { headGitRepo, err = git.OpenRepository(headRepo.RepoPath())
headGitRepo, err = git.OpenRepository(models.RepoPath(headUser.Name, headRepo.Name))
ctx.Data["BaseName"] = baseRepo.OwnerName
if err != nil { if err != nil {
ctx.ServerError("OpenRepository", err) ctx.ServerError("OpenRepository", err)
return nil, nil, nil, nil, "", "" return nil, nil, nil, nil, "", ""
@ -160,7 +267,11 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
defer headGitRepo.Close() 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) permBase, err := models.GetUserRepoPermission(baseRepo, ctx.User)
if err != nil { if err != nil {
ctx.ServerError("GetUserRepoPermission", err) ctx.ServerError("GetUserRepoPermission", err)
@ -177,8 +288,9 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
return nil, nil, nil, nil, "", "" return nil, nil, nil, nil, "", ""
} }
// If we're not merging from the same repo:
if !isSameRepo { 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) permHead, err := models.GetUserRepoPermission(headRepo, ctx.User)
if err != nil { if err != nil {
ctx.ServerError("GetUserRepoPermission", err) 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. // Check if head branch is valid.
headIsCommit := headGitRepo.IsCommitExist(headBranch) headIsCommit := headGitRepo.IsCommitExist(headBranch)
headIsBranch := headGitRepo.IsBranchExist(headBranch) headIsBranch := headGitRepo.IsBranchExist(headBranch)
@ -343,28 +493,25 @@ func PrepareCompareDiff(
return false return false
} }
// parseBaseRepoInfo parse base repository if current repo is forked. func getBranchesForRepo(user *models.User, repo *models.Repository) (bool, []string, error) {
// The "base" here means the repository where current repo forks from, perm, err := models.GetUserRepoPermission(repo, user)
// 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())
if err != nil { if err != nil {
return err return false, nil, err
} }
defer baseGitRepo.Close() if !perm.CanRead(models.UnitTypeCode) {
return false, nil, nil
ctx.Data["BaseRepoBranches"], err = baseGitRepo.GetBranches() }
gitRepo, err := git.OpenRepository(repo.RepoPath())
if err != nil { 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 // CompareDiff show different from one commit to another commit
@ -375,12 +522,6 @@ func CompareDiff(ctx *context.Context) {
} }
defer headGitRepo.Close() 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) nothingToCompare := PrepareCompareDiff(ctx, headUser, headRepo, headGitRepo, compareInfo, baseBranch, headBranch)
if ctx.Written() { if ctx.Written() {
return return

35
templates/repo/diff/compare.tmpl
View File

@ -26,11 +26,21 @@
</div> </div>
<div class="scrolling menu"> <div class="scrolling menu">
{{range .Branches}} {{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}} {{end}}
{{if .Repository.IsFork}} {{if not .PullRequestCtx.SameRepo}}
{{range .BaseRepoBranches}} {{range .HeadBranches}}
<div class="item" data-url="{{$.PullRequestCtx.BaseRepo.Link}}/compare/{{EscapePound .}}...{{$.HeadUser.Name}}:{{EscapePound $.HeadBranch}}">{{$.PullRequestCtx.BaseRepo.OwnerName}}:{{.}}</div> <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}}
{{end}} {{end}}
</div> </div>
@ -49,7 +59,22 @@
</div> </div>
<div class="scrolling menu"> <div class="scrolling menu">
{{range .HeadBranches}} {{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}} {{end}}
</div> </div>
</div> </div>