-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Use merge tree to improve conflict checking performance when possible #35542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
lunny
wants to merge
38
commits into
go-gitea:main
Choose a base branch
from
lunny:lunny/merge_tree_conflict_check
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+872
−288
Open
Changes from 8 commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
dda296a
Fix lint
lunny 21cc4aa
improvements
lunny 2f74aec
remove unused functions
lunny 3d0222c
allow empty pull request
lunny 7e973b3
improvements
lunny 72a154a
fix bug
lunny 76da4bf
fix bug
lunny 4d4f3ae
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny af79992
add tests for mergeable tmprepo checking
lunny ed5a749
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny e8636b7
Add both mergetree and tmprepo for rebase and retarget tests
lunny 4b8c047
make test happy
lunny 9fad9fb
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny 6e96a4a
remove unnecessary check
lunny 22f0aa2
Fix test
lunny 793cbf7
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny 5b1229e
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny 07f6a8b
improvements
lunny 0a9eff3
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny acb99d4
remove unused comment
lunny dc0abc4
Fix test
lunny 703a0c5
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny b61a5ae
remove unnecessary code
lunny 146e816
Fix test
lunny 09f519e
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny d34e640
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny d68ad1b
improvements
lunny 9318bbe
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny 0d28912
Merge branch 'lunny/merge_tree_conflict_check' of github.com:lunny/gi…
lunny da5fb5c
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny 59ea28c
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny 2784879
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny 3845774
improvements
lunny 2609edb
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny 562ab41
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny 053817a
Fix test
lunny 0bf39c7
Merge branch 'lunny/merge_tree_conflict_check' of github.com:lunny/gi…
lunny 556ad16
Merge branch 'main' into lunny/merge_tree_conflict_check
lunny File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| // Copyright 2025 The Gitea Authors. All rights reserved. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package gitrepo | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| "code.gitea.io/gitea/modules/git/gitcmd" | ||
| ) | ||
|
|
||
| func FetchRemoteCommit(ctx context.Context, repo, remoteRepo Repository, commitID string) error { | ||
| _, _, err := gitcmd.NewCommand("fetch", "--no-tags"). | ||
| AddDynamicArguments(repoPath(remoteRepo)). | ||
| AddDynamicArguments(commitID). | ||
| RunStdString(ctx, &gitcmd.RunOpts{Dir: repoPath(repo)}) | ||
| return err | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| // Copyright 2025 The Gitea Authors. All rights reserved. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package gitrepo | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "code.gitea.io/gitea/modules/git" | ||
| "code.gitea.io/gitea/modules/git/gitcmd" | ||
| "code.gitea.io/gitea/modules/log" | ||
| ) | ||
|
|
||
| func MergeBase(ctx context.Context, repo Repository, commit1, commit2 string) (string, error) { | ||
| mergeBase, _, err := gitcmd.NewCommand("merge-base", "--"). | ||
| AddDynamicArguments(commit1, commit2). | ||
| RunStdString(ctx, &gitcmd.RunOpts{Dir: repoPath(repo)}) | ||
| if err != nil { | ||
| return "", fmt.Errorf("get merge-base of %s and %s failed: %w", commit1, commit2, err) | ||
| } | ||
| return strings.TrimSpace(mergeBase), nil | ||
| } | ||
|
|
||
| func MergeTree(ctx context.Context, repo Repository, base, ours, theirs string) (string, bool, []string, error) { | ||
| cmd := gitcmd.NewCommand("merge-tree", "--write-tree", "-z", "--name-only", "--no-messages") | ||
| // https://git-scm.com/docs/git-merge-tree/2.40.0#_mistakes_to_avoid | ||
| if git.DefaultFeatures().CheckVersionAtLeast("2.40") && !git.DefaultFeatures().CheckVersionAtLeast("2.41") { | ||
| cmd.AddOptionFormat("--merge-base=%s", base) | ||
| } | ||
|
|
||
| stdout := &bytes.Buffer{} | ||
| gitErr := cmd.AddDynamicArguments(ours, theirs).Run(ctx, &gitcmd.RunOpts{ | ||
| Dir: repoPath(repo), | ||
| Stdout: stdout, | ||
| }) | ||
| if gitErr != nil && !gitcmd.IsErrorExitCode(gitErr, 1) { | ||
| log.Error("run merge-tree failed: %v", gitErr) | ||
| return "", false, nil, fmt.Errorf("run merge-tree failed: %w", gitErr) | ||
| } | ||
|
|
||
| // There are two situations that we consider for the output: | ||
| // 1. Clean merge and the output is <OID of toplevel tree>NUL | ||
| // 2. Merge conflict and the output is <OID of toplevel tree>NUL<Conflicted file info>NUL | ||
| treeOID, conflictedFileInfo, _ := strings.Cut(stdout.String(), "\x00") | ||
| if len(conflictedFileInfo) == 0 { | ||
| return treeOID, gitcmd.IsErrorExitCode(gitErr, 1), nil, nil | ||
| } | ||
|
|
||
| // Remove last NULL-byte from conflicted file info, then split with NULL byte as separator. | ||
| return treeOID, true, strings.Split(conflictedFileInfo[:len(conflictedFileInfo)-1], "\x00"), nil | ||
| } | ||
|
|
||
| func DiffTree(ctx context.Context, repo Repository, treeHash, mergeBase string) error { | ||
| return gitcmd.NewCommand("diff-tree", "--quiet").AddDynamicArguments(treeHash, mergeBase). | ||
| Run(ctx, &gitcmd.RunOpts{ | ||
| Dir: repoPath(repo), | ||
| }) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| // Copyright 2025 The Gitea Authors. All rights reserved. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package pull | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
|
|
||
| issues_model "code.gitea.io/gitea/models/issues" | ||
| "code.gitea.io/gitea/modules/git" | ||
| "code.gitea.io/gitea/modules/git/gitcmd" | ||
| "code.gitea.io/gitea/modules/gitrepo" | ||
| "code.gitea.io/gitea/modules/log" | ||
| ) | ||
|
|
||
| // checkPullRequestMergeableAndUpdateStatus checks whether a pull request is mergeable and updates its status accordingly. | ||
| // It uses 'git merge-tree' if supported by the Git version, otherwise it falls back to using a temporary repository. | ||
| // This function updates the pr.Status, pr.MergeBase and pr.ConflictedFiles fields as necessary. | ||
| // The pull request parameter may not be created yet in the database, so do not assume it has an ID. | ||
| func checkPullRequestMergeableAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) error { | ||
| if git.DefaultFeatures().SupportGitMergeTree { | ||
| return checkPullRequestMergeableAndUpdateStatusMergeTree(ctx, pr) | ||
| } | ||
|
|
||
| return checkPullRequestMergeableAndUpdateStatusTmpRepo(ctx, pr) | ||
| } | ||
|
|
||
| // checkConflictsMergeTree uses git merge-tree to check for conflicts and if none are found checks if the patch is empty | ||
| // return true if there is conflicts otherwise return false | ||
| // pr.Status and pr.ConflictedFiles will be updated as necessary | ||
| func checkConflictsMergeTree(ctx context.Context, pr *issues_model.PullRequest, baseCommitID string) (bool, error) { | ||
| treeHash, conflict, conflictFiles, err := gitrepo.MergeTree(ctx, pr.BaseRepo, pr.MergeBase, baseCommitID, pr.HeadCommitID) | ||
| if err != nil { | ||
| return false, fmt.Errorf("MergeTree: %w", err) | ||
| } | ||
| if conflict { | ||
| pr.Status = issues_model.PullRequestStatusConflict | ||
| pr.ConflictedFiles = conflictFiles | ||
|
|
||
| log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles) | ||
| return true, nil | ||
| } | ||
|
|
||
| // No conflicts were detected, now check if the pull request actually | ||
| // contains anything useful via a diff. git-diff-tree(1) with --quiet | ||
| // will return exit code 0 if there's no diff and exit code 1 if there's | ||
| // a diff. | ||
| isEmpty := true | ||
| if err = gitrepo.DiffTree(ctx, pr.BaseRepo, treeHash, pr.MergeBase); err != nil { | ||
| if !gitcmd.IsErrorExitCode(err, 1) { | ||
| return false, fmt.Errorf("DiffTree: %w", err) | ||
| } | ||
| isEmpty = false | ||
| } | ||
|
|
||
| if isEmpty { | ||
| log.Debug("PullRequest[%d]: Patch is empty - ignoring", pr.ID) | ||
| pr.Status = issues_model.PullRequestStatusEmpty | ||
| } | ||
| return false, nil | ||
| } | ||
|
|
||
| func checkPullRequestMergeableAndUpdateStatusMergeTree(ctx context.Context, pr *issues_model.PullRequest) error { | ||
| // 1. Get head commit | ||
| if err := pr.LoadHeadRepo(ctx); err != nil { | ||
| return err | ||
| } | ||
| headGitRepo, err := gitrepo.OpenRepository(ctx, pr.HeadRepo) | ||
| if err != nil { | ||
| return fmt.Errorf("OpenRepository: %w", err) | ||
| } | ||
| defer headGitRepo.Close() | ||
|
|
||
| // 2. Get base commit id | ||
| var baseGitRepo *git.Repository | ||
| if pr.IsSameRepo() { | ||
| baseGitRepo = headGitRepo | ||
| } else { | ||
| baseGitRepo, err = gitrepo.OpenRepository(ctx, pr.BaseRepo) | ||
| if err != nil { | ||
| return fmt.Errorf("OpenRepository: %w", err) | ||
| } | ||
| defer baseGitRepo.Close() | ||
| } | ||
|
|
||
| // 3. Get head commit id | ||
| if pr.Flow == issues_model.PullRequestFlowGithub { | ||
| pr.HeadCommitID, err = headGitRepo.GetRefCommitID(git.BranchPrefix + pr.HeadBranch) | ||
| if err != nil { | ||
| return fmt.Errorf("GetBranchCommitID: can't find commit ID for head: %w", err) | ||
| } | ||
| } else { | ||
| if pr.ID > 0 { | ||
| pr.HeadCommitID, err = baseGitRepo.GetRefCommitID(pr.GetGitHeadRefName()) | ||
| if err != nil { | ||
| return fmt.Errorf("GetRefCommitID: can't find commit ID for head: %w", err) | ||
| } | ||
| } else if pr.HeadCommitID == "" { // for new pull request with agit, the head commit id must be provided | ||
| return errors.New("head commit ID is empty for pull request Agit flow") | ||
| } | ||
| } | ||
|
|
||
| // 4. fetch head commit id into the current repository | ||
| // it will be checked in 2 weeks by default from git if the pull request created failure. | ||
| if !pr.IsSameRepo() { | ||
| if err := gitrepo.FetchRemoteCommit(ctx, pr.BaseRepo, pr.HeadRepo, pr.HeadCommitID); err != nil { | ||
| return fmt.Errorf("FetchRemoteCommit: %w", err) | ||
| } | ||
| } | ||
|
|
||
| // 5. update merge base | ||
| baseCommitID, err := baseGitRepo.GetRefCommitID(git.BranchPrefix + pr.BaseBranch) | ||
| if err != nil { | ||
| return fmt.Errorf("GetBranchCommitID: can't find commit ID for base: %w", err) | ||
| } | ||
|
|
||
| pr.MergeBase, err = gitrepo.MergeBase(ctx, pr.BaseRepo, baseCommitID, pr.HeadCommitID) | ||
| if err != nil { | ||
| log.Error("GetMergeBase: %v and can't find commit ID for base: %v", err, baseCommitID) | ||
| pr.Status = issues_model.PullRequestStatusEmpty // if there is no merge base, then it's empty but we still need to allow the pull request created | ||
| return nil | ||
| } | ||
|
|
||
| // 6. if base == head, then it's an ancestor | ||
| if pr.HeadCommitID == pr.MergeBase { | ||
| pr.Status = issues_model.PullRequestStatusAncestor | ||
| return nil | ||
| } | ||
|
|
||
| // 7. Check for conflicts | ||
| conflicted, err := checkConflictsMergeTree(ctx, pr, baseCommitID) | ||
| if err != nil { | ||
| log.Error("checkConflictsMergeTree: %v", err) | ||
| pr.Status = issues_model.PullRequestStatusEmpty // if there is no merge base, then it's empty but we still need to allow the pull request created | ||
| } | ||
| if conflicted || pr.Status == issues_model.PullRequestStatusEmpty { | ||
| return nil | ||
| } | ||
|
|
||
| // 7. Check for protected files changes | ||
| if err = checkPullFilesProtection(ctx, pr, pr.BaseRepo.RepoPath()); err != nil { | ||
| return fmt.Errorf("pr.CheckPullFilesProtection(): %v", err) | ||
| } | ||
| if len(pr.ChangedProtectedFiles) > 0 { | ||
| log.Trace("Found %d protected files changed", len(pr.ChangedProtectedFiles)) | ||
| } | ||
|
|
||
| pr.Status = issues_model.PullRequestStatusMergeable | ||
| return nil | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have told you, you need to clearly test this function's behavior, but not keep copy-pasting or polluting other unrelated tests