From 5abe1c52de533b52725b1e584db9fea6fb1b39ab Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 3 Mar 2020 06:31:55 +0800 Subject: [PATCH] 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 --- models/pull.go | 104 +++++++++++++------------- models/pull_sign.go | 2 +- models/pull_test.go | 10 +-- modules/convert/pull.go | 137 +++++++++++++++++------------------ modules/convert/pull_test.go | 10 ++- routers/api/v1/repo/pull.go | 20 ++--- routers/repo/issue.go | 8 +- routers/repo/pull.go | 34 +++++---- services/pull/merge.go | 30 ++++---- services/pull/pull.go | 9 +-- services/pull/review.go | 4 +- services/pull/temp_repo.go | 12 +-- 12 files changed, 192 insertions(+), 188 deletions(-) diff --git a/models/pull.go b/models/pull.go index 46c50986b..3e177ea5e 100644 --- a/models/pull.go +++ b/models/pull.go @@ -62,6 +62,8 @@ type PullRequest struct { MergerID int64 `xorm:"INDEX"` Merger *User `xorm:"-"` MergedUnix timeutil.TimeStamp `xorm:"updated INDEX"` + + isHeadRepoLoaded bool `xorm:"-"` } // MustHeadUserName returns the HeadRepo's username if failed return blank @@ -74,6 +76,9 @@ func (pr *PullRequest) MustHeadUserName() string { } return "" } + if pr.HeadRepo == nil { + return "" + } return pr.HeadRepo.OwnerName } @@ -97,38 +102,55 @@ func (pr *PullRequest) LoadAttributes() error { return pr.loadAttributes(x) } -// LoadBaseRepo loads pull request base repository from database -func (pr *PullRequest) LoadBaseRepo() error { - if pr.BaseRepo == nil { - if pr.HeadRepoID == pr.BaseRepoID && pr.HeadRepo != nil { - pr.BaseRepo = pr.HeadRepo - return nil +func (pr *PullRequest) loadHeadRepo(e Engine) (err error) { + if !pr.isHeadRepoLoaded && pr.HeadRepo == nil && pr.HeadRepoID > 0 { + if pr.HeadRepoID == pr.BaseRepoID { + if pr.BaseRepo != nil { + pr.HeadRepo = pr.BaseRepo + return nil + } else if pr.Issue != nil && pr.Issue.Repo != nil { + pr.HeadRepo = pr.Issue.Repo + return nil + } } - var repo Repository - if has, err := x.ID(pr.BaseRepoID).Get(&repo); err != nil { - return err - } else if !has { - return ErrRepoNotExist{ID: pr.BaseRepoID} + + pr.HeadRepo, err = getRepositoryByID(e, pr.HeadRepoID) + if err != nil && !IsErrRepoNotExist(err) { // Head repo maybe deleted, but it should still work + return fmt.Errorf("getRepositoryByID(head): %v", err) } - pr.BaseRepo = &repo + pr.isHeadRepoLoaded = true } return nil } -// LoadHeadRepo loads pull request head repository from database +// LoadHeadRepo loads the head repository func (pr *PullRequest) LoadHeadRepo() error { - if pr.HeadRepo == nil { - if pr.HeadRepoID == pr.BaseRepoID && pr.BaseRepo != nil { - pr.HeadRepo = pr.BaseRepo - return nil - } - var repo Repository - if has, err := x.ID(pr.HeadRepoID).Get(&repo); err != nil { - return err - } else if !has { - return ErrRepoNotExist{ID: pr.HeadRepoID} - } - pr.HeadRepo = &repo + return pr.loadHeadRepo(x) +} + +// LoadBaseRepo loads the target repository +func (pr *PullRequest) LoadBaseRepo() error { + return pr.loadBaseRepo(x) +} + +func (pr *PullRequest) loadBaseRepo(e Engine) (err error) { + if pr.BaseRepo != nil { + return nil + } + + if pr.HeadRepoID == pr.BaseRepoID && pr.HeadRepo != nil { + pr.BaseRepo = pr.HeadRepo + return nil + } + + if pr.Issue != nil && pr.Issue.Repo != nil { + pr.BaseRepo = pr.Issue.Repo + return nil + } + + pr.BaseRepo, err = getRepositoryByID(e, pr.BaseRepoID) + if err != nil { + return fmt.Errorf("GetRepositoryByID(base): %v", err) } return nil } @@ -414,32 +436,6 @@ func (pr *PullRequest) GetGitRefName() string { return fmt.Sprintf("refs/pull/%d/head", pr.Index) } -func (pr *PullRequest) getHeadRepo(e Engine) (err error) { - pr.HeadRepo, err = getRepositoryByID(e, pr.HeadRepoID) - if err != nil && !IsErrRepoNotExist(err) { - return fmt.Errorf("getRepositoryByID(head): %v", err) - } - return nil -} - -// GetHeadRepo loads the head repository -func (pr *PullRequest) GetHeadRepo() error { - return pr.getHeadRepo(x) -} - -// GetBaseRepo loads the target repository -func (pr *PullRequest) GetBaseRepo() (err error) { - if pr.BaseRepo != nil { - return nil - } - - pr.BaseRepo, err = GetRepositoryByID(pr.BaseRepoID) - if err != nil { - return fmt.Errorf("GetRepositoryByID(base): %v", err) - } - return nil -} - // IsChecking returns true if this pull request is still checking conflict. func (pr *PullRequest) IsChecking() bool { return pr.Status == PullRequestStatusChecking @@ -452,7 +448,7 @@ func (pr *PullRequest) CanAutoMerge() bool { // GetLastCommitStatus returns the last commit status for this pull request. func (pr *PullRequest) GetLastCommitStatus() (status *CommitStatus, err error) { - if err = pr.GetHeadRepo(); err != nil { + if err = pr.LoadHeadRepo(); err != nil { return nil, err } @@ -774,7 +770,7 @@ func (pr *PullRequest) GetWorkInProgressPrefix() string { // IsHeadEqualWithBranch returns if the commits of branchName are available in pull request head func (pr *PullRequest) IsHeadEqualWithBranch(branchName string) (bool, error) { var err error - if err = pr.GetBaseRepo(); err != nil { + if err = pr.LoadBaseRepo(); err != nil { return false, err } baseGitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath()) @@ -786,7 +782,7 @@ func (pr *PullRequest) IsHeadEqualWithBranch(branchName string) (bool, error) { return false, err } - if err = pr.GetHeadRepo(); err != nil { + if err = pr.LoadHeadRepo(); err != nil { return false, err } headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath()) diff --git a/models/pull_sign.go b/models/pull_sign.go index 5b26b4bdc..91b8defa5 100644 --- a/models/pull_sign.go +++ b/models/pull_sign.go @@ -12,7 +12,7 @@ import ( // SignMerge determines if we should sign a PR merge commit to the base repository func (pr *PullRequest) SignMerge(u *User, tmpBasePath, baseCommit, headCommit string) (bool, string, error) { - if err := pr.GetBaseRepo(); err != nil { + if err := pr.LoadBaseRepo(); err != nil { log.Error("Unable to get Base Repo for pull request") return false, "", err } diff --git a/models/pull_test.go b/models/pull_test.go index 0dc3bb86d..3cc6abfec 100644 --- a/models/pull_test.go +++ b/models/pull_test.go @@ -29,21 +29,21 @@ func TestPullRequest_LoadIssue(t *testing.T) { assert.Equal(t, int64(2), pr.Issue.ID) } -func TestPullRequest_GetBaseRepo(t *testing.T) { +func TestPullRequest_LoadBaseRepo(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) pr := AssertExistsAndLoadBean(t, &PullRequest{ID: 1}).(*PullRequest) - assert.NoError(t, pr.GetBaseRepo()) + assert.NoError(t, pr.LoadBaseRepo()) assert.NotNil(t, pr.BaseRepo) assert.Equal(t, pr.BaseRepoID, pr.BaseRepo.ID) - assert.NoError(t, pr.GetBaseRepo()) + assert.NoError(t, pr.LoadBaseRepo()) assert.NotNil(t, pr.BaseRepo) assert.Equal(t, pr.BaseRepoID, pr.BaseRepo.ID) } -func TestPullRequest_GetHeadRepo(t *testing.T) { +func TestPullRequest_LoadHeadRepo(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) pr := AssertExistsAndLoadBean(t, &PullRequest{ID: 1}).(*PullRequest) - assert.NoError(t, pr.GetHeadRepo()) + assert.NoError(t, pr.LoadHeadRepo()) assert.NotNil(t, pr.HeadRepo) assert.Equal(t, pr.HeadRepoID, pr.HeadRepo.ID) } diff --git a/modules/convert/pull.go b/modules/convert/pull.go index cf2bf88aa..ccf64ef3e 100644 --- a/modules/convert/pull.go +++ b/modules/convert/pull.go @@ -22,7 +22,6 @@ func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest { baseBranch *git.Branch headBranch *git.Branch baseCommit *git.Commit - headCommit *git.Commit err error ) @@ -32,20 +31,14 @@ func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest { } apiIssue := ToAPIIssue(pr.Issue) - if pr.BaseRepo == nil { - pr.BaseRepo, err = models.GetRepositoryByID(pr.BaseRepoID) - if err != nil { - log.Error("GetRepositoryById[%d]: %v", pr.ID, err) - return nil - } + if err := pr.LoadBaseRepo(); err != nil { + log.Error("GetRepositoryById[%d]: %v", pr.ID, err) + return nil } - if pr.HeadRepoID != 0 && pr.HeadRepo == nil { - pr.HeadRepo, err = models.GetRepositoryByID(pr.HeadRepoID) - if err != nil && !models.IsErrRepoNotExist(err) { - log.Error("GetRepositoryById[%d]: %v", pr.ID, err) - return nil - } + if err := pr.LoadHeadRepo(); err != nil { + log.Error("GetRepositoryById[%d]: %v", pr.ID, err) + return nil } apiPullRequest := &api.PullRequest{ @@ -69,70 +62,74 @@ func ToAPIPullRequest(pr *models.PullRequest) *api.PullRequest { Deadline: apiIssue.Deadline, Created: pr.Issue.CreatedUnix.AsTimePtr(), Updated: pr.Issue.UpdatedUnix.AsTimePtr(), - } - baseBranch, err = repo_module.GetBranch(pr.BaseRepo, pr.BaseBranch) - if err != nil { - if git.IsErrBranchNotExist(err) { - apiPullRequest.Base = nil - } else { - log.Error("GetBranch[%s]: %v", pr.BaseBranch, err) - return nil - } - } else { - apiBaseBranchInfo := &api.PRBranchInfo{ + + Base: &api.PRBranchInfo{ Name: pr.BaseBranch, Ref: pr.BaseBranch, RepoID: pr.BaseRepoID, Repository: pr.BaseRepo.APIFormat(models.AccessModeNone), - } - baseCommit, err = baseBranch.GetCommit() - if err != nil { - if git.IsErrNotExist(err) { - apiBaseBranchInfo.Sha = "" - } else { - log.Error("GetCommit[%s]: %v", baseBranch.Name, err) - return nil - } - } else { - apiBaseBranchInfo.Sha = baseCommit.ID.String() - } - apiPullRequest.Base = apiBaseBranchInfo - } - - if pr.HeadRepo != nil { - headBranch, err = repo_module.GetBranch(pr.HeadRepo, pr.HeadBranch) - if err != nil { - if git.IsErrBranchNotExist(err) { - apiPullRequest.Head = nil - } else { - log.Error("GetBranch[%s]: %v", pr.HeadBranch, err) - return nil - } - } else { - apiHeadBranchInfo := &api.PRBranchInfo{ - Name: pr.HeadBranch, - Ref: pr.HeadBranch, - RepoID: pr.HeadRepoID, - Repository: pr.HeadRepo.APIFormat(models.AccessModeNone), - } - headCommit, err = headBranch.GetCommit() - if err != nil { - if git.IsErrNotExist(err) { - apiHeadBranchInfo.Sha = "" - } else { - log.Error("GetCommit[%s]: %v", headBranch.Name, err) - return nil - } - } else { - apiHeadBranchInfo.Sha = headCommit.ID.String() - } - apiPullRequest.Head = apiHeadBranchInfo - } - } else { - apiPullRequest.Head = &api.PRBranchInfo{ + }, + Head: &api.PRBranchInfo{ Name: pr.HeadBranch, Ref: fmt.Sprintf("refs/pull/%d/head", pr.Index), RepoID: -1, + }, + } + + baseBranch, err = repo_module.GetBranch(pr.BaseRepo, pr.BaseBranch) + if err != nil && !git.IsErrBranchNotExist(err) { + log.Error("GetBranch[%s]: %v", pr.BaseBranch, err) + return nil + } + + if err == nil { + baseCommit, err = baseBranch.GetCommit() + if err != nil && !git.IsErrNotExist(err) { + log.Error("GetCommit[%s]: %v", baseBranch.Name, err) + return nil + } + + if err == nil { + apiPullRequest.Base.Sha = baseCommit.ID.String() + } + } + + if pr.HeadRepo != nil { + apiPullRequest.Head.RepoID = pr.HeadRepo.ID + apiPullRequest.Head.Repository = pr.HeadRepo.APIFormat(models.AccessModeNone) + + headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath()) + if err != nil { + log.Error("OpenRepository[%s]: %v", pr.HeadRepo.RepoPath(), err) + return nil + } + defer headGitRepo.Close() + + headBranch, err = headGitRepo.GetBranch(pr.HeadBranch) + if err != nil && !git.IsErrBranchNotExist(err) { + log.Error("GetBranch[%s]: %v", pr.HeadBranch, err) + return nil + } + + if git.IsErrBranchNotExist(err) { + headCommitID, err := headGitRepo.GetRefCommitID(apiPullRequest.Head.Ref) + if err != nil && !git.IsErrNotExist(err) { + log.Error("GetCommit[%s]: %v", headBranch.Name, err) + return nil + } + if err == nil { + apiPullRequest.Head.Sha = headCommitID + } + } else { + commit, err := headBranch.GetCommit() + if err != nil && !git.IsErrNotExist(err) { + log.Error("GetCommit[%s]: %v", headBranch.Name, err) + return nil + } + if err == nil { + apiPullRequest.Head.Ref = pr.HeadBranch + apiPullRequest.Head.Sha = commit.ID.String() + } } } diff --git a/modules/convert/pull_test.go b/modules/convert/pull_test.go index fe82a61bd..b78bbe87c 100644 --- a/modules/convert/pull_test.go +++ b/modules/convert/pull_test.go @@ -8,6 +8,7 @@ import ( "testing" "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/structs" "github.com/stretchr/testify/assert" ) @@ -15,12 +16,19 @@ import ( func TestPullRequest_APIFormat(t *testing.T) { //with HeadRepo assert.NoError(t, models.PrepareTestDatabase()) + headRepo := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 1}).(*models.Repository) pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest) assert.NoError(t, pr.LoadAttributes()) assert.NoError(t, pr.LoadIssue()) apiPullRequest := ToAPIPullRequest(pr) assert.NotNil(t, apiPullRequest) - assert.Nil(t, apiPullRequest.Head) + assert.EqualValues(t, &structs.PRBranchInfo{ + Name: "branch1", + Ref: "refs/pull/2/head", + Sha: "4a357436d925b5c974181ff12a994538ddc5a269", + RepoID: 1, + Repository: headRepo.APIFormat(models.AccessModeNone), + }, apiPullRequest.Head) //withOut HeadRepo pr = models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index d6d6918bb..428f11263 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -101,12 +101,12 @@ func ListPullRequests(ctx *context.APIContext, form api.ListPullRequestsOptions) ctx.Error(http.StatusInternalServerError, "LoadAttributes", err) return } - if err = prs[i].GetBaseRepo(); err != nil { - ctx.Error(http.StatusInternalServerError, "GetBaseRepo", err) + if err = prs[i].LoadBaseRepo(); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadBaseRepo", err) return } - if err = prs[i].GetHeadRepo(); err != nil { - ctx.Error(http.StatusInternalServerError, "GetHeadRepo", err) + if err = prs[i].LoadHeadRepo(); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadHeadRepo", err) return } apiPrs[i] = convert.ToAPIPullRequest(prs[i]) @@ -156,12 +156,12 @@ func GetPullRequest(ctx *context.APIContext) { return } - if err = pr.GetBaseRepo(); err != nil { - ctx.Error(http.StatusInternalServerError, "GetBaseRepo", err) + if err = pr.LoadBaseRepo(); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadBaseRepo", err) return } - if err = pr.GetHeadRepo(); err != nil { - ctx.Error(http.StatusInternalServerError, "GetHeadRepo", err) + if err = pr.LoadHeadRepo(); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadHeadRepo", err) return } ctx.JSON(http.StatusOK, convert.ToAPIPullRequest(pr)) @@ -579,8 +579,8 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) { return } - if err = pr.GetHeadRepo(); err != nil { - ctx.ServerError("GetHeadRepo", err) + if err = pr.LoadHeadRepo(); err != nil { + ctx.ServerError("LoadHeadRepo", err) return } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index ca7be7bd9..a4cd184c1 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -910,8 +910,8 @@ func ViewIssue(ctx *context.Context) { ctx.Data["AllowMerge"] = false if ctx.IsSigned { - if err := pull.GetHeadRepo(); err != nil { - log.Error("GetHeadRepo: %v", err) + if err := pull.LoadHeadRepo(); err != nil { + log.Error("LoadHeadRepo: %v", err) } else if pull.HeadRepo != nil && pull.HeadBranch != pull.HeadRepo.DefaultBranch { perm, err := models.GetUserRepoPermission(pull.HeadRepo, ctx.User) if err != nil { @@ -929,8 +929,8 @@ func ViewIssue(ctx *context.Context) { } } - if err := pull.GetBaseRepo(); err != nil { - log.Error("GetBaseRepo: %v", err) + if err := pull.LoadBaseRepo(); err != nil { + log.Error("LoadBaseRepo: %v", err) } perm, err := models.GetUserRepoPermission(pull.BaseRepo, ctx.User) if err != nil { diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 92538945b..e4fd19ee4 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -273,8 +273,8 @@ func checkPullInfo(ctx *context.Context) *models.Issue { return nil } - if err = issue.PullRequest.GetHeadRepo(); err != nil { - ctx.ServerError("GetHeadRepo", err) + if err = issue.PullRequest.LoadHeadRepo(); err != nil { + ctx.ServerError("LoadHeadRepo", err) return nil } @@ -313,7 +313,7 @@ func PrepareMergedViewPullInfo(ctx *context.Context, issue *models.Issue) *git.C if err != nil { if strings.Contains(err.Error(), "fatal: Not a valid object name") { ctx.Data["IsPullRequestBroken"] = true - ctx.Data["BaseTarget"] = "deleted" + ctx.Data["BaseTarget"] = pull.BaseBranch ctx.Data["NumCommits"] = 0 ctx.Data["NumFiles"] = 0 return nil @@ -332,13 +332,13 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare repo := ctx.Repo.Repository pull := issue.PullRequest - if err := pull.GetHeadRepo(); err != nil { - ctx.ServerError("GetHeadRepo", err) + if err := pull.LoadHeadRepo(); err != nil { + ctx.ServerError("LoadHeadRepo", err) return nil } - if err := pull.GetBaseRepo(); err != nil { - ctx.ServerError("GetBaseRepo", err) + if err := pull.LoadBaseRepo(); err != nil { + ctx.ServerError("LoadBaseRepo", err) return nil } @@ -432,7 +432,15 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare if pull.HeadRepo == nil || !headBranchExist || headBranchSha != sha { ctx.Data["IsPullRequestBroken"] = true - ctx.Data["HeadTarget"] = "deleted" + if pull.IsSameRepo() { + ctx.Data["HeadTarget"] = pull.HeadBranch + } else { + if pull.HeadRepo == nil { + ctx.Data["HeadTarget"] = ":" + pull.HeadBranch + } else { + ctx.Data["HeadTarget"] = pull.HeadRepo.OwnerName + ":" + pull.HeadBranch + } + } } compareInfo, err := baseGitRepo.GetCompareInfo(pull.BaseRepo.RepoPath(), @@ -440,7 +448,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare if err != nil { if strings.Contains(err.Error(), "fatal: Not a valid object name") { ctx.Data["IsPullRequestBroken"] = true - ctx.Data["BaseTarget"] = "deleted" + ctx.Data["BaseTarget"] = pull.BaseBranch ctx.Data["NumCommits"] = 0 ctx.Data["NumFiles"] = 0 return nil @@ -976,15 +984,15 @@ func CleanUpPullRequest(ctx *context.Context) { return } - if err := pr.GetHeadRepo(); err != nil { - ctx.ServerError("GetHeadRepo", err) + if err := pr.LoadHeadRepo(); err != nil { + ctx.ServerError("LoadHeadRepo", err) return } else if pr.HeadRepo == nil { // Forked repository has already been deleted ctx.NotFound("CleanUpPullRequest", nil) return - } else if err = pr.GetBaseRepo(); err != nil { - ctx.ServerError("GetBaseRepo", err) + } else if err = pr.LoadBaseRepo(); err != nil { + ctx.ServerError("LoadBaseRepo", err) return } else if err = pr.HeadRepo.GetOwner(); err != nil { ctx.ServerError("HeadRepo.GetOwner", err) diff --git a/services/pull/merge.go b/services/pull/merge.go index 4d02d7193..c8351ba85 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -33,13 +33,12 @@ import ( // Caller should check PR is ready to be merged (review and status checks) // FIXME: add repoWorkingPull make sure two merges does not happen at same time. func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repository, mergeStyle models.MergeStyle, message string) (err error) { - - if err = pr.GetHeadRepo(); err != nil { - log.Error("GetHeadRepo: %v", err) - return fmt.Errorf("GetHeadRepo: %v", err) - } else if err = pr.GetBaseRepo(); err != nil { - log.Error("GetBaseRepo: %v", err) - return fmt.Errorf("GetBaseRepo: %v", err) + if err = pr.LoadHeadRepo(); err != nil { + log.Error("LoadHeadRepo: %v", err) + return fmt.Errorf("LoadHeadRepo: %v", err) + } else if err = pr.LoadBaseRepo(); err != nil { + log.Error("LoadBaseRepo: %v", err) + return fmt.Errorf("LoadBaseRepo: %v", err) } prUnit, err := pr.BaseRepo.GetUnit(models.UnitTypePullRequests) @@ -535,18 +534,15 @@ func IsUserAllowedToMerge(pr *models.PullRequest, p models.Permission, user *mod // CheckPRReadyToMerge checks whether the PR is ready to be merged (reviews and status checks) func CheckPRReadyToMerge(pr *models.PullRequest) (err error) { - if pr.BaseRepo == nil { - if err = pr.GetBaseRepo(); err != nil { - return fmt.Errorf("GetBaseRepo: %v", err) - } + if err = pr.LoadBaseRepo(); err != nil { + return fmt.Errorf("LoadBaseRepo: %v", err) + } + + if err = pr.LoadProtectedBranch(); err != nil { + return fmt.Errorf("LoadProtectedBranch: %v", err) } if pr.ProtectedBranch == nil { - if err = pr.LoadProtectedBranch(); err != nil { - return fmt.Errorf("LoadProtectedBranch: %v", err) - } - if pr.ProtectedBranch == nil { - return nil - } + return nil } isPass, err := IsPullCommitStatusPass(pr) diff --git a/services/pull/pull.go b/services/pull/pull.go index 6af751856..5c7ec91f5 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -236,16 +236,15 @@ func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSy // checkIfPRContentChanged checks if diff to target branch has changed by push // A commit can be considered to leave the PR untouched if the patch/diff with its merge base is unchanged func checkIfPRContentChanged(pr *models.PullRequest, oldCommitID, newCommitID string) (hasChanged bool, err error) { - - if err = pr.GetHeadRepo(); err != nil { - return false, fmt.Errorf("GetHeadRepo: %v", err) + if err = pr.LoadHeadRepo(); err != nil { + return false, fmt.Errorf("LoadHeadRepo: %v", err) } else if pr.HeadRepo == nil { // corrupt data assumed changed return true, nil } - if err = pr.GetBaseRepo(); err != nil { - return false, fmt.Errorf("GetBaseRepo: %v", err) + if err = pr.LoadBaseRepo(); err != nil { + return false, fmt.Errorf("LoadBaseRepo: %v", err) } headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath()) diff --git a/services/pull/review.go b/services/pull/review.go index 2b7c9e864..25e0ca858 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -111,8 +111,8 @@ func createCodeComment(doer *models.User, repo *models.Repository, issue *models return nil, fmt.Errorf("GetPullRequestByIssueID: %v", err) } pr := issue.PullRequest - if err := pr.GetBaseRepo(); err != nil { - return nil, fmt.Errorf("GetHeadRepo: %v", err) + if err := pr.LoadBaseRepo(); err != nil { + return nil, fmt.Errorf("LoadHeadRepo: %v", err) } gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath()) if err != nil { diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go index bb6ce2921..ffac92ade 100644 --- a/services/pull/temp_repo.go +++ b/services/pull/temp_repo.go @@ -17,17 +17,17 @@ import ( ) func createTemporaryRepo(pr *models.PullRequest) (string, error) { - if err := pr.GetHeadRepo(); err != nil { - log.Error("GetHeadRepo: %v", err) - return "", fmt.Errorf("GetHeadRepo: %v", err) + if err := pr.LoadHeadRepo(); err != nil { + log.Error("LoadHeadRepo: %v", err) + return "", fmt.Errorf("LoadHeadRepo: %v", err) } else if pr.HeadRepo == nil { log.Error("Pr %d HeadRepo %d does not exist", pr.ID, pr.HeadRepoID) return "", &models.ErrRepoNotExist{ ID: pr.HeadRepoID, } - } else if err := pr.GetBaseRepo(); err != nil { - log.Error("GetBaseRepo: %v", err) - return "", fmt.Errorf("GetBaseRepo: %v", err) + } else if err := pr.LoadBaseRepo(); err != nil { + log.Error("LoadBaseRepo: %v", err) + return "", fmt.Errorf("LoadBaseRepo: %v", err) } else if pr.BaseRepo == nil { log.Error("Pr %d BaseRepo %d does not exist", pr.ID, pr.BaseRepoID) return "", &models.ErrRepoNotExist{