From 09adc26eb63fed671ce45499365139fac092c224 Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 22 Apr 2022 01:11:42 +0000 Subject: [PATCH] Set correct PR status on 3way on conflict checking (#19457) (#19458) - Backport #19457 - When 3-way merge is enabled for conflict checking, it has a new interesting behavior that it doesn't return any error when it found a conflict, so we change the condition to not check for the error, but instead check if conflictedfiles is populated, this fixes a issue whereby PR status wasn't correctly on conflicted PR's. - Refactor the mergeable property(which was incorrectly set and lead me this bug) to be more maintainable. - Add a dedicated test for conflicting checking, so it should prevent future issues with this. - Ref: Fix the latest error for https://gitea.com/gitea/go-sdk/pulls/579 Co-authored-by: zeripath --- integrations/pull_merge_test.go | 81 +++++++++++++++++++++++++++++++-- models/pull.go | 11 +++++ modules/convert/pull.go | 5 +- services/pull/patch.go | 6 ++- 4 files changed, 93 insertions(+), 10 deletions(-) diff --git a/integrations/pull_merge_test.go b/integrations/pull_merge_test.go index a0b87eeee8..7646d9f412 100644 --- a/integrations/pull_merge_test.go +++ b/integrations/pull_merge_test.go @@ -25,6 +25,8 @@ import ( api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/services/pull" + repo_service "code.gitea.io/gitea/services/repository" + files_service "code.gitea.io/gitea/services/repository/files" "github.com/stretchr/testify/assert" "github.com/unknwon/i18n" @@ -65,7 +67,7 @@ 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 := webhook.HookTasks(1, 1) //Retrieve previous hook number + hookTasks, err := webhook.HookTasks(1, 1) // Retrieve previous hook number assert.NoError(t, err) hookTasksLenBefore := len(hookTasks) @@ -87,7 +89,7 @@ func TestPullMerge(t *testing.T) { func TestPullRebase(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - hookTasks, err := webhook.HookTasks(1, 1) //Retrieve previous hook number + hookTasks, err := webhook.HookTasks(1, 1) // Retrieve previous hook number assert.NoError(t, err) hookTasksLenBefore := len(hookTasks) @@ -109,7 +111,7 @@ func TestPullRebase(t *testing.T) { func TestPullRebaseMerge(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - hookTasks, err := webhook.HookTasks(1, 1) //Retrieve previous hook number + hookTasks, err := webhook.HookTasks(1, 1) // Retrieve previous hook number assert.NoError(t, err) hookTasksLenBefore := len(hookTasks) @@ -131,7 +133,7 @@ func TestPullRebaseMerge(t *testing.T) { func TestPullSquash(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - hookTasks, err := webhook.HookTasks(1, 1) //Retrieve previous hook number + hookTasks, err := webhook.HookTasks(1, 1) // Retrieve previous hook number assert.NoError(t, err) hookTasksLenBefore := len(hookTasks) @@ -335,3 +337,74 @@ func TestCantMergeUnrelated(t *testing.T) { gitRepo.Close() }) } + +func TestConflictChecking(t *testing.T) { + onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).(*user_model.User) + + // Create new clean repo to test conflict checking. + baseRepo, err := repo_service.CreateRepository(user, user, models.CreateRepoOptions{ + Name: "conflict-checking", + Description: "Tempo repo", + AutoInit: true, + Readme: "Default", + DefaultBranch: "main", + }) + assert.NoError(t, err) + assert.NotEmpty(t, baseRepo) + + // create a commit on new branch. + _, err = files_service.CreateOrUpdateRepoFile(baseRepo, user, &files_service.UpdateRepoFileOptions{ + TreePath: "important_file", + Message: "Add a important file", + Content: "Just a non-important file", + IsNewFile: true, + OldBranch: "main", + NewBranch: "important-secrets", + }) + assert.NoError(t, err) + + // create a commit on main branch. + _, err = files_service.CreateOrUpdateRepoFile(baseRepo, user, &files_service.UpdateRepoFileOptions{ + TreePath: "important_file", + Message: "Add a important file", + Content: "Not the same content :P", + IsNewFile: true, + OldBranch: "main", + NewBranch: "main", + }) + assert.NoError(t, err) + + // create Pull to merge the important-secrets branch into main branch. + pullIssue := &models.Issue{ + RepoID: baseRepo.ID, + Title: "PR with conflict!", + PosterID: user.ID, + Poster: user, + IsPull: true, + } + + pullRequest := &models.PullRequest{ + HeadRepoID: baseRepo.ID, + BaseRepoID: baseRepo.ID, + HeadBranch: "important-secrets", + BaseBranch: "main", + HeadRepo: baseRepo, + BaseRepo: baseRepo, + Type: models.PullRequestGitea, + } + err = pull.NewPullRequest(baseRepo, pullIssue, nil, nil, pullRequest, nil) + assert.NoError(t, err) + + issue := unittest.AssertExistsAndLoadBean(t, &models.Issue{Title: "PR with conflict!"}).(*models.Issue) + conflictingPR, err := models.GetPullRequestByIssueID(issue.ID) + assert.NoError(t, err) + + // Ensure conflictedFiles is populated. + assert.Equal(t, 1, len(conflictingPR.ConflictedFiles)) + // Check if status is correct. + assert.Equal(t, models.PullRequestStatusConflict, conflictingPR.Status) + // Ensure that mergeable returns false + assert.False(t, conflictingPR.Mergeable()) + }) +} diff --git a/models/pull.go b/models/pull.go index 0511d29dfc..4b2b07cdef 100644 --- a/models/pull.go +++ b/models/pull.go @@ -697,3 +697,14 @@ func (pr *PullRequest) GetHeadBranchHTMLURL() string { } return pr.HeadRepo.HTMLURL() + "/src/branch/" + util.PathEscapeSegments(pr.HeadBranch) } + +// Mergeable returns if the pullrequest is mergeable. +func (pr *PullRequest) Mergeable() bool { + // If a pull request isn't mergable if it's: + // - Being conflict checked. + // - Has a conflict. + // - Received a error while being conflict checked. + // - Is a work-in-progress pull request. + return pr.Status != PullRequestStatusChecking && pr.Status != PullRequestStatusConflict && + pr.Status != PullRequestStatusError && !pr.IsWorkInProgress() +} diff --git a/modules/convert/pull.go b/modules/convert/pull.go index 2e2c1b85bc..ad757dbe5f 100644 --- a/modules/convert/pull.go +++ b/modules/convert/pull.go @@ -67,6 +67,7 @@ func ToAPIPullRequest(pr *models.PullRequest, doer *user_model.User) *api.PullRe PatchURL: pr.Issue.PatchURL(), HasMerged: pr.HasMerged, MergeBase: pr.MergeBase, + Mergeable: pr.Mergeable(), Deadline: apiIssue.Deadline, Created: pr.Issue.CreatedUnix.AsTimePtr(), Updated: pr.Issue.UpdatedUnix.AsTimePtr(), @@ -190,10 +191,6 @@ func ToAPIPullRequest(pr *models.PullRequest, doer *user_model.User) *api.PullRe } } - if pr.Status != models.PullRequestStatusChecking { - mergeable := !(pr.Status == models.PullRequestStatusConflict || pr.Status == models.PullRequestStatusError) && !pr.IsWorkInProgress() - apiPullRequest.Mergeable = mergeable - } if pr.HasMerged { apiPullRequest.Merged = pr.MergedUnix.AsTimePtr() apiPullRequest.MergedCommitID = &pr.MergedCommitID diff --git a/services/pull/patch.go b/services/pull/patch.go index ee10c97392..e2851709e0 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -431,14 +431,16 @@ func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath return nil }) - // 8. If there is a conflict the `git apply` command will return a non-zero error code - so there will be a positive error. - if err != nil { + // 9. Check if the found conflictedfiles is non-zero, "err" could be non-nil, so we should ignore it if we found conflicts. + // Note: `"err" could be non-nil` is due that if enable 3-way merge, it doesn't return any error on found conflicts. + if len(pr.ConflictedFiles) > 0 { if conflict { pr.Status = models.PullRequestStatusConflict log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles) return true, nil } + } else if err != nil { return false, fmt.Errorf("git apply --check: %v", err) } return false, nil