Also sync DB branches on push if necessary #28361

lunny · 2023-12-05T08:28:02Z

This PR will check whether the repo has zero branch when pushing a branch. If that, it means this repository hasn't been synced.

The reason caused that is after user upgrade from v1.20 -> v1.21, he just push branches without visit the repository user interface. Because all repositories routers will check whether a branches sync is necessary but push has not such check.

For every repository, it has two states, synced or not synced. If there is zero branch for a repository, then it will be assumed as non-sync state. Otherwise, it's synced state. So if we think it's synced, we just need to update branch/insert new branch. Otherwise do a full sync. So that, for every push, there will be almost no extra load added. It's high performance than yours.

For the implementation, we in fact will try to update the branch first, if updated success with affect records > 0, then all are done. Because that means the branch has been in the database. If no record is affected, that means the branch does not exist in database. So there are two possibilities. One is this is a new branch, then we just need to insert the record. Another is the branches haven't been synced, then we need to sync all the branches into database.

…ly but not visit UI after upgrading from v1.20 -> v1.21

models/git/branch.go

modules/repository/branch.go

lunny · 2023-12-07T07:44:22Z

@wolfogre done.

models/db/context.go

services/repository/push.go

delvh · 2023-12-08T12:01:21Z

modules/repository/branch.go

