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

Changes

4 changed files (+89/-6)

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