From bf1970d0bd5137a2e8102e117d56b27a13fb1627 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 3 Feb 2020 04:27:34 +0800 Subject: [PATCH] Improve push update options (#10105) * Improve push update options * fix test * More refactor and fix lint * fix lint * Fix lint Co-authored-by: Lauris BH --- .golangci.yml | 3 + modules/repofiles/action.go | 23 +-- modules/repofiles/action_test.go | 38 +++-- modules/repofiles/update.go | 271 ++++++++++++++++++------------- routers/private/hook.go | 11 +- 5 files changed, 193 insertions(+), 153 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 121ef583a..0bd71b7fb 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -95,3 +95,6 @@ issues: - linters: - misspell text: '`Unknwon` is a misspelling of `Unknown`' + - path: models/update.go + linters: + - unused diff --git a/modules/repofiles/action.go b/modules/repofiles/action.go index a1c2bd993..44ca285ef 100644 --- a/modules/repofiles/action.go +++ b/modules/repofiles/action.go @@ -8,7 +8,6 @@ import ( "encoding/json" "fmt" "html" - "strings" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/git" @@ -151,12 +150,9 @@ func UpdateIssuesCommit(doer *models.User, repo *models.Repository, commits []*r // CommitRepoActionOptions represent options of a new commit action. type CommitRepoActionOptions struct { - PusherName string + PushUpdateOptions + RepoOwnerID int64 - RepoName string - RefFullName string - OldCommitID string - NewCommitID string Commits *repository.PushCommits } @@ -192,7 +188,7 @@ func CommitRepoAction(optsList ...*CommitRepoActionOptions) error { refName := git.RefEndName(opts.RefFullName) // Change default branch and empty status only if pushed ref is non-empty branch. - if repo.IsEmpty && opts.NewCommitID != git.EmptySHA && strings.HasPrefix(opts.RefFullName, git.BranchPrefix) { + if repo.IsEmpty && opts.IsBranch() && !opts.IsDelRef() { repo.DefaultBranch = refName repo.IsEmpty = false if refName != "master" { @@ -210,24 +206,21 @@ func CommitRepoAction(optsList ...*CommitRepoActionOptions) error { } } - isNewBranch := false opType := models.ActionCommitRepo // Check it's tag push or branch. - if strings.HasPrefix(opts.RefFullName, git.TagPrefix) { + if opts.IsTag() { opType = models.ActionPushTag - if opts.NewCommitID == git.EmptySHA { + if opts.IsDelRef() { opType = models.ActionDeleteTag } opts.Commits = &repository.PushCommits{} - } else if opts.NewCommitID == git.EmptySHA { + } else if opts.IsDelRef() { opType = models.ActionDeleteBranch opts.Commits = &repository.PushCommits{} } else { // if not the first commit, set the compare URL. - if opts.OldCommitID == git.EmptySHA { - isNewBranch = true - } else { + if !opts.IsNewRef() { opts.Commits.CompareURL = repo.ComposeCompareURL(opts.OldCommitID, opts.NewCommitID) } @@ -259,7 +252,7 @@ func CommitRepoAction(optsList ...*CommitRepoActionOptions) error { var isHookEventPush = true switch opType { case models.ActionCommitRepo: // Push - if isNewBranch { + if opts.IsNewBranch() { notification.NotifyCreateRef(pusher, repo, "branch", opts.RefFullName) } case models.ActionDeleteBranch: // Delete Branch diff --git a/modules/repofiles/action_test.go b/modules/repofiles/action_test.go index 85bf39c83..2d659cde2 100644 --- a/modules/repofiles/action_test.go +++ b/modules/repofiles/action_test.go @@ -32,9 +32,11 @@ func TestCommitRepoAction(t *testing.T) { userID: 2, repositoryID: 16, commitRepoActionOptions: CommitRepoActionOptions{ - RefFullName: "refName", - OldCommitID: "oldCommitID", - NewCommitID: "newCommitID", + PushUpdateOptions: PushUpdateOptions{ + RefFullName: "refName", + OldCommitID: "oldCommitID", + NewCommitID: "newCommitID", + }, Commits: &repository.PushCommits{ Commits: []*repository.PushCommit{ { @@ -66,10 +68,12 @@ func TestCommitRepoAction(t *testing.T) { userID: 2, repositoryID: 1, commitRepoActionOptions: CommitRepoActionOptions{ - RefFullName: git.TagPrefix + "v1.1", - OldCommitID: git.EmptySHA, - NewCommitID: "newCommitID", - Commits: &repository.PushCommits{}, + PushUpdateOptions: PushUpdateOptions{ + RefFullName: git.TagPrefix + "v1.1", + OldCommitID: git.EmptySHA, + NewCommitID: "newCommitID", + }, + Commits: &repository.PushCommits{}, }, action: models.Action{ OpType: models.ActionPushTag, @@ -80,10 +84,12 @@ func TestCommitRepoAction(t *testing.T) { userID: 2, repositoryID: 1, commitRepoActionOptions: CommitRepoActionOptions{ - RefFullName: git.TagPrefix + "v1.1", - OldCommitID: "oldCommitID", - NewCommitID: git.EmptySHA, - Commits: &repository.PushCommits{}, + PushUpdateOptions: PushUpdateOptions{ + RefFullName: git.TagPrefix + "v1.1", + OldCommitID: "oldCommitID", + NewCommitID: git.EmptySHA, + }, + Commits: &repository.PushCommits{}, }, action: models.Action{ OpType: models.ActionDeleteTag, @@ -94,10 +100,12 @@ func TestCommitRepoAction(t *testing.T) { userID: 2, repositoryID: 1, commitRepoActionOptions: CommitRepoActionOptions{ - RefFullName: git.BranchPrefix + "feature/1", - OldCommitID: "oldCommitID", - NewCommitID: git.EmptySHA, - Commits: &repository.PushCommits{}, + PushUpdateOptions: PushUpdateOptions{ + RefFullName: git.BranchPrefix + "feature/1", + OldCommitID: "oldCommitID", + NewCommitID: git.EmptySHA, + }, + Commits: &repository.PushCommits{}, }, action: models.Action{ OpType: models.ActionDeleteBranch, diff --git a/modules/repofiles/update.go b/modules/repofiles/update.go index 5188529d1..c0f669d68 100644 --- a/modules/repofiles/update.go +++ b/modules/repofiles/update.go @@ -452,15 +452,77 @@ type PushUpdateOptions struct { RefFullName string OldCommitID string NewCommitID string - Branch string +} + +// IsNewRef return true if it's a first-time push to a branch, tag or etc. +func (opts PushUpdateOptions) IsNewRef() bool { + return opts.OldCommitID == git.EmptySHA +} + +// IsDelRef return true if it's a deletion to a branch or tag +func (opts PushUpdateOptions) IsDelRef() bool { + return opts.NewCommitID == git.EmptySHA +} + +// IsUpdateRef return true if it's an update operation +func (opts PushUpdateOptions) IsUpdateRef() bool { + return !opts.IsNewRef() && !opts.IsDelRef() +} + +// IsTag return true if it's an operation to a tag +func (opts PushUpdateOptions) IsTag() bool { + return strings.HasPrefix(opts.RefFullName, git.TagPrefix) +} + +// IsNewTag return true if it's a creation to a tag +func (opts PushUpdateOptions) IsNewTag() bool { + return opts.IsTag() && opts.IsNewRef() +} + +// IsDelTag return true if it's a deletion to a tag +func (opts PushUpdateOptions) IsDelTag() bool { + return opts.IsTag() && opts.IsDelRef() +} + +// IsBranch return true if it's a push to branch +func (opts PushUpdateOptions) IsBranch() bool { + return strings.HasPrefix(opts.RefFullName, git.BranchPrefix) +} + +// IsNewBranch return true if it's the first-time push to a branch +func (opts PushUpdateOptions) IsNewBranch() bool { + return opts.IsBranch() && opts.IsNewRef() +} + +// IsUpdateBranch return true if it's not the first push to a branch +func (opts PushUpdateOptions) IsUpdateBranch() bool { + return opts.IsBranch() && opts.IsUpdateRef() +} + +// IsDelBranch return true if it's a deletion to a branch +func (opts PushUpdateOptions) IsDelBranch() bool { + return opts.IsBranch() && opts.IsDelRef() +} + +// TagName returns simple tag name if it's an operation to a tag +func (opts PushUpdateOptions) TagName() string { + return opts.RefFullName[len(git.TagPrefix):] +} + +// BranchName returns simple branch name if it's an operation to branch +func (opts PushUpdateOptions) BranchName() string { + return opts.RefFullName[len(git.BranchPrefix):] +} + +// RepoFullName returns repo full name +func (opts PushUpdateOptions) RepoFullName() string { + return opts.RepoUserName + "/" + opts.RepoName } // PushUpdate must be called for any push actions in order to // generates necessary push action history feeds and other operations func PushUpdate(repo *models.Repository, branch string, opts PushUpdateOptions) error { - isNewRef := opts.OldCommitID == git.EmptySHA - isDelRef := opts.NewCommitID == git.EmptySHA - if isNewRef && isDelRef { + if opts.IsNewRef() && opts.IsDelRef() { return fmt.Errorf("Old and new revisions are both %s", git.EmptySHA) } @@ -481,35 +543,75 @@ func PushUpdate(repo *models.Repository, branch string, opts PushUpdateOptions) log.Error("Failed to update size for repository: %v", err) } - commitRepoActionOptions, err := createCommitRepoActionOption(repo, gitRepo, &opts) - if err != nil { - return err - } + var commits = &repo_module.PushCommits{} - if err := CommitRepoAction(commitRepoActionOptions); err != nil { - return fmt.Errorf("CommitRepoAction: %v", err) - } - - pusher, err := models.GetUserByID(opts.PusherID) - if err != nil { - return err - } - - if !isDelRef { - if err = models.RemoveDeletedBranch(repo.ID, opts.Branch); err != nil { - log.Error("models.RemoveDeletedBranch %s/%s failed: %v", repo.ID, opts.Branch, err) + if opts.IsTag() { // If is tag reference + tagName := opts.TagName() + if opts.IsDelRef() { + if err := models.PushUpdateDeleteTag(repo, tagName); err != nil { + return fmt.Errorf("PushUpdateDeleteTag: %v", err) + } + } else { + // Clear cache for tag commit count + cache.Remove(repo.GetCommitsCountCacheKey(tagName, true)) + if err := repo_module.PushUpdateAddTag(repo, gitRepo, tagName); err != nil { + return fmt.Errorf("PushUpdateAddTag: %v", err) + } + } + } else if opts.IsBranch() { // If is branch reference + pusher, err := models.GetUserByID(opts.PusherID) + if err != nil { + return err } - log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name) + if !opts.IsDelRef() { + // Clear cache for branch commit count + cache.Remove(repo.GetCommitsCountCacheKey(opts.BranchName(), true)) - go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, opts.OldCommitID, opts.NewCommitID) - // close all related pulls - } else if err = pull_service.CloseBranchPulls(pusher, repo.ID, branch); err != nil { - log.Error("close related pull request failed: %v", err) + newCommit, err := gitRepo.GetCommit(opts.NewCommitID) + if err != nil { + return fmt.Errorf("gitRepo.GetCommit: %v", err) + } + + // Push new branch. + var l *list.List + if opts.IsNewRef() { + l, err = newCommit.CommitsBeforeLimit(10) + if err != nil { + return fmt.Errorf("newCommit.CommitsBeforeLimit: %v", err) + } + } else { + l, err = newCommit.CommitsBeforeUntil(opts.OldCommitID) + if err != nil { + return fmt.Errorf("newCommit.CommitsBeforeUntil: %v", err) + } + } + + commits = repo_module.ListToPushCommits(l) + + if err = models.RemoveDeletedBranch(repo.ID, opts.BranchName()); err != nil { + log.Error("models.RemoveDeletedBranch %s/%s failed: %v", repo.ID, opts.BranchName(), err) + } + + if err = models.WatchIfAuto(opts.PusherID, repo.ID, true); err != nil { + log.Warn("Fail to perform auto watch on user %v for repo %v: %v", opts.PusherID, repo.ID, err) + } + + log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name) + + go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, opts.OldCommitID, opts.NewCommitID) + } else if err = pull_service.CloseBranchPulls(pusher, repo.ID, branch); err != nil { + // close all related pulls + log.Error("close related pull request failed: %v", err) + } } - if err = models.WatchIfAuto(opts.PusherID, repo.ID, true); err != nil { - log.Warn("Fail to perform auto watch on user %v for repo %v: %v", opts.PusherID, repo.ID, err) + if err := CommitRepoAction(&CommitRepoActionOptions{ + PushUpdateOptions: opts, + RepoOwnerID: repo.OwnerID, + Commits: commits, + }); err != nil { + return fmt.Errorf("CommitRepoAction: %v", err) } return nil @@ -526,6 +628,8 @@ func PushUpdates(repo *models.Repository, optsList []*PushUpdateOptions) error { if err != nil { return fmt.Errorf("OpenRepository: %v", err) } + defer gitRepo.Close() + if err = repo.UpdateSize(models.DefaultDBContext()); err != nil { log.Error("Failed to update size for repository: %v", err) } @@ -541,6 +645,12 @@ func PushUpdates(repo *models.Repository, optsList []*PushUpdateOptions) error { var pusher *models.User for _, opts := range optsList { + if !opts.IsBranch() { + continue + } + + branch := opts.BranchName() + if pusher == nil || pusher.ID != opts.PusherID { var err error pusher, err = models.GetUserByID(opts.PusherID) @@ -549,22 +659,22 @@ func PushUpdates(repo *models.Repository, optsList []*PushUpdateOptions) error { } } - if opts.NewCommitID != git.EmptySHA { - if err = models.RemoveDeletedBranch(repo.ID, opts.Branch); err != nil { - log.Error("models.RemoveDeletedBranch %s/%s failed: %v", repo.ID, opts.Branch, err) + if !opts.IsDelRef() { + if err = models.RemoveDeletedBranch(repo.ID, branch); err != nil { + log.Error("models.RemoveDeletedBranch %s/%s failed: %v", repo.ID, branch, err) } - log.Trace("TriggerTask '%s/%s' by %s", repo.Name, opts.Branch, pusher.Name) + if err = models.WatchIfAuto(opts.PusherID, repo.ID, true); err != nil { + log.Warn("Fail to perform auto watch on user %v for repo %v: %v", opts.PusherID, repo.ID, err) + } - go pull_service.AddTestPullRequestTask(pusher, repo.ID, opts.Branch, true, opts.OldCommitID, opts.NewCommitID) + log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name) + + go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, opts.OldCommitID, opts.NewCommitID) // close all related pulls - } else if err = pull_service.CloseBranchPulls(pusher, repo.ID, opts.Branch); err != nil { + } else if err = pull_service.CloseBranchPulls(pusher, repo.ID, branch); err != nil { log.Error("close related pull request failed: %v", err) } - - if err = models.WatchIfAuto(opts.PusherID, repo.ID, true); err != nil { - log.Warn("Fail to perform auto watch on user %v for repo %v: %v", opts.PusherID, repo.ID, err) - } } return nil @@ -576,26 +686,24 @@ func createCommitRepoActions(repo *models.Repository, gitRepo *git.Repository, o actions := make([]*CommitRepoActionOptions, 0, len(optsList)) for _, opts := range optsList { - isNewRef := opts.OldCommitID == git.EmptySHA - isDelRef := opts.NewCommitID == git.EmptySHA - if isNewRef && isDelRef { + if opts.IsNewRef() && opts.IsDelRef() { return nil, fmt.Errorf("Old and new revisions are both %s", git.EmptySHA) } var commits = &repo_module.PushCommits{} - if strings.HasPrefix(opts.RefFullName, git.TagPrefix) { + if opts.IsNewTag() { // If is tag reference - tagName := opts.RefFullName[len(git.TagPrefix):] - if isDelRef { + tagName := opts.TagName() + if opts.IsDelRef() { delTags = append(delTags, tagName) } else { cache.Remove(repo.GetCommitsCountCacheKey(tagName, true)) addTags = append(addTags, tagName) } - } else if !isDelRef { + } else if !opts.IsDelRef() { // If is branch reference // Clear cache for branch commit count - cache.Remove(repo.GetCommitsCountCacheKey(opts.RefFullName[len(git.BranchPrefix):], true)) + cache.Remove(repo.GetCommitsCountCacheKey(opts.BranchName(), true)) newCommit, err := gitRepo.GetCommit(opts.NewCommitID) if err != nil { @@ -604,7 +712,7 @@ func createCommitRepoActions(repo *models.Repository, gitRepo *git.Repository, o // Push new branch. var l *list.List - if isNewRef { + if opts.IsNewRef() { l, err = newCommit.CommitsBeforeLimit(10) if err != nil { return nil, fmt.Errorf("newCommit.CommitsBeforeLimit: %v", err) @@ -619,13 +727,9 @@ func createCommitRepoActions(repo *models.Repository, gitRepo *git.Repository, o commits = repo_module.ListToPushCommits(l) } actions = append(actions, &CommitRepoActionOptions{ - PusherName: opts.PusherName, - RepoOwnerID: repo.OwnerID, - RepoName: repo.Name, - RefFullName: opts.RefFullName, - OldCommitID: opts.OldCommitID, - NewCommitID: opts.NewCommitID, - Commits: commits, + PushUpdateOptions: *opts, + RepoOwnerID: repo.OwnerID, + Commits: commits, }) } if err := models.PushUpdateAddDeleteTags(repo, gitRepo, addTags, delTags); err != nil { @@ -633,64 +737,3 @@ func createCommitRepoActions(repo *models.Repository, gitRepo *git.Repository, o } return actions, nil } - -func createCommitRepoActionOption(repo *models.Repository, gitRepo *git.Repository, opts *PushUpdateOptions) (*CommitRepoActionOptions, error) { - isNewRef := opts.OldCommitID == git.EmptySHA - isDelRef := opts.NewCommitID == git.EmptySHA - if isNewRef && isDelRef { - return nil, fmt.Errorf("Old and new revisions are both %s", git.EmptySHA) - } - - var commits = &repo_module.PushCommits{} - if strings.HasPrefix(opts.RefFullName, git.TagPrefix) { - // If is tag reference - tagName := opts.RefFullName[len(git.TagPrefix):] - if isDelRef { - if err := models.PushUpdateDeleteTag(repo, tagName); err != nil { - return nil, fmt.Errorf("PushUpdateDeleteTag: %v", err) - } - } else { - // Clear cache for tag commit count - cache.Remove(repo.GetCommitsCountCacheKey(tagName, true)) - if err := repo_module.PushUpdateAddTag(repo, gitRepo, tagName); err != nil { - return nil, fmt.Errorf("PushUpdateAddTag: %v", err) - } - } - } else if !isDelRef { - // If is branch reference - - // Clear cache for branch commit count - cache.Remove(repo.GetCommitsCountCacheKey(opts.RefFullName[len(git.BranchPrefix):], true)) - - newCommit, err := gitRepo.GetCommit(opts.NewCommitID) - if err != nil { - return nil, fmt.Errorf("gitRepo.GetCommit: %v", err) - } - - // Push new branch. - var l *list.List - if isNewRef { - l, err = newCommit.CommitsBeforeLimit(10) - if err != nil { - return nil, fmt.Errorf("newCommit.CommitsBeforeLimit: %v", err) - } - } else { - l, err = newCommit.CommitsBeforeUntil(opts.OldCommitID) - if err != nil { - return nil, fmt.Errorf("newCommit.CommitsBeforeUntil: %v", err) - } - } - - commits = repo_module.ListToPushCommits(l) - } - - return &CommitRepoActionOptions{ - PusherName: opts.PusherName, - RepoOwnerID: repo.OwnerID, - RepoName: repo.Name, - RefFullName: opts.RefFullName, - OldCommitID: opts.OldCommitID, - NewCommitID: opts.NewCommitID, - Commits: commits, - }, nil -} diff --git a/routers/private/hook.go b/routers/private/hook.go index 7044fdac2..44e82ebe6 100644 --- a/routers/private/hook.go +++ b/routers/private/hook.go @@ -304,12 +304,6 @@ func HookPostReceive(ctx *macaron.Context, opts private.HookOptions) { for i := range opts.OldCommitIDs { refFullName := opts.RefFullNames[i] - branch := opts.RefFullNames[i] - if strings.HasPrefix(branch, git.BranchPrefix) { - branch = strings.TrimPrefix(branch, git.BranchPrefix) - } else { - branch = strings.TrimPrefix(branch, git.TagPrefix) - } // Only trigger activity updates for changes to branches or // tags. Updates to other refs (eg, refs/notes, refs/changes, @@ -336,14 +330,13 @@ func HookPostReceive(ctx *macaron.Context, opts private.HookOptions) { RefFullName: refFullName, OldCommitID: opts.OldCommitIDs[i], NewCommitID: opts.NewCommitIDs[i], - Branch: branch, PusherID: opts.UserID, PusherName: opts.UserName, RepoUserName: ownerName, RepoName: repoName, } updates = append(updates, &option) - if repo.IsEmpty && branch == "master" && strings.HasPrefix(refFullName, git.BranchPrefix) { + if repo.IsEmpty && option.IsBranch() && option.BranchName() == "master" { // put the master branch first copy(updates[1:], updates) updates[0] = &option @@ -355,7 +348,7 @@ func HookPostReceive(ctx *macaron.Context, opts private.HookOptions) { if err := repofiles.PushUpdates(repo, updates); err != nil { log.Error("Failed to Update: %s/%s Total Updates: %d", ownerName, repoName, len(updates)) for i, update := range updates { - log.Error("Failed to Update: %s/%s Update: %d/%d: Branch: %s", ownerName, repoName, i, len(updates), update.Branch) + log.Error("Failed to Update: %s/%s Update: %d/%d: Branch: %s", ownerName, repoName, i, len(updates), update.BranchName()) } log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err)