From 9466fec879f4f2c88c7c1e7a5cffba319282ab66 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 12 Apr 2024 18:11:16 +0800 Subject: [PATCH] Fix rename branch 500 when the target branch is deleted but exist in database (#30430) Fix #30428 --- models/git/branch.go | 31 ++++++-- routers/web/repo/setting/protected_branch.go | 8 +- tests/integration/rename_branch_test.go | 80 ++++++++++++++++++-- 3 files changed, 107 insertions(+), 12 deletions(-) diff --git a/models/git/branch.go b/models/git/branch.go index fa0781fed1d..2979dff3d21 100644 --- a/models/git/branch.go +++ b/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 - if err = gitAction(ctx, isDefault); err != nil { - return err - } - - // 6. insert renamed branch record + // 5. 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() } diff --git a/routers/web/repo/setting/protected_branch.go b/routers/web/repo/setting/protected_branch.go index b30dc3b0614..4bab3f897a2 100644 --- a/routers/web/repo/setting/protected_branch.go +++ b/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 { - ctx.ServerError("RenameBranch", err) + 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 } diff --git a/tests/integration/rename_branch_test.go b/tests/integration/rename_branch_test.go index 703fc243a4d..13f6cf204b5 100644 --- a/tests/integration/rename_branch_test.go +++ b/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) }