Skip to content

Commit 5937a75

Browse files
skarimskarim
committed
addressing review comments
1 parent 03c4b92 commit 5937a75

8 files changed

Lines changed: 87 additions & 43 deletions

File tree

‎cmd/init.go‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package cmd
22

33
import (
44
"fmt"
5-
"os"
65

76
"github.com/cli/go-gh/v2/pkg/prompter"
87
"github.com/github/gh-stack/internal/config"
@@ -124,7 +123,11 @@ func runInit(cfg *config.Config, opts *initOptions) error {
124123
branches = opts.branches
125124
} else {
126125
// Interactive mode
127-
p := prompter.New(os.Stdin, os.Stdout, os.Stderr)
126+
if !cfg.IsInteractive() {
127+
cfg.Errorf("interactive input required; provide branch names or use --adopt")
128+
return nil
129+
}
130+
p := prompter.New(cfg.In, cfg.Out, cfg.Err)
128131

129132
if currentBranch != "" && currentBranch != trunk {
130133
// Already on a non-trunk branch — offer to use it

‎cmd/navigate.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,11 @@ func runNavigateToEnd(cfg *config.Config, top bool) error {
124124
return nil
125125
}
126126

127+
if len(s.Branches) == 0 {
128+
cfg.Errorf("stack has no branches")
129+
return nil
130+
}
131+
127132
var target string
128133
if top {
129134
target = s.Branches[len(s.Branches)-1].Branch

‎cmd/push.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -78,11 +78,11 @@ func runPush(cfg *config.Config, opts *pushOptions) error {
7878
// Push all branches
7979
for _, b := range s.Branches {
8080
if opts.dryRun {
81-
cfg.Printf("Would push %s\n", b.Branch)
81+
cfg.Printf("Would push %s", b.Branch)
8282
continue
8383
}
8484

85-
cfg.Printf("Pushing %s...\n", b.Branch)
85+
cfg.Printf("Pushing %s...", b.Branch)
8686
if err := git.Push("origin", []string{b.Branch}, opts.force, false); err != nil {
8787
cfg.Errorf("failed to push %s: %s", b.Branch, err)
8888
return nil
@@ -99,7 +99,7 @@ func runPush(cfg *config.Config, opts *pushOptions) error {
9999

100100
pr, err := client.FindPRForBranch(b.Branch)
101101
if err != nil {
102-
cfg.Warningf("failed to check PR for %s: %v\n", b.Branch, err)
102+
cfg.Warningf("failed to check PR for %s: %v", b.Branch, err)
103103
continue
104104
}
105105

@@ -110,10 +110,10 @@ func runPush(cfg *config.Config, opts *pushOptions) error {
110110

111111
newPR, createErr := client.CreatePR(baseBranch, b.Branch, title, body, opts.draft)
112112
if createErr != nil {
113-
cfg.Warningf("failed to create PR for %s: %v\n", b.Branch, createErr)
113+
cfg.Warningf("failed to create PR for %s: %v", b.Branch, createErr)
114114
continue
115115
}
116-
cfg.Successf("Created PR #%d for %s\n", newPR.Number, b.Branch)
116+
cfg.Successf("Created PR #%d for %s", newPR.Number, b.Branch)
117117
s.Branches[i].PullRequest = &stack.PullRequestRef{
118118
Number: newPR.Number,
119119
ID: newPR.ID,
@@ -123,12 +123,12 @@ func runPush(cfg *config.Config, opts *pushOptions) error {
123123
// Update base if needed
124124
if pr.BaseRefName != baseBranch {
125125
if err := client.UpdatePRBase(pr.ID, baseBranch); err != nil {
126-
cfg.Warningf("failed to update PR #%d base: %v\n", pr.Number, err)
126+
cfg.Warningf("failed to update PR #%d base: %v", pr.Number, err)
127127
} else {
128-
cfg.Successf("Updated PR #%d base to %s\n", pr.Number, baseBranch)
128+
cfg.Successf("Updated PR #%d base to %s", pr.Number, baseBranch)
129129
}
130130
} else {
131-
cfg.Printf("PR #%d for %s is up to date\n", pr.Number, b.Branch)
131+
cfg.Printf("PR #%d for %s is up to date", pr.Number, b.Branch)
132132
}
133133
if s.Branches[i].PullRequest == nil {
134134
s.Branches[i].PullRequest = &stack.PullRequestRef{
@@ -146,7 +146,7 @@ func runPush(cfg *config.Config, opts *pushOptions) error {
146146
// or we can add a flag to the existing PR API to incrementally build the stack.
147147
//
148148
// For now, the PRs are pushed and created individually but are NOT linked as a formal stack on GitHub.
149-
cfg.Warningf("Stacked PRs is not yet implemented — PRs were created individually.\n")
149+
cfg.Warningf("Stacked PRs is not yet implemented — PRs were created individually.")
150150
fmt.Fprintf(cfg.Err, " Once the GitHub Stacks API is available, PRs will be automatically\n")
151151
fmt.Fprintf(cfg.Err, " grouped into a Stack.\n")
152152

@@ -170,6 +170,6 @@ func runPush(cfg *config.Config, opts *pushOptions) error {
170170
return nil
171171
}
172172

173-
cfg.Successf("Pushed and synced %d branches\n", len(s.Branches))
173+
cfg.Successf("Pushed and synced %d branches", len(s.Branches))
174174
return nil
175175
}

‎cmd/rebase.go‎

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,11 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error {
157157

158158
originalRefs := make(map[string]string)
159159
for _, b := range s.Branches {
160-
sha, _ := git.HeadSHA(b.Branch)
160+
sha, err := git.HeadSHA(b.Branch)
161+
if err != nil {
162+
cfg.Errorf("failed to resolve HEAD SHA for %s: %s", b.Branch, err)
163+
return nil
164+
}
161165
originalRefs[b.Branch] = sha
162166
}
163167

@@ -213,7 +217,9 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error {
213217
UseOnto: true,
214218
OntoOldBase: originalRefs[br.Branch],
215219
}
216-
saveRebaseState(gitDir, state)
220+
if err := saveRebaseState(gitDir, state); err != nil {
221+
cfg.Warningf("failed to save rebase state: %s", err)
222+
}
217223

218224
printConflictDetails(cfg, newBase)
219225
cfg.Printf("")
@@ -261,7 +267,9 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error {
261267
OriginalBranch: currentBranch,
262268
OriginalRefs: originalRefs,
263269
}
264-
saveRebaseState(gitDir, state)
270+
if err := saveRebaseState(gitDir, state); err != nil {
271+
cfg.Warningf("failed to save rebase state: %s", err)
272+
}
265273

266274
printConflictDetails(cfg, base)
267275
cfg.Printf("")
@@ -408,7 +416,9 @@ func continueRebase(cfg *config.Config, gitDir string) error {
408416
state.CurrentBranchIndex = idx
409417
state.ConflictBranch = branchName
410418
state.OntoOldBase = state.OriginalRefs[branchName]
411-
saveRebaseState(gitDir, state)
419+
if err := saveRebaseState(gitDir, state); err != nil {
420+
cfg.Warningf("failed to save rebase state: %s", err)
421+
}
412422

413423
cfg.Warningf("Rebasing %s onto %s ... conflict", branchName, newBase)
414424
printConflictDetails(cfg, newBase)
@@ -448,7 +458,9 @@ func continueRebase(cfg *config.Config, gitDir string) error {
448458
state.RemainingBranches = state.RemainingBranches[remainIdx+1:]
449459
state.CurrentBranchIndex = idx
450460
state.ConflictBranch = branchName
451-
saveRebaseState(gitDir, state)
461+
if err := saveRebaseState(gitDir, state); err != nil {
462+
cfg.Warningf("failed to save rebase state: %s", err)
463+
}
452464

453465
cfg.Warningf("Rebasing %s onto %s ... conflict", branchName, base)
454466
printConflictDetails(cfg, base)
@@ -509,21 +521,41 @@ func abortRebase(cfg *config.Config, gitDir string) error {
509521
_ = git.RebaseAbort()
510522
}
511523

524+
var restoreErrors []string
512525
for branch, sha := range state.OriginalRefs {
513-
_ = git.CheckoutBranch(branch)
514-
_ = git.ResetHard(sha)
526+
if err := git.CheckoutBranch(branch); err != nil {
527+
restoreErrors = append(restoreErrors, fmt.Sprintf("checkout %s: %s", branch, err))
528+
continue
529+
}
530+
if err := git.ResetHard(sha); err != nil {
531+
restoreErrors = append(restoreErrors, fmt.Sprintf("reset %s: %s", branch, err))
532+
}
515533
}
516534

517535
_ = git.CheckoutBranch(state.OriginalBranch)
518536
clearRebaseState(gitDir)
519-
cfg.Successf("Rebase aborted and branches restored")
520537

538+
if len(restoreErrors) > 0 {
539+
cfg.Warningf("Rebase aborted but some branches could not be fully restored:")
540+
for _, e := range restoreErrors {
541+
cfg.Printf(" %s", e)
542+
}
543+
return nil
544+
}
545+
546+
cfg.Successf("Rebase aborted and branches restored")
521547
return nil
522548
}
523549

524-
func saveRebaseState(gitDir string, state *rebaseState) {
525-
data, _ := json.MarshalIndent(state, "", " ")
526-
_ = os.WriteFile(filepath.Join(gitDir, rebaseStateFile), data, 0644)
550+
func saveRebaseState(gitDir string, state *rebaseState) error {
551+
data, err := json.MarshalIndent(state, "", " ")
552+
if err != nil {
553+
return fmt.Errorf("error serializing rebase state: %w", err)
554+
}
555+
if err := os.WriteFile(filepath.Join(gitDir, rebaseStateFile), data, 0644); err != nil {
556+
return fmt.Errorf("error writing rebase state: %w", err)
557+
}
558+
return nil
527559
}
528560

529561
func loadRebaseState(gitDir string) (*rebaseState, error) {

‎cmd/view.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,10 @@ func runPager(cfg *config.Config, content string) error {
172172
}
173173

174174
args := strings.Fields(pagerCmd)
175+
if len(args) == 0 {
176+
_, err := fmt.Fprint(cfg.Out, content)
177+
return err
178+
}
175179
if args[0] == "less" {
176180
hasR := false
177181
for _, a := range args[1:] {
@@ -250,16 +254,16 @@ func viewWeb(cfg *config.Config, s *stack.Stack) error {
250254
}
251255
url := fmt.Sprintf("https://github.com/%s/%s/pull/%d", repo.Owner, repo.Name, pr.Number)
252256
if err := b.Browse(url); err != nil {
253-
cfg.Warningf("failed to open %s: %v\n", url, err)
257+
cfg.Warningf("failed to open %s: %v", url, err)
254258
} else {
255259
opened++
256260
}
257261
}
258262

259263
if opened == 0 {
260-
cfg.Printf("No PRs found to open in browser.\n")
264+
cfg.Printf("No PRs found to open in browser.")
261265
} else {
262-
cfg.Successf("Opened %d PRs in browser\n", opened)
266+
cfg.Successf("Opened %d PRs in browser", opened)
263267
}
264268

265269
return nil

‎internal/config/config.go‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package config
22

33
import (
44
"fmt"
5-
"io"
65
"os"
76

87
"github.com/cli/go-gh/v2/pkg/repository"
@@ -15,9 +14,9 @@ import (
1514
// Config holds shared state for all commands.
1615
type Config struct {
1716
Terminal term.Term
18-
Out io.Writer
19-
Err io.Writer
20-
In io.Reader
17+
Out *os.File
18+
Err *os.File
19+
In *os.File
2120

2221
ColorSuccess func(string) string
2322
ColorError func(string) string
@@ -34,8 +33,8 @@ func New() *Config {
3433
terminal := term.FromEnv()
3534
cfg := &Config{
3635
Terminal: terminal,
37-
Out: terminal.Out(),
38-
Err: terminal.ErrOut(),
36+
Out: os.Stdout,
37+
Err: os.Stderr,
3938
In: os.Stdin,
4039
}
4140

‎internal/git/git.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"os"
66
"os/exec"
7+
"path/filepath"
78
"strconv"
89
"strings"
910
"time"
@@ -197,8 +198,8 @@ func IsRebaseInProgress() bool {
197198
return false
198199
}
199200
for _, dir := range []string{"rebase-merge", "rebase-apply"} {
200-
cmd := exec.Command("test", "-d", gitDir+"/"+dir)
201-
if cmd.Run() == nil {
201+
rebasePath := filepath.Join(gitDir, dir)
202+
if info, err := os.Stat(rebasePath); err == nil && info.IsDir() {
202203
return true
203204
}
204205
}

‎internal/github/github.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,15 +9,15 @@ import (
99

1010
// PullRequest represents a GitHub pull request.
1111
type PullRequest struct {
12-
ID string
13-
Number int
14-
Title string
15-
State string
16-
URL string
17-
HeadRefName string
18-
BaseRefName string
19-
IsDraft bool
20-
Merged bool
12+
ID string `graphql:"id"`
13+
Number int `graphql:"number"`
14+
Title string `graphql:"title"`
15+
State string `graphql:"state"`
16+
URL string `graphql:"url"`
17+
HeadRefName string `graphql:"headRefName"`
18+
BaseRefName string `graphql:"baseRefName"`
19+
IsDraft bool `graphql:"isDraft"`
20+
Merged bool `graphql:"merged"`
2121
}
2222

2323
// Client wraps GitHub API operations.

0 commit comments

Comments
 (0)