From aac663e0da0af644ae1011d268d027160265dce3 Mon Sep 17 00:00:00 2001 From: KN4CK3R Date: Tue, 29 Jun 2021 15:34:03 +0200 Subject: [PATCH] Implemented head_commit for webhooks (#16282) * Removed Len field. * Added head_commit webhook field. * Added comment for returns. --- modules/notification/webhook/webhook.go | 6 +- modules/repository/commits.go | 133 +++++++++++++----------- modules/repository/commits_test.go | 18 +++- routers/api/v1/repo/hook.go | 19 ++-- routers/web/repo/webhook.go | 44 ++++---- services/repository/push.go | 4 +- templates/user/dashboard/feeds.tmpl | 2 +- 7 files changed, 129 insertions(+), 97 deletions(-) diff --git a/modules/notification/webhook/webhook.go b/modules/notification/webhook/webhook.go index 90dc59021..acdb91efe 100644 --- a/modules/notification/webhook/webhook.go +++ b/modules/notification/webhook/webhook.go @@ -562,7 +562,7 @@ func (m *webhookNotifier) NotifyIssueChangeMilestone(doer *models.User, issue *m func (m *webhookNotifier) NotifyPushCommits(pusher *models.User, repo *models.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) { apiPusher := convert.ToUser(pusher, nil) - apiCommits, err := commits.ToAPIPayloadCommits(repo.RepoPath(), repo.HTMLURL()) + apiCommits, apiHeadCommit, err := commits.ToAPIPayloadCommits(repo.RepoPath(), repo.HTMLURL()) if err != nil { log.Error("commits.ToAPIPayloadCommits failed: %v", err) return @@ -574,6 +574,7 @@ func (m *webhookNotifier) NotifyPushCommits(pusher *models.User, repo *models.Re After: opts.NewCommitID, CompareURL: setting.AppURL + commits.CompareURL, Commits: apiCommits, + HeadCommit: apiHeadCommit, Repo: convert.ToRepo(repo, models.AccessModeOwner), Pusher: apiPusher, Sender: apiPusher, @@ -790,7 +791,7 @@ func (m *webhookNotifier) NotifyDeleteRelease(doer *models.User, rel *models.Rel func (m *webhookNotifier) NotifySyncPushCommits(pusher *models.User, repo *models.Repository, opts *repository.PushUpdateOptions, commits *repository.PushCommits) { apiPusher := convert.ToUser(pusher, nil) - apiCommits, err := commits.ToAPIPayloadCommits(repo.RepoPath(), repo.HTMLURL()) + apiCommits, apiHeadCommit, err := commits.ToAPIPayloadCommits(repo.RepoPath(), repo.HTMLURL()) if err != nil { log.Error("commits.ToAPIPayloadCommits failed: %v", err) return @@ -802,6 +803,7 @@ func (m *webhookNotifier) NotifySyncPushCommits(pusher *models.User, repo *model After: opts.NewCommitID, CompareURL: setting.AppURL + commits.CompareURL, Commits: apiCommits, + HeadCommit: apiHeadCommit, Repo: convert.ToRepo(repo, models.AccessModeOwner), Pusher: apiPusher, Sender: apiPusher, diff --git a/modules/repository/commits.go b/modules/repository/commits.go index 6b67c2c26..eaaf3b8b1 100644 --- a/modules/repository/commits.go +++ b/modules/repository/commits.go @@ -28,8 +28,8 @@ type PushCommit struct { // PushCommits represents list of commits in a push operation. type PushCommits struct { - Len int Commits []*PushCommit + HeadCommit *PushCommit CompareURL string avatars map[string]string @@ -44,67 +44,88 @@ func NewPushCommits() *PushCommits { } } -// ToAPIPayloadCommits converts a PushCommits object to -// api.PayloadCommit format. -func (pc *PushCommits) ToAPIPayloadCommits(repoPath, repoLink string) ([]*api.PayloadCommit, error) { +// toAPIPayloadCommit converts a single PushCommit to an api.PayloadCommit object. +func (pc *PushCommits) toAPIPayloadCommit(repoPath, repoLink string, commit *PushCommit) (*api.PayloadCommit, error) { + var err error + authorUsername := "" + author, ok := pc.emailUsers[commit.AuthorEmail] + if !ok { + author, err = models.GetUserByEmail(commit.AuthorEmail) + if err == nil { + authorUsername = author.Name + pc.emailUsers[commit.AuthorEmail] = author + } + } else { + authorUsername = author.Name + } + + committerUsername := "" + committer, ok := pc.emailUsers[commit.CommitterEmail] + if !ok { + committer, err = models.GetUserByEmail(commit.CommitterEmail) + if err == nil { + // TODO: check errors other than email not found. + committerUsername = committer.Name + pc.emailUsers[commit.CommitterEmail] = committer + } + } else { + committerUsername = committer.Name + } + + fileStatus, err := git.GetCommitFileStatus(repoPath, commit.Sha1) + if err != nil { + return nil, fmt.Errorf("FileStatus [commit_sha1: %s]: %v", commit.Sha1, err) + } + + return &api.PayloadCommit{ + ID: commit.Sha1, + Message: commit.Message, + URL: fmt.Sprintf("%s/commit/%s", repoLink, commit.Sha1), + Author: &api.PayloadUser{ + Name: commit.AuthorName, + Email: commit.AuthorEmail, + UserName: authorUsername, + }, + Committer: &api.PayloadUser{ + Name: commit.CommitterName, + Email: commit.CommitterEmail, + UserName: committerUsername, + }, + Added: fileStatus.Added, + Removed: fileStatus.Removed, + Modified: fileStatus.Modified, + Timestamp: commit.Timestamp, + }, nil +} + +// ToAPIPayloadCommits converts a PushCommits object to api.PayloadCommit format. +// It returns all converted commits and, if provided, the head commit or an error otherwise. +func (pc *PushCommits) ToAPIPayloadCommits(repoPath, repoLink string) ([]*api.PayloadCommit, *api.PayloadCommit, error) { commits := make([]*api.PayloadCommit, len(pc.Commits)) + var headCommit *api.PayloadCommit if pc.emailUsers == nil { pc.emailUsers = make(map[string]*models.User) } - var err error for i, commit := range pc.Commits { - authorUsername := "" - author, ok := pc.emailUsers[commit.AuthorEmail] - if !ok { - author, err = models.GetUserByEmail(commit.AuthorEmail) - if err == nil { - authorUsername = author.Name - pc.emailUsers[commit.AuthorEmail] = author - } - } else { - authorUsername = author.Name - } - - committerUsername := "" - committer, ok := pc.emailUsers[commit.CommitterEmail] - if !ok { - committer, err = models.GetUserByEmail(commit.CommitterEmail) - if err == nil { - // TODO: check errors other than email not found. - committerUsername = committer.Name - pc.emailUsers[commit.CommitterEmail] = committer - } - } else { - committerUsername = committer.Name - } - - fileStatus, err := git.GetCommitFileStatus(repoPath, commit.Sha1) + apiCommit, err := pc.toAPIPayloadCommit(repoPath, repoLink, commit) if err != nil { - return nil, fmt.Errorf("FileStatus [commit_sha1: %s]: %v", commit.Sha1, err) + return nil, nil, err } - commits[i] = &api.PayloadCommit{ - ID: commit.Sha1, - Message: commit.Message, - URL: fmt.Sprintf("%s/commit/%s", repoLink, commit.Sha1), - Author: &api.PayloadUser{ - Name: commit.AuthorName, - Email: commit.AuthorEmail, - UserName: authorUsername, - }, - Committer: &api.PayloadUser{ - Name: commit.CommitterName, - Email: commit.CommitterEmail, - UserName: committerUsername, - }, - Added: fileStatus.Added, - Removed: fileStatus.Removed, - Modified: fileStatus.Modified, - Timestamp: commit.Timestamp, + commits[i] = apiCommit + if pc.HeadCommit != nil && pc.HeadCommit.Sha1 == commits[i].ID { + headCommit = apiCommit } } - return commits, nil + if pc.HeadCommit != nil && headCommit == nil { + var err error + headCommit, err = pc.toAPIPayloadCommit(repoPath, repoLink, pc.HeadCommit) + if err != nil { + return nil, nil, err + } + } + return commits, headCommit, nil } // AvatarLink tries to match user in database with e-mail @@ -157,13 +178,9 @@ func CommitToPushCommit(commit *git.Commit) *PushCommit { // ListToPushCommits transforms a list.List to PushCommits type. func ListToPushCommits(l *list.List) *PushCommits { var commits []*PushCommit - var actEmail string for e := l.Front(); e != nil; e = e.Next() { - commit := e.Value.(*git.Commit) - if actEmail == "" { - actEmail = commit.Committer.Email - } - commits = append(commits, CommitToPushCommit(commit)) + commit := CommitToPushCommit(e.Value.(*git.Commit)) + commits = append(commits, commit) } - return &PushCommits{l.Len(), commits, "", make(map[string]string), make(map[string]*models.User)} + return &PushCommits{commits, nil, "", make(map[string]string), make(map[string]*models.User)} } diff --git a/modules/repository/commits_test.go b/modules/repository/commits_test.go index a5b28ce93..8e0d8bf90 100644 --- a/modules/repository/commits_test.go +++ b/modules/repository/commits_test.go @@ -46,12 +46,13 @@ func TestPushCommits_ToAPIPayloadCommits(t *testing.T) { Message: "good signed commit", }, } - pushCommits.Len = len(pushCommits.Commits) + pushCommits.HeadCommit = &PushCommit{Sha1: "69554a6"} repo := models.AssertExistsAndLoadBean(t, &models.Repository{ID: 16}).(*models.Repository) - payloadCommits, err := pushCommits.ToAPIPayloadCommits(repo.RepoPath(), "/user2/repo16") + payloadCommits, headCommit, err := pushCommits.ToAPIPayloadCommits(repo.RepoPath(), "/user2/repo16") assert.NoError(t, err) assert.Len(t, payloadCommits, 3) + assert.NotNil(t, headCommit) assert.Equal(t, "69554a6", payloadCommits[0].ID) assert.Equal(t, "not signed commit", payloadCommits[0].Message) @@ -85,6 +86,17 @@ func TestPushCommits_ToAPIPayloadCommits(t *testing.T) { assert.EqualValues(t, []string{"readme.md"}, payloadCommits[2].Added) assert.EqualValues(t, []string{}, payloadCommits[2].Removed) assert.EqualValues(t, []string{}, payloadCommits[2].Modified) + + assert.Equal(t, "69554a6", headCommit.ID) + assert.Equal(t, "not signed commit", headCommit.Message) + assert.Equal(t, "/user2/repo16/commit/69554a6", headCommit.URL) + assert.Equal(t, "User2", headCommit.Committer.Name) + assert.Equal(t, "user2", headCommit.Committer.UserName) + assert.Equal(t, "User2", headCommit.Author.Name) + assert.Equal(t, "user2", headCommit.Author.UserName) + assert.EqualValues(t, []string{}, headCommit.Added) + assert.EqualValues(t, []string{}, headCommit.Removed) + assert.EqualValues(t, []string{"readme.md"}, headCommit.Modified) } func TestPushCommits_AvatarLink(t *testing.T) { @@ -109,7 +121,6 @@ func TestPushCommits_AvatarLink(t *testing.T) { Message: "message2", }, } - pushCommits.Len = len(pushCommits.Commits) assert.Equal(t, "https://secure.gravatar.com/avatar/ab53a2911ddf9b4817ac01ddcd3d975f?d=identicon&s=112", @@ -177,7 +188,6 @@ func TestListToPushCommits(t *testing.T) { }) pushCommits := ListToPushCommits(l) - assert.Equal(t, 2, pushCommits.Len) if assert.Len(t, pushCommits.Commits, 2) { assert.Equal(t, "Message1", pushCommits.Commits[0].Message) assert.Equal(t, hexString1, pushCommits.Commits[0].Sha1) diff --git a/routers/api/v1/repo/hook.go b/routers/api/v1/repo/hook.go index 5a0911544..da0a2c501 100644 --- a/routers/api/v1/repo/hook.go +++ b/routers/api/v1/repo/hook.go @@ -140,16 +140,17 @@ func TestHook(ctx *context.APIContext) { return } + commit := convert.ToPayloadCommit(ctx.Repo.Repository, ctx.Repo.Commit) + if err := webhook.PrepareWebhook(hook, ctx.Repo.Repository, models.HookEventPush, &api.PushPayload{ - Ref: git.BranchPrefix + ctx.Repo.Repository.DefaultBranch, - Before: ctx.Repo.Commit.ID.String(), - After: ctx.Repo.Commit.ID.String(), - Commits: []*api.PayloadCommit{ - convert.ToPayloadCommit(ctx.Repo.Repository, ctx.Repo.Commit), - }, - Repo: convert.ToRepo(ctx.Repo.Repository, models.AccessModeNone), - Pusher: convert.ToUserWithAccessMode(ctx.User, models.AccessModeNone), - Sender: convert.ToUserWithAccessMode(ctx.User, models.AccessModeNone), + Ref: git.BranchPrefix + ctx.Repo.Repository.DefaultBranch, + Before: ctx.Repo.Commit.ID.String(), + After: ctx.Repo.Commit.ID.String(), + Commits: []*api.PayloadCommit{commit}, + HeadCommit: commit, + Repo: convert.ToRepo(ctx.Repo.Repository, models.AccessModeNone), + Pusher: convert.ToUserWithAccessMode(ctx.User, models.AccessModeNone), + Sender: convert.ToUserWithAccessMode(ctx.User, models.AccessModeNone), }); err != nil { ctx.Error(http.StatusInternalServerError, "PrepareWebhook: ", err) return diff --git a/routers/web/repo/webhook.go b/routers/web/repo/webhook.go index ab254dbe4..e8d86db51 100644 --- a/routers/web/repo/webhook.go +++ b/routers/web/repo/webhook.go @@ -1085,28 +1085,30 @@ func TestWebhook(ctx *context.Context) { } apiUser := convert.ToUserWithAccessMode(ctx.User, models.AccessModeNone) - p := &api.PushPayload{ - Ref: git.BranchPrefix + ctx.Repo.Repository.DefaultBranch, - Before: commit.ID.String(), - After: commit.ID.String(), - Commits: []*api.PayloadCommit{ - { - ID: commit.ID.String(), - Message: commit.Message(), - URL: ctx.Repo.Repository.HTMLURL() + "/commit/" + commit.ID.String(), - Author: &api.PayloadUser{ - Name: commit.Author.Name, - Email: commit.Author.Email, - }, - Committer: &api.PayloadUser{ - Name: commit.Committer.Name, - Email: commit.Committer.Email, - }, - }, + + apiCommit := &api.PayloadCommit{ + ID: commit.ID.String(), + Message: commit.Message(), + URL: ctx.Repo.Repository.HTMLURL() + "/commit/" + commit.ID.String(), + Author: &api.PayloadUser{ + Name: commit.Author.Name, + Email: commit.Author.Email, }, - Repo: convert.ToRepo(ctx.Repo.Repository, models.AccessModeNone), - Pusher: apiUser, - Sender: apiUser, + Committer: &api.PayloadUser{ + Name: commit.Committer.Name, + Email: commit.Committer.Email, + }, + } + + p := &api.PushPayload{ + Ref: git.BranchPrefix + ctx.Repo.Repository.DefaultBranch, + Before: commit.ID.String(), + After: commit.ID.String(), + Commits: []*api.PayloadCommit{apiCommit}, + HeadCommit: apiCommit, + Repo: convert.ToRepo(ctx.Repo.Repository, models.AccessModeNone), + Pusher: apiUser, + Sender: apiUser, } if err := webhook.PrepareWebhook(w, ctx.Repo.Repository, models.HookEventPush, p); err != nil { ctx.Flash.Error("PrepareWebhook: " + err.Error()) diff --git a/services/repository/push.go b/services/repository/push.go index dcb3bc779..26df6b8e4 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -95,7 +95,6 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { if opts.IsNewRef() && opts.IsDelRef() { return fmt.Errorf("Old and new revisions are both %s", git.EmptySHA) } - var commits = &repo_module.PushCommits{} if opts.IsTag() { // If is tag reference if pusher == nil || pusher.ID != opts.PusherID { var err error @@ -192,7 +191,8 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { } } - commits = repo_module.ListToPushCommits(l) + commits := repo_module.ListToPushCommits(l) + commits.HeadCommit = repo_module.CommitToPushCommit(newCommit) if err := repofiles.UpdateIssuesCommit(pusher, repo, commits.Commits, refName); err != nil { log.Error("updateIssuesCommit: %v", err) diff --git a/templates/user/dashboard/feeds.tmpl b/templates/user/dashboard/feeds.tmpl index 057a4a762..6ed75ee14 100644 --- a/templates/user/dashboard/feeds.tmpl +++ b/templates/user/dashboard/feeds.tmpl @@ -101,7 +101,7 @@ {{end}} {{end}} - {{if and (gt $push.Len 1) $push.CompareURL}}
  • {{$.i18n.Tr "action.compare_commits" $push.Len}} »
    • {{end}} + {{if and (gt (len $push.Commits) 1) $push.CompareURL}}
  • {{$.i18n.Tr "action.compare_commits" (len $push.Commits)}} »
    • {{end}} {{else if eq .GetOpType 6}}