apply suggestions from code review

Author: a1012112796

models/branches.go

@@ -600,10 +600,11 @@ func RemoveOldDeletedBranches(ctx context.Context, olderThan time.Duration) {
 // RenamedBranch proivde renamed branch log
 // will check it when an branch can't be found
 type RenamedBranch struct {
-	ID     int64 `xorm:"pk autoincr"`
-	RepoID int64 `xorm:"INDEX NOT NULL"`
-	From   string
-	To     string
+	ID          int64 `xorm:"pk autoincr"`
+	RepoID      int64 `xorm:"INDEX NOT NULL"`
+	From        string
+	To          string
+	CreatedUnix timeutil.TimeStamp `xorm:"created"`
 }
 
 // FindRenamedBranch check if a branch was renamed
@@ -638,19 +639,13 @@ func (repo *Repository) RenameBranch(from, to string, gitAction func(isDefault b
 	// 2. Update protected branch if needed
 	protectedBranch, err := getProtectedBranchBy(sess, repo.ID, from)
 	if err != nil {
-		if err2 := sess.Rollback(); err2 != nil {
-			log.Error("RenameBranch: sess.Rollback: %v", err2)
-		}
 		return err
 	}
 
 	if protectedBranch != nil {
 		protectedBranch.BranchName = to
 		_, err = sess.ID(protectedBranch.ID).Cols("branch_name").Update(protectedBranch)
 		if err != nil {
-			if err2 := sess.Rollback(); err2 != nil {
-				log.Error("RenameBranch: sess.Rollback: %v", err2)
-			}
 			return err
 		}
 	}
@@ -660,18 +655,11 @@ func (repo *Repository) RenameBranch(from, to string, gitAction func(isDefault b
 		repo.ID, from, false).
 		Update(map[string]interface{}{"base_branch": to})
 	if err != nil {
-		if err2 := sess.Rollback(); err2 != nil {
-			log.Error("RenameBranch: sess.Rollback: %v", err2)
-		}
 		return err
 	}
 
 	// 4. do git action
 	if err = gitAction(isDefault); err != nil {
-		if err2 := sess.Rollback(); err2 != nil {
-			log.Error("RenameBranch: sess.Rollback: %v", err2)
-		}
-
 		return err
 	}
 
@@ -681,13 +669,8 @@ func (repo *Repository) RenameBranch(from, to string, gitAction func(isDefault b
 		From:   from,
 		To:     to,
 	}
-
 	_, err = sess.Insert(renamedBranch)
 	if err != nil {
-		if err2 := sess.Rollback(); err2 != nil {
-			log.Error("RenameBranch: sess.Rollback: %v", err2)
-		}
-
 		return err
 	}
 

models/migrations/v180.go

@@ -10,10 +10,11 @@ import (
 
 func addRenamedBranchTable(x *xorm.Engine) error {
 	type RenamedBranch struct {
-		ID     int64 `xorm:"pk autoincr"`
-		RepoID int64 `xorm:"INDEX NOT NULL"`
-		From   string
-		To     string
+		ID          int64 `xorm:"pk autoincr"`
+		RepoID      int64 `xorm:"INDEX NOT NULL"`
+		From        string
+		To          string
+		CreatedUnix int64 `xorm:"created"`
 	}
 	return x.Sync2(new(RenamedBranch))
 }

modules/context/repo.go

@@ -49,25 +49,23 @@ type PullRequest struct {
 // Repository contains information to operate a repository
 type Repository struct {
 	models.Permission
-	IsWatching        bool
-	IsViewBranch      bool
-	IsViewTag         bool
-	IsViewCommit      bool
-	Repository        *models.Repository
-	Owner             *models.User
-	Commit            *git.Commit
-	Tag               *git.Tag
-	GitRepo           *git.Repository
-	BranchName        string
-	IsRenamedBranch   bool
-	RenamedBranchName string
-	TagName           string
-	TreePath          string
-	CommitID          string
-	RepoLink          string
-	CloneLink         models.CloneLink
-	CommitsCount      int64
-	Mirror            *models.Mirror
+	IsWatching   bool
+	IsViewBranch bool
+	IsViewTag    bool
+	IsViewCommit bool
+	Repository   *models.Repository
+	Owner        *models.User
+	Commit       *git.Commit
+	Tag          *git.Tag
+	GitRepo      *git.Repository
+	BranchName   string
+	TagName      string
+	TreePath     string
+	CommitID     string
+	RepoLink     string
+	CloneLink    models.CloneLink
+	CommitsCount int64
+	Mirror       *models.Mirror
 
 	PullRequest *PullRequest
 }
@@ -717,8 +715,8 @@ func getRefName(ctx *Context, pathType RepoRefType) string {
 					return false
 				}
 
-				ctx.Repo.IsRenamedBranch = true
-				ctx.Repo.RenamedBranchName = b.To
+				ctx.Data["IsRenamedBranch"] = true
+				ctx.Data["RenamedBranchName"] = b.To
 
 				return true
 			})
@@ -803,9 +801,11 @@ func RepoRefByType(refType RepoRefType, ignoreNotExistErr ...bool) func(*Context
 		} else {
 			refName = getRefName(ctx, refType)
 			ctx.Repo.BranchName = refName
-			if ctx.Repo.IsRenamedBranch {
-				ctx.Flash.Info(ctx.Tr("repo.branch.renamed", refName, ctx.Repo.RenamedBranchName))
-				link := strings.Replace(ctx.Req.RequestURI, refName, ctx.Repo.RenamedBranchName, 1)
+			isRenamedBranch, has := ctx.Data["IsRenamedBranch"].(bool)
+			if isRenamedBranch && has {
+				renamedBranchName := ctx.Data["RenamedBranchName"].(string)
+				ctx.Flash.Info(ctx.Tr("repo.branch.renamed", refName, renamedBranchName))
+				link := strings.Replace(ctx.Req.RequestURI, refName, renamedBranchName, 1)
 				ctx.Redirect(link)
 				return
 			}

routers/repo/setting_protected_branch.go

@@ -15,11 +15,11 @@ import (
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
-	"code.gitea.io/gitea/modules/notification"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/services/forms"
 	pull_service "code.gitea.io/gitea/services/pull"
+	"code.gitea.io/gitea/services/repository"
 )
 
 // ProtectedBranch render the page to protect the repository
@@ -301,44 +301,24 @@ func RenameBranchPost(ctx *context.Context) {
 		return
 	}
 
-	if form.From == form.To {
-		ctx.Flash.Error(ctx.Tr("repo.settings.rename_branch_failed_exist", form.To))
-		ctx.Redirect(fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink))
+	msg, err := repository.RenameBranch(ctx.Repo.Repository, ctx.User, ctx.Repo.GitRepo, form.From, form.To)
+	if err != nil {
+		ctx.ServerError("RenameBranch", err)
 		return
 	}
 
-	if ctx.Repo.GitRepo.IsBranchExist(form.To) {
+	if msg == "target_exist" {
 		ctx.Flash.Error(ctx.Tr("repo.settings.rename_branch_failed_exist", form.To))
 		ctx.Redirect(fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink))
+		return
 	}
 
-	if !ctx.Repo.GitRepo.IsBranchExist(form.From) {
+	if msg == "from_not_exist" {
 		ctx.Flash.Error(ctx.Tr("repo.settings.rename_branch_failed_not_exist", form.From))
 		ctx.Redirect(fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink))
-	}
-
-	if err := ctx.Repo.Repository.RenameBranch(form.From, form.To, func(isDefault bool) error {
-		err2 := ctx.Repo.GitRepo.RenameBranch(form.From, form.To)
-		if err2 != nil {
-			return err2
-		}
-
-		if isDefault {
-			err2 = ctx.Repo.GitRepo.SetDefaultBranch(form.To)
-			if err2 != nil {
-				return err2
-			}
-		}
-
-		return nil
-	}); err != nil {
-		ctx.ServerError("RenameBranch", err)
 		return
 	}
 
-	notification.NotifyDeleteRef(ctx.User, ctx.Repo.Repository, "branch", "refs/heads/"+form.From)
-	notification.NotifyCreateRef(ctx.User, ctx.Repo.Repository, "branch", "refs/heads/"+form.To)
-
 	ctx.Flash.Success(ctx.Tr("repo.settings.rename_branch_success", form.From, form.To))
 	ctx.Redirect(fmt.Sprintf("%s/settings/branches", ctx.Repo.RepoLink))
 }

services/repository/branch.go

@@ -0,0 +1,49 @@
+// Copyright 2021 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 repository
+
+import (
+	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/notification"
+)
+
+// RenameBranch rename a branch
+func RenameBranch(repo *models.Repository, doer *models.User, gitRepo *git.Repository, from, to string) (string, error) {
+	if from == to {
+		return "target_exist", nil
+	}
+
+	if gitRepo.IsBranchExist(to) {
+		return "target_exist", nil
+	}
+
+	if !gitRepo.IsBranchExist(from) {
+		return "from_not_exist", nil
+	}
+
+	if err := repo.RenameBranch(from, to, func(isDefault bool) error {
+		err2 := gitRepo.RenameBranch(from, to)
+		if err2 != nil {
+			return err2
+		}
+
+		if isDefault {
+			err2 = gitRepo.SetDefaultBranch(to)
+			if err2 != nil {
+				return err2
+			}
+		}
+
+		return nil
+	}); err != nil {
+		return "", err
+	}
+
+	notification.NotifyDeleteRef(doer, repo, "branch", "refs/heads/"+from)
+	notification.NotifyCreateRef(doer, repo, "branch", "refs/heads/"+to)
+
+	return "", nil
+}