fix: refactor and a bit of cleanup

change.go

@@ -0,0 +1,61 @@
+package main
+
+import (
+	"fmt"
+	"strings"
+)
+
+// Change represents a single change that will be pushed to the remote.
+type Change struct {
+	hash   string
+	author string
+
+	message string
+
+	// trailers are lines to add to the end of the body stored as a list to maintain insertion order
+	trailers []string
+
+	// entries is a map of path -> content for files modified in the change
+	// empty or nil content indicates a deleted file
+	entries map[string][]byte
+}
+
+// Splits a commit message on the first blank line
+func (c Change) splitMessage() (string, string) {
+	h, b, _ := strings.Cut(c.message, "\n\n")
+	return h, b
+}
+
+// Headline is the first paragraph of the message
+func (c Change) Headline() string {
+	h, _ := c.splitMessage()
+	return h
+}
+
+// Body is everything after the headline, including trailers
+func (c Change) Body() string {
+	_, b := c.splitMessage()
+	b = strings.TrimSpace(b)
+
+	sb := &strings.Builder{}
+	sb.WriteString(b)
+	sb.WriteString("\n\n")
+
+	// maybe write trailers, if the trailer doesn't already exist in the body
+	// this is a naive implementation, but it mostly does the job
+	lowerbody := strings.ToLower(b)
+
+	if c.author != "" {
+		authorline := fmt.Sprintf("Co-authored-by: %s", c.author)
+		c.trailers = append([]string{authorline}, c.trailers...)
+	}
+
+	for _, t := range c.trailers {
+		if !strings.Contains(lowerbody, strings.ToLower(t)) {
+			sb.WriteString(t)
+			sb.WriteString("\n")
+		}
+	}
+
+	return strings.TrimSpace(sb.String())
+}

change_test.go

@@ -0,0 +1,59 @@
+package main
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestChangeBody(t *testing.T) {
+	testcases := []struct {
+		input    string
+		author   string
+		trailers []string
+
+		headline string
+		body     string
+	}{{
+		"subject\n\nbody\n\nco-authored-by: author", "author", nil,
+		"subject", "body\n\nco-authored-by: author",
+	}, {
+		"subject only", "", nil,
+		"subject only", "",
+	}, {
+		"no trailers and no author\n\nbody", "", nil,
+		"no trailers and no author", "body",
+	}, {
+		"no trailers with author", "author", nil,
+		"no trailers with author", "Co-authored-by: author",
+	}, {
+		"no trailers with author and body\n\nbody", "author", nil,
+		"no trailers with author and body", "body\n\nCo-authored-by: author",
+	}, {
+		// if the first line looks like a trailer, it's not a trailer
+		"Co-authored-by: subject", "author", nil,
+		"Co-authored-by: subject", "Co-authored-by: author",
+	}, {
+		"subject\n\nbody", "author",
+		[]string{"Foo: bar"},
+		"subject", "body\n\nCo-authored-by: author\nFoo: bar",
+	}}
+
+	for _, tc := range testcases {
+		t.Run("", func(t *testing.T) {
+			change := Change{author: tc.author, message: tc.input, trailers: tc.trailers}
+			headline, body := change.Headline(), change.Body()
+
+			if headline != tc.headline {
+				t.Logf("wrong headline, got=%s, want=%s", headline, tc.headline)
+				t.Fail()
+			}
+
+			tc.body = strings.TrimSpace(tc.body)
+
+			if body != tc.body {
+				t.Logf("wrong body, got=%q, want=%q", body, tc.body)
+				t.Fail()
+			}
+		})
+	}
+}

cmd_push.go

@@ -2,63 +2,14 @@ package main
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"os"
-	"strings"
-
-	"github.com/alecthomas/kong"
 )
 
