From ef019e796421358f557381ccfa982c693dcb9e94 Mon Sep 17 00:00:00 2001 From: oppiliappan Date: Thu, 29 Jan 2026 06:50:14 +0000 Subject: [PATCH] knotserver/git: fix pagination in git.Tags Signed-off-by: oppiliappan --- knotserver/git/tag.go | 41 +++- knotserver/git/tag_test.go | 365 +++++++++++++++++++++++++++++++++++ knotserver/xrpc/repo_tags.go | 37 ++-- 3 files changed, 418 insertions(+), 25 deletions(-) create mode 100644 knotserver/git/tag_test.go diff --git a/knotserver/git/tag.go b/knotserver/git/tag.go index bee1a57c..3eeaedf5 100644 --- a/knotserver/git/tag.go +++ b/knotserver/git/tag.go @@ -10,7 +10,21 @@ import ( "github.com/go-git/go-git/v5/plumbing/object" ) -func (g *GitRepo) Tags() ([]object.Tag, error) { +type TagsOptions struct { + Limit int + Offset int + Pattern string +} + +func (g *GitRepo) Tags(opts *TagsOptions) ([]object.Tag, error) { + if opts == nil { + opts = &TagsOptions{} + } + + if opts.Pattern == "" { + opts.Pattern = "refs/tags" + } + fields := []string{ "refname:short", "objectname", @@ -29,12 +43,22 @@ func (g *GitRepo) Tags() ([]object.Tag, error) { if i != 0 { outFormat.WriteString(fieldSeparator) } - outFormat.WriteString(fmt.Sprintf("%%(%s)", f)) + fmt.Fprintf(&outFormat, "%%(%s)", f) } outFormat.WriteString("") outFormat.WriteString(recordSeparator) - output, err := g.forEachRef(outFormat.String(), "--sort=-creatordate", "refs/tags") + args := []string{outFormat.String(), "--sort=-creatordate"} + + // only add the count if the limit is a non-zero value, + // if it is zero, get as many tags as we can + if opts.Limit > 0 { + args = append(args, fmt.Sprintf("--count=%d", opts.Offset+opts.Limit)) + } + + args = append(args, opts.Pattern) + + output, err := g.forEachRef(args...) if err != nil { return nil, fmt.Errorf("failed to get tags: %w", err) } @@ -44,6 +68,17 @@ func (g *GitRepo) Tags() ([]object.Tag, error) { return nil, nil } + startIdx := opts.Offset + if startIdx >= len(records) { + return nil, nil + } + + endIdx := len(records) + if opts.Limit > 0 { + endIdx = min(startIdx+opts.Limit, len(records)) + } + + records = records[startIdx:endIdx] tags := make([]object.Tag, 0, len(records)) for _, line := range records { diff --git a/knotserver/git/tag_test.go b/knotserver/git/tag_test.go new file mode 100644 index 00000000..bd206305 --- /dev/null +++ b/knotserver/git/tag_test.go @@ -0,0 +1,365 @@ +package git + +import ( + "path/filepath" + "testing" + "time" + + gogit "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/object" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" +) + +type TagSuite struct { + suite.Suite + *RepoSuite +} + +func TestTagSuite(t *testing.T) { + t.Parallel() + suite.Run(t, new(TagSuite)) +} + +func (s *TagSuite) SetupTest() { + s.RepoSuite = NewRepoSuite(s.T()) +} + +func (s *TagSuite) TearDownTest() { + s.RepoSuite.cleanup() +} + +func (s *TagSuite) setupRepoWithTags() { + s.init() + + // create commits for tagging + commit1 := s.commitFile("file1.txt", "content 1", "Add file1") + commit2 := s.commitFile("file2.txt", "content 2", "Add file2") + commit3 := s.commitFile("file3.txt", "content 3", "Add file3") + commit4 := s.commitFile("file4.txt", "content 4", "Add file4") + commit5 := s.commitFile("file5.txt", "content 5", "Add file5") + + // create annotated tags + s.createAnnotatedTag( + "v1.0.0", + commit1, + "Tagger One", + "tagger1@example.com", + "Release version 1.0.0\n\nThis is the first stable release.", + s.baseTime.Add(1*time.Hour), + ) + + s.createAnnotatedTag( + "v1.1.0", + commit2, + "Tagger Two", + "tagger2@example.com", + "Release version 1.1.0", + s.baseTime.Add(2*time.Hour), + ) + + // create lightweight tags + s.createLightweightTag("v2.0.0", commit3) + s.createLightweightTag("v2.1.0", commit4) + + // create another annotated tag + s.createAnnotatedTag( + "v3.0.0", + commit5, + "Tagger Three", + "tagger3@example.com", + "Major version 3.0.0\n\nBreaking changes included.", + s.baseTime.Add(3*time.Hour), + ) +} + +func (s *TagSuite) TestTags_All() { + s.setupRepoWithTags() + + tags, err := s.repo.Tags(nil) + require.NoError(s.T(), err) + + // we created 5 tags total (3 annotated, 2 lightweight) + assert.Len(s.T(), tags, 5, "expected 5 tags") + + // verify tags are sorted by creation date (newest first) + expectedAnnotated := map[string]bool{ + "v1.0.0": true, + "v1.1.0": true, + "v3.0.0": true, + } + + expectedLightweight := map[string]bool{ + "v2.0.0": true, + "v2.1.0": true, + } + + for _, tag := range tags { + if expectedAnnotated[tag.Name] { + // annotated tags should have tagger info + assert.NotEmpty(s.T(), tag.Tagger.Name, "annotated tag %s should have tagger name", tag.Name) + assert.NotEmpty(s.T(), tag.Message, "annotated tag %s should have message", tag.Name) + } else if expectedLightweight[tag.Name] { + // lightweight tags won't have tagger info or message (they'll have empty values) + } else { + s.T().Errorf("unexpected tag name: %s", tag.Name) + } + } +} + +func (s *TagSuite) TestTags_WithLimit() { + s.setupRepoWithTags() + + tests := []struct { + name string + limit int + expectedCount int + }{ + { + name: "limit 1", + limit: 1, + expectedCount: 1, + }, + { + name: "limit 2", + limit: 2, + expectedCount: 2, + }, + { + name: "limit 3", + limit: 3, + expectedCount: 3, + }, + { + name: "limit 10 (more than available)", + limit: 10, + expectedCount: 5, + }, + } + + for _, tt := range tests { + s.Run(tt.name, func() { + tags, err := s.repo.Tags(&TagsOptions{ + Limit: tt.limit, + }) + require.NoError(s.T(), err) + assert.Len(s.T(), tags, tt.expectedCount, "expected %d tags", tt.expectedCount) + }) + } +} + +func (s *TagSuite) TestTags_WithOffset() { + s.setupRepoWithTags() + + tests := []struct { + name string + offset int + expectedCount int + }{ + { + name: "offset 0", + offset: 0, + expectedCount: 5, + }, + { + name: "offset 1", + offset: 1, + expectedCount: 4, + }, + { + name: "offset 2", + offset: 2, + expectedCount: 3, + }, + { + name: "offset 4", + offset: 4, + expectedCount: 1, + }, + { + name: "offset 5 (all skipped)", + offset: 5, + expectedCount: 0, + }, + { + name: "offset 10 (more than available)", + offset: 10, + expectedCount: 0, + }, + } + + for _, tt := range tests { + s.Run(tt.name, func() { + tags, err := s.repo.Tags(&TagsOptions{ + Offset: tt.offset, + }) + require.NoError(s.T(), err) + assert.Len(s.T(), tags, tt.expectedCount, "expected %d tags", tt.expectedCount) + }) + } +} + +func (s *TagSuite) TestTags_WithLimitAndOffset() { + s.setupRepoWithTags() + + tests := []struct { + name string + limit int + offset int + expectedCount int + }{ + { + name: "limit 2, offset 0", + limit: 2, + offset: 0, + expectedCount: 2, + }, + { + name: "limit 2, offset 1", + limit: 2, + offset: 1, + expectedCount: 2, + }, + { + name: "limit 2, offset 3", + limit: 2, + offset: 3, + expectedCount: 2, + }, + { + name: "limit 2, offset 4", + limit: 2, + offset: 4, + expectedCount: 1, + }, + { + name: "limit 3, offset 2", + limit: 3, + offset: 2, + expectedCount: 3, + }, + { + name: "limit 10, offset 3", + limit: 10, + offset: 3, + expectedCount: 2, + }, + } + + for _, tt := range tests { + s.Run(tt.name, func() { + tags, err := s.repo.Tags(&TagsOptions{ + Limit: tt.limit, + Offset: tt.offset, + }) + require.NoError(s.T(), err) + assert.Len(s.T(), tags, tt.expectedCount, "expected %d tags", tt.expectedCount) + }) + } +} + +func (s *TagSuite) TestTags_EmptyRepo() { + repoPath := filepath.Join(s.tempDir, "empty-repo") + + _, err := gogit.PlainInit(repoPath, false) + require.NoError(s.T(), err) + + gitRepo, err := PlainOpen(repoPath) + require.NoError(s.T(), err) + + tags, err := gitRepo.Tags(nil) + require.NoError(s.T(), err) + + if tags != nil { + assert.Empty(s.T(), tags, "expected no tags in empty repo") + } +} + +func (s *TagSuite) TestTags_Pagination() { + s.setupRepoWithTags() + + allTags, err := s.repo.Tags(nil) + require.NoError(s.T(), err) + assert.Len(s.T(), allTags, 5, "expected 5 tags") + + pageSize := 2 + var paginatedTags []object.Tag + + for offset := 0; offset < len(allTags); offset += pageSize { + tags, err := s.repo.Tags(&TagsOptions{ + Limit: pageSize, + Offset: offset, + }) + require.NoError(s.T(), err) + paginatedTags = append(paginatedTags, tags...) + } + + assert.Len(s.T(), paginatedTags, len(allTags), "pagination should return all tags") + + for i := range allTags { + assert.Equal(s.T(), allTags[i].Name, paginatedTags[i].Name, + "tag at index %d differs", i) + } +} + +func (s *TagSuite) TestTags_VerifyAnnotatedTagFields() { + s.setupRepoWithTags() + + tags, err := s.repo.Tags(nil) + require.NoError(s.T(), err) + + var v1Tag *object.Tag + for i := range tags { + if tags[i].Name == "v1.0.0" { + v1Tag = &tags[i] + break + } + } + + require.NotNil(s.T(), v1Tag, "v1.0.0 tag not found") + + assert.Equal(s.T(), "Tagger One", v1Tag.Tagger.Name, "tagger name should match") + assert.Equal(s.T(), "tagger1@example.com", v1Tag.Tagger.Email, "tagger email should match") + + assert.Equal(s.T(), "Release version 1.0.0\n\nThis is the first stable release.", + v1Tag.Message, "tag message should match") + + assert.Equal(s.T(), plumbing.TagObject, v1Tag.TargetType, + "target type should be CommitObject") + + assert.False(s.T(), v1Tag.Hash.IsZero(), "tag hash should be set") + + assert.False(s.T(), v1Tag.Target.IsZero(), "target hash should be set") +} + +func (s *TagSuite) TestTags_NilOptions() { + s.setupRepoWithTags() + + tags, err := s.repo.Tags(nil) + require.NoError(s.T(), err) + assert.Len(s.T(), tags, 5, "nil options should return all tags") +} + +func (s *TagSuite) TestTags_ZeroLimitAndOffset() { + s.setupRepoWithTags() + + tags, err := s.repo.Tags(&TagsOptions{ + Limit: 0, + Offset: 0, + }) + require.NoError(s.T(), err) + assert.Len(s.T(), tags, 5, "zero limit should return all tags") +} + +func (s *TagSuite) TestTags_Pattern() { + s.setupRepoWithTags() + + v1tag, err := s.repo.Tags(&TagsOptions{ + Pattern: "refs/tags/v1.0.0", + }) + + require.NoError(s.T(), err) + assert.Len(s.T(), v1tag, 1, "expected 1 tag") +} diff --git a/knotserver/xrpc/repo_tags.go b/knotserver/xrpc/repo_tags.go index af7c41c0..96c6a621 100644 --- a/knotserver/xrpc/repo_tags.go +++ b/knotserver/xrpc/repo_tags.go @@ -20,13 +20,16 @@ func (x *Xrpc) RepoTags(w http.ResponseWriter, r *http.Request) { return } - cursor := r.URL.Query().Get("cursor") + // default + limit := 50 + offset := 0 - limit := 50 // default - if limitStr := r.URL.Query().Get("limit"); limitStr != "" { - if l, err := strconv.Atoi(limitStr); err == nil && l > 0 && l <= 100 { - limit = l - } + if l, err := strconv.Atoi(r.URL.Query().Get("limit")); err == nil && l > 0 && l <= 100 { + limit = l + } + + if o, err := strconv.Atoi(r.URL.Query().Get("cursor")); err == nil && o > 0 { + offset = o } gr, err := git.PlainOpen(repoPath) @@ -36,7 +39,11 @@ func (x *Xrpc) RepoTags(w http.ResponseWriter, r *http.Request) { return } - tags, err := gr.Tags() + tags, err := gr.Tags(&git.TagsOptions{ + Limit: limit, + Offset: offset, + }) + if err != nil { x.Logger.Warn("getting tags", "error", err.Error()) tags = []object.Tag{} @@ -64,22 +71,8 @@ func (x *Xrpc) RepoTags(w http.ResponseWriter, r *http.Request) { rtags = append(rtags, &tr) } - // apply pagination manually - offset := 0 - if cursor != "" { - if o, err := strconv.Atoi(cursor); err == nil && o >= 0 && o < len(rtags) { - offset = o - } - } - - // calculate end index - end := min(offset+limit, len(rtags)) - - paginatedTags := rtags[offset:end] - - // Create response using existing types.RepoTagsResponse response := types.RepoTagsResponse{ - Tags: paginatedTags, + Tags: rtags, } writeJson(w, response) -- 2.51.2