Skip to content

Commit f732676

Browse files
committed
fix: address PR review comments
- Add TestGetCommits, TestGetCommitsNoCommits, TestGetCommitsSingleCommit for full coverage of the new GetCommits method - Add --fill-verbose to hasBodyFlag check so we don't override user's explicit body generation request - Document GetPRTitles limitation for very large stacks (100+ PRs) - Document porcelain output format in printPorcelain function comment
1 parent a4e3882 commit f732676

4 files changed

Lines changed: 109 additions & 2 deletions

File tree

cmd/log.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,15 @@ func printTree(node *tree.Node, prefix string, isLast bool, current string, gh *
104104
}
105105
}
106106

107+
// printPorcelain outputs machine-readable tab-separated format:
108+
// BRANCH<tab>PARENT<tab>PR_NUMBER<tab>IS_CURRENT<tab>PR_URL
109+
//
110+
// Fields:
111+
// - BRANCH: branch name
112+
// - PARENT: parent branch name (empty for trunk)
113+
// - PR_NUMBER: associated PR number (0 if none)
114+
// - IS_CURRENT: "1" if current branch, "0" otherwise
115+
// - PR_URL: full PR URL (empty if no PR or GitHub client unavailable)
107116
func printPorcelain(node *tree.Node, current string, gh *github.Client) {
108117
var printNode func(*tree.Node, int)
109118
printNode = func(n *tree.Node, depth int) {

cmd/pr.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -176,8 +176,8 @@ func hasBodyFlag(args []string) bool {
176176
if arg == "--fill" || arg == "-f" {
177177
return true
178178
}
179-
// Check for --fill-first
180-
if arg == "--fill-first" {
179+
// Check for --fill-first or --fill-verbose
180+
if arg == "--fill-first" || arg == "--fill-verbose" {
181181
return true
182182
}
183183
// Check for combined short flags like -bf

internal/git/git_test.go

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,3 +256,97 @@ func TestNeedsRebase(t *testing.T) {
256256
t.Error("feature should need rebase after main moved forward")
257257
}
258258
}
259+
260+
func TestGetCommits(t *testing.T) {
261+
dir := setupTestRepo(t)
262+
g := git.New(dir)
263+
264+
current, _ := g.CurrentBranch()
265+
266+
// Create feature branch and add commits
267+
g.CreateAndCheckout("feature")
268+
269+
// Commit 1: subject only
270+
os.WriteFile(filepath.Join(dir, "file1.txt"), []byte("content1"), 0644)
271+
exec.Command("git", "-C", dir, "add", ".").Run()
272+
exec.Command("git", "-C", dir, "commit", "-m", "feat: first commit").Run()
273+
274+
// Commit 2: subject and body
275+
os.WriteFile(filepath.Join(dir, "file2.txt"), []byte("content2"), 0644)
276+
exec.Command("git", "-C", dir, "add", ".").Run()
277+
exec.Command("git", "-C", dir, "commit", "-m", "feat: second commit\n\nThis is the body of the commit.\nIt has multiple lines.").Run()
278+
279+
// Get commits between main and feature
280+
commits, err := g.GetCommits(current, "feature")
281+
if err != nil {
282+
t.Fatalf("GetCommits failed: %v", err)
283+
}
284+
285+
if len(commits) != 2 {
286+
t.Fatalf("expected 2 commits, got %d", len(commits))
287+
}
288+
289+
// Commits are in reverse chronological order (newest first)
290+
if commits[0].Subject != "feat: second commit" {
291+
t.Errorf("expected first commit subject 'feat: second commit', got %q", commits[0].Subject)
292+
}
293+
if commits[0].Body == "" {
294+
t.Error("expected first commit to have a body")
295+
}
296+
297+
if commits[1].Subject != "feat: first commit" {
298+
t.Errorf("expected second commit subject 'feat: first commit', got %q", commits[1].Subject)
299+
}
300+
if commits[1].Body != "" {
301+
t.Errorf("expected second commit to have no body, got %q", commits[1].Body)
302+
}
303+
}
304+
305+
func TestGetCommitsNoCommits(t *testing.T) {
306+
dir := setupTestRepo(t)
307+
g := git.New(dir)
308+
309+
current, _ := g.CurrentBranch()
310+
311+
// Create feature branch at same commit (no new commits)
312+
g.CreateBranch("feature")
313+
314+
// Should return empty slice
315+
commits, err := g.GetCommits(current, "feature")
316+
if err != nil {
317+
t.Fatalf("GetCommits failed: %v", err)
318+
}
319+
320+
if len(commits) != 0 {
321+
t.Errorf("expected 0 commits, got %d", len(commits))
322+
}
323+
}
324+
325+
func TestGetCommitsSingleCommit(t *testing.T) {
326+
dir := setupTestRepo(t)
327+
g := git.New(dir)
328+
329+
current, _ := g.CurrentBranch()
330+
331+
// Create feature branch with one commit
332+
g.CreateAndCheckout("feature")
333+
os.WriteFile(filepath.Join(dir, "single.txt"), []byte("single"), 0644)
334+
exec.Command("git", "-C", dir, "add", ".").Run()
335+
exec.Command("git", "-C", dir, "commit", "-m", "fix: single commit\n\nThis fixes the bug.").Run()
336+
337+
commits, err := g.GetCommits(current, "feature")
338+
if err != nil {
339+
t.Fatalf("GetCommits failed: %v", err)
340+
}
341+
342+
if len(commits) != 1 {
343+
t.Fatalf("expected 1 commit, got %d", len(commits))
344+
}
345+
346+
if commits[0].Subject != "fix: single commit" {
347+
t.Errorf("expected subject 'fix: single commit', got %q", commits[0].Subject)
348+
}
349+
if commits[0].Body != "This fixes the bug." {
350+
t.Errorf("expected body 'This fixes the bug.', got %q", commits[0].Body)
351+
}
352+
}

internal/github/github.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,10 @@ type PRInfo struct {
4040

4141
// GetPRTitles fetches titles for multiple PRs in a single GraphQL request.
4242
// Returns a map of PR number to PRInfo. PRs that don't exist are omitted.
43+
//
44+
// Note: This builds a single GraphQL query with all PR requests. For extremely
45+
// large stacks (100+ PRs), this could potentially hit query size limits. In
46+
// practice, stacks rarely exceed 10-20 PRs, so batching is not implemented.
4347
func (c *Client) GetPRTitles(prNumbers []int) (map[int]PRInfo, error) {
4448
if len(prNumbers) == 0 {
4549
return make(map[int]PRInfo), nil

0 commit comments

Comments
 (0)