Skip to content

Commit 5abe1c5

Browse files
lunnyzeripath
lunny
and
zeripath
authored
Display pull request head branch even the branch deleted or repository deleted (#10413)
* Display pull request head branch even the branch deleted or repository deleted * Merge getHeadRepo/loadHeadRepo and getBaseRepo/loadBaseRepo on pull and fill repo when pr.Issue.Repo is available * retrieve sha from pull head when pull request branch deleted and fix tests * Fix test * Ensure MustHeadRepoName returns empty string if no head repo Co-authored-by: zeripath <[email protected]>
1 parent 22b7507 commit 5abe1c5

File tree

12 files changed

+184
-180
lines changed

12 files changed

+184
-180
lines changed

‎models/pull.go

+50-54
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,8 @@ type PullRequest struct {
6262
MergerID int64 `xorm:"INDEX"`
6363
Merger *User `xorm:"-"`
6464
MergedUnix timeutil.TimeStamp `xorm:"updated INDEX"`
65+
66+
isHeadRepoLoaded bool `xorm:"-"`
6567
}
6668

6769
// MustHeadUserName returns the HeadRepo's username if failed return blank
@@ -74,6 +76,9 @@ func (pr *PullRequest) MustHeadUserName() string {
7476
}
7577
return ""
7678
}
79+
if pr.HeadRepo == nil {
80+
return ""
81+
}
7782
return pr.HeadRepo.OwnerName
7883
}
7984

@@ -97,38 +102,55 @@ func (pr *PullRequest) LoadAttributes() error {
97102
return pr.loadAttributes(x)
98103
}
99104

