-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
Use merge tree to detect conflicts when possible #36400
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
base: main
Are you sure you want to change the base?
Changes from 21 commits
a69678e
5c9ed23
d444ec4
96e18eb
fbed6af
bd940ea
1f80f77
eb64186
3602ba5
a331d26
4bf32f0
07dd71a
27fe844
c8bbc14
f2dc1f6
e9950c4
abf6b3f
9901d9e
80373af
9cf50af
01d6444
0c317e8
ab1d77d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| // Copyright 2026 The Gitea Authors. All rights reserved. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package gitrepo | ||
|
|
||
| import ( | ||
| "bufio" | ||
| "bytes" | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "strings" | ||
|
|
||
| "code.gitea.io/gitea/modules/git" | ||
| "code.gitea.io/gitea/modules/git/gitcmd" | ||
| ) | ||
|
|
||
| // scanLine is a split function for a Scanner that returns each line of git output | ||
| func scanLine(data []byte, atEOF bool) (advance int, token []byte, err error) { | ||
| if atEOF && len(data) == 0 { | ||
| return 0, nil, nil | ||
| } | ||
| if i := bytes.IndexByte(data, '\x00'); i >= 0 { | ||
| return i + 1, data[0:i], nil | ||
| } | ||
| if atEOF { | ||
| return len(data), data, nil | ||
| } | ||
| return 0, nil, nil | ||
| } | ||
|
|
||
| // parseMergeTreeOutput parses the output of git merge-tree --write-tree -z --name-only --no-messages | ||
| // For a successful merge, the output is a simply one line <OID of toplevel tree>NUL | ||
| // Whereas for a conflicted merge, the output is: | ||
| // <OID of toplevel tree>NUL | ||
| // <Conflicted file name 1>NUL | ||
| // <Conflicted file name 2>NUL | ||
| // ... | ||
| // ref: https://git-scm.com/docs/git-merge-tree/2.38.0#OUTPUT | ||
| func parseMergeTreeOutput(output io.Reader, maxListFiles int) (treeID string, conflictedFiles []string, err error) { | ||
| scanner := bufio.NewScanner(output) | ||
|
|
||
| scanner.Split(scanLine) | ||
| var lineCount int | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| if line == "" { | ||
| continue | ||
| } | ||
| if treeID == "" { // first line is tree ID | ||
| treeID = line | ||
| continue | ||
| } | ||
| conflictedFiles = append(conflictedFiles, line) | ||
| lineCount++ | ||
| if lineCount >= maxListFiles { | ||
| break | ||
| } | ||
| } | ||
| if treeID == "" { | ||
| return "", nil, errors.New("unexpected empty output") | ||
| } | ||
| return treeID, conflictedFiles, nil | ||
lunny marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| const MaxConflictedDetectFiles = 10 | ||
|
|
||
| // MergeTree performs a merge between two commits (baseRef and headRef) with an optional merge base. | ||
| // It returns the resulting tree hash, a list of conflicted files (if any), and an error if the operation fails. | ||
| // If there are no conflicts, the list of conflicted files will be nil. | ||
| func MergeTree(ctx context.Context, repo Repository, baseRef, headRef, mergeBase string) (string, bool, []string, error) { | ||
| cmd := gitcmd.NewCommand("merge-tree", "--write-tree", "-z", "--name-only", "--no-messages") | ||
| if git.DefaultFeatures().CheckVersionAtLeast("2.40") && mergeBase != "" { | ||
| cmd.AddOptionFormat("--merge-base=%s", mergeBase) | ||
| } | ||
|
|
||
| stdout := &bytes.Buffer{} | ||
| gitErr := RunCmd(ctx, repo, cmd.AddDynamicArguments(baseRef, headRef).WithStdoutCopy(stdout)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need to use stdout, why not RunStrstring or RunStdBytes
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to catch all conflicted files, at the moment, only the first 10 conflicted files will be catched. If we use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| // For a successful, non-conflicted merge, the exit status is 0. When the merge has conflicts, the exit status is 1. | ||
| // A merge can have conflicts without having individual files conflict | ||
| // https://git-scm.com/docs/git-merge-tree/2.38.0#_mistakes_to_avoid | ||
| switch { | ||
| case gitcmd.IsErrorExitCode(gitErr, 0) || gitErr == nil: | ||
wxiaoguang marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| return strings.TrimSpace(strings.TrimSuffix(stdout.String(), "\x00")), false, nil, nil | ||
| case gitcmd.IsErrorExitCode(gitErr, 1): | ||
| treeID, conflictedFiles, err := parseMergeTreeOutput(stdout, MaxConflictedDetectFiles) | ||
| if err != nil { | ||
| return "", false, nil, fmt.Errorf("parse merge-tree output failed: %w", err) | ||
| } | ||
| return treeID, true, conflictedFiles, nil | ||
| } | ||
| return "", false, nil, fmt.Errorf("run merge-tree failed: %w", gitErr) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| // Copyright 2026 The Gitea Authors. All rights reserved. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package gitrepo | ||
|
|
||
| import ( | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func Test_parseMergeTreeOutput(t *testing.T) { | ||
| conflictedOutput := "837480c2773160381cbe6bcce90f7732789b5856\x00options/locale/locale_en-US.ini\x00services/webhook/webhook_test.go\x00" | ||
| treeID, conflictedFiles, err := parseMergeTreeOutput(strings.NewReader(conflictedOutput), 10) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "837480c2773160381cbe6bcce90f7732789b5856", treeID) | ||
| assert.Len(t, conflictedFiles, 2) | ||
| assert.Equal(t, "options/locale/locale_en-US.ini", conflictedFiles[0]) | ||
| assert.Equal(t, "services/webhook/webhook_test.go", conflictedFiles[1]) | ||
|
|
||
| nonConflictedOutput := "837480c2773160381cbe6bcce90f7732789b5856\x00" | ||
| treeID, conflictedFiles, err = parseMergeTreeOutput(strings.NewReader(nonConflictedOutput), 10) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "837480c2773160381cbe6bcce90f7732789b5856", treeID) | ||
| assert.Empty(t, conflictedFiles) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1795,6 +1795,7 @@ | |
| "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)", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This has been explained, please take a look at the pull request content.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can it use tests to cover these cases? And, can you avoid assigning
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It said |
||
| "repo.pulls.is_checking": "Checking for merge conflicts…", | ||
| "repo.pulls.is_ancestor": "This branch is already included in the target branch. There is nothing to merge.", | ||
| "repo.pulls.is_empty": "The changes on this branch are already on the target branch. This will be an empty commit.", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,142 @@ | ||
| // Copyright 2026 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" | ||
| ) | ||
|
|
||
| // checkConflictsMergeTree uses git merge-tree to check for conflicts and if none are found checks if the patch is empty | ||
| // return true if there are 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, baseCommitID, pr.HeadCommitID, pr.MergeBase) | ||
| if err != nil { | ||
| return false, fmt.Errorf("MergeTree: %w", err) | ||
| } | ||
| if conflict { | ||
| pr.Status = issues_model.PullRequestStatusConflict | ||
| // sometimes git merge-tree will detect conflicts but not list any conflicted files | ||
| // so that pr.ConflictedFiles will be empty | ||
| pr.ConflictedFiles = conflictFiles | ||
|
|
||
| log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles) | ||
| return true, nil | ||
| } | ||
|
|
||
| // Detect whether the pull request introduces changes by comparing the merged tree (treeHash) | ||
| // against the merge base (pr.MergeBase) using `git diff-tree`. The command returns exit code 0 | ||
| // if there is no diff between these trees (empty patch) and exit code 1 if there is a diff. | ||
| gitErr := gitrepo.RunCmd(ctx, pr.BaseRepo, gitcmd.NewCommand("diff-tree", "-r", "--quiet"). | ||
| AddDynamicArguments(treeHash, pr.MergeBase)) | ||
| switch { | ||
| case gitcmd.IsErrorExitCode(gitErr, 0) || gitErr == nil: | ||
| log.Debug("PullRequest[%d]: Patch is empty - ignoring", pr.ID) | ||
| pr.Status = issues_model.PullRequestStatusEmpty | ||
| case gitcmd.IsErrorExitCode(gitErr, 1): | ||
| pr.Status = issues_model.PullRequestStatusMergeable | ||
| default: | ||
| return false, fmt.Errorf("run diff-tree exit abnormally: %w", gitErr) | ||
| } | ||
| return false, nil | ||
| } | ||
|
|
||
| func testPullRequestMergeTree(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/open base repository | ||
| 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 !baseGitRepo.IsReferenceExist(pr.HeadCommitID) { | ||
| 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 | ||
| } | ||
|
|
||
| // reset conflicted files and changed protected files | ||
| pr.ConflictedFiles = nil | ||
| pr.ChangedProtectedFiles = 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.PullRequestStatusError | ||
| return fmt.Errorf("checkConflictsMergeTree: %w", err) | ||
| } | ||
| if conflicted || pr.Status == issues_model.PullRequestStatusEmpty { | ||
| return nil | ||
| } | ||
|
|
||
| // 8. Check for protected files changes | ||
| if err = checkPullFilesProtection(ctx, pr, baseGitRepo, pr.HeadCommitID); err != nil { | ||
| return fmt.Errorf("pr.CheckPullFilesProtection(): %v", err) | ||
| } | ||
| return nil | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| // Copyright 2026 The Gitea Authors. All rights reserved. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package pull | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| issues_model "code.gitea.io/gitea/models/issues" | ||
| "code.gitea.io/gitea/models/unittest" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func Test_testPullRequestMergeTree(t *testing.T) { | ||
| assert.NoError(t, unittest.PrepareTestDatabase()) | ||
|
|
||
| pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) | ||
| assert.NoError(t, pull.LoadIssue(t.Context())) | ||
| assert.NoError(t, pull.LoadBaseRepo(t.Context())) | ||
| assert.NoError(t, pull.LoadHeadRepo(t.Context())) | ||
|
|
||
| // pull 2 is mergeable, set to conflicted to see if the function updates it correctly | ||
| pull.Status = issues_model.PullRequestStatusConflict | ||
| pull.ConflictedFiles = []string{"old_file.go"} | ||
| pull.ChangedProtectedFiles = []string{"protected_file.go"} | ||
|
|
||
| err := testPullRequestMergeTree(t.Context(), pull) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, issues_model.PullRequestStatusMergeable, pull.Status) | ||
| assert.Empty(t, pull.ConflictedFiles) | ||
| assert.Empty(t, pull.ChangedProtectedFiles) | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.