From 7bf297237927bda843ca8bac040b914e66a8074e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 26 Sep 2025 08:15:42 -0700 Subject: [PATCH] Move GetDiverging functions to gitrepo (#35524) Extracted from #35469 --------- Co-authored-by: wxiaoguang --- models/issues/pull.go | 9 +- models/migrations/v1_12/v136.go | 12 +-- modules/git/repo.go | 30 ------- modules/git/repo_test.go | 24 ----- modules/gitrepo/compare.go | 44 ++++++++++ modules/gitrepo/compare_test.go | 42 +++++++++ modules/gitrepo/gitrepo.go | 6 +- modules/gitrepo/main_test.go | 32 +++++++ routers/web/repo/issue_comment.go | 4 +- services/pull/check.go | 4 +- services/pull/pull.go | 87 +++++++++---------- services/pull/update.go | 41 +++++---- services/repository/branch.go | 17 ++-- services/repository/files/commit.go | 10 --- .../integration/change_default_branch_test.go | 8 +- tests/integration/pull_update_test.go | 26 ++++-- 16 files changed, 220 insertions(+), 176 deletions(-) create mode 100644 modules/gitrepo/compare.go create mode 100644 modules/gitrepo/compare_test.go create mode 100644 modules/gitrepo/main_test.go diff --git a/models/issues/pull.go b/models/issues/pull.go index 7a37b627e1b..fb7dff3cc9e 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -417,10 +417,6 @@ func (pr *PullRequest) GetGitHeadRefName() string { return fmt.Sprintf("%s%d/head", git.PullPrefix, pr.Index) } -func (pr *PullRequest) GetGitHeadBranchRefName() string { - return fmt.Sprintf("%s%s", git.BranchPrefix, pr.HeadBranch) -} - // GetReviewCommentsCount returns the number of review comments made on the diff of a PR review (not including comments on commits or issues in a PR) func (pr *PullRequest) GetReviewCommentsCount(ctx context.Context) int { opts := FindCommentsOptions{ @@ -646,9 +642,8 @@ func (pr *PullRequest) UpdateCols(ctx context.Context, cols ...string) error { } // UpdateColsIfNotMerged updates specific fields of a pull request if it has not been merged -func (pr *PullRequest) UpdateColsIfNotMerged(ctx context.Context, cols ...string) error { - _, err := db.GetEngine(ctx).Where("id = ? AND has_merged = ?", pr.ID, false).Cols(cols...).Update(pr) - return err +func (pr *PullRequest) UpdateColsIfNotMerged(ctx context.Context, cols ...string) (int64, error) { + return db.GetEngine(ctx).Where("id = ? AND has_merged = ?", pr.ID, false).Cols(cols...).Update(pr) } // IsWorkInProgress determine if the Pull Request is a Work In Progress by its title diff --git a/models/migrations/v1_12/v136.go b/models/migrations/v1_12/v136.go index 0f53278b462..20b892b6cc5 100644 --- a/models/migrations/v1_12/v136.go +++ b/models/migrations/v1_12/v136.go @@ -6,11 +6,10 @@ package v1_12 import ( "fmt" "math" - "path/filepath" - "strings" "time" - "code.gitea.io/gitea/modules/git" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -85,12 +84,9 @@ func AddCommitDivergenceToPulls(x *xorm.Engine) error { log.Error("Missing base repo with id %d for PR ID %d", pr.BaseRepoID, pr.ID) continue } - userPath := filepath.Join(setting.RepoRootPath, strings.ToLower(baseRepo.OwnerName)) - repoPath := filepath.Join(userPath, strings.ToLower(baseRepo.Name)+".git") - + repoStore := repo_model.StorageRepo(repo_model.RelativePath(baseRepo.OwnerName, baseRepo.Name)) gitRefName := fmt.Sprintf("refs/pull/%d/head", pr.Index) - - divergence, err := git.GetDivergingCommits(graceful.GetManager().HammerContext(), repoPath, pr.BaseBranch, gitRefName) + divergence, err := gitrepo.GetDivergingCommits(graceful.GetManager().HammerContext(), repoStore, pr.BaseBranch, gitRefName) if err != nil { log.Warn("Could not recalculate Divergence for pull: %d", pr.ID) pr.CommitsAhead = 0 diff --git a/modules/git/repo.go b/modules/git/repo.go index 38cb4592a04..9f8b6225c8f 100644 --- a/modules/git/repo.go +++ b/modules/git/repo.go @@ -243,36 +243,6 @@ func GetLatestCommitTime(ctx context.Context, repoPath string) (time.Time, error return time.Parse("Mon Jan _2 15:04:05 2006 -0700", commitTime) } -// DivergeObject represents commit count diverging commits -type DivergeObject struct { - Ahead int - Behind int -} - -// GetDivergingCommits returns the number of commits a targetBranch is ahead or behind a baseBranch -func GetDivergingCommits(ctx context.Context, repoPath, baseBranch, targetBranch string) (do DivergeObject, err error) { - cmd := gitcmd.NewCommand("rev-list", "--count", "--left-right"). - AddDynamicArguments(baseBranch + "..." + targetBranch).AddArguments("--") - stdout, _, err := cmd.RunStdString(ctx, &gitcmd.RunOpts{Dir: repoPath}) - if err != nil { - return do, err - } - left, right, found := strings.Cut(strings.Trim(stdout, "\n"), "\t") - if !found { - return do, fmt.Errorf("git rev-list output is missing a tab: %q", stdout) - } - - do.Behind, err = strconv.Atoi(left) - if err != nil { - return do, err - } - do.Ahead, err = strconv.Atoi(right) - if err != nil { - return do, err - } - return do, nil -} - // CreateBundle create bundle content to the target path func (repo *Repository) CreateBundle(ctx context.Context, commit string, out io.Writer) error { tmp, cleanup, err := setting.AppDataTempDir("git-repo-content").MkdirTempRandom("gitea-bundle") diff --git a/modules/git/repo_test.go b/modules/git/repo_test.go index 813844d1ea1..26ee3a091a2 100644 --- a/modules/git/repo_test.go +++ b/modules/git/repo_test.go @@ -29,27 +29,3 @@ func TestRepoIsEmpty(t *testing.T) { assert.NoError(t, err) assert.True(t, isEmpty) } - -func TestRepoGetDivergingCommits(t *testing.T) { - bareRepo1Path := filepath.Join(testReposDir, "repo1_bare") - do, err := GetDivergingCommits(t.Context(), bareRepo1Path, "master", "branch2") - assert.NoError(t, err) - assert.Equal(t, DivergeObject{ - Ahead: 1, - Behind: 5, - }, do) - - do, err = GetDivergingCommits(t.Context(), bareRepo1Path, "master", "master") - assert.NoError(t, err) - assert.Equal(t, DivergeObject{ - Ahead: 0, - Behind: 0, - }, do) - - do, err = GetDivergingCommits(t.Context(), bareRepo1Path, "master", "test") - assert.NoError(t, err) - assert.Equal(t, DivergeObject{ - Ahead: 0, - Behind: 2, - }, do) -} diff --git a/modules/gitrepo/compare.go b/modules/gitrepo/compare.go new file mode 100644 index 00000000000..1c8f5421fa7 --- /dev/null +++ b/modules/gitrepo/compare.go @@ -0,0 +1,44 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package gitrepo + +import ( + "context" + "fmt" + "strconv" + "strings" + + "code.gitea.io/gitea/modules/git/gitcmd" +) + +// DivergeObject represents commit count diverging commits +type DivergeObject struct { + Ahead int + Behind int +} + +// GetDivergingCommits returns the number of commits a targetBranch is ahead or behind a baseBranch +func GetDivergingCommits(ctx context.Context, repo Repository, baseBranch, targetBranch string) (*DivergeObject, error) { + cmd := gitcmd.NewCommand("rev-list", "--count", "--left-right"). + AddDynamicArguments(baseBranch + "..." + targetBranch).AddArguments("--") + stdout, _, err1 := cmd.RunStdString(ctx, &gitcmd.RunOpts{Dir: repoPath(repo)}) + if err1 != nil { + return nil, err1 + } + + left, right, found := strings.Cut(strings.Trim(stdout, "\n"), "\t") + if !found { + return nil, fmt.Errorf("git rev-list output is missing a tab: %q", stdout) + } + + behind, err := strconv.Atoi(left) + if err != nil { + return nil, err + } + ahead, err := strconv.Atoi(right) + if err != nil { + return nil, err + } + return &DivergeObject{Ahead: ahead, Behind: behind}, nil +} diff --git a/modules/gitrepo/compare_test.go b/modules/gitrepo/compare_test.go new file mode 100644 index 00000000000..f8661d94121 --- /dev/null +++ b/modules/gitrepo/compare_test.go @@ -0,0 +1,42 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package gitrepo + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +type mockRepository struct { + path string +} + +func (r *mockRepository) RelativePath() string { + return r.path +} + +func TestRepoGetDivergingCommits(t *testing.T) { + repo := &mockRepository{path: "repo1_bare"} + do, err := GetDivergingCommits(t.Context(), repo, "master", "branch2") + assert.NoError(t, err) + assert.Equal(t, &DivergeObject{ + Ahead: 1, + Behind: 5, + }, do) + + do, err = GetDivergingCommits(t.Context(), repo, "master", "master") + assert.NoError(t, err) + assert.Equal(t, &DivergeObject{ + Ahead: 0, + Behind: 0, + }, do) + + do, err = GetDivergingCommits(t.Context(), repo, "master", "test") + assert.NoError(t, err) + assert.Equal(t, &DivergeObject{ + Ahead: 0, + Behind: 2, + }, do) +} diff --git a/modules/gitrepo/gitrepo.go b/modules/gitrepo/gitrepo.go index fad8f70c4cb..59d23235992 100644 --- a/modules/gitrepo/gitrepo.go +++ b/modules/gitrepo/gitrepo.go @@ -20,9 +20,9 @@ type Repository interface { RelativePath() string // We don't assume how the directory structure of the repository is, so we only need the relative path } -// RelativePath should be an unix style path like username/reponame.git -// This method should change it according to the current OS. -func repoPath(repo Repository) string { +// repoPath resolves the Repository.RelativePath (which is a unix-style path like "username/reponame.git") +// to a local filesystem path according to setting.RepoRootPath +var repoPath = func(repo Repository) string { return filepath.Join(setting.RepoRootPath, filepath.FromSlash(repo.RelativePath())) } diff --git a/modules/gitrepo/main_test.go b/modules/gitrepo/main_test.go new file mode 100644 index 00000000000..6e6636ce770 --- /dev/null +++ b/modules/gitrepo/main_test.go @@ -0,0 +1,32 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package gitrepo + +import ( + "os" + "path/filepath" + "testing" + + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/tempdir" + "code.gitea.io/gitea/modules/test" +) + +func TestMain(m *testing.M) { + gitHomePath, cleanup, err := tempdir.OsTempDir("gitea-test").MkdirTempRandom("git-home") + if err != nil { + log.Fatal("Unable to create temp dir: %v", err) + } + defer cleanup() + + // resolve repository path relative to the test directory + testRootDir := test.SetupGiteaRoot() + repoPath = func(repo Repository) string { + return filepath.Join(testRootDir, "/modules/git/tests/repos", repo.RelativePath()) + } + + setting.Git.HomePath = gitHomePath + os.Exit(m.Run()) +} diff --git a/routers/web/repo/issue_comment.go b/routers/web/repo/issue_comment.go index 592d1fbde06..4e7f245296c 100644 --- a/routers/web/repo/issue_comment.go +++ b/routers/web/repo/issue_comment.go @@ -126,8 +126,8 @@ func NewComment(ctx *context.Context) { ctx.JSONError("The origin branch is delete, cannot reopen.") return } - headBranchRef := pull.GetGitHeadBranchRefName() - headBranchCommitID, err := git.GetFullCommitID(ctx, pull.HeadRepo.RepoPath(), headBranchRef) + headBranchRef := git.RefNameFromBranch(pull.HeadBranch) + headBranchCommitID, err := git.GetFullCommitID(ctx, pull.HeadRepo.RepoPath(), headBranchRef.String()) if err != nil { ctx.ServerError("Get head commit Id of head branch fail", err) return diff --git a/services/pull/check.go b/services/pull/check.go index c2ee49b6bb4..088fd3702c6 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -50,7 +50,7 @@ var ( func markPullRequestStatusAsChecking(ctx context.Context, pr *issues_model.PullRequest) bool { pr.Status = issues_model.PullRequestStatusChecking - err := pr.UpdateColsIfNotMerged(ctx, "status") + _, err := pr.UpdateColsIfNotMerged(ctx, "status") if err != nil { log.Error("UpdateColsIfNotMerged failed, pr: %-v, err: %v", pr, err) return false @@ -256,7 +256,7 @@ func markPullRequestAsMergeable(ctx context.Context, pr *issues_model.PullReques return } - if err := pr.UpdateColsIfNotMerged(ctx, "merge_base", "status", "conflicted_files", "changed_protected_files"); err != nil { + if _, err := pr.UpdateColsIfNotMerged(ctx, "merge_base", "status", "conflicted_files", "changed_protected_files"); err != nil { log.Error("Update[%-v]: %v", pr, err) } diff --git a/services/pull/pull.go b/services/pull/pull.go index c7b3a0d523d..b64a846adc0 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -99,13 +99,6 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { return err } - divergence, err := git.GetDivergingCommits(ctx, prCtx.tmpBasePath, baseBranch, trackingBranch) - if err != nil { - return err - } - pr.CommitsAhead = divergence.Ahead - pr.CommitsBehind = divergence.Behind - assigneeCommentMap := make(map[int64]*issues_model.Comment) var reviewNotifiers []*issue_service.ReviewRequestNotifier @@ -134,6 +127,12 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { return err } + // Update Commit Divergence + err = syncCommitDivergence(ctx, pr) + if err != nil { + return err + } + // add first push codes comment if _, err := CreatePushPullComment(ctx, issue.Poster, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitHeadRefName(), false); err != nil { return err @@ -287,25 +286,21 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer pr.Status = issues_model.PullRequestStatusMergeable } - // Update Commit Divergence - divergence, err := GetDiverging(ctx, pr) - if err != nil { - return err - } - pr.CommitsAhead = divergence.Ahead - pr.CommitsBehind = divergence.Behind - // add first push codes comment - baseGitRepo, err := gitrepo.OpenRepository(ctx, pr.BaseRepo) - if err != nil { - return err - } - defer baseGitRepo.Close() - return db.WithTx(ctx, func(ctx context.Context) error { - if err := pr.UpdateColsIfNotMerged(ctx, "merge_base", "status", "conflicted_files", "changed_protected_files", "base_branch", "commits_ahead", "commits_behind"); err != nil { + // The UPDATE acquires the transaction lock, if the UPDATE succeeds, it should have updated one row (the "base_branch" is changed) + // If no row is updated, it means the PR has been merged or closed in the meantime + updated, err := pr.UpdateColsIfNotMerged(ctx, "merge_base", "status", "conflicted_files", "changed_protected_files", "base_branch") + if err != nil { return err } + if updated == 0 { + return util.ErrorWrap(util.ErrInvalidArgument, "pull request status has changed") + } + + if err := syncCommitDivergence(ctx, pr); err != nil { + return fmt.Errorf("syncCommitDivergence: %w", err) + } // Create comment options := &issues_model.CreateCommentOptions{ @@ -372,15 +367,21 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { // If you don't let it run all the way then you will lose data // TODO: graceful: AddTestPullRequestTask needs to become a queue! + repo, err := repo_model.GetRepositoryByID(ctx, opts.RepoID) + if err != nil { + log.Error("GetRepositoryByID: %v", err) + return + } // GetUnmergedPullRequestsByHeadInfo() only return open and unmerged PR. - prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, opts.RepoID, opts.Branch) + headBranchPRs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, opts.RepoID, opts.Branch) if err != nil { log.Error("Find pull requests [head_repo_id: %d, head_branch: %s]: %v", opts.RepoID, opts.Branch, err) return } - for _, pr := range prs { + for _, pr := range headBranchPRs { log.Trace("Updating PR[%d]: composing new test task", pr.ID) + pr.HeadRepo = repo // avoid loading again if pr.Flow == issues_model.PullRequestFlowGithub { if err := PushToBaseRepo(ctx, pr); err != nil { log.Error("PushToBaseRepo: %v", err) @@ -398,14 +399,14 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { } if opts.IsSync { - if err = prs.LoadAttributes(ctx); err != nil { + if err = headBranchPRs.LoadAttributes(ctx); err != nil { log.Error("PullRequestList.LoadAttributes: %v", err) } - if invalidationErr := checkForInvalidation(ctx, prs, opts.RepoID, opts.Doer, opts.Branch); invalidationErr != nil { + if invalidationErr := checkForInvalidation(ctx, headBranchPRs, opts.RepoID, opts.Doer, opts.Branch); invalidationErr != nil { log.Error("checkForInvalidation: %v", invalidationErr) } if err == nil { - for _, pr := range prs { + for _, pr := range headBranchPRs { objectFormat := git.ObjectFormatFromName(pr.BaseRepo.ObjectFormatName) if opts.NewCommitID != "" && opts.NewCommitID != objectFormat.EmptyObjectID().String() { changed, err := checkIfPRContentChanged(ctx, pr, opts.OldCommitID, opts.NewCommitID) @@ -432,14 +433,8 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, opts.NewCommitID); err != nil { log.Error("MarkReviewsAsNotStale: %v", err) } - divergence, err := GetDiverging(ctx, pr) - if err != nil { - log.Error("GetDiverging: %v", err) - } else { - err = pr.UpdateCommitDivergence(ctx, divergence.Ahead, divergence.Behind) - if err != nil { - log.Error("UpdateCommitDivergence: %v", err) - } + if err = syncCommitDivergence(ctx, pr); err != nil { + log.Error("syncCommitDivergence: %v", err) } } @@ -459,24 +454,22 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { } log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", opts.RepoID, opts.Branch) - prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, opts.RepoID, opts.Branch) + // The base repositories of baseBranchPRs are the same one (opts.RepoID) + baseBranchPRs, err := issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, opts.RepoID, opts.Branch) if err != nil { log.Error("Find pull requests [base_repo_id: %d, base_branch: %s]: %v", opts.RepoID, opts.Branch, err) return } - for _, pr := range prs { - divergence, err := GetDiverging(ctx, pr) + for _, pr := range baseBranchPRs { + pr.BaseRepo = repo // avoid loading again + err = syncCommitDivergence(ctx, pr) if err != nil { - if git_model.IsErrBranchNotExist(err) && !gitrepo.IsBranchExist(ctx, pr.HeadRepo, pr.HeadBranch) { - log.Warn("Cannot test PR %s/%d: head_branch %s no longer exists", pr.BaseRepo.Name, pr.IssueID, pr.HeadBranch) + if errors.Is(err, util.ErrNotExist) { + log.Warn("Cannot test PR %s/%d with base=%s head=%s: no longer exists", pr.BaseRepo.FullName(), pr.IssueID, pr.BaseBranch, pr.HeadBranch) } else { - log.Error("GetDiverging: %v", err) - } - } else { - err = pr.UpdateCommitDivergence(ctx, divergence.Ahead, divergence.Behind) - if err != nil { - log.Error("UpdateCommitDivergence: %v", err) + log.Error("syncCommitDivergence: %v", err) } + continue } StartPullRequestCheckDelayable(ctx, pr) } @@ -486,7 +479,7 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { // checkIfPRContentChanged checks if diff to target branch has changed by push // A commit can be considered to leave the PR untouched if the patch/diff with its merge base is unchanged func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest, oldCommitID, newCommitID string) (hasChanged bool, err error) { - prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr) + prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr) // FIXME: why it still needs to create a temp repo, since the alongside calls like GetDiverging doesn't do so anymore if err != nil { log.Error("CreateTemporaryRepoForPR %-v: %v", pr, err) return false, err diff --git a/services/pull/update.go b/services/pull/update.go index b8f84e3d658..cce39374516 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -14,7 +14,7 @@ import ( repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/globallock" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/repository" @@ -34,17 +34,21 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. } defer releaser() - diffCount, err := GetDiverging(ctx, pr) - if err != nil { - return err - } else if diffCount.Behind == 0 { - return fmt.Errorf("HeadBranch of PR %d is up to date", pr.Index) - } - if err := pr.LoadBaseRepo(ctx); err != nil { log.Error("unable to load BaseRepo for %-v during update-by-merge: %v", pr, err) return fmt.Errorf("unable to load BaseRepo for PR[%d] during update-by-merge: %w", pr.ID, err) } + + // TODO: FakePR: if the PR is a fake PR (for example: from Merge Upstream), then no need to check diverging + if pr.ID > 0 { + diffCount, err := gitrepo.GetDivergingCommits(ctx, pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName()) + if err != nil { + return err + } else if diffCount.Behind == 0 { + return fmt.Errorf("HeadBranch of PR %d is up to date", pr.Index) + } + } + if err := pr.LoadHeadRepo(ctx); err != nil { log.Error("unable to load HeadRepo for PR %-v during update-by-merge: %v", pr, err) return fmt.Errorf("unable to load HeadRepo for PR[%d] during update-by-merge: %w", pr.ID, err) @@ -172,18 +176,13 @@ func IsUserAllowedToUpdate(ctx context.Context, pull *issues_model.PullRequest, return mergeAllowed, rebaseAllowed, nil } -// GetDiverging determines how many commits a PR is ahead or behind the PR base branch -func GetDiverging(ctx context.Context, pr *issues_model.PullRequest) (*git.DivergeObject, error) { - log.Trace("GetDiverging[%-v]: compare commits", pr) - prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr) - if err != nil { - if !git_model.IsErrBranchNotExist(err) { - log.Error("CreateTemporaryRepoForPR %-v: %v", pr, err) - } - return nil, err +func syncCommitDivergence(ctx context.Context, pr *issues_model.PullRequest) error { + if err := pr.LoadBaseRepo(ctx); err != nil { + return err } - defer cancel() - - diff, err := git.GetDivergingCommits(ctx, prCtx.tmpBasePath, baseBranch, trackingBranch) - return &diff, err + divergence, err := gitrepo.GetDivergingCommits(ctx, pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName()) + if err != nil { + return err + } + return pr.UpdateCommitDivergence(ctx, divergence.Ahead, divergence.Behind) } diff --git a/services/repository/branch.go b/services/repository/branch.go index df7202227aa..1a34121d990 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -33,7 +33,6 @@ import ( actions_service "code.gitea.io/gitea/services/actions" notify_service "code.gitea.io/gitea/services/notify" release_service "code.gitea.io/gitea/services/release" - files_service "code.gitea.io/gitea/services/repository/files" "xorm.io/builder" ) @@ -123,9 +122,9 @@ func getDivergenceCacheKey(repoID int64, branchName string) string { } // getDivergenceFromCache gets the divergence from cache -func getDivergenceFromCache(repoID int64, branchName string) (*git.DivergeObject, bool) { +func getDivergenceFromCache(repoID int64, branchName string) (*gitrepo.DivergeObject, bool) { data, ok := cache.GetCache().Get(getDivergenceCacheKey(repoID, branchName)) - res := git.DivergeObject{ + res := gitrepo.DivergeObject{ Ahead: -1, Behind: -1, } @@ -139,7 +138,7 @@ func getDivergenceFromCache(repoID int64, branchName string) (*git.DivergeObject return &res, true } -func putDivergenceFromCache(repoID int64, branchName string, divergence *git.DivergeObject) error { +func putDivergenceFromCache(repoID int64, branchName string, divergence *gitrepo.DivergeObject) error { bs, err := json.Marshal(divergence) if err != nil { return err @@ -178,7 +177,7 @@ func loadOneBranch(ctx context.Context, repo *repo_model.Repository, dbBranch *g p := protectedBranches.GetFirstMatched(branchName) isProtected := p != nil - var divergence *git.DivergeObject + var divergence *gitrepo.DivergeObject // it's not default branch if repo.DefaultBranch != dbBranch.Name && !dbBranch.IsDeleted { @@ -186,9 +185,9 @@ func loadOneBranch(ctx context.Context, repo *repo_model.Repository, dbBranch *g divergence, cached = getDivergenceFromCache(repo.ID, dbBranch.Name) if !cached { var err error - divergence, err = files_service.CountDivergingCommits(ctx, repo, git.BranchPrefix+branchName) + divergence, err = gitrepo.GetDivergingCommits(ctx, repo, repo.DefaultBranch, git.BranchPrefix+branchName) if err != nil { - log.Error("CountDivergingCommits: %v", err) + log.Error("GetDivergingCommits: %v", err) } else { if err = putDivergenceFromCache(repo.ID, dbBranch.Name, divergence); err != nil { log.Error("putDivergenceFromCache: %v", err) @@ -199,7 +198,7 @@ func loadOneBranch(ctx context.Context, repo *repo_model.Repository, dbBranch *g if divergence == nil { // tolerate the error that we cannot get divergence - divergence = &git.DivergeObject{Ahead: -1, Behind: -1} + divergence = &gitrepo.DivergeObject{Ahead: -1, Behind: -1} } pr, err := issues_model.GetLatestPullRequestByHeadInfo(ctx, repo.ID, branchName) @@ -720,7 +719,7 @@ func GetBranchDivergingInfo(ctx reqctx.RequestContext, baseRepo *repo_model.Repo // if the fork repo has new commits, this call will fail because they are not in the base repo // exit status 128 - fatal: Invalid symmetric difference expression aaaaaaaaaaaa...bbbbbbbbbbbb // so at the moment, we first check the update time, then check whether the fork branch has base's head - diff, err := git.GetDivergingCommits(ctx, baseRepo.RepoPath(), baseGitBranch.CommitID, headGitBranch.CommitID) + diff, err := gitrepo.GetDivergingCommits(ctx, baseRepo, baseGitBranch.CommitID, headGitBranch.CommitID) if err != nil { info.BaseHasNewCommits = baseGitBranch.UpdatedUnix > headGitBranch.UpdatedUnix if headRepo.IsFork && info.BaseHasNewCommits { diff --git a/services/repository/files/commit.go b/services/repository/files/commit.go index 3cc326d065b..2e279214949 100644 --- a/services/repository/files/commit.go +++ b/services/repository/files/commit.go @@ -6,21 +6,11 @@ package files import ( "context" - repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/structs" asymkey_service "code.gitea.io/gitea/services/asymkey" ) -// CountDivergingCommits determines how many commits a branch is ahead or behind the repository's base branch -func CountDivergingCommits(ctx context.Context, repo *repo_model.Repository, branch string) (*git.DivergeObject, error) { - divergence, err := git.GetDivergingCommits(ctx, repo.RepoPath(), repo.DefaultBranch, branch) - if err != nil { - return nil, err - } - return &divergence, nil -} - // GetPayloadCommitVerification returns the verification information of a commit func GetPayloadCommitVerification(ctx context.Context, commit *git.Commit) *structs.PayloadCommitVerification { verification := &structs.PayloadCommitVerification{} diff --git a/tests/integration/change_default_branch_test.go b/tests/integration/change_default_branch_test.go index 9b61cff9fd0..6b752f6d3e0 100644 --- a/tests/integration/change_default_branch_test.go +++ b/tests/integration/change_default_branch_test.go @@ -12,7 +12,7 @@ import ( 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/gitrepo" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -43,7 +43,7 @@ func TestChangeDefaultBranch(t *testing.T) { session.MakeRequest(t, req, http.StatusNotFound) } -func checkDivergence(t *testing.T, session *TestSession, branchesURL, expectedDefaultBranch string, expectedBranchToDivergence map[string]git.DivergeObject) { +func checkDivergence(t *testing.T, session *TestSession, branchesURL, expectedDefaultBranch string, expectedBranchToDivergence map[string]*gitrepo.DivergeObject) { req := NewRequest(t, "GET", branchesURL) resp := session.MakeRequest(t, req, http.StatusOK) @@ -92,7 +92,7 @@ func TestChangeDefaultBranchDivergence(t *testing.T) { settingsBranchesURL := fmt.Sprintf("/%s/%s/settings/branches", owner.Name, repo.Name) // check branch divergence before switching default branch - expectedBranchToDivergenceBefore := map[string]git.DivergeObject{ + expectedBranchToDivergenceBefore := map[string]*gitrepo.DivergeObject{ "not-signed": { Ahead: 0, Behind: 0, @@ -119,7 +119,7 @@ func TestChangeDefaultBranchDivergence(t *testing.T) { session.MakeRequest(t, req, http.StatusSeeOther) // check branch divergence after switching default branch - expectedBranchToDivergenceAfter := map[string]git.DivergeObject{ + expectedBranchToDivergenceAfter := map[string]*gitrepo.DivergeObject{ "master": { Ahead: 1, Behind: 0, diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index f4565d8c9c6..eadc61d8496 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -14,11 +14,13 @@ import ( issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/gitrepo" pull_service "code.gitea.io/gitea/services/pull" repo_service "code.gitea.io/gitea/services/repository" files_service "code.gitea.io/gitea/services/repository/files" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestAPIPullUpdate(t *testing.T) { @@ -27,14 +29,16 @@ func TestAPIPullUpdate(t *testing.T) { user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) org26 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 26}) pr := createOutdatedPR(t, user, org26) + require.NoError(t, pr.LoadBaseRepo(t.Context())) + require.NoError(t, pr.LoadIssue(t.Context())) // Test GetDiverging - diffCount, err := pull_service.GetDiverging(t.Context(), pr) - assert.NoError(t, err) + diffCount, err := gitrepo.GetDivergingCommits(t.Context(), pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName()) + require.NoError(t, err) assert.Equal(t, 1, diffCount.Behind) assert.Equal(t, 1, diffCount.Ahead) - assert.NoError(t, pr.LoadBaseRepo(t.Context())) - assert.NoError(t, pr.LoadIssue(t.Context())) + assert.Equal(t, diffCount.Behind, pr.CommitsBehind) + assert.Equal(t, diffCount.Ahead, pr.CommitsAhead) session := loginUser(t, "user2") token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) @@ -43,10 +47,14 @@ func TestAPIPullUpdate(t *testing.T) { session.MakeRequest(t, req, http.StatusOK) // Test GetDiverging after update - diffCount, err = pull_service.GetDiverging(t.Context(), pr) - assert.NoError(t, err) + diffCount, err = gitrepo.GetDivergingCommits(t.Context(), pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName()) + require.NoError(t, err) assert.Equal(t, 0, diffCount.Behind) assert.Equal(t, 2, diffCount.Ahead) + assert.Eventually(t, func() bool { + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) + return diffCount.Behind == pr.CommitsBehind && diffCount.Ahead == pr.CommitsAhead + }, 5*time.Second, 20*time.Millisecond) }) } @@ -56,13 +64,13 @@ func TestAPIPullUpdateByRebase(t *testing.T) { user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) org26 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 26}) pr := createOutdatedPR(t, user, org26) + assert.NoError(t, pr.LoadBaseRepo(t.Context())) // Test GetDiverging - diffCount, err := pull_service.GetDiverging(t.Context(), pr) + diffCount, err := gitrepo.GetDivergingCommits(t.Context(), pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName()) assert.NoError(t, err) assert.Equal(t, 1, diffCount.Behind) assert.Equal(t, 1, diffCount.Ahead) - assert.NoError(t, pr.LoadBaseRepo(t.Context())) assert.NoError(t, pr.LoadIssue(t.Context())) session := loginUser(t, "user2") @@ -72,7 +80,7 @@ func TestAPIPullUpdateByRebase(t *testing.T) { session.MakeRequest(t, req, http.StatusOK) // Test GetDiverging after update - diffCount, err = pull_service.GetDiverging(t.Context(), pr) + diffCount, err = gitrepo.GetDivergingCommits(t.Context(), pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName()) assert.NoError(t, err) assert.Equal(t, 0, diffCount.Behind) assert.Equal(t, 1, diffCount.Ahead)