Nulo/forgejo
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

[GITEA] services: Gracefully handle missing branches

Browse source 
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 <forgejo@gergo.csillger.hu>
(cherry picked from commit 8266105f24eb76b1dfb4c79d9bfde2ef9a98417a)

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 <forgejo@gergo.csillger.hu>
(cherry picked from commit e552a8fd629b11503569f605c824c1c0b01eeab2)

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 <forgejo@gergo.csillger.hu>
(cherry picked from commit e20eb7b3853e25ab29d4ca63b015517b44e4954f)

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 <forgejo@gergo.csillger.hu>
(cherry picked from commit 8c2ccfcecec6182dd80d463f58223acbf16b039b)
(cherry picked from commit 439fadf5635c47c2a1be9cc83614b60f76ac05d0)
(cherry picked from commit 44dd80552ca63c6d22f4a139a0297486f1a2e655)
(cherry picked from commit 37b91fe6f2f05feee0f8db8f44c3eaf1ff060af9)
This commit is contained in:
Gergely Nagy 2024-01-13 21:02:49 +01:00 committed by Earl Warren
parent 2cfcd266b4
commit 2924fcf814
No known key found for this signature in database
GPG key ID: 0579CB2928A78A00
2 changed files with 62 additions and 2 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
services/repository/branch.go
View file

@ -97,7 +97,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)
@ -133,7 +139,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)
}
}

54
tests/integration/repo_branch_test.go
View file

@ -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
@ -7,14 +8,21 @@ import (
"net/http"
"net/url"
"path"
"strconv"
"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"
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"
@ -159,3 +167,49 @@ 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) {
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, and that it
// reports at least three branches (master, will-be-present, and
// will-be-missing).
req := NewRequest(t, "GET", "/user2/repo1/branches")
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)
})
}