From ebe569a268bbe71bf2bc30cd2829227700688b57 Mon Sep 17 00:00:00 2001 From: Gusted Date: Thu, 21 Apr 2022 21:55:45 +0000 Subject: [PATCH] Set correct PR status on 3way on conflict checking (#19457) * Set correct PR status on 3way on conflict checking - 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. * Fix linter --- integrations/pull_merge_test.go | 73 +++++++++++++++++++++++++++++++++ models/pull.go | 11 +++++ modules/convert/pull.go | 5 +-- services/pull/patch.go | 6 ++- 4 files changed, 89 insertions(+), 6 deletions(-) diff --git a/integrations/pull_merge_test.go b/integrations/pull_merge_test.go index d7cb042e9..f8e8c79a8 100644 --- a/integrations/pull_merge_test.go +++ b/integrations/pull_merge_test.go @@ -26,6 +26,8 @@ import ( "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/modules/translation/i18n" "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" ) @@ -346,3 +348,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(git.DefaultContext, 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(git.DefaultContext, 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(git.DefaultContext, 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 439005deb..ac44ebf0b 100644 --- a/models/pull.go +++ b/models/pull.go @@ -701,3 +701,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 9c53afe8f..3b39e3d2c 100644 --- a/modules/convert/pull.go +++ b/modules/convert/pull.go @@ -68,6 +68,7 @@ func ToAPIPullRequest(ctx context.Context, pr *models.PullRequest, doer *user_mo 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(), @@ -191,10 +192,6 @@ func ToAPIPullRequest(ctx context.Context, pr *models.PullRequest, doer *user_mo } } - 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 f86141aa7..f118ef33d 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -444,14 +444,16 @@ func checkConflicts(ctx context.Context, pr *models.PullRequest, gitRepo *git.Re }, }) - // 9. 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