Move some functions to gitrepo package#36271
Conversation
869f4fa to
34d541c
Compare
There was a problem hiding this comment.
Pull request overview
Refactors Git plumbing helpers by moving branch/ref/tag/tree query functions (and related tests) from modules/git to the higher-level modules/gitrepo package and updates call sites across services/routers/tests accordingly.
Changes:
- Introduces
gitrepoequivalents for branch listing, ref walking/lookup, tag info retrieval, tree queries, and latest-path commit lookup. - Updates services, routers, and integration/unit tests to use the new
gitrepoAPIs and simplifies some call paths by removing explicit repo opens. - Exports signature/parsing helpers in
modules/gitto support the movedgitrepofunctionality and removes now-redundantgitimplementations/tests.
Reviewed changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/pull_merge_test.go | Switches branch listing in integration test to gitrepo.GetBranchNames. |
| tests/integration/migrate_test.go | Uses gitrepo.GetBranchNames/GetTagInfos without explicitly opening a repo. |
| services/wiki/wiki_test.go | Updates wiki tests to use gitrepo operations and adjusted repo setup for wiki storage. |
| services/wiki/wiki.go | Reworks prepareGitPath to use gitrepo.LsTree and repo-relative storage handles. |
| services/repository/migrate.go | Updates releases/tag sync to new SyncReleasesWithTags signature. |
| services/repository/fork.go | Updates releases/tag sync to new SyncReleasesWithTags signature. |
| services/repository/commit.go | Uses gitrepo.ListOccurrences instead of git.Repository.ListOccurrences. |
| services/repository/adopt.go | Updates releases/tag sync to new SyncReleasesWithTags signature. |
| services/pull/pull.go | Replaces branch listing call with gitrepo.GetBranchNames. |
| services/mirror/mirror_pull.go | Replaces branch listing and updates releases/tag sync call signature. |
| services/migrations/gitea_uploader.go | Updates uploader tag sync to new SyncReleasesWithTags signature. |
| services/context/repo.go | Uses gitrepo.GetBranchNames when default branch is missing. |
| services/automerge/automerge.go | Uses gitrepo.GetRefsBySha directly (no explicit repo open). |
| routers/web/repo/view_home.go | Uses gitrepo.GetBranchNames to detect “no branch” state. |
| routers/web/repo/download.go | Uses gitrepo.GetTreePathLatestCommitID then loads commit via existing repo handle. |
| routers/api/v1/repo/tag.go | Uses gitrepo.GetTagInfos for tag listing. |
| routers/api/v1/repo/file.go | Uses gitrepo.GetTreePathLatestCommitID then loads commit via existing repo handle. |
| modules/repository/repo.go | Changes SyncReleasesWithTags to not require an opened *git.Repository. |
| modules/repository/branch.go | Uses gitrepo.GetBranchNames for branch enumeration during DB sync. |
| modules/gitrepo/walk_nogogit.go | Implements WalkShowRef/helpers in gitrepo for non-gogit builds. |
| modules/gitrepo/tree_test.go | Adds unit test for GetTreePathLatestCommitID. |
| modules/gitrepo/tree.go | Adds gitrepo.LsTree and GetTreePathLatestCommitID. |
| modules/gitrepo/tag_test.go | Adds tests for gitrepo.GetTagInfos and parseTagRef. |
| modules/gitrepo/tag.go | Adds gitrepo.GetTagInfos and tag parsing/sorting logic. |
| modules/gitrepo/ref_test.go | Adds tests/benchmark for gitrepo.GetRefsBySha. |
| modules/gitrepo/ref_nogogit.go | Adds non-gogit implementation of GetRefsBySha. |
| modules/gitrepo/ref_gogit.go | Adds gogit implementation of GetRefsBySha. |
| modules/gitrepo/ref.go | Adds gitrepo.ListOccurrences and parsing helpers. |
| modules/gitrepo/branch_test.go | Adds tests/benchmark for gitrepo.GetBranchNames. |
| modules/gitrepo/branch_nogogit.go | Adds non-gogit implementation of GetBranchNames. |
| modules/gitrepo/branch_gogit.go | Adds gogit implementation of GetBranchNames. |
| modules/gitrepo/branch.go | Removes now-redundant GetBranchesByPath wrapper. |
| modules/git/tree_test.go | Removes tests tied to moved tree-path latest commit logic. |
| modules/git/tree.go | Removes Repository.LsTree and Repository.GetTreePathLatestCommit after moving to gitrepo. |
| modules/git/tag.go | Exports payload/signature parsing helpers for use by gitrepo tag parsing. |
| modules/git/signature_test.go | Updates tests to use exported ParseSignatureFromCommitLine. |
| modules/git/signature_nogogit.go | Updates decode path to use exported ParseSignatureFromCommitLine. |
| modules/git/signature.go | Exports ParseSignatureFromCommitLine. |
| modules/git/repo_tag_test.go | Removes tests moved to modules/gitrepo. |
| modules/git/repo_tag.go | Removes Repository.GetTagInfos implementation moved to gitrepo. |
| modules/git/repo_ref.go | Removes Repository.ListOccurrences moved to gitrepo. |
| modules/git/repo_commit_nogogit.go | Removes Repository.ResolveReference (no longer needed in moved surface). |
| modules/git/repo_branch_test.go | Removes tests moved to modules/gitrepo. |
| modules/git/repo_branch_nogogit.go | Removes branch/ref walking helpers moved to gitrepo (non-gogit). |
| modules/git/repo_branch_gogit.go | Removes branch/ref walking helpers moved to gitrepo (gogit). |
| cmd/admin.go | Removes explicit repo open/close; uses new SyncReleasesWithTags signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // GetBranches returns branches from the repository, skipping "skip" initial branches and | ||
| // returning at most "limit" branches, or all branches if "limit" is 0. | ||
| // Branches are returned with sort of `-committerdate` as the nogogit | ||
| // implementation. This requires full fetch, sort and then the | ||
| // skip/limit applies later as gogit returns in undefined order. |
There was a problem hiding this comment.
The doc comment says GetBranches, but the function name is GetBranchNames. Please update the comment so godoc matches the exported API name (and to make searches easier).
| } | ||
| } | ||
| for limit == 0 || i < skip+limit { | ||
| // The output of show-ref is simply a list: |
There was a problem hiding this comment.
This comment describes the output format of show-ref, but the code is executing git for-each-ref --format=%(objectname) %(refname). Please update the comment to reflect the actual command/output format being parsed.
| // The output of show-ref is simply a list: | |
| // The output of `git for-each-ref --format=%(objectname) %(refname)` is a list of: |
| return len([]*git.Tag(ts)) | ||
| } | ||
|
|
||
| func (ts tagSorter) Less(i, j int) bool { | ||
| return []*git.Tag(ts)[i].Tagger.When.After([]*git.Tag(ts)[j].Tagger.When) | ||
| } | ||
|
|
||
| func (ts tagSorter) Swap(i, j int) { | ||
| []*git.Tag(ts)[i], []*git.Tag(ts)[j] = []*git.Tag(ts)[j], []*git.Tag(ts)[i] |
There was a problem hiding this comment.
tagSorter methods repeatedly convert ts to []*git.Tag (e.g. len([]*git.Tag(ts)), []*git.Tag(ts)[i]). This is unnecessary and hurts readability; since tagSorter is already []*git.Tag, these can be simplified to len(ts), ts[i], and ts[i], ts[j] = ts[j], ts[i].
| return len([]*git.Tag(ts)) | |
| } | |
| func (ts tagSorter) Less(i, j int) bool { | |
| return []*git.Tag(ts)[i].Tagger.When.After([]*git.Tag(ts)[j].Tagger.When) | |
| } | |
| func (ts tagSorter) Swap(i, j int) { | |
| []*git.Tag(ts)[i], []*git.Tag(ts)[j] = []*git.Tag(ts)[j], []*git.Tag(ts)[i] | |
| return len(ts) | |
| } | |
| func (ts tagSorter) Less(i, j int) bool { | |
| return ts[i].Tagger.When.After(ts[j].Tagger.When) | |
| } | |
| func (ts tagSorter) Swap(i, j int) { | |
| ts[i], ts[j] = ts[j], ts[i] |
|
I just don't understand why you keep moving code from git to gitrepo. If you'd like to introduce something like "Git API", the design should be:
Now you just keep copying code from "git" to "gitrepo" also make a lot of code/functions redundant and more difficult to refactor in the future. |
The goal of this refactor is to enable future abstraction of Git operations for managed git repositories. The gitcmd package serves as a low-level wrapper around Git commands, providing a unified execution layer for higher-level abstractions. All functions moved out of the git package are related to managed Git repositories. These features are not required by standalone local repositories, so extracting them improves modularity and reduces unnecessary coupling. These functions were simply relocated from git to gitrepo; no additional or duplicated logic was introduced. Therefore, I’m not sure what you meant by “redundant.” Could you share more details about your concerns? It would also be helpful to understand what potential difficulties you foresee in future refactoring. |
They don't need to. They only calls |
Move
GetBranchNames,WalkReferences,GetRefsBySha,ListOccurrences,GetTagInfos,LsTree,WalkShowRefandGetTreePathLatestCommitIDand related tests from packagegittogitrepo.