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>

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())
+	})
+}

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()
+}

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

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