[GITEA] Remove redundant syncBranchToDB

- The transaction in combination with Git push was causing deadlocks if
you had the `push_update` queue set to `immediate`. This was the root
cause of slow integration tests in CI.
- Remove the sync branch code as this is already being done in the Git
post-receive hook.
- Add tests to proof the branch models are in sync even with this code
removed.

Backport of https://codeberg.org/forgejo/forgejo/pulls/1962

(cherry picked from commit a064065cb9)

services/repository/branch.go

@@ -281,28 +281,18 @@ func CreateNewBranchFromCommit(ctx context.Context, doer *user_model.User, repo
 		return err
 	}
 
-	return db.WithTx(ctx, func(ctx context.Context) error {
+	if err := git.Push(ctx, repo.RepoPath(), git.PushOptions{
-		commit, err := gitRepo.GetCommit(commitID)
+		Remote: repo.RepoPath(),
-		if err != nil {
+		Branch: fmt.Sprintf("%s:%s%s", commitID, git.BranchPrefix, branchName),
-			return err
+		Env:    repo_module.PushingEnvironment(doer, repo),
-		}
+	}); err != nil {
-		// database operation should be done before git operation so that we can rollback if git operation failed
+		if git.IsErrPushOutOfDate(err) || git.IsErrPushRejected(err) {
-		if err := syncBranchToDB(ctx, repo.ID, doer.ID, branchName, commit); err != nil {
 			return err
 		}
+		return fmt.Errorf("push: %w", err)
+	}
 
-		if err := git.Push(ctx, repo.RepoPath(), git.PushOptions{
+	return nil
-			Remote: repo.RepoPath(),
-			Branch: fmt.Sprintf("%s:%s%s", commitID, git.BranchPrefix, branchName),
-			Env:    repo_module.PushingEnvironment(doer, repo),
-		}); err != nil {
-			if git.IsErrPushOutOfDate(err) || git.IsErrPushRejected(err) {
-				return err
-			}
-			return fmt.Errorf("push: %w", err)
-		}
-		return nil
-	})
 }
 
 // RenameBranch rename a branch

tests/integration/branches_test.go

@@ -4,72 +4,55 @@
 package integration
 
 import (
+	"fmt"
 	"net/http"
 	"net/url"
 	"testing"
 
-	"code.gitea.io/gitea/modules/translation"
+	git_model "code.gitea.io/gitea/models/git"
-	"code.gitea.io/gitea/tests"
+	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/models/unittest"
+	gitea_context "code.gitea.io/gitea/modules/context"
 
 	"github.com/stretchr/testify/assert"
 )
 
