Skip to content

Commit 0198bbe

Browse files
zeripathzeripath
authored
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 <[email protected]> * as per @guillep2k Signed-off-by: Andrew Thornton <[email protected]> * Update routers/repo/compare.go * as per @guillep2k Signed-off-by: Andrew Thornton <[email protected]> * Rationalise the names a little Signed-off-by: Andrew Thornton <[email protected]> * Rationalise the names a little (2) Signed-off-by: Andrew Thornton <[email protected]> * Fix 500 with fork of fork Signed-off-by: Andrew Thornton <[email protected]> * Prevent 500 on compare different trees Signed-off-by: Andrew Thornton <[email protected]> * dotdotdot is perfectly valid in both usernames and repo names Signed-off-by: Andrew Thornton <[email protected]> * ensure we can set the head and base repos too Signed-off-by: Andrew Thornton <[email protected]> * ensure we can set the head and base repos too (2) Signed-off-by: Andrew Thornton <[email protected]> * fix lint Signed-off-by: Andrew Thornton <[email protected]> * only set headRepo == baseRepo if isSameRepo Signed-off-by: Andrew Thornton <[email protected]>
1 parent a4c7ad9 commit 0198bbe

File tree

4 files changed

+211
-50
lines changed

4 files changed

+211
-50
lines changed

‎modules/git/commit.go

