From b23c35b4749d105dd75c2febc8688fa5ed649061 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 16 Dec 2021 20:23:28 +0000 Subject: [PATCH 1/5] Improve TestPatch to use git read-tree -m The current TestPatch conflict code uses a plain git apply which does not properly account for 3-way merging. However, we can improve things using `git read-tree -m` to do a three-way merge. We can also use `--patience` to generate a nicer diff for applying patches too. Fix #13679 Signed-off-by: Andrew Thornton --- modules/git/repo_compare.go | 2 +- services/pull/patch.go | 64 ++++++++++++++++++++++++++++++++++++- 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 4342eb4b2fefd..7039074998f9d 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -237,7 +237,7 @@ func (repo *Repository) GetDiff(base, head string, w io.Writer) error { // GetDiffBinary generates and returns patch data between given revisions, including binary diffs. func (repo *Repository) GetDiffBinary(base, head string, w io.Writer) error { - return NewCommandContext(repo.Ctx, "diff", "-p", "--binary", base, head). + return NewCommandContext(repo.Ctx, "diff", "-p", "--binary", "--patience", base, head). RunInDirPipeline(repo.Path, w, nil) } diff --git a/services/pull/patch.go b/services/pull/patch.go index b971eb4bf30d1..0a55c42796264 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -99,6 +99,68 @@ func TestPatch(pr *models.PullRequest) error { } func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath string) (bool, error) { + if _, err := git.NewCommand("read-tree", "-m", pr.MergeBase, "base", "tracking").RunInDir(tmpBasePath); err != nil { + log.Error("Unable to run read-tree -m! Error: %v", err) + return false, fmt.Errorf("Unable to run read-tree -m! Error: %v", err) + } + + diffReader, diffWriter, err := os.Pipe() + if err != nil { + log.Error("Unable to open stderr pipe: %v", err) + return false, fmt.Errorf("Unable to open stderr pipe: %v", err) + } + defer func() { + _ = diffReader.Close() + _ = diffWriter.Close() + }() + stderr := &strings.Builder{} + + conflict := false + numberOfConflicts := 0 + err = git.NewCommand("diff", "--name-status", "--diff-filter=U"). + RunInDirTimeoutEnvFullPipelineFunc( + nil, -1, tmpBasePath, + diffWriter, stderr, nil, + func(ctx context.Context, cancel context.CancelFunc) error { + // Close the writer end of the pipe to begin processing + _ = diffWriter.Close() + defer func() { + // Close the reader on return to terminate the git command if necessary + _ = diffReader.Close() + }() + + // Now scan the output from the command + scanner := bufio.NewScanner(diffReader) + for scanner.Scan() { + line := scanner.Text() + split := strings.SplitN(line, "\t", 2) + file := "" + if len(split) == 2 { + file = split[1] + } + + if file != "" { + conflict = true + if numberOfConflicts < 10 { + pr.ConflictedFiles = append(pr.ConflictedFiles, file) + } + numberOfConflicts++ + } + } + + return nil + }) + + if err != nil { + return false, fmt.Errorf("git diff --name-status --filter=U: %v", git.ConcatenateError(err, stderr.String())) + } + + if !conflict { + return false, nil + } + + // OK read-tree has failed so we need to try a different thing + // 1. Create a plain patch from head to base tmpPatchFile, err := os.CreateTemp("", "patch") if err != nil { @@ -176,7 +238,7 @@ func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath }() // 7. Run the check command - conflict := false + conflict = false err = git.NewCommand(args...). RunInDirTimeoutEnvFullPipelineFunc( nil, -1, tmpBasePath, From 46bcf3e9849da44493493c22d5a8f1e8526d7736 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 17 Dec 2021 15:44:52 +0000 Subject: [PATCH 2/5] Implement the git-merge-one-file algorithm Signed-off-by: Andrew Thornton --- modules/git/repo_compare.go | 4 + services/pull/patch.go | 201 ++++++++++++++++++++++++-------- services/pull/patch_unmerged.go | 180 ++++++++++++++++++++++++++++ 3 files changed, 335 insertions(+), 50 deletions(-) create mode 100644 services/pull/patch_unmerged.go diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 7039074998f9d..5fe37aed7b6ec 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -237,6 +237,10 @@ func (repo *Repository) GetDiff(base, head string, w io.Writer) error { // GetDiffBinary generates and returns patch data between given revisions, including binary diffs. func (repo *Repository) GetDiffBinary(base, head string, w io.Writer) error { + if CheckGitVersionAtLeast("1.7.7") == nil { + return NewCommandContext(repo.Ctx, "diff", "-p", "--binary", "--histogram", base, head). + RunInDirPipeline(repo.Path, w, nil) + } return NewCommandContext(repo.Ctx, "diff", "-p", "--binary", "--patience", base, head). RunInDirPipeline(repo.Path, w, nil) } diff --git a/services/pull/patch.go b/services/pull/patch.go index 0a55c42796264..9b10ab5c452b3 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -16,7 +16,9 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/util" "github.com/gobwas/glob" @@ -98,74 +100,170 @@ func TestPatch(pr *models.PullRequest) error { return nil } +func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, gitRepo *git.Repository, markConflict func(string)) error { + switch { + case file.stage1 != nil && (file.stage2 == nil || file.stage3 == nil): + // 1. Deleted in one or both: + // + // Conflict <==> the stage1 !Equivalent to the undeleted one + if (file.stage2 != nil && !file.stage1.SameAs(file.stage2)) || (file.stage3 != nil && !file.stage1.SameAs(file.stage3)) { + // Conflict! + markConflict(file.stage1.path) + return nil + } + + // Not a genuine conflict and we can simply remove the file from the index + return gitRepo.RemoveFilesFromIndex(file.stage1.path) + case file.stage1 == nil && file.stage2 != nil && (file.stage3 == nil || file.stage2.SameAs(file.stage3)): + // 2. Added in ours but not in theirs or identical in both + // + // Not a genuine conflict just add to the index + if err := gitRepo.AddObjectToIndex(file.stage2.mode, git.MustIDFromString(file.stage2.sha), file.stage2.path); err != nil { + return err + } + return nil + case file.stage1 == nil && file.stage2 != nil && file.stage3 != nil && file.stage2.sha == file.stage3.sha && file.stage2.mode != file.stage3.mode: + // 3. Added in both with the same sha but the modes are different + // + // Conflict! (Not sure that this can actually happen but we should handle) + markConflict(file.stage2.path) + return nil + case file.stage1 == nil && file.stage2 == nil && file.stage3 != nil: + // 4. Added in theirs but not ours: + // + // Not a genuine conflict just add to the index + return gitRepo.AddObjectToIndex(file.stage3.mode, git.MustIDFromString(file.stage3.sha), file.stage3.path) + case file.stage1 == nil: + // 5. Created by new in both + // + // Conflict! + markConflict(file.stage2.path) + return nil + case file.stage2 != nil && file.stage3 != nil: + // 5. Modified in both - we should try to merge in the changes but first: + // + if file.stage2.mode == "120000" || file.stage3.mode == "120000" { + // 5a. Conflicting symbolic link change + markConflict(file.stage2.path) + return nil + } + if file.stage2.mode == "160000" || file.stage3.mode == "160000" { + // 5b. Conflicting submodule change + markConflict(file.stage2.path) + return nil + } + if file.stage2.mode != file.stage3.mode { + // 5c. Conflicting mode change + markConflict(file.stage2.path) + return nil + } + + // Need to get the objects from the object db to attempt to merge + root, err := git.NewCommandContext(ctx, "unpack-file", file.stage1.sha).RunInDir(tmpBasePath) + if err != nil { + return fmt.Errorf("unable to get root object: %s at path: %s for merging. Error: %w", file.stage1.sha, file.stage1.path, err) + } + root = strings.TrimSpace(root) + defer util.Remove(root) + base, err := git.NewCommandContext(ctx, "unpack-file", file.stage2.sha).RunInDir(tmpBasePath) + if err != nil { + return fmt.Errorf("unable to get base object: %s at path: %s for merging. Error: %w", file.stage2.sha, file.stage2.path, err) + } + base = strings.TrimSpace(base) + defer util.Remove(base) + head, err := git.NewCommandContext(ctx, "unpack-file", file.stage3.sha).RunInDir(tmpBasePath) + if err != nil { + return fmt.Errorf("unable to get head object:%s at path: %s for merging. Error: %w", file.stage3.sha, file.stage3.path, err) + } + head = strings.TrimSpace(head) + defer util.Remove(head) + + // now git merge-file annoyingly takes a different order to the merge-tree ... + _, conflictErr := git.NewCommandContext(ctx, "merge-file", base, root, head).RunInDir(tmpBasePath) + if conflictErr != nil { + markConflict(file.stage2.path) + return nil + } + + // base now contains the merged data + hash, err := git.NewCommandContext(ctx, "hash-object", "-w", "--path", file.stage2.path, base).RunInDir(tmpBasePath) + if err != nil { + return err + } + hash = strings.TrimSpace(hash) + return gitRepo.AddObjectToIndex(file.stage2.mode, git.MustIDFromString(hash), file.stage2.path) + default: + if file.stage1 != nil { + markConflict(file.stage1.path) + } else if file.stage2 != nil { + markConflict(file.stage2.path) + } else if file.stage3 != nil { + markConflict(file.stage2.path) + } else { + // This shouldn't happen! + } + } + return nil +} + func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath string) (bool, error) { - if _, err := git.NewCommand("read-tree", "-m", pr.MergeBase, "base", "tracking").RunInDir(tmpBasePath); err != nil { + ctx, cancel, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("checkConflicts: pr[%d] %s/%s#%d", pr.ID, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, pr.Index)) + defer finished() + + // First we use read-tree to do a simple three-way merge + if _, err := git.NewCommandContext(ctx, "read-tree", "-m", pr.MergeBase, "base", "tracking").RunInDir(tmpBasePath); err != nil { log.Error("Unable to run read-tree -m! Error: %v", err) - return false, fmt.Errorf("Unable to run read-tree -m! Error: %v", err) + return false, fmt.Errorf("unable to run read-tree -m! Error: %v", err) } - diffReader, diffWriter, err := os.Pipe() - if err != nil { - log.Error("Unable to open stderr pipe: %v", err) - return false, fmt.Errorf("Unable to open stderr pipe: %v", err) - } + // Then we use git ls-files -u to list the unmerged files and collate the triples in unmergedfiles + unmerged := make(chan *unmergedFile) + go unmergedFiles(ctx, tmpBasePath, unmerged) + defer func() { - _ = diffReader.Close() - _ = diffWriter.Close() + cancel() + for range unmerged { + // empty the unmerged channel + } }() - stderr := &strings.Builder{} - conflict := false numberOfConflicts := 0 - err = git.NewCommand("diff", "--name-status", "--diff-filter=U"). - RunInDirTimeoutEnvFullPipelineFunc( - nil, -1, tmpBasePath, - diffWriter, stderr, nil, - func(ctx context.Context, cancel context.CancelFunc) error { - // Close the writer end of the pipe to begin processing - _ = diffWriter.Close() - defer func() { - // Close the reader on return to terminate the git command if necessary - _ = diffReader.Close() - }() - - // Now scan the output from the command - scanner := bufio.NewScanner(diffReader) - for scanner.Scan() { - line := scanner.Text() - split := strings.SplitN(line, "\t", 2) - file := "" - if len(split) == 2 { - file = split[1] - } - - if file != "" { - conflict = true - if numberOfConflicts < 10 { - pr.ConflictedFiles = append(pr.ConflictedFiles, file) - } - numberOfConflicts++ - } - } + conflict := false + markConflict := func(filepath string) { + log.Trace("Conflict: %s in PR[%d] %s/%s#%d", filepath, pr.ID, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, pr.Index) + conflict = true + if numberOfConflicts < 10 { + pr.ConflictedFiles = append(pr.ConflictedFiles, filepath) + } + numberOfConflicts++ + } - return nil - }) + for file := range unmerged { + if file == nil { + break + } + if file.err != nil { + cancel() + return false, file.err + } - if err != nil { - return false, fmt.Errorf("git diff --name-status --filter=U: %v", git.ConcatenateError(err, stderr.String())) + // OK now we have the unmerged file triplet attempt to merge it + if err := attemptMerge(ctx, file, tmpBasePath, gitRepo, markConflict); err != nil { + return false, err + } } if !conflict { return false, nil } - // OK read-tree has failed so we need to try a different thing + // OK read-tree has failed so we need to try a different thing - this might actually suceed where the above fails due to whitespace handling. // 1. 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) - return false, fmt.Errorf("Unable to create temporary patch file! Error: %v", err) + return false, fmt.Errorf("unable to create temporary patch file! Error: %v", err) } defer func() { _ = util.Remove(tmpPatchFile.Name()) @@ -174,12 +272,12 @@ func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath if err := gitRepo.GetDiffBinary(pr.MergeBase, "tracking", tmpPatchFile); err != nil { tmpPatchFile.Close() log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) - return false, fmt.Errorf("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) + return false, fmt.Errorf("unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) } stat, err := tmpPatchFile.Stat() if err != nil { tmpPatchFile.Close() - return false, fmt.Errorf("Unable to stat patch file: %v", err) + return false, fmt.Errorf("unable to stat patch file: %v", err) } patchPath := tmpPatchFile.Name() tmpPatchFile.Close() @@ -216,6 +314,9 @@ func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath if prConfig.IgnoreWhitespaceConflicts { args = append(args, "--ignore-whitespace") } + if git.CheckGitVersionAtLeast("2.32.0") == nil { + args = append(args, "--3way") + } args = append(args, patchPath) pr.ConflictedFiles = make([]string, 0, 5) @@ -230,7 +331,7 @@ func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath stderrReader, stderrWriter, err := os.Pipe() if err != nil { log.Error("Unable to open stderr pipe: %v", err) - return false, fmt.Errorf("Unable to open stderr pipe: %v", err) + return false, fmt.Errorf("unable to open stderr pipe: %v", err) } defer func() { _ = stderrReader.Close() diff --git a/services/pull/patch_unmerged.go b/services/pull/patch_unmerged.go new file mode 100644 index 0000000000000..1ada229b053c4 --- /dev/null +++ b/services/pull/patch_unmerged.go @@ -0,0 +1,180 @@ +// Copyright 2021 The Gitea Authors. +// All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package pull + +import ( + "bufio" + "context" + "fmt" + "io" + "os" + "strconv" + "strings" + + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/log" +) + +// lsFileLine is a Quadruplet struct (+error) representing a partially parsed line from ls-files +type lsFileLine struct { + mode string + sha string + stage int + path string + err error +} + +// SameAs checks if two lsFileLines are referring to the same path, sha and mode (ignoring stage) +func (line *lsFileLine) SameAs(other *lsFileLine) bool { + if line == nil || other == nil { + return false + } + + if line.err != nil || other.err != nil { + return false + } + + return line.mode == other.mode && + line.sha == other.sha && + line.path == other.path +} + +// readUnmergedLsFileLines calls git ls-files -u -z and parses the lines into mode-sha-stage-path quadruplets +// it will push these to the provided channel closing it at the end +func readUnmergedLsFileLines(ctx context.Context, tmpBasePath string, outputChan chan *lsFileLine) { + defer func() { + // Always close the outputChan at the end of this function + close(outputChan) + }() + + lsFilesReader, lsFilesWriter, err := os.Pipe() + if err != nil { + log.Error("Unable to open stderr pipe: %v", err) + outputChan <- &lsFileLine{err: fmt.Errorf("unable to open stderr pipe: %v", err)} + return + } + defer func() { + _ = lsFilesWriter.Close() + _ = lsFilesReader.Close() + }() + + stderr := &strings.Builder{} + err = git.NewCommandContext(ctx, "ls-files", "-u", "-z"). + RunInDirTimeoutEnvFullPipelineFunc( + nil, -1, tmpBasePath, + lsFilesWriter, stderr, nil, + func(_ context.Context, _ context.CancelFunc) error { + _ = lsFilesWriter.Close() + defer func() { + _ = lsFilesReader.Close() + }() + bufferedReader := bufio.NewReader(lsFilesReader) + + for { + line, err := bufferedReader.ReadString('\000') + if err != nil { + if err == io.EOF { + return nil + } + return err + } + toemit := &lsFileLine{} + + split := strings.SplitN(line, " ", 3) + if len(split) < 3 { + return fmt.Errorf("malformed line: %s", line) + } + toemit.mode = split[0] + toemit.sha = split[1] + + if len(split[2]) < 4 { + return fmt.Errorf("malformed line: %s", line) + } + + toemit.stage, err = strconv.Atoi(split[2][0:1]) + if err != nil { + return fmt.Errorf("malformed line: %s", line) + } + + toemit.path = split[2][2 : len(split[2])-1] + outputChan <- toemit + } + }) + + if err != nil { + outputChan <- &lsFileLine{err: fmt.Errorf("git ls-files -u -z: %v", git.ConcatenateError(err, stderr.String()))} + } +} + +// unmergedFile is triple (+error) of lsFileLines split into stages 1,2 & 3. +type unmergedFile struct { + stage1 *lsFileLine + stage2 *lsFileLine + stage3 *lsFileLine + err error +} + +// unmergedFiles will collate the output from readUnstagedLsFileLines in to file triplets and send them +// to the provided channel, closing at the end. +func unmergedFiles(ctx context.Context, tmpBasePath string, unmerged chan *unmergedFile) { + defer func() { + // Always close the channel + close(unmerged) + }() + + ctx, cancel := context.WithCancel(ctx) + lsFileLineChan := make(chan *lsFileLine, 10) // give lsFileLineChan a buffer + go readUnmergedLsFileLines(ctx, tmpBasePath, lsFileLineChan) + defer func() { + cancel() + for range lsFileLineChan { + // empty channel + } + }() + + next := &unmergedFile{} + for line := range lsFileLineChan { + if line.err != nil { + log.Error("Unable to run ls-files -u -z! Error: %v", line.err) + unmerged <- &unmergedFile{err: fmt.Errorf("unable to run ls-files -u -z! Error: %v", line.err)} + return + } + + // stages are always emitted 1,2,3 but sometimes 1, 2 or 3 are dropped + switch line.stage { + case 0: + // Should not happen as this represents succesfully merged file - we will tolerate and ignore though + case 1: + if next.stage1 != nil { + // We need to handle the unstaged file stage1,stage2,stage3 + unmerged <- next + } + next = &unmergedFile{stage1: line} + case 2: + if next.stage3 != nil || next.stage2 != nil || (next.stage1 != nil && next.stage1.path != line.path) { + // We need to handle the unstaged file stage1,stage2,stage3 + unmerged <- next + next = &unmergedFile{} + } + next.stage2 = line + case 3: + if next.stage3 != nil || (next.stage1 != nil && next.stage1.path != line.path) || (next.stage2 != nil && next.stage2.path != line.path) { + // We need to handle the unstaged file stage1,stage2,stage3 + unmerged <- next + next = &unmergedFile{} + } + next.stage3 = line + default: + log.Error("Unexpected stage %d for path %s in run ls-files -u -z!", line.stage, line.path) + unmerged <- &unmergedFile{err: fmt.Errorf("unexpected stage %d for path %s in git ls-files -u -z", line.stage, line.path)} + return + } + } + // We need to handle the unstaged file stage1,stage2,stage3 + if next.stage1 != nil || next.stage2 != nil || next.stage3 != nil { + unmerged <- next + } +} From cb4e9e16f79d21eb43e5ee5b313526148ad0cc3c Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 17 Dec 2021 16:56:13 +0000 Subject: [PATCH 3/5] and handle empty patches too Signed-off-by: Andrew Thornton --- services/pull/patch.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/services/pull/patch.go b/services/pull/patch.go index 9b10ab5c452b3..ea4ecfc7950d6 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -254,6 +254,22 @@ func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath } if !conflict { + treeHash, err := git.NewCommandContext(ctx, "write-tree").RunInDir(tmpBasePath) + if err != nil { + return false, err + } + treeHash = strings.TrimSpace(treeHash) + baseTree, err := gitRepo.GetTree("base") + if err != nil { + return false, err + } + if treeHash == baseTree.ID.String() { + log.Debug("PullRequest[%d]: Patch is empty - ignoring", pr.ID) + pr.Status = models.PullRequestStatusEmpty + pr.ConflictedFiles = []string{} + pr.ChangedProtectedFiles = []string{} + } + return false, nil } From 507ede427474cdede80e7cac7a4320da4407580e Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 17 Dec 2021 17:02:54 +0000 Subject: [PATCH 4/5] placate lint Signed-off-by: Andrew Thornton --- services/pull/patch.go | 20 +++++++++++++------- services/pull/patch_unmerged.go | 2 +- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/services/pull/patch.go b/services/pull/patch.go index ea4ecfc7950d6..dc86be5e844b6 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -11,6 +11,7 @@ import ( "fmt" "io" "os" + "path/filepath" "strings" "code.gitea.io/gitea/models" @@ -164,19 +165,26 @@ func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, g return fmt.Errorf("unable to get root object: %s at path: %s for merging. Error: %w", file.stage1.sha, file.stage1.path, err) } root = strings.TrimSpace(root) - defer util.Remove(root) + defer func() { + _ = util.Remove(filepath.Join(tmpBasePath, root)) + }() + base, err := git.NewCommandContext(ctx, "unpack-file", file.stage2.sha).RunInDir(tmpBasePath) if err != nil { return fmt.Errorf("unable to get base object: %s at path: %s for merging. Error: %w", file.stage2.sha, file.stage2.path, err) } - base = strings.TrimSpace(base) - defer util.Remove(base) + base = strings.TrimSpace(filepath.Join(tmpBasePath, base)) + defer func() { + _ = util.Remove(base) + }() head, err := git.NewCommandContext(ctx, "unpack-file", file.stage3.sha).RunInDir(tmpBasePath) if err != nil { return fmt.Errorf("unable to get head object:%s at path: %s for merging. Error: %w", file.stage3.sha, file.stage3.path, err) } head = strings.TrimSpace(head) - defer util.Remove(head) + defer func() { + _ = util.Remove(filepath.Join(tmpBasePath, head)) + }() // now git merge-file annoyingly takes a different order to the merge-tree ... _, conflictErr := git.NewCommandContext(ctx, "merge-file", base, root, head).RunInDir(tmpBasePath) @@ -199,8 +207,6 @@ func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, g markConflict(file.stage2.path) } else if file.stage3 != nil { markConflict(file.stage2.path) - } else { - // This shouldn't happen! } } return nil @@ -273,7 +279,7 @@ func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath return false, nil } - // OK read-tree has failed so we need to try a different thing - this might actually suceed where the above fails due to whitespace handling. + // 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. // 1. Create a plain patch from head to base tmpPatchFile, err := os.CreateTemp("", "patch") diff --git a/services/pull/patch_unmerged.go b/services/pull/patch_unmerged.go index 1ada229b053c4..e15b299350c15 100644 --- a/services/pull/patch_unmerged.go +++ b/services/pull/patch_unmerged.go @@ -146,7 +146,7 @@ func unmergedFiles(ctx context.Context, tmpBasePath string, unmerged chan *unmer // stages are always emitted 1,2,3 but sometimes 1, 2 or 3 are dropped switch line.stage { case 0: - // Should not happen as this represents succesfully merged file - we will tolerate and ignore though + // Should not happen as this represents successfully merged file - we will tolerate and ignore though case 1: if next.stage1 != nil { // We need to handle the unstaged file stage1,stage2,stage3 From 7a523ddc7e36fc6d038340e13f3360e3c2819408 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 17 Dec 2021 17:40:00 +0000 Subject: [PATCH 5/5] use errConflict instead callback Signed-off-by: Andrew Thornton --- services/pull/patch.go | 58 ++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/services/pull/patch.go b/services/pull/patch.go index dc86be5e844b6..4bc875f630455 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -101,16 +101,23 @@ func TestPatch(pr *models.PullRequest) error { return nil } -func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, gitRepo *git.Repository, markConflict func(string)) error { +type errMergeConflict struct { + filename string +} + +func (e *errMergeConflict) Error() string { + return fmt.Sprintf("conflict detected at: %s", e.filename) +} + +func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, gitRepo *git.Repository) error { switch { case file.stage1 != nil && (file.stage2 == nil || file.stage3 == nil): // 1. Deleted in one or both: // - // Conflict <==> the stage1 !Equivalent to the undeleted one + // Conflict <==> the stage1 !SameAs to the undeleted one if (file.stage2 != nil && !file.stage1.SameAs(file.stage2)) || (file.stage3 != nil && !file.stage1.SameAs(file.stage3)) { // Conflict! - markConflict(file.stage1.path) - return nil + return &errMergeConflict{file.stage1.path} } // Not a genuine conflict and we can simply remove the file from the index @@ -127,8 +134,7 @@ func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, g // 3. Added in both with the same sha but the modes are different // // Conflict! (Not sure that this can actually happen but we should handle) - markConflict(file.stage2.path) - return nil + return &errMergeConflict{file.stage2.path} case file.stage1 == nil && file.stage2 == nil && file.stage3 != nil: // 4. Added in theirs but not ours: // @@ -138,25 +144,21 @@ func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, g // 5. Created by new in both // // Conflict! - markConflict(file.stage2.path) - return nil + return &errMergeConflict{file.stage2.path} case file.stage2 != nil && file.stage3 != nil: // 5. Modified in both - we should try to merge in the changes but first: // if file.stage2.mode == "120000" || file.stage3.mode == "120000" { // 5a. Conflicting symbolic link change - markConflict(file.stage2.path) - return nil + return &errMergeConflict{file.stage2.path} } if file.stage2.mode == "160000" || file.stage3.mode == "160000" { // 5b. Conflicting submodule change - markConflict(file.stage2.path) - return nil + return &errMergeConflict{file.stage2.path} } if file.stage2.mode != file.stage3.mode { // 5c. Conflicting mode change - markConflict(file.stage2.path) - return nil + return &errMergeConflict{file.stage2.path} } // Need to get the objects from the object db to attempt to merge @@ -189,8 +191,7 @@ func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, g // now git merge-file annoyingly takes a different order to the merge-tree ... _, conflictErr := git.NewCommandContext(ctx, "merge-file", base, root, head).RunInDir(tmpBasePath) if conflictErr != nil { - markConflict(file.stage2.path) - return nil + return &errMergeConflict{file.stage2.path} } // base now contains the merged data @@ -202,11 +203,11 @@ func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, g return gitRepo.AddObjectToIndex(file.stage2.mode, git.MustIDFromString(hash), file.stage2.path) default: if file.stage1 != nil { - markConflict(file.stage1.path) + return &errMergeConflict{file.stage1.path} } else if file.stage2 != nil { - markConflict(file.stage2.path) + return &errMergeConflict{file.stage2.path} } else if file.stage3 != nil { - markConflict(file.stage2.path) + return &errMergeConflict{file.stage3.path} } } return nil @@ -235,14 +236,6 @@ func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath numberOfConflicts := 0 conflict := false - markConflict := func(filepath string) { - log.Trace("Conflict: %s in PR[%d] %s/%s#%d", filepath, pr.ID, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, pr.Index) - conflict = true - if numberOfConflicts < 10 { - pr.ConflictedFiles = append(pr.ConflictedFiles, filepath) - } - numberOfConflicts++ - } for file := range unmerged { if file == nil { @@ -254,7 +247,16 @@ func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath } // OK now we have the unmerged file triplet attempt to merge it - if err := attemptMerge(ctx, file, tmpBasePath, gitRepo, markConflict); err != nil { + if err := attemptMerge(ctx, file, tmpBasePath, gitRepo); err != nil { + if conflictErr, ok := err.(*errMergeConflict); ok { + log.Trace("Conflict: %s in PR[%d] %s/%s#%d", conflictErr.filename, pr.ID, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, pr.Index) + conflict = true + if numberOfConflicts < 10 { + pr.ConflictedFiles = append(pr.ConflictedFiles, conflictErr.filename) + } + numberOfConflicts++ + continue + } return false, err } }