Merge pull request '[v7.0/forgejo] [PORT] gitea#30430: Fix rename branch 500 when the target branch is deleted but exist in database' (#3236) from bp-v7.0/forgejo-db39b8f into v7.0/forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3236
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

models/git/branch.go

@@ -301,6 +301,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 {
@@ -312,6 +313,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,
@@ -366,12 +385,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,
@@ -382,6 +396,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

@@ -321,6 +321,9 @@ func RenameBranchPost(ctx *context.Context) {
 		if errors.Is(err, git_model.ErrBranchIsProtected) {
 			ctx.Flash.Error(ctx.Tr("repo.settings.rename_branch_failed_protected", form.To))
 			ctx.Redirect(fmt.Sprintf("%s/branches", ctx.Repo.RepoLink))
+		} else if 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))
 		} else {
 			ctx.ServerError("RenameBranch", err)
 		}

tests/integration/rename_branch_test.go

@@ -5,6 +5,7 @@ package integration
 
 import (
 	"net/http"
+	"net/url"
 	"testing"
 
 	git_model "code.gitea.io/gitea/models/git"
@@ -17,6 +18,10 @@ import (
 )
 
 func TestRenameBranch(t *testing.T) {
+	onGiteaRun(t, testRenameBranch)
+}
+
+func testRenameBranch(t *testing.T, u *url.URL) {
 	defer tests.PrepareTestEnv(t)()
 
 	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
@@ -49,6 +54,96 @@ func TestRenameBranch(t *testing.T) {
 		assert.Equal(t, "main", repo1.DefaultBranch)
 	})
 
+	t.Run("Database syncronization", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		req := NewRequestWithValues(t, "POST", "/user2/repo1/settings/rename_branch", map[string]string{
+			"_csrf": GetCSRF(t, session, "/user2/repo1/settings/branches"),
+			"from":  "master",
+			"to":    "main",
+		})
+		session.MakeRequest(t, req, http.StatusSeeOther)
+
+		// check new branch link
+		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", nil)
+		resp := session.MakeRequest(t, req, http.StatusSeeOther)
+		location := resp.Header().Get("Location")
+		assert.Equal(t, "/user2/repo1/src/branch/main/README.md", location)
+
+		// 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": GetCSRF(t, session, "/user2/repo1/settings/branches"),
+			"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": GetCSRF(t, session, "/user2/repo1/settings/branches"),
+			"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": GetCSRF(t, session, "/user2/repo1/settings/branches"),
+			"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)
+	})
+
 	t.Run("Protected branch", func(t *testing.T) {
 		defer tests.PrintCurrentTest(t)()