improvement

modules/gitrepo/merge.go

@@ -20,15 +20,3 @@ func MergeBase(ctx context.Context, repo Repository, baseCommitID, headCommitID
 	}
 	return strings.TrimSpace(mergeBase), nil
 }
-
-// MergeBaseFromRemote checks and returns merge base of two commits from different repositories.
-func MergeBaseFromRemote(ctx context.Context, baseRepo, headRepo Repository, baseCommitID, headCommitID string) (string, error) {
-	// fetch head commit id into the current repository if the repositories are different
-	if baseRepo.RelativePath() != headRepo.RelativePath() {
-		if err := FetchRemoteCommit(ctx, baseRepo, headRepo, headCommitID); err != nil {
-			return "", fmt.Errorf("FetchRemoteCommit: %w", err)
-		}
-	}
-
-	return MergeBase(ctx, baseRepo, baseCommitID, headCommitID)
-}

routers/web/repo/issue_view.go

@@ -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 {

services/asymkey/sign.go

@@ -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)
+			verification := ParseCommitWithSignature(ctx, baseCommit)
-			if err != nil {
-				return false, nil, nil, err
-			}
-			verification := ParseCommitWithSignature(ctx, commit)
 			if !verification.Verified {
 				return false, nil, nil, &ErrWontSign{baseSigned}
 			}
 		case headSigned:
-			commit, err := gitRepo.GetCommit(headCommit)
+			verification := ParseCommitWithSignature(ctx, headCommit)
-			if err != nil {
-				return false, nil, nil, err
-			}
-			verification := ParseCommitWithSignature(ctx, commit)
 			if !verification.Verified {
 				return false, nil, nil, &ErrWontSign{headSigned}
 			}
 		case commitsSigned:
-			commit, err := gitRepo.GetCommit(headCommit)
+			verification := ParseCommitWithSignature(ctx, headCommit)
-			if err != nil {
-				return false, nil, nil, err
-			}
-			verification := ParseCommitWithSignature(ctx, commit)
 			if !verification.Verified {
 				return false, nil, nil, &ErrWontSign{commitsSigned}
 			}
 			// need to work out merge-base
-			mergeBaseCommit, err := gitrepo.MergeBaseFromRemote(ctx, pr.BaseRepo, pr.HeadRepo, 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
 			}

services/git/compare.go

@@ -6,14 +6,11 @@ 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"
+	"code.gitea.io/gitea/modules/util"
-	logger "code.gitea.io/gitea/modules/log"
 )
 
 // CompareInfo represents needed information for comparing references.
@@ -41,22 +38,6 @@ func (ci *CompareInfo) IsSameRef() 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 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,
@@ -66,48 +47,43 @@ func GetCompareInfo(ctx context.Context, baseRepo, headRepo *repo_model.Reposito
 		DirectComparison: 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
-	}
-
-	compareInfo.MergeBase, err = gitrepo.MergeBaseFromRemote(ctx, baseRepo, headRepo, compareInfo.BaseCommitID, compareInfo.HeadCommitID)
-	if err == nil {
-		compareInfo.BaseCommitID, err = gitrepo.GetFullCommitID(ctx, headRepo, headRef.String())
-		if err != nil {
-			compareInfo.BaseCommitID = headRef.String()
-		}
-		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)
-			}
-		} else {
-			compareInfo.Commits = []*git.Commit{}
-		}
-	} else {
-		compareInfo.Commits = []*git.Commit{}
-		compareInfo.MergeBase, err = gitrepo.GetFullCommitID(ctx, headRepo, headRef.String())
-		if err != nil {
-			compareInfo.MergeBase = headRef.String()
-		}
-		compareInfo.BaseCommitID = compareInfo.MergeBase
 	}
 
+	// 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 {
 		if err := gitrepo.FetchRemoteCommit(ctx, headRepo, baseRepo, compareInfo.BaseCommitID); err != nil {
 			return nil, fmt.Errorf("FetchRemoteCommit: %w", err)
 		}
 	}
 
+	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 {
+		separator := util.Iif(directComparison, "..", "...")
+		compareInfo.Commits, err = headGitRepo.ShowPrettyFormatLogToList(ctx, compareInfo.BaseCommitID+separator+compareInfo.HeadCommitID)
+		if err != nil {
+			return nil, fmt.Errorf("ShowPrettyFormatLogToList: %w", err)
+		}
+	} else {
+		compareInfo.Commits = []*git.Commit{}
+	}
+
 	// 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

services/issue/pull.go

@@ -81,7 +81,7 @@ func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque
 
 	// get the mergebase
 	if pr.MergeBase == "" {
-		mergeBase, err := gitrepo.MergeBaseFromRemote(ctx, pr.BaseRepo, pr.HeadRepo, 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
 		}

services/pull/check.go

@@ -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
 }

services/pull/merge_prepare.go

@@ -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 {

services/pull/pull.go

@@ -511,7 +511,7 @@ func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest,
 	}
 	defer cancel()
 
-	mergeBase, err = gitrepo.MergeBaseFromRemote(ctx, pr.BaseRepo, pr.HeadRepo, pr.BaseBranch, pr.HeadBranch)
+	mergeBase, err = gitrepo.MergeBase(ctx, pr.BaseRepo, pr.BaseBranch, pr.GetGitHeadRefName())
 	if err != nil {
 		return false, "", fmt.Errorf("GetMergeBase: %w", err)
 	}