Use merge tree to detect conflicts when possible #36400
Conversation
GiteaBot
added
the
lgtm/need 2
lunny
force-pushed
the
lunny/merge_tree2
branch
from
b802e08 to
fbed6af
Compare
github-actions
bot
added
the
modifies/templates
There was a problem hiding this comment.
Pull request overview
This PR introduces support for Git 2.38+'s merge-tree --write-tree command to detect merge conflicts directly on bare repositories without creating temporary repositories. This provides a significant performance improvement by avoiding the overhead of setting up and tearing down temporary repos. The PR also addresses an edge case where merge conflicts may exist but no specific conflicted files are listed by git merge-tree.
Changes:
- Added new merge tree implementation in
modules/gitrepo/merge_tree.goandservices/pull/merge_tree.gothat usesgit merge-tree --write-treefor Git 2.38+ - Modified conflict detection logic to use the new merge tree path when available, with fallback to the existing temporary repository approach
- Updated
IsFilesConflicted()to check PR status instead of file list length to handle conflicts without listed files - Added test coverage for both code paths (merge tree and temporary repo) and utility function for mocking atomic booleans
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/gitrepo/merge_tree.go | Core implementation of merge-tree command execution and output parsing |
| modules/gitrepo/merge_tree_test.go | Unit tests for merge-tree output parsing |
| services/pull/merge_tree.go | Service layer implementation using merge-tree for PR conflict detection |
| services/pull/merge_tree_test.go | Integration test for the merge tree path |
| modules/git/git.go | Added SupportGitMergeTree feature flag for Git 2.38+ |
| modules/test/utils.go | Added MockVariableAtomicBool utility for testing |
| models/issues/pull.go | Updated IsFilesConflicted to check status instead of file list |
| services/pull/patch.go | Integrated merge tree path with feature flag check and improved logging |
| templates/repo/issue/view_content/pull_merge_box.tmpl | Added UI message for conflicts without listed files |
| options/locale/locale_en-US.json | Added locale string for empty conflict file list |
| tests/integration/pull_merge_test.go | Duplicated tests to cover both merge tree and temporary repo paths |
| tests/integration/pull_comment_test.go | Duplicated tests to cover both merge tree and temporary repo paths |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "repo.pulls.remove_prefix": "Remove <strong>%s</strong> prefix", | ||
| "repo.pulls.data_broken": "This pull request is broken due to missing fork information.", | ||
| "repo.pulls.files_conflicted": "This pull request has changes conflicting with the target branch.", | ||
| "repo.pulls.files_conflicted_no_listed_files": "(No conflicting files listed)", |
There was a problem hiding this comment.
It also fixes a possible situation that conflict files might be empty but it's a conflict status according to git-scm.com/docs/git-merge-tree#_mistakes_to_avoid
This has been explained, please take a look at the pull request content.
There was a problem hiding this comment.
Can it use tests to cover these cases?
And, can you avoid assigning SupportGitMergeTree, just test the function explicitly? I don't see why it should keep testing the different callers again and again.
There was a problem hiding this comment.
The test needs to create new commit to make rebase, retarget, conflict situations, they will update the git repositories but seems the current unit tests framework don't support that. That's why they are not put in the unit tests rather than integration tests.
There was a problem hiding this comment.
git-scm.com/docs/git-merge-tree#_mistakes_to_avoid
It said there are a few types of directory rename conflicts that fall into this category, and others might also be added in the future. I will try to have a test for a directory rename.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Lunny Xiao <xiaolunwen@gmail.com>
| } | ||
|
|
||
| stdout := &bytes.Buffer{} | ||
| gitErr := RunCmd(ctx, repo, cmd.AddDynamicArguments(baseRef, headRef).WithStdoutCopy(stdout)) |
There was a problem hiding this comment.
If you need to use stdout, why not RunStrstring or RunStdBytes
There was a problem hiding this comment.
We don't need to catch all conflicted files, at the moment, only the first 10 conflicted files will be catched. If we use RunStrstring or RunStdBytes, we have to read all the contents to the memory.
There was a problem hiding this comment.
WithStdoutCopy exactly copies all memory into your buffer.
3 participants
In Git 2.38, the
merge-treecommand introduced the--write-treeoption, which works directly on bare repositories.This option produces the merged tree object ID, allowing us to perform diffs between commits without creating a temporary repository. By avoiding the overhead of setting up and tearing down temporary repos, this approach delivers a notable performance improvement.
It also fixes a possible situation that conflict files might be empty but it's a conflict status according to https://git-scm.com/docs/git-merge-tree#_mistakes_to_avoid
Replace #35542