Refactor push update (#13381)

* Refactor Push update

* Remove the push_test since the function has been removed.

* Use default branch setting instead master

modules/notification/action/action.go

@@ -275,6 +275,75 @@ func (*actionNotifier) NotifyMergePullRequest(pr *models.PullRequest, doer *mode
 	}
 }
 
+func (a *actionNotifier) NotifyPushCommits(pusher *models.User, repo *models.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) {
+	data, err := json.Marshal(commits)
+	if err != nil {
+		log.Error("Marshal: %v", err)
+		return
+	}
+
+	opType := models.ActionCommitRepo
+
+	// Check it's tag push or branch.
+	if opts.IsTag() {
+		opType = models.ActionPushTag
+		if opts.IsDelRef() {
+			opType = models.ActionDeleteTag
+		}
+	} else if opts.IsDelRef() {
+		opType = models.ActionDeleteBranch
+	}
+
+	if err = models.NotifyWatchers(&models.Action{
+		ActUserID: pusher.ID,
+		ActUser:   pusher,
+		OpType:    opType,
+		Content:   string(data),
+		RepoID:    repo.ID,
+		Repo:      repo,
+		RefName:   opts.RefFullName,
+		IsPrivate: repo.IsPrivate,
+	}); err != nil {
+		log.Error("notifyWatchers: %v", err)
+	}
+}
+
+func (a *actionNotifier) NotifyCreateRef(doer *models.User, repo *models.Repository, refType, refFullName string) {
+	opType := models.ActionCommitRepo
+	if refType == "tag" {
+		opType = models.ActionPushTag
+	}
+	if err := models.NotifyWatchers(&models.Action{
+		ActUserID: doer.ID,
+		ActUser:   doer,
+		OpType:    opType,
+		RepoID:    repo.ID,
+		Repo:      repo,
+		IsPrivate: repo.IsPrivate,
+		RefName:   refFullName,
+	}); err != nil {
+		log.Error("notifyWatchers: %v", err)
+	}
+}
+
+func (a *actionNotifier) NotifyDeleteRef(doer *models.User, repo *models.Repository, refType, refFullName string) {
+	opType := models.ActionDeleteBranch
+	if refType == "tag" {
+		opType = models.ActionDeleteTag
+	}
+	if err := models.NotifyWatchers(&models.Action{
+		ActUserID: doer.ID,
+		ActUser:   doer,
+		OpType:    opType,
+		RepoID:    repo.ID,
+		Repo:      repo,
+		IsPrivate: repo.IsPrivate,
+		RefName:   refFullName,
+	}); err != nil {
+		log.Error("notifyWatchers: %v", err)
+	}
+}
+
 func (a *actionNotifier) NotifySyncPushCommits(pusher *models.User, repo *models.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) {
 	data, err := json.Marshal(commits)
 	if err != nil {

services/repository/push.go

@@ -6,7 +6,6 @@ package repository
 
 import (
 	"container/list"
-	"encoding/json"
 	"fmt"
 	"time"
 
@@ -90,7 +89,6 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
 
 	addTags := make([]string, 0, len(optsList))
 	delTags := make([]string, 0, len(optsList))
-	actions := make([]*commitRepoActionOptions, 0, len(optsList))
 	var pusher *models.User
 
 	for _, opts := range optsList {
@@ -102,8 +100,10 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
 			tagName := opts.TagName()
 			if opts.IsDelRef() {
 				delTags = append(delTags, tagName)
+				notification.NotifyDeleteRef(pusher, repo, "tag", opts.RefFullName)
 			} else { // is new tag
 				addTags = append(addTags, tagName)
+				notification.NotifyCreateRef(pusher, repo, "tag", opts.RefFullName)
 			}
 		} else if opts.IsBranch() { // If is branch reference
 			if pusher == nil || pusher.ID != opts.PusherID {
@@ -123,13 +123,32 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
 					return fmt.Errorf("gitRepo.GetCommit: %v", err)
 				}
 
+				refName := opts.RefName()
+
 				// Push new branch.
 				var l *list.List
 				if opts.IsNewRef() {
+					if repo.IsEmpty { // Change default branch and empty status only if pushed ref is non-empty branch.
+						repo.DefaultBranch = refName
+						repo.IsEmpty = false
+						if repo.DefaultBranch != setting.Repository.DefaultBranch {
+							if err := gitRepo.SetDefaultBranch(repo.DefaultBranch); err != nil {
+								if !git.IsErrUnsupportedVersion(err) {
+									return err
+								}
+							}
+						}
+						// Update the is empty and default_branch columns
+						if err := models.UpdateRepositoryCols(repo, "default_branch", "is_empty"); err != nil {
+							return fmt.Errorf("UpdateRepositoryCols: %v", err)
+						}
+					}
+
 					l, err = newCommit.CommitsBeforeLimit(10)
 					if err != nil {
 						return fmt.Errorf("newCommit.CommitsBeforeLimit: %v", err)
 					}
+					notification.NotifyCreateRef(pusher, repo, "branch", opts.RefFullName)
 				} else {
 					l, err = newCommit.CommitsBeforeUntil(opts.OldCommitID)
 					if err != nil {
@@ -152,6 +171,15 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
 				}
 
 				commits = repo_module.ListToPushCommits(l)
+				if len(commits.Commits) > setting.UI.FeedMaxCommitNum {
+					commits.Commits = commits.Commits[:setting.UI.FeedMaxCommitNum]
+				}
+				commits.CompareURL = repo.ComposeCompareURL(opts.OldCommitID, opts.NewCommitID)
+				notification.NotifyPushCommits(pusher, repo, opts, commits)
+
+				if err := repofiles.UpdateIssuesCommit(pusher, repo, commits.Commits, refName); err != nil {
+					log.Error("updateIssuesCommit: %v", err)
+				}
 
 				if err = models.RemoveDeletedBranch(repo.ID, branch); err != nil {
 					log.Error("models.RemoveDeletedBranch %s/%s failed: %v", repo.ID, branch, err)
@@ -161,149 +189,30 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
 				if err := repo_module.CacheRef(repo, gitRepo, opts.RefFullName); err != nil {
 					log.Error("repo_module.CacheRef %s/%s failed: %v", repo.ID, branch, err)
 				}
-			} else if err = pull_service.CloseBranchPulls(pusher, repo.ID, branch); err != nil {
+			} else {
+				notification.NotifyDeleteRef(pusher, repo, "branch", opts.RefFullName)
+				if err = pull_service.CloseBranchPulls(pusher, repo.ID, branch); err != nil {
 					// close all related pulls
 					log.Error("close related pull request failed: %v", err)
 				}
+			}
 
 			// Even if user delete a branch on a repository which he didn't watch, he will be watch that.
 			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)
 			}
+		} else {
+			log.Trace("Non-tag and non-branch commits pushed.")
 		}
-		actions = append(actions, &commitRepoActionOptions{
-			PushUpdateOptions: *opts,
-			Pusher:            pusher,
-			RepoOwnerID:       repo.OwnerID,
-			Commits:           commits,
-		})
 	}
 	if err := repo_module.PushUpdateAddDeleteTags(repo, gitRepo, addTags, delTags); err != nil {
 		return fmt.Errorf("PushUpdateAddDeleteTags: %v", err)
 	}
 
-	if err := commitRepoAction(repo, gitRepo, actions...); err != nil {
-		return fmt.Errorf("commitRepoAction: %v", err)
-	}
-
-	return nil
-}
-
-// commitRepoActionOptions represent options of a new commit action.
-type commitRepoActionOptions struct {
-	repo_module.PushUpdateOptions
-
-	Pusher      *models.User
-	RepoOwnerID int64
-	Commits     *repo_module.PushCommits
-}
-
-// commitRepoAction adds new commit action to the repository, and prepare
-// corresponding webhooks.
-func commitRepoAction(repo *models.Repository, gitRepo *git.Repository, optsList ...*commitRepoActionOptions) error {
-	actions := make([]*models.Action, len(optsList))
-
-	for i, opts := range optsList {
-		if opts.Pusher == nil || opts.Pusher.Name != opts.PusherName {
-			var err error
-			opts.Pusher, err = models.GetUserByName(opts.PusherName)
-			if err != nil {
-				return fmt.Errorf("GetUserByName [%s]: %v", opts.PusherName, err)
-			}
-		}
-
-		refName := git.RefEndName(opts.RefFullName)
-
-		// Change default branch and empty status only if pushed ref is non-empty branch.
-		if repo.IsEmpty && opts.IsBranch() && !opts.IsDelRef() {
-			repo.DefaultBranch = refName
-			repo.IsEmpty = false
-			if refName != "master" {
-				if err := gitRepo.SetDefaultBranch(repo.DefaultBranch); err != nil {
-					if !git.IsErrUnsupportedVersion(err) {
-						return err
-					}
-				}
-			}
-			// Update the is empty and default_branch columns
-			if err := models.UpdateRepositoryCols(repo, "default_branch", "is_empty"); err != nil {
-				return fmt.Errorf("UpdateRepositoryCols: %v", err)
-			}
-		}
-
-		opType := models.ActionCommitRepo
-
-		// Check it's tag push or branch.
-		if opts.IsTag() {
-			opType = models.ActionPushTag
-			if opts.IsDelRef() {
-				opType = models.ActionDeleteTag
-			}
-			opts.Commits = &repo_module.PushCommits{}
-		} else if opts.IsDelRef() {
-			opType = models.ActionDeleteBranch
-			opts.Commits = &repo_module.PushCommits{}
-		} else {
-			// if not the first commit, set the compare URL.
-			if !opts.IsNewRef() {
-				opts.Commits.CompareURL = repo.ComposeCompareURL(opts.OldCommitID, opts.NewCommitID)
-			}
-
-			if err := repofiles.UpdateIssuesCommit(opts.Pusher, repo, opts.Commits.Commits, refName); err != nil {
-				log.Error("updateIssuesCommit: %v", err)
-			}
-		}
-
-		if len(opts.Commits.Commits) > setting.UI.FeedMaxCommitNum {
-			opts.Commits.Commits = opts.Commits.Commits[:setting.UI.FeedMaxCommitNum]
-		}
-
-		data, err := json.Marshal(opts.Commits)
-		if err != nil {
-			return fmt.Errorf("Marshal: %v", err)
-		}
-
-		actions[i] = &models.Action{
-			ActUserID: opts.Pusher.ID,
-			ActUser:   opts.Pusher,
-			OpType:    opType,
-			Content:   string(data),
-			RepoID:    repo.ID,
-			Repo:      repo,
-			RefName:   refName,
-			IsPrivate: repo.IsPrivate,
-		}
-
-		var isHookEventPush = true
-		switch opType {
-		case models.ActionCommitRepo: // Push
-			if opts.IsNewBranch() {
-				notification.NotifyCreateRef(opts.Pusher, repo, "branch", opts.RefFullName)
-			}
-		case models.ActionDeleteBranch: // Delete Branch
-			notification.NotifyDeleteRef(opts.Pusher, repo, "branch", opts.RefFullName)
-
-		case models.ActionPushTag: // Create
-			notification.NotifyCreateRef(opts.Pusher, repo, "tag", opts.RefFullName)
-
-		case models.ActionDeleteTag: // Delete Tag
-			notification.NotifyDeleteRef(opts.Pusher, repo, "tag", opts.RefFullName)
-		default:
-			isHookEventPush = false
-		}
-
-		if isHookEventPush {
-			notification.NotifyPushCommits(opts.Pusher, repo, &opts.PushUpdateOptions, opts.Commits)
-		}
-	}
-
 	// Change repository last updated time.
 	if err := models.UpdateRepositoryUpdatedTime(repo.ID, time.Now()); err != nil {
 		return fmt.Errorf("UpdateRepositoryUpdatedTime: %v", err)
 	}
 
-	if err := models.NotifyWatchers(actions...); err != nil {
-		return fmt.Errorf("NotifyWatchers: %v", err)
-	}
 	return nil
 }

