From d6b96627c1cf04d254326ce5e751fddf2093a9b1 Mon Sep 17 00:00:00 2001 From: zeripath Date: Mon, 19 Dec 2022 11:37:15 +0000 Subject: [PATCH] Add setting to disable the git apply step in test patch (#22130) For a long time Gitea has tested PR patches using a git apply --check method, and in fact prior to the introduction of a read-tree assisted three-way merge in #18004, this was the only way of checking patches. Since #18004, the git apply --check method has been a fallback method, only used when the read-tree three-way merge method has detected a conflict. The read-tree assisted three-way merge method is much faster and less resource intensive method of detecting conflicts. #18004 kept the git apply method around because it was thought possible that this fallback might be able to rectify conflicts that the read-tree three-way merge detected. I am not certain if this could ever be the case. Given the uncertainty here and the now relative stability of the read-tree method - this PR makes using this fallback optional and disables it by default. The hope is that users will not notice any significant difference in conflict detection and we will be able to remove the git apply fallback in future, and/or improve the read-tree three-way merge method to catch any conflicts that git apply method might have been able to fix. An additional benefit is that patch checking should be significantly less resource intensive and much quicker. (See https://github.com/go-gitea/gitea/issues/22083\#issuecomment-1347961737) Ref #22083 Signed-off-by: Andrew Thornton Co-authored-by: Lunny Xiao Co-authored-by: KN4CK3R --- custom/conf/app.example.ini | 3 +++ .../doc/advanced/config-cheat-sheet.en-us.md | 1 + modules/setting/repository.go | 2 ++ services/pull/patch.go | 21 +++++++++++++++---- 4 files changed, 23 insertions(+), 4 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 8824d31a4..754eab452 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1036,6 +1036,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 = false ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; 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 dcf91c339..026893818 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -134,6 +134,7 @@ In addition there is _`StaticRootPath`_ which can be set as a built-in at build - `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`: **false**: 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 19594369b..ea288d2ed 100644 --- a/modules/setting/repository.go +++ b/modules/setting/repository.go @@ -82,6 +82,7 @@ var ( DefaultMergeMessageOfficialApproversOnly bool PopulateSquashCommentWithCommitMessages bool AddCoCommitterTrailers bool + TestConflictingPatchesWithGitApply bool } `ini:"repository.pull-request"` // Issue Setting @@ -204,6 +205,7 @@ var ( DefaultMergeMessageOfficialApproversOnly bool PopulateSquashCommentWithCommitMessages bool AddCoCommitterTrailers bool + TestConflictingPatchesWithGitApply bool }{ WorkInProgressPrefixes: []string{"WIP:", "[WIP]"}, // Same as GitHub. See diff --git a/services/pull/patch.go b/services/pull/patch.go index e0da410c4..9ef8b8604 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,7 +353,7 @@ 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.Debug("PullRequest[%d]: Patch is empty - ignoring", pr.ID) pr.Status = issues_model.PullRequestStatusEmpty