diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 3f5ce8046f2..3ca49cf7305 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -996,6 +996,9 @@ ROUTER = console ;; ;; Add co-authored-by and co-committed-by trailers if committer does not match author ;ADD_CO_COMMITTER_TRAILERS = true +;; +;; In addition to testing patches using the three-way merge method, re-test conflicting patches with git apply +;TEST_CONFLICTING_PATCHES_WITH_GIT_APPLY = true ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md index 955b9ac2287..c5e6d45ce1b 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -101,6 +101,7 @@ Values containing `#` or `;` must be quoted using `` ` `` or `"""`. - `DEFAULT_MERGE_MESSAGE_OFFICIAL_APPROVERS_ONLY`: **true**: In default merge messages only include approvers who are officially allowed to review. - `POPULATE_SQUASH_COMMENT_WITH_COMMIT_MESSAGES`: **false**: In default squash-merge messages include the commit message of all commits comprising the pull request. - `ADD_CO_COMMITTER_TRAILERS`: **true**: Add co-authored-by and co-committed-by trailers to merge commit messages if committer does not match author. +- `TEST_CONFLICTING_PATCHES_WITH_GIT_APPLY`: **true**: PR patches are tested using a three-way merge method to discover if there are conflicts. If this setting is set to **true**, conflicting patches will be retested using `git apply` - This was the previous behaviour in 1.18 (and earlier) but is somewhat inefficient. Please report if you find that this setting is required. ### Repository - Issue (`repository.issue`) diff --git a/modules/setting/repository.go b/modules/setting/repository.go index d0406dbf902..0947cc00cf0 100644 --- a/modules/setting/repository.go +++ b/modules/setting/repository.go @@ -83,6 +83,7 @@ var ( DefaultMergeMessageOfficialApproversOnly bool PopulateSquashCommentWithCommitMessages bool AddCoCommitterTrailers bool + TestConflictingPatchesWithGitApply bool } `ini:"repository.pull-request"` // Issue Setting @@ -205,6 +206,7 @@ var ( DefaultMergeMessageOfficialApproversOnly bool PopulateSquashCommentWithCommitMessages bool AddCoCommitterTrailers bool + TestConflictingPatchesWithGitApply bool }{ WorkInProgressPrefixes: []string{"WIP:", "[WIP]"}, // Same as GitHub. See @@ -219,6 +221,7 @@ var ( DefaultMergeMessageOfficialApproversOnly: true, PopulateSquashCommentWithCommitMessages: false, AddCoCommitterTrailers: true, + TestConflictingPatchesWithGitApply: true, }, // Issue settings diff --git a/services/pull/patch.go b/services/pull/patch.go index d3e049fc1b5..fdfbf7e072e 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -23,6 +23,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/process" repo_module "code.gitea.io/gitea/modules/repository" + "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" "github.com/gobwas/glob" @@ -289,13 +290,15 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * // 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, + conflict, conflictFiles, err := AttemptThreeWayMerge(ctx, tmpBasePath, gitRepo, pr.MergeBase, "base", "tracking", description) if err != nil { return false, err } if !conflict { + // No conflicts detected so we need to check if the patch is empty... + // a. Write the newly merged tree and check the new tree-hash var treeHash string treeHash, _, err = git.NewCommand(ctx, "write-tree").RunStdString(&git.RunOpts{Dir: tmpBasePath}) if err != nil { @@ -307,6 +310,8 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * if err != nil { return false, err } + + // b. compare the new tree-hash with the base tree hash if treeHash == baseTree.ID.String() { log.Debug("PullRequest[%d]: Patch is empty - ignoring", pr.ID) pr.Status = issues_model.PullRequestStatusEmpty @@ -315,9 +320,17 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * return false, nil } - // 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. + // 3. OK the three-way merge method has detected conflicts + // 3a. Are still testing with GitApply? If not set the conflict status and move on + if !setting.Repository.PullRequest.TestConflictingPatchesWithGitApply { + pr.Status = issues_model.PullRequestStatusConflict + pr.ConflictedFiles = conflictFiles - // 3a. Create a plain patch from head to base + log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles) + return true, nil + } + + // 3b. 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) @@ -340,8 +353,9 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * patchPath := tmpPatchFile.Name() tmpPatchFile.Close() - // 3b. if the size of that patch is 0 - there can be no conflicts! + // 3c. if the size of that patch is 0 - there can be no conflicts! if stat.Size() == 0 { + log.Critical("git-apply--check patch checker found empty PR when read-tree found conflicts in PR#%d[%d] in %#-v", pr.Index, pr.ID, pr.BaseRepo) log.Debug("PullRequest[%d]: Patch is empty - ignoring", pr.ID) pr.Status = issues_model.PullRequestStatusEmpty return false, nil @@ -472,6 +486,8 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * } else if err != nil { return false, fmt.Errorf("git apply --check: %w", err) } + + log.Critical("git-apply--check patch checker found no conflicts when read-tree found conflicts in PR#%d[%d] in %#-v", pr.Index, pr.ID, pr.BaseRepo) return false, nil }