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 <lauris@nix.lv>

.golangci.yml

@@ -95,3 +95,6 @@ issues:
     - linters:
         - misspell
       text: '`Unknwon` is a misspelling of `Unknown`'
+    - path: models/update.go
+      linters:
+        - unused

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 {
+			if !opts.IsNewRef() {
-				isNewBranch = true
-			} else {
 				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

modules/repofiles/action_test.go

@@ -32,9 +32,11 @@ func TestCommitRepoAction(t *testing.T) {
 			userID:       2,
 			repositoryID: 16,
 			commitRepoActionOptions: CommitRepoActionOptions{
-				RefFullName: "refName",
+				PushUpdateOptions: PushUpdateOptions{
-				OldCommitID: "oldCommitID",
+					RefFullName: "refName",
-				NewCommitID: "newCommitID",
+					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",
+				PushUpdateOptions: PushUpdateOptions{
-				OldCommitID: git.EmptySHA,
+					RefFullName: git.TagPrefix + "v1.1",
-				NewCommitID: "newCommitID",
+					OldCommitID: git.EmptySHA,
-				Commits:     &repository.PushCommits{},
+					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",
+				PushUpdateOptions: PushUpdateOptions{
-				OldCommitID: "oldCommitID",
+					RefFullName: git.TagPrefix + "v1.1",
-				NewCommitID: git.EmptySHA,
+					OldCommitID: "oldCommitID",
-				Commits:     &repository.PushCommits{},
+					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",
+				PushUpdateOptions: PushUpdateOptions{
-				OldCommitID: "oldCommitID",
+					RefFullName: git.BranchPrefix + "feature/1",
-				NewCommitID: git.EmptySHA,
+					OldCommitID: "oldCommitID",
-				Commits:     &repository.PushCommits{},
+					NewCommitID: git.EmptySHA,
+				},
+				Commits: &repository.PushCommits{},
 			},
 			action: models.Action{
 				OpType:  models.ActionDeleteBranch,

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
+	if opts.IsNewRef() && opts.IsDelRef() {
-	isDelRef := opts.NewCommitID == git.EmptySHA
-	if isNewRef && 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)
+	var commits = &repo_module.PushCommits{}
-	if err != nil {
-		return err
-	}
 
-	if err := CommitRepoAction(commitRepoActionOptions); err != nil {
+	if opts.IsTag() { // If is tag reference
-		return fmt.Errorf("CommitRepoAction: %v", err)
+		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
+		}
 
-	pusher, err := models.GetUserByID(opts.PusherID)
+		if !opts.IsDelRef() {
-	if err != nil {
+			// Clear cache for branch commit count
-		return err
+			cache.Remove(repo.GetCommitsCountCacheKey(opts.BranchName(), true))
-	}
 
-	if !isDelRef {
+			newCommit, err := gitRepo.GetCommit(opts.NewCommitID)
-		if err = models.RemoveDeletedBranch(repo.ID, opts.Branch); err != nil {
+			if err != nil {
-			log.Error("models.RemoveDeletedBranch %s/%s failed: %v", repo.ID, opts.Branch, err)
+				return fmt.Errorf("gitRepo.GetCommit: %v", err)
-		}
+			}
 
-		log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name)
+			// 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)
+			}
 
-		go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, opts.OldCommitID, opts.NewCommitID)
+			if err = models.WatchIfAuto(opts.PusherID, repo.ID, true); err != nil {
-		// close all related pulls
+				log.Warn("Fail to perform auto watch on user %v for repo %v: %v", opts.PusherID, repo.ID, err)
-	} else if err = pull_service.CloseBranchPulls(pusher, repo.ID, branch); err != nil {
+			}
-		log.Error("close related pull request failed: %v", 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 {
+	if err := CommitRepoAction(&CommitRepoActionOptions{
-		log.Warn("Fail to perform auto watch on user %v for repo %v: %v", opts.PusherID, repo.ID, err)
+		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 !opts.IsDelRef() {
-			if err = models.RemoveDeletedBranch(repo.ID, opts.Branch); err != nil {
+			if err = models.RemoveDeletedBranch(repo.ID, branch); err != nil {
-				log.Error("models.RemoveDeletedBranch %s/%s failed: %v", repo.ID, opts.Branch, err)
+				log.Error("models.RemoveDeletedBranch %s/%s failed: %v", repo.ID, branch, 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, opts.Branch, pusher.Name)
+			log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name)
 
-			go pull_service.AddTestPullRequestTask(pusher, repo.ID, opts.Branch, true, opts.OldCommitID, opts.NewCommitID)
+			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
+		if opts.IsNewRef() && opts.IsDelRef() {
-		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 opts.IsNewTag() {
 			// If is tag reference
-			tagName := opts.RefFullName[len(git.TagPrefix):]
+			tagName := opts.TagName()
-			if isDelRef {
+			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,
+			PushUpdateOptions: *opts,
-			RepoOwnerID: repo.OwnerID,
+			RepoOwnerID:       repo.OwnerID,
-			RepoName:    repo.Name,
+			Commits:           commits,
-			RefFullName: opts.RefFullName,
-			OldCommitID: opts.OldCommitID,
-			NewCommitID: opts.NewCommitID,
-			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
-}

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)