100-
// LoadBaseRepo loads pull request base repository from database
101-
func (pr *PullRequest) LoadBaseRepo() error {
102-
if pr.BaseRepo == nil {
103-
if pr.HeadRepoID == pr.BaseRepoID && pr.HeadRepo != nil {
104-
pr.BaseRepo = pr.HeadRepo
105-
return nil
105+
func (pr *PullRequest) loadHeadRepo(e Engine) (err error) {
106+
if !pr.isHeadRepoLoaded && pr.HeadRepo == nil && pr.HeadRepoID > 0 {
107+
if pr.HeadRepoID == pr.BaseRepoID {
108+
if pr.BaseRepo != nil {
109+
pr.HeadRepo = pr.BaseRepo
110+
return nil
111+
} else if pr.Issue != nil && pr.Issue.Repo != nil {
112+
pr.HeadRepo = pr.Issue.Repo
113+
return nil
114+
}
106115
}
107-
var repo Repository
108-
if has, err := x.ID(pr.BaseRepoID).Get(&repo); err != nil {
109-
return err
110-
} else if !has {
111-
return ErrRepoNotExist{ID: pr.BaseRepoID}
116+
117+
pr.HeadRepo, err = getRepositoryByID(e, pr.HeadRepoID)
118+
if err != nil && !IsErrRepoNotExist(err) { // Head repo maybe deleted, but it should still work
119+
return fmt.Errorf("getRepositoryByID(head): %v", err)
112120
}
113-
pr.BaseRepo = &repo
121+
pr.isHeadRepoLoaded = true
114122
}
115123
return nil
116124
}
117125

118-
// LoadHeadRepo loads pull request head repository from database
126+
// LoadHeadRepo loads the head repository
119127
func (pr *PullRequest) LoadHeadRepo() error {
120-
if pr.HeadRepo == nil {
121-
if pr.HeadRepoID == pr.BaseRepoID && pr.BaseRepo != nil {
122-
pr.HeadRepo = pr.BaseRepo
123-
return nil
124-
}
125-
var repo Repository
126-
if has, err := x.ID(pr.HeadRepoID).Get(&repo); err != nil {
127-
return err
128-
} else if !has {
129-
return ErrRepoNotExist{ID: pr.HeadRepoID}
130-
}
131-
pr.HeadRepo = &repo
128+
return pr.loadHeadRepo(x)
129+
}
130+
131+
// LoadBaseRepo loads the target repository
132+
func (pr *PullRequest) LoadBaseRepo() error {
133+
return pr.loadBaseRepo(x)
134+
}
135+
136+
func (pr *PullRequest) loadBaseRepo(e Engine) (err error) {
137+
if pr.BaseRepo != nil {
138+
return nil
139+
}
140+
141+
if pr.HeadRepoID == pr.BaseRepoID && pr.HeadRepo != nil {
142+
pr.BaseRepo = pr.HeadRepo
143+
return nil
144+
}
145+
146+
if pr.Issue != nil && pr.Issue.Repo != nil {
147+
pr.BaseRepo = pr.Issue.Repo
148+
return nil
149+
}
150+
151+
pr.BaseRepo, err = getRepositoryByID(e, pr.BaseRepoID)
152+
if err != nil {
153+
return fmt.Errorf("GetRepositoryByID(base): %v", err)
132154
}
133155
return nil
134156
}
@@ -414,32 +436,6 @@ func (pr *PullRequest) GetGitRefName() string {
414436
return fmt.Sprintf("refs/pull/%d/head", pr.Index)
415437
}
416438

417-
func (pr *PullRequest) getHeadRepo(e Engine) (err error) {
418-
pr.HeadRepo, err = getRepositoryByID(e, pr.HeadRepoID)
419-
if err != nil && !IsErrRepoNotExist(err) {
420-
return fmt.Errorf("getRepositoryByID(head): %v", err)
421-
}
422-
return nil
423-
}
424-
425-
// GetHeadRepo loads the head repository
426-
func (pr *PullRequest) GetHeadRepo() error {
427-
return pr.getHeadRepo(x)
428-
}
429-
430-
// GetBaseRepo loads the target repository
431-
func (pr *PullRequest) GetBaseRepo() (err error) {
432-
if pr.BaseRepo != nil {
433-
return nil
434-
}
435-
436-
pr.BaseRepo, err = GetRepositoryByID(pr.BaseRepoID)
437-
if err != nil {
438-
return fmt.Errorf("GetRepositoryByID(base): %v", err)
439-
}
440-
return nil
441-
}
442-
443439
// IsChecking returns true if this pull request is still checking conflict.
444440
func (pr *PullRequest) IsChecking() bool {
445441
return pr.Status == PullRequestStatusChecking
@@ -452,7 +448,7 @@ func (pr *PullRequest) CanAutoMerge() bool {
452448

453449
// GetLastCommitStatus returns the last commit status for this pull request.
454450
func (pr *PullRequest) GetLastCommitStatus() (status *CommitStatus, err error) {
455-
if err = pr.GetHeadRepo(); err != nil {
451+
if err = pr.LoadHeadRepo(); err != nil {
456452
return nil, err
457453
}
458454

@@ -774,7 +770,7 @@ func (pr *PullRequest) GetWorkInProgressPrefix() string {
774770
// IsHeadEqualWithBranch returns if the commits of branchName are available in pull request head
775771
func (pr *PullRequest) IsHeadEqualWithBranch(branchName string) (bool, error) {
776772
var err error
777-
if err = pr.GetBaseRepo(); err != nil {
773+
if err = pr.LoadBaseRepo(); err != nil {
778774
return false, err
779775
}
780776
baseGitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
@@ -786,7 +782,7 @@ func (pr *PullRequest) IsHeadEqualWithBranch(branchName string) (bool, error) {
786782
return false, err
787783
}
788784

789-
if err = pr.GetHeadRepo(); err != nil {
785+
if err = pr.LoadHeadRepo(); err != nil {
790786
return false, err
791787
}
792788
headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath())

‎models/pull_sign.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import (
1212

1313
// SignMerge determines if we should sign a PR merge commit to the base repository
1414
func (pr *PullRequest) SignMerge(u *User, tmpBasePath, baseCommit, headCommit string) (bool, string, error) {
15-
if err := pr.GetBaseRepo(); err != nil {
15+
if err := pr.LoadBaseRepo(); err != nil {
1616
log.Error("Unable to get Base Repo for pull request")
1717
return false, "", err
1818
}

‎models/pull_test.go

+5-5
Original file line numberDiff line numberDiff line change
@@ -29,21 +29,21 @@ func TestPullRequest_LoadIssue(t *testing.T) {
2929
assert.Equal(t, int64(2), pr.Issue.ID)
3030
}
3131

32-
func TestPullRequest_GetBaseRepo(t *testing.T) {
32+
func TestPullRequest_LoadBaseRepo(t *testing.T) {
3333
assert.NoError(t, PrepareTestDatabase())
3434
pr := AssertExistsAndLoadBean(t, &PullRequest{ID: 1}).(*PullRequest)
35-
assert.NoError(t, pr.GetBaseRepo())
35+
assert.NoError(t, pr.LoadBaseRepo())
3636
assert.NotNil(t, pr.BaseRepo)
3737
assert.Equal(t, pr.BaseRepoID, pr.BaseRepo.ID)
38-
assert.NoError(t, pr.GetBaseRepo())
38+
assert.NoError(t, pr.LoadBaseRepo())
3939
assert.NotNil(t, pr.BaseRepo)
4040
assert.Equal(t, pr.BaseRepoID, pr.BaseRepo.ID)
4141
}
4242

43-
func TestPullRequest_GetHeadRepo(t *testing.T) {
43+
func TestPullRequest_LoadHeadRepo(t *testing.T) {
4444
assert.NoError(t, PrepareTestDatabase())
4545
pr := AssertExistsAndLoadBean(t, &PullRequest{ID: 1}).(*PullRequest)
46-
assert.NoError(t, pr.GetHeadRepo())
46+
assert.NoError(t, pr.LoadHeadRepo())
4747
assert.NotNil(t, pr.HeadRepo)
4848
assert.Equal(t, pr.HeadRepoID, pr.HeadRepo.ID)
4949
}

‎modules/convert/pull.go

+59-62
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest {
2222
baseBranch *git.Branch
2323
headBranch *git.Branch
2424
baseCommit *git.Commit
25-
headCommit *git.Commit
2625
err error
2726
)
2827

@@ -32,20 +31,14 @@ func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest {
3231
}
3332

3433
apiIssue := ToAPIIssue(pr.Issue)
35-
if pr.BaseRepo == nil {
36-
pr.BaseRepo, err = models.GetRepositoryByID(pr.BaseRepoID)
37-
if err != nil {
38-
log.Error("GetRepositoryById[%d]: %v", pr.ID, err)
39-
return nil
40-
}
34+
if err := pr.LoadBaseRepo(); err != nil {
35+
log.Error("GetRepositoryById[%d]: %v", pr.ID, err)
36+
return nil
4137
}
42-
if pr.HeadRepoID != 0 && pr.HeadRepo == nil {
43-
pr.HeadRepo, err = models.GetRepositoryByID(pr.HeadRepoID)
44-
if err != nil && !models.IsErrRepoNotExist(err) {
45-
log.Error("GetRepositoryById[%d]: %v", pr.ID, err)
46-
return nil
4738

48-
}
39+
if err := pr.LoadHeadRepo(); err != nil {
40+
log.Error("GetRepositoryById[%d]: %v", pr.ID, err)
41+
return nil
4942
}
5043

5144
apiPullRequest := &api.PullRequest{
@@ -69,70 +62,74 @@ func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest {
6962
Deadline: apiIssue.Deadline,
7063
Created: pr.Issue.CreatedUnix.AsTimePtr(),
7164
Updated: pr.Issue.UpdatedUnix.AsTimePtr(),
72-
}
73-
baseBranch, err = repo_module.GetBranch(pr.BaseRepo, pr.BaseBranch)
74-
if err != nil {
75-
if git.IsErrBranchNotExist(err) {
76-
apiPullRequest.Base = nil
77-
} else {
78-
log.Error("GetBranch[%s]: %v", pr.BaseBranch, err)
79-
return nil
80-
}
81-
} else {
82-
apiBaseBranchInfo := &api.PRBranchInfo{
65+
66+
Base: &api.PRBranchInfo{
8367
Name: pr.BaseBranch,
8468
Ref: pr.BaseBranch,
8569
RepoID: pr.BaseRepoID,
8670
Repository: pr.BaseRepo.APIFormat(models.AccessModeNone),
87-
}
71+
},
72+
Head: &api.PRBranchInfo{
73+
Name: pr.HeadBranch,
74+
Ref: fmt.Sprintf("refs/pull/%d/head", pr.Index),
75+
RepoID: -1,
76+
},
77+
}
78+
79+
baseBranch, err = repo_module.GetBranch(pr.BaseRepo, pr.BaseBranch)
80+
if err != nil && !git.IsErrBranchNotExist(err) {
81+
log.Error("GetBranch[%s]: %v", pr.BaseBranch, err)
82+
return nil
83+
}
84+
85+
if err == nil {
8886
baseCommit, err = baseBranch.GetCommit()
89-
if err != nil {
90-
if git.IsErrNotExist(err) {
91-
apiBaseBranchInfo.Sha = ""
92-
} else {
93-
log.Error("GetCommit[%s]: %v", baseBranch.Name, err)
94-
return nil
95-
}
96-
} else {
97-
apiBaseBranchInfo.Sha = baseCommit.ID.String()
87+
if err != nil && !git.IsErrNotExist(err) {
88+
log.Error("GetCommit[%s]: %v", baseBranch.Name, err)
89+
return nil
90+
}
91+
92+
if err == nil {
93+
apiPullRequest.Base.Sha = baseCommit.ID.String()
9894
}
99-
apiPullRequest.Base = apiBaseBranchInfo
10095
}
10196

10297
if pr.HeadRepo != nil {
103-
headBranch, err = repo_module.GetBranch(pr.HeadRepo, pr.HeadBranch)
98+
apiPullRequest.Head.RepoID = pr.HeadRepo.ID
99+
apiPullRequest.Head.Repository = pr.HeadRepo.APIFormat(models.AccessModeNone)
100+
101+
headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath())
104102
if err != nil {
105-
if git.IsErrBranchNotExist(err) {
106-
apiPullRequest.Head = nil
107-
} else {
108-
log.Error("GetBranch[%s]: %v", pr.HeadBranch, err)
103+
log.Error("OpenRepository[%s]: %v", pr.HeadRepo.RepoPath(), err)
104+
return nil
105+
}
106+
defer headGitRepo.Close()
107+
108+
headBranch, err = headGitRepo.GetBranch(pr.HeadBranch)
109+
if err != nil && !git.IsErrBranchNotExist(err) {
110+
log.Error("GetBranch[%s]: %v", pr.HeadBranch, err)
111+
return nil
112+
}
113+
114+
if git.IsErrBranchNotExist(err) {
115+
headCommitID, err := headGitRepo.GetRefCommitID(apiPullRequest.Head.Ref)
116+
if err != nil && !git.IsErrNotExist(err) {
117+
log.Error("GetCommit[%s]: %v", headBranch.Name, err)
109118
return nil
110119
}
120+
if err == nil {
121+
apiPullRequest.Head.Sha = headCommitID
122+
}
111123
} else {
112-
apiHeadBranchInfo := &api.PRBranchInfo{
113-
Name: pr.HeadBranch,
114-
Ref: pr.HeadBranch,
115-
RepoID: pr.HeadRepoID,
116-
Repository: pr.HeadRepo.APIFormat(models.AccessModeNone),
124+
commit, err := headBranch.GetCommit()
125+
if err != nil && !git.IsErrNotExist(err) {
126+
log.Error("GetCommit[%s]: %v", headBranch.Name, err)
127+
return nil
117128
}
118-
headCommit, err = headBranch.GetCommit()
119-
if err != nil {
120-
if git.IsErrNotExist(err) {
121-
apiHeadBranchInfo.Sha = ""
122-
} else {
123-
log.Error("GetCommit[%s]: %v", headBranch.Name, err)
124-
return nil
125-
}
126-
} else {
127-
apiHeadBranchInfo.Sha = headCommit.ID.String()
129+
if err == nil {
130+
apiPullRequest.Head.Ref = pr.HeadBranch
131+
apiPullRequest.Head.Sha = commit.ID.String()
128132
}
129-
apiPullRequest.Head = apiHeadBranchInfo
130-
}
131-
} else {
132-
apiPullRequest.Head = &api.PRBranchInfo{
133-
Name: pr.HeadBranch,
134-
Ref: fmt.Sprintf("refs/pull/%d/head", pr.Index),
135-
RepoID: -1,
136133
}
137134
}
138135

‎modules/convert/pull_test.go

+9-1
Original file line numberDiff line numberDiff line change
@@ -8,19 +8,27 @@ import (
88
"testing"
99

1010
"code.gitea.io/gitea/models"
11+
"code.gitea.io/gitea/modules/structs"
1112

1213
"github.com/stretchr/testify/assert"
1314
)
1415

1516
func TestPullRequest_APIFormat(t *testing.T) {
1617
//with HeadRepo
1718
assert.NoError(t, models.PrepareTestDatabase())
19+
headRepo := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 1}).(*models.Repository)
1820
pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest)
1921
assert.NoError(t, pr.LoadAttributes())
2022
assert.NoError(t, pr.LoadIssue())
2123
apiPullRequest := ToAPIPullRequest(pr)
2224
assert.NotNil(t, apiPullRequest)
23-
assert.Nil(t, apiPullRequest.Head)
25+
assert.EqualValues(t, &structs.PRBranchInfo{
26+
Name: "branch1",
27+
Ref: "refs/pull/2/head",
28+
Sha: "4a357436d925b5c974181ff12a994538ddc5a269",
29+
RepoID: 1,
30+
Repository: headRepo.APIFormat(models.AccessModeNone),
31+
}, apiPullRequest.Head)
2432

2533
//withOut HeadRepo
2634
pr = models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest)

0 commit comments

Comments
 (0)