branch submit: Respect PR templates (#156)

Picks up the first available PR template, if any
and fills the default PR body with it (along with the commit messages).
Allows for users to fill in details, and adjust the PR message
before submission.

Caveat: This will perform a PR template look up on every PR submission.
For a 'stack submit', that means N lookups.
We can probably optimize this by caching the PR template
between submissions in the same command invocation,
or by generating some sort of hash key based on the .github/ directory
and store it in the data store.

Resolves #87

Author: abhinav

.changes/unreleased/Added-20240604-201208.yaml

@@ -0,0 +1,3 @@
+kind: Added
+body: 'branch submit: Populate default PR message with PR template, if found.'
+time: 2024-06-04T20:12:08.961368-07:00

branch_submit.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"strings"
+	"time"
 
 	"github.com/charmbracelet/log"
 	"go.abhg.dev/gs/internal/forge"
@@ -175,6 +176,26 @@ func (cmd *branchSubmitCmd) Run(
 			return nil
 		}
 
+		// Fetch the template while we're figuring out the default PR
+		// title and body.
+		changeTemplate := make(chan *forge.ChangeTemplate, 1)
+		go func() {
+			defer close(changeTemplate)
+
+			ctx, cancel := context.WithTimeout(ctx, time.Second)
+			defer cancel()
+
+			templates, err := remoteRepo.ListChangeTemplates(ctx)
+			if err != nil {
+				log.Warn("Could not list change templates", "error", err)
+				return
+			}
+
+			if len(templates) > 0 {
+				changeTemplate <- templates[0]
+			}
+		}()
+
 		msgs, err := repo.CommitMessageRange(ctx, cmd.Name, branch.Base)
 		if err != nil {
 			return fmt.Errorf("list commits: %w", err)
@@ -183,30 +204,31 @@ func (cmd *branchSubmitCmd) Run(
 			return errors.New("no commits to submit")
 		}
 
-		var defaultTitle, defaultBody string
+		var (
+			defaultTitle string
+			defaultBody  strings.Builder
+		)
 		if len(msgs) == 1 {
 			// If there's only one commit,
 			// just the body will be the default body.
 			defaultTitle = msgs[0].Subject
-			defaultBody = msgs[0].Body
+			defaultBody.WriteString(msgs[0].Body)
 		} else {
 			// Otherwise, we'll concatenate all the messages.
 			// The revisions are in reverse order,
 			// so we'll want to iterate in reverse.
-			var body strings.Builder
 			defaultTitle = msgs[len(msgs)-1].Subject
 			for i := len(msgs) - 1; i >= 0; i-- {
 				msg := msgs[i]
-				if body.Len() > 0 {
-					body.WriteString("\n\n")
+				if defaultBody.Len() > 0 {
+					defaultBody.WriteString("\n\n")
 				}
-				body.WriteString(msg.Subject)
+				defaultBody.WriteString(msg.Subject)
 				if msg.Body != "" {
-					body.WriteString("\n\n")
-					body.WriteString(msg.Body)
+					defaultBody.WriteString("\n\n")
+					defaultBody.WriteString(msg.Body)
 				}
 			}
-			defaultBody = body.String()
 		}
 
 		var fields []ui.Field
@@ -226,7 +248,12 @@ func (cmd *branchSubmitCmd) Run(
 		}
 
 		if cmd.Body == "" {
-			cmd.Body = defaultBody
+			if tmpl, ok := <-changeTemplate; ok {
+				defaultBody.WriteString("\n\n")
+				defaultBody.WriteString(tmpl.Body)
+			}
+
+			cmd.Body = defaultBody.String()
 			body := ui.NewOpenEditor().
 				WithValue(&cmd.Body).
 				WithTitle("Body").

internal/forge/forge.go

@@ -97,6 +97,12 @@ type Repository interface {
 	FindChangesByBranch(ctx context.Context, branch string) ([]*FindChangeItem, error)
 	FindChangeByID(ctx context.Context, id ChangeID) (*FindChangeItem, error)
 	IsMerged(ctx context.Context, id ChangeID) (bool, error)
+
+	// ListChangeTemplates returns templates defined in the repository
+	// for new change proposals.
+	//
+	// Returns an empty list if no templates are found.
+	ListChangeTemplates(context.Context) ([]*ChangeTemplate, error)
 }
 
 // SubmitChangeRequest is a request to submit a new change in a repository.
@@ -162,3 +168,14 @@ type FindChangeItem struct {
 	// Draft is true if the change is not yet ready to be reviewed.
 	Draft bool
 }
+
+// ChangeTemplate is a template for a new change proposal.
+type ChangeTemplate struct {
+	// Filename is the name of the template file.
+	//
+	// This is NOT a path.
+	Filename string
+
+	// Body is the content of the template file.
+	Body string
+}

internal/forge/github/integration_test.go

@@ -203,3 +203,33 @@ func TestIntegration_Repository_IsMerged(t *testing.T) {
 		assert.True(t, ok)
 	})
 }
+
+func TestIntegration_Repository_ListChangeTemplates(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("absent", func(t *testing.T) {
+		rec := newRecorder(t, t.Name())
+		ghc := githubv4.NewClient(rec.GetDefaultClient())
+		repo, err := github.NewRepository(ctx, "abhinav", "git-spice", logtest.New(t), ghc, _gitSpiceRepoID)
+		require.NoError(t, err)
+
+		templates, err := repo.ListChangeTemplates(ctx)
+		require.NoError(t, err)
+		assert.Empty(t, templates)
+	})
+
+	t.Run("present", func(t *testing.T) {
+		rec := newRecorder(t, t.Name())
+		ghc := githubv4.NewClient(rec.GetDefaultClient())
+		repo, err := github.NewRepository(ctx, "golang", "go", logtest.New(t), ghc, nil)
+		require.NoError(t, err)
+
+		templates, err := repo.ListChangeTemplates(ctx)
+		require.NoError(t, err)
+		require.Len(t, templates, 1)
+
+		template := templates[0]
+		assert.Equal(t, "PULL_REQUEST_TEMPLATE", template.Filename)
+		assert.NotEmpty(t, template.Body)
+	})
+}

internal/forge/github/template.go

@@ -0,0 +1,39 @@
+package github
+
+import (
+	"context"
+
+	"github.com/shurcooL/githubv4"
+	"go.abhg.dev/gs/internal/forge"
+)
+
+// ListChangeTemplates returns PR templates defined in the repository.
+func (r *Repository) ListChangeTemplates(ctx context.Context) ([]*forge.ChangeTemplate, error) {
+	var q struct {
+		Repository struct {
+			PullRequestTemplates []struct {
+				Filename githubv4.String `graphql:"filename"`
+				Body     githubv4.String `graphql:"body"`
+			} `graphql:"pullRequestTemplates"`
+		} `graphql:"repository(owner: $owner, name: $name)"`
+	}
+
+	if err := r.client.Query(ctx, &q, map[string]any{
+		"owner": githubv4.String(r.owner),
+		"name":  githubv4.String(r.repo),
+	}); err != nil {
+		return nil, err
+	}
+
+	out := make([]*forge.ChangeTemplate, 0, len(q.Repository.PullRequestTemplates))
+	for _, t := range q.Repository.PullRequestTemplates {
+		if t.Body != "" {
+			out = append(out, &forge.ChangeTemplate{
+				Filename: string(t.Filename),
+				Body:     string(t.Body),
+			})
+		}
+	}
+
+	return out, nil
+}

internal/forge/github/testdata/fixtures/TestIntegration_Repository_ListChangeTemplates/absent.yaml

@@ -0,0 +1,37 @@
+---
+version: 2
+interactions:
+    - id: 0
+      request:
+        proto: HTTP/1.1
+        proto_major: 1
+        proto_minor: 1
+        content_length: 175
+        transfer_encoding: []
+        trailer: {}
+        host: api.github.com
+        remote_addr: ""
+        request_uri: ""
+        body: |
+            {"query":"query($name:String!$owner:String!){repository(owner: $owner, name: $name){pullRequestTemplates{filename,body}}}","variables":{"name":"git-spice","owner":"abhinav"}}
+        form: {}
+        headers:
+            Content-Type:
+                - application/json
+        url: https://api.github.com/graphql
+        method: POST
+      response:
+        proto: HTTP/2.0
+        proto_major: 2
+        proto_minor: 0
+        transfer_encoding: []
+        trailer: {}
+        content_length: -1
+        uncompressed: true
+        body: '{"data":{"repository":{"pullRequestTemplates":[]}}}'
+        headers:
+            Content-Type:
+                - application/json; charset=utf-8
+        status: 200 OK
+        code: 200
+        duration: 300.745709ms

internal/forge/github/testdata/fixtures/TestIntegration_Repository_ListChangeTemplates/present.yaml

@@ -0,0 +1,71 @@
+---
+version: 2
+interactions:
+    - id: 0
+      request:
+        proto: HTTP/1.1
+        proto_major: 1
+        proto_minor: 1
+        content_length: 134
+        transfer_encoding: []
+        trailer: {}
+        host: api.github.com
+        remote_addr: ""
+        request_uri: ""
+        body: |
+            {"query":"query($owner:String!$repo:String!){repository(owner: $owner, name: $repo){id}}","variables":{"owner":"golang","repo":"go"}}
+        form: {}
+        headers:
+            Content-Type:
+                - application/json
+        url: https://api.github.com/graphql
+        method: POST
+      response:
+        proto: HTTP/2.0
+        proto_major: 2
+        proto_minor: 0
+        transfer_encoding: []
+        trailer: {}
+        content_length: -1
+        uncompressed: true
+        body: '{"data":{"repository":{"id":"MDEwOlJlcG9zaXRvcnkyMzA5Njk1OQ=="}}}'
+        headers:
+            Content-Type:
+                - application/json; charset=utf-8
+        status: 200 OK
+        code: 200
+        duration: 198.453709ms
+    - id: 1
+      request:
+        proto: HTTP/1.1
+        proto_major: 1
+        proto_minor: 1
+        content_length: 167
+        transfer_encoding: []
+        trailer: {}
+        host: api.github.com
+        remote_addr: ""
+        request_uri: ""
+        body: |
+            {"query":"query($name:String!$owner:String!){repository(owner: $owner, name: $name){pullRequestTemplates{filename,body}}}","variables":{"name":"go","owner":"golang"}}
+        form: {}
+        headers:
+            Content-Type:
+                - application/json
+        url: https://api.github.com/graphql
+        method: POST
+      response:
+        proto: HTTP/2.0
+        proto_major: 2
+        proto_minor: 0
+        transfer_encoding: []
+        trailer: {}
+        content_length: -1
+        uncompressed: true
+        body: '{"data":{"repository":{"pullRequestTemplates":[{"filename":"PULL_REQUEST_TEMPLATE","body":"This PR will be imported into Gerrit with the title and first\ncomment (this text) used to generate the subject and body of\nthe Gerrit change.\n\n**Please ensure you adhere to every item in this list.**\n\nMore info can be found at https://github.com/golang/go/wiki/CommitMessage\n\n+ The PR title is formatted as follows: `net/http: frob the quux before blarfing`\n  + The package name goes before the colon\n  + The part after the colon uses the verb tense + phrase that completes the blank in,\n    \"This change modifies Go to ___________\"\n  + Lowercase verb after the colon\n  + No trailing period\n  + Keep the title as short as possible. ideally under 76 characters or shorter\n+ No Markdown\n+ The first PR comment (this one) is wrapped at 76 characters, unless it''s\n  really needed (ASCII art, table, or long link)\n+ If there is a corresponding issue, add either `Fixes #1234` or `Updates #1234`\n  (the latter if this is not a complete fix) to this comment\n+ If referring to a repo other than `golang/go` you can use the\n  `owner/repo#issue_number` syntax: `Fixes golang/tools#1234`\n+ We do not use Signed-off-by lines in Go. Please don''t add them.\n  Our Gerrit server & GitHub bots enforce CLA compliance instead.\n+ Delete these instructions once you have read and applied them\n"}]}}}'
+        headers:
+            Content-Type:
+                - application/json; charset=utf-8
+        status: 200 OK
+        code: 200
+        duration: 189.032875ms

internal/forge/shamhub/forge.go

@@ -164,6 +164,24 @@ func (f *forgeRepository) IsMerged(ctx context.Context, id forge.ChangeID) (bool
 	return res.Merged, nil
 }
 
+func (f *forgeRepository) ListChangeTemplates(ctx context.Context) ([]*forge.ChangeTemplate, error) {
+	u := f.apiURL.JoinPath(f.owner, f.repo, "change-template")
+	var res changeTemplateResponse
+	if err := f.client.Get(ctx, u.String(), &res); err != nil {
+		return nil, fmt.Errorf("lookup change body template: %w", err)
+	}
+
+	out := make([]*forge.ChangeTemplate, len(res))
+	for i, t := range res {
+		out[i] = &forge.ChangeTemplate{
+			Filename: t.Filename,
+			Body:     t.Body,
+		}
+	}
+
+	return out, nil
+}
+
 type jsonHTTPClient struct {
 	log    *log.Logger
 	client interface {

internal/forge/shamhub/shamhub.go

@@ -330,6 +330,7 @@ func (sh *ShamHub) apiHandler() http.Handler {
 	mux.HandleFunc("GET /{owner}/{repo}/change/{number}", sh.handleGetChange)
 	mux.HandleFunc("PATCH /{owner}/{repo}/change/{number}", sh.handleEditChange)
 	mux.HandleFunc("GET /{owner}/{repo}/change/{number}/merged", sh.handleIsMerged)
+	mux.HandleFunc("GET /{owner}/{repo}/change-template", sh.handleChangeTemplate)
 	mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
 		sh.log.Errorf("Unexpected request: %s %s", r.Method, r.URL.Path)
 		http.Error(w, "not found", http.StatusNotFound)
@@ -580,6 +581,44 @@ func (sh *ShamHub) handleIsMerged(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
+type changeTemplateResponse []*changeTemplate
+
+type changeTemplate struct {
+	Filename string `json:"filename,omitempty"`
+	Body     string `json:"body,omitempty"`
+}
+
+func (sh *ShamHub) handleChangeTemplate(w http.ResponseWriter, r *http.Request) {
+	owner, repo := r.PathValue("owner"), r.PathValue("repo")
+	if owner == "" || repo == "" {
+		http.Error(w, "owner, and repo are required", http.StatusBadRequest)
+		return
+	}
+
+	// If the repository has a .shamhub/CHANGE_TEMPLATE.md file,
+	// that's the template to use.
+	logw, flush := ioutil.LogWriter(sh.log, log.DebugLevel)
+	defer flush()
+
+	cmd := exec.Command(sh.gitExe, "cat-file", "-p", "HEAD:.shamhub/CHANGE_TEMPLATE.md")
+	cmd.Dir = sh.repoDir(owner, repo)
+	cmd.Stderr = logw
+
+	var res changeTemplateResponse
+	if out, err := cmd.Output(); err == nil {
+		res = append(res, &changeTemplate{
+			Filename: "CHANGE_TEMPLATE.md",
+			Body:     strings.TrimSpace(string(out)) + "\n",
+		})
+	}
+
+	enc := json.NewEncoder(w)
+	enc.SetIndent("", "  ")
+	if err := enc.Encode(res); err != nil {
+		http.Error(w, err.Error(), http.StatusInternalServerError)
+	}
+}
+
 // shamChangeState records the state of a Change.
 type shamChangeState int