From fffd9bb7d5424cd9c2887597950bd6566db77c32 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sat, 13 Jan 2024 21:02:49 +0100 Subject: [PATCH 1/4] services: in loadOneBranch, return if CountDivergingCommits fail If we can't count the number of diverging commits for one reason or another (such as the branch being in the database, but missing from disk), rather than logging an error and continuing into a crash (because `divergence` will be nil), return an error instead. Signed-off-by: Gergely Nagy --- services/repository/branch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/repository/branch.go b/services/repository/branch.go index d11038bfe8..472e3521b8 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -136,7 +136,7 @@ func loadOneBranch(ctx context.Context, repo *repo_model.Repository, dbBranch *g var err error divergence, err = files_service.CountDivergingCommits(ctx, repo, git.BranchPrefix+branchName) if err != nil { - log.Error("CountDivergingCommits: %v", err) + return nil, fmt.Errorf("CountDivergingCommits: %v", err) } } From c2fa9c308f5cdb08dd84fb8ec6623a57e75d5152 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sat, 13 Jan 2024 21:05:21 +0100 Subject: [PATCH 2/4] services: Gracefully handle missing branches When loading branches, if loading one fails, log an error, and ignore the branch, rather than returning and causing an internal server error. Ideally, we would only ignore the error if it was caused by a missing branch, and do it silently, like the respective API endpoint does. However, veryfing that at this place is not very practical, so for the time being, ignore any and all branch loading errors. Signed-off-by: Gergely Nagy --- services/repository/branch.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/services/repository/branch.go b/services/repository/branch.go index 472e3521b8..567104d29f 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -100,7 +100,13 @@ func LoadBranches(ctx context.Context, repo *repo_model.Repository, gitRepo *git for i := range dbBranches { branch, err := loadOneBranch(ctx, repo, dbBranches[i], &rules, repoIDToRepo, repoIDToGitRepo) if err != nil { - return nil, nil, 0, fmt.Errorf("loadOneBranch: %v", err) + log.Error("loadOneBranch() on repo #%d, branch '%s' failed: %v", repo.ID, dbBranches[i].Name, err) + + // TODO: Ideally, we would only do this if the branch doesn't exist + // anymore. That is not practical to check here currently, so we do + // this for all kinds of errors. + totalNumOfBranches-- + continue } branches = append(branches, branch) From 754f97b1e2b73f6b4cbdea8b2322f00ac56a9b82 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sat, 13 Jan 2024 23:07:12 +0100 Subject: [PATCH 3/4] tests: Add a testcase for missing branches This tests the scenario reported in Codeberg/Community#1408: a branch that is recorded in the database, but missing on disk was causing internal server errors. With recent changes, that is no longer the case, the error is logged and then ignored. This test case tests this behaviour, that the repo's branches page on the web UI functions even if the git branch is missing. Signed-off-by: Gergely Nagy --- tests/integration/repo_branch_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/integration/repo_branch_test.go b/tests/integration/repo_branch_test.go index 9eb17eda00..851abf95ca 100644 --- a/tests/integration/repo_branch_test.go +++ b/tests/integration/repo_branch_test.go @@ -1,4 +1,5 @@ // Copyright 2017 The Gitea Authors. All rights reserved. +// Copyright 2024 The Forgejo Authors c/o Codeberg e.V.. All rights reserved. // SPDX-License-Identifier: MIT package integration @@ -10,8 +11,11 @@ import ( "strings" "testing" + "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" + repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" + "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/translation" @@ -159,3 +163,23 @@ func TestCreateBranchInvalidCSRF(t *testing.T) { strings.TrimSpace(htmlDoc.doc.Find(".ui.message").Text()), ) } + +func TestDatabaseMissingABranch(t *testing.T) { + onGiteaRun(t, func(t *testing.T, URL *url.URL) { + session := loginUser(t, "user2") + + // Create two branches + testCreateBranch(t, session, "user2", "repo1", "branch/master", "will-be-present", http.StatusSeeOther) + testCreateBranch(t, session, "user2", "repo1", "branch/master", "will-be-missing", http.StatusSeeOther) + + // Delete one branch from git only, leaving it in the database + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + cmd := git.NewCommand(db.DefaultContext, "branch", "-D").AddDynamicArguments("will-be-missing") + _, _, err := cmd.RunStdString(&git.RunOpts{Dir: repo.RepoPath()}) + assert.NoError(t, err) + + // Verify that loading the repo's branches page works still + req := NewRequest(t, "GET", "/user2/repo1/branches") + session.MakeRequest(t, req, http.StatusOK) + }) +} From 49efd192e99cbd0cab0b622067a97c0c701cc61f Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Sun, 14 Jan 2024 21:44:26 +0100 Subject: [PATCH 4/4] tests: More testing in TestDatabaseMissingABranch In the `TestDatabaseMissingABranch` testcase, make sure that the branches are in sync between the db and git before deleting a branch via git, then compare the branch count from the web UI, making sure that it returns an out-of-sync value first, and the correct one after another sync. This is currently tested by scraping the UI, and relies on the fact that the branch counter is out of date before syncing. If that issue gets resolved, we'll have to adjust the test to verify the sync another way. Signed-off-by: Gergely Nagy --- tests/integration/repo_branch_test.go | 34 +++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/tests/integration/repo_branch_test.go b/tests/integration/repo_branch_test.go index 851abf95ca..d6b2b39d9a 100644 --- a/tests/integration/repo_branch_test.go +++ b/tests/integration/repo_branch_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/url" "path" + "strconv" "strings" "testing" @@ -15,10 +16,13 @@ import ( git_model "code.gitea.io/gitea/models/git" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/translation" + repo_service "code.gitea.io/gitea/services/repository" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -166,20 +170,46 @@ func TestCreateBranchInvalidCSRF(t *testing.T) { func TestDatabaseMissingABranch(t *testing.T) { onGiteaRun(t, func(t *testing.T, URL *url.URL) { + adminUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{IsAdmin: true}) session := loginUser(t, "user2") // Create two branches testCreateBranch(t, session, "user2", "repo1", "branch/master", "will-be-present", http.StatusSeeOther) testCreateBranch(t, session, "user2", "repo1", "branch/master", "will-be-missing", http.StatusSeeOther) + // Run the repo branch sync, to ensure the db and git agree. + err2 := repo_service.AddAllRepoBranchesToSyncQueue(graceful.GetManager().ShutdownContext(), adminUser.ID) + assert.NoError(t, err2) + // Delete one branch from git only, leaving it in the database repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) cmd := git.NewCommand(db.DefaultContext, "branch", "-D").AddDynamicArguments("will-be-missing") _, _, err := cmd.RunStdString(&git.RunOpts{Dir: repo.RepoPath()}) assert.NoError(t, err) - // Verify that loading the repo's branches page works still + // Verify that loading the repo's branches page works still, and that it + // reports at least three branches (master, will-be-present, and + // will-be-missing). req := NewRequest(t, "GET", "/user2/repo1/branches") - session.MakeRequest(t, req, http.StatusOK) + resp := session.MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) + firstBranchCount, _ := strconv.Atoi(doc.Find(".repository-menu a[href*='/branches'] b").Text()) + assert.GreaterOrEqual(t, firstBranchCount, 3) + + // Run the repo branch sync again + err2 = repo_service.AddAllRepoBranchesToSyncQueue(graceful.GetManager().ShutdownContext(), adminUser.ID) + assert.NoError(t, err2) + + // Verify that loading the repo's branches page works still, and that it + // reports one branch less than the first time. + // + // NOTE: This assumes that the branch counter on the web UI is out of + // date before the sync. If that problem gets resolved, we'll have to + // find another way to test that the syncing works. + req = NewRequest(t, "GET", "/user2/repo1/branches") + resp = session.MakeRequest(t, req, http.StatusOK) + doc = NewHTMLParser(t, resp.Body) + secondBranchCount, _ := strconv.Atoi(doc.Find(".repository-menu a[href*='/branches'] b").Text()) + assert.Equal(t, firstBranchCount-1, secondBranchCount) }) }