Move repoWorkPool outside rename/transfer repository (#9086)

* Move repoWorkPool outside rename/transfer repository

* fix import

* Fix test

Author: lunny

models/pull.go

@@ -609,8 +609,8 @@ func (pr *PullRequest) testPatch(e Engine) (err error) {
 		return nil
 	}
 
-	repoWorkingPool.CheckIn(com.ToStr(pr.BaseRepoID))
-	defer repoWorkingPool.CheckOut(com.ToStr(pr.BaseRepoID))
+	RepoWorkingPool.CheckIn(com.ToStr(pr.BaseRepoID))
+	defer RepoWorkingPool.CheckOut(com.ToStr(pr.BaseRepoID))
 
 	log.Trace("PullRequest[%d].testPatch (patchPath): %s", pr.ID, patchPath)
 

models/repo.go

@@ -43,7 +43,8 @@ import (
 	"xorm.io/builder"
 )
 
-var repoWorkingPool = sync.NewExclusivePool()
+// RepoWorkingPool represents a working pool to order the parallel changes to the same repository
+var RepoWorkingPool = sync.NewExclusivePool()
 
 var (
 	// ErrMirrorNotExist mirror does not exist error
@@ -1655,7 +1656,7 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) error
 		return fmt.Errorf("sess.Begin: %v", err)
 	}
 
-	owner := repo.Owner
+	oldOwner := repo.Owner
 
 	// Note: we have to set value here to make sure recalculate accesses is based on
 	// new owner.
@@ -1691,8 +1692,8 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) error
 	}
 
 	// Remove old team-repository relations.
-	if owner.IsOrganization() {
-		if err = owner.removeOrgRepo(sess, repo.ID); err != nil {
+	if oldOwner.IsOrganization() {
+		if err = oldOwner.removeOrgRepo(sess, repo.ID); err != nil {
 			return fmt.Errorf("removeOrgRepo: %v", err)
 		}
 	}
@@ -1716,7 +1717,7 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) error
 	// Update repository count.
 	if _, err = sess.Exec("UPDATE `user` SET num_repos=num_repos+1 WHERE id=?", newOwner.ID); err != nil {
 		return fmt.Errorf("increase new owner repository count: %v", err)
-	} else if _, err = sess.Exec("UPDATE `user` SET num_repos=num_repos-1 WHERE id=?", owner.ID); err != nil {
+	} else if _, err = sess.Exec("UPDATE `user` SET num_repos=num_repos-1 WHERE id=?", oldOwner.ID); err != nil {
 		return fmt.Errorf("decrease old owner repository count: %v", err)
 	}
 
@@ -1725,8 +1726,8 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) error
 	}
 
 	// Remove watch for organization.
-	if owner.IsOrganization() {
-		if err = watchRepo(sess, owner.ID, repo.ID, false); err != nil {
+	if oldOwner.IsOrganization() {
+		if err = watchRepo(sess, oldOwner.ID, repo.ID, false); err != nil {
 			return fmt.Errorf("watchRepo [false]: %v", err)
 		}
 	}
@@ -1738,12 +1739,12 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) error
 		return fmt.Errorf("Failed to create dir %s: %v", dir, err)
 	}
 
