False Merge Conflict On Pull Request - A blocking issue for using Gitea

[clintwood]

First off thank you for this OS project - amazing work!!

• Gitea version (or commit ref): 1.11.0+dev-352-g66028d58f (gitea.io)
• Also on private hosted v1.10.0, and latest dev build in Docker
• Can you reproduce the bug at https://try.gitea.io:
  • [x ] Yes (https://try.gitea.io/clintwood/pr-conflict-issue/pulls/2)

Description

Gitea shows a false merge/PR conflict when there is none.

This issue is also present in many other scenarios that involve pushing changes to a branch being merged into which do not cause a merge conflict. I have simplified this issue by using the most trivial example to clearly illustrate the issue.

This issue is also on my locally hosted servers of which I have both v1.10.0 and the latest dev build in Docker - they all (including on try.gitea.io) exhibit the same issue.

NOTE: This is a blocking issue for using Gitea even in a trivial scenario.

Steps to reproduce:

Create repo on Server (https://try.gitea.io/)

Clone (empty) repo from Server
Add a file (README.md) with some text
Commit initial commit
Push to branch master on Server

Create branch A off of master
Add a line to file (non-conflicting - i.e. at the end)
Commit A
Push to branch A on Server

Create branch B off of A
Add line to file
Commit B
Push to branch B on Server

On Server:
Create PR for A into master
Create PR for B into master
Do PR A merge on Server
PR B now shows merge conflict as shown in screenshot

On my local instance of Gitea (v1.10.0) which has the same issue, I was able to do a git diff of master with B (on the bare repo) and it correctly showed only the changes that were made on branch B (since PR A was now merged int master) which is not a conflict if B was merged into master (i.e. for PR B).

To prove that last statement, do the following additional steps:

Locally:
Checkout master
Pull master from Server (https://try.gitea.io/) - it updates no problem (i.e. master + A - merged from PR A)
Merge B into master - works 100% - no conflicts

Screenshots

What you see in PR #2 (PR B) in the example.

[clintwood]

This issue is likely related to #8630 but for clarity this trivial example is not complicated by changes to the PR's target branch (if that makes sense).

[clintwood]

Further observation: If you close PR B and reopen it then you are able to proceed with the merging the pull request (PR B).

Speculation: It seems like the internal PR merge patch is not being updated to reflect changes from the previously merged pull request (PR A).

[lunny]

Any error logs could you find on your system about merging.

[clintwood]

@lunny I'm not seeing any errors in the logs that jump out at me - is there anything specific I should be looking for!?

Edit: The example on gitea.io is currently in the errored state - i.e. showing a merge conflict when there is none.

[MarkusAmshove]

Related: #6417

[zeripath]

So the issue is that when we determine conflict we don't actually try to merge - we create a patch and try to apply that.

The code testing the patch is:

gitea/models/pull.go

Lines 498 to 595 in 95a5739

	func (pr *PullRequest) testPatch(e Engine) (err error) {
	if pr.BaseRepo == nil {
	pr.BaseRepo, err = getRepositoryByID(e, pr.BaseRepoID)
	if err != nil {
	return fmt.Errorf("GetRepositoryByID: %v", err)
	}
	}
	patchPath, err := pr.BaseRepo.patchPath(e, pr.Index)
	if err != nil {
	return fmt.Errorf("BaseRepo.PatchPath: %v", err)
	}
	// Fast fail if patch does not exist, this assumes data is corrupted.
	if !com.IsFile(patchPath) {
	log.Trace("PullRequest[%d].testPatch: ignored corrupted data", pr.ID)
	return nil
	}
	RepoWorkingPool.CheckIn(com.ToStr(pr.BaseRepoID))
	defer RepoWorkingPool.CheckOut(com.ToStr(pr.BaseRepoID))
	log.Trace("PullRequest[%d].testPatch (patchPath): %s", pr.ID, patchPath)
	pr.Status = PullRequestStatusChecking
	indexTmpPath := filepath.Join(os.TempDir(), "gitea-"+pr.BaseRepo.Name+"-"+strconv.Itoa(time.Now().Nanosecond()))
	defer os.Remove(indexTmpPath)
	_, err = git.NewCommand("read-tree", pr.BaseBranch).RunInDirWithEnv("", []string{"GIT_DIR=" + pr.BaseRepo.RepoPath(), "GIT_INDEX_FILE=" + indexTmpPath})
	if err != nil {
	return fmt.Errorf("git read-tree --index-output=%s %s: %v", indexTmpPath, pr.BaseBranch, err)
	}
	prUnit, err := pr.BaseRepo.getUnit(e, UnitTypePullRequests)
	if err != nil {
	return err
	}
	prConfig := prUnit.PullRequestsConfig()
	args := []string{"apply", "--check", "--cached"}
	if prConfig.IgnoreWhitespaceConflicts {
	args = append(args, "--ignore-whitespace")
	}
	args = append(args, patchPath)
	pr.ConflictedFiles = []string{}
	stderrBuilder := new(strings.Builder)
	err = git.NewCommand(args...).RunInDirTimeoutEnvPipeline(
	[]string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()},
	-1,
	"",
	nil,
	stderrBuilder)
	stderr := stderrBuilder.String()
	if err != nil {
	for i := range patchConflicts {
	if strings.Contains(stderr, patchConflicts[i]) {
	log.Trace("PullRequest[%d].testPatch (apply): has conflict: %s", pr.ID, stderr)
	const prefix = "error: patch failed:"
	pr.Status = PullRequestStatusConflict
	pr.ConflictedFiles = make([]string, 0, 5)
	scanner := bufio.NewScanner(strings.NewReader(stderr))
	for scanner.Scan() {
	line := scanner.Text()
	if strings.HasPrefix(line, prefix) {
	var found bool
	var filepath = strings.TrimSpace(strings.Split(line[len(prefix):], ":")[0])
	for _, f := range pr.ConflictedFiles {
	if f == filepath {
	found = true
	break
	}
	}
	if !found {
	pr.ConflictedFiles = append(pr.ConflictedFiles, filepath)
	}
	}
	// only list 10 conflicted files
	if len(pr.ConflictedFiles) >= 10 {
	break
	}
	}
	if len(pr.ConflictedFiles) > 0 {
	log.Trace("Found %d files conflicted: %v", len(pr.ConflictedFiles), pr.ConflictedFiles)
	}
	return nil
	}
	}
	return fmt.Errorf("git apply --check: %v - %s", err, stderr)
	}
	return nil
	}

With the patch being created here:

gitea/models/pull.go

Line 506 in 95a5739

patchPath, err := pr.BaseRepo.patchPath(e, pr.Index)

Which looks a bit like an odd construction, and maps to:

gitea/models/repo.go

Line 895 in 95a5739

func (repo *Repository) patchPath(e Engine, index int64) (string, error) {

With the file appearing to be generated in functions below this using repo.SavePatch(int64, []byte) or the lowercase variant.

There's multiple issues with this, not least the fact that patches can be arbitrarily large and this function implies we're storing these in memory at some point!!

It looks like savepatch is called in two places:

gitea/models/pull.go

Line 640 in 82e0383

if err = repo.savePatch(sess, pr.Index, patch); err != nil {

gitea/models/pull.go

Line 808 in 82e0383

if err = pr.BaseRepo.SavePatch(pr.Index, patch); err != nil {

And I would guess that either they're generated plainly from head to merge base or they're not run necessarily at the right time.

This code needs completely redoing. We absolutely cannot store arbitrary patches in memory - diff and blame are other places where this happens. It appears that the only reason for this to go in to memory is to check its length (!) I'm not certain of the utility of creating a patch file to be stored on the server in any case - we don't use it for anything else afaics - and as shown above it is probably better to just generate on the fly at testing when we know what we're patching against.

(This reading of patch files in to memory is likely the cause of several other issues complaining about high memory use.)

[zeripath]

I should note that the above was fixed by #9302

[zeripath]

Would anyone be able to test on master to see if #9302 has solved the issue?

[zeripath]

I can't replicate this since #9302 was merged. Therefore I'm closing this issue.