-type targetFlag string
-
-func (f *targetFlag) Decode(ctx *kong.DecodeContext) error {
-	if err := ctx.Scan.PopValueInto("string", &f); err != nil {
-		return err
-	}
-
-	slashes := strings.Count(string(*f), "/")
-	if slashes == 1 {
-		return nil
-	}
-
-	return fmt.Errorf("must be of the form owner/repo with exactly one slash")
-}
-
 type PushCmd struct {
-	Target  targetFlag `name:"target" short:"T" required:"" help:"Target repository in owner/repo format."`
-	Branch  string     `required:"" help:"Name of the target branch on the remote."`
-	RepoDir string     `name:"repo" default:"." help:"Path to the local repository that contains commits you want to push. Must not be a worktree."`
-	DryRun  bool       `name:"dry-run" help:"Perform everything except the final remote writes to GitHub."`
-
-	Commits []string `arg:"" optional:"" help:"Commit hashes to be applied to the target. Defaults to reading a list of commit hashes from standard input."`
-}
-
-func (c *PushCmd) Run() error {
-	if len(c.Commits) == 0 {
-		var err error
-		c.Commits, err = commitsFromStdin(os.Stdin)
-		if err != nil {
-			return err
-		}
-	}
-
-	owner, repository, _ := strings.Cut(string(c.Target), "/")
-
-	commits := c.Commits[:]
-	if len(c.Commits) >= 10 {
-		commits = commits[:10]
-		commits = append(commits, fmt.Sprintf("...and %d more.", len(c.Commits)-10))
-	}
-
-	commitsout := strings.Join(commits, ", ")
-
-	log("Owner: %s\n", owner)
-	log("Repository: %s\n", repository)
-	log("Branch: %s\n", c.Branch)
-	log("Commits: %s\n", commitsout)
-
-	return push(c.RepoDir, owner, repository, c.Branch, c.Commits, c.DryRun)
+	remoteFlags
+	RepoPath string   `name:"repo-path" default:"." help:"Path to the repository that contains the commits. Defaults to the current directory."`
+	Commits  []string `arg:"" optional:"" help:"Commit hashes to be applied to the target. Defaults to reading a list of commit hashes from standard input."`
 }
 
 func (c *PushCmd) Help() string {
@@ -96,60 +47,27 @@ pushed commits, you should hard reset the local checkout to the remote version a
 
 	git fetch origin <branch>
 	git reset --hard origin/<branch>
-	`
+`
 }
 
