From 29fbed20e124a90403f4c92863b1f01ae8b3f313 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 8 Apr 2021 20:43:38 +0100 Subject: [PATCH 1/6] Attempt to get diff twice Signed-off-by: Andrew Thornton --- .../api_helper_for_declarative_test.go | 20 +++++++++++++ integrations/git_test.go | 28 +++++++++++++++---- modules/queue/manager.go | 8 +++++- 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/integrations/api_helper_for_declarative_test.go b/integrations/api_helper_for_declarative_test.go index 9399924b5e60d..d56ae5d27c145 100644 --- a/integrations/api_helper_for_declarative_test.go +++ b/integrations/api_helper_for_declarative_test.go @@ -239,6 +239,26 @@ func doAPICreatePullRequest(ctx APITestContext, owner, repo, baseBranch, headBra } } +func doAPIGetPullRequest(ctx APITestContext, owner, repo string, index int64) func(*testing.T) (api.PullRequest, error) { + return func(t *testing.T) (api.PullRequest, error) { + urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d?token=%s", + owner, repo, index, ctx.Token) + req := NewRequest(t, http.MethodGet, urlStr) + + expected := 200 + if ctx.ExpectedCode != 0 { + expected = ctx.ExpectedCode + } + resp := ctx.Session.MakeRequest(t, req, expected) + + json := jsoniter.ConfigCompatibleWithStandardLibrary + decoder := json.NewDecoder(resp.Body) + pr := api.PullRequest{} + err := decoder.Decode(&pr) + return pr, err + } +} + func doAPIMergePullRequest(ctx APITestContext, owner, repo string, index int64) func(*testing.T) { return func(t *testing.T) { urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/merge?token=%s", diff --git a/integrations/git_test.go b/integrations/git_test.go index aa1b3ee2accf2..c90ff5b367683 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -5,11 +5,13 @@ package integrations import ( + "encoding/hex" "fmt" "io/ioutil" "math/rand" "net/http" "net/url" + "os" "path" "path/filepath" "strconv" @@ -452,26 +454,41 @@ func doMergeFork(ctx, baseCtx APITestContext, baseBranch, headBranch string) fun // Then get the diff string var diffHash string + var diffLength int t.Run("GetDiff", func(t *testing.T) { req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) resp := ctx.Session.MakeRequestNilResponseHashSumRecorder(t, req, http.StatusOK) diffHash = string(resp.Hash.Sum(nil)) + diffLength = resp.Length + if diffLength == 0 { + fmt.Fprintf(os.Stdout, "Had to request diff twice due to 0 bytes\n") + req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) + resp := ctx.Session.MakeRequestNilResponseHashSumRecorder(t, req, http.StatusOK) + diffHash = string(resp.Hash.Sum(nil)) + diffLength = resp.Length + } }) // Now: Merge the PR & make sure that doesn't break the PR page or change its diff t.Run("MergePR", doAPIMergePullRequest(baseCtx, baseCtx.Username, baseCtx.Reponame, pr.Index)) t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr)) - t.Run("EnsureDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffHash)) + t.Run("CheckPR", func(t *testing.T) { + oldMergeBase := pr.MergeBase + pr2, err := doAPIGetPullRequest(baseCtx, baseCtx.Username, baseCtx.Reponame, pr.Index)(t) + assert.NoError(t, err) + assert.Equal(t, oldMergeBase, pr2.MergeBase) + }) + t.Run("EnsurDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffHash, diffLength)) // Then: Delete the head branch & make sure that doesn't break the PR page or change its diff t.Run("DeleteHeadBranch", doBranchDelete(baseCtx, baseCtx.Username, baseCtx.Reponame, headBranch)) t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr)) - t.Run("EnsureDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffHash)) + t.Run("EnsureDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffHash, diffLength)) // Delete the head repository & make sure that doesn't break the PR page or change its diff t.Run("DeleteHeadRepository", doAPIDeleteRepository(ctx)) t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr)) - t.Run("EnsureDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffHash)) + t.Run("EnsureDiffNoChange", doEnsureDiffNoChange(baseCtx, pr, diffHash, diffLength)) } } @@ -515,14 +532,15 @@ func doEnsureCanSeePull(ctx APITestContext, pr api.PullRequest) func(t *testing. } } -func doEnsureDiffNoChange(ctx APITestContext, pr api.PullRequest, diffHash string) func(t *testing.T) { +func doEnsureDiffNoChange(ctx APITestContext, pr api.PullRequest, diffHash string, diffLength int) func(t *testing.T) { return func(t *testing.T) { req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(ctx.Username), url.PathEscape(ctx.Reponame), pr.Index)) resp := ctx.Session.MakeRequestNilResponseHashSumRecorder(t, req, http.StatusOK) actual := string(resp.Hash.Sum(nil)) + actualLength := resp.Length equal := diffHash == actual - assert.True(t, equal, "Unexpected change in the diff string: expected hash: %s but was actually: %s", diffHash, actual) + assert.True(t, equal, "Unexpected change in the diff string: expected hash: %s size: %d but was actually: %s size: %d", hex.EncodeToString([]byte(diffHash)), diffLength, hex.EncodeToString([]byte(actual)), actualLength) } } diff --git a/modules/queue/manager.go b/modules/queue/manager.go index d44007a0f007e..da0fc606e6e16 100644 --- a/modules/queue/manager.go +++ b/modules/queue/manager.go @@ -174,6 +174,7 @@ func (m *Manager) FlushAll(baseCtx context.Context, timeout time.Duration) error default: } mqs := m.ManagedQueues() + log.Debug("Found %d Managed Queues", len(mqs)) wg := sync.WaitGroup{} wg.Add(len(mqs)) allEmpty := true @@ -184,6 +185,7 @@ func (m *Manager) FlushAll(baseCtx context.Context, timeout time.Duration) error } allEmpty = false if flushable, ok := mq.Managed.(Flushable); ok { + log.Debug("Flushing (flushable) queue: %s", mq.Name) go func(q *ManagedQueue) { localCtx, localCancel := context.WithCancel(ctx) pid := q.RegisterWorkers(1, start, hasTimeout, end, localCancel, true) @@ -196,7 +198,11 @@ func (m *Manager) FlushAll(baseCtx context.Context, timeout time.Duration) error wg.Done() }(mq) } else { - wg.Done() + log.Debug("Queue: %s is non-empty but is not flushable - adding 100 millisecond wait", mq.Name) + go func() { + <-time.After(100 * time.Millisecond) + wg.Done() + }() } } From dccb72d31561b6226c177e49bfbe6e0e15996aa7 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 9 Apr 2021 17:31:03 +0100 Subject: [PATCH 2/6] Turn off autodetect manual merge Signed-off-by: Andrew Thornton --- integrations/git_test.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/integrations/git_test.go b/integrations/git_test.go index c90ff5b367683..669ebd25389e2 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -11,7 +11,6 @@ import ( "math/rand" "net/http" "net/url" - "os" "path" "path/filepath" "strconv" @@ -460,15 +459,17 @@ func doMergeFork(ctx, baseCtx APITestContext, baseBranch, headBranch string) fun resp := ctx.Session.MakeRequestNilResponseHashSumRecorder(t, req, http.StatusOK) diffHash = string(resp.Hash.Sum(nil)) diffLength = resp.Length - if diffLength == 0 { - fmt.Fprintf(os.Stdout, "Had to request diff twice due to 0 bytes\n") - req := NewRequest(t, "GET", fmt.Sprintf("/%s/%s/pulls/%d.diff", url.PathEscape(baseCtx.Username), url.PathEscape(baseCtx.Reponame), pr.Index)) - resp := ctx.Session.MakeRequestNilResponseHashSumRecorder(t, req, http.StatusOK) - diffHash = string(resp.Hash.Sum(nil)) - diffLength = resp.Length - } }) + trueBool := true + falseBool := false + + t.Run("AllowSetManuallyMergedAndSwitchOffAutodetectManualMerge", doAPIEditRepository(baseCtx, &api.EditRepoOption{ + HasPullRequests: &trueBool, + AllowManualMerge: &trueBool, + AutodetectManualMerge: &falseBool, + })) + // Now: Merge the PR & make sure that doesn't break the PR page or change its diff t.Run("MergePR", doAPIMergePullRequest(baseCtx, baseCtx.Username, baseCtx.Reponame, pr.Index)) t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr)) From b71b1c700e670702b9c964dc1708ee2e54bfbffc Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 9 Apr 2021 18:33:00 +0100 Subject: [PATCH 3/6] Attempt again... Signed-off-by: Andrew Thornton --- integrations/git_test.go | 18 +++++++++--------- services/pull/check.go | 1 + services/pull/merge.go | 5 ++++- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/integrations/git_test.go b/integrations/git_test.go index 669ebd25389e2..540858a79dc30 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -442,6 +442,15 @@ func doMergeFork(ctx, baseCtx APITestContext, baseBranch, headBranch string) fun var pr api.PullRequest var err error + trueBool := true + falseBool := false + + t.Run("AllowSetManuallyMergedAndSwitchOffAutodetectManualMerge", doAPIEditRepository(baseCtx, &api.EditRepoOption{ + HasPullRequests: &trueBool, + AllowManualMerge: &trueBool, + AutodetectManualMerge: &falseBool, + })) + // Create a test pullrequest t.Run("CreatePullRequest", func(t *testing.T) { pr, err = doAPICreatePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, baseBranch, headBranch)(t) @@ -461,15 +470,6 @@ func doMergeFork(ctx, baseCtx APITestContext, baseBranch, headBranch string) fun diffLength = resp.Length }) - trueBool := true - falseBool := false - - t.Run("AllowSetManuallyMergedAndSwitchOffAutodetectManualMerge", doAPIEditRepository(baseCtx, &api.EditRepoOption{ - HasPullRequests: &trueBool, - AllowManualMerge: &trueBool, - AutodetectManualMerge: &falseBool, - })) - // Now: Merge the PR & make sure that doesn't break the PR page or change its diff t.Run("MergePR", doAPIMergePullRequest(baseCtx, baseCtx.Username, baseCtx.Reponame, pr.Index)) t.Run("EnsureCanSeePull", doEnsureCanSeePull(baseCtx, pr)) diff --git a/services/pull/check.go b/services/pull/check.go index 3ec76de5e8748..78a249a0e090c 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -180,6 +180,7 @@ func manuallyMerged(pr *models.PullRequest) bool { log.Error("PullRequest[%d].setMerged : %v", pr.ID, err) return false } else if !merged { + log.Info("Setting pr #%d in %-v as merged with MergeCommitID %s. Success: %t", pr.Index, pr.BaseRepo, pr.MergedCommitID, merged) return false } diff --git a/services/pull/merge.go b/services/pull/merge.go index 518ffa48492a7..343befca58d33 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -64,8 +64,10 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor pr.Merger = doer pr.MergerID = doer.ID - if _, err = pr.SetMerged(); err != nil { + if success, err := pr.SetMerged(); err != nil { log.Error("setMerged [%d]: %v", pr.ID, err) + } else { + log.Info("Setting pr #%d in %-v as merged with MergeCommitID %s. Success: %t", pr.Index, pr.BaseRepo, pr.MergedCommitID, success) } if err := pr.LoadIssue(); err != nil { @@ -659,6 +661,7 @@ func MergedManually(pr *models.PullRequest, doer *models.User, baseGitRepo *git. if merged, err = pr.SetMerged(); err != nil { return } else if !merged { + log.Info("Setting pr #%d in %-v as merged with MergeCommitID %s. Success: %t", pr.Index, pr.BaseRepo, pr.MergedCommitID, merged) return fmt.Errorf("SetMerged failed") } From bc144b61a61f6eaf8039cdfbef075b78c2820149 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 9 Apr 2021 19:29:27 +0100 Subject: [PATCH 4/6] Ensure merge_base is updated at time of merge Signed-off-by: Andrew Thornton --- models/pull.go | 2 +- services/pull/check.go | 1 - services/pull/merge.go | 5 +---- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/models/pull.go b/models/pull.go index 47e699e192418..182e63fb6d3d5 100644 --- a/models/pull.go +++ b/models/pull.go @@ -406,7 +406,7 @@ func (pr *PullRequest) SetMerged() (bool, error) { return false, fmt.Errorf("Issue.changeStatus: %v", err) } - if _, err := sess.Where("id = ?", pr.ID).Cols("has_merged, status, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil { + if _, err := sess.Where("id = ?", pr.ID).Cols("has_merged, status, merge_base, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil { return false, fmt.Errorf("Failed to update pr[%d]: %v", pr.ID, err) } diff --git a/services/pull/check.go b/services/pull/check.go index 78a249a0e090c..3ec76de5e8748 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -180,7 +180,6 @@ func manuallyMerged(pr *models.PullRequest) bool { log.Error("PullRequest[%d].setMerged : %v", pr.ID, err) return false } else if !merged { - log.Info("Setting pr #%d in %-v as merged with MergeCommitID %s. Success: %t", pr.Index, pr.BaseRepo, pr.MergedCommitID, merged) return false } diff --git a/services/pull/merge.go b/services/pull/merge.go index 343befca58d33..7e6a214b87d92 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -64,10 +64,8 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor pr.Merger = doer pr.MergerID = doer.ID - if success, err := pr.SetMerged(); err != nil { + if _, err := pr.SetMerged(); err != nil { log.Error("setMerged [%d]: %v", pr.ID, err) - } else { - log.Info("Setting pr #%d in %-v as merged with MergeCommitID %s. Success: %t", pr.Index, pr.BaseRepo, pr.MergedCommitID, success) } if err := pr.LoadIssue(); err != nil { @@ -661,7 +659,6 @@ func MergedManually(pr *models.PullRequest, doer *models.User, baseGitRepo *git. if merged, err = pr.SetMerged(); err != nil { return } else if !merged { - log.Info("Setting pr #%d in %-v as merged with MergeCommitID %s. Success: %t", pr.Index, pr.BaseRepo, pr.MergedCommitID, merged) return fmt.Errorf("SetMerged failed") } From 1488f79fccdb4cbbf6af8c174820e538a2367354 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 9 Apr 2021 20:13:49 +0100 Subject: [PATCH 5/6] remove manual merge switch off Signed-off-by: Andrew Thornton --- integrations/git_test.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/integrations/git_test.go b/integrations/git_test.go index 540858a79dc30..13a60076a7e9e 100644 --- a/integrations/git_test.go +++ b/integrations/git_test.go @@ -442,15 +442,6 @@ func doMergeFork(ctx, baseCtx APITestContext, baseBranch, headBranch string) fun var pr api.PullRequest var err error - trueBool := true - falseBool := false - - t.Run("AllowSetManuallyMergedAndSwitchOffAutodetectManualMerge", doAPIEditRepository(baseCtx, &api.EditRepoOption{ - HasPullRequests: &trueBool, - AllowManualMerge: &trueBool, - AutodetectManualMerge: &falseBool, - })) - // Create a test pullrequest t.Run("CreatePullRequest", func(t *testing.T) { pr, err = doAPICreatePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, baseBranch, headBranch)(t) From b95e835ab72985471b6d2fd92f650540d98ce345 Mon Sep 17 00:00:00 2001 From: zeripath Date: Sat, 10 Apr 2021 07:07:10 +0100 Subject: [PATCH 6/6] Update models/pull.go --- models/pull.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/pull.go b/models/pull.go index 182e63fb6d3d5..133f196aaed49 100644 --- a/models/pull.go +++ b/models/pull.go @@ -406,6 +406,7 @@ func (pr *PullRequest) SetMerged() (bool, error) { return false, fmt.Errorf("Issue.changeStatus: %v", err) } + // We need to save all of the data used to compute this merge as it may have already been changed by TestPatch. FIXME: need to set some state to prevent TestPatch from running whilst we are merging. if _, err := sess.Where("id = ?", pr.ID).Cols("has_merged, status, merge_base, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil { return false, fmt.Errorf("Failed to update pr[%d]: %v", pr.ID, err) }