From 210b096da72acacbddc28d65f6ce2502772307f1 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 29 Jul 2022 15:37:18 +0200 Subject: [PATCH] Ensure that all unmerged files are merged when conflict checking (#20528) (#20536) There is a subtle bug in the code relating to collating the results of `git ls-files -u -z` in `unmergedFiles()`. The code here makes the mistake of assuming that every unmerged file will always have a stage 1 conflict, and this results in conflicts that occur in stage 3 only being dropped. This PR simply adjusts this code to ensure that any empty unmergedFile will always be passed down the channel. The PR also adds a lot of Trace commands to attempt to help find future bugs in this code. Fix #19527 Signed-off-by: Andrew Thornton Co-authored-by: zeripath --- services/pull/patch.go | 4 +++- services/pull/patch_unmerged.go | 25 ++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/services/pull/patch.go b/services/pull/patch.go index bb09acc89f..32895b2e78 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -124,6 +124,7 @@ func (e *errMergeConflict) Error() string { } func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, gitRepo *git.Repository) error { + log.Trace("Attempt to merge:\n%v", file) switch { case file.stage1 != nil && (file.stage2 == nil || file.stage3 == nil): // 1. Deleted in one or both: @@ -295,7 +296,8 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * var treeHash string treeHash, _, err = git.NewCommand(ctx, "write-tree").RunStdString(&git.RunOpts{Dir: tmpBasePath}) if err != nil { - return false, err + lsfiles, _, _ := git.NewCommand(ctx, "ls-files", "-u").RunStdString(&git.RunOpts{Dir: tmpBasePath}) + return false, fmt.Errorf("unable to write unconflicted tree: %w\n`git ls-files -u`:\n%s", err, lsfiles) } treeHash = strings.TrimSpace(treeHash) baseTree, err := gitRepo.GetTree("base") diff --git a/services/pull/patch_unmerged.go b/services/pull/patch_unmerged.go index 3839419142..465465d0da 100644 --- a/services/pull/patch_unmerged.go +++ b/services/pull/patch_unmerged.go @@ -42,6 +42,17 @@ func (line *lsFileLine) SameAs(other *lsFileLine) bool { line.path == other.path } +// String provides a string representation for logging +func (line *lsFileLine) String() string { + if line == nil { + return "" + } + if line.err != nil { + return fmt.Sprintf("%d %s %s %s %v", line.stage, line.mode, line.path, line.sha, line.err) + } + return fmt.Sprintf("%d %s %s %s", line.stage, line.mode, line.path, line.sha) +} + // 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) { @@ -118,6 +129,17 @@ type unmergedFile struct { err error } +// String provides a string representation of the an unmerged file for logging +func (u *unmergedFile) String() string { + if u == nil { + return "" + } + if u.err != nil { + return fmt.Sprintf("error: %v\n%v\n%v\n%v", u.err, u.stage1, u.stage2, u.stage3) + } + return fmt.Sprintf("%v\n%v\n%v", u.stage1, u.stage2, u.stage3) +} + // 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) { @@ -138,6 +160,7 @@ func unmergedFiles(ctx context.Context, tmpBasePath string, unmerged chan *unmer next := &unmergedFile{} for line := range lsFileLineChan { + log.Trace("Got line: %v Current State:\n%v", line, next) 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)} @@ -149,7 +172,7 @@ func unmergedFiles(ctx context.Context, tmpBasePath string, unmerged chan *unmer case 0: // Should not happen as this represents successfully merged file - we will tolerate and ignore though case 1: - if next.stage1 != nil { + if next.stage1 != nil || next.stage2 != nil || next.stage3 != nil { // We need to handle the unstaged file stage1,stage2,stage3 unmerged <- next }