fixer: Output the source of conflicts in report

Now conflicts are reported by type. If they conflict with an existing
file, these are shown first. Then, conflicts created by more than one
file being moved to the same location are shown after that - if any.

Signed-off-by: Charlie Egan <charlie@styra.com>

e2e/cli_test.go

@@ -947,29 +947,28 @@ func TestFixWithConflicts(t *testing.T) {
 
 	initialState := map[string]string{
 		".regal/config.yaml": "", // needed to find the root in the right place
-		"foo/bar.rego": `package bar
+		// this file is in the correct location
+		"foo/foo.rego": `package foo
 
 import rego.v1
-
-allow if {
-	true
-}
 `,
-		"baz/bar.rego": `package bar
+		// this file should be at foo/foo.rego, but that file already exists
+		"quz/foo.rego": `package foo
 
 import rego.v1
+`,
+		// these thre files should all be at bar/bar.rego, but they cannot all be moved there
+		"foo/bar.rego": `package bar
 
-allow if {
-	true
-}
+import rego.v1
 `,
-		"bax/foo/bar/bar.rego": `package bar
+		"baz/bar.rego": `package bar
 
 import rego.v1
+`,
+		"bax/foo/wow/bar.rego": `package bar
 
-allow if {
-	true
-}
+import rego.v1
 `,
 	}
 
@@ -983,9 +982,15 @@ allow if {
 	// 0 exit status is expected as all violations should have been fixed
 	expectExitCode(t, err, 1, &stdout, &stderr)
 
-	expStdout := fmt.Sprintf(`Fix conflicts detected:
-In project root: %s
+	expStdout := fmt.Sprintf(`Source file conflicts:
+In project root: %[1]s
+Cannot overwrite existing file: foo/foo.rego
+- quz/foo.rego
+
+Many to one conflicts:
+In project root: %[1]s
 Cannot move multiple files to: bar/bar.rego
+- bax/foo/wow/bar.rego
 - baz/bar.rego
 - foo/bar.rego
 `, td)

pkg/fixer/fixer.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"path/filepath"
+	"slices"
 
 	"github.com/open-policy-agent/opa/ast"
 
@@ -194,6 +195,11 @@ func (f *Fixer) applyLinterFixes(
 		}
 	}
 
+	startingFiles, err := fp.List()
+	if err != nil {
+		return fmt.Errorf("failed to list files: %w", err)
+	}
+
 	for {
 		fixMadeInIteration := false
 
@@ -269,10 +275,7 @@ func (f *Fixer) applyLinterFixes(
 				// A conflict occurs if attempting to rename to an existing path
 				// which exists due to another renaming fix
 				if errors.As(err, &fileprovider.RenameConflictError{}) {
-					otherOldNames, ok := fixReport.movedFiles[to]
-					if ok && len(otherOldNames) > 0 {
-						isConflict = true
-					}
+					isConflict = true
 				}
 
 				if err != nil && !isConflict {
@@ -281,16 +284,19 @@ func (f *Fixer) applyLinterFixes(
 
 				fixReport.AddFileFix(to, fixResult)
 				fixReport.MergeFixes(to, from)
+				fixReport.RegisterOldPathForFile(to, from)
 
-				if err := fixReport.RegisterOldPathForFile(to, from); err != nil {
-					return fmt.Errorf("failed to register old path for file %s: %w", to, err)
-				}
-
-				// Clean the old file to prevent repeated fixes
 				if isConflict {
+					// Clean the old file to prevent repeated fixes
 					if err := fp.Delete(from); err != nil {
 						return fmt.Errorf("failed to delete file %s: %w", from, err)
 					}
+
+					if slices.Contains(startingFiles, to) {
+						fixReport.RegisterConflictSourceFile(fixResult.Root, to, from)
+					} else {
+						fixReport.RegisterConflictManyToOne(fixResult.Root, to, from)
+					}
 				}
 
 				fixMadeInIteration = true

pkg/fixer/report.go

@@ -10,15 +10,19 @@ import (
 // Report contains updated file contents and summary information about the fixes that were applied
 // during a fix operation.
 type Report struct {
-	totalFixes uint
-	fileFixes  map[string][]fixes.FixResult
-	movedFiles map[string][]string
+	totalFixes          uint
+	fileFixes           map[string][]fixes.FixResult
+	movedFiles          map[string][]string
+	conflictsManyToOne  map[string]map[string][]string
+	conflictsSourceFile map[string]map[string][]string
 }
 
 func NewReport() *Report {
 	return &Report{
-		fileFixes:  make(map[string][]fixes.FixResult),
-		movedFiles: make(map[string][]string),
+		fileFixes:           make(map[string][]fixes.FixResult),
+		movedFiles:          make(map[string][]string),
+		conflictsManyToOne:  make(map[string]map[string][]string),
+		conflictsSourceFile: make(map[string]map[string][]string),
 	}
 }
 
@@ -36,10 +40,8 @@ func (r *Report) MergeFixes(path1, path2 string) {
 	delete(r.fileFixes, path2)
 }
 
-func (r *Report) RegisterOldPathForFile(newPath, oldPath string) error {
+func (r *Report) RegisterOldPathForFile(newPath, oldPath string) {
 	r.movedFiles[newPath] = append(r.movedFiles[newPath], oldPath)
-
-	return nil
 }
 
 func (r *Report) OldPathForFile(newPath string) (string, bool) {
@@ -66,12 +68,30 @@ func (r *Report) TotalFixes() uint {
 	return r.totalFixes
 }
 
-func (r *Report) HasConflicts() bool {
-	for _, oldPaths := range r.movedFiles {
-		if len(oldPaths) > 1 {
-			return true
-		}
+func (r *Report) RegisterConflictManyToOne(root, newPath, oldPath string) {
+	if _, ok := r.conflictsManyToOne[root]; !ok {
+		r.conflictsManyToOne[root] = make(map[string][]string)
+	}
+
+	if _, ok := r.conflictsManyToOne[root][newPath]; !ok {
+		r.conflictsManyToOne[root][newPath] = make([]string, 0)
 	}
 
-	return false
+	r.conflictsManyToOne[root][newPath] = append(r.conflictsManyToOne[root][newPath], oldPath)
+}
+
+func (r *Report) RegisterConflictSourceFile(root, newPath, oldPath string) {
+	if _, ok := r.conflictsSourceFile[root]; !ok {
+		r.conflictsSourceFile[root] = make(map[string][]string)
+	}
+
+	if _, ok := r.conflictsSourceFile[root][newPath]; !ok {
+		r.conflictsSourceFile[root][newPath] = make([]string, 0)
+	}
+
+	r.conflictsSourceFile[root][newPath] = append(r.conflictsSourceFile[root][newPath], oldPath)
+}
+
+func (r *Report) HasConflicts() bool {
+	return len(r.conflictsManyToOne) > 0 || len(r.conflictsSourceFile) > 0
 }

pkg/fixer/reporter.go

@@ -44,46 +44,79 @@ func (r *PrettyReporter) SetDryRun(dryRun bool) {
 }
 
 func (r *PrettyReporter) ReportConflicts(fixReport *Report) error {
-	byRoot := make(map[string][]string)
+	roots := util.Keys(fixReport.conflictsSourceFile)
+	slices.Sort(roots)
 
-	for _, file := range fixReport.FixedFiles() {
-		fixes := fixReport.FixesForFile(file)
-		if len(fixes) == 0 {
-			continue
-		}
+	if len(roots) > 0 {
+		fmt.Fprintln(r.outputWriter, "Source file conflicts:")
 
-		root := fixes[0].Root
+		i := 0
 
-		byRoot[root] = append(byRoot[root], file)
-	}
+		for _, rootKey := range roots {
+			if i > 0 {
+				fmt.Fprintln(r.outputWriter)
+			}
 
-	i := 0
+			cs, ok := fixReport.conflictsSourceFile[rootKey]
+			if !ok {
+				continue
+			}
 
-	rootsSorted := util.Keys(byRoot)
+			conflictingFiles := util.Keys(cs)
+			slices.Sort(conflictingFiles)
 
-	slices.Sort(rootsSorted)
+			fmt.Fprintln(r.outputWriter, "In project root:", rootKey)
 
-	fmt.Fprintln(r.outputWriter, "Fix conflicts detected:")
+			for _, file := range conflictingFiles {
+				conflicts := fixReport.conflictsSourceFile[rootKey][file]
+				slices.Sort(conflicts)
 
-	for _, root := range rootsSorted {
-		if i > 0 {
+				fmt.Fprintln(r.outputWriter, "Cannot overwrite existing file:", strings.TrimPrefix(file, rootKey+"/"))
+
+				for _, oldPath := range conflicts {
+					fmt.Fprintln(r.outputWriter, "-", strings.TrimPrefix(oldPath, rootKey+"/"))
+				}
+			}
+		}
+	}
+
+	roots = util.Keys(fixReport.conflictsManyToOne)
+	slices.Sort(roots)
+
+	if len(roots) > 0 {
+		if len(fixReport.conflictsSourceFile) > 0 {
 			fmt.Fprintln(r.outputWriter)
 		}
 
-		fmt.Fprintln(r.outputWriter, "In project root:", root)
+		fmt.Fprintln(r.outputWriter, "Many to one conflicts:")
 
-		for _, file := range byRoot[root] {
-			oldPaths, ok := fixReport.movedFiles[file]
-			if !ok || len(oldPaths) < 2 {
+		i := 0
+
+		for _, rootKey := range roots {
+			if i > 0 {
+				fmt.Fprintln(r.outputWriter)
+			}
+
+			cs, ok := fixReport.conflictsManyToOne[rootKey]
+			if !ok {
 				continue
 			}
 
-			slices.Sort(oldPaths)
+			conflictingFiles := util.Keys(cs)
+			slices.Sort(conflictingFiles)
+
+			fmt.Fprintln(r.outputWriter, "In project root:", rootKey)
+
+			for _, file := range conflictingFiles {
+				fmt.Fprintln(r.outputWriter, "Cannot move multiple files to:", strings.TrimPrefix(file, rootKey+"/"))
 
-			fmt.Fprintln(r.outputWriter, "Cannot move multiple files to:", strings.TrimPrefix(file, root+"/"))
+				// get the old paths from the movedFiles since that includes all the files moved, not just the conflicting ones
+				oldPaths := fixReport.movedFiles[file]
+				slices.Sort(oldPaths)
 
-			for _, oldPath := range fixReport.movedFiles[file] {
-				fmt.Fprintln(r.outputWriter, "-", strings.TrimPrefix(oldPath, root+"/"))
+				for _, oldPath := range oldPaths {
+					fmt.Fprintln(r.outputWriter, "-", strings.TrimPrefix(oldPath, rootKey+"/"))
+				}
 			}
 		}
 	}