Mirrors/gitea
0
0
Fork 0
mirror of https://github.com/go-gitea/gitea.git synced 2026-01-22 02:27:11 +01:00
Code Issues Activity

Some refactors about GetMergeBase (#36186)

Browse Source 
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`.
This commit is contained in:
Lunny Xiao 2026-01-17 11:22:09 -08:00 committed by GitHub
parent 149f7a6f1f
commit fafd1db19e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
12 changed files with 113 additions and 129 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
modules/git/repo_compare.go
View File

@ -18,32 +18,6 @@ import (
"code.gitea.io/gitea/modules/git/gitcmd"
)
// GetMergeBase checks and returns merge base of two branches and the reference used as base.
func (repo *Repository) GetMergeBase(tmpRemote, base, head string) (string, string, error) {
if tmpRemote == "" {
tmpRemote = "origin"
}
if tmpRemote != "origin" {
tmpBaseName := RemotePrefix + tmpRemote + "/tmp_" + base
// Fetch commit into a temporary branch in order to be able to handle commits and tags
_, _, err := gitcmd.NewCommand("fetch", "--no-tags").
AddDynamicArguments(tmpRemote).
AddDashesAndList(base + ":" + tmpBaseName).
WithDir(repo.Path).
RunStdString(repo.Ctx)
if err == nil {
base = tmpBaseName
}
}
stdout, _, err := gitcmd.NewCommand("merge-base").
AddDashesAndList(base, head).
WithDir(repo.Path).
RunStdString(repo.Ctx)
return strings.TrimSpace(stdout), base, err
}
type lineCountWriter struct {
numLines int
}

28
modules/gitrepo/fetch.go Normal file
View File

@ -0,0 +1,28 @@
// Copyright 2025 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package gitrepo
import (
"context"
"code.gitea.io/gitea/modules/git/gitcmd"
"code.gitea.io/gitea/modules/globallock"
)
// FetchRemoteCommit fetches a specific commit and its related objects from a remote
// repository into the managed repository.
//
// If no reference (branch, tag, or other ref) points to the fetched commit, it will
// be treated as unreachable and cleaned up by `git gc` after the default prune
// expiration period (2 weeks). Ref: https://www.kernel.org/pub/software/scm/git/docs/git-gc.html
//
// This behavior is sufficient for temporary operations, such as determining the
// merge base between commits.
func FetchRemoteCommit(ctx context.Context, repo, remoteRepo Repository, commitID string) error {
return globallock.LockAndDo(ctx, getRepoWriteLockKey(repo.RelativePath()), func(ctx context.Context) error {
return RunCmd(ctx, repo, gitcmd.NewCommand("fetch", "--no-tags").
AddDynamicArguments(repoPath(remoteRepo)).
AddDynamicArguments(commitID))
})
}

22
modules/gitrepo/merge.go Normal file
View File

@ -0,0 +1,22 @@
// Copyright 2025 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package gitrepo
import (
"context"
"fmt"
"strings"
"code.gitea.io/gitea/modules/git/gitcmd"
)
// MergeBase checks and returns merge base of two commits.
func MergeBase(ctx context.Context, repo Repository, baseCommitID, headCommitID string) (string, error) {
mergeBase, err := RunCmdString(ctx, repo, gitcmd.NewCommand("merge-base").
AddDashesAndList(baseCommitID, headCommitID))
if err != nil {
return "", fmt.Errorf("get merge-base of %s and %s failed: %w", baseCommitID, headCommitID, err)
}
return strings.TrimSpace(mergeBase), nil
}

10
modules/gitrepo/repo_lock.go Normal file
View File

@ -0,0 +1,10 @@
// Copyright 2026 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package gitrepo
// getRepoWriteLockKey returns the global lock key for write operations on the repository.
// Parallel write operations on the same git repository should be avoided to prevent data corruption.
func getRepoWriteLockKey(repoStoragePath string) string {
return "repo-write:" + repoStoragePath
}

2
routers/web/repo/issue_view.go
View File

@ -495,7 +495,7 @@ func preparePullViewSigning(ctx *context.Context, issue *issues_model.Issue) {
pull := issue.PullRequest
ctx.Data["WillSign"] = false
if ctx.Doer != nil {
sign, key, _, err := asymkey_service.SignMerge(ctx, pull, ctx.Doer, ctx.Repo.GitRepo, pull.BaseBranch, pull.GetGitHeadRefName())
sign, key, _, err := asymkey_service.SignMerge(ctx, pull, ctx.Doer, ctx.Repo.GitRepo)
ctx.Data["WillSign"] = sign
ctx.Data["SigningKeyMergeDisplay"] = asymkey_model.GetDisplaySigningKey(key)
if err != nil {

33
services/asymkey/sign.go
View File

@ -271,13 +271,22 @@ Loop:
}
// SignMerge determines if we should sign a PR merge commit to the base repository
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) {
func SignMerge(ctx context.Context, pr *issues_model.PullRequest, u *user_model.User, gitRepo *git.Repository) (bool, *git.SigningKey, *git.Signature, error) {
if err := pr.LoadBaseRepo(ctx); err != nil {
log.Error("Unable to get Base Repo for pull request")
return false, nil, nil, err
}
repo := pr.BaseRepo
baseCommit, err := gitRepo.GetCommit(pr.BaseBranch)
if err != nil {
return false, nil, nil, err
}
headCommit, err := gitRepo.GetCommit(pr.GetGitHeadRefName())
if err != nil {
return false, nil, nil, err
}
signingKey, signer := gitrepo.GetSigningKey(ctx)
if signingKey == nil {
return false, nil, nil, &ErrWontSign{noKey}
@ -319,38 +328,26 @@ Loop:
return false, nil, nil, &ErrWontSign{approved}
}
case baseSigned:
commit, err := gitRepo.GetCommit(baseCommit)
if err != nil {
return false, nil, nil, err
}
verification := ParseCommitWithSignature(ctx, commit)
verification := ParseCommitWithSignature(ctx, baseCommit)
if !verification.Verified {
return false, nil, nil, &ErrWontSign{baseSigned}
}
case headSigned:
commit, err := gitRepo.GetCommit(headCommit)
if err != nil {
return false, nil, nil, err
}
verification := ParseCommitWithSignature(ctx, commit)
verification := ParseCommitWithSignature(ctx, headCommit)
if !verification.Verified {
return false, nil, nil, &ErrWontSign{headSigned}
}
case commitsSigned:
commit, err := gitRepo.GetCommit(headCommit)
if err != nil {
return false, nil, nil, err
}
verification := ParseCommitWithSignature(ctx, commit)
verification := ParseCommitWithSignature(ctx, headCommit)
if !verification.Verified {
return false, nil, nil, &ErrWontSign{commitsSigned}
}
// need to work out merge-base
mergeBaseCommit, _, err := gitRepo.GetMergeBase("", baseCommit, headCommit)
mergeBaseCommit, err := gitrepo.MergeBase(ctx, pr.BaseRepo, baseCommit.ID.String(), headCommit.ID.String())
if err != nil {
return false, nil, nil, err
}
commitList, err := commit.CommitsBeforeUntil(mergeBaseCommit)
commitList, err := headCommit.CommitsBeforeUntil(mergeBaseCommit)
if err != nil {
return false, nil, nil, err
}

81
services/git/compare.go
View File

@ -6,14 +6,10 @@ package git
import (
"context"
"fmt"
"strconv"
"time"
repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/gitrepo"
"code.gitea.io/gitea/modules/graceful"
logger "code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/util"
)
@ -48,25 +44,6 @@ func (ci *CompareInfo) DirectComparison() bool {
// GetCompareInfo generates and returns compare information between base and head branches of repositories.
func GetCompareInfo(ctx context.Context, baseRepo, headRepo *repo_model.Repository, headGitRepo *git.Repository, baseRef, headRef git.RefName, directComparison, fileOnly bool) (_ *CompareInfo, err error) {
var (
remoteBranch string
tmpRemote string
)
// We don't need a temporary remote for same repository.
if baseRepo.ID != headRepo.ID {
// Add a temporary remote
tmpRemote = strconv.FormatInt(time.Now().UnixNano(), 10)
if err = gitrepo.GitRemoteAdd(ctx, headRepo, tmpRemote, baseRepo.RepoPath()); err != nil {
return nil, fmt.Errorf("GitRemoteAdd: %w", err)
}
defer func() {
if err := gitrepo.GitRemoteRemove(graceful.GetManager().ShutdownContext(), headRepo, tmpRemote); err != nil {
logger.Error("GetPullRequestInfo: GitRemoteRemove: %v", err)
}
}()
}
compareInfo := &CompareInfo{
BaseRepo: baseRepo,
BaseRef: baseRef,
@ -76,47 +53,49 @@ func GetCompareInfo(ctx context.Context, baseRepo, headRepo *repo_model.Reposito
CompareSeparator: util.Iif(directComparison, "..", "..."),
}
compareInfo.BaseCommitID, err = gitrepo.GetFullCommitID(ctx, baseRepo, baseRef.String())
if err != nil {
return nil, err
}
compareInfo.HeadCommitID, err = gitrepo.GetFullCommitID(ctx, headRepo, headRef.String())
if err != nil {
compareInfo.HeadCommitID = headRef.String()
return nil, err
}
// FIXME: It seems we don't need mergebase if it's a direct comparison?
compareInfo.MergeBase, remoteBranch, err = headGitRepo.GetMergeBase(tmpRemote, baseRef.String(), headRef.String())
if err == nil {
compareInfo.BaseCommitID, err = gitrepo.GetFullCommitID(ctx, headRepo, remoteBranch)
if err != nil {
compareInfo.BaseCommitID = remoteBranch
}
separator := "..."
baseCommitID := compareInfo.MergeBase
if directComparison {
separator = ".."
baseCommitID = compareInfo.BaseCommitID
}
// We have a common base - therefore we know that ... should work
if !fileOnly {
compareInfo.Commits, err = headGitRepo.ShowPrettyFormatLogToList(ctx, baseCommitID+separator+headRef.String())
if err != nil {
return nil, fmt.Errorf("ShowPrettyFormatLogToList: %w", err)
// if they are not the same repository, then we need to fetch the base commit into the head repository
// because we will use headGitRepo in the following code
if baseRepo.ID != headRepo.ID {
exist := headGitRepo.IsReferenceExist(compareInfo.BaseCommitID)
if !exist {
if err := gitrepo.FetchRemoteCommit(ctx, headRepo, baseRepo, compareInfo.BaseCommitID); err != nil {
return nil, fmt.Errorf("FetchRemoteCommit: %w", err)
}
} else {
compareInfo.Commits = []*git.Commit{}
}
}
if !directComparison {
compareInfo.MergeBase, err = gitrepo.MergeBase(ctx, headRepo, compareInfo.BaseCommitID, compareInfo.HeadCommitID)
if err != nil {
return nil, fmt.Errorf("MergeBase: %w", err)
}
} else {
compareInfo.MergeBase = compareInfo.BaseCommitID
}
// We have a common base - therefore we know that ... should work
if !fileOnly {
compareInfo.Commits, err = headGitRepo.ShowPrettyFormatLogToList(ctx, compareInfo.BaseCommitID+compareInfo.CompareSeparator+compareInfo.HeadCommitID)
if err != nil {
return nil, fmt.Errorf("ShowPrettyFormatLogToList: %w", err)
}
} else {
compareInfo.Commits = []*git.Commit{}
compareInfo.MergeBase, err = gitrepo.GetFullCommitID(ctx, headRepo, remoteBranch)
if err != nil {
compareInfo.MergeBase = remoteBranch
}
compareInfo.BaseCommitID = compareInfo.MergeBase
}
// Count number of changed files.
// This probably should be removed as we need to use shortstat elsewhere
// Now there is git diff --shortstat but this appears to be slower than simply iterating with --nameonly
compareInfo.NumFiles, err = headGitRepo.GetDiffNumChangedFiles(remoteBranch, headRef.String(), directComparison)
compareInfo.NumFiles, err = headGitRepo.GetDiffNumChangedFiles(compareInfo.BaseCommitID, compareInfo.HeadCommitID, directComparison)
if err != nil {
return nil, err
}

22
services/issue/pull.go
View File

@ -7,35 +7,16 @@ import (
"context"
"fmt"
"slices"
"time"
issues_model "code.gitea.io/gitea/models/issues"
org_model "code.gitea.io/gitea/models/organization"
repo_model "code.gitea.io/gitea/models/repo"
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/graceful"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
)
func getMergeBase(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, pr *issues_model.PullRequest, baseBranch, headBranch string) (string, error) {
// Add a temporary remote
tmpRemote := fmt.Sprintf("mergebase-%d-%d", pr.ID, time.Now().UnixNano())
if err := gitrepo.GitRemoteAdd(ctx, repo, tmpRemote, gitRepo.Path); err != nil {
return "", fmt.Errorf("GitRemoteAdd: %w", err)
}
defer func() {
if err := gitrepo.GitRemoteRemove(graceful.GetManager().ShutdownContext(), repo, tmpRemote); err != nil {
log.Error("getMergeBase: GitRemoteRemove: %v", err)
}
}()
mergeBase, _, err := gitRepo.GetMergeBase(tmpRemote, baseBranch, headBranch)
return mergeBase, err
}
type ReviewRequestNotifier struct {
Comment *issues_model.Comment
IsAdd bool
@ -99,11 +80,10 @@ func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque
}
// get the mergebase
mergeBase, err := getMergeBase(ctx, pr.BaseRepo, repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitHeadRefName())
mergeBase, err := gitrepo.MergeBase(ctx, pr.BaseRepo, git.BranchPrefix+pr.BaseBranch, pr.GetGitHeadRefName())
if err != nil {
return nil, err
}
// https://github.com/go-gitea/gitea/issues/29763, we need to get the files changed
// between the merge base and the head commit but not the base branch and the head commit
changedFiles, err := repo.GetFilesChangedBetween(mergeBase, pr.GetGitHeadRefName())

2
services/migrations/gitea_uploader.go
View File

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

2
services/pull/check.go
View File

@ -238,7 +238,7 @@ func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer
}
defer closer.Close()
sign, _, _, err := asymkey_service.SignMerge(ctx, pr, doer, gitRepo, pr.BaseBranch, pr.GetGitHeadRefName())
sign, _, _, err := asymkey_service.SignMerge(ctx, pr, doer, gitRepo)
return sign, err
}

5
services/pull/merge_prepare.go
View File

@ -19,6 +19,7 @@ import (
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/git/gitcmd"
"code.gitea.io/gitea/modules/gitrepo"
"code.gitea.io/gitea/modules/log"
asymkey_service "code.gitea.io/gitea/services/asymkey"
)
@ -105,7 +106,7 @@ func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullReque
mergeCtx.sig = doer.NewGitSig()
mergeCtx.committer = mergeCtx.sig
gitRepo, err := git.OpenRepository(ctx, mergeCtx.tmpBasePath)
gitRepo, err := gitrepo.OpenRepository(ctx, pr.BaseRepo)
if err != nil {
defer cancel()
return nil, nil, fmt.Errorf("failed to open temp git repo for pr[%d]: %w", mergeCtx.pr.ID, err)
@ -113,7 +114,7 @@ func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullReque
defer gitRepo.Close()
// Determine if we should sign
sign, key, signer, _ := asymkey_service.SignMerge(ctx, mergeCtx.pr, mergeCtx.doer, gitRepo, "HEAD", trackingBranch)
sign, key, signer, _ := asymkey_service.SignMerge(ctx, pr, doer, gitRepo)
if sign {
mergeCtx.signKey = key
if pr.BaseRepo.GetTrustModel() == repo_model.CommitterTrustModel || pr.BaseRepo.GetTrustModel() == repo_model.CollaboratorCommitterTrustModel {

9
services/pull/pull.go
View File

@ -521,14 +521,7 @@ func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest,
}
defer cancel()
tmpRepo, err := git.OpenRepository(ctx, prCtx.tmpBasePath)
if err != nil {
return false, "", fmt.Errorf("OpenRepository: %w", err)
}
defer tmpRepo.Close()
// Find the merge-base
mergeBase, _, err = tmpRepo.GetMergeBase("", "base", "tracking")
mergeBase, err = gitrepo.MergeBase(ctx, pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName())
if err != nil {
return false, "", fmt.Errorf("GetMergeBase: %w", err)
}