diff --git a/.golangci.yml b/.golangci.yml index 9fea222..7803c87 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -5,12 +5,22 @@ run: linters: enable: + - copyloopvar + - dupword - errcheck + - errname + - errorlint + - gocritic - govet - - staticcheck - - unused - ineffassign + - intrange - misspell + - modernize + - nilerr + - perfsprint + - staticcheck + - unconvert + - unused # gosec disabled - false positives for CLI tools that shell out to git settings: errcheck: diff --git a/cmd/abort.go b/cmd/abort.go index 180a24c..2b9a66b 100644 --- a/cmd/abort.go +++ b/cmd/abort.go @@ -2,6 +2,7 @@ package cmd import ( + "errors" "fmt" "os" @@ -35,7 +36,7 @@ func runAbort(cmd *cobra.Command, args []string) error { // Check if cascade in progress st, err := state.Load(g.GetGitDir()) if err != nil { - return fmt.Errorf("no operation in progress") + return errors.New("no operation in progress") } // Abort rebase if in progress diff --git a/cmd/adopt.go b/cmd/adopt.go index a2b9bb3..da8c49d 100644 --- a/cmd/adopt.go +++ b/cmd/adopt.go @@ -2,6 +2,7 @@ package cmd import ( + "errors" "fmt" "os" @@ -88,7 +89,7 @@ func runAdopt(cmd *cobra.Command, args []string) error { if parentNode != nil { for _, ancestor := range tree.GetAncestors(parentNode) { if ancestor.Name == branchName { - return fmt.Errorf("cannot adopt: would create a cycle") + return errors.New("cannot adopt: would create a cycle") } } } diff --git a/cmd/cascade.go b/cmd/cascade.go index 9316e25..5a0a6ab 100644 --- a/cmd/cascade.go +++ b/cmd/cascade.go @@ -54,7 +54,7 @@ func runCascade(cmd *cobra.Command, args []string) error { // Check if cascade already in progress if state.Exists(g.GetGitDir()) { - return fmt.Errorf("operation already in progress; use 'gh stack continue' or 'gh stack abort'") + return errors.New("operation already in progress; use 'gh stack continue' or 'gh stack abort'") } currentBranch, err := g.CurrentBranch() @@ -94,7 +94,7 @@ func runCascade(cmd *cobra.Command, args []string) error { err = doCascadeWithState(g, cfg, branches, cascadeDryRunFlag, state.OperationCascade, false, false, false, nil, stashRef, s) // Restore auto-stashed changes after operation (unless conflict, which saves stash in state) - if stashRef != "" && err != ErrConflict { + if stashRef != "" && !errors.Is(err, ErrConflict) { fmt.Println("Restoring auto-stashed changes...") if popErr := g.StashPop(stashRef); popErr != nil { fmt.Printf("%s could not restore stashed changes (commit %s): %v\n", s.WarningIcon(), git.AbbrevSHA(stashRef), popErr) diff --git a/cmd/continue.go b/cmd/continue.go index f2052e2..732af4b 100644 --- a/cmd/continue.go +++ b/cmd/continue.go @@ -2,6 +2,7 @@ package cmd import ( + "errors" "fmt" "os" @@ -37,14 +38,14 @@ func runContinue(cmd *cobra.Command, args []string) error { // Check if operation in progress st, err := state.Load(g.GetGitDir()) if err != nil { - return fmt.Errorf("no operation in progress") + return errors.New("no operation in progress") } // Complete the in-progress rebase if g.IsRebaseInProgress() { fmt.Println("Continuing rebase...") if rebaseErr := g.RebaseContinue(); rebaseErr != nil { - return fmt.Errorf("rebase --continue failed; resolve conflicts first") + return errors.New("rebase --continue failed; resolve conflicts first") } } @@ -75,7 +76,7 @@ func runContinue(cmd *cobra.Command, args []string) error { if cascadeErr := doCascadeWithState(g, cfg, branches, false, st.Operation, st.UpdateOnly, st.Web, st.PushOnly, st.Branches, st.StashRef, s); cascadeErr != nil { // Stash handling is done by doCascadeWithState (conflict saves in state, errors restore) - if cascadeErr != ErrConflict && st.StashRef != "" { + if !errors.Is(cascadeErr, ErrConflict) && st.StashRef != "" { fmt.Println("Restoring auto-stashed changes...") if popErr := g.StashPop(st.StashRef); popErr != nil { fmt.Printf("%s could not restore stashed changes (commit %s): %v\n", s.WarningIcon(), git.AbbrevSHA(st.StashRef), popErr) diff --git a/cmd/create.go b/cmd/create.go index b1f7862..fa2aa58 100644 --- a/cmd/create.go +++ b/cmd/create.go @@ -2,6 +2,7 @@ package cmd import ( + "errors" "fmt" "os" @@ -76,7 +77,7 @@ func runCreate(cmd *cobra.Command, args []string) error { if hasStaged && !createEmptyFlag { if createMessageFlag == "" { - return fmt.Errorf("staged changes found but no commit message provided; use -m or --empty") + return errors.New("staged changes found but no commit message provided; use -m or --empty") } } diff --git a/cmd/init.go b/cmd/init.go index 3664d35..6953430 100644 --- a/cmd/init.go +++ b/cmd/init.go @@ -2,6 +2,7 @@ package cmd import ( + "errors" "fmt" "os" @@ -42,12 +43,13 @@ func runInit(cmd *cobra.Command, args []string) error { trunk := trunkFlag if trunk == "" { // Try main, then master - if g.BranchExists("main") { + switch { + case g.BranchExists("main"): trunk = "main" - } else if g.BranchExists("master") { + case g.BranchExists("master"): trunk = "master" - } else { - return fmt.Errorf("could not determine trunk branch; use --trunk to specify") + default: + return errors.New("could not determine trunk branch; use --trunk to specify") } } diff --git a/cmd/log_test.go b/cmd/log_test.go index 056a42d..b39ea73 100644 --- a/cmd/log_test.go +++ b/cmd/log_test.go @@ -98,6 +98,7 @@ func TestLogMultipleBranches(t *testing.T) { if featureA == nil { t.Fatal("feature-a not found") + return } if len(featureA.Children) != 1 { diff --git a/cmd/submit.go b/cmd/submit.go index 126a357..b340660 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -2,6 +2,7 @@ package cmd import ( + "errors" "fmt" "os" "regexp" @@ -57,10 +58,10 @@ func runSubmit(cmd *cobra.Command, args []string) error { // Validate flag combinations if submitPushOnlyFlag && submitUpdateOnlyFlag { - return fmt.Errorf("--push-only and --update-only cannot be used together: --push-only skips all PR operations") + return errors.New("--push-only and --update-only cannot be used together: --push-only skips all PR operations") } if submitPushOnlyFlag && submitWebFlag { - return fmt.Errorf("--push-only and --web cannot be used together: --push-only skips all PR operations") + return errors.New("--push-only and --web cannot be used together: --push-only skips all PR operations") } cwd, err := os.Getwd() @@ -77,7 +78,7 @@ func runSubmit(cmd *cobra.Command, args []string) error { // Check if operation already in progress if state.Exists(g.GetGitDir()) { - return fmt.Errorf("operation already in progress; use 'gh stack continue' or 'gh stack abort'") + return errors.New("operation already in progress; use 'gh stack continue' or 'gh stack abort'") } currentBranch, err := g.CurrentBranch() @@ -140,7 +141,7 @@ func runSubmit(cmd *cobra.Command, args []string) error { fmt.Println(s.Bold("=== Phase 1: Restack ===")) if cascadeErr := doCascadeWithState(g, cfg, branches, submitDryRunFlag, state.OperationSubmit, submitUpdateOnlyFlag, submitWebFlag, submitPushOnlyFlag, branchNames, stashRef, s); cascadeErr != nil { // Stash is saved in state for conflicts; restore on other errors - if cascadeErr != ErrConflict && stashRef != "" { + if !errors.Is(cascadeErr, ErrConflict) && stashRef != "" { fmt.Println("Restoring auto-stashed changes...") if popErr := g.StashPop(stashRef); popErr != nil { fmt.Printf("%s could not restore stashed changes (commit %s): %v\n", s.WarningIcon(), git.AbbrevSHA(stashRef), popErr) @@ -232,7 +233,8 @@ func doSubmitPRs(g *git.Git, cfg *config.Config, root *tree.Node, branches []*tr existingPR, _ := cfg.GetPR(b.Name) //nolint:errcheck // 0 is fine - if existingPR > 0 { + switch { + case existingPR > 0: // Update existing PR if dryRun { fmt.Printf("%s Would update PR #%d base to %s\n", s.Muted("dry-run:"), existingPR, s.Branch(parent)) @@ -255,27 +257,28 @@ func doSubmitPRs(g *git.Git, cfg *config.Config, root *tree.Node, branches []*tr // If PR is a draft and now targets trunk, offer to publish maybeMarkPRReady(ghClient, existingPR, b.Name, parent, trunk, s) } - } else if !updateOnly { + case !updateOnly: // Create new PR if dryRun { fmt.Printf("%s Would create PR for %s (base: %s)\n", s.Muted("dry-run:"), s.Branch(b.Name), s.Branch(parent)) } else { prNum, adopted, err := createPRForBranch(g, ghClient, cfg, root, b.Name, parent, trunk, remoteBranches, s) - if err != nil { + switch { + case err != nil: fmt.Printf("%s failed to create PR for %s: %v\n", s.WarningIcon(), s.Branch(b.Name), err) - } else if adopted { + case adopted: fmt.Printf("%s Adopted PR #%d for %s (%s)\n", s.SuccessIcon(), prNum, s.Branch(b.Name), ghClient.PRURL(prNum)) if openWeb { prURLs = append(prURLs, ghClient.PRURL(prNum)) } - } else { + default: fmt.Printf("%s Created PR #%d for %s (%s)\n", s.SuccessIcon(), prNum, s.Branch(b.Name), ghClient.PRURL(prNum)) if openWeb { prURLs = append(prURLs, ghClient.PRURL(prNum)) } } } - } else { + default: fmt.Printf("Skipping %s %s\n", s.Branch(b.Name), s.Muted("(no existing PR, --update-only)")) } } @@ -407,7 +410,7 @@ func promptForPRDetails(branch, defaultTitle, defaultBody string, s *style.Style } title = strings.TrimSpace(title) if title == "" { - return "", "", fmt.Errorf("PR title cannot be empty") + return "", "", errors.New("PR title cannot be empty") } // Show the generated body and ask if user wants to edit diff --git a/cmd/sync.go b/cmd/sync.go index c6a875f..67a0c4f 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -2,8 +2,10 @@ package cmd import ( + "errors" "fmt" "os" + "slices" "github.com/boneskull/gh-stack/internal/config" "github.com/boneskull/gh-stack/internal/git" @@ -184,7 +186,7 @@ func runSync(cmd *cobra.Command, args []string) error { // Uses git diff to detect when a branch's content is already in trunk for _, branch := range branches { // Skip already detected via PR - if sliceContains(merged, branch) { + if slices.Contains(merged, branch) { continue } @@ -215,7 +217,7 @@ func runSync(cmd *cobra.Command, args []string) error { } // Skip if parent is already marked as merged (will be handled) - if sliceContains(merged, parent) { + if slices.Contains(merged, parent) { continue } @@ -351,7 +353,7 @@ func runSync(cmd *cobra.Command, args []string) error { allBranches := []*tree.Node{child} allBranches = append(allBranches, tree.GetDescendants(child)...) if err := doCascadeWithState(g, cfg, allBranches, syncDryRunFlag, state.OperationCascade, false, false, false, nil, stashRef, s); err != nil { - if err == ErrConflict { + if errors.Is(err, ErrConflict) { hitConflict = true } return err @@ -373,16 +375,6 @@ func runSync(cmd *cobra.Command, args []string) error { return nil } -// sliceContains returns true if slice contains the given string. -func sliceContains(slice []string, s string) bool { - for _, item := range slice { - if item == s { - return true - } - } - return false -} - // mergedAction represents the user's choice for handling a merged branch. type mergedAction int diff --git a/cmd/undo.go b/cmd/undo.go index e9cd930..76ac651 100644 --- a/cmd/undo.go +++ b/cmd/undo.go @@ -79,11 +79,12 @@ func runUndo(cmd *cobra.Command, args []string) error { // Show branch changes for name, bs := range snapshot.Branches { currentSHA, tipErr := g.GetTip(name) - if tipErr != nil { + switch { + case tipErr != nil: fmt.Printf(" - %s: %s → %s\n", s.Branch(name), s.Muted("(branch missing)"), git.AbbrevSHA(bs.SHA)) - } else if currentSHA != bs.SHA { + case currentSHA != bs.SHA: fmt.Printf(" - %s: %s → %s\n", s.Branch(name), git.AbbrevSHA(currentSHA), git.AbbrevSHA(bs.SHA)) - } else { + default: fmt.Printf(" - %s: %s\n", s.Branch(name), s.Muted("(unchanged)")) } } diff --git a/e2e/helpers_test.go b/e2e/helpers_test.go index 728dd2d..8d6b286 100644 --- a/e2e/helpers_test.go +++ b/e2e/helpers_test.go @@ -3,6 +3,7 @@ package e2e_test import ( "bytes" + "errors" "os" "os/exec" "path/filepath" @@ -108,7 +109,8 @@ func (e *TestEnv) Run(args ...string) *Result { } if err != nil { - if exitErr, ok := err.(*exec.ExitError); ok { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { result.ExitCode = exitErr.ExitCode() } else { e.t.Fatalf("failed to run command: %v", err) diff --git a/internal/config/config.go b/internal/config/config.go index 4b705e9..4c93686 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -128,8 +128,8 @@ func (c *Config) ListTrackedBranches() ([]string, error) { // Note: git normalizes config keys to lowercase, so stackParent becomes stackparent out, err := exec.Command("git", "-C", c.repoPath, "config", "--get-regexp", "^branch\\..*\\.stackparent$").Output() if err != nil { - // No matches is not an error - return []string{}, nil + // No matches is not an error — git config exits non-zero when no keys match + return []string{}, nil //nolint:nilerr // non-zero exit = no matching config keys } var branches []string diff --git a/internal/git/git.go b/internal/git/git.go index 25a5f30..1f8e525 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -20,15 +20,15 @@ var ErrDirtyWorkTree = errors.New("working tree has uncommitted changes") var ( gitPath string gitPathOnce sync.Once - gitPathErr error + errGitPath error ) // resolveGitPath finds the git executable using safeexec to prevent PATH injection. func resolveGitPath() (string, error) { gitPathOnce.Do(func() { - gitPath, gitPathErr = safeexec.LookPath("git") + gitPath, errGitPath = safeexec.LookPath("git") }) - return gitPath, gitPathErr + return gitPath, errGitPath } // Git provides git operations for a repository. @@ -124,8 +124,8 @@ func (g *Git) IsDirty() (bool, error) { func (g *Git) HasStagedChanges() (bool, error) { err := g.runSilent("diff", "--cached", "--quiet") if err != nil { - // Exit code 1 means there are differences - return true, nil + // Exit code 1 means there are differences — this is expected, not a real error + return true, nil //nolint:nilerr // non-zero exit = staged changes exist } return false, nil } @@ -197,7 +197,7 @@ func (g *Git) HasUnmergedCommits(branch, upstream string) (bool, error) { } // If any line starts with +, there are unmerged commits - for _, line := range strings.Split(out, "\n") { + for line := range strings.SplitSeq(out, "\n") { if strings.HasPrefix(line, "+ ") { return true, nil } @@ -212,8 +212,8 @@ func (g *Git) IsContentMerged(branch, upstream string) (bool, error) { // git diff upstream branch --quiet exits 0 if no diff, 1 if diff exists err := g.runSilent("diff", "--quiet", upstream, branch) if err != nil { - // Exit code 1 means there are differences - return false, nil + // Exit code 1 means there are differences — this is expected, not a real error + return false, nil //nolint:nilerr // non-zero exit = content differs } // Exit code 0 means no differences - content is merged return true, nil @@ -234,7 +234,7 @@ func (g *Git) ListRemoteBranches() (map[string]bool, error) { return nil, err } branches := make(map[string]bool) - for _, line := range strings.Split(out, "\n") { + for line := range strings.SplitSeq(out, "\n") { line = strings.TrimSpace(line) if line != "" && line != "HEAD" { branches[line] = true @@ -359,7 +359,7 @@ func (g *Git) StashPop(ref string) error { stashRef, err := g.findStashByHash(ref) if err != nil { // Apply succeeded but we couldn't find stash to drop - not fatal - return nil + return nil //nolint:nilerr // apply already succeeded, drop failure is best-effort } if stashRef != "" { // Silently drop; errors aren't critical since apply already succeeded diff --git a/internal/github/github.go b/internal/github/github.go index 39c3db3..6b346b0 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -429,6 +429,7 @@ func getDefaultClient() (*Client, error) { } // CreatePR creates a new pull request using the default client. +// // Deprecated: Use NewClient() and call methods directly for better error handling. func CreatePR(head, base, title, body string) (int, error) { client, err := getDefaultClient() diff --git a/internal/github/github_test.go b/internal/github/github_test.go index b3d0e43..7978af2 100644 --- a/internal/github/github_test.go +++ b/internal/github/github_test.go @@ -452,6 +452,7 @@ func TestClient_FindPRByHead(t *testing.T) { } if pr == nil { t.Fatal("expected PR, got nil") + return } if pr.Number != 42 { t.Errorf("expected PR number 42, got %d", pr.Number) @@ -496,7 +497,7 @@ func TestClient_FindPRByHead_Error(t *testing.T) { } func TestClient_CreateSubmitPR(t *testing.T) { - var capturedBody map[string]interface{} + var capturedBody map[string]any mock := &mockREST{ postFn: func(path string, body io.Reader, response any) error { if path != "repos/owner/repo/pulls" { @@ -538,7 +539,7 @@ func TestClient_CreateSubmitPR(t *testing.T) { } func TestClient_CreateSubmitPR_Draft(t *testing.T) { - var capturedBody map[string]interface{} + var capturedBody map[string]any mock := &mockREST{ postFn: func(path string, body io.Reader, response any) error { if err := json.NewDecoder(body).Decode(&capturedBody); err != nil { diff --git a/internal/prompt/prompt.go b/internal/prompt/prompt.go index 16f8910..b06fa60 100644 --- a/internal/prompt/prompt.go +++ b/internal/prompt/prompt.go @@ -2,6 +2,7 @@ package prompt import ( + "errors" "fmt" "os" "os/exec" @@ -137,7 +138,7 @@ func EditInEditor(text string) (string, error) { // If not in an interactive terminal, returns the defaultIndex. func Select(prompt string, options []string, defaultIndex int) (int, error) { if len(options) == 0 { - return 0, fmt.Errorf("no options provided") + return 0, errors.New("no options provided") } // Clamp defaultIndex to valid range diff --git a/internal/style/style.go b/internal/style/style.go index dd76f86..e4c5083 100644 --- a/internal/style/style.go +++ b/internal/style/style.go @@ -77,7 +77,7 @@ func (s *Style) Success(t string) string { } // Successf styles formatted text for success messages (green). -func (s *Style) Successf(format string, args ...interface{}) string { +func (s *Style) Successf(format string, args ...any) string { return s.Success(fmt.Sprintf(format, args...)) } @@ -90,7 +90,7 @@ func (s *Style) Warning(t string) string { } // Warningf styles formatted text for warning messages (yellow). -func (s *Style) Warningf(format string, args ...interface{}) string { +func (s *Style) Warningf(format string, args ...any) string { return s.Warning(fmt.Sprintf(format, args...)) } @@ -103,7 +103,7 @@ func (s *Style) Error(t string) string { } // Errorf styles formatted text for error messages (red). -func (s *Style) Errorf(format string, args ...interface{}) string { +func (s *Style) Errorf(format string, args ...any) string { return s.Error(fmt.Sprintf(format, args...)) } @@ -116,7 +116,7 @@ func (s *Style) Branch(t string) string { } // Branchf styles formatted branch names (cyan). -func (s *Style) Branchf(format string, args ...interface{}) string { +func (s *Style) Branchf(format string, args ...any) string { return s.Branch(fmt.Sprintf(format, args...)) } @@ -129,7 +129,7 @@ func (s *Style) Merged(t string) string { } // Mergedf styles formatted merged PR references (magenta). -func (s *Style) Mergedf(format string, args ...interface{}) string { +func (s *Style) Mergedf(format string, args ...any) string { return s.Merged(fmt.Sprintf(format, args...)) } @@ -142,7 +142,7 @@ func (s *Style) Muted(t string) string { } // Mutedf styles formatted secondary/hint text (gray). -func (s *Style) Mutedf(format string, args ...interface{}) string { +func (s *Style) Mutedf(format string, args ...any) string { return s.Muted(fmt.Sprintf(format, args...)) } @@ -155,7 +155,7 @@ func (s *Style) Bold(t string) string { } // Boldf styles formatted text with bold weight. -func (s *Style) Boldf(format string, args ...interface{}) string { +func (s *Style) Boldf(format string, args ...any) string { return s.Bold(fmt.Sprintf(format, args...)) } diff --git a/internal/tree/tree_test.go b/internal/tree/tree_test.go index add4a5d..2c9be2a 100644 --- a/internal/tree/tree_test.go +++ b/internal/tree/tree_test.go @@ -62,6 +62,7 @@ func TestFindNode(t *testing.T) { node := tree.FindNode(root, "feature-a") if node == nil { t.Fatal("FindNode returned nil") + return } if node.Name != "feature-a" { t.Errorf("expected 'feature-a', got %q", node.Name) diff --git a/internal/undo/undo.go b/internal/undo/undo.go index ed57b80..bc1e2f9 100644 --- a/internal/undo/undo.go +++ b/internal/undo/undo.go @@ -247,7 +247,7 @@ func pruneDir(dir string, max int) error { // Delete oldest files until we're at or below max toDelete := len(jsonFiles) - max - for i := 0; i < toDelete; i++ { + for i := range toDelete { path := filepath.Join(dir, jsonFiles[i].Name()) if removeErr := os.Remove(path); removeErr != nil && !os.IsNotExist(removeErr) { return removeErr diff --git a/internal/undo/undo_test.go b/internal/undo/undo_test.go index 159ed4e..fcfbc76 100644 --- a/internal/undo/undo_test.go +++ b/internal/undo/undo_test.go @@ -2,6 +2,7 @@ package undo_test import ( + "errors" "os" "path/filepath" "testing" @@ -112,7 +113,7 @@ func TestNoSnapshot(t *testing.T) { } _, _, err := undo.LoadLatest(gitDir) - if err != undo.ErrNoSnapshot { + if !errors.Is(err, undo.ErrNoSnapshot) { t.Errorf("Expected ErrNoSnapshot, got %v", err) } } @@ -158,7 +159,7 @@ func TestArchive(t *testing.T) { // LoadLatest should now return ErrNoSnapshot _, _, err = undo.LoadLatest(gitDir) - if err != undo.ErrNoSnapshot { + if !errors.Is(err, undo.ErrNoSnapshot) { t.Errorf("Expected ErrNoSnapshot after archive, got %v", err) } } @@ -329,7 +330,7 @@ func TestSavePrunesOldSnapshots(t *testing.T) { } // Create 55 snapshots (exceeds the 50 limit) - for i := 0; i < 55; i++ { + for i := range 55 { snapshot := undo.NewSnapshot("cascade", "gh stack cascade", "main") // Use distinct timestamps to ensure unique filenames snapshot.Timestamp = time.Date(2024, 1, 1, 0, 0, i, 0, time.UTC) @@ -371,7 +372,7 @@ func TestArchivePrunesOldSnapshots(t *testing.T) { } // Create and archive 55 snapshots (exceeds the 50 limit) - for i := 0; i < 55; i++ { + for i := range 55 { snapshot := undo.NewSnapshot("cascade", "gh stack cascade", "main") snapshot.Timestamp = time.Date(2024, 1, 1, 0, 0, i, 0, time.UTC) snapshot.Branches["feature"] = undo.BranchState{SHA: "abc"}