General code quality improvement

7 files changed, 150 insertions, 149 deletions

diff --git a/models/action.go b/models/action.go
index 8ac599c0e1..54695417a7 100644
--- a/models/action.go
+++ b/models/action.go
@@ -71,7 +71,6 @@ type Action struct {
 	OpType       ActionType
 	ActUserID    int64  // Action user id.
 	ActUserName  string // Action user name.
-	ActEmail     string
 	ActAvatar    string `xorm:"-"`
 	RepoID       int64
 	RepoUserName string
@@ -106,10 +105,6 @@ func (a *Action) ShortActUserName() string {
 	return base.EllipsisString(a.ActUserName, 20)
 }
 
-func (a *Action) GetActEmail() string {
-	return a.ActEmail
-}
-
 func (a *Action) GetRepoUserName() string {
 	return a.RepoUserName
 }
@@ -181,7 +176,6 @@ func newRepoAction(e Engine, u *User, repo *Repository) (err error) {
 	if err = notifyWatchers(e, &Action{
 		ActUserID:    u.ID,
 		ActUserName:  u.Name,
-		ActEmail:     u.Email,
 		OpType:       ACTION_CREATE_REPO,
 		RepoID:       repo.ID,
 		RepoUserName: repo.Owner.Name,
@@ -204,7 +198,6 @@ func renameRepoAction(e Engine, actUser *User, oldRepoName string, repo *Reposit
 	if err = notifyWatchers(e, &Action{
 		ActUserID:    actUser.ID,
 		ActUserName:  actUser.Name,
-		ActEmail:     actUser.Email,
 		OpType:       ACTION_RENAME_REPO,
 		RepoID:       repo.ID,
 		RepoUserName: repo.Owner.Name,
@@ -305,8 +298,8 @@ func (push *PushCommits) AvatarLink(email string) string {
 	return push.avatars[email]
 }
 
-// updateIssuesCommit checks if issues are manipulated by commit message.
-func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string, commits []*PushCommit) error {
+// UpdateIssuesCommit checks if issues are manipulated by commit message.
+func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit) error {
 	// Commits are appended in the reverse order.
 	for i := len(commits) - 1; i >= 0; i-- {
 		c := commits[i]
@@ -322,7 +315,7 @@ func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string
 
 			// Add repo name if missing
 			if ref[0] == '#' {
-				ref = fmt.Sprintf("%s/%s%s", repoUserName, repoName, ref)
+				ref = fmt.Sprintf("%s%s", repo.FullName(), ref)
 			} else if !strings.Contains(ref, "/") {
 				// FIXME: We don't support User#ID syntax yet
 				// return ErrNotImplemented
@@ -342,9 +335,8 @@ func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string
 			}
 			refMarked[issue.ID] = true
 
-			url := fmt.Sprintf("%s/%s/%s/commit/%s", setting.AppSubUrl, repoUserName, repoName, c.Sha1)
-			message := fmt.Sprintf(`<a href="%s">%s</a>`, url, c.Message)
-			if err = CreateRefComment(u, repo, issue, message, c.Sha1); err != nil {
+			message := fmt.Sprintf(`<a href="%s/commit/%s">%s</a>`, repo.Link(), c.Sha1, c.Message)
+			if err = CreateRefComment(doer, repo, issue, message, c.Sha1); err != nil {
 				return err
 			}
 		}
@@ -361,7 +353,7 @@ func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string
 
 			// Add repo name if missing
 			if ref[0] == '#' {
-				ref = fmt.Sprintf("%s/%s%s", repoUserName, repoName, ref)
+				ref = fmt.Sprintf("%s%s", repo.FullName(), ref)
 			} else if !strings.Contains(ref, "/") {
 				// We don't support User#ID syntax yet
 				// return ErrNotImplemented
@@ -385,7 +377,7 @@ func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string
 				continue
 			}
 
-			if err = issue.ChangeStatus(u, repo, true); err != nil {
+			if err = issue.ChangeStatus(doer, repo, true); err != nil {
 				return err
 			}
 		}
@@ -401,7 +393,7 @@ func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string
 
 			// Add repo name if missing
 			if ref[0] == '#' {
-				ref = fmt.Sprintf("%s/%s%s", repoUserName, repoName, ref)
+				ref = fmt.Sprintf("%s%s", repo.FullName(), ref)
 			} else if !strings.Contains(ref, "/") {
 				// We don't support User#ID syntax yet
 				// return ErrNotImplemented
@@ -425,7 +417,7 @@ func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string
 				continue
 			}
 
-			if err = issue.ChangeStatus(u, repo, false); err != nil {
+			if err = issue.ChangeStatus(doer, repo, false); err != nil {
 				return err
 			}
 		}
@@ -433,27 +425,26 @@ func updateIssuesCommit(u *User, repo *Repository, repoUserName, repoName string
 	return nil
 }
 
-// CommitRepoAction adds new action for committing repository.
-// TODO: use CommitRepoActionOptions
-func CommitRepoAction(
-	userID, repoUserID int64,
-	userName, actEmail string,
-	repoID int64,
-	repoUserName, repoName string,
-	refFullName string,
-	commit *PushCommits,
-	oldCommitID string, newCommitID string) error {
+type CommitRepoActionOptions struct {
+	PusherName  string
+	RepoOwnerID int64
+	RepoName    string
+	RefFullName string
+	OldCommitID string
+	NewCommitID string
+	Commits     *PushCommits
+}
 
-	u, err := GetUserByID(userID)
+// CommitRepoAction adds new commit actio to the repository, and prepare corresponding webhooks.
+func CommitRepoAction(opts CommitRepoActionOptions) error {
+	pusher, err := GetUserByName(opts.PusherName)
 	if err != nil {
-		return fmt.Errorf("GetUserByID: %v", err)
+		return fmt.Errorf("GetUserByName [%s]: %v", opts.PusherName, err)
 	}
 
-	repo, err := GetRepositoryByName(repoUserID, repoName)
+	repo, err := GetRepositoryByName(opts.RepoOwnerID, opts.RepoName)
 	if err != nil {
-		return fmt.Errorf("GetRepositoryByName: %v", err)
-	} else if err = repo.GetOwner(); err != nil {
-		return fmt.Errorf("GetOwner: %v", err)
+		return fmt.Errorf("GetRepositoryByName [owner_id: %d, name: %s]: %v", opts.RepoOwnerID, opts.RepoName, err)
 	}
 
 	// Change repository bare status and update last updated time.
@@ -465,40 +456,39 @@ func CommitRepoAction(
 	isNewBranch := false
 	opType := ACTION_COMMIT_REPO
 	// Check it's tag push or branch.
-	if strings.HasPrefix(refFullName, "refs/tags/") {
+	if strings.HasPrefix(opts.RefFullName, git.TAG_PREFIX) {
 		opType = ACTION_PUSH_TAG
-		commit = &PushCommits{}
+		opts.Commits = &PushCommits{}
 	} else {
-		// if not the first commit, set the compareUrl
-		if !strings.HasPrefix(oldCommitID, "0000000") {
-			commit.CompareURL = repo.ComposeCompareURL(oldCommitID, newCommitID)
-		} else {
+		// if not the first commit, set the compare URL.
+		if opts.OldCommitID == git.EMPTY_SHA {
 			isNewBranch = true
+		} else {
+			opts.Commits.CompareURL = repo.ComposeCompareURL(opts.OldCommitID, opts.NewCommitID)
 		}
 
-		if err = updateIssuesCommit(u, repo, repoUserName, repoName, commit.Commits); err != nil {
+		if err = UpdateIssuesCommit(pusher, repo, opts.Commits.Commits); err != nil {
 			log.Error(4, "updateIssuesCommit: %v", err)
 		}
 	}
 
-	if len(commit.Commits) > setting.UI.FeedMaxCommitNum {
-		commit.Commits = commit.Commits[:setting.UI.FeedMaxCommitNum]
+	if len(opts.Commits.Commits) > setting.UI.FeedMaxCommitNum {
+		opts.Commits.Commits = opts.Commits.Commits[:setting.UI.FeedMaxCommitNum]
 	}
 
-	bs, err := json.Marshal(commit)
+	data, err := json.Marshal(opts.Commits)
 	if err != nil {
 		return fmt.Errorf("Marshal: %v", err)
 	}
 
-	refName := git.RefEndName(refFullName)
+	refName := git.RefEndName(opts.RefFullName)
 	if err = NotifyWatchers(&Action{
-		ActUserID:    u.ID,
-		ActUserName:  userName,
-		ActEmail:     actEmail,
+		ActUserID:    pusher.ID,
+		ActUserName:  pusher.Name,
 		OpType:       opType,
-		Content:      string(bs),
+		Content:      string(data),
 		RepoID:       repo.ID,
-		RepoUserName: repoUserName,
+		RepoUserName: repo.MustOwner().Name,
 		RepoName:     repo.Name,
 		RefName:      refName,
 		IsPrivate:    repo.IsPrivate,
@@ -506,21 +496,20 @@ func CommitRepoAction(
 		return fmt.Errorf("NotifyWatchers: %v", err)
 	}
 
-	pusher, err := GetUserByName(userName)
-	if err != nil {
-		return fmt.Errorf("GetUserByName: %v", err)
-	}
-	apiPusher := pusher.APIFormat()
+	defer func() {
+		go HookQueue.Add(repo.ID)
+	}()
 
+	apiPusher := pusher.APIFormat()
 	apiRepo := repo.APIFormat(nil)
 	switch opType {
 	case ACTION_COMMIT_REPO: // Push
 		if err = PrepareWebhooks(repo, HOOK_EVENT_PUSH, &api.PushPayload{
-			Ref:        refFullName,
-			Before:     oldCommitID,
-			After:      newCommitID,
-			CompareURL: setting.AppUrl + commit.CompareURL,
-			Commits:    commit.ToApiPayloadCommits(repo.HTMLURL()),
+			Ref:        opts.RefFullName,
+			Before:     opts.OldCommitID,
+			After:      opts.NewCommitID,
+			CompareURL: setting.AppUrl + opts.Commits.CompareURL,
+			Commits:    opts.Commits.ToApiPayloadCommits(repo.HTMLURL()),
 			Repo:       apiRepo,
 			Pusher:     apiPusher,
 			Sender:     apiPusher,
@@ -549,44 +538,42 @@ func CommitRepoAction(
 	return nil
 }
 
-func transferRepoAction(e Engine, actUser, oldOwner, newOwner *User, repo *Repository) (err error) {
+func transferRepoAction(e Engine, doer, oldOwner *User, repo *Repository) (err error) {
 	if err = notifyWatchers(e, &Action{
-		ActUserID:    actUser.ID,
-		ActUserName:  actUser.Name,
-		ActEmail:     actUser.Email,
+		ActUserID:    doer.ID,
+		ActUserName:  doer.Name,
 		OpType:       ACTION_TRANSFER_REPO,
 		RepoID:       repo.ID,
-		RepoUserName: newOwner.Name,
+		RepoUserName: repo.Owner.Name,
 		RepoName:     repo.Name,
 		IsPrivate:    repo.IsPrivate,
 		Content:      path.Join(oldOwner.Name, repo.Name),
 	}); err != nil {
-		return fmt.Errorf("notify watchers '%d/%d': %v", actUser.ID, repo.ID, err)
+		return fmt.Errorf("notifyWatchers: %v", err)
 	}
 
 	// Remove watch for organization.
-	if repo.Owner.IsOrganization() {
-		if err = watchRepo(e, repo.Owner.ID, repo.ID, false); err != nil {
-			return fmt.Errorf("watch repository: %v", err)
+	if oldOwner.IsOrganization() {
+		if err = watchRepo(e, oldOwner.ID, repo.ID, false); err != nil {
+			return fmt.Errorf("watchRepo [false]: %v", err)
 		}
 	}
 
-	log.Trace("action.transferRepoAction: %s/%s", actUser.Name, repo.Name)
 	return nil
 }
 
-// TransferRepoAction adds new action for transferring repository.
-func TransferRepoAction(actUser, oldOwner, newOwner *User, repo *Repository) error {
-	return transferRepoAction(x, actUser, oldOwner, newOwner, repo)
+// TransferRepoAction adds new action for transferring repository,
+// the Owner field of repository is assumed to be new owner.
+func TransferRepoAction(doer, oldOwner *User, repo *Repository) error {
+	return transferRepoAction(x, doer, oldOwner, repo)
 }
 
-func mergePullRequestAction(e Engine, actUser *User, repo *Repository, pull *Issue) error {
+func mergePullRequestAction(e Engine, doer *User, repo *Repository, issue *Issue) error {
 	return notifyWatchers(e, &Action{
-		ActUserID:    actUser.ID,
-		ActUserName:  actUser.Name,
-		ActEmail:     actUser.Email,
+		ActUserID:    doer.ID,
+		ActUserName:  doer.Name,
 		OpType:       ACTION_MERGE_PULL_REQUEST,
-		Content:      fmt.Sprintf("%d|%s", pull.Index, pull.Title),
+		Content:      fmt.Sprintf("%d|%s", issue.Index, issue.Title),
 		RepoID:       repo.ID,
 		RepoUserName: repo.Owner.Name,
 		RepoName:     repo.Name,
diff --git a/models/issue.go b/models/issue.go
index 53f5b04f85..3e19d11907 100644
--- a/models/issue.go
+++ b/models/issue.go
@@ -707,7 +707,6 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string)
 	if err = NotifyWatchers(&Action{
 		ActUserID:    issue.Poster.ID,
 		ActUserName:  issue.Poster.Name,
-		ActEmail:     issue.Poster.Email,
 		OpType:       ACTION_CREATE_ISSUE,
 		Content:      fmt.Sprintf("%d|%s", issue.Index, issue.Title),
 		RepoID:       repo.ID,
diff --git a/models/issue_comment.go b/models/issue_comment.go
index 9194060938..bd1a089ba6 100644
--- a/models/issue_comment.go
+++ b/models/issue_comment.go
@@ -157,7 +157,6 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err
 	act := &Action{
 		ActUserID:    opts.Doer.ID,
 		ActUserName:  opts.Doer.Name,
-		ActEmail:     opts.Doer.Email,
 		Content:      fmt.Sprintf("%d|%s", opts.Issue.Index, strings.Split(opts.Content, "\n")[0]),
 		RepoID:       opts.Repo.ID,
 		RepoUserName: opts.Repo.Owner.Name,
diff --git a/models/pull.go b/models/pull.go
index 472c87b52b..fa9a44897a 100644
--- a/models/pull.go
+++ b/models/pull.go
@@ -429,7 +429,6 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str
 	if err = NotifyWatchers(&Action{
 		ActUserID:    pull.Poster.ID,
 		ActUserName:  pull.Poster.Name,
-		ActEmail:     pull.Poster.Email,
 		OpType:       ACTION_CREATE_PULL_REQUEST,
 		Content:      fmt.Sprintf("%d|%s", pull.Index, pull.Title),
 		RepoID:       repo.ID,
diff --git a/models/repo.go b/models/repo.go
index e7c218a5f3..c08c7842fd 100644
--- a/models/repo.go
+++ b/models/repo.go
@@ -393,12 +393,12 @@ func (repo *Repository) GitConfigPath() string {
 	return filepath.Join(repo.RepoPath(), "config")
 }
 
-func (repo *Repository) Link() string {
-	return setting.AppSubUrl + "/" + repo.MustOwner().Name + "/" + repo.Name
+func (repo *Repository) RelLink() string {
+	return "/" + repo.FullName()
 }
 
-func (repo *Repository) RelLink() string {
-	return "/" + repo.MustOwner().Name + "/" + repo.Name
+func (repo *Repository) Link() string {
+	return setting.AppSubUrl + "/" + repo.FullName()
 }
 
 func (repo *Repository) ComposeCompareURL(oldCommitID, newCommitID string) string {
@@ -1167,7 +1167,7 @@ func RepoPath(userName, repoName string) string {
 }
 
 // TransferOwnership transfers all corresponding setting from old user to new one.
-func TransferOwnership(u *User, newOwnerName string, repo *Repository) error {
+func TransferOwnership(doer *User, newOwnerName string, repo *Repository) error {
 	newOwner, err := GetUserByName(newOwnerName)
 	if err != nil {
 		return fmt.Errorf("get new owner '%s': %v", newOwnerName, err)
@@ -1190,7 +1190,7 @@ func TransferOwnership(u *User, newOwnerName string, repo *Repository) error {
 	owner := repo.Owner
 
 	// Note: we have to set value here to make sure recalculate accesses is based on
-	//	new owner.
+	// new owner.
 	repo.OwnerID = newOwner.ID
 	repo.Owner = newOwner
 
@@ -1260,7 +1260,7 @@ func TransferOwnership(u *User, newOwnerName string, repo *Repository) error {
 
 	if err = watchRepo(sess, newOwner.ID, repo.ID, true); err != nil {
 		return fmt.Errorf("watchRepo: %v", err)
-	} else if err = transferRepoAction(sess, u, owner, newOwner, repo); err != nil {
+	} else if err = transferRepoAction(sess, doer, owner, repo); err != nil {
 		return fmt.Errorf("transferRepoAction: %v", err)
 	}
 
@@ -1530,16 +1530,16 @@ func GetRepositoryByRef(ref string) (*Repository, error) {
 }
 
 // GetRepositoryByName returns the repository by given name under user if exists.
-func GetRepositoryByName(uid int64, repoName string) (*Repository, error) {
+func GetRepositoryByName(ownerID int64, name string) (*Repository, error) {
 	repo := &Repository{
-		OwnerID:   uid,
-		LowerName: strings.ToLower(repoName),
+		OwnerID:   ownerID,
+		LowerName: strings.ToLower(name),
 	}
 	has, err := x.Get(repo)
 	if err != nil {
 		return nil, err
 	} else if !has {
-		return nil, ErrRepoNotExist{0, uid, repoName}
+		return nil, ErrRepoNotExist{0, ownerID, name}
 	}
 	return repo, err
 }
diff --git a/models/repo_editor.go b/models/repo_editor.go
index 740abfee1d..bbe2ac04bc 100644
--- a/models/repo_editor.go
+++ b/models/repo_editor.go
@@ -156,13 +156,18 @@ func (repo *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) (
 	if opts.NewBranch != opts.OldBranch {
 		oldCommitID = git.EMPTY_SHA
 	}
-	if err := CommitRepoAction(doer.ID, repo.MustOwner().ID, doer.Name, doer.Email,
-		repo.ID, repo.MustOwner().Name, repo.Name, git.BRANCH_PREFIX+opts.NewBranch,
-		pushCommits, oldCommitID, commit.ID.String()); err != nil {
+	if err := CommitRepoAction(CommitRepoActionOptions{
+		PusherName:  doer.Name,
+		RepoOwnerID: repo.MustOwner().ID,
+		RepoName:    repo.Name,
+		RefFullName: git.BRANCH_PREFIX + opts.NewBranch,
+		OldCommitID: oldCommitID,
+		NewCommitID: commit.ID.String(),
+		Commits:     pushCommits,
+	}); err != nil {
 		log.Error(4, "CommitRepoAction: %v", err)
 		return nil
 	}
-	go HookQueue.Add(repo.ID)
 
 	return nil
 }
@@ -221,30 +226,41 @@ func (repo *Repository) GetDiffPreview(branch, treeName, content string) (diff *
 //         \/     \/          \/          \/       \/                 \/
 //
 
-func (repo *Repository) DeleteRepoFile(doer *User, oldCommitID, branch, treeName, message string) (err error) {
+type DeleteRepoFileOptions struct {
+	LastCommitID string
+	Branch       string
+	TreePath     string
+	Message      string
+}
+
+func (repo *Repository) DeleteRepoFile(doer *User, opts DeleteRepoFileOptions) (err error) {
 	repoWorkingPool.CheckIn(com.ToStr(repo.ID))
 	defer repoWorkingPool.CheckOut(com.ToStr(repo.ID))
 
 	localPath := repo.LocalCopyPath()
-	if err = discardLocalRepoBranchChanges(localPath, branch); err != nil {
-		return fmt.Errorf("discardLocalRepoChanges: %v", err)
-	} else if err = repo.UpdateLocalCopyBranch(branch); err != nil {
-		return fmt.Errorf("UpdateLocalCopyBranch: %v", err)
+	if err = discardLocalRepoBranchChanges(localPath, opts.Branch); err != nil {
+		return fmt.Errorf("discardLocalRepoBranchChanges [branch: %s]: %v", opts.Branch, err)
+	} else if err = repo.UpdateLocalCopyBranch(opts.Branch); err != nil {
+		return fmt.Errorf("UpdateLocalCopyBranch [branch: %s]: %v", opts.Branch, err)
 	}
 
-	filePath := path.Join(localPath, treeName)
-	os.Remove(filePath)
+	if err = os.Remove(path.Join(localPath, opts.TreePath)); err != nil {
+		return fmt.Errorf("Remove: %v", err)
+	}
 
-	if len(message) == 0 {
-		message = "Delete file '" + treeName + "'"
+	if len(opts.Message) == 0 {
+		opts.Message = "Delete file '" + opts.TreePath + "'"
 	}
 
 	if err = git.AddChanges(localPath, true); err != nil {
-		return fmt.Errorf("AddChanges: %v", err)
-	} else if err = git.CommitChanges(localPath, message, doer.NewGitSig()); err != nil {
-		return fmt.Errorf("CommitChanges: %v", err)
-	} else if err = git.Push(localPath, "origin", branch); err != nil {
-		return fmt.Errorf("Push: %v", err)
+		return fmt.Errorf("git add --all: %v", err)
+	}
+
+	signaure := doer.NewGitSig()
+	if err = git.CommitChanges(localPath, opts.Message, signaure); err != nil {
+		return fmt.Errorf("git commit -m %s --author='%s <%s>': %v", opts.Message, signaure.Name, signaure.Email, err)
+	} else if err = git.Push(localPath, "origin", opts.Branch); err != nil {
+		return fmt.Errorf("git push origin %s: %v", opts.Branch, err)
 	}
 
 	gitRepo, err := git.OpenRepository(repo.RepoPath())
@@ -252,23 +268,29 @@ func (repo *Repository) DeleteRepoFile(doer *User, oldCommitID, branch, treeName
 		log.Error(4, "OpenRepository: %v", err)
 		return nil
 	}
-	commit, err := gitRepo.GetBranchCommit(branch)
+	commit, err := gitRepo.GetBranchCommit(opts.Branch)
 	if err != nil {
-		log.Error(4, "GetBranchCommit [branch: %s]: %v", branch, err)
+		log.Error(4, "GetBranchCommit [branch: %s]: %v", opts.Branch, err)
 		return nil
 	}
 
+	// Simulate push event.
 	pushCommits := &PushCommits{
 		Len:     1,
 		Commits: []*PushCommit{CommitToPushCommit(commit)},
 	}
-	if err := CommitRepoAction(doer.ID, repo.MustOwner().ID, doer.Name, doer.Email,
-		repo.ID, repo.MustOwner().Name, repo.Name, git.BRANCH_PREFIX+branch,
-		pushCommits, oldCommitID, commit.ID.String()); err != nil {
+	if err := CommitRepoAction(CommitRepoActionOptions{
+		PusherName:  doer.Name,
+		RepoOwnerID: repo.MustOwner().ID,
+		RepoName:    repo.Name,
+		RefFullName: git.BRANCH_PREFIX + opts.Branch,
+		OldCommitID: opts.LastCommitID,
+		NewCommitID: commit.ID.String(),
+		Commits:     pushCommits,
+	}); err != nil {
 		log.Error(4, "CommitRepoAction: %v", err)
 		return nil
 	}
-	go HookQueue.Add(repo.ID)
 
 	return nil
 }
diff --git a/models/update.go b/models/update.go
index 952794538a..79d62c8f6a 100644
--- a/models/update.go
+++ b/models/update.go
@@ -74,22 +74,22 @@ func ListToPushCommits(l *list.List) *PushCommits {
 }
 
 type PushUpdateOptions struct {
-	RefName      string
-	OldCommitID  string
-	NewCommitID  string
 	PusherID     int64
 	PusherName   string
 	RepoUserName string
 	RepoName     string
+	RefFullName  string
+	OldCommitID  string
+	NewCommitID  string
 }
 
 // PushUpdate must be called for any push actions in order to
 // generates necessary push action history feeds.
 func PushUpdate(opts PushUpdateOptions) (err error) {
-	isNewRef := strings.HasPrefix(opts.OldCommitID, "0000000")
-	isDelRef := strings.HasPrefix(opts.NewCommitID, "0000000")
+	isNewRef := opts.OldCommitID == git.EMPTY_SHA
+	isDelRef := opts.NewCommitID == git.EMPTY_SHA
 	if isNewRef && isDelRef {
-		return fmt.Errorf("Old and new revisions both start with 000000")
+		return fmt.Errorf("Old and new revisions are both %s", git.EMPTY_SHA)
 	}
 
 	repoPath := RepoPath(opts.RepoUserName, opts.RepoName)
@@ -102,7 +102,7 @@ func PushUpdate(opts PushUpdateOptions) (err error) {
 
 	if isDelRef {
 		log.GitLogger.Info("Reference '%s' has been deleted from '%s/%s' by %d",
-			opts.RefName, opts.RepoUserName, opts.RepoName, opts.PusherName)
+			opts.RefFullName, opts.RepoUserName, opts.RepoName, opts.PusherName)
 		return nil
 	}
 
@@ -111,41 +111,30 @@ func PushUpdate(opts PushUpdateOptions) (err error) {
 		return fmt.Errorf("OpenRepository: %v", err)
 	}
 
-	repoUser, err := GetUserByName(opts.RepoUserName)
+	owner, err := GetUserByName(opts.RepoUserName)
 	if err != nil {
 		return fmt.Errorf("GetUserByName: %v", err)
 	}
 
-	repo, err := GetRepositoryByName(repoUser.ID, opts.RepoName)
+	repo, err := GetRepositoryByName(owner.ID, opts.RepoName)
 	if err != nil {
 		return fmt.Errorf("GetRepositoryByName: %v", err)
 	}
 
 	// Push tags.
-	if strings.HasPrefix(opts.RefName, "refs/tags/") {
-		tag, err := gitRepo.GetTag(git.RefEndName(opts.RefName))
-		if err != nil {
-			return fmt.Errorf("gitRepo.GetTag: %v", err)
-		}
-
-		// When tagger isn't available, fall back to get committer email.
-		var actEmail string
-		if tag.Tagger != nil {
-			actEmail = tag.Tagger.Email
-		} else {
-			cmt, err := tag.Commit()
-			if err != nil {
-				return fmt.Errorf("tag.Commit: %v", err)
-			}
-			actEmail = cmt.Committer.Email
-		}
-
-		commit := &PushCommits{}
-		if err = CommitRepoAction(opts.PusherID, repoUser.ID, opts.PusherName, actEmail,
-			repo.ID, opts.RepoUserName, opts.RepoName, opts.RefName, commit, opts.OldCommitID, opts.NewCommitID); err != nil {
+	if strings.HasPrefix(opts.RefFullName, git.TAG_PREFIX) {
+		if err := CommitRepoAction(CommitRepoActionOptions{
+			PusherName:  opts.PusherName,
+			RepoOwnerID: owner.ID,
+			RepoName:    repo.Name,
+			RefFullName: opts.RefFullName,
+			OldCommitID: opts.OldCommitID,
+			NewCommitID: opts.NewCommitID,
+			Commits:     &PushCommits{},
+		}); err != nil {
 			return fmt.Errorf("CommitRepoAction (tag): %v", err)
 		}
-		return err
+		return nil
 	}
 
 	newCommit, err := gitRepo.GetCommit(opts.NewCommitID)
@@ -167,9 +156,15 @@ func PushUpdate(opts PushUpdateOptions) (err error) {
 		}
 	}
 
-	if err = CommitRepoAction(opts.PusherID, repoUser.ID, opts.PusherName, repoUser.Email,
-		repo.ID, opts.RepoUserName, opts.RepoName, opts.RefName, ListToPushCommits(l),
-		opts.OldCommitID, opts.NewCommitID); err != nil {
+	if err := CommitRepoAction(CommitRepoActionOptions{
+		PusherName:  opts.PusherName,
+		RepoOwnerID: owner.ID,
+		RepoName:    repo.Name,
+		RefFullName: opts.RefFullName,
+		OldCommitID: opts.OldCommitID,
+		NewCommitID: opts.NewCommitID,
+		Commits:     ListToPushCommits(l),
+	}); err != nil {
 		return fmt.Errorf("CommitRepoAction (branch): %v", err)
 	}
 	return nil