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

* fix bug on pull requests when transfer head repository

* add migration and fix lint

* fix tests and add a cache check on LoadBaseRepo

Author: lunny

models/fixtures/pull_request.yml

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

models/migrations/migrations.go

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

models/migrations/v102.go

@@ -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 (
+	"github.com/go-xorm/xorm"
+)
+
+func dropColumnHeadUserNameOnPullRequest(x *xorm.Engine) error {
+	sess := x.NewSession()
+	defer sess.Close()
+	return dropTableColumns(sess, "pull_request", "head_user_name")
+}

models/pull.go

@@ -66,7 +66,6 @@ type PullRequest struct {
 	HeadRepo        *Repository `xorm:"-"`
 	BaseRepoID      int64       `xorm:"INDEX"`
 	BaseRepo        *Repository `xorm:"-"`
-	HeadUserName    string
 	HeadBranch      string
 	BaseBranch      string
 	ProtectedBranch *ProtectedBranch `xorm:"-"`
@@ -79,6 +78,15 @@ type PullRequest struct {
 	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.
 func (pr *PullRequest) loadAttributes(e Engine) (err error) {
 	if pr.HasMerged && pr.Merger == nil {
@@ -102,6 +110,10 @@ func (pr *PullRequest) LoadAttributes() error {
 // 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
+		}
 		var repo Repository
 		if has, err := x.ID(pr.BaseRepoID).Get(&repo); err != nil {
 			return err
@@ -113,6 +125,24 @@ func (pr *PullRequest) LoadBaseRepo() error {
 	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
 func (pr *PullRequest) LoadIssue() (err error) {
 	return pr.loadIssue(x)
@@ -152,7 +182,7 @@ func (pr *PullRequest) GetDefaultMergeMessage() string {
 			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
@@ -1159,18 +1189,6 @@ func checkForInvalidation(requests PullRequestList, repoID int64, doer *User, br
 	return nil
 }
 
-// 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,
 // and set to be either conflict or mergeable.
 func (pr *PullRequest) checkAndUpdateStatus() {

models/pull_test.go

@@ -232,20 +232,6 @@ func TestPullRequestList_LoadAttributes(t *testing.T) {
 
 // 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) {
 	assert.NoError(t, PrepareTestDatabase())
 

models/user.go

@@ -994,10 +994,6 @@ func ChangeUserName(u *User, newUserName string) (err error) {
 		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
 	if err = os.Rename(UserPath(u.Name), UserPath(newUserName)); err != nil && !os.IsNotExist(err) {
 		return fmt.Errorf("Rename user directory: %v", err)

modules/migrations/gitea.go

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

routers/api/v1/repo/pull.go

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

routers/repo/pull.go

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

services/pull/merge.go

@@ -65,7 +65,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 {
 		return fmt.Errorf("git init: %v", err)
@@ -260,14 +260,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 !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
 		}
-		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
+	} else {
+		headUser = pr.HeadRepo.Owner
 	}
 
 	env := models.FullPushingEnvironment(