Fix bug on pull requests when transfer head repository (#8564)

* fix bug on pull requests when transfer head repository

* add migration and fix lint

* fix tests and add a cache check on LoadBaseRepo
This commit is contained in:
Lunny Xiao 2019-10-18 19:13:31 +08:00 committed by GitHub
parent fecd8f949d
commit 945f121262
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 87 additions and 73 deletions

View file

@ -6,7 +6,6 @@
index: 2 index: 2
head_repo_id: 1 head_repo_id: 1
base_repo_id: 1 base_repo_id: 1
head_user_name: user1
head_branch: branch1 head_branch: branch1
base_branch: master base_branch: master
merge_base: 1234567890abcdef merge_base: 1234567890abcdef
@ -21,7 +20,6 @@
index: 3 index: 3
head_repo_id: 1 head_repo_id: 1
base_repo_id: 1 base_repo_id: 1
head_user_name: user1
head_branch: branch2 head_branch: branch2
base_branch: master base_branch: master
merge_base: fedcba9876543210 merge_base: fedcba9876543210
@ -35,7 +33,6 @@
index: 1 index: 1
head_repo_id: 11 head_repo_id: 11
base_repo_id: 10 base_repo_id: 10
head_user_name: user13
head_branch: branch2 head_branch: branch2
base_branch: master base_branch: master
merge_base: 0abcb056019adb83 merge_base: 0abcb056019adb83

View file

@ -258,6 +258,8 @@ var migrations = []Migration{
NewMigration("update migration repositories' service type", updateMigrationServiceTypes), NewMigration("update migration repositories' service type", updateMigrationServiceTypes),
// v101 -> v102 // v101 -> v102
NewMigration("change length of some external login users columns", changeSomeColumnsLengthOfExternalLoginUser), NewMigration("change length of some external login users columns", changeSomeColumnsLengthOfExternalLoginUser),
// v102 -> v103
NewMigration("update migration repositories' service type", dropColumnHeadUserNameOnPullRequest),
} }
// Migrate database to current version // Migrate database to current version

15
models/migrations/v102.go Normal file
View file

@ -0,0 +1,15 @@
// Copyright 2019 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.
package migrations
import (
"xorm.io/xorm"
)
func dropColumnHeadUserNameOnPullRequest(x *xorm.Engine) error {
sess := x.NewSession()
defer sess.Close()
return dropTableColumns(sess, "pull_request", "head_user_name")
}

View file