-	if err = os.Rename(RepoPath(owner.Name, repo.Name), RepoPath(newOwner.Name, repo.Name)); err != nil {
+	if err = os.Rename(RepoPath(oldOwner.Name, repo.Name), RepoPath(newOwner.Name, repo.Name)); err != nil {
 		return fmt.Errorf("rename repository directory: %v", err)
 	}
 
 	// Rename remote wiki repository to new path and delete local copy.
-	wikiPath := WikiPath(owner.Name, repo.Name)
+	wikiPath := WikiPath(oldOwner.Name, repo.Name)
 	if com.IsExist(wikiPath) {
 		if err = os.Rename(wikiPath, WikiPath(newOwner.Name, repo.Name)); err != nil {
 			return fmt.Errorf("rename repository wiki: %v", err)
@@ -1755,11 +1756,16 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) error
 		return fmt.Errorf("delete repo redirect: %v", err)
 	}
 
+	if err := NewRepoRedirect(DBContext{sess}, oldOwner.ID, repo.ID, repo.Name, repo.Name); err != nil {
+		return fmt.Errorf("NewRepoRedirect: %v", err)
+	}
+
 	return sess.Commit()
 }
 
 // ChangeRepositoryName changes all corresponding setting from old repository name to new one.
 func ChangeRepositoryName(doer *User, repo *Repository, newRepoName string) (err error) {
+	oldRepoName := repo.Name
 	newRepoName = strings.ToLower(newRepoName)
 	if err = IsUsableRepoName(newRepoName); err != nil {
 		return err
@@ -1776,12 +1782,6 @@ func ChangeRepositoryName(doer *User, repo *Repository, newRepoName string) (err
 		return ErrRepoAlreadyExist{repo.Owner.Name, newRepoName}
 	}
 
-	// Change repository directory name. We must lock the local copy of the
-	// repo so that we can atomically rename the repo path and updates the
-	// local copy's origin accordingly.
-	repoWorkingPool.CheckIn(com.ToStr(repo.ID))
-	defer repoWorkingPool.CheckOut(com.ToStr(repo.ID))
-
 	newRepoPath := RepoPath(repo.Owner.Name, newRepoName)
 	if err = os.Rename(repo.RepoPath(), newRepoPath); err != nil {
 		return fmt.Errorf("rename repository directory: %v", err)
@@ -1805,6 +1805,10 @@ func ChangeRepositoryName(doer *User, repo *Repository, newRepoName string) (err
 		return fmt.Errorf("delete repo redirect: %v", err)
 	}
 
+	if err := NewRepoRedirect(DBContext{sess}, repo.Owner.ID, repo.ID, oldRepoName, newRepoName); err != nil {
+		return err
+	}
+
 	return sess.Commit()
 }
 

models/repo_redirect.go

@@ -6,8 +6,6 @@ package models
 
 import (
 	"strings"
-
-	"code.gitea.io/gitea/modules/log"
 )
 
 // RepoRedirect represents that a repo name should be redirected to another
@@ -31,36 +29,22 @@ func LookupRepoRedirect(ownerID int64, repoName string) (int64, error) {
 }
 
 // NewRepoRedirect create a new repo redirect
-func NewRepoRedirect(ownerID, repoID int64, oldRepoName, newRepoName string) error {
+func NewRepoRedirect(ctx DBContext, ownerID, repoID int64, oldRepoName, newRepoName string) error {
 	oldRepoName = strings.ToLower(oldRepoName)
 	newRepoName = strings.ToLower(newRepoName)
-	sess := x.NewSession()
-	defer sess.Close()
-
-	if err := sess.Begin(); err != nil {
-		return err
-	}
 
-	if err := deleteRepoRedirect(sess, ownerID, newRepoName); err != nil {
-		errRollback := sess.Rollback()
-		if errRollback != nil {
-			log.Error("NewRepoRedirect sess.Rollback: %v", errRollback)
-		}
+	if err := deleteRepoRedirect(ctx.e, ownerID, newRepoName); err != nil {
 		return err
 	}
 
-	if _, err := sess.Insert(&RepoRedirect{
+	if _, err := ctx.e.Insert(&RepoRedirect{
 		OwnerID:        ownerID,
 		LowerName:      oldRepoName,
 		RedirectRepoID: repoID,
 	}); err != nil {
-		errRollback := sess.Rollback()
-		if errRollback != nil {
-			log.Error("NewRepoRedirect sess.Rollback: %v", errRollback)
-		}
 		return err
 	}
-	return sess.Commit()
+	return nil
 }
 
 // deleteRepoRedirect delete any redirect from the specified repo name to

models/repo_redirect_test.go

@@ -26,7 +26,7 @@ func TestNewRepoRedirect(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 
 	repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
-	assert.NoError(t, NewRepoRedirect(repo.OwnerID, repo.ID, repo.Name, "newreponame"))
+	assert.NoError(t, NewRepoRedirect(DefaultDBContext(), repo.OwnerID, repo.ID, repo.Name, "newreponame"))
 
 	AssertExistsAndLoadBean(t, &RepoRedirect{
 		OwnerID:        repo.OwnerID,
@@ -45,7 +45,7 @@ func TestNewRepoRedirect2(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 
 	repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
-	assert.NoError(t, NewRepoRedirect(repo.OwnerID, repo.ID, repo.Name, "oldrepo1"))
+	assert.NoError(t, NewRepoRedirect(DefaultDBContext(), repo.OwnerID, repo.ID, repo.Name, "oldrepo1"))
 
 	AssertExistsAndLoadBean(t, &RepoRedirect{
 		OwnerID:        repo.OwnerID,
@@ -64,7 +64,7 @@ func TestNewRepoRedirect3(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 
 	repo := AssertExistsAndLoadBean(t, &Repository{ID: 2}).(*Repository)
-	assert.NoError(t, NewRepoRedirect(repo.OwnerID, repo.ID, repo.Name, "newreponame"))
+	assert.NoError(t, NewRepoRedirect(DefaultDBContext(), repo.OwnerID, repo.ID, repo.Name, "newreponame"))
 
 	AssertExistsAndLoadBean(t, &RepoRedirect{
 		OwnerID:        repo.OwnerID,

services/repository/transfer.go

@@ -5,10 +5,10 @@
 package repository
 
 import (
-	"fmt"
-
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/notification"
+
+	"github.com/unknwon/com"
 )
 
 // TransferOwnership transfers all corresponding setting from old user to new one.
@@ -19,13 +19,12 @@ func TransferOwnership(doer *models.User, newOwnerName string, repo *models.Repo
 
 	oldOwner := repo.Owner
 
+	models.RepoWorkingPool.CheckIn(com.ToStr(repo.ID))
 	if err := models.TransferOwnership(doer, newOwnerName, repo); err != nil {
+		models.RepoWorkingPool.CheckOut(com.ToStr(repo.ID))
 		return err
 	}
-
-	if err := models.NewRepoRedirect(oldOwner.ID, repo.ID, repo.Name, repo.Name); err != nil {
-		return fmt.Errorf("NewRepoRedirect: %v", err)
-	}
+	models.RepoWorkingPool.CheckOut(com.ToStr(repo.ID))
 
 	notification.NotifyTransferRepository(doer, repo, oldOwner.Name)
 
@@ -36,17 +35,16 @@ func TransferOwnership(doer *models.User, newOwnerName string, repo *models.Repo
 func ChangeRepositoryName(doer *models.User, repo *models.Repository, newRepoName string) error {
 	oldRepoName := repo.Name
 
-	if err := models.ChangeRepositoryName(doer, repo, newRepoName); err != nil {
-		return err
-	}
-
-	if err := repo.GetOwner(); err != nil {
-		return err
-	}
+	// Change repository directory name. We must lock the local copy of the
+	// repo so that we can atomically rename the repo path and updates the
+	// local copy's origin accordingly.
 
-	if err := models.NewRepoRedirect(repo.Owner.ID, repo.ID, oldRepoName, newRepoName); err != nil {
+	models.RepoWorkingPool.CheckIn(com.ToStr(repo.ID))
+	if err := models.ChangeRepositoryName(doer, repo, newRepoName); err != nil {
+		models.RepoWorkingPool.CheckOut(com.ToStr(repo.ID))
 		return err
 	}
+	models.RepoWorkingPool.CheckOut(com.ToStr(repo.ID))
 
 	notification.NotifyRenameRepository(doer, repo, oldRepoName)