From ab5e88b16c550ace87be2c00a66e637925aa7732 Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Sat, 27 Jun 2026 11:06:12 -0400 Subject: [PATCH 1/3] Add error type to mark conflicts during apply 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. --- applier.go | 41 ++++++++++++++---- conflict.go | 93 ++++++++++++++++++++++++++++++++++++++++ conflict_test.go | 108 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 233 insertions(+), 9 deletions(-) create mode 100644 conflict.go create mode 100644 conflict_test.go diff --git a/applier.go b/applier.go index 4730d70..1cdfd55 100644 --- a/applier.go +++ b/applier.go @@ -7,6 +7,7 @@ import ( "encoding/base64" "errors" "fmt" + "io" "io/ioutil" "strconv" "strings" @@ -48,6 +49,8 @@ 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 +94,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,9 +120,9 @@ 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()) @@ -127,7 +130,7 @@ func (a *Applier) applyDelete(ctx context.Context, f *gitdiff.File) (*github.Tre 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 +150,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 +166,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 +344,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 +360,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/conflict.go b/conflict.go new file mode 100644 index 0000000..2223668 --- /dev/null +++ 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 new file mode 100644 index 0000000..c5a7866 --- /dev/null +++ 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) + } + }) + } +} From 5c6187db031006e4f19bae084079b923a4c06bfa Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Sat, 27 Jun 2026 12:02:01 -0400 Subject: [PATCH 2/3] Account for conflict format in command --- applier.go | 2 +- cmd/patch2pr/main.go | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/applier.go b/applier.go index 1cdfd55..b91639c 100644 --- a/applier.go +++ b/applier.go @@ -360,7 +360,7 @@ func base64Apply(data []byte, name string, f *gitdiff.File) (string, error) { return b.String(), nil } -// apply runs gitdiff.Apply, wrapping any conflicts in patch2pr's Conflict type. +// 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 diff --git a/cmd/patch2pr/main.go b/cmd/patch2pr/main.go index 66b7781..025f788 100644 --- 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) } } From a268e321f70dec1aba4f352e82305e486dd0f763 Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Sat, 27 Jun 2026 12:10:17 -0400 Subject: [PATCH 3/3] Use new types in GraphQL applier --- applier.go | 3 ++- graphql_applier.go | 18 ++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/applier.go b/applier.go index b91639c..08d093f 100644 --- a/applier.go +++ b/applier.go @@ -50,7 +50,8 @@ func NewApplier(client *github.Client, repo Repository, c *github.Commit) *Appli // 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. +// 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 diff --git a/graphql_applier.go b/graphql_applier.go index fc4474c..6d1a17d 100644 --- 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()