From d5d101e4c0027b2f2321055f96ef0bda3b6751de Mon Sep 17 00:00:00 2001 From: nmiyake Date: Wed, 30 Sep 2026 10:22:02 -0700 Subject: [PATCH 1/2] Add support for empty commits Adds a new "SetAllowEmptyCommits" option that, when provided to the Applier with a value of "true", makes the Applier accept empty commits. Empty commits are valid and can be expressed as patches, so the new behavior is conceptually consistent. The option approach is taken to preserve backwards compatibility for existing behavior. --- applier.go | 16 +++-- applier_empty_test.go | 149 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 4 deletions(-) create mode 100644 applier_empty_test.go diff --git a/applier.go b/applier.go index 1091749..45cb798 100644 --- a/applier.go +++ b/applier.go @@ -32,7 +32,8 @@ type Applier struct { entries map[string]*github.TreeEntry uncommitted bool - applyOptions []gitdiff.ApplyOption + applyOptions []gitdiff.ApplyOption + allowEmptyCommits bool } // NewApplier creates a new Applier for a repository. The Applier applies @@ -53,6 +54,13 @@ func (a *Applier) SetApplyOptions(opts ...gitdiff.ApplyOption) { a.applyOptions = opts } +// SetAllowEmptyCommits controls whether Commit may create a commit without +// pending changes. Empty commits reuse the current tree. The default is false. +// This setting is preserved across calls to Commit and Reset. +func (a *Applier) SetAllowEmptyCommits(allow bool) { + a.allowEmptyCommits = allow +} + // 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. @@ -231,8 +239,8 @@ func (a *Applier) CreateTree(ctx context.Context) (*github.Tree, error) { // Commit commits the latest tree, optionally using the details in tmpl and // header. If there are pending tree entries, it calls CreateTree before -// creating the commit. It returns an error if there are no pending trees or -// tree entries. +// creating the commit. If there are no pending trees or tree entries, it returns +// an error unless empty commits are enabled with [Applier.SetAllowEmptyCommits]. // // If tmpl is not nil, Commit uses it as a template for the new commit, // overwriting fields as needed. If header is not nil, Commit uses it to set @@ -243,7 +251,7 @@ func (a *Applier) CreateTree(ctx context.Context) (*github.Tree, error) { // message, the current time, and the authenticated user as needed for the // commit details. func (a *Applier) Commit(ctx context.Context, tmpl *github.Commit, header *gitdiff.PatchHeader) (*github.Commit, error) { - if !a.uncommitted && len(a.entries) == 0 { + if !a.allowEmptyCommits && !a.uncommitted && len(a.entries) == 0 { return nil, errors.New("no pending tree or tree entries") } if len(a.entries) > 0 { diff --git a/applier_empty_test.go b/applier_empty_test.go new file mode 100644 index 0000000..39d555a --- /dev/null +++ b/applier_empty_test.go @@ -0,0 +1,149 @@ +package patch2pr + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "reflect" + "strings" + "testing" + "time" + + "github.com/bluekeyes/go-gitdiff/gitdiff" + "github.com/google/go-github/v92/github" +) + +func TestApplierEmptyCommitDisabled(t *testing.T) { + for _, tc := range []struct { + name string + settings []bool + }{ + {name: "default"}, + {name: "explicitly disabled", settings: []bool{false}}, + {name: "disabled after enabling", settings: []bool{true, false}}, + } { + t.Run(tc.name, func(t *testing.T) { + base := &github.Commit{ + SHA: new("base"), + Tree: &github.Tree{SHA: new("tree")}, + } + applier := NewApplier(nil, Repository{Owner: "owner", Name: "repo"}, base) + for _, enabled := range tc.settings { + applier.SetAllowEmptyCommits(enabled) + } + commit, err := applier.Commit(context.Background(), nil, nil) + if err == nil || err.Error() != "no pending tree or tree entries" { + t.Fatalf("expected empty commit error, got commit %v, error %v", commit, err) + } + if commit != nil { + t.Fatalf("expected no commit, got %v", commit) + } + }) + } +} + +func TestApplierEmptyCommits(t *testing.T) { + parent, tree := "base", "base-tree" + requests := 0 + date := time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC) + header := &gitdiff.PatchHeader{ + Title: "Check failed", + Body: "[skip ci]", + Author: &gitdiff.PatchIdentity{Name: "Author", Email: "author@example.com"}, + AuthorDate: date, + Committer: &gitdiff.PatchIdentity{Name: "Committer", Email: "committer@example.com"}, + CommitterDate: date.Add(time.Minute), + } + tmpl := &github.Commit{ + Verification: &github.SignatureVerification{Signature: new("test-signature")}, + } + + transport := emptyCommitTransport(func(r *http.Request) (*http.Response, error) { + requests++ + if r.Method != http.MethodPost || r.URL.Path != "/repos/owner/repo/git/commits" { + return nil, fmt.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + } + defer r.Body.Close() + + var got map[string]any + if err := json.NewDecoder(r.Body).Decode(&got); err != nil { + return nil, err + } + want := map[string]any{ + "tree": tree, + "parents": []any{parent}, + "message": "Check failed\n\n[skip ci]", + "signature": "test-signature", + "author": map[string]any{ + "name": "Author", + "email": "author@example.com", + "date": date.Format(time.RFC3339), + }, + "committer": map[string]any{ + "name": "Committer", + "email": "committer@example.com", + "date": date.Add(time.Minute).Format(time.RFC3339), + }, + } + if !reflect.DeepEqual(got, want) { + t.Errorf("unexpected commit request:\n got: %#v\nwant: %#v", got, want) + } + + parent = fmt.Sprintf("commit-%d", requests) + body, err := json.Marshal(&github.Commit{ + SHA: new(parent), + Tree: &github.Tree{SHA: new(tree)}, + }) + if err != nil { + return nil, err + } + return &http.Response{ + StatusCode: http.StatusCreated, + Header: http.Header{"Content-Type": []string{"application/json"}}, + Body: io.NopCloser(strings.NewReader(string(body))), + Request: r, + }, nil + }) + client, err := github.NewClient(github.WithHTTPClient(&http.Client{Transport: transport})) + if err != nil { + t.Fatal(err) + } + base := &github.Commit{SHA: new(parent), Tree: &github.Tree{SHA: new(tree)}} + applier := NewApplier(client, Repository{Owner: "owner", Name: "repo"}, base) + applier.SetAllowEmptyCommits(true) + + for i := 0; i < 3; i++ { + if i == 2 { + // Reset changes the parent and tree, but preserves the opt-in. + parent, tree = "reset-base", "reset-tree" + applier.Reset(&github.Commit{SHA: new(parent), Tree: &github.Tree{SHA: new(tree)}}) + } + commit, err := applier.Commit(context.Background(), tmpl, header) + if err != nil { + t.Fatalf("commit %d failed: %v", i+1, err) + } + if commit.GetSHA() != parent || commit.GetTree().GetSHA() != tree { + t.Fatalf("unexpected commit result: %v", commit) + } + } + + // Allowing empty commits does not allow creating a tree with no entries. + if _, err := applier.CreateTree(context.Background()); err == nil { + t.Error("expected an error creating a tree with no entries") + } + applier.SetAllowEmptyCommits(false) + if _, err := applier.Commit(context.Background(), nil, nil); err == nil { + t.Error("expected empty commit rejection after disabling") + } + if requests != 3 { + t.Errorf("expected exactly three commit requests, got %d", requests) + } +} + +type emptyCommitTransport func(*http.Request) (*http.Response, error) + +func (f emptyCommitTransport) RoundTrip(r *http.Request) (*http.Response, error) { + return f(r) +} From f59e083f414d46ee5e6f518ea393b3af61d1e5b5 Mon Sep 17 00:00:00 2001 From: nmiyake Date: Wed, 30 Sep 2026 10:54:29 -0700 Subject: [PATCH 2/2] Update the tests to be more consistent with existing tests --- applier_empty_test.go | 149 ------------------------------------------ applier_test.go | 29 ++++++++ 2 files changed, 29 insertions(+), 149 deletions(-) delete mode 100644 applier_empty_test.go diff --git a/applier_empty_test.go b/applier_empty_test.go deleted file mode 100644 index 39d555a..0000000 --- a/applier_empty_test.go +++ /dev/null @@ -1,149 +0,0 @@ -package patch2pr - -import ( - "context" - "encoding/json" - "fmt" - "io" - "net/http" - "reflect" - "strings" - "testing" - "time" - - "github.com/bluekeyes/go-gitdiff/gitdiff" - "github.com/google/go-github/v92/github" -) - -func TestApplierEmptyCommitDisabled(t *testing.T) { - for _, tc := range []struct { - name string - settings []bool - }{ - {name: "default"}, - {name: "explicitly disabled", settings: []bool{false}}, - {name: "disabled after enabling", settings: []bool{true, false}}, - } { - t.Run(tc.name, func(t *testing.T) { - base := &github.Commit{ - SHA: new("base"), - Tree: &github.Tree{SHA: new("tree")}, - } - applier := NewApplier(nil, Repository{Owner: "owner", Name: "repo"}, base) - for _, enabled := range tc.settings { - applier.SetAllowEmptyCommits(enabled) - } - commit, err := applier.Commit(context.Background(), nil, nil) - if err == nil || err.Error() != "no pending tree or tree entries" { - t.Fatalf("expected empty commit error, got commit %v, error %v", commit, err) - } - if commit != nil { - t.Fatalf("expected no commit, got %v", commit) - } - }) - } -} - -func TestApplierEmptyCommits(t *testing.T) { - parent, tree := "base", "base-tree" - requests := 0 - date := time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC) - header := &gitdiff.PatchHeader{ - Title: "Check failed", - Body: "[skip ci]", - Author: &gitdiff.PatchIdentity{Name: "Author", Email: "author@example.com"}, - AuthorDate: date, - Committer: &gitdiff.PatchIdentity{Name: "Committer", Email: "committer@example.com"}, - CommitterDate: date.Add(time.Minute), - } - tmpl := &github.Commit{ - Verification: &github.SignatureVerification{Signature: new("test-signature")}, - } - - transport := emptyCommitTransport(func(r *http.Request) (*http.Response, error) { - requests++ - if r.Method != http.MethodPost || r.URL.Path != "/repos/owner/repo/git/commits" { - return nil, fmt.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) - } - defer r.Body.Close() - - var got map[string]any - if err := json.NewDecoder(r.Body).Decode(&got); err != nil { - return nil, err - } - want := map[string]any{ - "tree": tree, - "parents": []any{parent}, - "message": "Check failed\n\n[skip ci]", - "signature": "test-signature", - "author": map[string]any{ - "name": "Author", - "email": "author@example.com", - "date": date.Format(time.RFC3339), - }, - "committer": map[string]any{ - "name": "Committer", - "email": "committer@example.com", - "date": date.Add(time.Minute).Format(time.RFC3339), - }, - } - if !reflect.DeepEqual(got, want) { - t.Errorf("unexpected commit request:\n got: %#v\nwant: %#v", got, want) - } - - parent = fmt.Sprintf("commit-%d", requests) - body, err := json.Marshal(&github.Commit{ - SHA: new(parent), - Tree: &github.Tree{SHA: new(tree)}, - }) - if err != nil { - return nil, err - } - return &http.Response{ - StatusCode: http.StatusCreated, - Header: http.Header{"Content-Type": []string{"application/json"}}, - Body: io.NopCloser(strings.NewReader(string(body))), - Request: r, - }, nil - }) - client, err := github.NewClient(github.WithHTTPClient(&http.Client{Transport: transport})) - if err != nil { - t.Fatal(err) - } - base := &github.Commit{SHA: new(parent), Tree: &github.Tree{SHA: new(tree)}} - applier := NewApplier(client, Repository{Owner: "owner", Name: "repo"}, base) - applier.SetAllowEmptyCommits(true) - - for i := 0; i < 3; i++ { - if i == 2 { - // Reset changes the parent and tree, but preserves the opt-in. - parent, tree = "reset-base", "reset-tree" - applier.Reset(&github.Commit{SHA: new(parent), Tree: &github.Tree{SHA: new(tree)}}) - } - commit, err := applier.Commit(context.Background(), tmpl, header) - if err != nil { - t.Fatalf("commit %d failed: %v", i+1, err) - } - if commit.GetSHA() != parent || commit.GetTree().GetSHA() != tree { - t.Fatalf("unexpected commit result: %v", commit) - } - } - - // Allowing empty commits does not allow creating a tree with no entries. - if _, err := applier.CreateTree(context.Background()); err == nil { - t.Error("expected an error creating a tree with no entries") - } - applier.SetAllowEmptyCommits(false) - if _, err := applier.Commit(context.Background(), nil, nil); err == nil { - t.Error("expected empty commit rejection after disabling") - } - if requests != 3 { - t.Errorf("expected exactly three commit requests, got %d", requests) - } -} - -type emptyCommitTransport func(*http.Request) (*http.Response, error) - -func (f emptyCommitTransport) RoundTrip(r *http.Request) (*http.Response, error) { - return f(r) -} diff --git a/applier_test.go b/applier_test.go index 3f50ed8..47746c1 100644 --- a/applier_test.go +++ b/applier_test.go @@ -53,6 +53,35 @@ func TestApplier(t *testing.T) { } } +func TestApplierSupportsEmptyCommits(t *testing.T) { + tctx := prepareTestContext(t) + + t.Logf("Test ID: %s", tctx.ID) + t.Logf("Test Repository: %s", tctx.Repo.String()) + + createBranch(t, tctx) + defer cleanupBranches(t, tctx) + + applier := NewApplier(tctx.Client, tctx.Repo, tctx.BaseCommit) + applier.SetAllowEmptyCommits(true) + + commit, err := applier.Commit(tctx, nil, &gitdiff.PatchHeader{Title: "empty"}) + if err != nil { + t.Fatalf("error committing changes form empty patch: %v", err) + } + // calling "Commit" on the applier without making any calls to "Apply" to apply patches should create a new empty + // commit. The returned commit should be different from the base commit, its tree should be the same. + if commit.GetSHA() == tctx.BaseCommit.GetSHA() { + t.Errorf("empty commit has same SHA as base commit: %s", commit.GetSHA()) + } + if len(commit.Parents) != 1 || commit.Parents[0].GetSHA() != tctx.BaseCommit.GetSHA() { + t.Errorf("empty commit does not have base commit as its parent: %v", commit.Parents) + } + if commit.GetTree().GetSHA() != tctx.BaseTree.GetSHA() { + t.Errorf("empty commit changed tree: expected %s, actual %s", tctx.BaseTree.GetSHA(), commit.GetTree().GetSHA()) + } +} + type TestContext struct { context.Context