feat: avoid remote for codeowner's merge base (#9610)

The codeowner features computes the mergebase (I'm not exactly sure why, because this should be stored in the database in `merge_base` column, but if there's no harm to compute it again as that will always be the correct answer) in order to get the changed files between the merge base and the head commit. To do this a function was used that adds a remote... my best reasoning is that this was done because the only function that that was exported on the repository struct had this requirement. Add a new function that *simply* computes the merge base without requiring a remote.

The main benefit of not using a remote is that within Codeberg we are frequently seeing `config.lock` being lingered around (see forgejo/forgejo#1946) so its best to avoid modifying the config when possible - in this case it was completely unnecessary.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9610
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>

modules/git/repo_compare.go

@@ -30,6 +30,12 @@ type CompareInfo struct {
 	NumFiles     int
 }
 
+// GetMergeBaseSimple returns the merge base of base and head.
+func (repo *Repository) GetMergeBaseSimple(base, head string) (string, error) {
+	stdout, _, err := NewCommand(repo.Ctx, "merge-base").AddDashesAndList(base, head).RunStdString(&RunOpts{Dir: repo.Path})
+	return strings.TrimSpace(stdout), err
+}
+
 // 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 == "" {

modules/git/repo_compare_test.go

@@ -302,3 +302,28 @@ func TestGetShortStat(t *testing.T) {
 		assert.Zero(t, totalDeletions)
 	})
 }
+
+func TestGetMergeBaseSimple(t *testing.T) {
+	repo, err := OpenRepository(t.Context(), filepath.Join(testReposDir, "symmetric_repo"))
+	require.NoError(t, err)
+
+	defer repo.Close()
+
+	t.Run("Normal", func(t *testing.T) {
+		mergebase, err := repo.GetMergeBaseSimple("main", "br2")
+		require.NoError(t, err)
+		assert.Equal(t, "9d36f18c8ca14ad28c4751afd14f3e3146a785dc", mergebase)
+	})
+
+	t.Run("No mergebase", func(t *testing.T) {
+		mergebase, err := repo.GetMergeBaseSimple("main", "br3")
+		require.ErrorContains(t, err, "exit status 1")
+		assert.Empty(t, mergebase)
+	})
+
+	t.Run("Multiple mergebase", func(t *testing.T) {
+		mergebase, err := repo.GetMergeBaseSimple("main", "br1")
+		require.NoError(t, err)
+		assert.Equal(t, "9d36f18c8ca14ad28c4751afd14f3e3146a785dc", mergebase)
+	})
+}

services/issue/pull.go

@@ -6,7 +6,6 @@ package issue
 import (
 	"context"
 	"fmt"
-	"time"
 
 	issues_model "forgejo.org/models/issues"
 	org_model "forgejo.org/models/organization"
@@ -19,22 +18,6 @@ import (
 	"forgejo.org/modules/setting"
 )
 
-func getMergeBase(repo *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 := repo.AddRemote(tmpRemote, repo.Path, false); err != nil {
-		return "", fmt.Errorf("AddRemote: %w", err)
-	}
-	defer func() {
-		if err := repo.RemoveRemote(tmpRemote); err != nil {
-			log.Error("getMergeBase: RemoveRemote: %v", err)
-		}
-	}()
-
-	mergeBase, _, err := repo.GetMergeBase(tmpRemote, baseBranch, headBranch)
-	return mergeBase, err
-}
-
 type ReviewRequestNotifier struct {
 	Comment    *issues_model.Comment
 	IsAdd      bool
@@ -81,8 +64,7 @@ func PullRequestCodeOwnersReview(ctx context.Context, issue *issues_model.Issue,
 		}
 	}
 
-	// get the mergebase
-	mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName())
+	mergeBase, err := repo.GetMergeBaseSimple(git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName())
 	if err != nil {
 		return nil, err
 	}