@ -66,7 +66,6 @@ type PullRequest struct {
HeadRepo *Repository `xorm:"-"` HeadRepo *Repository `xorm:"-"`
BaseRepoID int64 `xorm:"INDEX"` BaseRepoID int64 `xorm:"INDEX"`
BaseRepo *Repository `xorm:"-"` BaseRepo *Repository `xorm:"-"`
HeadUserName string
HeadBranch string HeadBranch string
BaseBranch string BaseBranch string
ProtectedBranch *ProtectedBranch `xorm:"-"` ProtectedBranch *ProtectedBranch `xorm:"-"`
@ -79,6 +78,15 @@ type PullRequest struct {
MergedUnix timeutil.TimeStamp `xorm:"updated INDEX"` MergedUnix timeutil.TimeStamp `xorm:"updated INDEX"`
} }
// MustHeadUserName returns the HeadRepo's username if failed return blank
func (pr *PullRequest) MustHeadUserName() string {
if err := pr.LoadHeadRepo(); err != nil {
log.Error("LoadHeadRepo: %v", err)
return ""
}
return pr.HeadRepo.MustOwnerName()
}
// Note: don't try to get Issue because will end up recursive querying. // Note: don't try to get Issue because will end up recursive querying.
func (pr *PullRequest) loadAttributes(e Engine) (err error) { func (pr *PullRequest) loadAttributes(e Engine) (err error) {
if pr.HasMerged && pr.Merger == nil { if pr.HasMerged && pr.Merger == nil {
@ -102,6 +110,10 @@ func (pr *PullRequest) LoadAttributes() error {
// LoadBaseRepo loads pull request base repository from database // LoadBaseRepo loads pull request base repository from database
func (pr *PullRequest) LoadBaseRepo() error { func (pr *PullRequest) LoadBaseRepo() error {
if pr.BaseRepo == nil { if pr.BaseRepo == nil {
if pr.HeadRepoID == pr.BaseRepoID && pr.HeadRepo != nil {
pr.BaseRepo = pr.HeadRepo
return nil
}
var repo Repository var repo Repository
if has, err := x.ID(pr.BaseRepoID).Get(&repo); err != nil { if has, err := x.ID(pr.BaseRepoID).Get(&repo); err != nil {
return err return err
@ -113,6 +125,24 @@ func (pr *PullRequest) LoadBaseRepo() error {
return nil return nil
} }
// LoadHeadRepo loads pull request head repository from database
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.BaseRepoID}
}
pr.HeadRepo = &repo
}
return nil
}
// LoadIssue loads issue information from database // LoadIssue loads issue information from database
func (pr *PullRequest) LoadIssue() (err error) { func (pr *PullRequest) LoadIssue() (err error) {
return pr.loadIssue(x) return pr.loadIssue(x)
@ -152,7 +182,7 @@ func (pr *PullRequest) GetDefaultMergeMessage() string {
return "" return ""
} }
} }
return fmt.Sprintf("Merge branch '%s' of %s/%s into %s", pr.HeadBranch, pr.HeadUserName, pr.HeadRepo.Name, pr.BaseBranch) return fmt.Sprintf("Merge branch '%s' of %s/%s into %s", pr.HeadBranch, pr.MustHeadUserName(), pr.HeadRepo.Name, pr.BaseBranch)
} }
// GetDefaultSquashMessage returns default message used when squash and merging pull request // GetDefaultSquashMessage returns default message used when squash and merging pull request
@ -1072,18 +1102,6 @@ func (prs PullRequestList) InvalidateCodeComments(doer *User, repo *git.Reposito
return prs.invalidateCodeComments(x, doer, repo, branch) return prs.invalidateCodeComments(x, doer, repo, branch)
} }
// ChangeUsernameInPullRequests changes the name of head_user_name
func ChangeUsernameInPullRequests(oldUserName, newUserName string) error {
pr := PullRequest{
HeadUserName: strings.ToLower(newUserName),
}
_, err := x.
Cols("head_user_name").
Where("head_user_name = ?", strings.ToLower(oldUserName)).
Update(pr)
return err
}
// checkAndUpdateStatus checks if pull request is possible to leaving checking status, // checkAndUpdateStatus checks if pull request is possible to leaving checking status,
// and set to be either conflict or mergeable. // and set to be either conflict or mergeable.
func (pr *PullRequest) checkAndUpdateStatus() { func (pr *PullRequest) checkAndUpdateStatus() {

View file

@ -232,20 +232,6 @@ func TestPullRequestList_LoadAttributes(t *testing.T) {
// TODO TestAddTestPullRequestTask // TODO TestAddTestPullRequestTask
func TestChangeUsernameInPullRequests(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())
const newUsername = "newusername"
assert.NoError(t, ChangeUsernameInPullRequests("user1", newUsername))
prs := make([]*PullRequest, 0, 10)
assert.NoError(t, x.Where("head_user_name = ?", newUsername).Find(&prs))
assert.Len(t, prs, 2)
for _, pr := range prs {
assert.Equal(t, newUsername, pr.HeadUserName)
}
CheckConsistencyFor(t, &PullRequest{})
}
func TestPullRequest_IsWorkInProgress(t *testing.T) { func TestPullRequest_IsWorkInProgress(t *testing.T) {
assert.NoError(t, PrepareTestDatabase()) assert.NoError(t, PrepareTestDatabase())

View file

@ -994,10 +994,6 @@ func ChangeUserName(u *User, newUserName string) (err error) {
return ErrUserAlreadyExist{newUserName} return ErrUserAlreadyExist{newUserName}
} }
if err = ChangeUsernameInPullRequests(u.Name, newUserName); err != nil {
return fmt.Errorf("ChangeUsernameInPullRequests: %v", err)
}
// Do not fail if directory does not exist // Do not fail if directory does not exist
if err = os.Rename(UserPath(u.Name), UserPath(newUserName)); err != nil && !os.IsNotExist(err) { if err = os.Rename(UserPath(u.Name), UserPath(newUserName)); err != nil && !os.IsNotExist(err) {
return fmt.Errorf("Rename user directory: %v", err) return fmt.Errorf("Rename user directory: %v", err)

View file

@ -579,14 +579,13 @@ func (g *GiteaLocalUploader) newPullRequest(pr *base.PullRequest) (*models.PullR
} }
var pullRequest = models.PullRequest{ var pullRequest = models.PullRequest{
HeadRepoID: g.repo.ID, HeadRepoID: g.repo.ID,
HeadBranch: head, HeadBranch: head,
HeadUserName: g.repoOwner, BaseRepoID: g.repo.ID,
BaseRepoID: g.repo.ID, BaseBranch: pr.Base.Ref,
BaseBranch: pr.Base.Ref, MergeBase: pr.Base.SHA,
MergeBase: pr.Base.SHA, Index: pr.Number,
Index: pr.Number, HasMerged: pr.Merged,
HasMerged: pr.Merged,
Issue: &issue, Issue: &issue,
} }

View file

@ -190,7 +190,7 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption
) )
// Get repo/branch information // Get repo/branch information
headUser, headRepo, headGitRepo, compareInfo, baseBranch, headBranch := parseCompareInfo(ctx, form) _, headRepo, headGitRepo, compareInfo, baseBranch, headBranch := parseCompareInfo(ctx, form)
if ctx.Written() { if ctx.Written() {
return return
} }
@ -265,15 +265,14 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption
DeadlineUnix: deadlineUnix, DeadlineUnix: deadlineUnix,
} }
pr := &models.PullRequest{ pr := &models.PullRequest{
HeadRepoID: headRepo.ID, HeadRepoID: headRepo.ID,
BaseRepoID: repo.ID, BaseRepoID: repo.ID,
HeadUserName: headUser.Name, HeadBranch: headBranch,
HeadBranch: headBranch, BaseBranch: baseBranch,
BaseBranch: baseBranch, HeadRepo: headRepo,
HeadRepo: headRepo, BaseRepo: repo,
BaseRepo: repo, MergeBase: compareInfo.MergeBase,
MergeBase: compareInfo.MergeBase, Type: models.PullRequestGitea,
Type: models.PullRequestGitea,
} }
// Get all assignee IDs // Get all assignee IDs

View file

@ -272,12 +272,12 @@ func checkPullInfo(ctx *context.Context) *models.Issue {
} }
func setMergeTarget(ctx *context.Context, pull *models.PullRequest) { func setMergeTarget(ctx *context.Context, pull *models.PullRequest) {
if ctx.Repo.Owner.Name == pull.HeadUserName { if ctx.Repo.Owner.Name == pull.MustHeadUserName() {
ctx.Data["HeadTarget"] = pull.HeadBranch ctx.Data["HeadTarget"] = pull.HeadBranch
} else if pull.HeadRepo == nil { } else if pull.HeadRepo == nil {
ctx.Data["HeadTarget"] = pull.HeadUserName + ":" + pull.HeadBranch ctx.Data["HeadTarget"] = pull.MustHeadUserName() + ":" + pull.HeadBranch
} else { } else {
ctx.Data["HeadTarget"] = pull.HeadUserName + "/" + pull.HeadRepo.Name + ":" + pull.HeadBranch ctx.Data["HeadTarget"] = pull.MustHeadUserName() + "/" + pull.HeadRepo.Name + ":" + pull.HeadBranch
} }
ctx.Data["BaseTarget"] = pull.BaseBranch ctx.Data["BaseTarget"] = pull.BaseBranch
} }
@ -440,7 +440,7 @@ func ViewPullCommits(ctx *context.Context) {
ctx.NotFound("ViewPullCommits", nil) ctx.NotFound("ViewPullCommits", nil)
return return
} }
ctx.Data["Username"] = pull.HeadUserName ctx.Data["Username"] = pull.MustHeadUserName()
ctx.Data["Reponame"] = pull.HeadRepo.Name ctx.Data["Reponame"] = pull.HeadRepo.Name
commits = prInfo.Commits commits = prInfo.Commits
} }
@ -512,7 +512,7 @@ func ViewPullFiles(ctx *context.Context) {
return return
} }
headRepoPath := models.RepoPath(pull.HeadUserName, pull.HeadRepo.Name) headRepoPath := pull.HeadRepo.RepoPath()
headGitRepo, err := git.OpenRepository(headRepoPath) headGitRepo, err := git.OpenRepository(headRepoPath)
if err != nil { if err != nil {
@ -531,8 +531,8 @@ func ViewPullFiles(ctx *context.Context) {
endCommitID = headCommitID endCommitID = headCommitID
gitRepo = headGitRepo gitRepo = headGitRepo
headTarget = path.Join(pull.HeadUserName, pull.HeadRepo.Name) headTarget = path.Join(pull.MustHeadUserName(), pull.HeadRepo.Name)
ctx.Data["Username"] = pull.HeadUserName ctx.Data["Username"] = pull.MustHeadUserName()
ctx.Data["Reponame"] = pull.HeadRepo.Name ctx.Data["Reponame"] = pull.HeadRepo.Name
} }
@ -754,15 +754,14 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm)
Content: form.Content, Content: form.Content,
} }
pullRequest := &models.PullRequest{ pullRequest := &models.PullRequest{
HeadRepoID: headRepo.ID, HeadRepoID: headRepo.ID,
BaseRepoID: repo.ID, BaseRepoID: repo.ID,
HeadUserName: headUser.Name, HeadBranch: headBranch,
HeadBranch: headBranch, BaseBranch: baseBranch,
BaseBranch: baseBranch, HeadRepo: headRepo,
HeadRepo: headRepo, BaseRepo: repo,
BaseRepo: repo, MergeBase: prInfo.MergeBase,
MergeBase: prInfo.MergeBase, Type: models.PullRequestGitea,
Type: models.PullRequestGitea,
} }
// FIXME: check error in the case two people send pull request at almost same time, give nice error prompt // FIXME: check error in the case two people send pull request at almost same time, give nice error prompt
// instead of 500. // instead of 500.

