From c33ac14578fac8122a66fe6a8a32a55c72f6a660 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 23 Sep 2025 10:21:25 -0700 Subject: [PATCH 01/19] Move GetDiverging functions to gitrepo --- models/migrations/v1_12/v136.go | 11 +++-- modules/git/repo.go | 30 ------------- modules/git/repo_test.go | 24 ---------- modules/gitrepo/compare.go | 44 +++++++++++++++++++ modules/gitrepo/compare_test.go | 42 ++++++++++++++++++ modules/gitrepo/main_test.go | 44 +++++++++++++++++++ services/pull/pull.go | 33 ++++++++------ services/pull/update.go | 24 +++++----- services/repository/branch.go | 15 +++---- services/repository/files/commit.go | 10 ----- .../integration/change_default_branch_test.go | 8 ++-- 11 files changed, 175 insertions(+), 110 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/migrations/v1_12/v136.go b/models/migrations/v1_12/v136.go index 0f53278b4623f..3091176aa0f13 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,12 @@ 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") 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(), + repo_model.StorageRepo(repo_model.RelativePath(baseRepo.OwnerName, baseRepo.Name)), + 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 38cb4592a0498..9f8b6225c8f05 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 813844d1ea131..26ee3a091a269 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 0000000000000..1c8f5421fa7bf --- /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 0000000000000..f8661d9412102 --- /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/main_test.go b/modules/gitrepo/main_test.go new file mode 100644 index 0000000000000..4b5e7537ce60a --- /dev/null +++ b/modules/gitrepo/main_test.go @@ -0,0 +1,44 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package gitrepo + +import ( + "os" + "path/filepath" + "testing" + + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/setting" +) + +const ( + testReposDir = "../git/tests/repos/" +) + +func TestMain(m *testing.M) { + originalRepoRootPath := setting.RepoRootPath + defer func() { + setting.RepoRootPath = originalRepoRootPath + }() + setting.RepoRootPath, _ = filepath.Abs(testReposDir) + + originalHomePath := setting.Git.HomePath + defer func() { + setting.Git.HomePath = originalHomePath + }() + setting.Git.HomePath = filepath.Join(setting.RepoRootPath, ".home") + + originalGitPath := setting.Git.Path + defer func() { + setting.Git.Path = originalGitPath + }() + setting.Git.Path = "git" + + if err := git.InitSimple(); err != nil { + panic(err) + } + + exitStatus := m.Run() + os.Exit(exitStatus) +} diff --git a/services/pull/pull.go b/services/pull/pull.go index c7b3a0d523d11..5db86ead35774 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,19 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { return err } + // Update Commit Divergence + divergence, err := GetDiverging(ctx, pr) + if err != nil { + return err + } + pr.CommitsAhead = divergence.Ahead + pr.CommitsBehind = divergence.Behind + + err = pr.UpdateCommitDivergence(ctx, divergence.Ahead, divergence.Behind) + 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 @@ -295,13 +301,6 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer 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 { return err @@ -464,7 +463,13 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { log.Error("Find pull requests [base_repo_id: %d, base_branch: %s]: %v", opts.RepoID, opts.Branch, err) return } + baseRepo, err := repo_model.GetRepositoryByID(ctx, opts.RepoID) + if err != nil { + log.Error("GetRepositoryByID: %v", err) + return + } for _, pr := range prs { + pr.BaseRepo = baseRepo // avoid loading again divergence, err := GetDiverging(ctx, pr) if err != nil { if git_model.IsErrBranchNotExist(err) && !gitrepo.IsBranchExist(ctx, pr.HeadRepo, pr.HeadBranch) { diff --git a/services/pull/update.go b/services/pull/update.go index b8f84e3d658ae..92e9aed5b7c9a 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,6 +34,11 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. } defer releaser() + 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) + } + diffCount, err := GetDiverging(ctx, pr) if err != nil { return err @@ -41,10 +46,6 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. 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) - } 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) @@ -173,17 +174,12 @@ func IsUserAllowedToUpdate(ctx context.Context, pull *issues_model.PullRequest, } // 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) { +func GetDiverging(ctx context.Context, pr *issues_model.PullRequest) (*gitrepo.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) - } + + if err := pr.LoadBaseRepo(ctx); err != nil { return nil, err } - defer cancel() - diff, err := git.GetDivergingCommits(ctx, prCtx.tmpBasePath, baseBranch, trackingBranch) - return &diff, err + return gitrepo.GetDivergingCommits(ctx, pr.BaseRepo, baseBranch, pr.GetGitHeadRefName()) } diff --git a/services/repository/branch.go b/services/repository/branch.go index df7202227aa70..67206c2cc9248 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,7 +185,7 @@ 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) } else { @@ -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 3cc326d065b16..2e27921494981 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 9b61cff9fd082..6b752f6d3e0c2 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, From b26ecc2dea1df6d04faed90bc0827e0c1df69351 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 23 Sep 2025 15:23:13 -0700 Subject: [PATCH 02/19] Fix bug --- services/pull/update.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/pull/update.go b/services/pull/update.go index 92e9aed5b7c9a..66c03df25d9d2 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -181,5 +181,5 @@ func GetDiverging(ctx context.Context, pr *issues_model.PullRequest) (*gitrepo.D return nil, err } - return gitrepo.GetDivergingCommits(ctx, pr.BaseRepo, baseBranch, pr.GetGitHeadRefName()) + return gitrepo.GetDivergingCommits(ctx, pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName()) } From a66e32d77ffab028908b305ddd6d2193581cce28 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 23 Sep 2025 19:23:22 -0700 Subject: [PATCH 03/19] Fix bug --- services/pull/update.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/services/pull/update.go b/services/pull/update.go index 66c03df25d9d2..20de3d8d0e759 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -39,11 +39,13 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. return fmt.Errorf("unable to load BaseRepo for PR[%d] during update-by-merge: %w", pr.ID, err) } - 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 pr.ID > 0 { // only a real PR needs to check this + 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.LoadHeadRepo(ctx); err != nil { From 2c70eb9a87c19b05207cbb07a0180a8548399f59 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 24 Sep 2025 09:27:22 -0700 Subject: [PATCH 04/19] make the code simpler --- modules/gitrepo/main_test.go | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/modules/gitrepo/main_test.go b/modules/gitrepo/main_test.go index 4b5e7537ce60a..015cddc34fc04 100644 --- a/modules/gitrepo/main_test.go +++ b/modules/gitrepo/main_test.go @@ -18,21 +18,16 @@ const ( func TestMain(m *testing.M) { originalRepoRootPath := setting.RepoRootPath - defer func() { - setting.RepoRootPath = originalRepoRootPath - }() - setting.RepoRootPath, _ = filepath.Abs(testReposDir) - originalHomePath := setting.Git.HomePath - defer func() { - setting.Git.HomePath = originalHomePath - }() - setting.Git.HomePath = filepath.Join(setting.RepoRootPath, ".home") - originalGitPath := setting.Git.Path defer func() { + setting.RepoRootPath = originalRepoRootPath + setting.Git.HomePath = originalHomePath setting.Git.Path = originalGitPath }() + + setting.RepoRootPath, _ = filepath.Abs(testReposDir) + setting.Git.HomePath = filepath.Join(setting.RepoRootPath, ".home") setting.Git.Path = "git" if err := git.InitSimple(); err != nil { From 768c70c9d86d8f038964b74d122711842dde0c31 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 24 Sep 2025 09:36:07 -0700 Subject: [PATCH 05/19] remove unnecessary change --- modules/gitrepo/main_test.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/modules/gitrepo/main_test.go b/modules/gitrepo/main_test.go index 015cddc34fc04..eb5b72b05c5ad 100644 --- a/modules/gitrepo/main_test.go +++ b/modules/gitrepo/main_test.go @@ -8,7 +8,6 @@ import ( "path/filepath" "testing" - "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/setting" ) @@ -19,20 +18,13 @@ const ( func TestMain(m *testing.M) { originalRepoRootPath := setting.RepoRootPath originalHomePath := setting.Git.HomePath - originalGitPath := setting.Git.Path defer func() { setting.RepoRootPath = originalRepoRootPath setting.Git.HomePath = originalHomePath - setting.Git.Path = originalGitPath }() setting.RepoRootPath, _ = filepath.Abs(testReposDir) setting.Git.HomePath = filepath.Join(setting.RepoRootPath, ".home") - setting.Git.Path = "git" - - if err := git.InitSimple(); err != nil { - panic(err) - } exitStatus := m.Run() os.Exit(exitStatus) From 3d67efd9c2e7c7d990007a3b81eaa22fda47085f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 24 Sep 2025 11:08:16 -0700 Subject: [PATCH 06/19] follow wxiaoguang's suggestion --- modules/gitrepo/main_test.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/modules/gitrepo/main_test.go b/modules/gitrepo/main_test.go index eb5b72b05c5ad..40202d4993576 100644 --- a/modules/gitrepo/main_test.go +++ b/modules/gitrepo/main_test.go @@ -16,13 +16,6 @@ const ( ) func TestMain(m *testing.M) { - originalRepoRootPath := setting.RepoRootPath - originalHomePath := setting.Git.HomePath - defer func() { - setting.RepoRootPath = originalRepoRootPath - setting.Git.HomePath = originalHomePath - }() - setting.RepoRootPath, _ = filepath.Abs(testReposDir) setting.Git.HomePath = filepath.Join(setting.RepoRootPath, ".home") From c858a190ae82e64560d00bb9a218410c1e2619ec Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 24 Sep 2025 12:48:14 -0700 Subject: [PATCH 07/19] improvements --- modules/gitrepo/main_test.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/modules/gitrepo/main_test.go b/modules/gitrepo/main_test.go index 40202d4993576..f1006de4a7438 100644 --- a/modules/gitrepo/main_test.go +++ b/modules/gitrepo/main_test.go @@ -4,11 +4,13 @@ package gitrepo import ( + "fmt" "os" "path/filepath" "testing" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/util" ) const ( @@ -16,8 +18,17 @@ const ( ) func TestMain(m *testing.M) { + // since all test repository is defined by testReposDir, we need to set it here + // So that all functions could work properly because Repository will only contain + // Relative Path to RepoRootPath setting.RepoRootPath, _ = filepath.Abs(testReposDir) - setting.Git.HomePath = filepath.Join(setting.RepoRootPath, ".home") + // TODO: run command requires a home directory, we could set it to a temp dir + gitHomePath, err := os.MkdirTemp(os.TempDir(), "git-home") + if err != nil { + panic(fmt.Errorf("unable to create temp dir: %w", err)) + } + defer util.RemoveAll(gitHomePath) + setting.Git.HomePath = gitHomePath exitStatus := m.Run() os.Exit(exitStatus) From c397f49dd81b8c7cb9d34275e3006eaf134a3b25 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 25 Sep 2025 08:39:06 +0800 Subject: [PATCH 08/19] fix --- modules/gitrepo/gitrepo.go | 4 ++-- modules/gitrepo/main_test.go | 29 ++++++++++++----------------- services/pull/pull.go | 4 ++-- services/pull/update.go | 2 +- services/repository/branch.go | 2 +- 5 files changed, 18 insertions(+), 23 deletions(-) diff --git a/modules/gitrepo/gitrepo.go b/modules/gitrepo/gitrepo.go index fad8f70c4cbd0..98dca715196d3 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 +// RelativePath should be a unix style path like username/reponame.git // This method should change it according to the current OS. -func repoPath(repo Repository) string { +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 index f1006de4a7438..222681579f5f4 100644 --- a/modules/gitrepo/main_test.go +++ b/modules/gitrepo/main_test.go @@ -4,32 +4,27 @@ package gitrepo import ( - "fmt" "os" "path/filepath" "testing" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" - "code.gitea.io/gitea/modules/util" -) - -const ( - testReposDir = "../git/tests/repos/" + "code.gitea.io/gitea/modules/tempdir" ) func TestMain(m *testing.M) { - // since all test repository is defined by testReposDir, we need to set it here - // So that all functions could work properly because Repository will only contain - // Relative Path to RepoRootPath - setting.RepoRootPath, _ = filepath.Abs(testReposDir) - // TODO: run command requires a home directory, we could set it to a temp dir - gitHomePath, err := os.MkdirTemp(os.TempDir(), "git-home") + gitHomePath, cleanup, err := tempdir.OsTempDir("gitea-test").MkdirTempRandom("git-home") if err != nil { - panic(fmt.Errorf("unable to create temp dir: %w", err)) + log.Fatal("Unable to create temp dir: %v", err) } - defer util.RemoveAll(gitHomePath) - setting.Git.HomePath = gitHomePath + defer cleanup() - exitStatus := m.Run() - os.Exit(exitStatus) + // resolve repository path relative to the test directory + repoPath = func(repo Repository) string { + return filepath.Join("../git/tests/repos", repo.RelativePath()) + } + + setting.Git.HomePath = gitHomePath + os.Exit(m.Run()) } diff --git a/services/pull/pull.go b/services/pull/pull.go index 5db86ead35774..d6e1217fa0053 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -469,7 +469,7 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { return } for _, pr := range prs { - pr.BaseRepo = baseRepo // avoid loading again + pr.BaseRepo = baseRepo // avoid loading again // FIXME: why only here does so but the code above doesn't do so divergence, err := GetDiverging(ctx, pr) if err != nil { if git_model.IsErrBranchNotExist(err) && !gitrepo.IsBranchExist(ctx, pr.HeadRepo, pr.HeadBranch) { @@ -491,7 +491,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 20de3d8d0e759..55fbd74784d60 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -39,7 +39,7 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. return fmt.Errorf("unable to load BaseRepo for PR[%d] during update-by-merge: %w", pr.ID, err) } - if pr.ID > 0 { // only a real PR needs to check this + if pr.ID > 0 { // only a real PR needs to check this FIXME: why it needs this check? diffCount, err := GetDiverging(ctx, pr) if err != nil { return err diff --git a/services/repository/branch.go b/services/repository/branch.go index 67206c2cc9248..1a34121d9906b 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -187,7 +187,7 @@ func loadOneBranch(ctx context.Context, repo *repo_model.Repository, dbBranch *g var err error 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) From 47a6bbded1c5b551dbc46a28fa4c20f647820527 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 25 Sep 2025 10:21:40 +0800 Subject: [PATCH 09/19] fix --- services/pull/update.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/services/pull/update.go b/services/pull/update.go index 55fbd74784d60..1a5491fd8918c 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -39,7 +39,8 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. return fmt.Errorf("unable to load BaseRepo for PR[%d] during update-by-merge: %w", pr.ID, err) } - if pr.ID > 0 { // only a real PR needs to check this FIXME: why it needs this check? + // 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 := GetDiverging(ctx, pr) if err != nil { return err From 3d554b66c721cef9cff4d0b64ab237f4771d1164 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 25 Sep 2025 13:38:13 +0800 Subject: [PATCH 10/19] update comment --- services/pull/pull.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index d6e1217fa0053..653e087d45a92 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -372,6 +372,8 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { // TODO: graceful: AddTestPullRequestTask needs to become a queue! // GetUnmergedPullRequestsByHeadInfo() only return open and unmerged PR. + // TODO: rename the "prs" to a new variable like "headBranchPRs" to distinguish from the "baseBranchPRs" below + // The base repositories of headBranchPRs are different prs, 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) @@ -458,6 +460,8 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { } log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", opts.RepoID, opts.Branch) + // TODO: rename the "prs" to a new variable like "baseBranchPRs" to distinguish from the "headBranchPRs" above + // The base repositories of baseBranchPRs are the same one (opts.RepoID) prs, 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) @@ -469,7 +473,7 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { return } for _, pr := range prs { - pr.BaseRepo = baseRepo // avoid loading again // FIXME: why only here does so but the code above doesn't do so + pr.BaseRepo = baseRepo // avoid loading again divergence, err := GetDiverging(ctx, pr) if err != nil { if git_model.IsErrBranchNotExist(err) && !gitrepo.IsBranchExist(ctx, pr.HeadRepo, pr.HeadBranch) { From f76938dc7d38378cd3edaaf68011900046ef0636 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 25 Sep 2025 13:58:15 +0800 Subject: [PATCH 11/19] fix --- services/pull/pull.go | 31 +++++++------------------------ services/pull/update.go | 10 +++++++++- 2 files changed, 16 insertions(+), 25 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 653e087d45a92..812da0e1d9d1d 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -128,14 +128,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } // Update Commit Divergence - divergence, err := GetDiverging(ctx, pr) - if err != nil { - return err - } - pr.CommitsAhead = divergence.Ahead - pr.CommitsBehind = divergence.Behind - - err = pr.UpdateCommitDivergence(ctx, divergence.Ahead, divergence.Behind) + err = UpdateCommitDivergence(ctx, pr) if err != nil { return err } @@ -433,14 +426,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 = UpdateCommitDivergence(ctx, pr); err != nil { + log.Error("UpdateCommitDivergence: %v", err) } } @@ -474,18 +461,14 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { } for _, pr := range prs { pr.BaseRepo = baseRepo // avoid loading again - divergence, err := GetDiverging(ctx, pr) + err = UpdateCommitDivergence(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) } + continue } StartPullRequestCheckDelayable(ctx, pr) } diff --git a/services/pull/update.go b/services/pull/update.go index 1a5491fd8918c..a6fb3b5390b6d 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -41,7 +41,7 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. // 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 := GetDiverging(ctx, pr) + diffCount, err := gitrepo.GetDivergingCommits(ctx, pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName()) if err != nil { return err } else if diffCount.Behind == 0 { @@ -186,3 +186,11 @@ func GetDiverging(ctx context.Context, pr *issues_model.PullRequest) (*gitrepo.D return gitrepo.GetDivergingCommits(ctx, pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName()) } + +func UpdateCommitDivergence(ctx context.Context, pr *issues_model.PullRequest) error { + divergence, err := GetDiverging(ctx, pr) + if err != nil { + return err + } + return pr.UpdateCommitDivergence(ctx, divergence.Ahead, divergence.Behind) +} From 21e2f94a4dec1017f3da2905b700ad7a75bcd1ce Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 25 Sep 2025 14:08:55 +0800 Subject: [PATCH 12/19] fix --- models/issues/pull.go | 4 ---- routers/web/repo/issue_comment.go | 4 ++-- services/pull/pull.go | 27 +++++++++++++-------------- services/pull/update.go | 14 +++----------- tests/integration/pull_update_test.go | 14 ++++++++++---- 5 files changed, 28 insertions(+), 35 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index 7a37b627e1bd0..73e6cc379ab8c 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{ diff --git a/routers/web/repo/issue_comment.go b/routers/web/repo/issue_comment.go index cb5b2d801952d..9a44f18e45c26 100644 --- a/routers/web/repo/issue_comment.go +++ b/routers/web/repo/issue_comment.go @@ -124,8 +124,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/pull.go b/services/pull/pull.go index 812da0e1d9d1d..6c1700448c377 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -128,7 +128,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } // Update Commit Divergence - err = UpdateCommitDivergence(ctx, pr) + err = syncCommitDivergence(ctx, pr) if err != nil { return err } @@ -286,19 +286,18 @@ 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 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 { + if err := pr.UpdateColsIfNotMerged(ctx, "merge_base", "status", "conflicted_files", "changed_protected_files", "base_branch"); err != nil { return err } + if !pr.HasMerged { + if err = syncCommitDivergence(ctx, pr); err != nil { + return fmt.Errorf("syncCommitDivergence: %w", err) + } + } + // Create comment options := &issues_model.CreateCommentOptions{ Type: issues_model.CommentTypeChangeTargetBranch, @@ -426,8 +425,8 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, opts.NewCommitID); err != nil { log.Error("MarkReviewsAsNotStale: %v", err) } - if err = UpdateCommitDivergence(ctx, pr); err != nil { - log.Error("UpdateCommitDivergence: %v", err) + if err = syncCommitDivergence(ctx, pr); err != nil { + log.Error("syncCommitDivergence: %v", err) } } @@ -461,12 +460,12 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { } for _, pr := range prs { pr.BaseRepo = baseRepo // avoid loading again - err = UpdateCommitDivergence(ctx, pr) + err = syncCommitDivergence(ctx, pr) if err != nil { 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("UpdateCommitDivergence: %v", err) + log.Error("syncCommitDivergence: %v", err) } continue } diff --git a/services/pull/update.go b/services/pull/update.go index a6fb3b5390b6d..cce39374516b6 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -176,19 +176,11 @@ 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) (*gitrepo.DivergeObject, error) { - log.Trace("GetDiverging[%-v]: compare commits", pr) - +func syncCommitDivergence(ctx context.Context, pr *issues_model.PullRequest) error { if err := pr.LoadBaseRepo(ctx); err != nil { - return nil, err + return err } - - return gitrepo.GetDivergingCommits(ctx, pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName()) -} - -func UpdateCommitDivergence(ctx context.Context, pr *issues_model.PullRequest) error { - divergence, err := GetDiverging(ctx, pr) + divergence, err := gitrepo.GetDivergingCommits(ctx, pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName()) if err != nil { return err } diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index f4565d8c9c6a2..ccf90c4a13f37 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -14,6 +14,7 @@ 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" @@ -29,10 +30,12 @@ func TestAPIPullUpdate(t *testing.T) { pr := createOutdatedPR(t, user, org26) // 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.Equal(t, diffCount.Behind, pr.CommitsBehind) + assert.Equal(t, diffCount.Ahead, pr.CommitsAhead) assert.NoError(t, pr.LoadBaseRepo(t.Context())) assert.NoError(t, pr.LoadIssue(t.Context())) @@ -43,10 +46,13 @@ func TestAPIPullUpdate(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, 2, diffCount.Ahead) + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) + assert.Equal(t, diffCount.Behind, pr.CommitsBehind) + assert.Equal(t, diffCount.Ahead, pr.CommitsAhead) }) } @@ -58,7 +64,7 @@ func TestAPIPullUpdateByRebase(t *testing.T) { pr := createOutdatedPR(t, user, org26) // 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) @@ -72,7 +78,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) From 81b1b0412fb1dc49f5515df9a4965487239db5bd Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 25 Sep 2025 14:18:01 +0800 Subject: [PATCH 13/19] fix --- models/migrations/v1_12/v136.go | 7 ++----- modules/gitrepo/gitrepo.go | 4 ++-- modules/gitrepo/main_test.go | 4 +++- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/models/migrations/v1_12/v136.go b/models/migrations/v1_12/v136.go index 3091176aa0f13..20b892b6cc547 100644 --- a/models/migrations/v1_12/v136.go +++ b/models/migrations/v1_12/v136.go @@ -84,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 } - + repoStore := repo_model.StorageRepo(repo_model.RelativePath(baseRepo.OwnerName, baseRepo.Name)) gitRefName := fmt.Sprintf("refs/pull/%d/head", pr.Index) - - divergence, err := gitrepo.GetDivergingCommits(graceful.GetManager().HammerContext(), - repo_model.StorageRepo(repo_model.RelativePath(baseRepo.OwnerName, baseRepo.Name)), - 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/gitrepo/gitrepo.go b/modules/gitrepo/gitrepo.go index 98dca715196d3..59d2323599211 100644 --- a/modules/gitrepo/gitrepo.go +++ b/modules/gitrepo/gitrepo.go @@ -20,8 +20,8 @@ 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 a unix style path like username/reponame.git -// This method should change it according to the current OS. +// 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 index 222681579f5f4..6e6636ce770f9 100644 --- a/modules/gitrepo/main_test.go +++ b/modules/gitrepo/main_test.go @@ -11,6 +11,7 @@ import ( "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) { @@ -21,8 +22,9 @@ func TestMain(m *testing.M) { defer cleanup() // resolve repository path relative to the test directory + testRootDir := test.SetupGiteaRoot() repoPath = func(repo Repository) string { - return filepath.Join("../git/tests/repos", repo.RelativePath()) + return filepath.Join(testRootDir, "/modules/git/tests/repos", repo.RelativePath()) } setting.Git.HomePath = gitHomePath From 5b6ca878cedcee31ac48036c86da988ddb22246f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 24 Sep 2025 23:44:12 -0700 Subject: [PATCH 14/19] improvements --- services/pull/pull.go | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 6c1700448c377..65f6d824fadfd 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -363,17 +363,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. - // TODO: rename the "prs" to a new variable like "headBranchPRs" to distinguish from the "baseBranchPRs" below - // The base repositories of headBranchPRs are different - 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) @@ -391,14 +395,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) @@ -446,20 +450,14 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { } log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", opts.RepoID, opts.Branch) - // TODO: rename the "prs" to a new variable like "baseBranchPRs" to distinguish from the "headBranchPRs" above // The base repositories of baseBranchPRs are the same one (opts.RepoID) - prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, opts.RepoID, opts.Branch) + 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 } - baseRepo, err := repo_model.GetRepositoryByID(ctx, opts.RepoID) - if err != nil { - log.Error("GetRepositoryByID: %v", err) - return - } - for _, pr := range prs { - pr.BaseRepo = baseRepo // avoid loading again + for _, pr := range baseBranchPRs { + pr.BaseRepo = repo // avoid loading again err = syncCommitDivergence(ctx, pr) if err != nil { if errors.Is(err, util.ErrNotExist) { From 86bdf96d8c4cc06fcbae63c672b0f9b50897fbc4 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 24 Sep 2025 23:45:11 -0700 Subject: [PATCH 15/19] Fix test --- tests/integration/pull_update_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index ccf90c4a13f37..2f611da1cf0d2 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -28,6 +28,7 @@ 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) + assert.NoError(t, pr.LoadBaseRepo(t.Context())) // Test GetDiverging diffCount, err := gitrepo.GetDivergingCommits(t.Context(), pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName()) @@ -36,7 +37,6 @@ func TestAPIPullUpdate(t *testing.T) { assert.Equal(t, 1, diffCount.Ahead) assert.Equal(t, diffCount.Behind, pr.CommitsBehind) assert.Equal(t, diffCount.Ahead, pr.CommitsAhead) - assert.NoError(t, pr.LoadBaseRepo(t.Context())) assert.NoError(t, pr.LoadIssue(t.Context())) session := loginUser(t, "user2") @@ -62,13 +62,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 := 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") From c04fb8613e018ae8030f2b0d89d12eea20f85f86 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 24 Sep 2025 23:48:31 -0700 Subject: [PATCH 16/19] improvements --- services/pull/pull.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 65f6d824fadfd..3f2f90f747ea8 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -292,10 +292,8 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer return err } - if !pr.HasMerged { - if err = syncCommitDivergence(ctx, pr); err != nil { - return fmt.Errorf("syncCommitDivergence: %w", err) - } + if err = syncCommitDivergence(ctx, pr); err != nil { + return fmt.Errorf("syncCommitDivergence: %w", err) } // Create comment From d7bda6b5f83d5a8f013964c5c7b1140c93dbb96f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 24 Sep 2025 23:59:25 -0700 Subject: [PATCH 17/19] Revert "improvements" This reverts commit c04fb8613e018ae8030f2b0d89d12eea20f85f86. --- services/pull/pull.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 3f2f90f747ea8..65f6d824fadfd 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -292,8 +292,10 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer return err } - if err = syncCommitDivergence(ctx, pr); err != nil { - return fmt.Errorf("syncCommitDivergence: %w", err) + if !pr.HasMerged { + if err = syncCommitDivergence(ctx, pr); err != nil { + return fmt.Errorf("syncCommitDivergence: %w", err) + } } // Create comment From 22090f2fa33d253598d751bba6ae56f83733d131 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 25 Sep 2025 15:14:37 +0800 Subject: [PATCH 18/19] fix transaction pr status check --- models/issues/pull.go | 5 ++--- services/pull/check.go | 4 ++-- services/pull/pull.go | 14 +++++++++----- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index 73e6cc379ab8c..fb7dff3cc9e83 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -642,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/services/pull/check.go b/services/pull/check.go index c2ee49b6bb493..088fd3702c643 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 65f6d824fadfd..b64a846adc091 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -288,14 +288,18 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer // add first push codes comment return db.WithTx(ctx, func(ctx context.Context) error { - if err := pr.UpdateColsIfNotMerged(ctx, "merge_base", "status", "conflicted_files", "changed_protected_files", "base_branch"); 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 !pr.HasMerged { - if err = syncCommitDivergence(ctx, pr); err != nil { - return fmt.Errorf("syncCommitDivergence: %w", err) - } + if err := syncCommitDivergence(ctx, pr); err != nil { + return fmt.Errorf("syncCommitDivergence: %w", err) } // Create comment From 5b5a5d52107b9a0727355a053016a078a8fb5430 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 25 Sep 2025 16:09:46 +0800 Subject: [PATCH 19/19] fix test --- tests/integration/pull_update_test.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index 2f611da1cf0d2..eadc61d849668 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -20,6 +20,7 @@ import ( files_service "code.gitea.io/gitea/services/repository/files" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestAPIPullUpdate(t *testing.T) { @@ -28,16 +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) - assert.NoError(t, pr.LoadBaseRepo(t.Context())) + require.NoError(t, pr.LoadBaseRepo(t.Context())) + require.NoError(t, pr.LoadIssue(t.Context())) // Test GetDiverging diffCount, err := gitrepo.GetDivergingCommits(t.Context(), pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName()) - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, 1, diffCount.Behind) assert.Equal(t, 1, diffCount.Ahead) assert.Equal(t, diffCount.Behind, pr.CommitsBehind) assert.Equal(t, diffCount.Ahead, pr.CommitsAhead) - assert.NoError(t, pr.LoadIssue(t.Context())) session := loginUser(t, "user2") token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository) @@ -47,12 +48,13 @@ func TestAPIPullUpdate(t *testing.T) { // Test GetDiverging after update diffCount, err = gitrepo.GetDivergingCommits(t.Context(), pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName()) - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, 0, diffCount.Behind) assert.Equal(t, 2, diffCount.Ahead) - pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID}) - assert.Equal(t, diffCount.Behind, pr.CommitsBehind) - assert.Equal(t, diffCount.Ahead, pr.CommitsAhead) + 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) }) }