Add error type to mark conflicts during apply (#166)

bluekeyes · web-flow · commit c7d8914f25d4 · 2026-06-27T09:13:56.000-07:00
Apply now returns an error with type *Conflict if the error is because
of a conflict between the patch and the target. This allows callers to
respond to conflicts differently from other errors.
diff --git a/applier.go b/applier.go
@@ -7,6 +7,7 @@ import (
 	"encoding/base64"
 	"errors"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"strconv"
 	"strings"
@@ -48,6 +49,9 @@ func NewApplier(client *github.Client, repo Repository, c *github.Commit) *Appli
 // Apply applies the changes in a file, adds the result to the list of pending
 // tree entries, and returns the entry. If the application succeeds, Apply
 // creates a blob in the repository with the modified content.
+//
+// If the apply fails due to a conflict, Apply returns an error of type
+// *Conflict.
 func (a *Applier) Apply(ctx context.Context, f *gitdiff.File) (*github.TreeEntry, error) {
 	// TODO(bkeyes): validate file to make sure fields are consistent
 	// maybe two modes: validate and fix, where fix tries to set
@@ -91,10 +95,10 @@ func (a *Applier) applyCreate(ctx context.Context, f *gitdiff.File) (*github.Tre
 		return nil, err
 	}
 	if exists {
-		return nil, errors.New("existing entry for new file")
+		return nil, &Conflict{Type: ConflictNewFileExists, File: f.NewName}
 	}
 
-	c, err := base64Apply(nil, f)
+	c, err := base64Apply(nil, f.NewName, f)
 	if err != nil {
 		return nil, err
 	}
@@ -117,17 +121,17 @@ func (a *Applier) applyDelete(ctx context.Context, f *gitdiff.File) (*github.Tre
 		return nil, err
 	}
 	if !exists {
-		// because the rest of application is strict, return an error if the
+		// Because the rest of application is strict, return an error if the
 		// file was already deleted, since it indicates a conflict of some kind
-		return nil, errors.New("missing entry for deleted file")
+		return nil, &Conflict{Type: ConflictDeletedFileMissing, File: f.OldName}
 	}
 
 	data, _, err := a.client.Git.GetBlobRaw(ctx, a.owner, a.repo, entry.GetSHA())
 	if err != nil {
 		return nil, fmt.Errorf("get blob content failed: %w", err)
 	}
 
-	if err := gitdiff.Apply(ioutil.Discard, bytes.NewReader(data), f); err != nil {
+	if err := apply(ioutil.Discard, bytes.NewReader(data), f.OldName, f); err != nil {
 		return nil, err
 	}
 
@@ -147,7 +151,7 @@ func (a *Applier) applyModify(ctx context.Context, f *gitdiff.File) (*github.Tre
 		return nil, err
 	}
 	if !exists {
-		return nil, errors.New("no entry for modified file")
+		return nil, &Conflict{Type: ConflictModifiedFileMissing, File: f.OldName}
 	}
 
 	path := f.NewName
@@ -163,7 +167,7 @@ func (a *Applier) applyModify(ctx context.Context, f *gitdiff.File) (*github.Tre
 			return nil, fmt.Errorf("get blob content failed: %w", err)
 		}
 
-		c, err := base64Apply(data, f)
+		c, err := base64Apply(data, f.OldName, f)
 		if err != nil {
 			return nil, err
 		}
@@ -341,11 +345,13 @@ func findTreeEntry(t *github.Tree, name, entryType string) (*github.TreeEntry, b
 	return nil, false
 }
 
-func base64Apply(data []byte, f *gitdiff.File) (string, error) {
+// base64Apply applies the patch in f to data and returns the result as a
+// base64-encoded string.
+func base64Apply(data []byte, name string, f *gitdiff.File) (string, error) {
 	var b bytes.Buffer
 
 	enc := base64.NewEncoder(base64.StdEncoding, &b)
-	if err := gitdiff.Apply(enc, bytes.NewReader(data), f); err != nil {
+	if err := apply(enc, bytes.NewReader(data), name, f); err != nil {
 		return "", err
 	}
 	if err := enc.Close(); err != nil {
@@ -355,6 +361,24 @@ func base64Apply(data []byte, f *gitdiff.File) (string, error) {
 	return b.String(), nil
 }
 
+// apply runs gitdiff.Apply, wrapping any conflicts in patch2pr's Conflict type.
+func apply(dst io.Writer, src io.ReaderAt, name string, f *gitdiff.File) error {
+	if err := gitdiff.Apply(dst, src, f); err != nil {
+		var applyErr *gitdiff.ApplyError
+		var conflict *gitdiff.Conflict
+		if errors.As(err, &applyErr) && errors.As(err, &conflict) {
+			return &Conflict{
+				Type:  ConflictContent,
+				File:  name,
+				Line:  applyErr.Line,
+				cause: conflict,
+			}
+		}
+		return err
+	}
+	return nil
+}
+
 // TODO(bkeyes): extract this to go-gitdiff in some form?
 func getMode(f *gitdiff.File, existing *github.TreeEntry) string {
 	switch {
diff --git a/cmd/patch2pr/main.go b/cmd/patch2pr/main.go
@@ -249,11 +249,15 @@ func execute(ctx context.Context, client *github.Client, patchFiles []string, op
 	for _, patch := range allPatches {
 		for _, file := range patch.files {
 			if _, err := applier.Apply(ctx, file); err != nil {
-				name := file.NewName
-				if name == "" {
-					name = file.OldName
+				var namePart string
+				if !errors.Is(err, &patch2pr.Conflict{}) {
+					name := file.NewName
+					if name == "" {
+						name = file.OldName
+					}
+					namePart = name + ": "
 				}
-				return nil, fmt.Errorf("apply failed: %s: %w", name, err)
+				return nil, fmt.Errorf("apply failed: %s%w", namePart, err)
 			}
 		}
 
diff --git a/conflict.go b/conflict.go
@@ -0,0 +1,93 @@
+package patch2pr
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/bluekeyes/go-gitdiff/gitdiff"
+)
+
+// Conflict is the error returned when applying a patch fails because of a
+// conflict between the patch and the target.
+type Conflict struct {
+	// The type of the conflict.
+	Type ConflictType
+	// The path to the file in which the conflict occurs.
+	File string
+	// The line number of the conflict, if known.
+	Line int64
+
+	cause *gitdiff.Conflict
+}
+
+// ConflictType identifies the type of conflict.
+type ConflictType int
+
+const (
+	// ConflictUnspecified indicates the cause of the conflict was not specified.
+	ConflictUnspecified ConflictType = iota
+
+	// ConflictNewFileExists indicates the patch creates a new file that already exists.
+	ConflictNewFileExists
+
+	// ConflictDeletedFileMissing indicates the patch deletes a file that does not exist.
+	ConflictDeletedFileMissing
+
+	// ConflictModifiedFileMissing indicates the patch modifies a file that does not exist.
+	ConflictModifiedFileMissing
+
+	// ConflictContent indicates the patch content does not apply cleanly against the file's content.
+	ConflictContent
+)
+
+func (c *Conflict) Error() string {
+	var msg strings.Builder
+	if c.File != "" {
+		msg.WriteString(c.File)
+		if c.Line > 0 {
+			fmt.Fprintf(&msg, ":%d", c.Line)
+		}
+		msg.WriteString(": ")
+	}
+
+	switch c.Type {
+	case ConflictNewFileExists:
+		msg.WriteString("conflict: new file already exists")
+	case ConflictDeletedFileMissing:
+		msg.WriteString("conflict: deleted file does not exist")
+	case ConflictModifiedFileMissing:
+		msg.WriteString("conflict: modified file does not exist")
+	case ConflictContent:
+		if c.cause != nil {
+			msg.WriteString(c.cause.Error())
+		} else {
+			msg.WriteString("conflict: content")
+		}
+	default:
+		msg.WriteString("conflict")
+	}
+
+	return msg.String()
+}
+
+// Is returns true if all of the non-zero fields of target equal the values of
+// this Conflict. Passing an empty *Conflict{} always returns true.
+func (c *Conflict) Is(target error) bool {
+	if other, ok := target.(*Conflict); ok {
+		if other.Type == ConflictUnspecified {
+			if other.File == "" {
+				return true
+			}
+			return c.File == other.File
+		}
+		if other.File == "" {
+			return c.Type == other.Type
+		}
+		return other.Type == c.Type && other.File == c.File
+	}
+	return false
+}
+
+func (c *Conflict) Unwrap() error {
+	return c.cause
+}
diff --git a/conflict_test.go b/conflict_test.go
@@ -0,0 +1,108 @@
+package patch2pr
+
+import (
+	"errors"
+	"testing"
+)
+
+func TestConflictError(t *testing.T) {
+	for i, tc := range []struct {
+		Conflict Conflict
+		Error    string
+	}{
+		{
+			Conflict{},
+			"conflict",
+		},
+		{
+			Conflict{File: "path/to/file.txt"},
+			"path/to/file.txt: conflict",
+		},
+		{
+			Conflict{File: "path/to/file.txt", Type: ConflictNewFileExists},
+			"path/to/file.txt: conflict: new file already exists",
+		},
+		{
+			Conflict{File: "path/to/file.txt", Type: ConflictDeletedFileMissing},
+			"path/to/file.txt: conflict: deleted file does not exist",
+		},
+		{
+			Conflict{File: "path/to/file.txt", Type: ConflictModifiedFileMissing},
+			"path/to/file.txt: conflict: modified file does not exist",
+		},
+		{
+			Conflict{File: "path/to/file.txt", Type: ConflictContent},
+			"path/to/file.txt: conflict: content",
+		},
+		{
+			Conflict{File: "path/to/file.txt", Line: 23, Type: ConflictContent},
+			"path/to/file.txt:23: conflict: content",
+		},
+	} {
+		want := tc.Error
+		if got := tc.Conflict.Error(); got != want {
+			t.Errorf("case %d: Error(): want %q, got %q", i, want, got)
+		}
+	}
+}
+
+func TestConflictIs(t *testing.T) {
+	defaultConflict := Conflict{
+		Type: ConflictModifiedFileMissing,
+		File: "path/to/file.txt",
+	}
+
+	for name, tc := range map[string]struct {
+		Conflict Conflict
+		Target   error
+		Match    bool
+	}{
+		"nil": {
+			Conflict: defaultConflict,
+			Target:   nil,
+			Match:    false,
+		},
+		"otherType": {
+			Conflict: defaultConflict,
+			Target:   errors.New("different error"),
+			Match:    false,
+		},
+		"emptyConflictMatches": {
+			Conflict: defaultConflict,
+			Target:   &Conflict{},
+			Match:    true,
+		},
+		"typeOnlyMatch": {
+			Conflict: defaultConflict,
+			Target:   &Conflict{Type: ConflictModifiedFileMissing},
+			Match:    true,
+		},
+		"fileOnlyMatch": {
+			Conflict: defaultConflict,
+			Target:   &Conflict{File: "path/to/file.txt"},
+			Match:    true,
+		},
+		"typeAndFileMatch": {
+			Conflict: defaultConflict,
+			Target:   &Conflict{Type: ConflictModifiedFileMissing, File: "path/to/file.txt"},
+			Match:    true,
+		},
+		"differentType": {
+			Conflict: defaultConflict,
+			Target:   &Conflict{Type: ConflictContent},
+			Match:    false,
+		},
+		"differentFile": {
+			Conflict: defaultConflict,
+			Target:   &Conflict{File: "path/to/other/file.txt"},
+			Match:    false,
+		},
+	} {
+		want := tc.Match
+		t.Run(name, func(t *testing.T) {
+			if got := tc.Conflict.Is(tc.Target); got != want {
+				t.Errorf("Is(target): want %t, got %t", want, got)
+			}
+		})
+	}
+}
diff --git a/graphql_applier.go b/graphql_applier.go
@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"context"
 	"encoding/base64"
-	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -81,6 +80,9 @@ func (a *GraphQLApplier) SetV3Client(client *github.Client) {
 // When given an unsupported patch, Apply returns an error such that
 // IsUnsupported(err) is true. Setting a V3 client with SetV3Client allows
 // Apply to process some patches that are otherwise unsupported.
+//
+// If the apply fails due to a conflict, Apply returns an error of type
+// *Conflict.
 func (a *GraphQLApplier) Apply(ctx context.Context, f *gitdiff.File) error {
 	// As of 2021-09-22, createCommitOnBranch handles file modes
 	// inconsistently:
@@ -122,11 +124,11 @@ func (a *GraphQLApplier) applyCreate(ctx context.Context, f *gitdiff.File) error
 		return err
 	}
 	if exists {
-		return errors.New("existing entry for new file")
+		return &Conflict{Type: ConflictNewFileExists, File: f.NewName}
 	}
 
 	var b bytes.Buffer
-	if err := gitdiff.Apply(&b, bytes.NewReader(nil), f); err != nil {
+	if err := apply(&b, bytes.NewReader(nil), f.NewName, f); err != nil {
 		return err
 	}
 
@@ -141,12 +143,12 @@ func (a *GraphQLApplier) applyDelete(ctx context.Context, f *gitdiff.File) error
 		return err
 	}
 	if !exists {
-		// because the rest of application is strict, return an error if the
+		// Because the rest of application is strict, return an error if the
 		// file was already deleted, since it indicates a conflict of some kind
-		return errors.New("missing entry for deleted file")
+		return &Conflict{Type: ConflictDeletedFileMissing, File: f.OldName}
 	}
 
-	if err := gitdiff.Apply(ioutil.Discard, bytes.NewReader(data), f); err != nil {
+	if err := apply(ioutil.Discard, bytes.NewReader(data), f.OldName, f); err != nil {
 		return err
 	}
 
@@ -160,12 +162,12 @@ func (a *GraphQLApplier) applyModify(ctx context.Context, f *gitdiff.File) error
 		return err
 	}
 	if !exists {
-		return errors.New("no entry for modified file")
+		return &Conflict{Type: ConflictModifiedFileMissing, File: f.OldName}
 	}
 
 	if len(f.TextFragments) > 0 || f.BinaryFragment != nil {
 		var b bytes.Buffer
-		if err := gitdiff.Apply(&b, bytes.NewReader(data), f); err != nil {
+		if err := apply(&b, bytes.NewReader(data), f.OldName, f); err != nil {
 			return err
 		}
 		data = b.Bytes()

Original file line number	Diff line number	Diff line change
`@@ -249,11 +249,15 @@ func execute(ctx context.Context, client *github.Client, patchFiles []string, op`
`249`	`249`	`for _, patch := range allPatches {`
`250`	`250`	`for _, file := range patch.files {`
`251`	`251`	`if _, err := applier.Apply(ctx, file); err != nil {`
`252`		`- name := file.NewName`
`253`		`- if name == "" {`
`254`		`- name = file.OldName`
	`252`	`+ var namePart string`
	`253`	`+ if !errors.Is(err, &patch2pr.Conflict{}) {`
	`254`	`+ name := file.NewName`
	`255`	`+ if name == "" {`
	`256`	`+ name = file.OldName`
	`257`	`+ }`
	`258`	`+ namePart = name + ": "`
`255`	`259`	`}`
`256`		`- return nil, fmt.Errorf("apply failed: %s: %w", name, err)`
	`260`	`+ return nil, fmt.Errorf("apply failed: %s%w", namePart, err)`
`257`	`261`	`}`
`258`	`262`	`}`
`259`	`263`