From fad8ba3d070339d54cb373641a82315e02b6b32c Mon Sep 17 00:00:00 2001 From: Jason Song Date: Wed, 7 Aug 2024 16:12:11 +0800 Subject: [PATCH 1/5] test: TestRepository_IsObjectExist --- modules/git/repo_branch_test.go | 39 +++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/modules/git/repo_branch_test.go b/modules/git/repo_branch_test.go index fe788946e500b..6fe473e7fd9d8 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,41 @@ 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() + + tests := []struct { + name string + arg string + want bool + }{ + { + name: "empty", + arg: "", + want: false, + }, + { + name: "branch", + arg: "master", + want: false, + }, + { + name: "commit object", + arg: "ce064814f4a0d337b333e646ece456cd39fab612", + want: true, + }, + { + name: "blob object", + arg: "153f451b9ee7fa1da317ab17a127e9fd9d384310", + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, repo.IsObjectExist(tt.arg)) + }) + } +} From 21273d8307edcfc710c45e19174193e7a1d6cc47 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Wed, 7 Aug 2024 16:23:08 +0800 Subject: [PATCH 2/5] fix: new IsObjectExist --- modules/git/repo_branch_gogit.go | 4 ++-- modules/git/repo_branch_nogogit.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/git/repo_branch_gogit.go b/modules/git/repo_branch_gogit.go index d1ec14d81155f..63e3f9801e844 100644 --- a/modules/git/repo_branch_gogit.go +++ b/modules/git/repo_branch_gogit.go @@ -14,13 +14,13 @@ 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. func (repo *Repository) IsObjectExist(name string) bool { if name == "" { return false } - _, err := repo.gogitRepo.ResolveRevision(plumbing.Revision(name)) + _, err := repo.gogitRepo.Object(plumbing.AnyObject, plumbing.NewHash(name)) return err == nil } diff --git a/modules/git/repo_branch_nogogit.go b/modules/git/repo_branch_nogogit.go index 470faebe25f79..63d0f7268a65d 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 From c651a82f36181a1ce22fa751bc141cc18f0053e4 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Thu, 8 Aug 2024 12:18:32 +0800 Subject: [PATCH 3/5] fix: support short hashes --- modules/git/repo_branch_gogit.go | 38 ++++++++++++++++++++++++++++++-- modules/git/repo_branch_test.go | 14 ++++++++++-- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/modules/git/repo_branch_gogit.go b/modules/git/repo_branch_gogit.go index 63e3f9801e844..52f38c84ed2a0 100644 --- a/modules/git/repo_branch_gogit.go +++ b/modules/git/repo_branch_gogit.go @@ -7,10 +7,14 @@ package git import ( + "encoding/hex" + "errors" "sort" "strings" "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/hash" + "github.com/go-git/go-git/v5/plumbing/object" "github.com/go-git/go-git/v5/plumbing/storer" ) @@ -20,9 +24,39 @@ func (repo *Repository) IsObjectExist(name string) bool { return false } - _, err := repo.gogitRepo.Object(plumbing.AnyObject, plumbing.NewHash(name)) + h := plumbing.NewHash(name) + if len(name) >= hash.HexSize { + _, err := repo.gogitRepo.Object(plumbing.AnyObject, h) + return err == nil + } + + // FIXME: Unlike IsObjectExist in nogogit edition, gogitRepo.Object does support short hashes. + // To be consistent with nogogit edition, we have to iterate all objects to find the object. + // This is not efficient, but it seems to be the only way to do it. + // In such situation, we should avoid using short hashes with IsObjectExist. + // Or consider refusing short hashes with nogogit editions too to maintain consistency, to avoid potential bugs. + + // Check if the name is a valid hash, to avoid unnecessary iteration. + // A string with odd length is not a valid hash, but it's a valid prefix of a hash. + if _, err := hex.DecodeString(name); err != nil && !errors.Is(err, hex.ErrLength) { + return false + } + + iter, err := repo.gogitRepo.Objects() + if err != nil { + return false + } + defer iter.Close() - return err == nil + found := false + _ = iter.ForEach(func(obj object.Object) error { + if strings.HasPrefix(obj.ID().String(), name) { + found = true + return storer.ErrStop + } + return nil + }) + return found } // IsReferenceExist returns true if given reference exists in the repository. diff --git a/modules/git/repo_branch_test.go b/modules/git/repo_branch_test.go index 6fe473e7fd9d8..6ad8abbed40ff 100644 --- a/modules/git/repo_branch_test.go +++ b/modules/git/repo_branch_test.go @@ -117,15 +117,25 @@ func TestRepository_IsObjectExist(t *testing.T) { want: false, }, { - name: "commit object", + name: "commit hash", arg: "ce064814f4a0d337b333e646ece456cd39fab612", want: true, }, { - name: "blob object", + name: "short commit hash", + arg: "ce06481", + want: true, + }, + { + name: "blob hash", arg: "153f451b9ee7fa1da317ab17a127e9fd9d384310", want: true, }, + { + name: "short blob hash", + arg: "153f451", + want: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 75abcc1f8608d1eeb6db83b0c709e1ea80df378e Mon Sep 17 00:00:00 2001 From: Jason Song Date: Thu, 8 Aug 2024 12:20:09 +0800 Subject: [PATCH 4/5] chore: add TODO --- modules/markup/html.go | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/markup/html.go b/modules/markup/html.go index b8069d459a075..a30ea5b59353c 100644 --- a/modules/markup/html.go +++ b/modules/markup/html.go @@ -1144,6 +1144,7 @@ func hashCurrentPatternProcessor(ctx *RenderContext, node *html.Node) { }) } + // TODO: Avoid to use IsObjectExist since it's not efficient with gogit. exist = ctx.GitRepo.IsObjectExist(hash) ctx.ShaExistCache[hash] = exist } From 95e58eff8a6587bbd52e89f2dc42a4497c4d11e8 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Thu, 8 Aug 2024 14:16:52 +0800 Subject: [PATCH 5/5] fix: use IsReferenceExist --- modules/git/repo_branch_gogit.go | 55 +++++++---------------------- modules/git/repo_branch_test.go | 60 ++++++++++++++++++++++++++++++-- modules/markup/html.go | 4 +-- 3 files changed, 73 insertions(+), 46 deletions(-) diff --git a/modules/git/repo_branch_gogit.go b/modules/git/repo_branch_gogit.go index 52f38c84ed2a0..dbc4a5fedc91b 100644 --- a/modules/git/repo_branch_gogit.go +++ b/modules/git/repo_branch_gogit.go @@ -7,69 +7,40 @@ package git import ( - "encoding/hex" - "errors" "sort" "strings" "github.com/go-git/go-git/v5/plumbing" - "github.com/go-git/go-git/v5/plumbing/hash" - "github.com/go-git/go-git/v5/plumbing/object" "github.com/go-git/go-git/v5/plumbing/storer" ) // 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 } - h := plumbing.NewHash(name) - if len(name) >= hash.HexSize { - _, err := repo.gogitRepo.Object(plumbing.AnyObject, h) - return err == nil - } - - // FIXME: Unlike IsObjectExist in nogogit edition, gogitRepo.Object does support short hashes. - // To be consistent with nogogit edition, we have to iterate all objects to find the object. - // This is not efficient, but it seems to be the only way to do it. - // In such situation, we should avoid using short hashes with IsObjectExist. - // Or consider refusing short hashes with nogogit editions too to maintain consistency, to avoid potential bugs. - - // Check if the name is a valid hash, to avoid unnecessary iteration. - // A string with odd length is not a valid hash, but it's a valid prefix of a hash. - if _, err := hex.DecodeString(name); err != nil && !errors.Is(err, hex.ErrLength) { - return false - } - - iter, err := repo.gogitRepo.Objects() - if err != nil { - return false - } - defer iter.Close() - - found := false - _ = iter.ForEach(func(obj object.Object) error { - if strings.HasPrefix(obj.ID().String(), name) { - found = true - return storer.ErrStop - } - return nil - }) - return found + _, 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 } - reference, err := repo.gogitRepo.Reference(plumbing.ReferenceName(name), true) - if err != nil { - return false - } - return reference.Type() != plumbing.InvalidReference + _, err := repo.gogitRepo.ResolveRevision(plumbing.Revision(name)) + + return err == nil } // IsBranchExist returns true if given branch exists in current repository. diff --git a/modules/git/repo_branch_test.go b/modules/git/repo_branch_test.go index 6ad8abbed40ff..009c545832a49 100644 --- a/modules/git/repo_branch_test.go +++ b/modules/git/repo_branch_test.go @@ -101,6 +101,10 @@ func TestRepository_IsObjectExist(t *testing.T) { 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 @@ -124,7 +128,7 @@ func TestRepository_IsObjectExist(t *testing.T) { { name: "short commit hash", arg: "ce06481", - want: true, + want: supportShortHash, }, { name: "blob hash", @@ -134,7 +138,7 @@ func TestRepository_IsObjectExist(t *testing.T) { { name: "short blob hash", arg: "153f451", - want: true, + want: supportShortHash, }, } for _, tt := range tests { @@ -143,3 +147,55 @@ func TestRepository_IsObjectExist(t *testing.T) { }) } } + +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 a30ea5b59353c..8d3327c49eb8b 100644 --- a/modules/markup/html.go +++ b/modules/markup/html.go @@ -1144,8 +1144,8 @@ func hashCurrentPatternProcessor(ctx *RenderContext, node *html.Node) { }) } - // TODO: Avoid to use IsObjectExist since it's not efficient with gogit. - 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 }