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 <art27@cantab.net>

diff --git a/integrations/pull_merge_test.go b/integrations/pull_merge_test.go
index a0b87eeee8368f4780baa5966a2b1ff4f807bdcb..7646d9f4124591968afa315730f39322b5eea1fb 100644 (file)
--- 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 0511d29dfcc98694391bcf877e8424cb0006563d..4b2b07cdef841dc35d4a049d9687deac4150e4de 100644 (file)
--- 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 2e2c1b85bc6b466733a4f58e184443163a206004..ad757dbe5fe49c8cdacaa55cc54dc263aab74f0f 100644 (file)
--- 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 ee10c97392f2428dc0e50dac5ac39b7886c6bd41..e2851709e05de3576966894727784fd4d4648b6d 100644 (file)
--- 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