HasPreviousCommit causes recursive load of commits unnecessarily (#14598)

This PR improves HasPreviousCommit to prevent the automatic and recursive loading
of previous commits using git merge-base --is-ancestor and git rev-list

Fix #13684

Signed-off-by: Andrew Thornton <art27@cantab.net>

modules/git/commit.go

@@ -9,6 +9,7 @@ import (
 	"bufio"
 	"bytes"
 	"container/list"
+	"errors"
 	"fmt"
 	"image"
 	"image/color"
@@ -17,6 +18,7 @@ import (
 	_ "image/png"  // for processing png images
 	"io"
 	"net/http"
+	"os/exec"
 	"strconv"
 	"strings"
 )
@@ -264,25 +266,35 @@ func (c *Commit) CommitsBefore() (*list.List, error) {
 
 // HasPreviousCommit returns true if a given commitHash is contained in commit's parents
 func (c *Commit) HasPreviousCommit(commitHash SHA1) (bool, error) {
-	for i := 0; i < c.ParentCount(); i++ {
-		commit, err := c.Parent(i)
-		if err != nil {
-			return false, err
-		}
-		if commit.ID == commitHash {
-			return true, nil
-		}
-		commitInParentCommit, err := commit.HasPreviousCommit(commitHash)
-		if err != nil {
-			return false, err
-		}
-		if commitInParentCommit {
-			return true, nil
-		}
-	}
+	this := c.ID.String()
+	that := commitHash.String()
+
+	if this == that {
 		return false, nil
 	}
 
+	if err := CheckGitVersionAtLeast("1.8"); err == nil {
+		_, err := NewCommand("merge-base", "--is-ancestor", that, this).RunInDir(c.repo.Path)
+		if err == nil {
+			return true, nil
+		}
+		var exitError *exec.ExitError
+		if errors.As(err, &exitError) {
+			if exitError.ProcessState.ExitCode() == 1 && len(exitError.Stderr) == 0 {
+				return false, nil
+			}
+		}
+		return false, err
+	}
+
+	result, err := NewCommand("rev-list", "--ancestry-path", "-n1", that+".."+this, "--").RunInDir(c.repo.Path)
+	if err != nil {
+		return false, err
+	}
+
+	return len(strings.TrimSpace(result)) > 0, nil
+}
+
 // CommitsBeforeLimit returns num commits before current revision
 func (c *Commit) CommitsBeforeLimit(num int) (*list.List, error) {
 	return c.repo.getCommitsBeforeLimit(c.ID, num)

modules/git/commit_test.go

@@ -105,3 +105,28 @@ empty commit`, commitFromReader.Signature.Payload)
 	commitFromReader.Signature.Payload += "\n\n"
 	assert.EqualValues(t, commitFromReader, commitFromReader2)
 }
+
+func TestHasPreviousCommit(t *testing.T) {
+	bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
+
+	repo, err := OpenRepository(bareRepo1Path)
+	assert.NoError(t, err)
+
+	commit, err := repo.GetCommit("8006ff9adbf0cb94da7dad9e537e53817f9fa5c0")
+	assert.NoError(t, err)
+
+	parentSHA := MustIDFromString("8d92fc957a4d7cfd98bc375f0b7bb189a0d6c9f2")
+	notParentSHA := MustIDFromString("2839944139e0de9737a044f78b0e4b40d989a9e3")
+
+	haz, err := commit.HasPreviousCommit(parentSHA)
+	assert.NoError(t, err)
+	assert.True(t, haz)
+
+	hazNot, err := commit.HasPreviousCommit(notParentSHA)
+	assert.NoError(t, err)
+	assert.False(t, hazNot)
+
+	selfNot, err := commit.HasPreviousCommit(commit.ID)
+	assert.NoError(t, err)
+	assert.False(t, selfNot)
+}