Move some functions to gitrepo package#36287
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors git-related helpers by moving several commit/compare/diff/grep/stats utilities into the modules/gitrepo package and updating call sites to use the new APIs.
Changes:
- Migrate code-activity stats, diff/patch, commit ancestry/force-push checks, and file-change comparisons to
modules/gitrepo. - Update services/modules to use
gitrepo.*helpers (and adjust Actions workflow detection interfaces accordingly). - Refactor
git.CommitTreefrom agit.Repositorymethod into a package-level function that takes(ctx, repoPath, ...).
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| services/wiki/wiki.go | Switch wiki commits to git.CommitTree(ctx, basePath, ...) and use request ctx for push. |
| services/repository/push.go | Use gitrepo.IsCommitForcePush and thread ctx through. |
| services/repository/branch.go | Use gitrepo.IsCommitForcePush / gitrepo.HasPreviousCommit for branch logic. |
| services/pull/pull.go | Replace commit ancestry check with gitrepo.HasPreviousCommit. |
| services/pull/patch.go | Use gitrepo.GetDiff/GetDiffBinary/GetPatch instead of git.Repository methods. |
| services/pull/merge.go | Replace IsCommitInBranch with gitrepo.IsCommitInBranch. |
| services/issue/pull.go | Use gitrepo.GetFilesChangedBetween for CODEOWNERS checks. |
| services/gitdiff/gitdiff.go | Remove gitRepo parameter and use gitrepo.GetFilesChangedBetween via pull.BaseRepo. |
| services/git/compare.go | Use gitrepo.GetDiffNumChangedFiles for compare stats. |
| services/convert/convert.go | Use gitrepo.GetCommitBranchName instead of Commit.GetBranchName. |
| services/actions/notifier_helper.go | Update DetectWorkflows call signature to include (ctx, repo, ...). |
| routers/web/repo/search.go | Update git-grep search call signature to pass Repository object. |
| routers/web/repo/pull.go | Update call to gitdiff.SyncUserSpecificDiff after signature change. |
| modules/indexer/code/gitgrep/gitgrep.go | Move grep execution to gitrepo.GrepSearch and pass repo object (not repoID). |
| modules/gitrepo/stats_test.go | Port stats test to gitrepo package with mock repo. |
| modules/gitrepo/stats.go | Move code-activity stats implementation into gitrepo. |
| modules/gitrepo/patch_test.go | Add gitrepo.GetPatch test coverage. |
| modules/gitrepo/patch.go | Add gitrepo.GetDiff/GetDiffBinary/GetPatch helpers. |
| modules/gitrepo/grep_test.go | Port grep tests to gitrepo package with mock repo. |
| modules/gitrepo/grep.go | Move grep implementation into gitrepo and run via RunCmdWithStderr. |
| modules/gitrepo/compare_test.go | Add tests for GetFilesChangedBetween in gitrepo. |
| modules/gitrepo/compare.go | Add GetDiffNumChangedFiles and GetFilesChangedBetween to gitrepo. |
| modules/gitrepo/commit_test.go | Add tests for HasPreviousCommit (sha1/sha256) and IsCommitInBranch. |
| modules/gitrepo/commit.go | Add commit ancestry, force-push, branch-name, and branch-contains helpers to gitrepo. |
| modules/git/repo_tree.go | Refactor CommitTree to a package-level function taking (ctx, repoPath, ...). |
| modules/git/repo_compare_test.go | Remove tests moved to gitrepo. |
| modules/git/repo_compare.go | Remove diff/patch/files-changed helpers moved to gitrepo (keep remaining methods). |
| modules/git/repo_commit_test.go | Remove IsCommitInBranch test moved to gitrepo. |
| modules/git/repo_commit.go | Remove FilesCountBetween and IsCommitInBranch methods moved to gitrepo. |
| modules/git/commit_test.go | Remove HasPreviousCommit test moved to gitrepo. |
| modules/git/commit_sha256_test.go | Remove sha256 HasPreviousCommit test moved to gitrepo. |
| modules/git/commit.go | Remove HasPreviousCommit, IsForcePush, GetFilesChangedSinceCommit, and GetBranchName methods moved to gitrepo. |
| modules/actions/workflows_test.go | Update detectMatched test calls for new signature. |
| modules/actions/workflows.go | Thread (ctx, repo) into workflow detection and use gitrepo.GetFilesChangedBetween for path filters. |
| models/activities/repo_activity.go | Use gitrepo.CodeActivityStats and gitrepo.GetCodeActivityStats. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // But as that does not work for all potential errors, we simply mark all files as unchanged and drop the error which always works, even if not as good as possible | ||
| if errIgnored != nil { | ||
| log.Error("Could not get changed files between %s and %s for pull request %d in repo with path %s. Assuming no changes. Error: %w", review.CommitSHA, latestCommit, pull.Index, gitRepo.Path, err) | ||
| log.Error("Could not get changed files between %s and %s for pull request %d in repo with path %s. Assuming no changes. Error: %w", review.CommitSHA, latestCommit, pull.Index, pull.BaseRepo.RelativePath(), err) |
There was a problem hiding this comment.
The log statement in the errIgnored != nil branch logs err (from earlier in the function) instead of the git diff error (errIgnored). This will typically log <nil> or the wrong error and makes diagnosing issues hard. Use errIgnored for the final formatted error argument (and consider keeping the variable name consistent with what’s logged).
| log.Error("Could not get changed files between %s and %s for pull request %d in repo with path %s. Assuming no changes. Error: %w", review.CommitSHA, latestCommit, pull.Index, pull.BaseRepo.RelativePath(), err) | |
| log.Error("Could not get changed files between %s and %s for pull request %d in repo with path %s. Assuming no changes. Error: %w", review.CommitSHA, latestCommit, pull.Index, pull.BaseRepo.RelativePath(), errIgnored) |
| type lineCountWriter struct { | ||
| numLines int | ||
| } | ||
|
|
||
| // Write counts the number of newlines in the provided bytestream | ||
| func (l *lineCountWriter) Write(p []byte) (n int, err error) { | ||
| n = len(p) | ||
| l.numLines += bytes.Count(p, []byte{'\000'}) | ||
| return n, err |
There was a problem hiding this comment.
lineCountWriter.Write counts NUL (\0) separators (from git diff -z) rather than newlines. The current comment (and numLines field name) is misleading and makes it harder to understand why this correctly counts changed files. Update the comment to refer to NUL-separated records and consider renaming numLines to something like numFiles/numRecords.
|
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. |
ActivityStatsrelated functions togitrepopackageGetFilesChangedBetween,HasPreviousCommit,IsForcePush,GetBranchName,FilesCountBetween,IsCommitInBranch,GetDiff,GetDiffBinary,GetPatchfunctions togitrepopackagerepo_archive.goCommitTreegitrepopackage