-// push actually performs the push
-func push(gitdir, owner, repository, branch string, commits []string, dryrun bool) error {
-	token := getToken(os.Getenv)
-	if token == "" {
-		return errors.New("no GitHub token supplied")
-	}
-
-	client := NewClient(context.Background(), token, owner, repository, branch)
-	client.dryrun = dryrun
-
-	headRef, err := client.GetHeadCommitHash(context.Background())
-	if err != nil {
-		return err
+func (c *PushCmd) Run() error {
+	if len(c.Commits) == 0 {
+		var err error
+		c.Commits, err = commitsFromStdin(os.Stdin)
+		if err != nil {
+			return err
+		}
 	}
 
-	log("Current head commit: %s\n", headRef)
+	// Convert c.Commits into []Change which we can feed to the remote
+	repo := &Repository{path: c.RepoPath}
 
-	repo := &Repository{path: gitdir}
-
-	changes, err := repo.Changes(commits...)
+	changes, err := repo.Changes(c.Commits...)
 	if err != nil {
 		return fmt.Errorf("get changes: %w", err)
 	}
 
-	for _, c := range changes {
-		log("Commit %s\n", c.hash)
-		log("  Headline: %s\n", c.Headline)
-		log("  Body: %s\n", c.Body)
-		log("  Changed files: %d\n", len(c.Changes))
-		for p, content := range c.Changes {
-			action := "MODIFY"
-			if len(content) == 0 {
-				action = "DELETE"
-			}
-			log("    - %s: %s\n", action, p)
-		}
-	}
-
-	pushed, newHead, err := client.PushChanges(context.Background(), headRef, changes...)
-	if err != nil {
-		return err
-	} else if pushed != len(changes) {
-		return fmt.Errorf("pushed %d of %d changes", pushed, len(changes))
-	}
-
-	log("Pushed %d commits.\n", len(changes))
-	log("Branch URL: %s\n", client.browseCommitsURL())
-
-	// The only thing that goes to standard output is the new head reference, allowing callers to
-	// capture stdout if they need the reference.
-	fmt.Println(newHead)
+	owner, repository := c.Target.Owner(), c.Target.Repository()
 
-	return nil
+	return pushChanges(context.Background(), owner, repository, c.Branch, c.DryRun, changes...)
 }

git.go

@@ -44,22 +44,14 @@ func (r *Repository) changed(commit string) (Change, error) {
 		return Change{}, fmt.Errorf("range includes a merge commit (%s), not continuing", commit)
 	}
 
-	headline, body, _ := strings.Cut(strings.TrimSpace(message), "\n")
-
-	if body != "" {
-		body += "\n\n"
-	}
-
-	body = fmt.Sprintf("%sCo-authored-by: %s", body, author)
-
 	change := Change{
-		hash:     commit,
-		Headline: headline,
-		Body:     body,
-		Changes:  map[string][]byte{},
+		hash:    commit,
+		message: message,
+		author:  author,
+		entries: map[string][]byte{},
 	}
 
-	change.Changes, err = r.changedFiles(commit)
+	change.entries, err = r.changedFiles(commit)
 	if err != nil {
 		return Change{}, err
 	}

github.go

@@ -12,20 +12,6 @@ import (
 	"golang.org/x/oauth2"
 )
 
-// Change represents a change in a given commit.
-type Change struct {
-	hash string
-
-	// Headline is the first line of the commit message
-	Headline string
-	// Body is the rest of the commit message
-	Body string
-
-	// Changes is a map of path to file contents.
-	// Deleted files will map to nil contents or an empty byte slice.
-	Changes map[string][]byte
-}
-
 // Client provides methods for interacting with a remote repository on GitHub
 type Client struct {
 	httpC  *http.Client
@@ -124,7 +110,7 @@ func (c *Client) PushChange(ctx context.Context, headCommit string, change Chang
 	added := []fileChange{}
 	deleted := []fileChange{}
 
-	for path, content := range change.Changes {
+	for path, content := range change.entries {
 		if len(content) == 0 {
 			deleted = append(deleted, fileChange{
 				Path: path,
@@ -144,8 +130,8 @@ func (c *Client) PushChange(ctx context.Context, headCommit string, change Chang
 		},
 		ExpectedRef: headCommit,
 		Message: commitInputMessage{
-			Headline: change.Headline,
-			Body:     change.Body,
+			Headline: change.Headline(),
+			Body:     change.Body(),
 		},
 		Changes: commitInputChanges{
 			Additions: added,

main.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"strings"
 
 	"github.com/alecthomas/kong"
 )
@@ -14,14 +15,52 @@ func log(f string, args ...any) {
 	fmt.Fprintf(logwriter, f, args...)
 }
 
-var CLI struct {
+type targetFlag string
+
+func (f *targetFlag) Decode(ctx *kong.DecodeContext) error {
+	if err := ctx.Scan.PopValueInto("string", &f); err != nil {
+		return err
+	}
+
+	slashes := strings.Count(string(*f), "/")
+	if slashes == 1 {
+		return nil
+	}
+
+	return fmt.Errorf("must be of the form owner/repo with exactly one slash")
+}
+
+func (f targetFlag) Owner() string {
+	owner, _, _ := strings.Cut(string(f), "/")
+	return owner
+}
+
+func (f targetFlag) Repository() string {
+	_, repo, _ := strings.Cut(string(f), "/")
+	return repo
+}
+
+// flags that are shared among commands that interact with the remote
+type remoteFlags struct {
+	Target targetFlag `name:"target" short:"T" required:"" help:"Target repository in owner/repo format."`
+	Branch string     `required:"" help:"Name of the target branch on the remote."`
+	DryRun bool       `name:"dry-run" help:"Perform everything except the final remote writes to GitHub."`
+}
+
+type CLI struct {
 	Push    PushCmd    `cmd:"" help:"Push local commits to the remote."`
-	Version VersionCmd `cmd:"" help:"Print version information and quit."`
+	Version VersionCmd `cmd:"" help:"Print version information and exit."`
 }
 
 func main() {
 	logwriter = os.Stderr
 
-	ctx := kong.Parse(&CLI)
+	cli := CLI{}
+
+	ctx := kong.Parse(&cli,
+		kong.Name("commit-headless"),
+		kong.Description("A tool to create signed commits on GitHub."),
+		kong.UsageOnError(),
+	)
 	ctx.FatalIfErrorf(ctx.Run())
 }