-func TestViewBranches(t *testing.T) {
+func TestBranchActions(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
-	req := NewRequest(t, "GET", "/user2/repo1/branches")
-	resp := MakeRequest(t, req, http.StatusOK)
-
-	htmlDoc := NewHTMLParser(t, resp.Body)
-	_, exists := htmlDoc.doc.Find(".delete-branch-button").Attr("data-url")
-	assert.False(t, exists, "The template has changed")
-}
-
-func TestDeleteBranch(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
-	deleteBranch(t)
-}
-
-func TestUndoDeleteBranch(t *testing.T) {
 	onGiteaRun(t, func(t *testing.T, u *url.URL) {
-		deleteBranch(t)
+		session := loginUser(t, "user2")
-		htmlDoc, name := branchAction(t, ".restore-branch-button")
+		repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
-		assert.Contains(t,
+		branch3 := unittest.AssertExistsAndLoadBean(t, &git_model.Branch{ID: 3, RepoID: repo1.ID})
-			htmlDoc.doc.Find(".ui.positive.message").Text(),
+		branchesLink := repo1.FullName() + "/branches"
-			translation.NewLocale("en-US").Tr("repo.branch.restore_success", name),
+
-		)
+		t.Run("View", func(t *testing.T) {
+			req := NewRequest(t, "GET", branchesLink)
+			MakeRequest(t, req, http.StatusOK)
+		})
+
+		t.Run("Delete branch", func(t *testing.T) {
+			link := fmt.Sprintf("/%s/branches/delete?name=%s", repo1.FullName(), branch3.Name)
+			req := NewRequestWithValues(t, "POST", link, map[string]string{
+				"_csrf": GetCSRF(t, session, branchesLink),
+			})
+			session.MakeRequest(t, req, http.StatusOK)
+			flashCookie := session.GetCookie(gitea_context.CookieNameFlash)
+			assert.NotNil(t, flashCookie)
+			assert.Contains(t, flashCookie.Value, "success%3DBranch%2B%2522branch2%2522%2Bhas%2Bbeen%2Bdeleted.")
+
+			assert.True(t, unittest.AssertExistsAndLoadBean(t, &git_model.Branch{ID: 3, RepoID: repo1.ID}).IsDeleted)
+		})
+
+		t.Run("Restore branch", func(t *testing.T) {
+			link := fmt.Sprintf("/%s/branches/restore?branch_id=%d&name=%s", repo1.FullName(), branch3.ID, branch3.Name)
+			req := NewRequestWithValues(t, "POST", link, map[string]string{
+				"_csrf": GetCSRF(t, session, branchesLink),
+			})
+			session.MakeRequest(t, req, http.StatusOK)
+			flashCookie := session.GetCookie(gitea_context.CookieNameFlash)
+			assert.NotNil(t, flashCookie)
+			assert.Contains(t, flashCookie.Value, "success%3DBranch%2B%2522branch2%2522%2Bhas%2Bbeen%2Brestored")
+
+			assert.False(t, unittest.AssertExistsAndLoadBean(t, &git_model.Branch{ID: 3, RepoID: repo1.ID}).IsDeleted)
+		})
 	})
 }
-
-func deleteBranch(t *testing.T) {
-	htmlDoc, name := branchAction(t, ".delete-branch-button")
-	assert.Contains(t,
-		htmlDoc.doc.Find(".ui.positive.message").Text(),
-		translation.NewLocale("en-US").Tr("repo.branch.deletion_success", name),
-	)
-}
-
-func branchAction(t *testing.T, button string) (*HTMLDoc, string) {
-	session := loginUser(t, "user2")
-	req := NewRequest(t, "GET", "/user2/repo1/branches")
-	resp := session.MakeRequest(t, req, http.StatusOK)
-
-	htmlDoc := NewHTMLParser(t, resp.Body)
-	link, exists := htmlDoc.doc.Find(button).Attr("data-url")
-	if !assert.True(t, exists, "The template has changed") {
-		t.Skip()
-	}
-
-	req = NewRequestWithValues(t, "POST", link, map[string]string{
-		"_csrf": htmlDoc.GetCSRF(),
-	})
-	session.MakeRequest(t, req, http.StatusOK)
-
-	url, err := url.Parse(link)
-	assert.NoError(t, err)
-	req = NewRequest(t, "GET", "/user2/repo1/branches")
-	resp = session.MakeRequest(t, req, http.StatusOK)
-
-	return NewHTMLParser(t, resp.Body), url.Query().Get("name")
-}

tests/integration/repo_branch_test.go

@@ -10,6 +10,8 @@ import (
 	"strings"
 	"testing"
 
+	git_model "code.gitea.io/gitea/models/git"
+	"code.gitea.io/gitea/models/unittest"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/test"
 	"code.gitea.io/gitea/modules/translation"
@@ -47,12 +49,14 @@ func testCreateBranches(t *testing.T, giteaURL *url.URL) {
 		CreateRelease  string
 		FlashMessage   string
 		ExpectedStatus int
+		CheckBranch    bool
 	}{
 		{
 			OldRefSubURL:   "branch/master",
 			NewBranch:      "feature/test1",
 			ExpectedStatus: http.StatusSeeOther,
 			FlashMessage:   translation.NewLocale("en-US").Tr("repo.branch.create_success", "feature/test1"),
+			CheckBranch:    true,
 		},
 		{
 			OldRefSubURL:   "branch/master",
@@ -65,6 +69,7 @@ func testCreateBranches(t *testing.T, giteaURL *url.URL) {
 			NewBranch:      "feature=test1",
 			ExpectedStatus: http.StatusSeeOther,
 			FlashMessage:   translation.NewLocale("en-US").Tr("repo.branch.create_success", "feature=test1"),
+			CheckBranch:    true,
 		},
 		{
 			OldRefSubURL:   "branch/master",
@@ -94,6 +99,7 @@ func testCreateBranches(t *testing.T, giteaURL *url.URL) {
 			NewBranch:      "feature/test3",
 			ExpectedStatus: http.StatusSeeOther,
 			FlashMessage:   translation.NewLocale("en-US").Tr("repo.branch.create_success", "feature/test3"),
+			CheckBranch:    true,
 		},
 		{
 			OldRefSubURL:   "branch/master",
@@ -108,10 +114,15 @@ func testCreateBranches(t *testing.T, giteaURL *url.URL) {
 			CreateRelease:  "v1.0.1",
 			ExpectedStatus: http.StatusSeeOther,
 			FlashMessage:   translation.NewLocale("en-US").Tr("repo.branch.create_success", "feature/test4"),
+			CheckBranch:    true,
 		},
 	}
+
+	session := loginUser(t, "user2")
 	for _, test := range tests {
-		session := loginUser(t, "user2")
+		if test.CheckBranch {
+			unittest.AssertNotExistsBean(t, &git_model.Branch{RepoID: 1, Name: test.NewBranch})
+		}
 		if test.CreateRelease != "" {
 			createNewRelease(t, session, "/user2/repo1", test.CreateRelease, test.CreateRelease, false, false)
 		}
@@ -125,6 +136,9 @@ func testCreateBranches(t *testing.T, giteaURL *url.URL) {
 				test.FlashMessage,
 			)
 		}
+		if test.CheckBranch {
+			unittest.AssertExistsAndLoadBean(t, &git_model.Branch{RepoID: 1, Name: test.NewBranch})
+		}
 	}
 }