services/repository/push_test.go

@@ -1,139 +0,0 @@
-// Copyright 2020 The Gitea Authors. All rights reserved.
-// Use of this source code is governed by a MIT-style
-// license that can be found in the LICENSE file.
-
-package repository
-
-import (
-	"testing"
-
-	"code.gitea.io/gitea/models"
-	"code.gitea.io/gitea/modules/git"
-	repo_module "code.gitea.io/gitea/modules/repository"
-
-	"github.com/stretchr/testify/assert"
-)
-
-func testCorrectRepoAction(t *testing.T, repo *models.Repository, gitRepo *git.Repository, opts *commitRepoActionOptions, actionBean *models.Action) {
-	models.AssertNotExistsBean(t, actionBean)
-	assert.NoError(t, commitRepoAction(repo, gitRepo, opts))
-	models.AssertExistsAndLoadBean(t, actionBean)
-	models.CheckConsistencyFor(t, &models.Action{})
-}
-
-func TestCommitRepoAction(t *testing.T) {
-	samples := []struct {
-		userID                  int64
-		repositoryID            int64
-		commitRepoActionOptions commitRepoActionOptions
-		action                  models.Action
-	}{
-		{
-			userID:       2,
-			repositoryID: 16,
-			commitRepoActionOptions: commitRepoActionOptions{
-				PushUpdateOptions: repo_module.PushUpdateOptions{
-					RefFullName: "refName",
-					OldCommitID: "oldCommitID",
-					NewCommitID: "newCommitID",
-				},
-				Commits: &repo_module.PushCommits{
-					Commits: []*repo_module.PushCommit{
-						{
-							Sha1:           "69554a6",
-							CommitterEmail: "user2@example.com",
-							CommitterName:  "User2",
-							AuthorEmail:    "user2@example.com",
-							AuthorName:     "User2",
-							Message:        "not signed commit",
-						},
-						{
-							Sha1:           "27566bd",
-							CommitterEmail: "user2@example.com",
-							CommitterName:  "User2",
-							AuthorEmail:    "user2@example.com",
-							AuthorName:     "User2",
-							Message:        "good signed commit (with not yet validated email)",
-						},
-					},
-					Len: 2,
-				},
-			},
-			action: models.Action{
-				OpType:  models.ActionCommitRepo,
-				RefName: "refName",
-			},
-		},
-		{
-			userID:       2,
-			repositoryID: 1,
-			commitRepoActionOptions: commitRepoActionOptions{
-				PushUpdateOptions: repo_module.PushUpdateOptions{
-					RefFullName: git.TagPrefix + "v1.1",
-					OldCommitID: git.EmptySHA,
-					NewCommitID: "newCommitID",
-				},
-				Commits: &repo_module.PushCommits{},
-			},
-			action: models.Action{
-				OpType:  models.ActionPushTag,
-				RefName: "v1.1",
-			},
-		},
-		{
-			userID:       2,
-			repositoryID: 1,
-			commitRepoActionOptions: commitRepoActionOptions{
-				PushUpdateOptions: repo_module.PushUpdateOptions{
-					RefFullName: git.TagPrefix + "v1.1",
-					OldCommitID: "oldCommitID",
-					NewCommitID: git.EmptySHA,
-				},
-				Commits: &repo_module.PushCommits{},
-			},
-			action: models.Action{
-				OpType:  models.ActionDeleteTag,
-				RefName: "v1.1",
-			},
-		},
-		{
-			userID:       2,
-			repositoryID: 1,
-			commitRepoActionOptions: commitRepoActionOptions{
-				PushUpdateOptions: repo_module.PushUpdateOptions{
-					RefFullName: git.BranchPrefix + "feature/1",
-					OldCommitID: "oldCommitID",
-					NewCommitID: git.EmptySHA,
-				},
-				Commits: &repo_module.PushCommits{},
-			},
-			action: models.Action{
-				OpType:  models.ActionDeleteBranch,
-				RefName: "feature/1",
-			},
-		},
-	}
-
-	for _, s := range samples {
-		models.PrepareTestEnv(t)
-
-		user := models.AssertExistsAndLoadBean(t, &models.User{ID: s.userID}).(*models.User)
-		repo := models.AssertExistsAndLoadBean(t, &models.Repository{ID: s.repositoryID, OwnerID: user.ID}).(*models.Repository)
-		repo.Owner = user
-
-		gitRepo, err := git.OpenRepository(repo.RepoPath())
-		assert.NoError(t, err)
-
-		s.commitRepoActionOptions.PusherName = user.Name
-		s.commitRepoActionOptions.RepoOwnerID = user.ID
-		s.commitRepoActionOptions.RepoName = repo.Name
-
-		s.action.ActUserID = user.ID
-		s.action.RepoID = repo.ID
-		s.action.Repo = repo
-		s.action.IsPrivate = repo.IsPrivate
-
-		testCorrectRepoAction(t, repo, gitRepo, &s.commitRepoActionOptions, &s.action)
-		gitRepo.Close()
-	}
-}