From 12d6be60294358644d83e76b97a5f83b883bacf3 Mon Sep 17 00:00:00 2001 From: takeshi yamashita <220619+takecy@users.noreply.github.com> Date: Sun, 26 Apr 2026 03:19:42 +0900 Subject: [PATCH 01/11] chore: ignore worktree and tasks directories Co-Authored-By: Claude Opus 4.7 (1M context) --- .gitignore | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index a5b1187..321d933 100644 --- a/.gitignore +++ b/.gitignore @@ -29,4 +29,7 @@ gs_* .vscode dist -gih_dev \ No newline at end of file +gih_dev + +.worktrees/ +tasks/ \ No newline at end of file From 99ae654c864ab74d503e1f43c45b17de4d36a38c Mon Sep 17 00:00:00 2001 From: takeshi yamashita <220619+takecy@users.noreply.github.com> Date: Sun, 26 Apr 2026 03:19:56 +0900 Subject: [PATCH 02/11] feat(syncer): kill-on-timeout, per-repo timeout, and run summary Replace channel-based throttle with errgroup.SetLimit so the main goroutine no longer blocks while enqueueing repos beyond the concurrency limit. Move the timeout boundary into each goroutine via context.WithTimeout(parent, perRepoTimeout) so the README's "timeout per directory" semantics actually hold and so a single slow repo cannot starve the others. Switch Gitter.Git to exec.CommandContext and introduce an Executor interface so cancellation reaches the spawned git process (and its helper subprocesses via SIGPIPE) and so tests can swap in a fake executor without forking real git. Aggregate per-repo outcomes into runStats (succeeded / failed / timedOut) and emit a final summary line plus repository lists for failed and timed-out runs, reusing the previously-unused printer.PrintRepoErr formatter. Add focused unit tests for execute() and filterRepos() using a mutex-wrapped io.Discard writer so -race stays clean even though the Printer itself is not yet mutex-protected. Co-Authored-By: Claude Opus 4.7 (1M context) --- syncer/git.go | 14 ++- syncer/git_test.go | 5 +- syncer/syncer.go | 202 +++++++++++++++++++++++++---------------- syncer/syncer_test.go | 206 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 345 insertions(+), 82 deletions(-) create mode 100644 syncer/syncer_test.go diff --git a/syncer/git.go b/syncer/git.go index 9a52b10..1f88496 100644 --- a/syncer/git.go +++ b/syncer/git.go @@ -2,12 +2,19 @@ package syncer import ( "bytes" + "context" "fmt" "io" "os" "os/exec" ) +// Executor abstracts git command execution. Defined as an interface so that +// callers can substitute fakes in tests without spawning real processes. +type Executor interface { + Git(ctx context.Context, command, dir string, args ...string) (msg, errMsg string, err error) +} + // Gitter is struct type Gitter struct { writer io.Writer @@ -35,13 +42,14 @@ func (*Gitter) IsExist() error { return nil } -// Git is execute git command -func (g *Gitter) Git(command, dir string, args ...string) (msg, errMsg string, err error) { +// Git is execute git command. The given context is forwarded to exec.CommandContext +// so that cancellation (e.g. per-repo timeout) terminates the underlying process. +func (g *Gitter) Git(ctx context.Context, command, dir string, args ...string) (msg, errMsg string, err error) { wr := new(bytes.Buffer) errWr := new(bytes.Buffer) cmdArgs := append([]string{command}, args...) - cmd := exec.Command("git", cmdArgs...) + cmd := exec.CommandContext(ctx, "git", cmdArgs...) cmd.Dir = dir cmd.Stdin = os.Stdin cmd.Stdout = wr diff --git a/syncer/git_test.go b/syncer/git_test.go index 518d19a..2d485e5 100644 --- a/syncer/git_test.go +++ b/syncer/git_test.go @@ -1,6 +1,7 @@ package syncer import ( + "context" "os" "testing" @@ -20,7 +21,7 @@ func TestFetch(t *testing.T) { gi := NewGitter(os.Stdout, os.Stderr) args := []string{"--all", "-p"} - _, _, err := gi.Git("fetch", ".", args...) + _, _, err := gi.Git(context.Background(), "fetch", ".", args...) is.NoErr(err) } @@ -30,6 +31,6 @@ func TestPull(t *testing.T) { gi := NewGitter(os.Stdout, os.Stderr) args := []string{"--verbose"} - _, _, err := gi.Git("pull", ".", args...) + _, _, err := gi.Git(context.Background(), "pull", ".", args...) is.NoErr(err) } diff --git a/syncer/syncer.go b/syncer/syncer.go index 5a2dfd6..1c510c5 100644 --- a/syncer/syncer.go +++ b/syncer/syncer.go @@ -6,6 +6,8 @@ import ( "os" "path/filepath" "regexp" + "sync" + "sync/atomic" "time" "github.com/pkg/errors" @@ -37,14 +39,44 @@ type Sync struct { ConNum int // Gitter is the git command executor instance. - Gitter *Gitter + Gitter Executor +} + +// failedRepo carries the repository path together with the error that caused +// the failure so that it can be reported in the final summary. +type failedRepo struct { + Repo string + Err error +} + +// runStats accumulates per-repository outcomes safely from concurrent goroutines. +type runStats struct { + mu sync.Mutex + succeeded []string + failed []failedRepo + timedOut []string +} + +func (s *runStats) addSuccess(r string) { + s.mu.Lock() + defer s.mu.Unlock() + s.succeeded = append(s.succeeded, r) +} + +func (s *runStats) addFailed(r string, e error) { + s.mu.Lock() + defer s.mu.Unlock() + s.failed = append(s.failed, failedRepo{Repo: r, Err: e}) +} + +func (s *runStats) addTimedOut(r string) { + s.mu.Lock() + defer s.mu.Unlock() + s.timedOut = append(s.timedOut, r) } // Run is execute logic func (s *Sync) Run() (err error) { - // - // list target directories - // dirs, err := ListDirs() if err != nil { fmt.Fprintf(os.Stderr, "%v\n", err) @@ -56,127 +88,143 @@ func (s *Sync) Run() (err error) { } fmt.Printf("repositories are found: (%d)\n", len(dirs)) - s.Writer.PrintCmd(s.Command, s.Options) - // - // compile regex patterns for performance - // + repos, err := s.filterRepos(dirs) + if err != nil { + return err + } + if len(repos) == 0 { + s.Writer.PrintMsg("No target repositories.") + return + } + s.Writer.PrintMsg(fmt.Sprintf("target repositories: (%d)", len(repos))) + + perRepoTimeout, err := time.ParseDuration(s.TimeOut) + if err != nil { + fmt.Fprintf(os.Stderr, "%s\n", "invalid timeout value.") + os.Exit(1) + } + + stats := s.execute(context.Background(), repos, perRepoTimeout) + s.printSummary(stats) + return +} + +// filterRepos applies the target/ignore regex patterns to the discovered +// directories. It is split out from Run() so tests can exercise the matching +// logic without scanning the filesystem. +func (s *Sync) filterRepos(dirs []string) ([]string, error) { var ignoreRegex, targetRegex *regexp.Regexp if s.IgnoreDir != "" { - ignoreRegex, err = regexp.Compile(s.IgnoreDir) + re, err := regexp.Compile(s.IgnoreDir) if err != nil { - return errors.Wrapf(err, "invalid ignore directory regex pattern: %s", s.IgnoreDir) + return nil, errors.Wrapf(err, "invalid ignore directory regex pattern: %s", s.IgnoreDir) } + ignoreRegex = re } if s.TargetDir != "" { - targetRegex, err = regexp.Compile(s.TargetDir) + re, err := regexp.Compile(s.TargetDir) if err != nil { - return errors.Wrapf(err, "invalid target directory regex pattern: %s", s.TargetDir) + return nil, errors.Wrapf(err, "invalid target directory regex pattern: %s", s.TargetDir) } + targetRegex = re } - // - // retrieve target repos - // repos := make([]string, 0, len(dirs)) for _, d := range dirs { if ignoreRegex != nil && ignoreRegex.MatchString(d) { continue } - if targetRegex != nil && !targetRegex.MatchString(d) { continue } - repos = append(repos, d) } + return repos, nil +} - if len(repos) == 0 { - s.Writer.PrintMsg("No target repositories.") - return - } - - targetRepoNum := len(repos) - s.Writer.PrintMsg(fmt.Sprintf("target repositories: (%d)", targetRepoNum)) - - // - // execute command - // - eg := errgroup.Group{} +// execute runs the git command across all repos in parallel, throttled by +// ConNum. Each invocation is bounded by perRepoTimeout via a derived context; +// the parent context is *not* timed out so a slow repo never starves the +// remaining ones. +func (s *Sync) execute(parent context.Context, repos []string, perRepoTimeout time.Duration) *runStats { + stats := &runStats{} + total := len(repos) + var done atomic.Int64 start := time.Now() - throttle := make(chan struct{}, s.ConNum) - // set up context - to, err := time.ParseDuration(s.TimeOut) - if err != nil { - fmt.Fprintf(os.Stderr, "%s\n", "invalid timeout value.") - os.Exit(1) + eg := &errgroup.Group{} + if s.ConNum > 0 { + eg.SetLimit(s.ConNum) } - ctx, cancel := context.WithTimeout(context.Background(), to) - defer cancel() - - for i := range repos { - num := i + 1 - r := repos[i] - throttle <- struct{}{} + for _, r := range repos { + r := r eg.Go(func() error { - defer func() { - <-throttle - }() + ctx, cancel := context.WithTimeout(parent, perRepoTimeout) + defer cancel() err := s.execCmd(ctx, r) - if err != nil { - s.Writer.PrintMsgErr(fmt.Sprintf("Failed: %s\n%v", r, err)) - } else { + switch { + case err == nil: + stats.addSuccess(r) s.Writer.PrintMsg(fmt.Sprintf("Success: %s\n", r)) + case errors.Is(ctx.Err(), context.DeadlineExceeded): + stats.addTimedOut(r) + s.Writer.PrintMsgErr(fmt.Sprintf("Timeout: %s", r)) + default: + stats.addFailed(r, err) + s.Writer.PrintMsgErr(fmt.Sprintf("Failed: %s\n%v", r, err)) } - s.Writer.PrintMsg(fmt.Sprintf("Done: %d/%d", num, targetRepoNum)) + n := done.Add(1) + s.Writer.PrintMsg(fmt.Sprintf("Done: %d/%d", n, total)) return nil }) } - - // Handle timeout in a separate goroutine - done := make(chan struct{}) - go func() { - defer close(done) - if err := eg.Wait(); err != nil { - s.Writer.PrintMsgErr(fmt.Sprintf("Error.exists: %v", err)) - } - }() - - select { - case <-done: - // All goroutines completed successfully - case <-ctx.Done(): - s.Writer.PrintMsgErr(fmt.Sprintf("---- Timeouted (%v) ----", time.Since(start).String())) - // returns no error - return - } + _ = eg.Wait() s.Writer.PrintMsg(fmt.Sprintf("All done. (%v)", time.Since(start).Round(time.Millisecond))) - return + return stats } // execCmd is execute git command -func (s *Sync) execCmd(ctx context.Context, d string) (err error) { +func (s *Sync) execCmd(ctx context.Context, d string) error { absPath, err := filepath.Abs(d) if err != nil { - err = errors.Wrapf(err, "get.abs.failed: %s", d) - s.Writer.Error(printer.Result{Err: err}) - return + return errors.Wrapf(err, "get.abs.failed: %s", d) } - msg, errMsg, err := s.Gitter.Git(s.Command, absPath, s.Options...) + msg, errMsg, err := s.Gitter.Git(ctx, s.Command, absPath, s.Options...) if err != nil { - s.Writer.Error(printer.Result{Repo: absPath, Err: errors.Wrapf(err, "%s", errMsg)}) - } else { - s.Writer.Print(printer.Result{Repo: absPath, Msg: msg}) + return errors.Wrapf(err, "%s", errMsg) } + s.Writer.Print(printer.Result{Repo: absPath, Msg: msg}) + return nil +} - return +// printSummary emits the post-run summary: counts, plus the list of failed +// and timed-out repositories. Reuses printer.PrintRepoErr (previously unused). +func (s *Sync) printSummary(stats *runStats) { + stats.mu.Lock() + defer stats.mu.Unlock() + + s.Writer.PrintMsg(fmt.Sprintf( + "Summary: success=%d failed=%d timeout=%d", + len(stats.succeeded), len(stats.failed), len(stats.timedOut), + )) + + if len(stats.failed) > 0 { + repos := make([]string, len(stats.failed)) + for i, f := range stats.failed { + repos[i] = f.Repo + } + s.Writer.PrintRepoErr("Failed repositories:", repos) + } + if len(stats.timedOut) > 0 { + s.Writer.PrintRepoErr("Timed out repositories:", stats.timedOut) + } } diff --git a/syncer/syncer_test.go b/syncer/syncer_test.go new file mode 100644 index 0000000..b395ed3 --- /dev/null +++ b/syncer/syncer_test.go @@ -0,0 +1,206 @@ +package syncer + +import ( + "context" + "errors" + "io" + "path/filepath" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/matryer/is" + "github.com/takecy/git-here/printer" +) + +// syncWriter serialises writes from multiple goroutines so that tests using +// the real Printer don't trip the race detector. Without this wrapper, the +// concurrent template.Execute calls inside Printer would race on a shared +// io.Writer (Printer itself is not yet mutex-protected). +type syncWriter struct { + mu sync.Mutex + w io.Writer +} + +func (s *syncWriter) Write(p []byte) (int, error) { + s.mu.Lock() + defer s.mu.Unlock() + return s.w.Write(p) +} + +type execFn func(ctx context.Context, command, dir string, args ...string) (string, string, error) + +type fakeExecutor struct{ fn execFn } + +func (f *fakeExecutor) Git(ctx context.Context, command, dir string, args ...string) (string, string, error) { + return f.fn(ctx, command, dir, args...) +} + +func newSyncWithFake(fn execFn, conNum int) *Sync { + sw := &syncWriter{w: io.Discard} + return &Sync{ + Command: "status", + ConNum: conNum, + Gitter: &fakeExecutor{fn: fn}, + Writer: printer.NewPrinter(sw, sw), + } +} + +func TestSync_Execute(t *testing.T) { + t.Run("all success", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + s := newSyncWithFake(func(_ context.Context, _, _ string, _ ...string) (string, string, error) { + return "ok", "", nil + }, 4) + + stats := s.execute(context.Background(), []string{"a", "b", "c"}, time.Second) + is.Equal(len(stats.succeeded), 3) + is.Equal(len(stats.failed), 0) + is.Equal(len(stats.timedOut), 0) + }) + + t.Run("partial failure", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + s := newSyncWithFake(func(_ context.Context, _, dir string, _ ...string) (string, string, error) { + if filepath.Base(dir) == "b" { + return "", "boom", errors.New("fail") + } + return "ok", "", nil + }, 4) + + stats := s.execute(context.Background(), []string{"a", "b", "c"}, time.Second) + is.Equal(len(stats.succeeded), 2) + is.Equal(len(stats.failed), 1) + is.Equal(filepath.Base(stats.failed[0].Repo), "b") + }) + + t.Run("timeout is classified separately", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + s := newSyncWithFake(func(ctx context.Context, _, _ string, _ ...string) (string, string, error) { + <-ctx.Done() + return "", "", ctx.Err() + }, 1) + + stats := s.execute(context.Background(), []string{"a"}, 50*time.Millisecond) + is.Equal(len(stats.timedOut), 1) + is.Equal(len(stats.failed), 0) + is.Equal(len(stats.succeeded), 0) + }) + + t.Run("respects ConNum throttle", func(t *testing.T) { + // Time-sensitive: don't run in parallel with the rest of the suite. + is := is.New(t) + + var inflight, peak atomic.Int64 + + // Atomic max via CompareAndSwap loop. The naive + // `if n > peak.Load() { peak.Store(n) }` form races and may miss + // the real peak. + recordPeak := func(n int64) { + for { + cur := peak.Load() + if n <= cur { + return + } + if peak.CompareAndSwap(cur, n) { + return + } + } + } + + s := newSyncWithFake(func(_ context.Context, _, _ string, _ ...string) (string, string, error) { + n := inflight.Add(1) + recordPeak(n) + time.Sleep(20 * time.Millisecond) + inflight.Add(-1) + return "", "", nil + }, 2) + + s.execute(context.Background(), []string{"a", "b", "c", "d", "e"}, time.Second) + + is.True(peak.Load() >= 1) + is.True(peak.Load() <= int64(s.ConNum)) + }) + + t.Run("all timeout when every repo hangs", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + s := newSyncWithFake(func(ctx context.Context, _, _ string, _ ...string) (string, string, error) { + <-ctx.Done() + return "", "", ctx.Err() + }, 1) + + stats := s.execute(context.Background(), []string{"a", "b", "c"}, 30*time.Millisecond) + is.Equal(len(stats.timedOut), 3) + is.Equal(len(stats.failed), 0) + is.Equal(len(stats.succeeded), 0) + }) +} + +func TestSync_FilterRepos(t *testing.T) { + t.Run("no filters returns all", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + s := &Sync{} + got, err := s.filterRepos([]string{"a", "b", "c"}) + is.NoErr(err) + is.Equal(got, []string{"a", "b", "c"}) + }) + + t.Run("target only", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + s := &Sync{TargetDir: "^api"} + got, err := s.filterRepos([]string{"api-foo", "web-bar", "api-baz"}) + is.NoErr(err) + is.Equal(got, []string{"api-foo", "api-baz"}) + }) + + t.Run("ignore only", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + s := &Sync{IgnoreDir: "^test"} + got, err := s.filterRepos([]string{"test-a", "prod-b", "test-c"}) + is.NoErr(err) + is.Equal(got, []string{"prod-b"}) + }) + + t.Run("target and ignore combined", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + s := &Sync{TargetDir: "service$", IgnoreDir: "deprecated"} + got, err := s.filterRepos([]string{"user-service", "deprecated-service", "web-app"}) + is.NoErr(err) + is.Equal(got, []string{"user-service"}) + }) + + t.Run("invalid target regex returns error", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + s := &Sync{TargetDir: "[invalid("} + _, err := s.filterRepos([]string{"a"}) + is.True(err != nil) + }) + + t.Run("invalid ignore regex returns error", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + s := &Sync{IgnoreDir: "[invalid("} + _, err := s.filterRepos([]string{"a"}) + is.True(err != nil) + }) +} From fef5fefc26bbad19e1f68b0bddc16c8174eba6ab Mon Sep 17 00:00:00 2001 From: takeshi yamashita <220619+takecy@users.noreply.github.com> Date: Sun, 26 Apr 2026 03:22:07 +0900 Subject: [PATCH 03/11] chore(gih): bump version to 0.16.0 Co-Authored-By: Claude Opus 4.7 (1M context) --- gih/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gih/main.go b/gih/main.go index fe91067..9649d00 100644 --- a/gih/main.go +++ b/gih/main.go @@ -12,7 +12,7 @@ import ( // set by build var ( - version = "0.15.0" + version = "0.16.0" goversion = "1.26.2" ) From 38063dde909466972dc13f4dc68cad13684bfb16 Mon Sep 17 00:00:00 2001 From: takeshi yamashita <220619+takecy@users.noreply.github.com> Date: Sun, 26 Apr 2026 03:43:38 +0900 Subject: [PATCH 04/11] refactor(printer): precompile templates and drop t() helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each Print* method previously re-parsed its template on every call via template.Must(template.New(...).Funcs(helpers).Parse(...)). Move the six templates to package-level vars so parsing happens once at init. The t() helper that selected between successTmpl and errTmpl is no longer needed; Print and Error reference successTpl / errTpl directly. Pure refactor — no observable behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) --- printer/print.go | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/printer/print.go b/printer/print.go index a7d492f..3c00887 100644 --- a/printer/print.go +++ b/printer/print.go @@ -44,6 +44,18 @@ const repoErrTmpl = ` {{ range .Repos }}- {{ . }} {{ end }}` +// Templates are parsed once at package init. text/template.Template.Execute is +// safe for concurrent use, so the same parsed template can be shared across +// goroutines without re-parsing on every call. +var ( + successTpl = template.Must(template.New("success").Funcs(helpers).Parse(successTmpl)) + errTpl = template.Must(template.New("err").Funcs(helpers).Parse(errTmpl)) + cmdTpl = template.Must(template.New("cmd").Funcs(helpers).Parse(cmdTmpl)) + msgTpl = template.Must(template.New("msg").Funcs(helpers).Parse(msgTmpl)) + msgErrTpl = template.Must(template.New("msgErr").Funcs(helpers).Parse(msgErrTmpl)) + repoErrTpl = template.Must(template.New("repoErr").Funcs(helpers).Parse(repoErrTmpl)) +) + // Printer is struct type Printer struct { writer io.Writer @@ -78,9 +90,7 @@ func (p *Printer) PrintCmd(cmd string, options []string) { Cmd string Ops string } - t := template.Must(template.New("item").Funcs(helpers).Parse(cmdTmpl)) - err := t.Execute(p.writer, cmds{Cmd: cmd, Ops: strings.Join(options, " ")}) - if err != nil { + if err := cmdTpl.Execute(p.writer, cmds{Cmd: cmd, Ops: strings.Join(options, " ")}); err != nil { log.Println(err) } } @@ -90,9 +100,7 @@ func (p *Printer) PrintMsg(msg string) { type message struct { Msg string } - t := template.Must(template.New("msg").Funcs(helpers).Parse(msgTmpl)) - err := t.Execute(p.writer, message{Msg: msg}) - if err != nil { + if err := msgTpl.Execute(p.writer, message{Msg: msg}); err != nil { log.Println(err) } } @@ -102,9 +110,7 @@ func (p *Printer) PrintMsgErr(msg string) { type message struct { Msg string } - t := template.Must(template.New("msg").Funcs(helpers).Parse(msgErrTmpl)) - err := t.Execute(p.writer, message{Msg: msg}) - if err != nil { + if err := msgErrTpl.Execute(p.writer, message{Msg: msg}); err != nil { log.Println(err) } } @@ -115,17 +121,14 @@ func (p *Printer) PrintRepoErr(msg string, repos []string) { Msg string Repos []string } - t := template.Must(template.New("msg").Funcs(helpers).Parse(repoErrTmpl)) - err := t.Execute(p.writer, message{Msg: msg, Repos: repos}) - if err != nil { + if err := repoErrTpl.Execute(p.writer, message{Msg: msg, Repos: repos}); err != nil { log.Println(err) } } // Print prints result func (p *Printer) Print(res Result) { - err := t(true).Execute(p.writer, res) - if err != nil { + if err := successTpl.Execute(p.writer, res); err != nil { log.Println(err) } } @@ -133,15 +136,7 @@ func (p *Printer) Print(res Result) { // Error prints error func (p *Printer) Error(res Result) { res.Msg = res.Err.Error() - err := t(false).Execute(p.errWriter, res) - if err != nil { + if err := errTpl.Execute(p.errWriter, res); err != nil { log.Println(err) } } - -func t(isSuccess bool) *template.Template { - if isSuccess { - return template.Must(template.New("item").Funcs(helpers).Parse(successTmpl)) - } - return template.Must(template.New("item").Funcs(helpers).Parse(errTmpl)) -} From 599fa1db0da3b982d252f70c3c0d01b4df778432 Mon Sep 17 00:00:00 2001 From: takeshi yamashita <220619+takecy@users.noreply.github.com> Date: Sun, 26 Apr 2026 03:44:20 +0900 Subject: [PATCH 05/11] fix(printer): serialize Print calls to prevent output interleaving MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each Print* method calls text/template.Execute, which in turn issues multiple io.Writer.Write calls. Without external synchronisation, two goroutines emitting messages through the Printer can interleave their Writes — observed empirically in the previous commit's E2E run as "Timeout: r1Done: 1/3" (a result line and a Done line concatenated without the intervening newline). Add a single sync.Mutex to Printer, locked around every Execute call. A single mutex (rather than separate mutexes for writer and errWriter) is intentional: in typical CLI use both end up at the same TTY, and we want stdout/stderr messages to also stay non-overlapping. Drop the syncWriter helper from syncer_test.go: it was wrapping io.Discard purely to defend against the unprotected Printer, which is no longer needed. Add printer/print_test.go covering each public method concurrently, plus a TestPrinter_SharedMutex_AcrossWriterAndErrWriter test that wires both writer and errWriter to the same probe writer and asserts that the maximum number of in-flight Write calls observed is 1 — this directly verifies the single-mutex design rather than just exercising the methods in parallel. Co-Authored-By: Claude Opus 4.7 (1M context) --- printer/print.go | 17 ++++ printer/print_test.go | 188 ++++++++++++++++++++++++++++++++++++++++++ syncer/syncer_test.go | 19 +---- 3 files changed, 206 insertions(+), 18 deletions(-) create mode 100644 printer/print_test.go diff --git a/printer/print.go b/printer/print.go index 3c00887..5d1cc97 100644 --- a/printer/print.go +++ b/printer/print.go @@ -5,6 +5,7 @@ import ( "log" "os" "strings" + "sync" "text/template" "github.com/fatih/color" @@ -58,6 +59,10 @@ var ( // Printer is struct type Printer struct { + // mu serialises template.Execute calls so that the multiple Write calls + // emitted per Execute don't interleave across goroutines, even when + // writer and errWriter end up at the same TTY. + mu sync.Mutex writer io.Writer errWriter io.Writer } @@ -90,6 +95,8 @@ func (p *Printer) PrintCmd(cmd string, options []string) { Cmd string Ops string } + p.mu.Lock() + defer p.mu.Unlock() if err := cmdTpl.Execute(p.writer, cmds{Cmd: cmd, Ops: strings.Join(options, " ")}); err != nil { log.Println(err) } @@ -100,6 +107,8 @@ func (p *Printer) PrintMsg(msg string) { type message struct { Msg string } + p.mu.Lock() + defer p.mu.Unlock() if err := msgTpl.Execute(p.writer, message{Msg: msg}); err != nil { log.Println(err) } @@ -110,6 +119,8 @@ func (p *Printer) PrintMsgErr(msg string) { type message struct { Msg string } + p.mu.Lock() + defer p.mu.Unlock() if err := msgErrTpl.Execute(p.writer, message{Msg: msg}); err != nil { log.Println(err) } @@ -121,6 +132,8 @@ func (p *Printer) PrintRepoErr(msg string, repos []string) { Msg string Repos []string } + p.mu.Lock() + defer p.mu.Unlock() if err := repoErrTpl.Execute(p.writer, message{Msg: msg, Repos: repos}); err != nil { log.Println(err) } @@ -128,6 +141,8 @@ func (p *Printer) PrintRepoErr(msg string, repos []string) { // Print prints result func (p *Printer) Print(res Result) { + p.mu.Lock() + defer p.mu.Unlock() if err := successTpl.Execute(p.writer, res); err != nil { log.Println(err) } @@ -136,6 +151,8 @@ func (p *Printer) Print(res Result) { // Error prints error func (p *Printer) Error(res Result) { res.Msg = res.Err.Error() + p.mu.Lock() + defer p.mu.Unlock() if err := errTpl.Execute(p.errWriter, res); err != nil { log.Println(err) } diff --git a/printer/print_test.go b/printer/print_test.go new file mode 100644 index 0000000..95c81ca --- /dev/null +++ b/printer/print_test.go @@ -0,0 +1,188 @@ +package printer + +import ( + "bytes" + "errors" + "io" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/matryer/is" +) + +func TestPrinter_PrintMsg_ConcurrentSafe(t *testing.T) { + is := is.New(t) + + var buf bytes.Buffer + p := NewPrinter(&buf, &buf) + + const N = 200 + var wg sync.WaitGroup + for i := 0; i < N; i++ { + wg.Add(1) + go func() { + defer wg.Done() + p.PrintMsg("hello") + }() + } + wg.Wait() + + out := strings.TrimRight(buf.String(), "\n") + lines := strings.Split(out, "\n") + is.Equal(len(lines), N) + for _, l := range lines { + is.True(strings.Contains(l, "hello")) + } +} + +func TestPrinter_Print_ConcurrentSafe(t *testing.T) { + // successTmpl: "{{.Repo}}\n {{.Msg}}\n" — 2 lines per call. If a lock is + // missing here, the two lines from one call can interleave with another + // call's lines, breaking the strict pair structure verified below. + is := is.New(t) + + var buf bytes.Buffer + p := NewPrinter(&buf, &buf) + + const N = 200 + var wg sync.WaitGroup + for i := 0; i < N; i++ { + wg.Add(1) + go func() { + defer wg.Done() + p.Print(Result{Repo: "/path/repo", Msg: "ok"}) + }() + } + wg.Wait() + + lines := strings.Split(buf.String(), "\n") + if len(lines) > 0 && lines[len(lines)-1] == "" { + lines = lines[:len(lines)-1] + } + is.Equal(len(lines), N*2) + + for i := 0; i < len(lines); i += 2 { + is.True(strings.Contains(lines[i], "/path/repo")) + is.True(strings.HasPrefix(lines[i+1], " ")) + is.True(strings.Contains(lines[i+1], "ok")) + } +} + +func TestPrinter_Error_ConcurrentSafe(t *testing.T) { + is := is.New(t) + + var errBuf bytes.Buffer + p := NewPrinter(io.Discard, &errBuf) + + const N = 100 + var wg sync.WaitGroup + for i := 0; i < N; i++ { + wg.Add(1) + go func() { + defer wg.Done() + p.Error(Result{Repo: "/path/r", Err: errors.New("boom")}) + }() + } + wg.Wait() + + lines := strings.Split(errBuf.String(), "\n") + if len(lines) > 0 && lines[len(lines)-1] == "" { + lines = lines[:len(lines)-1] + } + is.Equal(len(lines), N*2) + for i := 0; i < len(lines); i += 2 { + is.True(strings.Contains(lines[i], "/path/r")) + is.True(strings.HasPrefix(lines[i+1], " ")) + is.True(strings.Contains(lines[i+1], "boom")) + } +} + +// concurrencyProbe is a thin io.Writer that records the maximum number of +// concurrent in-flight Write calls. Used to verify that a single mutex covers +// BOTH writer and errWriter in Printer. +type concurrencyProbe struct { + inflight atomic.Int32 + peak atomic.Int32 +} + +func (c *concurrencyProbe) Write(p []byte) (int, error) { + n := c.inflight.Add(1) + for { + cur := c.peak.Load() + if n <= cur { + break + } + if c.peak.CompareAndSwap(cur, n) { + break + } + } + // Tiny sleep widens the race window so that, if Print and Error were + // protected by separate mutexes, the probe would observe inflight >= 2. + time.Sleep(20 * time.Microsecond) + c.inflight.Add(-1) + return len(p), nil +} + +func TestPrinter_SharedMutex_AcrossWriterAndErrWriter(t *testing.T) { + // Wires both writer and errWriter to the same probe. Print uses writer, + // Error uses errWriter. With a single mutex covering both, no two Writes + // can be in-flight at once → peak == 1. With separate mutexes, peak >= 2 + // would be observed with high probability. + is := is.New(t) + + probe := &concurrencyProbe{} + p := NewPrinter(probe, probe) + + const N = 200 + var wg sync.WaitGroup + for i := 0; i < N; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + if i%2 == 0 { + p.Print(Result{Repo: "/r", Msg: "ok"}) + } else { + p.Error(Result{Repo: "/r", Err: errors.New("boom")}) + } + }(i) + } + wg.Wait() + + is.Equal(int(probe.peak.Load()), 1) +} + +func TestPrinter_MixedPrints_ConcurrentSafe(t *testing.T) { + // Smoke test exercising every public Printer method concurrently. Catches + // any lock omission via the race detector even when message-shape tests + // don't apply (e.g. PrintCmd, PrintRepoErr). + var buf bytes.Buffer + p := NewPrinter(&buf, &buf) + + var wg sync.WaitGroup + for i := 0; i < 200; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + switch i % 7 { + case 0: + p.PrintMsg("m") + case 1: + p.PrintMsgErr("e") + case 2: + p.PrintRepoErr("re", []string{"a", "b"}) + case 3: + p.PrintCmd("status", []string{"--short"}) + case 4: + p.Print(Result{Repo: "/path/r", Msg: "ok"}) + case 5: + p.Error(Result{Repo: "/path/r", Err: errors.New("boom")}) + case 6: + p.PrintMsg("another") + } + }(i) + } + wg.Wait() +} diff --git a/syncer/syncer_test.go b/syncer/syncer_test.go index b395ed3..43a082b 100644 --- a/syncer/syncer_test.go +++ b/syncer/syncer_test.go @@ -5,7 +5,6 @@ import ( "errors" "io" "path/filepath" - "sync" "sync/atomic" "testing" "time" @@ -14,21 +13,6 @@ import ( "github.com/takecy/git-here/printer" ) -// syncWriter serialises writes from multiple goroutines so that tests using -// the real Printer don't trip the race detector. Without this wrapper, the -// concurrent template.Execute calls inside Printer would race on a shared -// io.Writer (Printer itself is not yet mutex-protected). -type syncWriter struct { - mu sync.Mutex - w io.Writer -} - -func (s *syncWriter) Write(p []byte) (int, error) { - s.mu.Lock() - defer s.mu.Unlock() - return s.w.Write(p) -} - type execFn func(ctx context.Context, command, dir string, args ...string) (string, string, error) type fakeExecutor struct{ fn execFn } @@ -38,12 +22,11 @@ func (f *fakeExecutor) Git(ctx context.Context, command, dir string, args ...str } func newSyncWithFake(fn execFn, conNum int) *Sync { - sw := &syncWriter{w: io.Discard} return &Sync{ Command: "status", ConNum: conNum, Gitter: &fakeExecutor{fn: fn}, - Writer: printer.NewPrinter(sw, sw), + Writer: printer.NewPrinter(io.Discard, io.Discard), } } From 9f3561700ea53b41cffa6ce95cf8d2301da1cf46 Mon Sep 17 00:00:00 2001 From: takeshi yamashita <220619+takecy@users.noreply.github.com> Date: Sun, 26 Apr 2026 04:01:25 +0900 Subject: [PATCH 06/11] refactor(syncer): replace Gitter.IsExist method with ExistGit function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous Gitter.IsExist had two unrelated jobs glued together: report whether `git` is on PATH, and write that path to os.Stdout. The stdout write was an invisible side effect — production code never called IsExist, so the only consumer was a test, and even there the write only ever surfaced as noise next to the test summary. Promote the check to a package-level ExistGit() error and drop the side effect entirely. The receiver was empty anyway, so a method added no value. Update TestIsExist to call the new function. Co-Authored-By: Claude Opus 4.7 (1M context) --- syncer/git.go | 22 ++++++++-------------- syncer/git_test.go | 5 +---- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/syncer/git.go b/syncer/git.go index 1f88496..7c952e2 100644 --- a/syncer/git.go +++ b/syncer/git.go @@ -3,7 +3,6 @@ package syncer import ( "bytes" "context" - "fmt" "io" "os" "os/exec" @@ -15,6 +14,14 @@ type Executor interface { Git(ctx context.Context, command, dir string, args ...string) (msg, errMsg string, err error) } +// ExistGit reports whether the `git` executable is available on PATH. +// It returns the error from exec.LookPath when not found, and nil otherwise. +// Unlike the previous Gitter.IsExist, this function has no side effects. +func ExistGit() error { + _, err := exec.LookPath("git") + return err +} + // Gitter is struct type Gitter struct { writer io.Writer @@ -29,19 +36,6 @@ func NewGitter(writer, errWriter io.Writer) *Gitter { } } -// IsExist is check git command -func (*Gitter) IsExist() error { - s, err := exec.LookPath("git") - if err != nil { - return err - } - _, err = fmt.Fprintf(os.Stdout, "%s", s) - if err != nil { - return err - } - return nil -} - // Git is execute git command. The given context is forwarded to exec.CommandContext // so that cancellation (e.g. per-repo timeout) terminates the underlying process. func (g *Gitter) Git(ctx context.Context, command, dir string, args ...string) (msg, errMsg string, err error) { diff --git a/syncer/git_test.go b/syncer/git_test.go index 2d485e5..2c35da7 100644 --- a/syncer/git_test.go +++ b/syncer/git_test.go @@ -10,10 +10,7 @@ import ( func TestIsExist(t *testing.T) { is := is.New(t) - - gi := NewGitter(os.Stdout, os.Stderr) - err := gi.IsExist() - is.NoErr(err) + is.NoErr(ExistGit()) } func TestFetch(t *testing.T) { From fdd273e7cd89cf161cc1b9ff9032fc4643060fda Mon Sep 17 00:00:00 2001 From: takeshi yamashita <220619+takecy@users.noreply.github.com> Date: Sun, 26 Apr 2026 04:01:59 +0900 Subject: [PATCH 07/11] test(syncer): make TestFetch hermetic and assert ExistGit has no stdout output TestFetch used to run `git fetch --all -p` against ".", i.e. whatever directory the test happened to start in. On CI that meant fetching the git-here repo itself, which required network access and mutated the local clone's remote-tracking refs. Replace this with a fully self-contained fixture: a bare repo built in t.TempDir() acts as the remote, a sibling working repo pushes one empty commit to it, and the fetch under test pulls those refs back. The whole test runs in well under a second and never touches the network. After the fetch, resolve refs/remotes/origin/main; without this check the test would still pass on a no-op fetch because err==nil alone is not strong enough to detect "fetch succeeded but transferred nothing". Replace TestIsExist with TestExistGit_NoStdoutSideEffect, which redirects os.Stdout through an os.Pipe and asserts the captured output is empty. This guards the #11 contract ("ExistGit has no side effect") in a way that does not depend on which path resolves for git on the test host (Homebrew, Xcode, /usr/bin, etc.). Co-Authored-By: Claude Opus 4.7 (1M context) --- syncer/git_test.go | 62 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 59 insertions(+), 3 deletions(-) diff --git a/syncer/git_test.go b/syncer/git_test.go index 2c35da7..22e7bbd 100644 --- a/syncer/git_test.go +++ b/syncer/git_test.go @@ -2,24 +2,69 @@ package syncer import ( "context" + "io" "os" + "os/exec" + "strings" "testing" "github.com/matryer/is" ) -func TestIsExist(t *testing.T) { +func TestExistGit_NoStdoutSideEffect(t *testing.T) { + // Cannot use t.Parallel(): mutates global os.Stdout. is := is.New(t) - is.NoErr(ExistGit()) + + r, w, err := os.Pipe() + is.NoErr(err) + orig := os.Stdout + os.Stdout = w + defer func() { os.Stdout = orig }() + + err = ExistGit() + is.NoErr(err) + + if cerr := w.Close(); cerr != nil { + t.Fatalf("close pipe writer: %v", cerr) + } + out, rerr := io.ReadAll(r) + is.NoErr(rerr) + is.Equal(string(out), "") // ExistGit must not write anything to stdout } func TestFetch(t *testing.T) { is := is.New(t) + // bare repo serves as the "remote". Fetching from it never reaches the + // network, so this test is hermetic and CI-stable. + remote := t.TempDir() + runGit(t, remote, "init", "--bare", "-q") + + // working repo: register the bare repo as origin and push one commit so + // there is something for the fetch to discover. + work := t.TempDir() + runGit(t, work, "init", "-q") + runGit(t, work, "config", "user.email", "test@example.invalid") + runGit(t, work, "config", "user.name", "tester") + runGit(t, work, "config", "commit.gpgsign", "false") + runGit(t, work, "commit", "--allow-empty", "-m", "init", "-q") + runGit(t, work, "remote", "add", "origin", remote) + runGit(t, work, "push", "-q", "origin", "HEAD:refs/heads/main") + gi := NewGitter(os.Stdout, os.Stderr) args := []string{"--all", "-p"} - _, _, err := gi.Git(context.Background(), "fetch", ".", args...) + _, _, err := gi.Git(context.Background(), "fetch", work, args...) is.NoErr(err) + + // err==nil alone would let a no-op fetch pass. Verify the fetch actually + // populated the remote-tracking ref by resolving refs/remotes/origin/main. + cmd := exec.Command("git", "-C", work, "rev-parse", "refs/remotes/origin/main") + out, rpErr := cmd.CombinedOutput() + if rpErr != nil { + t.Fatalf("rev-parse refs/remotes/origin/main failed: %v\n%s", rpErr, out) + } + sha := strings.TrimSpace(string(out)) + is.True(sha != "") } func TestPull(t *testing.T) { @@ -31,3 +76,14 @@ func TestPull(t *testing.T) { _, _, err := gi.Git(context.Background(), "pull", ".", args...) is.NoErr(err) } + +// runGit is a fixture helper. Failures here mean the test setup is broken, +// not the system under test, so abort early with t.Fatalf rather than is.NoErr. +func runGit(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v in %s failed: %v\n%s", args, dir, err, out) + } +} From d67107acd687525a1b596f442fdd693487f82af9 Mon Sep 17 00:00:00 2001 From: takeshi yamashita <220619+takecy@users.noreply.github.com> Date: Sun, 26 Apr 2026 04:18:32 +0900 Subject: [PATCH 08/11] refactor(syncer): return RunSummary and error from Run instead of os.Exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sync.Run had three early os.Exit(1) calls for setup failures (ListDirs error, no git repositories present, invalid timeout duration). Calling os.Exit from inside Run skips deferred work and makes the function untestable from a test process — the public signature (err error) was already there, so the exits were also inconsistent with the declared contract. Change Run's signature to (*RunSummary, error). Setup failures now return an error; the new RunSummary type carries Succeeded / Failed / TimedOut counts plus a HasFailures predicate for callers that need to distinguish a clean run from a partial-failure run. Filter narrowing to zero matches returns an empty summary and nil error (it is not an error — there is just no work to do). Update gih/main.go to print the error to stderr and exit 1 in the err != nil case, replacing the earlier panic(err) which produced a stack trace and exit code 2 — the wrong code per the README spec. The summary value is currently ignored at the call site; the next commit wires it up to the exit-code 2 path for partial failures. Co-Authored-By: Claude Opus 4.7 (1M context) --- gih/main.go | 5 +++-- syncer/syncer.go | 51 +++++++++++++++++++++++++++++++++---------- syncer/syncer_test.go | 34 +++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 14 deletions(-) diff --git a/gih/main.go b/gih/main.go index 9649d00..5631cc3 100644 --- a/gih/main.go +++ b/gih/main.go @@ -75,7 +75,7 @@ func main() { writer := os.Stdout errWriter := os.Stderr - err := (&syncer.Sync{ + _, err := (&syncer.Sync{ TargetDir: *targetDir, IgnoreDir: *ignoreDir, TimeOut: *timeout, @@ -87,6 +87,7 @@ func main() { }).Run() if err != nil { - panic(err) + fmt.Fprintln(os.Stderr, err) + os.Exit(1) } } diff --git a/syncer/syncer.go b/syncer/syncer.go index 1c510c5..67a4679 100644 --- a/syncer/syncer.go +++ b/syncer/syncer.go @@ -3,7 +3,6 @@ package syncer import ( "context" "fmt" - "os" "path/filepath" "regexp" "sync" @@ -15,6 +14,20 @@ import ( "golang.org/x/sync/errgroup" ) +// RunSummary reports per-category counts after a Sync.Run completes +// successfully (i.e. the run itself was not aborted by a setup error). +type RunSummary struct { + Succeeded int + Failed int + TimedOut int +} + +// HasFailures reports whether any repository failed or timed out. +// Used by callers to decide between exit code 0 (clean) and 2 (partial). +func (s *RunSummary) HasFailures() bool { + return s.Failed > 0 || s.TimedOut > 0 +} + // Sync is struct type Sync struct { // TimeOut is timeout of performed command on one directory. @@ -75,16 +88,30 @@ func (s *runStats) addTimedOut(r string) { s.timedOut = append(s.timedOut, r) } -// Run is execute logic -func (s *Sync) Run() (err error) { +// summary builds a public RunSummary snapshot of the current stats. +func (s *runStats) summary() *RunSummary { + s.mu.Lock() + defer s.mu.Unlock() + return &RunSummary{ + Succeeded: len(s.succeeded), + Failed: len(s.failed), + TimedOut: len(s.timedOut), + } +} + +// Run discovers repositories, applies filters, and executes the configured +// git command across the matching set. It returns a summary of per-repo +// outcomes, or an error for setup failures (no repositories, invalid regex, +// invalid timeout, etc.). The returned summary is non-nil exactly when err +// is nil; callers can inspect summary.HasFailures() to decide between a +// clean run (exit 0) and a partial-failure run (exit 2). +func (s *Sync) Run() (*RunSummary, error) { dirs, err := ListDirs() if err != nil { - fmt.Fprintf(os.Stderr, "%v\n", err) - os.Exit(1) + return nil, errors.Wrap(err, "list directories") } if len(dirs) == 0 { - fmt.Fprintf(os.Stderr, "%s\n", "There is no repositories...") - os.Exit(1) + return nil, errors.New("no git repositories found in current directory") } fmt.Printf("repositories are found: (%d)\n", len(dirs)) @@ -92,23 +119,23 @@ func (s *Sync) Run() (err error) { repos, err := s.filterRepos(dirs) if err != nil { - return err + return nil, err } if len(repos) == 0 { + // Filter narrowed down to zero — no work, but not an error. s.Writer.PrintMsg("No target repositories.") - return + return &RunSummary{}, nil } s.Writer.PrintMsg(fmt.Sprintf("target repositories: (%d)", len(repos))) perRepoTimeout, err := time.ParseDuration(s.TimeOut) if err != nil { - fmt.Fprintf(os.Stderr, "%s\n", "invalid timeout value.") - os.Exit(1) + return nil, errors.Wrapf(err, "invalid timeout value: %s", s.TimeOut) } stats := s.execute(context.Background(), repos, perRepoTimeout) s.printSummary(stats) - return + return stats.summary(), nil } // filterRepos applies the target/ignore regex patterns to the discovered diff --git a/syncer/syncer_test.go b/syncer/syncer_test.go index 43a082b..c0f0a6e 100644 --- a/syncer/syncer_test.go +++ b/syncer/syncer_test.go @@ -187,3 +187,37 @@ func TestSync_FilterRepos(t *testing.T) { is.True(err != nil) }) } + +func TestRunSummary_HasFailures(t *testing.T) { + t.Run("clean run returns false", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + s := &RunSummary{Succeeded: 5, Failed: 0, TimedOut: 0} + is.Equal(s.HasFailures(), false) + }) + + t.Run("any failure returns true", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + s := &RunSummary{Succeeded: 4, Failed: 1, TimedOut: 0} + is.Equal(s.HasFailures(), true) + }) + + t.Run("any timeout returns true", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + s := &RunSummary{Succeeded: 4, Failed: 0, TimedOut: 1} + is.Equal(s.HasFailures(), true) + }) + + t.Run("empty summary returns false (no work done)", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + s := &RunSummary{} + is.Equal(s.HasFailures(), false) + }) +} From 77e85c41730307cf3c442096ca49cb47ae231d6b Mon Sep 17 00:00:00 2001 From: takeshi yamashita <220619+takecy@users.noreply.github.com> Date: Sun, 26 Apr 2026 04:18:51 +0900 Subject: [PATCH 09/11] feat(gih): exit with code 2 when any repository fails or times out The README has documented an exit-code-2 behaviour for partial repository failures since the project's first release, but the implementation has been silent: gih returned 0 whether all repos succeeded or only some did, so CI couldn't distinguish the two. Now that Sync.Run returns a RunSummary, inspect summary.HasFailures() after the err == nil path and exit 2 if any repository ended up in the Failed or TimedOut buckets. Pure setup failures continue to exit 1, and a clean run still exits 0. Co-Authored-By: Claude Opus 4.7 (1M context) --- gih/main.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/gih/main.go b/gih/main.go index 5631cc3..09e4126 100644 --- a/gih/main.go +++ b/gih/main.go @@ -75,7 +75,7 @@ func main() { writer := os.Stdout errWriter := os.Stderr - _, err := (&syncer.Sync{ + summary, err := (&syncer.Sync{ TargetDir: *targetDir, IgnoreDir: *ignoreDir, TimeOut: *timeout, @@ -86,8 +86,15 @@ func main() { Writer: printer.NewPrinter(writer, errWriter), }).Run() + // Map run outcome to exit codes per README spec: + // 0 - all operations completed successfully + // 1 - setup error (invalid args, no repos, invalid regex/timeout) + // 2 - some repositories failed or timed out if err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(1) } + if summary != nil && summary.HasFailures() { + os.Exit(2) + } } From f38ae879e8d0dee39b9b531617b3249668489dcc Mon Sep 17 00:00:00 2001 From: takeshi yamashita <220619+takecy@users.noreply.github.com> Date: Sun, 26 Apr 2026 04:35:55 +0900 Subject: [PATCH 10/11] refactor(syncer): detect repos with os.Stat to support git worktrees IsRepo previously called os.ReadDir(dirName) and walked every entry looking for a name match against ".git". That has two problems: it pays full directory-listing cost just to answer a yes/no question about a single fixed name, and the name-only match doesn't make the worktree case explicit even though git worktrees place ".git" as a *file* (a "gitdir: ..." pointer) rather than a directory. Replace the loop with a single os.Stat on the joined path. One syscall instead of N, and stat doesn't care whether the entry is a file or a directory, so a worktree's .git file is correctly accepted as a repository indicator. Add syncer/dir_test.go covering both layouts plus the negative cases (non-git directory, non-existent path), pinning the worktree-support contract so a future "must be a directory" change would surface as a test failure. Co-Authored-By: Claude Opus 4.7 (1M context) --- syncer/dir.go | 20 +++++---------- syncer/dir_test.go | 64 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 13 deletions(-) create mode 100644 syncer/dir_test.go diff --git a/syncer/dir.go b/syncer/dir.go index 1475dfc..c618031 100644 --- a/syncer/dir.go +++ b/syncer/dir.go @@ -2,6 +2,7 @@ package syncer import ( "os" + "path/filepath" "strings" ) @@ -22,18 +23,11 @@ func ListDirs() (dirs []string, err error) { return } -// IsRepo returns check result, the directory whether git repository +// IsRepo reports whether dirName contains a `.git` entry. It accepts both a +// regular repository (where `.git` is a directory) and a git worktree +// (where `.git` is a file pointing at the main repo's git directory), +// because exec'd git commands work in either layout. func IsRepo(dirName string) bool { - files, err := os.ReadDir(dirName) - if err != nil { - return false - } - - for _, f := range files { - if f.Name() == ".git" { - return true - } - } - - return false + _, err := os.Stat(filepath.Join(dirName, ".git")) + return err == nil } diff --git a/syncer/dir_test.go b/syncer/dir_test.go new file mode 100644 index 0000000..03c18b7 --- /dev/null +++ b/syncer/dir_test.go @@ -0,0 +1,64 @@ +package syncer + +import ( + "os" + "path/filepath" + "testing" + + "github.com/matryer/is" +) + +func TestIsRepo(t *testing.T) { + t.Run("regular repo with .git directory", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + dir := t.TempDir() + repo := filepath.Join(dir, "myrepo") + if err := os.MkdirAll(filepath.Join(repo, ".git"), 0o755); err != nil { + t.Fatalf("setup: %v", err) + } + + is.True(IsRepo(repo)) + }) + + t.Run("worktree with .git file", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + dir := t.TempDir() + worktree := filepath.Join(dir, "myworktree") + if err := os.MkdirAll(worktree, 0o755); err != nil { + t.Fatalf("setup: %v", err) + } + // git worktrees place a .git file (not directory) containing + // a `gitdir: ` pointer. The exact contents don't matter for + // IsRepo — only that the entry exists. + gitFile := filepath.Join(worktree, ".git") + if err := os.WriteFile(gitFile, []byte("gitdir: /tmp/main/.git/worktrees/x\n"), 0o644); err != nil { + t.Fatalf("setup: %v", err) + } + + is.True(IsRepo(worktree)) + }) + + t.Run("non-git directory returns false", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + dir := t.TempDir() + plain := filepath.Join(dir, "plain") + if err := os.MkdirAll(plain, 0o755); err != nil { + t.Fatalf("setup: %v", err) + } + + is.True(!IsRepo(plain)) + }) + + t.Run("non-existent directory returns false", func(t *testing.T) { + t.Parallel() + is := is.New(t) + + is.True(!IsRepo(filepath.Join(t.TempDir(), "does-not-exist"))) + }) +} From 298e76e13e453b17fcb5fb5cc49ac521546f8f8a Mon Sep 17 00:00:00 2001 From: takeshi yamashita <220619+takecy@users.noreply.github.com> Date: Sun, 26 Apr 2026 04:37:07 +0900 Subject: [PATCH 11/11] refactor(syncer): filter hidden directories before the IsRepo check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ListDirs used `strings.Index(f.Name(), ".") != 0` to drop hidden entries — a slightly indirect way of asking the same question that strings.HasPrefix asks more idiomatically. Switch to HasPrefix. While here, reorder the filter chain so the cheap HasPrefix check runs before IsRepo. IsRepo now performs an os.Stat per call, so short-circuiting on hidden directories avoids a syscall for each .something entry in the parent. Rewrite the conjunction as a sequence of early-continue guards so the intent of each step reads top-down. Pure refactor — for non-empty names, "name starts with ." and "strings.Index(name, \".\") == 0" are exactly the same predicate, so ListDirs returns the same set of paths it always did. Co-Authored-By: Claude Opus 4.7 (1M context) --- syncer/dir.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/syncer/dir.go b/syncer/dir.go index c618031..28df434 100644 --- a/syncer/dir.go +++ b/syncer/dir.go @@ -6,7 +6,10 @@ import ( "strings" ) -// ListDirs returns target directories +// ListDirs returns the names of direct child directories that look like git +// repositories, skipping hidden entries (those whose name starts with "."). +// The order matters for efficiency: HasPrefix is a cheap string check, so we +// filter out hidden directories before paying the os.Stat cost in IsRepo. func ListDirs() (dirs []string, err error) { files, err := os.ReadDir("./") if err != nil { @@ -15,11 +18,17 @@ func ListDirs() (dirs []string, err error) { dirs = make([]string, 0, len(files)) for _, f := range files { - if f.IsDir() && IsRepo(f.Name()) && strings.Index(f.Name(), ".") != 0 { - dirs = append(dirs, f.Name()) + if !f.IsDir() { + continue } + if strings.HasPrefix(f.Name(), ".") { + continue + } + if !IsRepo(f.Name()) { + continue + } + dirs = append(dirs, f.Name()) } - return }