+// UpdateBranch updates the branch information in the database. If the branch exist, it will update latest commit of this branch information
+// If it doest not exist, insert a new record into database
+func UpdateBranch(ctx context.Context, repoID, pusherID int64, branchName string, commit *git.Commit) error {
+	cnt, err := git_model.UpdateBranch(ctx, repoID, pusherID, branchName, commit)


That function is completely wrong here.

Why do you think that?

OK. I move the function to service layer and renamed function name to syncBranchToDB. Maybe it fixed your problem?

delvh · 2023-12-08T12:11:07Z

modules/repository/branch.go

+	// if user haven't visit UI but directly push to a branch after upgrading from 1.20 -> 1.21,
+	// we cannot simply insert the branch but need to check we have branches or not
+	hasBranch, err := db.Exist[git_model.Branch](ctx, git_model.FindBranchOptions{
+		RepoID:          repoID,
+		IsDeletedBranch: util.OptionalBoolFalse,
+	}.ToConds())
+	if err != nil {
+		return err
+	}
+	if !hasBranch {
+		if _, err = SyncRepoBranches(ctx, repoID, pusherID); err != nil {
+			return fmt.Errorf("repo_module.SyncRepoBranches %d:%s failed: %v", repoID, branchName, err)
+		}
+		return nil
+	}


I have a lot of trouble understanding this precondition: I don't think this is the correct behavior we are doing here.
Shouldn't the workflow be:
Push -> Add all branches added by the push if they don't exist already?

Your idea is OK to sync. But it will compare all branches every time. It will become a performance bottleneck for big repositories.

My method is for every repository, it has two states, synced or not synced. If there is zero branch for a repository, then it's non-sync state. otherwise, it's synced state. So if we think it's synced, we just need to update branch/insert new branch. Otherwise do a full sync. So that, for every push, there will be almost no extra load added. It's high performance than yours.

GiteaBot · 2023-12-09T13:31:01Z

I was unable to create a backport for 1.21. @lunny, please send one manually. 🍵

go run ./contrib/backport 28361
...  // fix git conflicts if any
go run ./contrib/backport --continue

Fix go-gitea#28056 This PR will check whether the repo has zero branch when pushing a branch. If that, it means this repository hasn't been synced. The reason caused that is after user upgrade from v1.20 -> v1.21, he just push branches without visit the repository user interface. Because all repositories routers will check whether a branches sync is necessary but push has not such check. For every repository, it has two states, synced or not synced. If there is zero branch for a repository, then it will be assumed as non-sync state. Otherwise, it's synced state. So if we think it's synced, we just need to update branch/insert new branch. Otherwise do a full sync. So that, for every push, there will be almost no extra load added. It's high performance than yours. For the implementation, we in fact will try to update the branch first, if updated success with affect records > 0, then all are done. Because that means the branch has been in the database. If no record is affected, that means the branch does not exist in database. So there are two possibilities. One is this is a new branch, then we just need to insert the record. Another is the branches haven't been synced, then we need to sync all the branches into database.

* giteaofficial/main: [skip ci] Updated licenses and gitignores Actually recover from a panic in cron task (go-gitea#28409) Fix missing check (go-gitea#28406) Also sync DB branches on push if necessary (go-gitea#28361) Remove stale since giteabot has similiar feature (go-gitea#28401) [skip ci] Updated translations via Crowdin

Fix #28056 Backport #28361 This PR will check whether the repo has zero branch when pushing a branch. If that, it means this repository hasn't been synced. The reason caused that is after user upgrade from v1.20 -> v1.21, he just push branches without visit the repository user interface. Because all repositories routers will check whether a branches sync is necessary but push has not such check. For every repository, it has two states, synced or not synced. If there is zero branch for a repository, then it will be assumed as non-sync state. Otherwise, it's synced state. So if we think it's synced, we just need to update branch/insert new branch. Otherwise do a full sync. So that, for every push, there will be almost no extra load added. It's high performance than yours. For the implementation, we in fact will try to update the branch first, if updated success with affect records > 0, then all are done. Because that means the branch has been in the database. If no record is affected, that means the branch does not exist in database. So there are two possibilities. One is this is a new branch, then we just need to insert the record. Another is the branches haven't been synced, then we need to sync all the branches into database.

kdumontnu · 2023-12-22T21:38:36Z

@lunny There is a pretty significant performance regression in this PR.

Most integration tests creating a branch now take minutes to complete.

You can see the effect of this in CI runtime. In a PR merged before this one, sqlite integration tests take 16min

In this PR and after, sqlite integration tests take >30min

lunny · 2023-12-22T23:53:59Z

I will investigate it.

#28361 introduced `syncBranchToDB` in `CreateNewBranchFromCommit`. This PR will revert the change because it's unnecessary. Every push will already be checked by `syncBranchToDB`. This PR also created a test to ensure it's right.

go-gitea#28361 introduced `syncBranchToDB` in `CreateNewBranchFromCommit`. This PR will revert the change because it's unnecessary. Every push will already be checked by `syncBranchToDB`. This PR also created a test to ensure it's right.

Replace #28625 Backport #28624 by lunny #28361 introduced `syncBranchToDB` in `CreateNewBranchFromCommit`. This PR will revert the change because it's unnecessary. Every push will already be checked by `syncBranchToDB`. This PR also created a test to ensure it's right.

Fix go-gitea#28056 This PR will check whether the repo has zero branch when pushing a branch. If that, it means this repository hasn't been synced. The reason caused that is after user upgrade from v1.20 -> v1.21, he just push branches without visit the repository user interface. Because all repositories routers will check whether a branches sync is necessary but push has not such check. For every repository, it has two states, synced or not synced. If there is zero branch for a repository, then it will be assumed as non-sync state. Otherwise, it's synced state. So if we think it's synced, we just need to update branch/insert new branch. Otherwise do a full sync. So that, for every push, there will be almost no extra load added. It's high performance than yours. For the implementation, we in fact will try to update the branch first, if updated success with affect records > 0, then all are done. Because that means the branch has been in the database. If no record is affected, that means the branch does not exist in database. So there are two possibilities. One is this is a new branch, then we just need to insert the record. Another is the branches haven't been synced, then we need to sync all the branches into database.

go-gitea#28361 introduced `syncBranchToDB` in `CreateNewBranchFromCommit`. This PR will revert the change because it's unnecessary. Every push will already be checked by `syncBranchToDB`. This PR also created a test to ensure it's right.

Fix go-gitea#28056 This PR will check whether the repo has zero branch when pushing a branch. If that, it means this repository hasn't been synced. The reason caused that is after user upgrade from v1.20 -> v1.21, he just push branches without visit the repository user interface. Because all repositories routers will check whether a branches sync is necessary but push has not such check. For every repository, it has two states, synced or not synced. If there is zero branch for a repository, then it will be assumed as non-sync state. Otherwise, it's synced state. So if we think it's synced, we just need to update branch/insert new branch. Otherwise do a full sync. So that, for every push, there will be almost no extra load added. It's high performance than yours. For the implementation, we in fact will try to update the branch first, if updated success with affect records > 0, then all are done. Because that means the branch has been in the database. If no record is affected, that means the branch does not exist in database. So there are two possibilities. One is this is a new branch, then we just need to insert the record. Another is the branches haven't been synced, then we need to sync all the branches into database.

go-gitea#28361 introduced `syncBranchToDB` in `CreateNewBranchFromCommit`. This PR will revert the change because it's unnecessary. Every push will already be checked by `syncBranchToDB`. This PR also created a test to ensure it's right.

Fix the possible branches sync failure when user push branches direct…

1856b43

…ly but not visit UI after upgrading from v1.20 -> v1.21

lunny added type/bug backport/v1.21 This PR should be backported to Gitea 1.21 labels Dec 5, 2023

lunny added this to the 1.22.0 milestone Dec 5, 2023

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 5, 2023

pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 5, 2023

lunny added 3 commits December 5, 2023 01:26

Merge branch 'main' into lunny/fix_branch_sync

083ab55

Fix lint

a036d14

Fix bugs

f413a5f

pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 5, 2023

github-actions bot added the modifies/api This PR adds API routes or modifies them label Dec 5, 2023

wolfogre reviewed Dec 7, 2023

View reviewed changes

models/git/branch.go Outdated Show resolved Hide resolved

modules/repository/branch.go Outdated Show resolved Hide resolved

lunny added 3 commits December 6, 2023 23:35

Merge branch 'main' into lunny/fix_branch_sync

c28c7c5

Merge branch 'main' into lunny/fix_branch_sync

18646a2

Use db.Exist instead of db.Count

a3d8eb1

Exist doesn't need to check empty condition

e6146c0

wolfogre approved these changes Dec 7, 2023

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 7, 2023

lunny mentioned this pull request Dec 7, 2023

1.21.2 changelog #28387

Merged

delvh changed the title ~~Fix the possible branches sync failure when user push branches directly but not visit UI after upgrading from v1.20 -> v1.21~~ Also sync DB branches on push if necessary Dec 8, 2023

delvh requested changes Dec 8, 2023

View reviewed changes

GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 8, 2023

lunny added 4 commits December 9, 2023 15:02

Move function to service layer

dc7828e

Merge branch 'main' into lunny/fix_branch_sync

68698fe

rename update branch function to syncBranchToDB

c6874c7

Fix bug

72669dc

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Dec 9, 2023

wxiaoguang approved these changes Dec 9, 2023

View reviewed changes

lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 9, 2023

lunny enabled auto-merge (squash) December 9, 2023 12:57

Merge branch 'main' into lunny/fix_branch_sync

f4f4bc5

lunny merged commit aeb3830 into go-gitea:main Dec 9, 2023
25 checks passed

GiteaBot added backport/manual No power to the bots! Create your backport yourself! and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Dec 9, 2023

lunny deleted the lunny/fix_branch_sync branch December 9, 2023 14:23

lunny mentioned this pull request Dec 9, 2023

Also sync DB branches on push if necessary (#28361) #28403

Merged

lunny added the backport/done All backports for this PR have been created label Dec 9, 2023

lunny mentioned this pull request Dec 28, 2023

Remove unnecessary syncbranchToDB with tests #28624

Merged

GiteaBot mentioned this pull request Dec 28, 2023

Remove unnecessary syncbranchToDB with tests (#28624) #28625

Closed

wxiaoguang mentioned this pull request Dec 28, 2023

Remove unnecessary syncbranchToDB with tests (#28624) #28629

Merged

seeplusplus mentioned this pull request Feb 4, 2024

New branches no longer sync to DB after upgrading from 1.20.4 to 1.21.5. #29052

Closed

go-gitea locked as resolved and limited conversation to collaborators Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Also sync DB branches on push if necessary #28361

Also sync DB branches on push if necessary #28361

lunny commented Dec 5, 2023 •

edited

Loading

lunny commented Dec 7, 2023

delvh Dec 8, 2023

lunny Dec 9, 2023

lunny Dec 9, 2023

delvh Dec 8, 2023

lunny Dec 9, 2023 •

edited

Loading

GiteaBot commented Dec 9, 2023

kdumontnu commented Dec 22, 2023

lunny commented Dec 22, 2023

Also sync DB branches on push if necessary #28361

Also sync DB branches on push if necessary #28361

Conversation

lunny commented Dec 5, 2023 • edited Loading

lunny commented Dec 7, 2023

delvh Dec 8, 2023

Choose a reason for hiding this comment

lunny Dec 9, 2023

Choose a reason for hiding this comment

lunny Dec 9, 2023

Choose a reason for hiding this comment

delvh Dec 8, 2023

Choose a reason for hiding this comment

lunny Dec 9, 2023 • edited Loading

Choose a reason for hiding this comment

GiteaBot commented Dec 9, 2023

kdumontnu commented Dec 22, 2023

lunny commented Dec 22, 2023

lunny commented Dec 5, 2023 •

edited

Loading

lunny Dec 9, 2023 •

edited

Loading