From e9bb75d8d18ba88dc26e4c4af6dc66812a9b8fb6 Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Sun, 11 Aug 2019 22:31:18 +0200 Subject: [PATCH] Fix duplicate call of webhook (#7821) --- integrations/pull_merge_test.go | 34 +++++++++++++++++++++++++++++++++ models/webhook.go | 1 - modules/pull/merge.go | 33 -------------------------------- modules/repofiles/delete.go | 26 ------------------------- modules/repofiles/update.go | 26 ------------------------- modules/repofiles/upload.go | 27 -------------------------- 6 files changed, 34 insertions(+), 113 deletions(-) diff --git a/integrations/pull_merge_test.go b/integrations/pull_merge_test.go index f3efa63b0..2f4e48f29 100644 --- a/integrations/pull_merge_test.go +++ b/integrations/pull_merge_test.go @@ -54,6 +54,10 @@ func testPullCleanUp(t *testing.T, session *TestSession, user, repo, pullnum str func TestPullMerge(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + hookTasks, err := models.HookTasks(1, 1) //Retrieve previous hook number + assert.NoError(t, err) + hookTasksLenBefore := len(hookTasks) + session := loginUser(t, "user1") testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") @@ -63,11 +67,19 @@ func TestPullMerge(t *testing.T) { elem := strings.Split(test.RedirectURL(resp), "/") assert.EqualValues(t, "pulls", elem[3]) testPullMerge(t, session, elem[1], elem[2], elem[4], models.MergeStyleMerge) + + hookTasks, err = models.HookTasks(1, 1) + assert.NoError(t, err) + assert.Len(t, hookTasks, hookTasksLenBefore+1) }) } func TestPullRebase(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + hookTasks, err := models.HookTasks(1, 1) //Retrieve previous hook number + assert.NoError(t, err) + hookTasksLenBefore := len(hookTasks) + session := loginUser(t, "user1") testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") @@ -77,12 +89,21 @@ func TestPullRebase(t *testing.T) { elem := strings.Split(test.RedirectURL(resp), "/") assert.EqualValues(t, "pulls", elem[3]) testPullMerge(t, session, elem[1], elem[2], elem[4], models.MergeStyleRebase) + + hookTasks, err = models.HookTasks(1, 1) + assert.NoError(t, err) + assert.Len(t, hookTasks, hookTasksLenBefore+1) }) } func TestPullRebaseMerge(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { prepareTestEnv(t) + + hookTasks, err := models.HookTasks(1, 1) //Retrieve previous hook number + assert.NoError(t, err) + hookTasksLenBefore := len(hookTasks) + session := loginUser(t, "user1") testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") @@ -92,12 +113,21 @@ func TestPullRebaseMerge(t *testing.T) { elem := strings.Split(test.RedirectURL(resp), "/") assert.EqualValues(t, "pulls", elem[3]) testPullMerge(t, session, elem[1], elem[2], elem[4], models.MergeStyleRebaseMerge) + + hookTasks, err = models.HookTasks(1, 1) + assert.NoError(t, err) + assert.Len(t, hookTasks, hookTasksLenBefore+1) }) } func TestPullSquash(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { prepareTestEnv(t) + + hookTasks, err := models.HookTasks(1, 1) //Retrieve previous hook number + assert.NoError(t, err) + hookTasksLenBefore := len(hookTasks) + session := loginUser(t, "user1") testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") @@ -108,6 +138,10 @@ func TestPullSquash(t *testing.T) { elem := strings.Split(test.RedirectURL(resp), "/") assert.EqualValues(t, "pulls", elem[3]) testPullMerge(t, session, elem[1], elem[2], elem[4], models.MergeStyleSquash) + + hookTasks, err = models.HookTasks(1, 1) + assert.NoError(t, err) + assert.Len(t, hookTasks, hookTasksLenBefore+1) }) } diff --git a/models/webhook.go b/models/webhook.go index e3e11e596..ac39501ca 100644 --- a/models/webhook.go +++ b/models/webhook.go @@ -890,7 +890,6 @@ func DeliverHooks() { for _, t := range tasks { if err = t.deliver(); err != nil { log.Error("deliver: %v", err) - continue } } diff --git a/modules/pull/merge.go b/modules/pull/merge.go index ed2fe4837..cf2fb7fc4 100644 --- a/modules/pull/merge.go +++ b/modules/pull/merge.go @@ -292,39 +292,6 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor go models.HookQueue.Add(pr.Issue.Repo.ID) } - l, err := baseGitRepo.CommitsBetweenIDs(pr.MergedCommitID, pr.MergeBase) - if err != nil { - log.Error("CommitsBetweenIDs: %v", err) - return nil - } - - // It is possible that head branch is not fully sync with base branch for merge commits, - // so we need to get latest head commit and append merge commit manually - // to avoid strange diff commits produced. - mergeCommit, err := baseGitRepo.GetBranchCommit(pr.BaseBranch) - if err != nil { - log.Error("GetBranchCommit: %v", err) - return nil - } - if mergeStyle == models.MergeStyleMerge { - l.PushFront(mergeCommit) - } - - p := &api.PushPayload{ - Ref: git.BranchPrefix + pr.BaseBranch, - Before: pr.MergeBase, - After: mergeCommit.ID.String(), - CompareURL: setting.AppURL + pr.BaseRepo.ComposeCompareURL(pr.MergeBase, pr.MergedCommitID), - Commits: models.ListToPushCommits(l).ToAPIPayloadCommits(pr.BaseRepo.HTMLURL()), - Repo: pr.BaseRepo.APIFormat(mode), - Pusher: pr.HeadRepo.MustOwner().APIFormat(), - Sender: doer.APIFormat(), - } - if err = models.PrepareWebhooks(pr.BaseRepo, models.HookEventPush, p); err != nil { - log.Error("PrepareWebhooks: %v", err) - } else { - go models.HookQueue.Add(pr.BaseRepo.ID) - } return nil } diff --git a/modules/repofiles/delete.go b/modules/repofiles/delete.go index a8ab277b2..2210faae6 100644 --- a/modules/repofiles/delete.go +++ b/modules/repofiles/delete.go @@ -178,32 +178,6 @@ func DeleteRepoFile(repo *models.Repository, doer *models.User, opts *DeleteRepo return nil, err } - // Simulate push event. - oldCommitID := opts.LastCommitID - if opts.NewBranch != opts.OldBranch { - oldCommitID = git.EmptySHA - } - - if err = repo.GetOwner(); err != nil { - return nil, fmt.Errorf("GetOwner: %v", err) - } - err = PushUpdate( - repo, - opts.NewBranch, - models.PushUpdateOptions{ - PusherID: doer.ID, - PusherName: doer.Name, - RepoUserName: repo.Owner.Name, - RepoName: repo.Name, - RefFullName: git.BranchPrefix + opts.NewBranch, - OldCommitID: oldCommitID, - NewCommitID: commitHash, - }, - ) - if err != nil { - return nil, fmt.Errorf("PushUpdate: %v", err) - } - commit, err = t.GetCommit(commitHash) if err != nil { return nil, err diff --git a/modules/repofiles/update.go b/modules/repofiles/update.go index 26b5113f1..62e2afbc9 100644 --- a/modules/repofiles/update.go +++ b/modules/repofiles/update.go @@ -396,32 +396,6 @@ func CreateOrUpdateRepoFile(repo *models.Repository, doer *models.User, opts *Up return nil, err } - // Simulate push event. - oldCommitID := opts.LastCommitID - if opts.NewBranch != opts.OldBranch || oldCommitID == "" { - oldCommitID = git.EmptySHA - } - - if err = repo.GetOwner(); err != nil { - return nil, fmt.Errorf("GetOwner: %v", err) - } - err = PushUpdate( - repo, - opts.NewBranch, - models.PushUpdateOptions{ - PusherID: doer.ID, - PusherName: doer.Name, - RepoUserName: repo.Owner.Name, - RepoName: repo.Name, - RefFullName: git.BranchPrefix + opts.NewBranch, - OldCommitID: oldCommitID, - NewCommitID: commitHash, - }, - ) - if err != nil { - return nil, fmt.Errorf("PushUpdate: %v", err) - } - commit, err = t.GetCommit(commitHash) if err != nil { return nil, err diff --git a/modules/repofiles/upload.go b/modules/repofiles/upload.go index 2da101c64..f2ffec7eb 100644 --- a/modules/repofiles/upload.go +++ b/modules/repofiles/upload.go @@ -11,7 +11,6 @@ import ( "strings" "code.gitea.io/gitea/models" - "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/lfs" "code.gitea.io/gitea/modules/setting" ) @@ -177,31 +176,5 @@ func UploadRepoFiles(repo *models.Repository, doer *models.User, opts *UploadRep return err } - // Simulate push event. - oldCommitID := opts.LastCommitID - if opts.NewBranch != opts.OldBranch { - oldCommitID = git.EmptySHA - } - - if err = repo.GetOwner(); err != nil { - return fmt.Errorf("GetOwner: %v", err) - } - err = PushUpdate( - repo, - opts.NewBranch, - models.PushUpdateOptions{ - PusherID: doer.ID, - PusherName: doer.Name, - RepoUserName: repo.Owner.Name, - RepoName: repo.Name, - RefFullName: git.BranchPrefix + opts.NewBranch, - OldCommitID: oldCommitID, - NewCommitID: commitHash, - }, - ) - if err != nil { - return fmt.Errorf("PushUpdate: %v", err) - } - return models.DeleteUploads(uploads...) }