Fix rename branch 500 when the target branch is deleted but exist in database (#30430) (#30438)

Backport #30430 by @lunny

Fix #30428

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

models/git/branch.go

@@ -297,6 +297,7 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, from, to str
 
 	sess := db.GetEngine(ctx)
 
+	// check whether from branch exist
 	var branch Branch
 	exist, err := db.GetEngine(ctx).Where("repo_id=? AND name=?", repo.ID, from).Get(&branch)
 	if err != nil {
@@ -308,6 +309,24 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, from, to str
 		}
 	}
 
+	// check whether to branch exist or is_deleted
+	var dstBranch Branch
+	exist, err = db.GetEngine(ctx).Where("repo_id=? AND name=?", repo.ID, to).Get(&dstBranch)
+	if err != nil {
+		return err
+	}
+	if exist {
+		if !dstBranch.IsDeleted {
+			return ErrBranchAlreadyExists{
+				BranchName: to,
+			}
+		}
+
+		if _, err := db.GetEngine(ctx).ID(dstBranch.ID).NoAutoCondition().Delete(&dstBranch); err != nil {
+			return err
+		}
+	}
+
 	// 1. update branch in database
 	if n, err := sess.Where("repo_id=? AND name=?", repo.ID, from).Update(&Branch{
 		Name: to,
@@ -362,12 +381,7 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, from, to str
 		return err
 	}
 
-	// 5. do git action
+	// 5. insert renamed branch record
-	if err = gitAction(ctx, isDefault); err != nil {
-		return err
-	}
-
-	// 6. insert renamed branch record
 	renamedBranch := &RenamedBranch{
 		RepoID: repo.ID,
 		From:   from,
@@ -378,6 +392,11 @@ func RenameBranch(ctx context.Context, repo *repo_model.Repository, from, to str
 		return err
 	}
 
+	// 6. do git action
+	if err = gitAction(ctx, isDefault); err != nil {
+		return err
+	}
+
 	return committer.Commit()
 }
 

routers/web/repo/setting/protected_branch.go

@@ -313,7 +313,13 @@ func RenameBranchPost(ctx *context.Context) {
 
 	msg, err := repository.RenameBranch(ctx, ctx.Repo.Repository, ctx.Doer, ctx.Repo.GitRepo, form.From, form.To)
 	if err != nil {
+		switch {
+		case git_model.IsErrBranchAlreadyExists(err):
+			ctx.Flash.Error(ctx.Tr("repo.branch.branch_already_exists", form.To))
+			ctx.Redirect(fmt.Sprintf("%s/branches", ctx.Repo.RepoLink))
+		default:
 			ctx.ServerError("RenameBranch", err)
+		}
 		return
 	}
 

tests/integration/rename_branch_test.go

@@ -5,17 +5,23 @@ package integration
 
 import (
 	"net/http"
+	"net/url"
 	"testing"
 
 	git_model "code.gitea.io/gitea/models/git"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unittest"
+	gitea_context "code.gitea.io/gitea/services/context"
 	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
 )
 
 func TestRenameBranch(t *testing.T) {
+	onGiteaRun(t, testRenameBranch)
+}
+
+func testRenameBranch(t *testing.T, u *url.URL) {
 	defer tests.PrepareTestEnv(t)()
 
 	unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: 1, Name: "master"})
@@ -26,20 +32,19 @@ func TestRenameBranch(t *testing.T) {
 	resp := session.MakeRequest(t, req, http.StatusOK)
 	htmlDoc := NewHTMLParser(t, resp.Body)
 
-	postData := map[string]string{
+	req = NewRequestWithValues(t, "POST", "/user2/repo1/settings/rename_branch", map[string]string{
 		"_csrf": htmlDoc.GetCSRF(),
 		"from":  "master",
 		"to":    "main",
-	}
+	})
-	req = NewRequestWithValues(t, "POST", "/user2/repo1/settings/rename_branch", postData)
 	session.MakeRequest(t, req, http.StatusSeeOther)
 
 	// check new branch link
-	req = NewRequestWithValues(t, "GET", "/user2/repo1/src/branch/main/README.md", postData)
+	req = NewRequestWithValues(t, "GET", "/user2/repo1/src/branch/main/README.md", nil)
 	session.MakeRequest(t, req, http.StatusOK)
 
 	// check old branch link
-	req = NewRequestWithValues(t, "GET", "/user2/repo1/src/branch/master/README.md", postData)
+	req = NewRequestWithValues(t, "GET", "/user2/repo1/src/branch/master/README.md", nil)
 	resp = session.MakeRequest(t, req, http.StatusSeeOther)
 	location := resp.Header().Get("Location")
 	assert.Equal(t, "/user2/repo1/src/branch/main/README.md", location)
@@ -47,4 +52,69 @@ func TestRenameBranch(t *testing.T) {
 	// check db
 	repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
 	assert.Equal(t, "main", repo1.DefaultBranch)
+
+	// create branch1
+	csrf := GetCSRF(t, session, "/user2/repo1/src/branch/main")
+
+	req = NewRequestWithValues(t, "POST", "/user2/repo1/branches/_new/branch/main", map[string]string{
+		"_csrf":           csrf,
+		"new_branch_name": "branch1",
+	})
+	session.MakeRequest(t, req, http.StatusSeeOther)
+
+	branch1 := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "branch1"})
+	assert.Equal(t, "branch1", branch1.Name)
+
+	// create branch2
+	req = NewRequestWithValues(t, "POST", "/user2/repo1/branches/_new/branch/main", map[string]string{
+		"_csrf":           csrf,
+		"new_branch_name": "branch2",
+	})
+	session.MakeRequest(t, req, http.StatusSeeOther)
+
+	branch2 := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "branch2"})
+	assert.Equal(t, "branch2", branch2.Name)
+
+	// rename branch2 to branch1
+	req = NewRequestWithValues(t, "POST", "/user2/repo1/settings/rename_branch", map[string]string{
+		"_csrf": htmlDoc.GetCSRF(),
+		"from":  "branch2",
+		"to":    "branch1",
+	})
+	session.MakeRequest(t, req, http.StatusSeeOther)
+	flashCookie := session.GetCookie(gitea_context.CookieNameFlash)
+	assert.NotNil(t, flashCookie)
+	assert.Contains(t, flashCookie.Value, "error")
+
+	branch2 = unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "branch2"})
+	assert.Equal(t, "branch2", branch2.Name)
+	branch1 = unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "branch1"})
+	assert.Equal(t, "branch1", branch1.Name)
+
+	// delete branch1
+	req = NewRequestWithValues(t, "POST", "/user2/repo1/branches/delete", map[string]string{
+		"_csrf": htmlDoc.GetCSRF(),
+		"name":  "branch1",
+	})
+	session.MakeRequest(t, req, http.StatusOK)
+	branch2 = unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "branch2"})
+	assert.Equal(t, "branch2", branch2.Name)
+	branch1 = unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "branch1"})
+	assert.True(t, branch1.IsDeleted) // virtual deletion
+
+	// rename branch2 to branch1 again
+	req = NewRequestWithValues(t, "POST", "/user2/repo1/settings/rename_branch", map[string]string{
+		"_csrf": htmlDoc.GetCSRF(),
+		"from":  "branch2",
+		"to":    "branch1",
+	})
+	session.MakeRequest(t, req, http.StatusSeeOther)
+
+	flashCookie = session.GetCookie(gitea_context.CookieNameFlash)
+	assert.NotNil(t, flashCookie)
+	assert.Contains(t, flashCookie.Value, "success")
+
+	unittest.AssertNotExistsBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "branch2"})
+	branch1 = unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: repo1.ID, Name: "branch1"})
+	assert.Equal(t, "branch1", branch1.Name)
 }