View file

@ -71,7 +71,7 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor
} }
}() }()
headRepoPath := models.RepoPath(pr.HeadUserName, pr.HeadRepo.Name) headRepoPath := pr.HeadRepo.RepoPath()
if err := git.InitRepository(tmpBasePath, false); err != nil { if err := git.InitRepository(tmpBasePath, false); err != nil {
return fmt.Errorf("git init: %v", err) return fmt.Errorf("git init: %v", err)
@ -306,14 +306,17 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor
} }
} }
headUser, err := models.GetUserByName(pr.HeadUserName) var headUser *models.User
err = pr.HeadRepo.GetOwner()
if err != nil { if err != nil {
if !models.IsErrUserNotExist(err) { if !models.IsErrUserNotExist(err) {
log.Error("Can't find user: %s for head repository - %v", pr.HeadUserName, err) log.Error("Can't find user: %d for head repository - %v", pr.HeadRepo.OwnerID, err)
return err return err
} }
log.Error("Can't find user: %s for head repository - defaulting to doer: %s - %v", pr.HeadUserName, doer.Name, err) log.Error("Can't find user: %d for head repository - defaulting to doer: %s - %v", pr.HeadRepo.OwnerID, doer.Name, err)
headUser = doer headUser = doer
} else {
headUser = pr.HeadRepo.Owner
} }
env = models.FullPushingEnvironment( env = models.FullPushingEnvironment(