fixer: Address rename conflicts with explanation (#1120)

* fixer: Address rename conflicts with explanation

Fixes #1119

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

* 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>

* fix: Address PR comments

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

---------

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

cmd/fix.go

@@ -313,11 +313,27 @@ func fix(args []string, params *fixCommandParams) error {
 		return fmt.Errorf("failed to create file provider: %w", err)
 	}
 
+	r, err := fixer.ReporterForFormat(params.format, outputWriter)
+	if err != nil {
+		return fmt.Errorf("failed to create reporter for format %s: %w", params.format, err)
+	}
+
+	r.SetDryRun(params.dryRun)
+
 	fixReport, err := f.Fix(ctx, &l, fileProvider)
 	if err != nil {
 		return fmt.Errorf("failed to fix: %w", err)
 	}
 
+	if fixReport.HasConflicts() {
+		err = r.Report(fixReport)
+		if err != nil {
+			return fmt.Errorf("failed to output fix report: %w", err)
+		}
+
+		return errors.New("fixing failed due to conflicts")
+	}
+
 	gitRepo, err := git.FindGitRepo(args...)
 	if err != nil {
 		return fmt.Errorf("failed to establish git repo: %w", err)
@@ -428,13 +444,6 @@ please run fix from a clean state to support the use of git checkout for undo`,
 		}
 	}
 
-	r, err := fixer.ReporterForFormat(params.format, outputWriter)
-	if err != nil {
-		return fmt.Errorf("failed to create reporter for format %s: %w", params.format, err)
-	}
-
-	r.SetDryRun(params.dryRun)
-
 	err = r.Report(fixReport)
 	if err != nil {
 		return fmt.Errorf("failed to output fix report: %w", err)

e2e/cli_test.go

@@ -938,6 +938,79 @@ test_allow := true
 	}
 }
 
+func TestFixWithConflicts(t *testing.T) {
+	t.Parallel()
+
+	stdout := bytes.Buffer{}
+	stderr := bytes.Buffer{}
+	td := t.TempDir()
+
+	initialState := map[string]string{
+		".regal/config.yaml": "", // needed to find the root in the right place
+		// this file is in the correct location
+		"foo/foo.rego": `package foo
+
+import rego.v1
+`,
+		// this file should be at foo/foo.rego, but that file already exists
+		"quz/foo.rego": `package foo
+
+import rego.v1
+`,
+		// these three files should all be at bar/bar.rego, but they cannot all be moved there
+		"foo/bar.rego": `package bar
+
+import rego.v1
+`,
+		"baz/bar.rego": `package bar
+
+import rego.v1
+`,
+		"bax/foo/wow/bar.rego": `package bar
+
+import rego.v1
+`,
+	}
+
+	for file, content := range initialState {
+		mustWriteToFile(t, filepath.Join(td, file), string(content))
+	}
+
+	// --force is required to make the changes when there is no git repo
+	err := regal(&stdout, &stderr)("fix", "--force", td)
+
+	// 0 exit status is expected as all violations should have been fixed
+	expectExitCode(t, err, 1, &stdout, &stderr)
+
+	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)
+
+	if act := stdout.String(); expStdout != act {
+		t.Errorf("expected stdout:\n%s\ngot\n%s", expStdout, act)
+	}
+
+	for file, expectedContent := range initialState {
+		bs, err := os.ReadFile(filepath.Join(td, file))
+		if err != nil {
+			t.Fatalf("failed to read %s: %v", file, err)
+		}
+
+		if act := string(bs); expectedContent != act {
+			t.Errorf("expected %s contents:\n%s\ngot\n%s", file, expectedContent, act)
+		}
+	}
+}
+
 // verify fix for https://github.com/StyraInc/regal/issues/1082
 func TestLintAnnotationCustomAttributeMultipleItems(t *testing.T) {
 	t.Parallel()

pkg/fixer/fileprovider/inmem.go

@@ -11,6 +11,15 @@ import (
 	"github.com/styrainc/regal/pkg/rules"
 )
 
+type RenameConflictError struct {
+	From string
+	To   string
+}
+
+func (e RenameConflictError) Error() string {
+	return fmt.Sprintf("rename conflict: %q cannot be renamed as the target location %q already exists", e.From, e.To)
+}
+
 type InMemoryFileProvider struct {
 	files         map[string][]byte
 	modifiedFiles map[string]struct{}
@@ -78,7 +87,10 @@ func (p *InMemoryFileProvider) Rename(from, to string) error {
 
 	_, ok = p.files[to]
 	if ok {
-		return fmt.Errorf("rename conflict: file %s already exists", to)
+		return RenameConflictError{
+			From: from,
+			To:   to,
+		}
 	}
 
 	err := p.Put(to, content)

pkg/fixer/fileprovider/inmem_test.go

@@ -60,7 +60,9 @@ func TestRenameConflict(t *testing.T) {
 		t.Fatal("expected error")
 	}
 
-	if err.Error() != "rename conflict: file /bar/foo already exists" {
-		t.Fatalf("expected %s, got %s", "rename conflict", err.Error())
+	exp := `rename conflict: "/foo/bar/baz" cannot be renamed as the target location "/bar/foo" already exists`
+
+	if got := err.Error(); got != exp {
+		t.Fatalf("expected %s, got %s", exp, got)
 	}
 }