From a1f07dec763583740689e2b01621fe02dc968c74 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 7 Feb 2021 16:54:45 +0000 Subject: [PATCH 1/5] HasPreviousCommit causes recursive load of commits unnecessarily This PR improves HasPreviousCommit to prevent the automatic and recursive loading of previous commits using git merge-base --is-ancestor and git rev-list Related #13684 Signed-off-by: Andrew Thornton --- modules/git/commit.go | 21 -------------------- modules/git/commit_gogit.go | 28 ++++++++++++++++++++++++++ modules/git/commit_nogogit.go | 37 +++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 21 deletions(-) create mode 100644 modules/git/commit_gogit.go create mode 100644 modules/git/commit_nogogit.go diff --git a/modules/git/commit.go b/modules/git/commit.go index ce82c2f58209c..c7835d1739069 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -262,27 +262,6 @@ func (c *Commit) CommitsBefore() (*list.List, error) { return c.repo.getCommitsBefore(c.ID) } -// 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 - } - } - return false, nil -} - // CommitsBeforeLimit returns num commits before current revision func (c *Commit) CommitsBeforeLimit(num int) (*list.List, error) { return c.repo.getCommitsBeforeLimit(c.ID, num) diff --git a/modules/git/commit_gogit.go b/modules/git/commit_gogit.go new file mode 100644 index 0000000000000..63282ccba3d93 --- /dev/null +++ b/modules/git/commit_gogit.go @@ -0,0 +1,28 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +// +build gogit + +package git + +// 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 + } + } + return false, nil +} diff --git a/modules/git/commit_nogogit.go b/modules/git/commit_nogogit.go new file mode 100644 index 0000000000000..9841449f7f489 --- /dev/null +++ b/modules/git/commit_nogogit.go @@ -0,0 +1,37 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +// +build !gogit + +package git + +import ( + "errors" + "os/exec" + "strings" +) + +// HasPreviousCommit returns true if a given commitHash is contained in commit's parents +func (c *Commit) HasPreviousCommit(commitHash SHA1) (bool, error) { + if err := CheckGitVersionAtLeast("1.8"); err != nil { + _, err := NewCommand("merge-base", "--ancestor", commitHash.String(), c.ID.String()).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", "-n1", commitHash.String()+".."+c.ID.String(), "--").RunInDir(c.repo.Path) + if err != nil { + return false, err + } + + return len(strings.TrimSpace(result)) > 0, nil +} From 52833efafb200012a29b3b4f29a63fac4dc9bbfe Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 7 Feb 2021 17:09:02 +0000 Subject: [PATCH 2/5] thinking on the go-git variant is bad too so it should just be removed Signed-off-by: Andrew Thornton --- modules/git/commit.go | 26 ++++++++++++++++++++++++ modules/git/commit_gogit.go | 28 -------------------------- modules/git/commit_nogogit.go | 37 ----------------------------------- 3 files changed, 26 insertions(+), 65 deletions(-) delete mode 100644 modules/git/commit_gogit.go delete mode 100644 modules/git/commit_nogogit.go diff --git a/modules/git/commit.go b/modules/git/commit.go index c7835d1739069..07151d36f7267 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" ) @@ -262,6 +264,30 @@ func (c *Commit) CommitsBefore() (*list.List, error) { return c.repo.getCommitsBefore(c.ID) } +// HasPreviousCommit returns true if a given commitHash is contained in commit's parents +func (c *Commit) HasPreviousCommit(commitHash SHA1) (bool, error) { + if err := CheckGitVersionAtLeast("1.8"); err != nil { + _, err := NewCommand("merge-base", "--ancestor", commitHash.String(), c.ID.String()).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", "-n1", commitHash.String()+".."+c.ID.String(), "--").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) diff --git a/modules/git/commit_gogit.go b/modules/git/commit_gogit.go deleted file mode 100644 index 63282ccba3d93..0000000000000 --- a/modules/git/commit_gogit.go +++ /dev/null @@ -1,28 +0,0 @@ -// Copyright 2021 The Gitea Authors. All rights reserved. -// Use of this source code is governed by a MIT-style -// license that can be found in the LICENSE file. - -// +build gogit - -package git - -// 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 - } - } - return false, nil -} diff --git a/modules/git/commit_nogogit.go b/modules/git/commit_nogogit.go deleted file mode 100644 index 9841449f7f489..0000000000000 --- a/modules/git/commit_nogogit.go +++ /dev/null @@ -1,37 +0,0 @@ -// Copyright 2021 The Gitea Authors. All rights reserved. -// Use of this source code is governed by a MIT-style -// license that can be found in the LICENSE file. - -// +build !gogit - -package git - -import ( - "errors" - "os/exec" - "strings" -) - -// HasPreviousCommit returns true if a given commitHash is contained in commit's parents -func (c *Commit) HasPreviousCommit(commitHash SHA1) (bool, error) { - if err := CheckGitVersionAtLeast("1.8"); err != nil { - _, err := NewCommand("merge-base", "--ancestor", commitHash.String(), c.ID.String()).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", "-n1", commitHash.String()+".."+c.ID.String(), "--").RunInDir(c.repo.Path) - if err != nil { - return false, err - } - - return len(strings.TrimSpace(result)) > 0, nil -} From 36809f93e1364e45f7e512e4f5432cee0053df00 Mon Sep 17 00:00:00 2001 From: zeripath Date: Mon, 8 Feb 2021 15:55:08 +0000 Subject: [PATCH 3/5] Update modules/git/commit.go --- modules/git/commit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/git/commit.go b/modules/git/commit.go index 07151d36f7267..00ae5dc7c7f62 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -267,7 +267,7 @@ 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) { if err := CheckGitVersionAtLeast("1.8"); err != nil { - _, err := NewCommand("merge-base", "--ancestor", commitHash.String(), c.ID.String()).RunInDir(c.repo.Path) + _, err := NewCommand("merge-base", "--is-ancestor", commitHash.String(), c.ID.String()).RunInDir(c.repo.Path) if err != nil { return true, nil } From 4712c03201549096b7c73a79a17bc6e41f547268 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 8 Feb 2021 17:35:46 +0000 Subject: [PATCH 4/5] Add test and fix multiple issues Signed-off-by: Andrew Thornton --- modules/git/commit.go | 6 +++--- modules/git/commit_test.go | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/modules/git/commit.go b/modules/git/commit.go index 00ae5dc7c7f62..35a06aa4f3443 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -266,9 +266,9 @@ 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) { - if err := CheckGitVersionAtLeast("1.8"); err != nil { + if err := CheckGitVersionAtLeast("1.8"); err == nil { _, err := NewCommand("merge-base", "--is-ancestor", commitHash.String(), c.ID.String()).RunInDir(c.repo.Path) - if err != nil { + if err == nil { return true, nil } var exitError *exec.ExitError @@ -280,7 +280,7 @@ func (c *Commit) HasPreviousCommit(commitHash SHA1) (bool, error) { return false, err } - result, err := NewCommand("rev-list", "-n1", commitHash.String()+".."+c.ID.String(), "--").RunInDir(c.repo.Path) + result, err := NewCommand("rev-list", "--ancestry-path", "-n1", commitHash.String()+".."+c.ID.String(), "--").RunInDir(c.repo.Path) if err != nil { return false, err } diff --git a/modules/git/commit_test.go b/modules/git/commit_test.go index 9b5fc8f6609cb..733ff150f7475 100644 --- a/modules/git/commit_test.go +++ b/modules/git/commit_test.go @@ -105,3 +105,24 @@ 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) +} From 48ecb1ff22868201ab1c5f650ebc7ca3ba2d054f Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 8 Feb 2021 19:54:53 +0000 Subject: [PATCH 5/5] --is-ancestor returns true if the commit is the same - fix yet another issue Signed-off-by: Andrew Thornton --- modules/git/commit.go | 11 +++++++++-- modules/git/commit_test.go | 4 ++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/modules/git/commit.go b/modules/git/commit.go index 35a06aa4f3443..027642720d0ab 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -266,8 +266,15 @@ 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) { + 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", commitHash.String(), c.ID.String()).RunInDir(c.repo.Path) + _, err := NewCommand("merge-base", "--is-ancestor", that, this).RunInDir(c.repo.Path) if err == nil { return true, nil } @@ -280,7 +287,7 @@ func (c *Commit) HasPreviousCommit(commitHash SHA1) (bool, error) { return false, err } - result, err := NewCommand("rev-list", "--ancestry-path", "-n1", commitHash.String()+".."+c.ID.String(), "--").RunInDir(c.repo.Path) + result, err := NewCommand("rev-list", "--ancestry-path", "-n1", that+".."+this, "--").RunInDir(c.repo.Path) if err != nil { return false, err } diff --git a/modules/git/commit_test.go b/modules/git/commit_test.go index 733ff150f7475..0925a6ce6ac1c 100644 --- a/modules/git/commit_test.go +++ b/modules/git/commit_test.go @@ -125,4 +125,8 @@ func TestHasPreviousCommit(t *testing.T) { 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) }