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

diff --git a/models/fixtures/pull_request.yml b/models/fixtures/pull_request.yml
index baaaf6bb8a6da01f7ba636e32490569b33ff2e98..505584ea18daea5cd242d9e41f11dc8cd750096d 100644 (file)
--- a/models/fixtures/pull_request.yml
+++ b/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

diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go
index 19b1095cd3444f32c84373b6e8b58e47f00a8ac7..8064eccfc17e2a3ed68e50f53b7b52a2639cbff3 100644 (file)
--- a/models/migrations/migrations.go
+++ b/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

diff --git a/models/migrations/v102.go b/models/migrations/v102.go
new file mode 100644 (file)
index 0000000..74e8574
--- /dev/null
+++ b/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 (
+       "xorm.io/xorm"
+)
+
+func dropColumnHeadUserNameOnPullRequest(x *xorm.Engine) error {
+       sess := x.NewSession()
+       defer sess.Close()
+       return dropTableColumns(sess, "pull_request", "head_user_name")
+}

diff --git a/models/pull.go b/models/pull.go
index 817ea09ccad60cdf541aef1ed411aa688ea6d766..a3554a1b3da89e117266e44975d2d20c8b85fc7d 100644 (file)
--- a/models/pull.go
+++ b/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
@@ -1072,18 +1102,6 @@ func (prs PullRequestList) InvalidateCodeComments(doer *User, repo *git.Reposito
        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,
 // and set to be either conflict or mergeable.
 func (pr *PullRequest) checkAndUpdateStatus() {

diff --git a/models/pull_test.go b/models/pull_test.go
index df051d51bceac135c2bb74f4b8dffbf65ebca2b4..8e2436b1a2e655a7db497df6dee5b1021376ff7a 100644 (file)
--- a/models/pull_test.go
+++ b/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())
 

diff --git a/models/user.go b/models/user.go
index c393d8dce500c8d01c916e3bec7e2f28a8441393..a3679c9a5b01f1473bd6b14274a5fdf006b7ea00 100644 (file)
--- a/models/user.go
+++ b/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)

diff --git a/modules/migrations/gitea.go b/modules/migrations/gitea.go
index 2452a7a88322f8ea6f43e4f2b405ce8b209b28a2..a095751c6b1c116826a4d264375a71b4cf63de69 100644 (file)
--- a/modules/migrations/gitea.go
+++ b/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,
        }

diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go
index 978c8a3f1f4911890d1c06a7424156be937d43c2..16ddd10c60f49cb669c7e86a500505ca39712830 100644 (file)
--- a/routers/api/v1/repo/pull.go
+++ b/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

diff --git a/routers/repo/pull.go b/routers/repo/pull.go
index 8b97e55670987bd2f1095831f650dfbc8eb3674c..4001e3a4f80e7bc705c11607a99f6b10ceeeba0a 100644 (file)
--- a/routers/repo/pull.go
+++ b/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.

diff --git a/services/pull/merge.go b/services/pull/merge.go
index 0d762dbc30d02f50b70e8050b1c644691a4ce6bd..5eb8eaa4d4701e4818cbdddb06880a9dd5b983fc 100644 (file)
--- a/services/pull/merge.go
+++ b/services/pull/merge.go
@@ -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 {
                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 !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(