From 6ca1656f93332c769ddfd72ee9fcb48656878d44 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sat, 15 Nov 2025 13:24:00 +0100 Subject: [PATCH] chore: two small refactors in git module (#10109) Move the function to the repository struct. There is no need to have it as a separate function, move it to the Repository struct. Add extra unit tests. --- Remove a field from a struct. It has nothing to do with git, it is not the right place to have that field in the git `Tag` struct. Get this value when it's converted to the API struct. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10109 Reviewed-by: Michael Kriese Co-authored-by: Gusted Co-committed-by: Gusted --- modules/git/commit_test.go | 1 + modules/git/repo.go | 11 ---------- modules/git/repo_commit.go | 12 +++++++++++ modules/git/repo_commit_test.go | 38 +++++++++++++++++++++++++++++++++ modules/git/repo_test.go | 10 --------- modules/git/tag.go | 17 +++++++-------- routers/api/v1/repo/tag.go | 25 +++++++++++----------- services/convert/convert.go | 25 ++++++++++++++++------ services/mirror/mirror_pull.go | 2 +- 9 files changed, 90 insertions(+), 51 deletions(-) diff --git a/modules/git/commit_test.go b/modules/git/commit_test.go index cd6b17d0d7..fdf2ba0956 100644 --- a/modules/git/commit_test.go +++ b/modules/git/commit_test.go @@ -1,4 +1,5 @@ // Copyright 2017 The Gitea Authors. All rights reserved. +// Copyright 2025 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package git diff --git a/modules/git/repo.go b/modules/git/repo.go index 4f2b05bca5..dc9cb12444 100644 --- a/modules/git/repo.go +++ b/modules/git/repo.go @@ -326,17 +326,6 @@ func Push(ctx context.Context, repoPath string, opts PushOptions) error { return nil } -// GetLatestCommitTime returns time for latest commit in repository (across all branches) -func GetLatestCommitTime(ctx context.Context, repoPath string) (time.Time, error) { - cmd := NewCommand(ctx, "for-each-ref", "--sort=-committerdate", BranchPrefix, "--count", "1", "--format=%(committerdate)") - stdout, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath}) - if err != nil { - return time.Time{}, err - } - commitTime := strings.TrimSpace(stdout) - return time.Parse("Mon Jan _2 15:04:05 2006 -0700", commitTime) -} - // DivergeObject represents commit count diverging commits type DivergeObject struct { Ahead int diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index a650e597d2..bd7d5e2014 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -11,6 +11,7 @@ import ( "io" "strconv" "strings" + "time" "forgejo.org/modules/cache" "forgejo.org/modules/log" @@ -670,3 +671,14 @@ func (repo *Repository) ConvertToGitID(commitID string) (ObjectID, error) { return MustIDFromString(string(sha)), nil } + +// GetLatestCommitTime returns time for latest commit in repository (across all branches) +func (repo *Repository) GetLatestCommitTime() (time.Time, error) { + cmd := NewCommand(repo.Ctx, "for-each-ref", "--sort=-committerdate", "--count=1", "--format=%(committerdate)", BranchPrefix) + stdout, _, err := cmd.RunStdString(&RunOpts{Dir: repo.Path}) + if err != nil { + return time.Time{}, err + } + commitTime := strings.TrimSpace(stdout) + return time.Parse("Mon Jan _2 15:04:05 2006 -0700", commitTime) +} diff --git a/modules/git/repo_commit_test.go b/modules/git/repo_commit_test.go index 5932f31e3c..99cde92300 100644 --- a/modules/git/repo_commit_test.go +++ b/modules/git/repo_commit_test.go @@ -236,3 +236,41 @@ func TestGetCommitsFromIDs(t *testing.T) { } }) } + +func TestGetLatestCommitTime(t *testing.T) { + t.Run("repo1", func(t *testing.T) { + repo, err := openRepositoryWithDefaultContext(filepath.Join(testReposDir, "repo1_bare")) + require.NoError(t, err) + defer repo.Close() + + lct, err := repo.GetLatestCommitTime() + require.NoError(t, err) + // Time is Sun Nov 13 16:40:14 2022 +0100 + // which is the time of commit + // ce064814f4a0d337b333e646ece456cd39fab612 (refs/heads/master) + assert.EqualValues(t, 1668354014, lct.Unix()) + }) + + t.Run("repo1_sha256", func(t *testing.T) { + skipIfSHA256NotSupported(t) + + repo, err := openRepositoryWithDefaultContext(filepath.Join(testReposDir, "repo1_bare_sha256")) + require.NoError(t, err) + defer repo.Close() + + lct, err := repo.GetLatestCommitTime() + require.NoError(t, err) + assert.EqualValues(t, 1698676906, lct.Unix()) + }) + + t.Run("repo3_notes", func(t *testing.T) { + repo, err := openRepositoryWithDefaultContext(filepath.Join(testReposDir, "repo3_notes")) + require.NoError(t, err) + defer repo.Close() + + lct, err := repo.GetLatestCommitTime() + require.NoError(t, err) + // Time is of refs/heads/master and not of refs/notes/commits + assert.EqualValues(t, 1567767909, lct.Unix()) + }) +} diff --git a/modules/git/repo_test.go b/modules/git/repo_test.go index b07fc3732d..1c36a4624b 100644 --- a/modules/git/repo_test.go +++ b/modules/git/repo_test.go @@ -19,16 +19,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestGetLatestCommitTime(t *testing.T) { - bareRepo1Path := filepath.Join(testReposDir, "repo1_bare") - lct, err := GetLatestCommitTime(DefaultContext, bareRepo1Path) - require.NoError(t, err) - // Time is Sun Nov 13 16:40:14 2022 +0100 - // which is the time of commit - // ce064814f4a0d337b333e646ece456cd39fab612 (refs/heads/master) - assert.EqualValues(t, 1668354014, lct.Unix()) -} - func TestRepoIsEmpty(t *testing.T) { emptyRepo2Path := filepath.Join(testReposDir, "repo2_empty") repo, err := openRepositoryWithDefaultContext(emptyRepo2Path) diff --git a/modules/git/tag.go b/modules/git/tag.go index 64f1b952ad..edbe54b853 100644 --- a/modules/git/tag.go +++ b/modules/git/tag.go @@ -1,4 +1,5 @@ // Copyright 2015 The Gogs Authors. All rights reserved. +// Copyright 2025 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package git @@ -7,7 +8,6 @@ import ( "bytes" "strings" - api "forgejo.org/modules/structs" "forgejo.org/modules/util" ) @@ -20,14 +20,13 @@ const ( // Tag represents a Git tag. type Tag struct { - Name string - ID ObjectID - Object ObjectID // The id of this commit object - Type string - Tagger *Signature - Message string - Signature *ObjectSignature - ArchiveDownloadCount *api.TagArchiveDownloadCount + Name string + ID ObjectID + Object ObjectID // The id of this commit object + Type string + Tagger *Signature + Message string + Signature *ObjectSignature } // Commit return the commit of the tag reference diff --git a/routers/api/v1/repo/tag.go b/routers/api/v1/repo/tag.go index 611fd57dbb..64104ea784 100644 --- a/routers/api/v1/repo/tag.go +++ b/routers/api/v1/repo/tag.go @@ -65,13 +65,12 @@ func ListTags(ctx *context.APIContext) { apiTags := make([]*api.Tag, len(tags)) for i := range tags { - tags[i].ArchiveDownloadCount, err = repo_model.GetArchiveDownloadCountForTagName(ctx, ctx.Repo.Repository.ID, tags[i].Name) + convertedTag, err := convert.ToTag(ctx, ctx.Repo.Repository, tags[i]) if err != nil { - ctx.Error(http.StatusInternalServerError, "GetTagArchiveDownloadCountForName", err) + ctx.Error(http.StatusInternalServerError, "ToTag", err) return } - - apiTags[i] = convert.ToTag(ctx.Repo.Repository, tags[i]) + apiTags[i] = convertedTag } ctx.SetLinkHeader(total, listOpts.PageSize) ctx.SetTotalCountHeader(int64(total)) @@ -123,13 +122,13 @@ func GetAnnotatedTag(ctx *context.APIContext) { ctx.Error(http.StatusBadRequest, "GetAnnotatedTag", err) } - tag.ArchiveDownloadCount, err = repo_model.GetArchiveDownloadCountForTagName(ctx, ctx.Repo.Repository.ID, tag.Name) + convertedAnnotatedTag, err := convert.ToAnnotatedTag(ctx, ctx.Repo.Repository, tag, commit) if err != nil { - ctx.Error(http.StatusInternalServerError, "GetTagArchiveDownloadCountForName", err) + ctx.Error(http.StatusInternalServerError, "ToAnnotatedTag", err) return } - ctx.JSON(http.StatusOK, convert.ToAnnotatedTag(ctx, ctx.Repo.Repository, tag, commit)) + ctx.JSON(http.StatusOK, convertedAnnotatedTag) } } @@ -169,13 +168,13 @@ func GetTag(ctx *context.APIContext) { return } - tag.ArchiveDownloadCount, err = repo_model.GetArchiveDownloadCountForTagName(ctx, ctx.Repo.Repository.ID, tag.Name) + convertedTag, err := convert.ToTag(ctx, ctx.Repo.Repository, tag) if err != nil { - ctx.Error(http.StatusInternalServerError, "GetTagArchiveDownloadCountForName", err) + ctx.Error(http.StatusInternalServerError, "ToTag", err) return } - ctx.JSON(http.StatusOK, convert.ToTag(ctx.Repo.Repository, tag)) + ctx.JSON(http.StatusOK, convertedTag) } // CreateTag create a new git tag in a repository @@ -248,13 +247,13 @@ func CreateTag(ctx *context.APIContext) { return } - tag.ArchiveDownloadCount, err = repo_model.GetArchiveDownloadCountForTagName(ctx, ctx.Repo.Repository.ID, tag.Name) + convertedTag, err := convert.ToTag(ctx, ctx.Repo.Repository, tag) if err != nil { - ctx.Error(http.StatusInternalServerError, "GetTagArchiveDownloadCountForName", err) + ctx.Error(http.StatusInternalServerError, "ToTag", err) return } - ctx.JSON(http.StatusCreated, convert.ToTag(ctx.Repo.Repository, tag)) + ctx.JSON(http.StatusCreated, convertedTag) } // DeleteTag delete a specific tag of in a repository by name diff --git a/services/convert/convert.go b/services/convert/convert.go index 9acbbe9b9e..eaf6459cfd 100644 --- a/services/convert/convert.go +++ b/services/convert/convert.go @@ -1,5 +1,6 @@ // Copyright 2015 The Gogs Authors. All rights reserved. -// Copyright 2018 The Gitea Authors. All rights reserved. +// Copyright 2018 The Gitea Authors. All rights +// Copyright 2025 The Forgejo Authors. All rights reserved. // SPDX-License-Identifier: MIT package convert @@ -187,7 +188,12 @@ func ToBranchProtection(ctx context.Context, bp *git_model.ProtectedBranch, repo } // ToTag convert a git.Tag to an api.Tag -func ToTag(repo *repo_model.Repository, t *git.Tag) *api.Tag { +func ToTag(ctx context.Context, repo *repo_model.Repository, t *git.Tag) (*api.Tag, error) { + archiveDownloadCount, err := repo_model.GetArchiveDownloadCountForTagName(ctx, repo.ID, t.Name) + if err != nil { + return nil, err + } + return &api.Tag{ Name: t.Name, Message: strings.TrimSpace(t.Message), @@ -195,8 +201,8 @@ func ToTag(repo *repo_model.Repository, t *git.Tag) *api.Tag { Commit: ToCommitMeta(repo, t), ZipballURL: util.URLJoin(repo.HTMLURL(), "archive", t.Name+".zip"), TarballURL: util.URLJoin(repo.HTMLURL(), "archive", t.Name+".tar.gz"), - ArchiveDownloadCount: t.ArchiveDownloadCount, - } + ArchiveDownloadCount: archiveDownloadCount, + }, nil } // ToActionTask convert a actions_model.ActionTask to an api.ActionTask @@ -391,7 +397,12 @@ func ToTeams(ctx context.Context, teams []*organization.Team, loadOrgs bool) ([] } // ToAnnotatedTag convert git.Tag to api.AnnotatedTag -func ToAnnotatedTag(ctx context.Context, repo *repo_model.Repository, t *git.Tag, c *git.Commit) *api.AnnotatedTag { +func ToAnnotatedTag(ctx context.Context, repo *repo_model.Repository, t *git.Tag, c *git.Commit) (*api.AnnotatedTag, error) { + archiveDownloadCount, err := repo_model.GetArchiveDownloadCountForTagName(ctx, repo.ID, t.Name) + if err != nil { + return nil, err + } + return &api.AnnotatedTag{ Tag: t.Name, SHA: t.ID.String(), @@ -400,8 +411,8 @@ func ToAnnotatedTag(ctx context.Context, repo *repo_model.Repository, t *git.Tag URL: util.URLJoin(repo.APIURL(), "git/tags", t.ID.String()), Tagger: ToCommitUser(t.Tagger), Verification: ToVerification(ctx, c), - ArchiveDownloadCount: t.ArchiveDownloadCount, - } + ArchiveDownloadCount: archiveDownloadCount, + }, nil } // ToAnnotatedTagObject convert a git.Commit to an api.AnnotatedTagObject diff --git a/services/mirror/mirror_pull.go b/services/mirror/mirror_pull.go index c46323f283..d6e1ff6c51 100644 --- a/services/mirror/mirror_pull.go +++ b/services/mirror/mirror_pull.go @@ -587,7 +587,7 @@ func SyncPullMirror(ctx context.Context, repoID int64) bool { } if !isEmpty { // Get latest commit date and update to current repository updated time - commitDate, err := git.GetLatestCommitTime(ctx, m.Repo.RepoPath()) + commitDate, err := gitRepo.GetLatestCommitTime() if err != nil { log.Error("SyncMirrors [repo: %-v]: unable to GetLatestCommitDate: %v", m.Repo, err) return false