-4
Original file line numberDiff line numberDiff line change
@@ -529,10 +529,6 @@ func GetCommitFileStatus(repoPath, commitID string) (*CommitFileStatus, error) {
529529

530530
// GetFullCommitID returns full length (40) of commit ID by given short SHA in a repository.
531531
func GetFullCommitID(repoPath, shortID string) (string, error) {
532-
if len(shortID) >= 40 {
533-
return shortID, nil
534-
}
535-
536532
commitID, err := NewCommand("rev-parse", shortID).RunInDir(repoPath)
537533
if err != nil {
538534
if strings.Contains(err.Error(), "exit status 128") {

‎modules/git/repo_compare.go

-1
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,6 @@ func (repo *Repository) GetCompareInfo(basePath, baseBranch, headBranch string)
8989
return nil, err
9090
}
9191
compareInfo.NumFiles = len(strings.Split(stdout, "\n")) - 1
92-
9392
return compareInfo, nil
9493
}
9594

‎routers/repo/compare.go

+181-40
Original file line numberDiff line numberDiff line change
@@ -71,25 +71,45 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
7171
baseRepo := ctx.Repo.Repository
7272

7373
// Get compared branches information
74+
// A full compare url is of the form:
75+
//
76+
// 1. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headBranch}
77+
// 2. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}:{:headBranch}
78+
// 3. /{:baseOwner}/{:baseRepoName}/compare/{:baseBranch}...{:headOwner}/{:headRepoName}:{:headBranch}
79+
//
80+
// Here we obtain the infoPath "{:baseBranch}...[{:headOwner}/{:headRepoName}:]{:headBranch}" as ctx.Params("*")
81+
// with the :baseRepo in ctx.Repo.
82+
//
83+
// Note: Generally :headRepoName is not provided here - we are only passed :headOwner.
84+
//
85+
// How do we determine the :headRepo?
86+
//
87+
// 1. If :headOwner is not set then the :headRepo = :baseRepo
88+
// 2. If :headOwner is set - then look for the fork of :baseRepo owned by :headOwner
89+
// 3. But... :baseRepo could be a fork of :headOwner's repo - so check that
90+
// 4. Now, :baseRepo and :headRepos could be forks of the same repo - so check that
91+
//
7492
// format: <base branch>...[<head repo>:]<head branch>
7593
// base<-head: master...head:feature
7694
// same repo: master...feature
7795

7896
var (
7997
headUser *models.User
98+
headRepo *models.Repository
8099
headBranch string
81100
isSameRepo bool
82101
infoPath string
83102
err error
84103
)
85104
infoPath = ctx.Params("*")
86-
infos := strings.Split(infoPath, "...")
105+
infos := strings.SplitN(infoPath, "...", 2)
87106
if len(infos) != 2 {
88107
log.Trace("ParseCompareInfo[%d]: not enough compared branches information %s", baseRepo.ID, infos)
89108
ctx.NotFound("CompareAndPullRequest", nil)
90109
return nil, nil, nil, nil, "", ""
91110
}
92111

112+
ctx.Data["BaseName"] = baseRepo.OwnerName
93113
baseBranch := infos[0]
94114
ctx.Data["BaseBranch"] = baseBranch
95115

@@ -101,17 +121,44 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
101121
headBranch = headInfos[0]
102122

103123
} else if len(headInfos) == 2 {
104-
headUser, err = models.GetUserByName(headInfos[0])
105-
if err != nil {
106-
if models.IsErrUserNotExist(err) {
107-
ctx.NotFound("GetUserByName", nil)
108-
} else {
109-
ctx.ServerError("GetUserByName", err)
124+
headInfosSplit := strings.Split(headInfos[0], "/")
125+
if len(headInfosSplit) == 1 {
126+
headUser, err = models.GetUserByName(headInfos[0])
127+
if err != nil {
128+
if models.IsErrUserNotExist(err) {
129+
ctx.NotFound("GetUserByName", nil)
130+
} else {
131+
ctx.ServerError("GetUserByName", err)
132+
}
133+
return nil, nil, nil, nil, "", ""
110134
}
111-
return nil, nil, nil, nil, "", ""
135+
headBranch = headInfos[1]
136+
isSameRepo = headUser.ID == ctx.Repo.Owner.ID
137+
if isSameRepo {
138+
headRepo = baseRepo
139+
}
140+
} else {
141+
headRepo, err = models.GetRepositoryByOwnerAndName(headInfosSplit[0], headInfosSplit[1])
142+
if err != nil {
143+
if models.IsErrRepoNotExist(err) {
144+
ctx.NotFound("GetRepositoryByOwnerAndName", nil)
145+
} else {
146+
ctx.ServerError("GetRepositoryByOwnerAndName", err)
147+
}
148+
return nil, nil, nil, nil, "", ""
149+
}
150+
if err := headRepo.GetOwner(); err != nil {
151+
if models.IsErrUserNotExist(err) {
152+
ctx.NotFound("GetUserByName", nil)
153+
} else {
154+
ctx.ServerError("GetUserByName", err)
155+
}
156+
return nil, nil, nil, nil, "", ""
157+
}
158+
headBranch = headInfos[1]
159+
headUser = headRepo.Owner
160+
isSameRepo = headRepo.ID == ctx.Repo.Repository.ID
112161
}
113-
headBranch = headInfos[1]
114-
isSameRepo = headUser.ID == ctx.Repo.Owner.ID
115162
} else {
116163
ctx.NotFound("CompareAndPullRequest", nil)
117164
return nil, nil, nil, nil, "", ""
@@ -139,28 +186,92 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
139186
ctx.Data["BaseIsBranch"] = baseIsBranch
140187
ctx.Data["BaseIsTag"] = baseIsTag
141188

142-
// Check if current user has fork of repository or in the same repository.
143-
headRepo, has := models.HasForkedRepo(headUser.ID, baseRepo.ID)
144-
if !has && !isSameRepo {
189+
// Now we have the repository that represents the base
190+
191+
// The current base and head repositories and branches may not
192+
// actually be the intended branches that the user wants to
193+
// create a pull-request from - but also determining the head
194+
// repo is difficult.
195+
196+
// We will want therefore to offer a few repositories to set as
197+
// our base and head
198+
199+
// 1. First if the baseRepo is a fork get the "RootRepo" it was
200+
// forked from
201+
var rootRepo *models.Repository
202+
if baseRepo.IsFork {
203+
err = baseRepo.GetBaseRepo()
204+
if err != nil {
205+
if !models.IsErrRepoNotExist(err) {
206+
ctx.ServerError("Unable to find root repo", err)
207+
return nil, nil, nil, nil, "", ""
208+
}
209+
} else {
210+
rootRepo = baseRepo.BaseRepo
211+
}
212+
}
213+
214+
// 2. Now if the current user is not the owner of the baseRepo,
215+
// check if they have a fork of the base repo and offer that as
216+
// "OwnForkRepo"
217+
var ownForkRepo *models.Repository
218+
if baseRepo.OwnerID != ctx.User.ID {
219+
repo, has := models.HasForkedRepo(ctx.User.ID, baseRepo.ID)
220+
if has {
221+
ownForkRepo = repo
222+
ctx.Data["OwnForkRepo"] = ownForkRepo
223+
}
224+
}
225+
226+
has := headRepo != nil
227+
// 3. If the base is a forked from "RootRepo" and the owner of
228+
// the "RootRepo" is the :headUser - set headRepo to that
229+
if !has && rootRepo != nil && rootRepo.OwnerID == headUser.ID {
230+
headRepo = rootRepo
231+
has = true
232+
}
233+
234+
// 4. If the ctx.User has their own fork of the baseRepo and the headUser is the ctx.User
235+
// set the headRepo to the ownFork
236+
if !has && ownForkRepo != nil && ownForkRepo.OwnerID == headUser.ID {
237+
headRepo = ownForkRepo
238+
has = true
239+
}
240+
241+
// 5. If the headOwner has a fork of the baseRepo - use that
242+
if !has {
243+
headRepo, has = models.HasForkedRepo(headUser.ID, baseRepo.ID)
244+
}
245+
246+
// 6. If the baseRepo is a fork and the headUser has a fork of that use that
247+
if !has && baseRepo.IsFork {
248+
headRepo, has = models.HasForkedRepo(headUser.ID, baseRepo.ForkID)
249+
}
250+
251+
// 7. Otherwise if we're not the same repo and haven't found a repo give up
252+
if !isSameRepo && !has {
145253
ctx.Data["PageIsComparePull"] = false
146254
}
147255

256+
// 8. Finally open the git repo
148257
var headGitRepo *git.Repository
149258
if isSameRepo {
150259
headRepo = ctx.Repo.Repository
151260
headGitRepo = ctx.Repo.GitRepo
152-
ctx.Data["BaseName"] = headUser.Name
153-
} else {
154-
headGitRepo, err = git.OpenRepository(models.RepoPath(headUser.Name, headRepo.Name))
155-
ctx.Data["BaseName"] = baseRepo.OwnerName
261+
} else if has {
262+
headGitRepo, err = git.OpenRepository(headRepo.RepoPath())
156263
if err != nil {
157264
ctx.ServerError("OpenRepository", err)
158265
return nil, nil, nil, nil, "", ""
159266
}
160267
defer headGitRepo.Close()
161268
}
162269

163-
// user should have permission to read baseRepo's codes and pulls, NOT headRepo's
270+
ctx.Data["HeadRepo"] = headRepo
271+
272+
// Now we need to assert that the ctx.User has permission to read
273+
// the baseRepo's code and pulls
274+
// (NOT headRepo's)
164275
permBase, err := models.GetUserRepoPermission(baseRepo, ctx.User)
165276
if err != nil {
166277
ctx.ServerError("GetUserRepoPermission", err)
@@ -177,8 +288,9 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
177288
return nil, nil, nil, nil, "", ""
178289
}
179290

291+
// If we're not merging from the same repo:
180292
if !isSameRepo {
181-
// user should have permission to read headrepo's codes
293+
// Assert ctx.User has permission to read headRepo's codes
182294
permHead, err := models.GetUserRepoPermission(headRepo, ctx.User)
183295
if err != nil {
184296
ctx.ServerError("GetUserRepoPermission", err)
@@ -196,6 +308,44 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
196308
}
197309
}
198310

311+
// If we have a rootRepo and it's different from:
312+
// 1. the computed base
313+
// 2. the computed head
314+
// then get the branches of it
315+
if rootRepo != nil &&
316+
rootRepo.ID != headRepo.ID &&
317+
rootRepo.ID != baseRepo.ID {
318+
perm, branches, err := getBranchesForRepo(ctx.User, rootRepo)
319+
if err != nil {
320+
ctx.ServerError("GetBranchesForRepo", err)
321+
return nil, nil, nil, nil, "", ""
322+
}
323+
if perm {
324+
ctx.Data["RootRepo"] = rootRepo
325+
ctx.Data["RootRepoBranches"] = branches
326+
}
327+
}
328+
329+
// If we have a ownForkRepo and it's different from:
330+
// 1. The computed base
331+
// 2. The computed hea
332+
// 3. The rootRepo (if we have one)
333+
// then get the branches from it.
334+
if ownForkRepo != nil &&
335+
ownForkRepo.ID != headRepo.ID &&
336+
ownForkRepo.ID != baseRepo.ID &&
337+
(rootRepo == nil || ownForkRepo.ID != rootRepo.ID) {
338+
perm, branches, err := getBranchesForRepo(ctx.User, ownForkRepo)
339+
if err != nil {
340+
ctx.ServerError("GetBranchesForRepo", err)
341+
return nil, nil, nil, nil, "", ""
342+
}
343+
if perm {
344+
ctx.Data["OwnForkRepo"] = ownForkRepo
345+
ctx.Data["OwnForkRepoBranches"] = branches
346+
}
347+
}
348+
199349
// Check if head branch is valid.
200350
headIsCommit := headGitRepo.IsCommitExist(headBranch)
201351
headIsBranch := headGitRepo.IsBranchExist(headBranch)
@@ -343,28 +493,25 @@ func PrepareCompareDiff(
343493
return false
344494
}
345495

346-
// parseBaseRepoInfo parse base repository if current repo is forked.
347-
// The "base" here means the repository where current repo forks from,
348-
// not the repository fetch from current URL.
349-
func parseBaseRepoInfo(ctx *context.Context, repo *models.Repository) error {
350-
if !repo.IsFork {
351-
return nil
496+
func getBranchesForRepo(user *models.User, repo *models.Repository) (bool, []string, error) {
497+
perm, err := models.GetUserRepoPermission(repo, user)
498+
if err != nil {
499+
return false, nil, err
352500
}
353-
if err := repo.GetBaseRepo(); err != nil {
354-
return err
501+
if !perm.CanRead(models.UnitTypeCode) {
502+
return false, nil, nil
355503
}
356-
357-
baseGitRepo, err := git.OpenRepository(repo.BaseRepo.RepoPath())
504+
gitRepo, err := git.OpenRepository(repo.RepoPath())
358505
if err != nil {
359-
return err
506+
return false, nil, err
360507
}
361-
defer baseGitRepo.Close()
508+
defer gitRepo.Close()
362509

363-
ctx.Data["BaseRepoBranches"], err = baseGitRepo.GetBranches()
510+
branches, err := gitRepo.GetBranches()
364511
if err != nil {
365-
return err
512+
return false, nil, err
366513
}
367-
return nil
514+
return true, branches, nil
368515
}
369516

370517
// CompareDiff show different from one commit to another commit
@@ -375,12 +522,6 @@ func CompareDiff(ctx *context.Context) {
375522
}
376523
defer headGitRepo.Close()
377524

378-
var err error
379-
if err = parseBaseRepoInfo(ctx, headRepo); err != nil {
380-
ctx.ServerError("parseBaseRepoInfo", err)
381-
return
382-
}
383-
384525
nothingToCompare := PrepareCompareDiff(ctx, headUser, headRepo, headGitRepo, compareInfo, baseBranch, headBranch)
385526
if ctx.Written() {
386527
return

‎templates/repo/diff/compare.tmpl

+30-5
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,21 @@
2626
</div>
2727
<div class="scrolling menu">
2828
{{range .Branches}}
29-
<div class="item {{if eq $.BaseBranch .}}selected{{end}}" data-url="{{$.RepoLink}}/compare/{{EscapePound .}}...{{if not $.PullRequestCtx.SameRepo}}{{$.HeadUser.Name}}:{{end}}{{EscapePound $.HeadBranch}}">{{$.BaseName}}:{{.}}</div>
29+
<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>
3030
{{end}}
31-
{{if .Repository.IsFork}}
32-
{{range .BaseRepoBranches}}
33-
<div class="item" data-url="{{$.PullRequestCtx.BaseRepo.Link}}/compare/{{EscapePound .}}...{{$.HeadUser.Name}}:{{EscapePound $.HeadBranch}}">{{$.PullRequestCtx.BaseRepo.OwnerName}}:{{.}}</div>
31+
{{if not .PullRequestCtx.SameRepo}}
32+
{{range .HeadBranches}}
33+
<div class="item" data-url="{{$.HeadRepo.Link}}/compare/{{EscapePound .}}...{{$.HeadUser.Name}}/{{$.HeadRepo.Name}}:{{EscapePound $.HeadBranch}}">{{$.HeadUser.Name}}:{{.}}</div>
34+
{{end}}
35+
{{end}}
36+
{{if .OwnForkRepo}}
37+
{{range .OwnForkRepoBranches}}
38+
<div class="item" data-url="{{$.OwnForkRepo.Link}}/compare/{{EscapePound .}}...{{$.HeadUser.Name}}/{{$.HeadRepo.Name}}:{{EscapePound $.HeadBranch}}">{{$.OwnForkRepo.OwnerName}}:{{.}}</div>
39+
{{end}}
40+
{{end}}
41+
{{if .RootRepo}}
42+
{{range .RootRepoBranches}}
43+
<div class="item" data-url="{{$.RootRepo.Link}}/compare/{{EscapePound .}}...{{$.HeadUser.Name}}/{{$.HeadRepo.Name}}:{{EscapePound $.HeadBranch}}">{{$.RootRepo.OwnerName}}:{{.}}</div>
3444
{{end}}
3545
{{end}}
3646
</div>
@@ -49,7 +59,22 @@
4959
</div>
5060
<div class="scrolling menu">
5161
{{range .HeadBranches}}
52-
<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>
62+
<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>
63+
{{end}}
64+
{{if not .PullRequestCtx.SameRepo}}
65+
{{range .Branches}}
66+
<div class="item" data-url="{{$.RepoLink}}/compare/{{EscapePound $.BaseBranch}}...{{$.BaseName}}/{{$.Repository.Name}}:{{EscapePound .}}">{{$.BaseName}}:{{.}}</div>
67+
{{end}}
68+
{{end}}
69+
{{if .OwnForkRepo}}
70+
{{range .OwnForkRepoBranches}}
71+
<div class="item" data-url="{{$.RepoLink}}/compare/{{EscapePound $.BaseBranch}}...{{$.OwnForkRepo.OwnerName}}/{{$.OwnForkRepo.Name}}:{{EscapePound .}}">{{$.OwnForkRepo.OwnerName}}:{{.}}</div>
72+
{{end}}
73+
{{end}}
74+
{{if .RootRepo}}
75+
{{range .RootRepoBranches}}
76+
<div class="item" data-url="{{$.RepoLink}}/compare/{{EscapePound $.BaseBranch}}...{{$.RootRepo.OwnerName}}/{{$.RootRepo.Name}}:{{EscapePound .}}">{{$.RootRepo.OwnerName}}:{{.}}</div>
77+
{{end}}
5378
{{end}}
5479
</div>
5580
</div>

0 commit comments

Comments
 (0)