From b66e963ff81105f86169ff318f628b6d95d2a595 Mon Sep 17 00:00:00 2001 From: Matt Vinall Date: Thu, 11 Jun 2026 20:13:58 +0100 Subject: [PATCH] refactor: apply code review improvements across scanner, ui, and report Refactor types, reduce duplication, improve test coverage, and clean up control flow throughout scanner, UI, and report packages. Also fix handling of detached heads. --- go.mod | 1 - go.sum | 2 - main.go | 40 +++++--- report.go | 19 +--- report_test.go | 167 +++++++++++++++++++++++++++++++ scanner/branch_status.go | 87 ++++++++-------- scanner/find.go | 5 +- scanner/git_status.go | 102 +++++++++---------- scanner/multi_git_status.go | 16 +-- scanner/scan.go | 26 +++-- scanner/scan_integration_test.go | 7 +- scanner/scan_test.go | 10 +- scanner/types.go | 47 +++++---- ui/app.go | 9 +- ui/model.go | 4 +- ui/mouse_line_select_test.go | 24 +++++ ui/mouse_resize.go | 2 +- ui/status_layout.go | 27 +---- ui/update.go | 54 +++------- 19 files changed, 394 insertions(+), 255 deletions(-) create mode 100644 report_test.go diff --git a/go.mod b/go.mod index 474a14c..72c3e4e 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,6 @@ require ( github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/lipgloss v1.1.0 github.com/go-git/go-git/v5 v5.18.0 - github.com/mitchellh/go-homedir v1.1.0 github.com/muesli/termenv v0.16.0 github.com/urfave/cli/v3 v3.8.0 golang.org/x/sync v0.20.0 diff --git a/go.sum b/go.sum index e2e6082..8bfdaec 100644 --- a/go.sum +++ b/go.sum @@ -81,8 +81,6 @@ github.com/mattn/go-localereader v0.0.1 h1:ygSAOl7ZXTx4RdPYinUpg6W99U8jWvWi9Ye2J github.com/mattn/go-localereader v0.0.1/go.mod h1:8fBrzywKY7BI3czFoHkuzRoWE9C+EiG4R1k4Cjx5p88= github.com/mattn/go-runewidth v0.0.23 h1:7ykA0T0jkPpzSvMS5i9uoNn2Xy3R383f9HDx3RybWcw= github.com/mattn/go-runewidth v0.0.23/go.mod h1:XBkDxAl56ILZc9knddidhrOlY5R/pDhgLpndooCuJAs= -github.com/mitchellh/go-homedir v1.1.0 h1:lukF9ziXFxDFPkA1vsr5zpc1XuPDn/wFntq5mG+4E0Y= -github.com/mitchellh/go-homedir v1.1.0/go.mod h1:SfyaCUpYCn1Vlf4IUYiD9fPX4A5wJrkLzIz1N1q0pr0= github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 h1:ZK8zHtRHOkbHy6Mmr5D264iyp3TiX5OmNcI5cIARiQI= github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6/go.mod h1:CJlz5H+gyd6CUWT45Oy4q24RdLyn7Md9Vj2/ldJBSIo= github.com/muesli/cancelreader v0.2.2 h1:3I4Kt4BQjOR54NavqnDogx/MIoWBFa0StPA8ELUXHmA= diff --git a/main.go b/main.go index 09dfc45..1e2e522 100644 --- a/main.go +++ b/main.go @@ -7,7 +7,6 @@ import ( "os" "path/filepath" - "github.com/mitchellh/go-homedir" "github.com/urfave/cli/v3" "github.com/boyvinall/dirtygit/scanner" @@ -15,13 +14,34 @@ import ( ) func getDefaultConfigPath() string { - home, err := homedir.Dir() + home, err := os.UserHomeDir() if err != nil { - fmt.Println("WARNING: Unable to determine home directory for default config path") + fmt.Fprintln(os.Stderr, "WARNING: Unable to determine home directory for default config path:", err) + return ".dirtygit.yml" } return filepath.Join(home, ".dirtygit.yml") } +// loadConfig parses the config file named by the --config flag and expands +// environment variables in ScanDirs. If positional args are provided they +// replace ScanDirs.Include. +func loadConfig(cmd *cli.Command, defaultConfig string) (*scanner.Config, error) { + config, err := scanner.ParseConfigFile(cmd.Root().String("config"), defaultConfig) + if err != nil { + return nil, err + } + if cmd.Args().Len() > 0 { + config.ScanDirs.Include = cmd.Args().Slice() + } + for i := range config.ScanDirs.Include { + config.ScanDirs.Include[i] = os.ExpandEnv(config.ScanDirs.Include[i]) + } + for i := range config.ScanDirs.Exclude { + config.ScanDirs.Exclude[i] = os.ExpandEnv(config.ScanDirs.Exclude[i]) + } + return config, nil +} + //go:embed .dirtygit.yml var defaultConfig string @@ -49,25 +69,15 @@ func main() { }, }, Action: func(ctx context.Context, cmd *cli.Command) error { - config, err := scanner.ParseConfigFile(cmd.String("config"), defaultConfig) + config, err := loadConfig(cmd, defaultConfig) if err != nil { return err } - if cmd.Args().Len() > 0 { - config.ScanDirs.Include = cmd.Args().Slice() - } - for i := range config.ScanDirs.Include { - config.ScanDirs.Include[i] = os.ExpandEnv(config.ScanDirs.Include[i]) - } - for i := range config.ScanDirs.Exclude { - config.ScanDirs.Exclude[i] = os.ExpandEnv(config.ScanDirs.Exclude[i]) - } - return ui.Run(config) }, } err := app.Run(context.Background(), os.Args) if err != nil { - fmt.Printf("%+v\n", err) + fmt.Fprintln(os.Stderr, err) } } diff --git a/report.go b/report.go index f78038d..6b7ee32 100644 --- a/report.go +++ b/report.go @@ -100,8 +100,8 @@ func buildReport(mgs *scanner.MultiGitStatus) report { return report{Repos: repos} } -func runReport(config *scanner.Config, outputFile string) error { - mgs, err := scanner.Scan(config) +func runReport(ctx context.Context, config *scanner.Config, outputFile string) error { + mgs, err := scanner.Scan(ctx, config) if err != nil { return err } @@ -134,7 +134,7 @@ func printReportSummary(r report) { fmt.Println(repo.Path) for _, b := range repo.Branches { if b.ShownInTUI { - fmt.Printf(" %s\n", b.GetDisplayName()) + fmt.Printf(" %s\n", b.DisplayName()) } } } @@ -152,20 +152,11 @@ func reportCommand() *cli.Command { }, }, Action: func(ctx context.Context, cmd *cli.Command) error { - config, err := scanner.ParseConfigFile(cmd.Root().String("config"), defaultConfig) + config, err := loadConfig(cmd, defaultConfig) if err != nil { return err } - if cmd.Args().Len() > 0 { - config.ScanDirs.Include = cmd.Args().Slice() - } - for i := range config.ScanDirs.Include { - config.ScanDirs.Include[i] = os.ExpandEnv(config.ScanDirs.Include[i]) - } - for i := range config.ScanDirs.Exclude { - config.ScanDirs.Exclude[i] = os.ExpandEnv(config.ScanDirs.Exclude[i]) - } - return runReport(config, cmd.String("output-file")) + return runReport(ctx, config, cmd.String("output-file")) }, } } diff --git a/report_test.go b/report_test.go new file mode 100644 index 0000000..c3ddfd3 --- /dev/null +++ b/report_test.go @@ -0,0 +1,167 @@ +package main + +import ( + "testing" + + "github.com/go-git/go-git/v5" + + "github.com/boyvinall/dirtygit/scanner" +) + +func TestBuildReportEmpty(t *testing.T) { + mgs := scanner.NewMultiGitStatus() + r := buildReport(mgs) + if len(r.Repos) != 0 { + t.Fatalf("expected 0 repos, got %d", len(r.Repos)) + } +} + +func TestBuildReportFileEntries(t *testing.T) { + mgs := scanner.NewMultiGitStatus() + mgs.AddResult("/repo/a", scanner.RepoStatus{ + Branch: "main", + Porcelain: scanner.PorcelainStatus{ + Entries: []scanner.PorcelainEntry{ + {Staging: 'M', Worktree: ' ', Path: "foo.go"}, + {Staging: ' ', Worktree: 'D', Path: "bar.go"}, + }, + }, + }) + + r := buildReport(mgs) + if len(r.Repos) != 1 { + t.Fatalf("expected 1 repo, got %d", len(r.Repos)) + } + repo := r.Repos[0] + if repo.Path != "/repo/a" { + t.Errorf("path = %q, want /repo/a", repo.Path) + } + if repo.IsClean { + t.Error("expected IsClean=false for dirty repo") + } + if repo.CurrentBranch != "main" { + t.Errorf("CurrentBranch = %q, want main", repo.CurrentBranch) + } + if len(repo.Files) != 2 { + t.Fatalf("files = %d, want 2", len(repo.Files)) + } + if repo.Files[0].Staging != string(git.StatusCode('M')) || repo.Files[0].Path != "foo.go" { + t.Errorf("unexpected file[0]: %+v", repo.Files[0]) + } + if repo.Files[1].Worktree != string(git.StatusCode('D')) || repo.Files[1].Path != "bar.go" { + t.Errorf("unexpected file[1]: %+v", repo.Files[1]) + } +} + +func TestBuildReportBranchShownInTUI(t *testing.T) { + local := scanner.BranchLocation{Name: "local", Exists: true, TipHash: "abc"} + remote := scanner.BranchLocation{Name: "origin", Exists: true, TipHash: "abc"} + + branches := []scanner.LocalBranchRef{ + {Name: "main", Current: true, Locations: []scanner.BranchLocation{local, remote}}, + {Name: "wip", Current: false, Locations: []scanner.BranchLocation{ + {Name: "local", Exists: true, TipHash: "def"}, + {Name: "origin", Exists: false}, + }}, + } + // FilteredBranches only contains "main" (wip is hidden by config) + filtered := []scanner.LocalBranchRef{branches[0]} + + mgs := scanner.NewMultiGitStatus() + mgs.AddResult("/repo/b", scanner.RepoStatus{ + Branch: "main", + Branches: branches, + FilteredBranches: filtered, + }) + + r := buildReport(mgs) + if len(r.Repos) != 1 { + t.Fatalf("expected 1 repo, got %d", len(r.Repos)) + } + if len(r.Repos[0].Branches) != 2 { + t.Fatalf("expected 2 branch entries, got %d", len(r.Repos[0].Branches)) + } + + byName := make(map[string]reportBranchEntry) + for _, b := range r.Repos[0].Branches { + byName[b.Name] = b + } + + if !byName["main"].ShownInTUI { + t.Error("main should be ShownInTUI=true (in FilteredBranches)") + } + if byName["wip"].ShownInTUI { + t.Error("wip should be ShownInTUI=false (not in FilteredBranches)") + } +} + +func TestBuildReportIsLocalOnly(t *testing.T) { + localOnly := scanner.LocalBranchRef{ + Name: "local-feature", + Locations: []scanner.BranchLocation{ + {Name: "local", Exists: true, TipHash: "111"}, + {Name: "origin", Exists: false}, + }, + } + withRemote := scanner.LocalBranchRef{ + Name: "main", + Locations: []scanner.BranchLocation{ + {Name: "local", Exists: true, TipHash: "222"}, + {Name: "origin", Exists: true, TipHash: "222"}, + }, + } + + mgs := scanner.NewMultiGitStatus() + mgs.AddResult("/repo/c", scanner.RepoStatus{ + Branch: "main", + Branches: []scanner.LocalBranchRef{localOnly, withRemote}, + FilteredBranches: []scanner.LocalBranchRef{localOnly, withRemote}, + }) + + r := buildReport(mgs) + byName := make(map[string]reportBranchEntry) + for _, b := range r.Repos[0].Branches { + byName[b.Name] = b + } + + if !byName["local-feature"].IsLocalOnly { + t.Error("local-feature should be IsLocalOnly=true") + } + if byName["main"].IsLocalOnly { + t.Error("main should be IsLocalOnly=false") + } +} + +func TestBuildReportSortedPaths(t *testing.T) { + mgs := scanner.NewMultiGitStatus() + mgs.AddResult("/z/repo", scanner.RepoStatus{Branch: "main"}) + mgs.AddResult("/a/repo", scanner.RepoStatus{Branch: "main"}) + mgs.AddResult("/m/repo", scanner.RepoStatus{Branch: "main"}) + + r := buildReport(mgs) + if len(r.Repos) != 3 { + t.Fatalf("expected 3 repos, got %d", len(r.Repos)) + } + if r.Repos[0].Path != "/a/repo" || r.Repos[1].Path != "/m/repo" || r.Repos[2].Path != "/z/repo" { + t.Errorf("repos not sorted: %v", []string{r.Repos[0].Path, r.Repos[1].Path, r.Repos[2].Path}) + } +} + +func TestBuildReportDetachedHead(t *testing.T) { + mgs := scanner.NewMultiGitStatus() + mgs.AddResult("/repo/d", scanner.RepoStatus{ + Branch: "abc1234", + Detached: true, + }) + + r := buildReport(mgs) + if len(r.Repos) != 1 { + t.Fatalf("expected 1 repo, got %d", len(r.Repos)) + } + if !r.Repos[0].Detached { + t.Error("expected Detached=true") + } + if r.Repos[0].CurrentBranch != "abc1234" { + t.Errorf("CurrentBranch = %q, want abc1234", r.Repos[0].CurrentBranch) + } +} diff --git a/scanner/branch_status.go b/scanner/branch_status.go index 6f52d37..b0678b0 100644 --- a/scanner/branch_status.go +++ b/scanner/branch_status.go @@ -1,6 +1,7 @@ package scanner import ( + "errors" "fmt" "os/exec" "sort" @@ -16,7 +17,8 @@ func haveMergeBase(dir, commitA, commitB string) (bool, error) { if err == nil { return true, nil } - if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { return false, nil } return false, fmt.Errorf("git merge-base: %w", err) @@ -71,15 +73,21 @@ func runGit(dir string, args ...string) (string, error) { } func currentBranch(dir string) (name string, detached bool, err error) { + // First try symbolic-ref to get the branch name name, err = runGit(dir, "symbolic-ref", "--quiet", "--short", "HEAD") if err == nil { return name, false, nil } - head, headErr := runGit(dir, "rev-parse", "--short", "HEAD") - if headErr != nil { - return "", false, headErr + + // If that fails, try rev-parse to see if we are in a detached HEAD state + // head, err := runGit(dir, "rev-parse", "--short", "HEAD") + head, err := runGit(dir, "rev-parse", "HEAD") + if err == nil { + return head, true, nil } - return head, true, nil + + // If both fail, return error + return "", false, err } func listRemotes(dir string) ([]string, error) { @@ -98,7 +106,13 @@ func listRemotes(dir string) ([]string, error) { func refTip(dir, ref string) (hash string, unix int64, exists bool, err error) { out, err := runGit(dir, "show", "-s", "--format=%H %ct", ref) if err != nil { - return "", 0, false, nil + // git show exits 128 for unknown refs; treat that as "doesn't exist" rather + // than an error so callers correctly see Exists=false for missing remotes. + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 128 { + return "", 0, false, nil + } + return "", 0, false, err } parts := strings.Fields(out) if len(parts) != 2 { @@ -226,10 +240,6 @@ func GitBranchStatus(dir string) (branch string, detached bool, locals []LocalBr if err != nil { return } - if detached { - locals, err = listLocalBranches(dir, branch, true) - return - } var remotes []string remotes, err = listRemotes(dir) @@ -237,11 +247,12 @@ func GitBranchStatus(dir string) (branch string, detached bool, locals []LocalBr return } - locals, err = listLocalBranches(dir, branch, false) + locals, err = listLocalBranches(dir, branch, detached) if err != nil { return } + // if !detached && len(locals) == 0 { if len(locals) == 0 { var locations []BranchLocation locations, err = computeBranchLocations(dir, branch, remotes) @@ -256,7 +267,6 @@ func GitBranchStatus(dir string) (branch string, detached bool, locals []LocalBr Current: true, Locations: locations, }} - return } for i := range locals { @@ -268,40 +278,27 @@ func GitBranchStatus(dir string) (branch string, detached bool, locals []LocalBr locals[i].Locations = locs } - // TODO: This fallback is likely unreachable: the only scenario (detached HEAD) is already handled above. - // This dead path adds computeBranchLocations calls and complexity for no gain. - var foundCurrent bool - for i := range locals { - if locals[i].Current { - foundCurrent = true - break - } - } - if !foundCurrent { - locs, err2 := computeBranchLocations(dir, branch, remotes) - if err2 != nil { - err = err2 - return - } - matched := false - for i := range locals { - if locals[i].Name == branch { - locals[i].Locations = locs - locals[i].Current = true - matched = true - break - } - } - if !matched { - tipHash, tipUnix := tipFromLocalBranchLocation(locs) - locals = append([]LocalBranchRef{{ - Name: branch, - TipHash: tipHash, - TipUnix: tipUnix, - Current: true, - Locations: locs, - }}, locals...) + if detached { + unix := int64(0) + if raw, e := runGit(dir, "log", "-1", "--format=%ct", "HEAD"); e == nil { + unix, _ = strconv.ParseInt(raw, 10, 64) } + locals = append([]LocalBranchRef{ + { + Name: "HEAD", + TipHash: branch, + TipUnix: unix, + Current: true, + Locations: []BranchLocation{ + { + Name: "local", + Exists: true, + TipHash: branch, + TipUnix: unix, + }, + }, + }, + }, locals...) // avoid mutating the original slice since it may be shared with the caller } return diff --git a/scanner/find.go b/scanner/find.go index 81f4196..64e9bec 100644 --- a/scanner/find.go +++ b/scanner/find.go @@ -124,10 +124,7 @@ func Walk(ctx context.Context, config *Config, results chan string, onRepoFound var eg errgroup.Group for i := range config.ScanDirs.Include { eg.Go(func() error { - err := walkone(ctx, config.ScanDirs.Include[i], config, results, onRepoFound) - if err == filepath.SkipDir { - cancel() - } else if err != nil { + if err := walkone(ctx, config.ScanDirs.Include[i], config, results, onRepoFound); err != nil { return err } return nil diff --git a/scanner/git_status.go b/scanner/git_status.go index fdf4549..1ed3df7 100644 --- a/scanner/git_status.go +++ b/scanner/git_status.go @@ -1,77 +1,73 @@ package scanner import ( - "bufio" + "bytes" "fmt" "io" "os/exec" - "strings" "github.com/go-git/go-git/v5" ) +// ParsePorcelainStatus parses NUL-delimited output from git status --porcelain -z. +// Rename/copy entries (staging code 'R' or 'C') consume an extra NUL-delimited +// token for the original path; all other entries are single-token records. func ParsePorcelainStatus(r io.Reader) (PorcelainStatus, error) { + data, err := io.ReadAll(r) + if err != nil { + return PorcelainStatus{}, err + } st := PorcelainStatus{} - lineScanner := bufio.NewScanner(r) - for lineScanner.Scan() { - entry, err := parsePorcelainLine(lineScanner.Text()) - if err != nil { - return PorcelainStatus{}, err + i := 0 + for i < len(data) { + j := bytes.IndexByte(data[i:], 0) + var token []byte + if j < 0 { + token = data[i:] + i = len(data) + } else { + token = data[i : i+j] + i += j + 1 + } + if len(token) == 0 { + continue + } + if len(token) < 4 || token[2] != ' ' { + return PorcelainStatus{}, fmt.Errorf("unable to parse status line: %q", token) + } + entry := PorcelainEntry{ + Staging: git.StatusCode(token[0]), + Worktree: git.StatusCode(token[1]), + Path: string(token[3:]), + } + if entry.Path == "" { + return PorcelainStatus{}, fmt.Errorf("unable to parse file path from status line: %q", token) + } + if entry.Staging == 'R' || entry.Staging == 'C' { + // Original path follows as the next NUL-delimited token. + k := bytes.IndexByte(data[i:], 0) + var orig []byte + if k < 0 { + orig = data[i:] + i = len(data) + } else { + orig = data[i : i+k] + i += k + 1 + } + entry.OriginalPath = string(orig) } st.Entries = append(st.Entries, entry) } - if err := lineScanner.Err(); err != nil { - return PorcelainStatus{}, err - } return st, nil } -func parsePorcelainLine(s string) (PorcelainEntry, error) { - if len(s) < 4 || s[2] != ' ' { - return PorcelainEntry{}, fmt.Errorf("unable to parse status line: %q", s) - } - - entry := PorcelainEntry{ - Staging: git.StatusCode(s[0]), - Worktree: git.StatusCode(s[1]), - } - - payload := s[3:] - if oldPath, newPath, ok := strings.Cut(payload, " -> "); ok { - entry.OriginalPath = oldPath - entry.Path = newPath - } else { - entry.Path = payload - } - - if entry.Path == "" { - return PorcelainEntry{}, fmt.Errorf("unable to parse file path from status line: %q", s) - } - - return entry, nil -} - -// GitStatus invokes git executable to determine the git status for a directory. +// GitStatus invokes git to return porcelain status for a directory. func GitStatus(d string) (PorcelainStatus, error) { - cmd := exec.Command("git", "status", "--porcelain") + cmd := exec.Command("git", "status", "--porcelain", "-z") cmd.Dir = d - stdout, err := cmd.StdoutPipe() - if err != nil { - return PorcelainStatus{}, fmt.Errorf("%s: %w", d, err) - } - - if err := cmd.Start(); err != nil { - return PorcelainStatus{}, fmt.Errorf("%s: %w", d, err) - } - - st, err := ParsePorcelainStatus(stdout) + out, err := cmd.Output() if err != nil { return PorcelainStatus{}, fmt.Errorf("%s: %w", d, err) } - - if err := cmd.Wait(); err != nil { - return PorcelainStatus{}, fmt.Errorf("%s: %w", d, err) - } - - return st, nil + return ParsePorcelainStatus(bytes.NewReader(out)) } diff --git a/scanner/multi_git_status.go b/scanner/multi_git_status.go index 0b714ad..6e7f0dc 100644 --- a/scanner/multi_git_status.go +++ b/scanner/multi_git_status.go @@ -7,9 +7,9 @@ import ( // MultiGitStatus holds per-repository scan results. The zero value is usable: // reads treat a nil receiver as empty; the first AddResult or Set allocates -// the inner map. Do not copy a non-zero MultiGitStatus (it contains a sync.Mutex). +// the inner map. Do not copy a non-zero MultiGitStatus (it contains a sync.RWMutex). type MultiGitStatus struct { - mu sync.Mutex + mu sync.RWMutex m map[string]RepoStatus } @@ -46,8 +46,8 @@ func (m *MultiGitStatus) Get(path string) (RepoStatus, bool) { if m == nil { return RepoStatus{}, false } - m.mu.Lock() - defer m.mu.Unlock() + m.mu.RLock() + defer m.mu.RUnlock() rs, ok := m.m[path] return rs, ok } @@ -57,8 +57,8 @@ func (m *MultiGitStatus) Len() int { if m == nil { return 0 } - m.mu.Lock() - defer m.mu.Unlock() + m.mu.RLock() + defer m.mu.RUnlock() return len(m.m) } @@ -67,12 +67,12 @@ func (m *MultiGitStatus) SortedRepoPaths() []string { if m == nil { return nil } - m.mu.Lock() + m.mu.RLock() paths := make([]string, 0, len(m.m)) for r := range m.m { paths = append(paths, r) } - m.mu.Unlock() + m.mu.RUnlock() sort.Strings(paths) return paths } diff --git a/scanner/scan.go b/scanner/scan.go index 9418a8d..e3e3cba 100644 --- a/scanner/scan.go +++ b/scanner/scan.go @@ -2,7 +2,7 @@ package scanner import ( "context" - "log" + "log/slog" "sync/atomic" "time" @@ -16,14 +16,13 @@ func reportProgress(onProgress func(ScanProgress), p ScanProgress) { } // Scan finds all "dirty" git repositories specified by config. -func Scan(config *Config) (*MultiGitStatus, error) { - return ScanWithProgress(config, nil) +func Scan(ctx context.Context, config *Config) (*MultiGitStatus, error) { + return ScanWithProgress(ctx, config, nil) } // ScanWithProgress runs the same scan as [Scan] and invokes onProgress from concurrent // discovery and the status loop. Callbacks should be non-blocking (e.g. small channel send). -func ScanWithProgress(config *Config, onProgress func(ScanProgress)) (*MultiGitStatus, error) { - ctx := context.Background() +func ScanWithProgress(ctx context.Context, config *Config, onProgress func(ScanProgress)) (*MultiGitStatus, error) { repositories := make(chan string, 1000) var found, checked atomic.Uint64 @@ -52,6 +51,7 @@ func ScanWithProgress(config *Config, onProgress func(ScanProgress)) (*MultiGitS results := NewMultiGitStatus() var eg errgroup.Group + ex := NewExcluder(config.GitIgnore.FileGlob, config.GitIgnore.DirGlob) for d := range repositories { eg.Go(func() error { @@ -63,7 +63,7 @@ func ScanWithProgress(config *Config, onProgress func(ScanProgress)) (*MultiGitS CurrentPath: d, }) - rs, include, err := StatusForRepo(config, d) + rs, include, err := statusForRepoWithExcluder(config, ex, d) if err != nil { return err } @@ -94,11 +94,14 @@ func ScanWithProgress(config *Config, onProgress func(ScanProgress)) (*MultiGitS // StatusForRepo returns fresh status for a single repository directory using the // same porcelain filtering and branch metadata as [ScanWithProgress]. The bool // is whether this repo should appear in the dirty list (!clean or remote mismatch). -// If ex is nil, a new excluder is built from config (for single-repo refresh paths). -// afterPorcelain, if non-nil, is invoked after porcelain is resolved and before -// branch metadata is collected (used by [ScanWithProgress] for progress timing). func StatusForRepo(config *Config, dir string) (RepoStatus, bool, error) { ex := NewExcluder(config.GitIgnore.FileGlob, config.GitIgnore.DirGlob) + return statusForRepoWithExcluder(config, ex, dir) +} + +// statusForRepoWithExcluder is the shared implementation used by [StatusForRepo] +// and [ScanWithProgress] (which builds the excluder once for all repos). +func statusForRepoWithExcluder(config *Config, ex Excluder, dir string) (RepoStatus, bool, error) { porcelain, err := GitStatus(dir) if err != nil { return RepoStatus{}, false, err @@ -106,7 +109,10 @@ func StatusForRepo(config *Config, dir string) (RepoStatus, bool, error) { porcelain = ex.FilterPorcelainStatus(porcelain) branch, detached, branches, err := GitBranchStatus(dir) if err != nil { - log.Printf("branch status scan failed for %s: %v", dir, err) + // Best-effort: a single repo's branch metadata failure should not abort the + // whole scan. The repo will still appear if it has uncommitted working-tree + // changes; it will just show no branch divergence information. + slog.Warn("branch status scan failed", "dir", dir, "err", err) } rs := RepoStatus{ diff --git a/scanner/scan_integration_test.go b/scanner/scan_integration_test.go index 990f5c1..9e75cce 100644 --- a/scanner/scan_integration_test.go +++ b/scanner/scan_integration_test.go @@ -1,6 +1,7 @@ package scanner import ( + "context" "os" "path/filepath" "sync" @@ -26,7 +27,7 @@ func TestScanFindsDirtyRepos(t *testing.T) { cfg := &Config{} cfg.ScanDirs.Include = []string{root} - mgs, err := Scan(cfg) + mgs, err := Scan(context.Background(), cfg) if err != nil { t.Fatalf("Scan: %v", err) } @@ -46,7 +47,7 @@ func TestScanEmptyTree(t *testing.T) { cfg := &Config{} cfg.ScanDirs.Include = []string{root} - mgs, err := Scan(cfg) + mgs, err := Scan(context.Background(), cfg) if err != nil { t.Fatalf("Scan: %v", err) } @@ -82,7 +83,7 @@ func TestScanWithProgressReportsProgress(t *testing.T) { } } - mgs, err := ScanWithProgress(cfg, callback) + mgs, err := ScanWithProgress(context.Background(), cfg, callback) if err != nil { t.Fatalf("ScanWithProgress: %v", err) } diff --git a/scanner/scan_test.go b/scanner/scan_test.go index abf35d7..06756f5 100644 --- a/scanner/scan_test.go +++ b/scanner/scan_test.go @@ -9,11 +9,9 @@ import ( ) func TestParsePorcelainStatus(t *testing.T) { - input := strings.Join([]string{ - " M scanner/scan.go", - "R old/name.go -> new/name.go", - "?? scanner/scan_test.go", - }, "\n") + // NUL-delimited format (git status --porcelain -z): + // renames: first token is "R new-path", second token is "old-path" + input := " M scanner/scan.go\x00R new/name.go\x00old/name.go\x00?? scanner/scan_test.go\x00" st, err := ParsePorcelainStatus(strings.NewReader(input)) if err != nil { @@ -38,7 +36,7 @@ func TestParsePorcelainStatus(t *testing.T) { } func TestParsePorcelainStatusRejectsMalformedLine(t *testing.T) { - _, err := ParsePorcelainStatus(strings.NewReader("bad")) + _, err := ParsePorcelainStatus(strings.NewReader("bad\x00")) if err == nil { t.Fatal("ParsePorcelainStatus() expected error for malformed line") } diff --git a/scanner/types.go b/scanner/types.go index e3d50ca..dd52d7c 100644 --- a/scanner/types.go +++ b/scanner/types.go @@ -29,9 +29,10 @@ type RepoStatus struct { Branches []LocalBranchRef // FilteredBranches is the subset of Branches that pass config filtering - // (e.g. hide local-only branches matching config patterns). It is what the - // UI branch pane shows and is used for mismatch detection and unpushed - // change status. It is always a subset of Branches and has the same order. + // (e.g. hide local-only branches matching config patterns). It drives the + // UI branch pane display and report inclusion decisions. HasUnpushedChanges + // iterates Branches directly with inline config filtering, so it does not + // use FilteredBranches. It is always a subset of Branches with the same order. FilteredBranches []LocalBranchRef } @@ -52,11 +53,11 @@ type LocalBranchRef struct { Locations []BranchLocation } -func (lbr *LocalBranchRef) GetDisplayName() string { - if lbr.Current { - return "*" + lbr.Name +func (lb *LocalBranchRef) DisplayName() string { + if lb.Current { + return "*" + lb.Name } - return lbr.Name + return lb.Name } // BranchLocation is one side of a local branch compared to same-named remotes: @@ -117,7 +118,7 @@ func (lb LocalBranchRef) IsLocalOnly() bool { // checked-out branch (the [LocalBranchRef] with Current: true). Returns nil when // detached or when no current row exists. func (rs *RepoStatus) CurrentBranchLocations() []BranchLocation { - if rs == nil || rs.Detached { + if rs == nil { return nil } for i := range rs.Branches { @@ -186,7 +187,7 @@ func (lb *LocalBranchRef) HasUnpushedChanges() bool { // [Config.ShouldHideLocalOnlyBranch] matches. // The checked-out branch is never removed so HEAD remote comparison stays available. func (rs *RepoStatus) Filter(c *Config) []LocalBranchRef { - out := make([]LocalBranchRef, 0) + out := make([]LocalBranchRef, 0, len(rs.Branches)) for _, lb := range rs.Branches { if lb.Current { out = append(out, lb) @@ -207,14 +208,12 @@ func (rs *RepoStatus) Filter(c *Config) []LocalBranchRef { return out } -// LocalRemoteMismatchReasons returns a short line explaining why -func (rs *RepoStatus) LocalRemoteMismatchReasons() []string { - if rs.Detached { - return nil - } +// LocalRemoteMismatchReason returns a human-readable explanation of why the +// current branch diverges from its remotes, and whether a mismatch was found. +func (rs *RepoStatus) LocalRemoteMismatchReason() (reason string, ok bool) { locs := rs.CurrentBranchLocations() if len(locs) == 0 { - return nil + return "", false } var local *BranchLocation for i := range locs { @@ -224,7 +223,7 @@ func (rs *RepoStatus) LocalRemoteMismatchReasons() []string { } } if local == nil || !local.Exists { - return nil + return "", false } branchName := rs.Branch if branchName == "" { @@ -238,31 +237,31 @@ func (rs *RepoStatus) LocalRemoteMismatchReasons() []string { } hasRemote = true if !loc.Exists { - return []string{fmt.Sprintf("On remote %q, there is no same-named branch to compare with your local %q (refs/remotes/…/… missing).", loc.Name, branchName)} + return fmt.Sprintf("On remote %q, there is no same-named branch to compare with your local %q (refs/remotes/…/… missing).", loc.Name, branchName), true } if loc.TipHash != local.TipHash { if !loc.HistoriesUnrelated && loc.Incoming > 0 && loc.Outgoing == 0 { continue } if loc.HistoriesUnrelated { - return []string{fmt.Sprintf("On remote %q, %q has unrelated history to your local tip (no merge base).", loc.Name, branchName)} + return fmt.Sprintf("On remote %q, %q has unrelated history to your local tip (no merge base).", loc.Name, branchName), true } if loc.Incoming > 0 && loc.Outgoing > 0 { - return []string{fmt.Sprintf("On remote %q, %q diverged from the local tip (incoming +%d, outgoing %d).", loc.Name, branchName, loc.Incoming, loc.Outgoing)} + return fmt.Sprintf("On remote %q, %q diverged from the local tip (incoming +%d, outgoing %d).", loc.Name, branchName, loc.Incoming, loc.Outgoing), true } if loc.Outgoing > 0 { - return []string{fmt.Sprintf("On remote %q, your local %q is ahead: %d commit(s) not on that remote (tips differ or unpushed).", loc.Name, branchName, loc.Outgoing)} + return fmt.Sprintf("On remote %q, your local %q is ahead: %d commit(s) not on that remote (tips differ or unpushed).", loc.Name, branchName, loc.Outgoing), true } if loc.Incoming > 0 { - return []string{fmt.Sprintf("On remote %q, the same-named ref tip differs from your local %q (and it is not the \"only behind the remote\" case).", loc.Name, branchName)} + return fmt.Sprintf("On remote %q, the same-named ref tip differs from your local %q (and it is not the \"only behind the remote\" case).", loc.Name, branchName), true } - return []string{fmt.Sprintf("On remote %q, the same-named ref tip differs from your local branch %q (not the \"only behind the remote\" case).", loc.Name, branchName)} + return fmt.Sprintf("On remote %q, the same-named ref tip differs from your local branch %q (not the \"only behind the remote\" case).", loc.Name, branchName), true } } if hasRemote && local.UniqueCount > 0 { - return []string{fmt.Sprintf("Branch %q: %d commit(s) on local are not on any of the other refs this scan compared (e.g. same-named remotes) — see the Branches pane for detail.", branchName, local.UniqueCount)} + return fmt.Sprintf("Branch %q: %d commit(s) on local are not on any of the other refs this scan compared (e.g. same-named remotes) — see the Branches pane for detail.", branchName, local.UniqueCount), true } - return nil + return "", false } // PorcelainEntry is one parsed line of git status --porcelain (short format). diff --git a/ui/app.go b/ui/app.go index d4aae02..5b45a70 100644 --- a/ui/app.go +++ b/ui/app.go @@ -1,6 +1,7 @@ package ui import ( + "context" "log" "time" @@ -44,10 +45,8 @@ func Run(config *scanner.Config) error { log.SetOutput(m.logBuf) p := tea.NewProgram(m, tea.WithAltScreen(), tea.WithMouseCellMotion()) - if _, err := p.Run(); err != nil { - return err - } - return nil + _, err := p.Run() + return err } // Init starts the initial repository scan when the app launches. @@ -72,7 +71,7 @@ func (m *model) beginScan() tea.Cmd { progCh := m.scanProgressCh m.scanSpinner = newScanSpinner() go func() { - mgs, err := scanner.ScanWithProgress(m.config, func(p scanner.ScanProgress) { + mgs, err := scanner.ScanWithProgress(context.Background(), m.config, func(p scanner.ScanProgress) { select { case progCh <- p: default: diff --git a/ui/model.go b/ui/model.go index 0f79985..d8f2e1a 100644 --- a/ui/model.go +++ b/ui/model.go @@ -137,8 +137,8 @@ type model struct { layoutBranchBody int layoutLogBody int - // layoutBranchesOuter is the framed Diff (right) column outer width in the middle row (0 = default pct). - layoutBranchesOuter int + // layoutDiffColumnOuter is the framed Diff (right) column outer width in the middle row (0 = default pct). + layoutDiffColumnOuter int resizeDrag resizeSplit } diff --git a/ui/mouse_line_select_test.go b/ui/mouse_line_select_test.go index c789cae..46ca491 100644 --- a/ui/mouse_line_select_test.go +++ b/ui/mouse_line_select_test.go @@ -13,6 +13,8 @@ import ( // TestBubblesTableInternalFields guards against upstream field renames that // would cause bubblesTableSlice and bubblesTableViewportYOffset to silently // return zeros. Field names are verified against bubbles v1.0.0 (go.mod). +// It also calls the functions on a populated table to confirm non-zero values +// are returned — a silent zero return on rename would be caught here too. func TestBubblesTableInternalFields(t *testing.T) { typ := reflect.TypeOf(table.Model{}) for _, name := range []string{"start", "end", "viewport"} { @@ -24,6 +26,28 @@ func TestBubblesTableInternalFields(t *testing.T) { if _, ok := vpField.Type.FieldByName("YOffset"); !ok { t.Error("bubbles viewport.Model missing field \"YOffset\" — update bubblesTableViewportYOffset in mouse_line_select.go") } + + // Confirm non-zero values are returned for a populated, scrolled table so + // a silent zero-return from a field rename is caught at runtime, not just + // at field-existence time above. + rows := make([]table.Row, 20) + for i := range rows { + rows[i] = table.Row{"x"} + } + tbl := table.New( + table.WithColumns([]table.Column{{Title: "C", Width: 10}}), + table.WithRows(rows), + table.WithHeight(5), + table.WithFocused(true), + ) + // Move cursor well into the list so the start/end window is non-zero. + for i := 0; i < 15; i++ { + tbl.MoveDown(1) + } + start, end := bubblesTableSlice(tbl) + if start == 0 && end == 0 { + t.Error("bubblesTableSlice returned (0,0) on a scrolled table — field names may have changed") + } } func TestMouseRepoLineSelect(t *testing.T) { diff --git a/ui/mouse_resize.go b/ui/mouse_resize.go index 51593ed..2eaa799 100644 --- a/ui/mouse_resize.go +++ b/ui/mouse_resize.go @@ -201,7 +201,7 @@ func (m *model) applyResizeDrag(x, y int) { if !ok { return } - m.layoutBranchesOuter = rightOuter + m.layoutDiffColumnOuter = rightOuter default: return diff --git a/ui/status_layout.go b/ui/status_layout.go index 52e8855..7476bcf 100644 --- a/ui/status_layout.go +++ b/ui/status_layout.go @@ -146,8 +146,8 @@ func (m *model) innerWidth() int { // middleRowColumnOuterWidths splits the middle row: left (Status+Branches stack) vs right (Diff). func (m *model) middleRowColumnOuterWidths(total int) (leftOuter, rightOuter int) { - if m.layoutBranchesOuter > 0 { - right := m.layoutBranchesOuter + if m.layoutDiffColumnOuter > 0 { + right := m.layoutDiffColumnOuter right = max(layoutMinStatusBranchesColumn, min(right, total-layoutMinStatusBranchesColumn)) return total - right, right } @@ -380,26 +380,6 @@ func (m *model) refreshBranchContent(totalWidth int) { return } - // TODO - // branch := st.Branches - // if branch.Detached { - // locals := append([]scanner.LocalBranchRef(nil), branch.LocalBranches...) - // sortLocalBranchesByTipNewestFirst(locals) - // rows := make([]table.Row, 0, 1+len(locals)) - // rows = append(rows, table.Row{"(detached HEAD)", shortHash(branch.Branch), "-", "-"}) - // for _, lb := range locals { - // rows = append(rows, table.Row{ - // lb.Name, - // shortHash(lb.TipHash), - // relativeTime(lb.TipUnix), - // "-", - // }) - // } - // m.branchTable.SetRows(rows) - // m.branchTable.SetHeight(max(4, len(rows)+1)) - // return - // } - // create a deep copy and sort it locals := append([]scanner.LocalBranchRef(nil), st.FilteredBranches...) sortLocalBranchesByTipNewestFirst(locals) @@ -407,11 +387,10 @@ func (m *model) refreshBranchContent(totalWidth int) { // show only the dirty branches in the UI rows := make([]table.Row, 0, len(locals)) for _, lb := range locals { - // TODO: check detached head remote := branchRemoteSummaryFromLocations(lb.Locations) rows = append(rows, table.Row{ - lb.GetDisplayName(), + lb.DisplayName(), shortHash(lb.TipHash), relativeTime(lb.TipUnix), remote, diff --git a/ui/update.go b/ui/update.go index 2682893..d61a117 100644 --- a/ui/update.go +++ b/ui/update.go @@ -28,11 +28,11 @@ func (m *model) handleWindowSize(msg tea.WindowSizeMsg) (tea.Model, tea.Cmd) { m.height = msg.Height if m.logVP.Height == 0 { inner := max(layoutMinInnerContentWidth, msg.Width-4) - m.logVP = viewport.New(inner, layoutMinInnerContentWidth) + m.logVP = viewport.New(inner, layoutDefaultTableViewRows) } if m.diffVP.Height == 0 { inner := max(layoutMinInnerContentWidth, msg.Width-4) - m.diffVP = viewport.New(inner, layoutMinInnerContentWidth) + m.diffVP = viewport.New(inner, layoutDefaultTableViewRows) } m.syncViewports() return m, nil @@ -99,6 +99,17 @@ func (m *model) handleHelpOverlayKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { } } +// handleConfirmNavKey handles the shared n/y/left/h/right/l navigation inside +// any yes/no confirmation dialog. +func (m *model) handleConfirmNavKey(msg tea.KeyMsg) { + switch msg.String() { + case "n", "left", "h": + m.deleteConfirmYes = false + case "y", "right", "l": + m.deleteConfirmYes = true + } +} + // handleDeleteRepoConfirmKey processes keys while the delete-directory confirmation is open. func (m *model) handleDeleteRepoConfirmKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { switch msg.String() { @@ -107,18 +118,6 @@ func (m *model) handleDeleteRepoConfirmKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) case "esc": m.deleteRepoConfirmOpen = false return m, nil - case "n": - m.deleteConfirmYes = false - return m, nil - case "y": - m.deleteConfirmYes = true - return m, nil - case "left", "h": - m.deleteConfirmYes = false - return m, nil - case "right", "l": - m.deleteConfirmYes = true - return m, nil case "enter": m.deleteRepoConfirmOpen = false if !m.deleteConfirmYes { @@ -127,6 +126,7 @@ func (m *model) handleDeleteRepoConfirmKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) m.deleteSelectedRepoFromDisk() return m, nil default: + m.handleConfirmNavKey(msg) return m, nil } } @@ -140,18 +140,6 @@ func (m *model) handleCheckoutStatusFileConfirmKey(msg tea.KeyMsg) (tea.Model, t m.checkoutStatusFileConfirmOpen = false m.checkoutStatusFilePendingRel = "" return m, nil - case "n": - m.deleteConfirmYes = false - return m, nil - case "y": - m.deleteConfirmYes = true - return m, nil - case "left", "h": - m.deleteConfirmYes = false - return m, nil - case "right", "l": - m.deleteConfirmYes = true - return m, nil case "enter": m.checkoutStatusFileConfirmOpen = false pending := m.checkoutStatusFilePendingRel @@ -169,6 +157,7 @@ func (m *model) handleCheckoutStatusFileConfirmKey(msg tea.KeyMsg) (tea.Model, t m.syncViewports() return m, nil default: + m.handleConfirmNavKey(msg) return m, nil } } @@ -182,18 +171,6 @@ func (m *model) handleDeleteStatusFileConfirmKey(msg tea.KeyMsg) (tea.Model, tea m.deleteStatusFileConfirmOpen = false m.deleteStatusFilePendingRel = "" return m, nil - case "n": - m.deleteConfirmYes = false - return m, nil - case "y": - m.deleteConfirmYes = true - return m, nil - case "left", "h": - m.deleteConfirmYes = false - return m, nil - case "right", "l": - m.deleteConfirmYes = true - return m, nil case "enter": m.deleteStatusFileConfirmOpen = false pending := m.deleteStatusFilePendingRel @@ -204,6 +181,7 @@ func (m *model) handleDeleteStatusFileConfirmKey(msg tea.KeyMsg) (tea.Model, tea m.deletePendingStatusFileFromDisk(pending) return m, nil default: + m.handleConfirmNavKey(msg) return m, nil } }