From f9abf94bd99b5b9c603037aaad4dd327b68263bd Mon Sep 17 00:00:00 2001 From: zeripath Date: Wed, 10 Feb 2021 07:00:57 +0000 Subject: [PATCH] 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 --- modules/git/commit.go | 38 +++++++++++++++++++++++++------------- modules/git/commit_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/modules/git/commit.go b/modules/git/commit.go index ce82c2f58..027642720 100644 --- a/modules/git/commit.go +++ b/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,23 +266,33 @@ 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 { + 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 } - commitInParentCommit, err := commit.HasPreviousCommit(commitHash) - if err != nil { - return false, err - } - if commitInParentCommit { - 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 false, nil + + return len(strings.TrimSpace(result)) > 0, nil } // CommitsBeforeLimit returns num commits before current revision diff --git a/modules/git/commit_test.go b/modules/git/commit_test.go index 9b5fc8f66..0925a6ce6 100644 --- a/modules/git/commit_test.go +++ b/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) +}