Skip to content

Commit fafd1db

Browse files
lunnylunny
authored
Some refactors about GetMergeBase (#36186)
Maybe fix #32018 - Use `gitrepo.GetMergeBase` method instead of other two implementations. - Add `FetchRemoteCommit` so that we don't need to add many `remote` to the git repository to avoid possible git lock conflicts. A lock will start when invoke the function, it will be invoked when cross-repository comparing. The head repository will fetch the base repository's base commit id. In most situations, it should lock the fork repositories so that it should not become a bottleneck. - Improve `GetCompareInfo` to remove unnecessarily adding remote. - Remove unnecessary parameters of `SignMerge`.
1 parent 149f7a6 commit fafd1db

12 files changed

Lines changed: 111 additions & 127 deletions

File tree

‎modules/git/repo_compare.go‎

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -18,32 +18,6 @@ import (
1818
"code.gitea.io/gitea/modules/git/gitcmd"
1919
)
2020

21-
// GetMergeBase checks and returns merge base of two branches and the reference used as base.
22-
func (repo *Repository) GetMergeBase(tmpRemote, base, head string) (string, string, error) {
23-
if tmpRemote == "" {
24-
tmpRemote = "origin"
25-
}
26-
27-
if tmpRemote != "origin" {
28-
tmpBaseName := RemotePrefix + tmpRemote + "/tmp_" + base
29-
// Fetch commit into a temporary branch in order to be able to handle commits and tags
30-
_, _, err := gitcmd.NewCommand("fetch", "--no-tags").
31-
AddDynamicArguments(tmpRemote).
32-
AddDashesAndList(base + ":" + tmpBaseName).
33-
WithDir(repo.Path).
34-
RunStdString(repo.Ctx)
35-
if err == nil {
36-
base = tmpBaseName
37-
}
38-
}
39-
40-
stdout, _, err := gitcmd.NewCommand("merge-base").
41-
AddDashesAndList(base, head).
42-
WithDir(repo.Path).
43-
RunStdString(repo.Ctx)
44-
return strings.TrimSpace(stdout), base, err
45-
}
46-
4721
type lineCountWriter struct {
4822
numLines int
4923
}

‎modules/gitrepo/fetch.go‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// Copyright 2025 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package gitrepo
5+
6+
import (
7+
"context"
8+
9+
"code.gitea.io/gitea/modules/git/gitcmd"
10+
"code.gitea.io/gitea/modules/globallock"
11+
)
12+
13+
// FetchRemoteCommit fetches a specific commit and its related objects from a remote
14+
// repository into the managed repository.
15+
//
16+
// If no reference (branch, tag, or other ref) points to the fetched commit, it will
17+
// be treated as unreachable and cleaned up by `git gc` after the default prune
18+
// expiration period (2 weeks). Ref: https://www.kernel.org/pub/software/scm/git/docs/git-gc.html
19+
//
20+
// This behavior is sufficient for temporary operations, such as determining the
21+
// merge base between commits.
22+
func FetchRemoteCommit(ctx context.Context, repo, remoteRepo Repository, commitID string) error {
23+
return globallock.LockAndDo(ctx, getRepoWriteLockKey(repo.RelativePath()), func(ctx context.Context) error {
24+
return RunCmd(ctx, repo, gitcmd.NewCommand("fetch", "--no-tags").
25+
AddDynamicArguments(repoPath(remoteRepo)).
26+
AddDynamicArguments(commitID))
27+
})
28+
}

‎modules/gitrepo/merge.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
// Copyright 2025 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package gitrepo
5+
6+
import (
7+
"context"
8+
"fmt"
9+
"strings"
10+
11+
"code.gitea.io/gitea/modules/git/gitcmd"
12+
)
13+
14+
// MergeBase checks and returns merge base of two commits.
15+
func MergeBase(ctx context.Context, repo Repository, baseCommitID, headCommitID string) (string, error) {
16+
mergeBase, err := RunCmdString(ctx, repo, gitcmd.NewCommand("merge-base").
17+
AddDashesAndList(baseCommitID, headCommitID))
18+
if err != nil {
19+
return "", fmt.Errorf("get merge-base of %s and %s failed: %w", baseCommitID, headCommitID, err)
20+
}
21+
return strings.TrimSpace(mergeBase), nil
22+
}

‎modules/gitrepo/repo_lock.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
// Copyright 2026 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package gitrepo
5+
6+
// getRepoWriteLockKey returns the global lock key for write operations on the repository.
7+
// Parallel write operations on the same git repository should be avoided to prevent data corruption.
8+
func getRepoWriteLockKey(repoStoragePath string) string {
9+
return "repo-write:" + repoStoragePath
10+
}

‎routers/web/repo/issue_view.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -495,7 +495,7 @@ func preparePullViewSigning(ctx *context.Context, issue *issues_model.Issue) {
495495
pull := issue.PullRequest
496496
ctx.Data["WillSign"] = false
497497
if ctx.Doer != nil {
498-
sign, key, _, err := asymkey_service.SignMerge(ctx, pull, ctx.Doer, ctx.Repo.GitRepo, pull.BaseBranch, pull.GetGitHeadRefName())
498+
sign, key, _, err := asymkey_service.SignMerge(ctx, pull, ctx.Doer, ctx.Repo.GitRepo)
499499
ctx.Data["WillSign"] = sign
500500
ctx.Data["SigningKeyMergeDisplay"] = asymkey_model.GetDisplaySigningKey(key)
501501
if err != nil {

‎services/asymkey/sign.go‎

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -271,13 +271,22 @@ Loop:
271271
}
272272

273273
// SignMerge determines if we should sign a PR merge commit to the base repository
274-
func SignMerge(ctx context.Context, pr *issues_model.PullRequest, u *user_model.User, gitRepo *git.Repository, baseCommit, headCommit string) (bool, *git.SigningKey, *git.Signature, error) {
274+
func SignMerge(ctx context.Context, pr *issues_model.PullRequest, u *user_model.User, gitRepo *git.Repository) (bool, *git.SigningKey, *git.Signature, error) {
275275
if err := pr.LoadBaseRepo(ctx); err != nil {
276276
log.Error("Unable to get Base Repo for pull request")
277277
return false, nil, nil, err
278278
}
279279
repo := pr.BaseRepo
280280

281+
baseCommit, err := gitRepo.GetCommit(pr.BaseBranch)
282+
if err != nil {
283+
return false, nil, nil, err
284+
}
285+
headCommit, err := gitRepo.GetCommit(pr.GetGitHeadRefName())
286+
if err != nil {
287+
return false, nil, nil, err
288+
}
289+
281290
signingKey, signer := gitrepo.GetSigningKey(ctx)
282291
if signingKey == nil {
283292
return false, nil, nil, &ErrWontSign{noKey}
@@ -319,38 +328,26 @@ Loop:
319328
return false, nil, nil, &ErrWontSign{approved}
320329
}
321330
case baseSigned:
322-
commit, err := gitRepo.GetCommit(baseCommit)
323-
if err != nil {
324-
return false, nil, nil, err
325-
}
326-
verification := ParseCommitWithSignature(ctx, commit)
331+
verification := ParseCommitWithSignature(ctx, baseCommit)
327332
if !verification.Verified {
328333
return false, nil, nil, &ErrWontSign{baseSigned}
329334
}
330335
case headSigned:
331-
commit, err := gitRepo.GetCommit(headCommit)
332-
if err != nil {
333-
return false, nil, nil, err
334-
}
335-
verification := ParseCommitWithSignature(ctx, commit)
336+
verification := ParseCommitWithSignature(ctx, headCommit)
336337
if !verification.Verified {
337338
return false, nil, nil, &ErrWontSign{headSigned}
338339
}
339340
case commitsSigned:
340-
commit, err := gitRepo.GetCommit(headCommit)
341-
if err != nil {
342-
return false, nil, nil, err
343-
}
344-
verification := ParseCommitWithSignature(ctx, commit)
341+
verification := ParseCommitWithSignature(ctx, headCommit)
345342
if !verification.Verified {
346343
return false, nil, nil, &ErrWontSign{commitsSigned}
347344
}
348345
// need to work out merge-base
349-
mergeBaseCommit, _, err := gitRepo.GetMergeBase("", baseCommit, headCommit)
346+
mergeBaseCommit, err := gitrepo.MergeBase(ctx, pr.BaseRepo, baseCommit.ID.String(), headCommit.ID.String())
350347
if err != nil {
351348
return false, nil, nil, err
352349
}
353-
commitList, err := commit.CommitsBeforeUntil(mergeBaseCommit)
350+
commitList, err := headCommit.CommitsBeforeUntil(mergeBaseCommit)
354351
if err != nil {
355352
return false, nil, nil, err
356353
}

‎services/git/compare.go‎

Lines changed: 28 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,10 @@ package git
66
import (
77
"context"
88
"fmt"
9-
"strconv"
10-
"time"
119

1210
repo_model "code.gitea.io/gitea/models/repo"
1311
"code.gitea.io/gitea/modules/git"
1412
"code.gitea.io/gitea/modules/gitrepo"
15-
"code.gitea.io/gitea/modules/graceful"
16-
logger "code.gitea.io/gitea/modules/log"
1713
"code.gitea.io/gitea/modules/util"
1814
)
1915

@@ -48,25 +44,6 @@ func (ci *CompareInfo) DirectComparison() bool {
4844

4945
// GetCompareInfo generates and returns compare information between base and head branches of repositories.
5046
func GetCompareInfo(ctx context.Context, baseRepo, headRepo *repo_model.Repository, headGitRepo *git.Repository, baseRef, headRef git.RefName, directComparison, fileOnly bool) (_ *CompareInfo, err error) {
51-
var (
52-
remoteBranch string
53-
tmpRemote string
54-
)
55-
56-
// We don't need a temporary remote for same repository.
57-
if baseRepo.ID != headRepo.ID {
58-
// Add a temporary remote
59-
tmpRemote = strconv.FormatInt(time.Now().UnixNano(), 10)
60-
if err = gitrepo.GitRemoteAdd(ctx, headRepo, tmpRemote, baseRepo.RepoPath()); err != nil {
61-
return nil, fmt.Errorf("GitRemoteAdd: %w", err)
62-
}
63-
defer func() {
64-
if err := gitrepo.GitRemoteRemove(graceful.GetManager().ShutdownContext(), headRepo, tmpRemote); err != nil {
65-
logger.Error("GetPullRequestInfo: GitRemoteRemove: %v", err)
66-
}
67-
}()
68-
}
69-
7047
compareInfo := &CompareInfo{
7148
BaseRepo: baseRepo,
7249
BaseRef: baseRef,
@@ -76,47 +53,49 @@ func GetCompareInfo(ctx context.Context, baseRepo, headRepo *repo_model.Reposito
7653
CompareSeparator: util.Iif(directComparison, "..", "..."),
7754
}
7855

56+
compareInfo.BaseCommitID, err = gitrepo.GetFullCommitID(ctx, baseRepo, baseRef.String())
57+
if err != nil {
58+
return nil, err
59+
}
7960
compareInfo.HeadCommitID, err = gitrepo.GetFullCommitID(ctx, headRepo, headRef.String())
8061
if err != nil {
81-
compareInfo.HeadCommitID = headRef.String()
62+
return nil, err
8263
}
8364

84-
// FIXME: It seems we don't need mergebase if it's a direct comparison?
85-
compareInfo.MergeBase, remoteBranch, err = headGitRepo.GetMergeBase(tmpRemote, baseRef.String(), headRef.String())
86-
if err == nil {
87-
compareInfo.BaseCommitID, err = gitrepo.GetFullCommitID(ctx, headRepo, remoteBranch)
88-
if err != nil {
89-
compareInfo.BaseCommitID = remoteBranch
90-
}
91-
separator := "..."
92-
baseCommitID := compareInfo.MergeBase
93-
if directComparison {
94-
separator = ".."
95-
baseCommitID = compareInfo.BaseCommitID
65+
// if they are not the same repository, then we need to fetch the base commit into the head repository
66+
// because we will use headGitRepo in the following code
67+
if baseRepo.ID != headRepo.ID {
68+
exist := headGitRepo.IsReferenceExist(compareInfo.BaseCommitID)
69+
if !exist {
70+
if err := gitrepo.FetchRemoteCommit(ctx, headRepo, baseRepo, compareInfo.BaseCommitID); err != nil {
71+
return nil, fmt.Errorf("FetchRemoteCommit: %w", err)
72+
}
9673
}
74+
}
9775

98-
// We have a common base - therefore we know that ... should work
99-
if !fileOnly {
100-
compareInfo.Commits, err = headGitRepo.ShowPrettyFormatLogToList(ctx, baseCommitID+separator+headRef.String())
101-
if err != nil {
102-
return nil, fmt.Errorf("ShowPrettyFormatLogToList: %w", err)
103-
}
104-
} else {
105-
compareInfo.Commits = []*git.Commit{}
76+
if !directComparison {
77+
compareInfo.MergeBase, err = gitrepo.MergeBase(ctx, headRepo, compareInfo.BaseCommitID, compareInfo.HeadCommitID)
78+
if err != nil {
79+
return nil, fmt.Errorf("MergeBase: %w", err)
10680
}
10781
} else {
108-
compareInfo.Commits = []*git.Commit{}
109-
compareInfo.MergeBase, err = gitrepo.GetFullCommitID(ctx, headRepo, remoteBranch)
82+
compareInfo.MergeBase = compareInfo.BaseCommitID
83+
}
84+
85+
// We have a common base - therefore we know that ... should work
86+
if !fileOnly {
87+
compareInfo.Commits, err = headGitRepo.ShowPrettyFormatLogToList(ctx, compareInfo.BaseCommitID+compareInfo.CompareSeparator+compareInfo.HeadCommitID)
11088
if err != nil {
111-
compareInfo.MergeBase = remoteBranch
89+
return nil, fmt.Errorf("ShowPrettyFormatLogToList: %w", err)
11290
}
113-
compareInfo.BaseCommitID = compareInfo.MergeBase
91+
} else {
92+
compareInfo.Commits = []*git.Commit{}
11493
}
11594

11695
// Count number of changed files.
11796
// This probably should be removed as we need to use shortstat elsewhere
11897
// Now there is git diff --shortstat but this appears to be slower than simply iterating with --nameonly
119-
compareInfo.NumFiles, err = headGitRepo.GetDiffNumChangedFiles(remoteBranch, headRef.String(), directComparison)
98+
compareInfo.NumFiles, err = headGitRepo.GetDiffNumChangedFiles(compareInfo.BaseCommitID, compareInfo.HeadCommitID, directComparison)
12099
if err != nil {
121100
return nil, err
122101
}

‎services/issue/pull.go‎

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -7,35 +7,16 @@ import (
77
"context"
88
"fmt"
99
"slices"
10-
"time"
1110

1211
issues_model "code.gitea.io/gitea/models/issues"
1312
org_model "code.gitea.io/gitea/models/organization"
14-
repo_model "code.gitea.io/gitea/models/repo"
1513
user_model "code.gitea.io/gitea/models/user"
1614
"code.gitea.io/gitea/modules/git"
1715
"code.gitea.io/gitea/modules/gitrepo"
18-
"code.gitea.io/gitea/modules/graceful"
1916
"code.gitea.io/gitea/modules/log"
2017
"code.gitea.io/gitea/modules/setting"
2118
)
2219

23-
func getMergeBase(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, pr *issues_model.PullRequest, baseBranch, headBranch string) (string, error) {
24-
// Add a temporary remote
25-
tmpRemote := fmt.Sprintf("mergebase-%d-%d", pr.ID, time.Now().UnixNano())
26-
if err := gitrepo.GitRemoteAdd(ctx, repo, tmpRemote, gitRepo.Path); err != nil {
27-
return "", fmt.Errorf("GitRemoteAdd: %w", err)
28-
}
29-
defer func() {
30-
if err := gitrepo.GitRemoteRemove(graceful.GetManager().ShutdownContext(), repo, tmpRemote); err != nil {
31-
log.Error("getMergeBase: GitRemoteRemove: %v", err)
32-
}
33-
}()
34-
35-
mergeBase, _, err := gitRepo.GetMergeBase(tmpRemote, baseBranch, headBranch)
36-
return mergeBase, err
37-
}
38-
3920
type ReviewRequestNotifier struct {
4021
Comment *issues_model.Comment
4122
IsAdd bool
@@ -99,11 +80,10 @@ func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque
9980
}
10081

10182
// get the mergebase
102-
mergeBase, err := getMergeBase(ctx, pr.BaseRepo, repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitHeadRefName())
83+
mergeBase, err := gitrepo.MergeBase(ctx, pr.BaseRepo, git.BranchPrefix+pr.BaseBranch, pr.GetGitHeadRefName())
10384
if err != nil {
10485
return nil, err
10586
}
106-
10787
// https://github.com/go-gitea/gitea/issues/29763, we need to get the files changed
10888
// between the merge base and the head commit but not the base branch and the head commit
10989
changedFiles, err := repo.GetFilesChangedBetween(mergeBase, pr.GetGitHeadRefName())

‎services/migrations/gitea_uploader.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -732,7 +732,7 @@ func (g *GiteaLocalUploader) newPullRequest(ctx context.Context, pr *base.PullRe
732732
if pr.Base.Ref != "" && pr.Head.SHA != "" {
733733
// A PR against a tag base does not make sense - therefore pr.Base.Ref must be a branch
734734
// TODO: should we be checking for the refs/heads/ prefix on the pr.Base.Ref? (i.e. are these actually branches or refs)
735-
pr.Base.SHA, _, err = g.gitRepo.GetMergeBase("", git.BranchPrefix+pr.Base.Ref, pr.Head.SHA)
735+
pr.Base.SHA, err = gitrepo.MergeBase(ctx, g.repo, git.BranchPrefix+pr.Base.Ref, pr.Head.SHA)
736736
if err != nil {
737737
log.Error("Cannot determine the merge base for PR #%d in %s/%s. Error: %v", pr.Number, g.repoOwner, g.repoName, err)
738738
}

‎services/pull/check.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,7 @@ func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer
238238
}
239239
defer closer.Close()
240240

241-
sign, _, _, err := asymkey_service.SignMerge(ctx, pr, doer, gitRepo, pr.BaseBranch, pr.GetGitHeadRefName())
241+
sign, _, _, err := asymkey_service.SignMerge(ctx, pr, doer, gitRepo)
242242

243243
return sign, err
244244
}

0 commit comments

Comments
 (0)