From 198342efe4bedb193a5cb8d09f7372f6737cb34d Mon Sep 17 00:00:00 2001
From: zeripath <art27@cantab.net>
Date: Thu, 22 Dec 2022 10:59:10 +0000
Subject: [PATCH] Add setting to disable the git apply step in test patch
(#22130) (#22170)
Backport #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 but
enables it by default. A `log.Critical` has been added which will alert
if the `git apply --check` method was successful at checking a PR that
`read-tree` failed on.
The hope is that none of these log.Critical messages will be found and
there will be no significant difference in conflict detection. Thus 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 for anyone who disables the check method 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 <art27@cantab.net>
<!--
Please check the following:
1. Make sure you are targeting the `main` branch, pull requests on
release branches are only allowed for bug fixes.
2. Read contributing guidelines:
https://github.com/go-gitea/gitea/blob/main/CONTRIBUTING.md
3. Describe what your pull request does and which issue you're targeting
(if any)
-->
Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
---
custom/conf/app.example.ini | 3 +++
.../doc/advanced/config-cheat-sheet.en-us.md | 1 +
modules/setting/repository.go | 3 +++
services/pull/patch.go | 24 +++++++++++++++----
4 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini
index 3f5ce8046f..3ca49cf730 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 955b9ac228..c5e6d45ce1 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 d0406dbf90..0947cc00cf 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 d3e049fc1b..fdfbf7e072 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
}