When conflicts have been previously detected ensure that they can be resolved (#19247)

There is yet another problem with conflicted files not being reset when
the test patch resolves them.

This PR adjusts the code for checkConflicts to reset the ConflictedFiles
field immediately at the top. It also adds a reset to conflictedFiles
for the manuallyMerged and a shortcut for the empty status in
protectedfiles.

Signed-off-by: Andrew Thornton <art27@cantab.net>

diff --git a/models/pull.go b/models/pull.go
index 863c8d708f8b68d9eb496a0c8bb577ea6d960d5f..ec1fde02595b0301dd39cae6b9394cffa14f4404 100644 (file)
--- a/models/pull.go
+++ b/models/pull.go
@@ -424,8 +424,11 @@ func (pr *PullRequest) SetMerged() (bool, error) {
                return false, fmt.Errorf("Issue.changeStatus: %v", err)
        }
 
+       // reset the conflicted files as there cannot be any if we're merged
+       pr.ConflictedFiles = []string{}
+
        // We need to save all of the data used to compute this merge as it may have already been changed by TestPatch. FIXME: need to set some state to prevent TestPatch from running whilst we are merging.
-       if _, err := sess.Where("id = ?", pr.ID).Cols("has_merged, status, merge_base, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil {
+       if _, err := sess.Where("id = ?", pr.ID).Cols("has_merged, status, merge_base, merged_commit_id, merger_id, merged_unix, conflicted_files").Update(pr); err != nil {
                return false, fmt.Errorf("Failed to update pr[%d]: %v", pr.ID, err)
        }
 

diff --git a/services/pull/patch.go b/services/pull/patch.go
index f401b85345ebe8b2ae86a47532b1a1a05a0e5740..398fadde1ad11c1290a15e9605a89d5f6d752422 100644 (file)
--- a/services/pull/patch.go
+++ b/services/pull/patch.go
@@ -270,6 +270,10 @@ func AttemptThreeWayMerge(ctx context.Context, gitPath string, gitRepo *git.Repo
 }
 
 func checkConflicts(ctx context.Context, pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath string) (bool, error) {
+       // 1. checkConflicts resets the conflict status - therefore - reset the conflict status
+       pr.ConflictedFiles = nil
+
+       // 2. AttemptThreeWayMerge first - this is much quicker than plain patch to base
        description := fmt.Sprintf("PR[%d] %s/%s#%d", pr.ID, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, pr.Index)
        conflict, _, err := AttemptThreeWayMerge(ctx,
                tmpBasePath, gitRepo, pr.MergeBase, "base", "tracking", description)
@@ -290,16 +294,14 @@ func checkConflicts(ctx context.Context, pr *models.PullRequest, gitRepo *git.Re
                if treeHash == baseTree.ID.String() {
                        log.Debug("PullRequest[%d]: Patch is empty - ignoring", pr.ID)
                        pr.Status = models.PullRequestStatusEmpty
-                       pr.ConflictedFiles = []string{}
-                       pr.ChangedProtectedFiles = []string{}
                }
 
                return false, nil
        }
 
-       // OK read-tree has failed so we need to try a different thing - this might actually succeed where the above fails due to whitespace handling.
+       // 3. OK read-tree has failed so we need to try a different thing - this might actually succeed where the above fails due to whitespace handling.
 
-       // 1. Create a plain patch from head to base
+       // 3a. Create a plain patch from head to base
        tmpPatchFile, err := os.CreateTemp("", "patch")
        if err != nil {
                log.Error("Unable to create temporary patch file! Error: %v", err)
@@ -322,34 +324,29 @@ func checkConflicts(ctx context.Context, pr *models.PullRequest, gitRepo *git.Re
        patchPath := tmpPatchFile.Name()
        tmpPatchFile.Close()
 
-       // 1a. if the size of that patch is 0 - there can be no conflicts!
+       // 3b. if the size of that patch is 0 - there can be no conflicts!
        if stat.Size() == 0 {
                log.Debug("PullRequest[%d]: Patch is empty - ignoring", pr.ID)
                pr.Status = models.PullRequestStatusEmpty
-               pr.ConflictedFiles = []string{}
-               pr.ChangedProtectedFiles = []string{}
                return false, nil
        }
 
        log.Trace("PullRequest[%d].testPatch (patchPath): %s", pr.ID, patchPath)
 
-       // 2. preset the pr.Status as checking (this is not save at present)
-       pr.Status = models.PullRequestStatusChecking
-
-       // 3. Read the base branch in to the index of the temporary repository
+       // 4. Read the base branch in to the index of the temporary repository
        _, err = git.NewCommand(gitRepo.Ctx, "read-tree", "base").RunInDir(tmpBasePath)
        if err != nil {
                return false, fmt.Errorf("git read-tree %s: %v", pr.BaseBranch, err)
        }
 
-       // 4. Now get the pull request configuration to check if we need to ignore whitespace
+       // 5. Now get the pull request configuration to check if we need to ignore whitespace
        prUnit, err := pr.BaseRepo.GetUnit(unit.TypePullRequests)
        if err != nil {
                return false, err
        }
        prConfig := prUnit.PullRequestsConfig()
 
-       // 5. Prepare the arguments to apply the patch against the index
+       // 6. Prepare the arguments to apply the patch against the index
        args := []string{"apply", "--check", "--cached"}
        if prConfig.IgnoreWhitespaceConflicts {
                args = append(args, "--ignore-whitespace")
@@ -360,9 +357,8 @@ func checkConflicts(ctx context.Context, pr *models.PullRequest, gitRepo *git.Re
                is3way = true
        }
        args = append(args, patchPath)
-       pr.ConflictedFiles = make([]string, 0, 5)
 
-       // 6. Prep the pipe:
+       // 7. Prep the pipe:
        //   - Here we could do the equivalent of:
        //  `git apply --check --cached patch_file > conflicts`
        //     Then iterate through the conflicts. However, that means storing all the conflicts
@@ -380,7 +376,7 @@ func checkConflicts(ctx context.Context, pr *models.PullRequest, gitRepo *git.Re
                _ = stderrWriter.Close()
        }()
 
-       // 7. Run the check command
+       // 8. Run the check command
        conflict = false
        err = git.NewCommand(gitRepo.Ctx, args...).
                RunWithContext(&git.RunContext{
@@ -448,7 +444,7 @@ func checkConflicts(ctx context.Context, pr *models.PullRequest, gitRepo *git.Re
                        },
                })
 
-       // 8. If there is a conflict the `git apply` command will return a non-zero error code - so there will be a positive error.
+       // 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 {
                if conflict {
                        pr.Status = models.PullRequestStatusConflict
@@ -518,6 +514,11 @@ func CheckUnprotectedFiles(repo *git.Repository, oldCommitID, newCommitID string
 
 // checkPullFilesProtection check if pr changed protected files and save results
 func checkPullFilesProtection(pr *models.PullRequest, gitRepo *git.Repository) error {
+       if pr.Status == models.PullRequestStatusEmpty {
+               pr.ChangedProtectedFiles = nil
+               return nil
+       }
+
        if err := pr.LoadProtectedBranch(); err != nil {
                return err
        }