From f4d3120f9d1de6a260a5e625b3ffa6b35a069e9b Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 9 Aug 2024 10:40:45 +0800 Subject: [PATCH] Fix `IsObjectExist` with gogit (#31790) Fix #31271. When gogit is enabled, `IsObjectExist` calls `repo.gogitRepo.ResolveRevision`, which is not correct. It's for checking references not objects, it could work with commit hash since it's both a valid reference and a commit object, but it doesn't work with blob objects. So it causes #31271 because it reports that all blob objects do not exist. --- modules/git/repo_branch_gogit.go | 33 +++++---- modules/git/repo_branch_nogogit.go | 2 +- modules/git/repo_branch_test.go | 105 +++++++++++++++++++++++++++++ modules/markup/html.go | 3 +- 4 files changed, 127 insertions(+), 16 deletions(-) diff --git a/modules/git/repo_branch_gogit.go b/modules/git/repo_branch_gogit.go index d1ec14d81..dbc4a5fed 100644 --- a/modules/git/repo_branch_gogit.go +++ b/modules/git/repo_branch_gogit.go @@ -14,30 +14,35 @@ import ( "github.com/go-git/go-git/v5/plumbing/storer" ) -// IsObjectExist returns true if given reference exists in the repository. +// IsObjectExist returns true if the given object exists in the repository. +// FIXME: Inconsistent behavior with nogogit edition +// Unlike the implementation of IsObjectExist in nogogit edition, it does not support short hashes here. +// For example, IsObjectExist("153f451") will return false, but it will return true in nogogit edition. +// To fix this, the solution could be adding support for short hashes in gogit edition if it's really needed. func (repo *Repository) IsObjectExist(name string) bool { if name == "" { return false } + _, err := repo.gogitRepo.Object(plumbing.AnyObject, plumbing.NewHash(name)) + return err == nil +} + +// IsReferenceExist returns true if given reference exists in the repository. +// FIXME: Inconsistent behavior with nogogit edition +// Unlike the implementation of IsObjectExist in nogogit edition, it does not support blob hashes here. +// For example, IsObjectExist([existing_blob_hash]) will return false, but it will return true in nogogit edition. +// To fix this, the solution could be refusing to support blob hashes in nogogit edition since a blob hash is not a reference. +func (repo *Repository) IsReferenceExist(name string) bool { + if name == "" { + return false + } + _, err := repo.gogitRepo.ResolveRevision(plumbing.Revision(name)) return err == nil } -// IsReferenceExist returns true if given reference exists in the repository. -func (repo *Repository) IsReferenceExist(name string) bool { - if name == "" { - return false - } - - reference, err := repo.gogitRepo.Reference(plumbing.ReferenceName(name), true) - if err != nil { - return false - } - return reference.Type() != plumbing.InvalidReference -} - // IsBranchExist returns true if given branch exists in current repository. func (repo *Repository) IsBranchExist(name string) bool { if name == "" { diff --git a/modules/git/repo_branch_nogogit.go b/modules/git/repo_branch_nogogit.go index 470faebe2..63d0f7268 100644 --- a/modules/git/repo_branch_nogogit.go +++ b/modules/git/repo_branch_nogogit.go @@ -16,7 +16,7 @@ import ( "code.gitea.io/gitea/modules/log" ) -// IsObjectExist returns true if given reference exists in the repository. +// IsObjectExist returns true if the given object exists in the repository. func (repo *Repository) IsObjectExist(name string) bool { if name == "" { return false diff --git a/modules/git/repo_branch_test.go b/modules/git/repo_branch_test.go index fe788946e..009c54583 100644 --- a/modules/git/repo_branch_test.go +++ b/modules/git/repo_branch_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestRepository_GetBranches(t *testing.T) { @@ -94,3 +95,107 @@ func BenchmarkGetRefsBySha(b *testing.B) { _, _ = bareRepo5.GetRefsBySha("c83380d7056593c51a699d12b9c00627bd5743e9", "") _, _ = bareRepo5.GetRefsBySha("58a4bcc53ac13e7ff76127e0fb518b5262bf09af", "") } + +func TestRepository_IsObjectExist(t *testing.T) { + repo, err := openRepositoryWithDefaultContext(filepath.Join(testReposDir, "repo1_bare")) + require.NoError(t, err) + defer repo.Close() + + // FIXME: Inconsistent behavior between gogit and nogogit editions + // See the comment of IsObjectExist in gogit edition for more details. + supportShortHash := !isGogit + + tests := []struct { + name string + arg string + want bool + }{ + { + name: "empty", + arg: "", + want: false, + }, + { + name: "branch", + arg: "master", + want: false, + }, + { + name: "commit hash", + arg: "ce064814f4a0d337b333e646ece456cd39fab612", + want: true, + }, + { + name: "short commit hash", + arg: "ce06481", + want: supportShortHash, + }, + { + name: "blob hash", + arg: "153f451b9ee7fa1da317ab17a127e9fd9d384310", + want: true, + }, + { + name: "short blob hash", + arg: "153f451", + want: supportShortHash, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, repo.IsObjectExist(tt.arg)) + }) + } +} + +func TestRepository_IsReferenceExist(t *testing.T) { + repo, err := openRepositoryWithDefaultContext(filepath.Join(testReposDir, "repo1_bare")) + require.NoError(t, err) + defer repo.Close() + + // FIXME: Inconsistent behavior between gogit and nogogit editions + // See the comment of IsReferenceExist in gogit edition for more details. + supportBlobHash := !isGogit + + tests := []struct { + name string + arg string + want bool + }{ + { + name: "empty", + arg: "", + want: false, + }, + { + name: "branch", + arg: "master", + want: true, + }, + { + name: "commit hash", + arg: "ce064814f4a0d337b333e646ece456cd39fab612", + want: true, + }, + { + name: "short commit hash", + arg: "ce06481", + want: true, + }, + { + name: "blob hash", + arg: "153f451b9ee7fa1da317ab17a127e9fd9d384310", + want: supportBlobHash, + }, + { + name: "short blob hash", + arg: "153f451", + want: supportBlobHash, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, repo.IsReferenceExist(tt.arg)) + }) + } +} diff --git a/modules/markup/html.go b/modules/markup/html.go index b8069d459..8d3327c49 100644 --- a/modules/markup/html.go +++ b/modules/markup/html.go @@ -1144,7 +1144,8 @@ func hashCurrentPatternProcessor(ctx *RenderContext, node *html.Node) { }) } - exist = ctx.GitRepo.IsObjectExist(hash) + // Don't use IsObjectExist since it doesn't support short hashs with gogit edition. + exist = ctx.GitRepo.IsReferenceExist(hash) ctx.ShaExistCache[hash] = exist }