From 71b6429d6054cd405f3da96c492fa2c63b92905c Mon Sep 17 00:00:00 2001 From: Mac Morgan Date: Tue, 21 Apr 2026 15:27:18 -0400 Subject: [PATCH 1/9] added default remote configuration --- internal/repo/config_test.go | 52 +++++++++++++++++++++++++++ internal/repo/init_test.go | 68 ++++++++++++++++++++++++++++++++++++ internal/repo/repo.go | 49 +++++++++++++++++++------- internal/repo/sync_test.go | 65 ++++++++++++++++++++++++++++++++++ 4 files changed, 221 insertions(+), 13 deletions(-) diff --git a/internal/repo/config_test.go b/internal/repo/config_test.go index b34c5179..7d25bdf7 100644 --- a/internal/repo/config_test.go +++ b/internal/repo/config_test.go @@ -152,6 +152,58 @@ func TestReadPrefixNoPrefixLine(t *testing.T) { } } +func TestRemoteNameDefault(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + if got := env.Repo.RemoteName(); got != "origin" { + t.Errorf("RemoteName() = %q, want %q", got, "origin") + } +} + +func TestRemoteNameFromBwConfig(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + if err := env.Repo.SetConfig("remote", "upstream"); err != nil { + t.Fatalf("SetConfig: %v", err) + } + env.CommitIntent("config remote=upstream") + + if got := env.Repo.RemoteName(); got != "upstream" { + t.Errorf("RemoteName() = %q, want %q", got, "upstream") + } +} + +func TestRemoteNameGitConfigWinsOverBwConfig(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + // .bwconfig says "upstream" + if err := env.Repo.SetConfig("remote", "upstream"); err != nil { + t.Fatalf("SetConfig: %v", err) + } + env.CommitIntent("config remote=upstream") + + // git config says "fork" — should win. + gitRun(t, env.Dir, "config", "beadwork.remote", "fork") + + if got := env.Repo.RemoteName(); got != "fork" { + t.Errorf("RemoteName() = %q, want %q (git config should win)", got, "fork") + } +} + +func TestRemoteNameGitConfigOnly(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + gitRun(t, env.Dir, "config", "beadwork.remote", "mirror") + + if got := env.Repo.RemoteName(); got != "mirror" { + t.Errorf("RemoteName() = %q, want %q", got, "mirror") + } +} + func init() { os.Setenv("GIT_AUTHOR_NAME", "Test") os.Setenv("GIT_AUTHOR_EMAIL", "test@test.com") diff --git a/internal/repo/init_test.go b/internal/repo/init_test.go index 9611897c..3411dbe5 100644 --- a/internal/repo/init_test.go +++ b/internal/repo/init_test.go @@ -204,6 +204,74 @@ func TestInitNoStatusGitkeeps(t *testing.T) { } } +// TestInitWithNonOriginRemoteViaGitConfig verifies that a fresh clone whose +// only remote is named "upstream" (origin renamed away) can still run +// `bw init` successfully when the caller has set `git config beadwork.remote +// upstream` beforehand. This is the bootstrap path: .bwconfig is not yet +// readable because the beadwork branch has not been fetched. +func TestInitWithNonOriginRemoteViaGitConfig(t *testing.T) { + // Set up a source repo with a beadwork branch, and a bare repo that + // has the beadwork branch pushed to it. + src := t.TempDir() + gitRun(t, src, "init") + gitRun(t, src, "config", "user.email", "test@test.com") + gitRun(t, src, "config", "user.name", "Test") + os.WriteFile(filepath.Join(src, "README"), []byte("test"), 0644) + gitRun(t, src, "add", ".") + gitRun(t, src, "commit", "-m", "initial") + + bare := filepath.Join(src, "bare.git") + gitRun(t, src, "init", "--bare", bare) + gitRun(t, src, "remote", "add", "origin", bare) + + orig, _ := os.Getwd() + os.Chdir(src) + srcRepo, err := repo.FindRepo() + if err != nil { + os.Chdir(orig) + t.Fatalf("FindRepo src: %v", err) + } + if err := srcRepo.Init("test"); err != nil { + os.Chdir(orig) + t.Fatalf("Init src: %v", err) + } + if _, _, err := srcRepo.Sync(); err != nil { + os.Chdir(orig) + t.Fatalf("Sync src: %v", err) + } + os.Chdir(orig) + + // Fresh clone, but rename origin to upstream so only "upstream" exists. + clone := t.TempDir() + cloneDir := filepath.Join(clone, "work") + gitRun(t, clone, "clone", bare, cloneDir) + gitRun(t, cloneDir, "config", "user.email", "clone@test.com") + gitRun(t, cloneDir, "config", "user.name", "Clone") + gitRun(t, cloneDir, "remote", "rename", "origin", "upstream") + + // Tell beadwork about the non-origin remote BEFORE init runs. + gitRun(t, cloneDir, "config", "beadwork.remote", "upstream") + + os.Chdir(cloneDir) + defer os.Chdir(orig) + + r, err := repo.FindRepo() + if err != nil { + t.Fatalf("FindRepo clone: %v", err) + } + if err := r.Init("test"); err != nil { + t.Fatalf("Init against upstream-only remote: %v", err) + } + + // The beadwork branch should now exist locally, populated from upstream. + if _, err := r.TreeFS().Stat("issues"); err != nil { + t.Errorf("issues dir not found after init: %v", err) + } + if r.RemoteName() != "upstream" { + t.Errorf("RemoteName() = %q, want 'upstream'", r.RemoteName()) + } +} + func TestValidatePrefix(t *testing.T) { tests := []struct { prefix string diff --git a/internal/repo/repo.go b/internal/repo/repo.go index 4c71ccc6..15e72edb 100644 --- a/internal/repo/repo.go +++ b/internal/repo/repo.go @@ -21,7 +21,6 @@ const BranchName = "beadwork" const CurrentVersion = 2 const refLocal = "refs/heads/" + BranchName -const refRemote = "refs/remotes/origin/" + BranchName type Repo struct { GitDir string @@ -170,14 +169,14 @@ func (r *Repo) Init(prefix string) error { if remoteExists { // Fetch remote branch - refSpec := config.RefSpec(fmt.Sprintf("+%s:%s", refLocal, refRemote)) - if err := r.fetch("origin", refSpec); err != nil { + refSpec := config.RefSpec(fmt.Sprintf("+%s:%s", refLocal, r.refRemote())) + if err := r.fetch(r.RemoteName(), refSpec); err != nil { return fmt.Errorf("fetch failed: %w", err) } if !localExists { // Create local branch from remote - remoteHash, err := r.tfs.LookupRef(refRemote) + remoteHash, err := r.tfs.LookupRef(r.refRemote()) if err != nil { return fmt.Errorf("lookup remote ref: %w", err) } @@ -239,7 +238,7 @@ func (r *Repo) Commit(message string) error { } func (r *Repo) remoteBranchExists() bool { - _, err := r.tfs.LookupRef(refRemote) + _, err := r.tfs.LookupRef(r.refRemote()) if err == nil { return true } @@ -250,7 +249,7 @@ func (r *Repo) remoteBranchExists() bool { return false } // Use git CLI for ls-remote since go-git remote.List requires network - out, err := execGit(r.RepoDir(), "ls-remote", "--heads", "origin", BranchName) + out, err := execGit(r.RepoDir(), "ls-remote", "--heads", r.RemoteName(), BranchName) if err != nil { return false } @@ -301,6 +300,30 @@ func (r *Repo) Version() int { return n } +// RemoteName returns the name of the git remote beadwork should fetch from +// and push to. Resolution order: +// 1. git config beadwork.remote (per-clone override; readable before a +// beadwork branch exists locally) +// 2. .bwconfig key "remote" (team-shared default, travels with the branch) +// 3. "origin" (built-in default, preserves pre-configurable behavior) +func (r *Repo) RemoteName() string { + if out, err := execGit(r.RepoDir(), "config", "--get", "beadwork.remote"); err == nil { + if name := strings.TrimSpace(out); name != "" { + return name + } + } + if v, ok := r.GetConfig("remote"); ok && v != "" { + return v + } + return "origin" +} + +// refRemote returns the local ref path for the fetched remote beadwork +// branch, e.g. "refs/remotes/upstream/beadwork". +func (r *Repo) refRemote() string { + return "refs/remotes/" + r.RemoteName() + "/" + BranchName +} + // GetConfig reads a single key from .bwconfig. func (r *Repo) GetConfig(key string) (string, bool) { cfg := r.ListConfig() @@ -350,16 +373,16 @@ func (r *Repo) readPrefix() string { return r.derivePrefix() } -// Sync fetches from origin, identifies local-only commits, and pushes or -// returns intents for replay. +// Sync fetches from the configured remote, identifies local-only commits, +// and pushes or returns intents for replay. func (r *Repo) Sync() (status string, replayed []string, err error) { if !r.hasRemote() { return "no remote configured", nil, nil } // Try to fetch - refSpec := config.RefSpec(fmt.Sprintf("+%s:%s", refLocal, refRemote)) - if fetchErr := r.fetch("origin", refSpec); fetchErr != nil { + refSpec := config.RefSpec(fmt.Sprintf("+%s:%s", refLocal, r.refRemote())) + if fetchErr := r.fetch(r.RemoteName(), refSpec); fetchErr != nil { // Remote branch may not exist — just push if err := r.push(); err != nil { return "", nil, fmt.Errorf("push failed: %w", err) @@ -367,7 +390,7 @@ func (r *Repo) Sync() (status string, replayed []string, err error) { return "pushed", nil, nil } - remoteHash, err := r.tfs.LookupRef(refRemote) + remoteHash, err := r.tfs.LookupRef(r.refRemote()) if err != nil { // No remote ref after fetch — just push if err := r.push(); err != nil { @@ -437,14 +460,14 @@ func (r *Repo) Sync() (status string, replayed []string, err error) { return "needs replay", localMsgs, nil } -// Push pushes the beadwork branch to origin. +// Push pushes the beadwork branch to the configured remote. func (r *Repo) Push() error { return r.push() } func (r *Repo) push() error { refSpec := config.RefSpec(refLocal + ":" + refLocal) - return r.gitPush("origin", refSpec) + return r.gitPush(r.RemoteName(), refSpec) } // RepoDir returns the repository root (the parent of the .git directory). diff --git a/internal/repo/sync_test.go b/internal/repo/sync_test.go index 53cc1856..087e4097 100644 --- a/internal/repo/sync_test.go +++ b/internal/repo/sync_test.go @@ -2,6 +2,7 @@ package repo_test import ( "os" + "os/exec" "testing" "github.com/jallum/beadwork/internal/intent" @@ -435,6 +436,70 @@ func TestPushNoRemote(t *testing.T) { } } +// TestSyncWithNonOriginRemoteViaBwConfig exercises the case where the only +// git remote is named "upstream" (no origin exists) and beadwork has been +// told to use it via .bwconfig. +func TestSyncWithNonOriginRemoteViaBwConfig(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + bare := env.Dir + "/remote.git" + gitRun(t, env.Dir, "init", "--bare", bare) + gitRun(t, env.Dir, "remote", "add", "upstream", bare) + + if err := env.Repo.SetConfig("remote", "upstream"); err != nil { + t.Fatalf("SetConfig: %v", err) + } + env.CommitIntent("config remote=upstream") + + env.Store.Create("Upstream issue", issue.CreateOpts{}) + env.CommitIntent("create upstream issue") + + status, _, err := env.Repo.Sync() + if err != nil { + t.Fatalf("Sync against upstream: %v", err) + } + if status != "pushed" { + t.Errorf("status = %q, want 'pushed'", status) + } + + // Confirm the remote actually received the beadwork branch. + out, err := exec.Command("git", "-C", bare, "rev-parse", "refs/heads/beadwork").CombinedOutput() + if err != nil { + t.Fatalf("remote beadwork ref missing: %s: %v", out, err) + } +} + +// TestSyncWithNonOriginRemoteViaGitConfig exercises the same case but with +// the remote name set through git config (beadwork.remote), which should +// take precedence over anything in .bwconfig. +func TestSyncWithNonOriginRemoteViaGitConfig(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + bare := env.Dir + "/remote.git" + gitRun(t, env.Dir, "init", "--bare", bare) + gitRun(t, env.Dir, "remote", "add", "upstream", bare) + + gitRun(t, env.Dir, "config", "beadwork.remote", "upstream") + + env.Store.Create("Upstream issue", issue.CreateOpts{}) + env.CommitIntent("create upstream issue") + + status, _, err := env.Repo.Sync() + if err != nil { + t.Fatalf("Sync against upstream: %v", err) + } + if status != "pushed" { + t.Errorf("status = %q, want 'pushed'", status) + } + + out, err := exec.Command("git", "-C", bare, "rev-parse", "refs/heads/beadwork").CombinedOutput() + if err != nil { + t.Fatalf("remote beadwork ref missing: %s: %v", out, err) + } +} + func init() { // Ensure we don't accidentally run tests against the real repo os.Setenv("GIT_AUTHOR_NAME", "Test") From c875e5921b72eab3c25d7762dabe0f3a2822c13e Mon Sep 17 00:00:00 2001 From: Mac Morgan Date: Wed, 22 Apr 2026 10:10:22 -0400 Subject: [PATCH 2/9] removed from beadwork config --- cmd/bw/config.go | 23 ++++++++++++++++++-- cmd/bw/config_test.go | 41 ++++++++++++++++++++++++++++++++++++ cmd/bw/parse_test.go | 17 +++++++++++++++ internal/repo/config_test.go | 38 +++++++++++++++++++++++++++++++++ internal/repo/repo.go | 21 ++++++++++++++++++ 5 files changed, 138 insertions(+), 2 deletions(-) diff --git a/cmd/bw/config.go b/cmd/bw/config.go index ce453c73..9bd533f9 100644 --- a/cmd/bw/config.go +++ b/cmd/bw/config.go @@ -18,7 +18,7 @@ type ConfigArgs struct { func parseConfigArgs(raw []string) (ConfigArgs, error) { if len(raw) == 0 { - return ConfigArgs{}, fmt.Errorf("usage: bw config get|set|list") + return ConfigArgs{}, fmt.Errorf("usage: bw config get|set|unset|list") } ca := ConfigArgs{Subcmd: raw[0]} switch ca.Subcmd { @@ -33,10 +33,15 @@ func parseConfigArgs(raw []string) (ConfigArgs, error) { } ca.Key = raw[1] ca.Value = raw[2] + case "unset": + if len(raw) < 2 { + return ca, fmt.Errorf("usage: bw config unset ") + } + ca.Key = raw[1] case "list": // no additional args default: - return ca, fmt.Errorf("usage: bw config get|set|list") + return ca, fmt.Errorf("usage: bw config get|set|unset|list") } return ca, nil } @@ -66,6 +71,20 @@ func cmdConfig(store *issue.Store, args []string, w Writer, _ *config.Config) (* } fmt.Fprintf(w, "%s=%s\n", ca.Key, ca.Value) + case "unset": + removed, err := r.UnsetConfig(ca.Key) + if err != nil { + return err + } + if !removed { + return fmt.Errorf("key not found: %s", ca.Key) + } + intent := fmt.Sprintf("config unset %s", ca.Key) + if err := r.Commit(intent); err != nil { + return fmt.Errorf("commit failed: %w", err) + } + fmt.Fprintf(w, "unset %s\n", ca.Key) + case "list": cfg := r.ListConfig() keys := make([]string, 0, len(cfg)) diff --git a/cmd/bw/config_test.go b/cmd/bw/config_test.go index c0fcc9ac..9d0f2f3b 100644 --- a/cmd/bw/config_test.go +++ b/cmd/bw/config_test.go @@ -55,3 +55,44 @@ func TestCmdConfigGetMissing(t *testing.T) { t.Error("expected error for missing key") } } + +func TestCmdConfigUnsetRemovesKey(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + var buf bytes.Buffer + if err := cmdConfig(env.Store, []string{"set", "remote", "upstream"}, PlainWriter(&buf)); err != nil { + t.Fatalf("config set: %v", err) + } + + buf.Reset() + if err := cmdConfig(env.Store, []string{"unset", "remote"}, PlainWriter(&buf)); err != nil { + t.Fatalf("config unset: %v", err) + } + if !strings.Contains(buf.String(), "unset remote") { + t.Errorf("unset output = %q", buf.String()) + } + + buf.Reset() + if err := cmdConfig(env.Store, []string{"list"}, PlainWriter(&buf)); err != nil { + t.Fatalf("config list: %v", err) + } + if strings.Contains(buf.String(), "remote=") { + t.Errorf("list still contains remote: %q", buf.String()) + } + // Other keys preserved. + if !strings.Contains(buf.String(), "prefix=test") { + t.Errorf("list missing prefix after unset: %q", buf.String()) + } +} + +func TestCmdConfigUnsetMissing(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + var buf bytes.Buffer + err := cmdConfig(env.Store, []string{"unset", "nonexistent"}, PlainWriter(&buf)) + if err == nil { + t.Error("expected error when unsetting a missing key") + } +} diff --git a/cmd/bw/parse_test.go b/cmd/bw/parse_test.go index b9667380..fe7a3be6 100644 --- a/cmd/bw/parse_test.go +++ b/cmd/bw/parse_test.go @@ -729,6 +729,23 @@ func TestParseConfigArgsSetNoValue(t *testing.T) { } } +func TestParseConfigArgsUnset(t *testing.T) { + a, err := parseConfigArgs([]string{"unset", "remote"}) + if err != nil { + t.Fatal(err) + } + if a.Subcmd != "unset" || a.Key != "remote" { + t.Errorf("got %+v, want {Subcmd:unset Key:remote}", a) + } +} + +func TestParseConfigArgsUnsetNoKey(t *testing.T) { + _, err := parseConfigArgs([]string{"unset"}) + if err == nil { + t.Error("expected error for unset without key") + } +} + // --- parseInitArgs --- func TestParseInitArgs(t *testing.T) { diff --git a/internal/repo/config_test.go b/internal/repo/config_test.go index 7d25bdf7..023544fe 100644 --- a/internal/repo/config_test.go +++ b/internal/repo/config_test.go @@ -152,6 +152,44 @@ func TestReadPrefixNoPrefixLine(t *testing.T) { } } +func TestUnsetConfigRemovesKey(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + if err := env.Repo.SetConfig("remote", "upstream"); err != nil { + t.Fatalf("SetConfig: %v", err) + } + + removed, err := env.Repo.UnsetConfig("remote") + if err != nil { + t.Fatalf("UnsetConfig: %v", err) + } + if !removed { + t.Error("UnsetConfig returned false for a key that was present") + } + + if _, ok := env.Repo.GetConfig("remote"); ok { + t.Error("remote should be gone after UnsetConfig") + } + // Sibling keys must remain. + if v, ok := env.Repo.GetConfig("prefix"); !ok || v != "test" { + t.Errorf("prefix = %q, ok=%v — want preserved after unset", v, ok) + } +} + +func TestUnsetConfigMissingKey(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + removed, err := env.Repo.UnsetConfig("nonexistent") + if err != nil { + t.Fatalf("UnsetConfig: %v", err) + } + if removed { + t.Error("UnsetConfig returned true for a key that was never set") + } +} + func TestRemoteNameDefault(t *testing.T) { env := testutil.NewEnv(t) defer env.Cleanup() diff --git a/internal/repo/repo.go b/internal/repo/repo.go index 15e72edb..e5d6c2b2 100644 --- a/internal/repo/repo.go +++ b/internal/repo/repo.go @@ -345,6 +345,27 @@ func (r *Repo) SetConfig(key, value string) error { return r.tfs.WriteFile(".bwconfig", []byte(data)) } +// UnsetConfig removes a key from .bwconfig. Returns true if the key was +// present and removed, false if it was not present. +func (r *Repo) UnsetConfig(key string) (bool, error) { + cfg := r.ListConfig() + if _, ok := cfg[key]; !ok { + return false, nil + } + delete(cfg, key) + + var lines []string + for k, v := range cfg { + lines = append(lines, k+"="+v) + } + sort.Strings(lines) + data := strings.Join(lines, "\n") + if len(lines) > 0 { + data += "\n" + } + return true, r.tfs.WriteFile(".bwconfig", []byte(data)) +} + // ListConfig reads all key=value pairs from .bwconfig. func (r *Repo) ListConfig() map[string]string { cfg := make(map[string]string) From f6fa7e8eaaa3aef57544c415f676fb0350992cd2 Mon Sep 17 00:00:00 2001 From: Mac Morgan Date: Wed, 22 Apr 2026 10:28:52 -0400 Subject: [PATCH 3/9] removed unset command --- cmd/bw/config.go | 23 ++------------------ cmd/bw/config_test.go | 41 ------------------------------------ cmd/bw/parse_test.go | 17 --------------- internal/repo/config_test.go | 38 --------------------------------- internal/repo/repo.go | 21 ------------------ 5 files changed, 2 insertions(+), 138 deletions(-) diff --git a/cmd/bw/config.go b/cmd/bw/config.go index 9bd533f9..ce453c73 100644 --- a/cmd/bw/config.go +++ b/cmd/bw/config.go @@ -18,7 +18,7 @@ type ConfigArgs struct { func parseConfigArgs(raw []string) (ConfigArgs, error) { if len(raw) == 0 { - return ConfigArgs{}, fmt.Errorf("usage: bw config get|set|unset|list") + return ConfigArgs{}, fmt.Errorf("usage: bw config get|set|list") } ca := ConfigArgs{Subcmd: raw[0]} switch ca.Subcmd { @@ -33,15 +33,10 @@ func parseConfigArgs(raw []string) (ConfigArgs, error) { } ca.Key = raw[1] ca.Value = raw[2] - case "unset": - if len(raw) < 2 { - return ca, fmt.Errorf("usage: bw config unset ") - } - ca.Key = raw[1] case "list": // no additional args default: - return ca, fmt.Errorf("usage: bw config get|set|unset|list") + return ca, fmt.Errorf("usage: bw config get|set|list") } return ca, nil } @@ -71,20 +66,6 @@ func cmdConfig(store *issue.Store, args []string, w Writer, _ *config.Config) (* } fmt.Fprintf(w, "%s=%s\n", ca.Key, ca.Value) - case "unset": - removed, err := r.UnsetConfig(ca.Key) - if err != nil { - return err - } - if !removed { - return fmt.Errorf("key not found: %s", ca.Key) - } - intent := fmt.Sprintf("config unset %s", ca.Key) - if err := r.Commit(intent); err != nil { - return fmt.Errorf("commit failed: %w", err) - } - fmt.Fprintf(w, "unset %s\n", ca.Key) - case "list": cfg := r.ListConfig() keys := make([]string, 0, len(cfg)) diff --git a/cmd/bw/config_test.go b/cmd/bw/config_test.go index 9d0f2f3b..c0fcc9ac 100644 --- a/cmd/bw/config_test.go +++ b/cmd/bw/config_test.go @@ -55,44 +55,3 @@ func TestCmdConfigGetMissing(t *testing.T) { t.Error("expected error for missing key") } } - -func TestCmdConfigUnsetRemovesKey(t *testing.T) { - env := testutil.NewEnv(t) - defer env.Cleanup() - - var buf bytes.Buffer - if err := cmdConfig(env.Store, []string{"set", "remote", "upstream"}, PlainWriter(&buf)); err != nil { - t.Fatalf("config set: %v", err) - } - - buf.Reset() - if err := cmdConfig(env.Store, []string{"unset", "remote"}, PlainWriter(&buf)); err != nil { - t.Fatalf("config unset: %v", err) - } - if !strings.Contains(buf.String(), "unset remote") { - t.Errorf("unset output = %q", buf.String()) - } - - buf.Reset() - if err := cmdConfig(env.Store, []string{"list"}, PlainWriter(&buf)); err != nil { - t.Fatalf("config list: %v", err) - } - if strings.Contains(buf.String(), "remote=") { - t.Errorf("list still contains remote: %q", buf.String()) - } - // Other keys preserved. - if !strings.Contains(buf.String(), "prefix=test") { - t.Errorf("list missing prefix after unset: %q", buf.String()) - } -} - -func TestCmdConfigUnsetMissing(t *testing.T) { - env := testutil.NewEnv(t) - defer env.Cleanup() - - var buf bytes.Buffer - err := cmdConfig(env.Store, []string{"unset", "nonexistent"}, PlainWriter(&buf)) - if err == nil { - t.Error("expected error when unsetting a missing key") - } -} diff --git a/cmd/bw/parse_test.go b/cmd/bw/parse_test.go index fe7a3be6..b9667380 100644 --- a/cmd/bw/parse_test.go +++ b/cmd/bw/parse_test.go @@ -729,23 +729,6 @@ func TestParseConfigArgsSetNoValue(t *testing.T) { } } -func TestParseConfigArgsUnset(t *testing.T) { - a, err := parseConfigArgs([]string{"unset", "remote"}) - if err != nil { - t.Fatal(err) - } - if a.Subcmd != "unset" || a.Key != "remote" { - t.Errorf("got %+v, want {Subcmd:unset Key:remote}", a) - } -} - -func TestParseConfigArgsUnsetNoKey(t *testing.T) { - _, err := parseConfigArgs([]string{"unset"}) - if err == nil { - t.Error("expected error for unset without key") - } -} - // --- parseInitArgs --- func TestParseInitArgs(t *testing.T) { diff --git a/internal/repo/config_test.go b/internal/repo/config_test.go index 023544fe..7d25bdf7 100644 --- a/internal/repo/config_test.go +++ b/internal/repo/config_test.go @@ -152,44 +152,6 @@ func TestReadPrefixNoPrefixLine(t *testing.T) { } } -func TestUnsetConfigRemovesKey(t *testing.T) { - env := testutil.NewEnv(t) - defer env.Cleanup() - - if err := env.Repo.SetConfig("remote", "upstream"); err != nil { - t.Fatalf("SetConfig: %v", err) - } - - removed, err := env.Repo.UnsetConfig("remote") - if err != nil { - t.Fatalf("UnsetConfig: %v", err) - } - if !removed { - t.Error("UnsetConfig returned false for a key that was present") - } - - if _, ok := env.Repo.GetConfig("remote"); ok { - t.Error("remote should be gone after UnsetConfig") - } - // Sibling keys must remain. - if v, ok := env.Repo.GetConfig("prefix"); !ok || v != "test" { - t.Errorf("prefix = %q, ok=%v — want preserved after unset", v, ok) - } -} - -func TestUnsetConfigMissingKey(t *testing.T) { - env := testutil.NewEnv(t) - defer env.Cleanup() - - removed, err := env.Repo.UnsetConfig("nonexistent") - if err != nil { - t.Fatalf("UnsetConfig: %v", err) - } - if removed { - t.Error("UnsetConfig returned true for a key that was never set") - } -} - func TestRemoteNameDefault(t *testing.T) { env := testutil.NewEnv(t) defer env.Cleanup() diff --git a/internal/repo/repo.go b/internal/repo/repo.go index e5d6c2b2..15e72edb 100644 --- a/internal/repo/repo.go +++ b/internal/repo/repo.go @@ -345,27 +345,6 @@ func (r *Repo) SetConfig(key, value string) error { return r.tfs.WriteFile(".bwconfig", []byte(data)) } -// UnsetConfig removes a key from .bwconfig. Returns true if the key was -// present and removed, false if it was not present. -func (r *Repo) UnsetConfig(key string) (bool, error) { - cfg := r.ListConfig() - if _, ok := cfg[key]; !ok { - return false, nil - } - delete(cfg, key) - - var lines []string - for k, v := range cfg { - lines = append(lines, k+"="+v) - } - sort.Strings(lines) - data := strings.Join(lines, "\n") - if len(lines) > 0 { - data += "\n" - } - return true, r.tfs.WriteFile(".bwconfig", []byte(data)) -} - // ListConfig reads all key=value pairs from .bwconfig. func (r *Repo) ListConfig() map[string]string { cfg := make(map[string]string) From c3199a75a224d448303e3004e0539a68a1675b10 Mon Sep 17 00:00:00 2001 From: Mac Morgan Date: Wed, 22 Apr 2026 11:14:31 -0400 Subject: [PATCH 4/9] Sync with every remote that has the beadwork branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bw sync now probes every git remote; any that has the beadwork branch gets fetched, merged, and pushed to. When no remote has it yet, a single remote is picked via: only-one-remote, then git config beadwork.remote, then a remote named origin, then an interactive prompt (TTY only, via term.IsTerminal — no /dev/tty fallback), with the selection persisted to git config so subsequent syncs don't re-prompt. Non-interactive runs fail with a message that names the remotes and the config fix. Drops the .bwconfig remote= fallback shipped with the configurable- default-remote feature; remote selection is now git-config-or-origin. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/bw/attach_test.go | 4 +- cmd/bw/sync.go | 87 ++++++- cmd/bw/sync_test.go | 234 +++++++++++++++++- internal/intent/replay_regression_test.go | 8 +- internal/repo/config_test.go | 32 --- internal/repo/init_test.go | 2 +- internal/repo/repo.go | 278 ++++++++++++++++------ internal/repo/sync_test.go | 270 +++++++++++++++++---- internal/treefs/treefs.go | 14 ++ 9 files changed, 760 insertions(+), 169 deletions(-) diff --git a/cmd/bw/attach_test.go b/cmd/bw/attach_test.go index 78b06b6e..1682cefb 100644 --- a/cmd/bw/attach_test.go +++ b/cmd/bw/attach_test.go @@ -130,7 +130,7 @@ func TestAttachSurvivesSyncConflictReplay(t *testing.T) { // Seed a shared issue and push so both sides agree on a base. shared, _ := env.Store.Create("Shared work", issue.CreateOpts{}) env.Repo.Commit(`create ` + shared.ID + ` p2 task "Shared work"`) - env.Repo.Sync() + env.Repo.Sync(nil) // Remote side: update the shared issue and push. env2 := env.CloneEnv(bare) @@ -140,7 +140,7 @@ func TestAttachSurvivesSyncConflictReplay(t *testing.T) { statusIP := "in_progress" env2.Store.Update(shared.ID, issue.UpdateOpts{Status: &statusIP}) env2.Repo.Commit("update " + shared.ID + " status=in_progress") - env2.Repo.Sync() + env2.Repo.Sync(nil) // Local side: same issue gets a different assignee, AND we attach // a file with a distinctive payload that must survive the replay. diff --git a/cmd/bw/sync.go b/cmd/bw/sync.go index 00947924..6fc21188 100644 --- a/cmd/bw/sync.go +++ b/cmd/bw/sync.go @@ -1,7 +1,13 @@ package main import ( + "bufio" "fmt" + "io" + "os" + "os/exec" + "strconv" + "strings" "github.com/jallum/beadwork/internal/config" @@ -9,13 +15,29 @@ import ( "github.com/jallum/beadwork/internal/intent" "github.com/jallum/beadwork/internal/issue" "github.com/jallum/beadwork/internal/repo" + "golang.org/x/term" ) +// syncStdin is the input source for the interactive remote-selection +// prompt. Tests override it via a strings.Reader the same way upgrade.go +// uses upgradeStdin. +var syncStdin io.Reader = os.Stdin + +// isInteractiveStdin decides whether the remote-selection prompt is +// allowed. It is a var (not a function) so tests can stub it without +// touching real stdin. When it returns false we must never prompt, even +// if a controlling terminal is reachable via /dev/tty. +var isInteractiveStdin = func() bool { + return term.IsTerminal(int(os.Stdin.Fd())) +} + func cmdSync(store *issue.Store, args []string, w Writer, _ *config.Config) (*config.Config, error) { _ = args r := store.Committer.(*repo.Repo) - status, intents, err := r.Sync() + resolver := makeRemoteResolver(r, w) + + status, intents, err := r.Sync(resolver) if err != nil { return nil, err } @@ -42,7 +64,7 @@ func cmdSync(store *issue.Store, args []string, w Writer, _ *config.Config) (*co } w.Pop() } - if err := r.Push(); err != nil { + if err := r.Push(resolver); err != nil { return nil, fmt.Errorf("push after replay failed: %w", err) } fmt.Fprintln(w, "replayed and pushed") @@ -51,3 +73,64 @@ func cmdSync(store *issue.Store, args []string, w Writer, _ *config.Config) (*co } return nil, nil } + +// makeRemoteResolver returns a RemoteResolver closed over the repo and +// the CLI writer. When stdin is interactive it prompts; otherwise it +// returns the same non-interactive error that a nil resolver would +// produce from the repo layer. +func makeRemoteResolver(r *repo.Repo, w Writer) repo.RemoteResolver { + return func(candidates []string) (string, error) { + if !isInteractiveStdin() { + return "", fmt.Errorf("no default remote — multiple remotes, none have the %q branch, no remote is named \"origin\", and git config beadwork.remote is unset. Set one with: git config beadwork.remote (remotes: %s)", + "beadwork", strings.Join(candidates, ", ")) + } + chosen, err := promptForRemote(candidates, w) + if err != nil { + return "", err + } + if err := gitConfigSet(r.RepoDir(), "beadwork.remote", chosen); err != nil { + return "", fmt.Errorf("set beadwork.remote: %w", err) + } + fmt.Fprintf(w, "saved: git config beadwork.remote=%s\n", chosen) + return chosen, nil + } +} + +// promptForRemote presents a numbered menu and reads a selection from +// syncStdin. Re-prompts up to 3 times on invalid input. +func promptForRemote(candidates []string, w Writer) (string, error) { + reader := bufio.NewReader(syncStdin) + for attempt := 0; attempt < 3; attempt++ { + fmt.Fprintln(w, "multiple remotes — pick one for bw to sync with:") + for i, name := range candidates { + fmt.Fprintf(w, " %d) %s\n", i+1, name) + } + fmt.Fprint(w, "select [1]: ") + + line, err := reader.ReadString('\n') + if err != nil && line == "" { + return "", fmt.Errorf("read input: %w", err) + } + line = strings.TrimSpace(line) + if line == "" { + line = "1" + } + n, convErr := strconv.Atoi(line) + if convErr != nil || n < 1 || n > len(candidates) { + fmt.Fprintf(w, "invalid selection %q; expected a number between 1 and %d\n", line, len(candidates)) + continue + } + return candidates[n-1], nil + } + return "", fmt.Errorf("too many invalid selections") +} + +func gitConfigSet(repoDir, key, value string) error { + cmd := exec.Command("git", "config", key, value) + cmd.Dir = repoDir + out, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("git config %s %s: %s: %w", key, value, strings.TrimSpace(string(out)), err) + } + return nil +} diff --git a/cmd/bw/sync_test.go b/cmd/bw/sync_test.go index 3a78d2b3..1925369a 100644 --- a/cmd/bw/sync_test.go +++ b/cmd/bw/sync_test.go @@ -2,6 +2,8 @@ package main import ( "bytes" + "os" + "os/exec" "strings" "testing" @@ -9,6 +11,29 @@ import ( "github.com/jallum/beadwork/internal/testutil" ) +// addBareRemote creates a bare repo at /.git and adds it +// as a remote named to the working repo. +func addBareRemote(t *testing.T, env *testutil.Env, name string) string { + t.Helper() + bare := env.Dir + "/" + name + ".git" + if out, err := exec.Command("git", "-C", env.Dir, "init", "--bare", bare).CombinedOutput(); err != nil { + t.Fatalf("init bare %s: %s: %v", name, out, err) + } + if out, err := exec.Command("git", "-C", env.Dir, "remote", "add", name, bare).CombinedOutput(); err != nil { + t.Fatalf("remote add %s: %s: %v", name, out, err) + } + return bare +} + +func getGitConfig(t *testing.T, dir, key string) string { + t.Helper() + out, err := exec.Command("git", "-C", dir, "config", "--get", key).CombinedOutput() + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) +} + func TestCmdSyncNoRemote(t *testing.T) { env := testutil.NewEnv(t) defer env.Cleanup() @@ -50,7 +75,7 @@ func TestCmdSyncUpToDate(t *testing.T) { env.Store.Create("Sync test", issue.CreateOpts{}) env.Repo.Commit("create issue") - env.Repo.Sync() // initial push + env.Repo.Sync(nil) // initial push var buf bytes.Buffer _, err := cmdSync(env.Store, []string{}, PlainWriter(&buf), nil) @@ -62,6 +87,209 @@ func TestCmdSyncUpToDate(t *testing.T) { } } +// TestIsInteractiveStdinPipe proves the TTY primitive returns false when +// stdin is a pipe. Guards against the underlying check being accidentally +// removed or inverted in future refactors. +func TestIsInteractiveStdinPipe(t *testing.T) { + rPipe, wPipe, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + defer rPipe.Close() + defer wPipe.Close() + + origStdin := os.Stdin + os.Stdin = rPipe + defer func() { os.Stdin = origStdin }() + + if isInteractiveStdin() { + t.Error("isInteractiveStdin() = true for pipe-backed stdin; want false") + } +} + +// TestCmdSyncNonInteractiveFailsWithoutPrompt verifies that when +// isInteractiveStdin reports false, the resolver never reads from +// syncStdin and never writes to git config, even if syncStdin has been +// primed with what would otherwise be a valid selection. +func TestCmdSyncNonInteractiveFailsWithoutPrompt(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + addBareRemote(t, env, "alpha") + addBareRemote(t, env, "beta") + + origInteractive := isInteractiveStdin + origStdin := syncStdin + defer func() { + isInteractiveStdin = origInteractive + syncStdin = origStdin + }() + isInteractiveStdin = func() bool { return false } + primed := strings.NewReader("1\n") + syncStdin = primed + + env.Store.Create("Needs resolution", issue.CreateOpts{}) + env.Repo.Commit("create needs resolution") + + var buf bytes.Buffer + err := cmdSync(env.Store, []string{}, PlainWriter(&buf)) + if err == nil { + t.Fatalf("expected error in non-interactive multi-remote run; output=%q", buf.String()) + } + if !strings.Contains(err.Error(), "no default remote") { + t.Errorf("error = %v, want a 'no default remote' message", err) + } + // syncStdin must remain untouched. + if primed.Len() == 0 { + t.Error("resolver consumed syncStdin input despite non-interactive stdin") + } + // git config must remain unset. + if v := getGitConfig(t, env.Dir, "beadwork.remote"); v != "" { + t.Errorf("git config beadwork.remote = %q, want unset", v) + } +} + +// TestCmdSyncInteractivePromptPersists verifies the happy interactive +// path: TTY stub true, valid numeric selection in syncStdin → prompt +// fires, selection is persisted to git config, a subsequent sync does +// not re-prompt (confirmed by no further input being consumed). +func TestCmdSyncInteractivePromptPersists(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + addBareRemote(t, env, "alpha") + addBareRemote(t, env, "beta") + + origInteractive := isInteractiveStdin + origStdin := syncStdin + defer func() { + isInteractiveStdin = origInteractive + syncStdin = origStdin + }() + isInteractiveStdin = func() bool { return true } + // Input for the first sync only — the second sync should use the + // persisted git config and never read from syncStdin again. + syncStdin = strings.NewReader("1\n") + + env.Store.Create("Interactive", issue.CreateOpts{}) + env.Repo.Commit("create interactive") + + var buf bytes.Buffer + if err := cmdSync(env.Store, []string{}, PlainWriter(&buf)); err != nil { + t.Fatalf("cmdSync: %v: %s", err, buf.String()) + } + if !strings.Contains(buf.String(), "pushed") { + t.Errorf("output = %q, want it to include 'pushed'", buf.String()) + } + // Resolver must have written the selection. + chosen := getGitConfig(t, env.Dir, "beadwork.remote") + if chosen != "alpha" { + t.Errorf("git config beadwork.remote = %q, want 'alpha'", chosen) + } + + // Second sync with empty syncStdin and an issue to push: must not + // need to prompt, because the config is now set. + syncStdin = strings.NewReader("") + env.Store.Create("Second", issue.CreateOpts{}) + env.Repo.Commit("create second") + + var buf2 bytes.Buffer + if err := cmdSync(env.Store, []string{}, PlainWriter(&buf2)); err != nil { + t.Fatalf("second cmdSync: %v: %s", err, buf2.String()) + } + if strings.Contains(buf2.String(), "multiple remotes") { + t.Errorf("second sync re-prompted: %q", buf2.String()) + } +} + +// TestCmdSyncGitConfigShortcutSkipsPrompt verifies that when +// git config beadwork.remote is already set, the resolver short-circuits +// before reaching the prompt — even with isInteractiveStdin stubbed to +// true and syncStdin primed with input that would otherwise be consumed. +func TestCmdSyncGitConfigShortcutSkipsPrompt(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + addBareRemote(t, env, "alpha") + addBareRemote(t, env, "beta") + if out, err := exec.Command("git", "-C", env.Dir, "config", "beadwork.remote", "beta").CombinedOutput(); err != nil { + t.Fatalf("git config: %s: %v", out, err) + } + + origInteractive := isInteractiveStdin + origStdin := syncStdin + defer func() { + isInteractiveStdin = origInteractive + syncStdin = origStdin + }() + isInteractiveStdin = func() bool { return true } + primed := strings.NewReader("1\n") + syncStdin = primed + + env.Store.Create("Config shortcut", issue.CreateOpts{}) + env.Repo.Commit("create config shortcut") + + var buf bytes.Buffer + if err := cmdSync(env.Store, []string{}, PlainWriter(&buf)); err != nil { + t.Fatalf("cmdSync: %v: %s", err, buf.String()) + } + if primed.Len() == 0 { + t.Error("resolver prompted despite git config beadwork.remote being set") + } + // beadwork branch should be on beta, not alpha. + if beadworkTipInDir(t, env.Dir+"/beta.git") == "" { + t.Error("expected beadwork on beta (the configured remote)") + } + if beadworkTipInDir(t, env.Dir+"/alpha.git") != "" { + t.Error("beadwork unexpectedly pushed to alpha") + } +} + +// TestCmdSyncOriginShortcutSkipsPrompt verifies the origin-by-name +// fallback short-circuits the prompt when no git config is set but a +// remote literally named "origin" exists. +func TestCmdSyncOriginShortcutSkipsPrompt(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + env.NewBareRemote() // adds "origin" + addBareRemote(t, env, "upstream") // second remote, without beadwork + + origInteractive := isInteractiveStdin + origStdin := syncStdin + defer func() { + isInteractiveStdin = origInteractive + syncStdin = origStdin + }() + isInteractiveStdin = func() bool { return true } + primed := strings.NewReader("2\n") + syncStdin = primed + + env.Store.Create("Origin shortcut", issue.CreateOpts{}) + env.Repo.Commit("create origin shortcut") + + var buf bytes.Buffer + if err := cmdSync(env.Store, []string{}, PlainWriter(&buf)); err != nil { + t.Fatalf("cmdSync: %v: %s", err, buf.String()) + } + if primed.Len() == 0 { + t.Error("resolver prompted despite an 'origin' remote being available") + } + if beadworkTipInDir(t, env.Dir+"/upstream.git") != "" { + t.Error("beadwork unexpectedly pushed to upstream; expected origin only") + } +} + +// beadworkTipInDir returns the beadwork tip hash in a bare repo, or "". +func beadworkTipInDir(t *testing.T, bare string) string { + t.Helper() + out, err := exec.Command("git", "-C", bare, "rev-parse", "refs/heads/beadwork").CombinedOutput() + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) +} + func TestCmdSyncReplay(t *testing.T) { env := testutil.NewEnv(t) defer env.Cleanup() @@ -71,7 +299,7 @@ func TestCmdSyncReplay(t *testing.T) { // Create a shared issue and push shared, _ := env.Store.Create("Shared", issue.CreateOpts{}) env.Repo.Commit("create " + shared.ID + " p3 task \"Shared\"") - env.Repo.Sync() + env.Repo.Sync(nil) // Clone and modify the same issue env2 := env.CloneEnv(bare) @@ -81,7 +309,7 @@ func TestCmdSyncReplay(t *testing.T) { statusIP := "in_progress" env2.Store.Update(shared.ID, issue.UpdateOpts{Status: &statusIP}) env2.Repo.Commit("update " + shared.ID + " status=in_progress") - env2.Repo.Sync() + env2.Repo.Sync(nil) // Back to original, modify the same issue (potential conflict) env.SwitchTo() diff --git a/internal/intent/replay_regression_test.go b/internal/intent/replay_regression_test.go index 73bd807a..0296e588 100644 --- a/internal/intent/replay_regression_test.go +++ b/internal/intent/replay_regression_test.go @@ -468,7 +468,7 @@ func TestSyncReplayPreservesStartDeferState(t *testing.T) { env.CommitIntent(`create ` + issA.ID + ` p2 task "Issue A"`) issB, _ := env.Store.Create("Issue B", issue.CreateOpts{}) env.CommitIntent(`create ` + issB.ID + ` p2 task "Issue B"`) - env.Repo.Sync() + env.Repo.Sync(nil) // Clone and make a conflicting change on the remote. env2 := env.CloneEnv(bare) @@ -478,7 +478,7 @@ func TestSyncReplayPreservesStartDeferState(t *testing.T) { p := 1 env2.Store.Update(issA.ID, issue.UpdateOpts{Priority: &p}) env2.CommitIntent(`update ` + issA.ID + ` priority=1`) - env2.Repo.Sync() + env2.Repo.Sync(nil) // On the original side: start issA, defer issB. env.SwitchTo() @@ -491,7 +491,7 @@ func TestSyncReplayPreservesStartDeferState(t *testing.T) { env.CommitIntent(`defer ` + issB.ID + ` until 2026-12-01`) // Sync should trigger conflict replay. - syncStatus, intents, err := env.Repo.Sync() + syncStatus, intents, err := env.Repo.Sync(nil) if err != nil { t.Fatalf("Sync: %v", err) } @@ -503,7 +503,7 @@ func TestSyncReplayPreservesStartDeferState(t *testing.T) { for _, e := range errs { t.Logf("replay error: %v", e) } - env.Repo.Push() + env.Repo.Push(nil) } // Verify start state was preserved. diff --git a/internal/repo/config_test.go b/internal/repo/config_test.go index 7d25bdf7..88bd9d1b 100644 --- a/internal/repo/config_test.go +++ b/internal/repo/config_test.go @@ -161,38 +161,6 @@ func TestRemoteNameDefault(t *testing.T) { } } -func TestRemoteNameFromBwConfig(t *testing.T) { - env := testutil.NewEnv(t) - defer env.Cleanup() - - if err := env.Repo.SetConfig("remote", "upstream"); err != nil { - t.Fatalf("SetConfig: %v", err) - } - env.CommitIntent("config remote=upstream") - - if got := env.Repo.RemoteName(); got != "upstream" { - t.Errorf("RemoteName() = %q, want %q", got, "upstream") - } -} - -func TestRemoteNameGitConfigWinsOverBwConfig(t *testing.T) { - env := testutil.NewEnv(t) - defer env.Cleanup() - - // .bwconfig says "upstream" - if err := env.Repo.SetConfig("remote", "upstream"); err != nil { - t.Fatalf("SetConfig: %v", err) - } - env.CommitIntent("config remote=upstream") - - // git config says "fork" — should win. - gitRun(t, env.Dir, "config", "beadwork.remote", "fork") - - if got := env.Repo.RemoteName(); got != "fork" { - t.Errorf("RemoteName() = %q, want %q (git config should win)", got, "fork") - } -} - func TestRemoteNameGitConfigOnly(t *testing.T) { env := testutil.NewEnv(t) defer env.Cleanup() diff --git a/internal/repo/init_test.go b/internal/repo/init_test.go index 3411dbe5..faf65609 100644 --- a/internal/repo/init_test.go +++ b/internal/repo/init_test.go @@ -235,7 +235,7 @@ func TestInitWithNonOriginRemoteViaGitConfig(t *testing.T) { os.Chdir(orig) t.Fatalf("Init src: %v", err) } - if _, _, err := srcRepo.Sync(); err != nil { + if _, _, err := srcRepo.Sync(nil); err != nil { os.Chdir(orig) t.Fatalf("Sync src: %v", err) } diff --git a/internal/repo/repo.go b/internal/repo/repo.go index 15e72edb..0320641a 100644 --- a/internal/repo/repo.go +++ b/internal/repo/repo.go @@ -300,21 +300,16 @@ func (r *Repo) Version() int { return n } -// RemoteName returns the name of the git remote beadwork should fetch from -// and push to. Resolution order: -// 1. git config beadwork.remote (per-clone override; readable before a -// beadwork branch exists locally) -// 2. .bwconfig key "remote" (team-shared default, travels with the branch) -// 3. "origin" (built-in default, preserves pre-configurable behavior) +// RemoteName returns the name of the git remote beadwork should use as a +// single-remote fallback (primarily for Init's bootstrap fetch). It reads +// git config beadwork.remote, defaulting to "origin" when unset. The +// richer multi-remote resolution for Sync/Push lives elsewhere. func (r *Repo) RemoteName() string { if out, err := execGit(r.RepoDir(), "config", "--get", "beadwork.remote"); err == nil { if name := strings.TrimSpace(out); name != "" { return name } } - if v, ok := r.GetConfig("remote"); ok && v != "" { - return v - } return "origin" } @@ -373,101 +368,238 @@ func (r *Repo) readPrefix() string { return r.derivePrefix() } -// Sync fetches from the configured remote, identifies local-only commits, -// and pushes or returns intents for replay. -func (r *Repo) Sync() (status string, replayed []string, err error) { - if !r.hasRemote() { - return "no remote configured", nil, nil - } +// RemoteResolver picks a single remote name from a list of candidate +// remotes. It is invoked by Sync/Push in the "no remote has beadwork yet" +// fallback when the deterministic rules (single remote, git config +// beadwork.remote, a remote named "origin") don't produce an answer. +// Passing nil to Sync/Push causes that fallback to return a non-interactive +// error instead of prompting. +type RemoteResolver func(candidates []string) (string, error) - // Try to fetch - refSpec := config.RefSpec(fmt.Sprintf("+%s:%s", refLocal, r.refRemote())) - if fetchErr := r.fetch(r.RemoteName(), refSpec); fetchErr != nil { - // Remote branch may not exist — just push - if err := r.push(); err != nil { - return "", nil, fmt.Errorf("push failed: %w", err) - } - return "pushed", nil, nil +// remoteHasBeadwork returns true if the given remote has refs/heads/beadwork. +// Uses `git ls-remote --heads beadwork` for a live probe. +func (r *Repo) remoteHasBeadwork(name string) bool { + out, err := execGit(r.RepoDir(), "ls-remote", "--heads", name, BranchName) + if err != nil { + return false } + return strings.TrimSpace(out) != "" +} - remoteHash, err := r.tfs.LookupRef(r.refRemote()) +// targetRemotes returns the list of remotes bw should act on. +// If any remote has the beadwork branch, only those are returned. Otherwise +// it resolves a single remote via resolveSingleRemote (short-circuit rules +// first, then the resolver callback). +func (r *Repo) targetRemotes(resolve RemoteResolver) ([]string, error) { + all, err := r.tfs.RemoteNames() if err != nil { - // No remote ref after fetch — just push - if err := r.push(); err != nil { - return "", nil, fmt.Errorf("push failed: %w", err) + return nil, err + } + if len(all) == 0 { + return nil, nil + } + var hasBW []string + for _, name := range all { + if r.remoteHasBeadwork(name) { + hasBW = append(hasBW, name) } - return "pushed", nil, nil } - - localHash := r.tfs.RefHash() - - // Check if we have local commits ahead of remote - localCommits, err := r.tfs.CommitsBetween(localHash, remoteHash) + if len(hasBW) > 0 { + sort.Strings(hasBW) + return hasBW, nil + } + chosen, err := r.resolveSingleRemote(all, resolve) if err != nil { - return "", nil, err + return nil, err } + return []string{chosen}, nil +} - if len(localCommits) == 0 { - // Nothing local — fast-forward to remote - if localHash != remoteHash { - if err := r.tfs.Reset(remoteHash); err != nil { - return "", nil, fmt.Errorf("fast-forward: %w", err) +// resolveSingleRemote applies the precedence rules for picking exactly one +// remote when no remote has the beadwork branch yet. +func (r *Repo) resolveSingleRemote(all []string, resolve RemoteResolver) (string, error) { + if len(all) == 1 { + return all[0], nil + } + if out, err := execGit(r.RepoDir(), "config", "--get", "beadwork.remote"); err == nil { + if cfg := strings.TrimSpace(out); cfg != "" { + for _, name := range all { + if name == cfg { + return cfg, nil + } } + return "", fmt.Errorf("git config beadwork.remote is set to %q but no remote by that name exists (remotes: %s)", cfg, strings.Join(all, ", ")) } - return "up to date", nil, nil } + for _, name := range all { + if name == "origin" { + return "origin", nil + } + } + if resolve != nil { + return resolve(all) + } + return "", fmt.Errorf("no default remote — multiple remotes, none have the %q branch, no remote is named \"origin\", and git config beadwork.remote is unset. Set one with: git config beadwork.remote (remotes: %s)", BranchName, strings.Join(all, ", ")) +} - // Check if remote has diverged from local - remoteCommits, err := r.tfs.CommitsBetween(remoteHash, localHash) +// Sync fetches from every remote that has the beadwork branch, merges +// their tips into local, and pushes the result back to every one of them. +// If no remote has the branch yet, it resolves a single remote via the +// precedence rules (single-remote auto-pick, git config beadwork.remote, +// "origin" by name, or the resolver callback for interactive selection) +// and pushes to just that one. +// +// On merge conflict against any remote, returns ("needs replay", +// conflicting-local-commits, nil) with preReplayHash captured. Callers run +// intent.Replay then re-invoke Sync to finish the fan-out. +func (r *Repo) Sync(resolve RemoteResolver) (status string, replayed []string, err error) { + if !r.hasRemote() { + return "no remote configured", nil, nil + } + + remotes, err := r.targetRemotes(resolve) if err != nil { return "", nil, err } + if len(remotes) == 0 { + return "no remote configured", nil, nil + } + + return r.syncTo(remotes) +} - if len(remoteCommits) == 0 { - // Remote hasn't diverged — local is strictly ahead, just push - if err := r.push(); err != nil { - return "", nil, fmt.Errorf("push failed: %w", err) +// syncTo runs the three-phase multi-remote sync across the given remotes: +// fetch each, merge each into local in sorted order (stopping on conflict), +// then push local to each. +func (r *Repo) syncTo(remotes []string) (string, []string, error) { + // Phase A: fetch every target remote into its own tracking ref. + // A failure on one remote (e.g. network, missing beadwork branch) + // is warned-and-skipped so the rest of the sync can proceed. + fetched := make([]string, 0, len(remotes)) + for _, name := range remotes { + refSpec := config.RefSpec(fmt.Sprintf("+%s:refs/remotes/%s/%s", refLocal, name, BranchName)) + if err := r.fetch(name, refSpec); err != nil { + fmt.Fprintf(os.Stderr, "warning: fetch from %s failed: %v\n", name, err) + continue } - return "pushed", nil, nil + fetched = append(fetched, name) } + sort.Strings(fetched) - // Diverged: try 3-way tree merge first - localMsgs := make([]string, 0, len(localCommits)) - for _, c := range localCommits { - localMsgs = append(localMsgs, c.Message) - } + // Phase B: merge each fetched remote's tip into local. + didMerge := false + didFastForward := false + for _, name := range fetched { + remoteRef := "refs/remotes/" + name + "/" + BranchName + remoteHash, err := r.tfs.LookupRef(remoteRef) + if err != nil { + // Tracking ref missing despite successful fetch — skip. + continue + } + localHash := r.tfs.RefHash() - merged, err := r.tfs.MergeCommit(localHash, remoteHash, localMsgs) - if err != nil { - return "", nil, fmt.Errorf("merge: %w", err) - } + localCommits, err := r.tfs.CommitsBetween(localHash, remoteHash) + if err != nil { + return "", nil, err + } + if len(localCommits) == 0 { + if localHash != remoteHash { + if err := r.tfs.Reset(remoteHash); err != nil { + return "", nil, fmt.Errorf("fast-forward from %s: %w", name, err) + } + didFastForward = true + } + continue + } - if merged { - if err := r.push(); err != nil { - return "", nil, fmt.Errorf("push after merge: %w", err) + remoteCommits, err := r.tfs.CommitsBetween(remoteHash, localHash) + if err != nil { + return "", nil, err } - return "rebased and pushed", nil, nil + if len(remoteCommits) == 0 { + // Local strictly ahead of this remote — Phase C will push. + continue + } + + // Diverged — attempt 3-way tree merge. + localMsgs := make([]string, 0, len(localCommits)) + for _, c := range localCommits { + localMsgs = append(localMsgs, c.Message) + } + merged, err := r.tfs.MergeCommit(localHash, remoteHash, localMsgs) + if err != nil { + return "", nil, fmt.Errorf("merge with %s: %w", name, err) + } + if merged { + didMerge = true + continue + } + + // Conflict: capture pre-reset hash for attachment-blob recovery + // (same as the single-remote path previously did), reset to this + // remote's tip, and bubble the conflicting intents out for replay. + r.preReplayHash = localHash + if err := r.tfs.Reset(remoteHash); err != nil { + return "", nil, fmt.Errorf("reset to %s: %w", name, err) + } + return "needs replay", localMsgs, nil + } + + // Phase C: push local to every target remote. Only counts as "pushed" + // when local is actually ahead of (or unrelated to) the remote's tip. + pushRefSpec := config.RefSpec(refLocal + ":" + refLocal) + pushedCount := 0 + for _, name := range remotes { + remoteRef := "refs/remotes/" + name + "/" + BranchName + localHash := r.tfs.RefHash() + remoteHash, refErr := r.tfs.LookupRef(remoteRef) + needsPush := refErr != nil || remoteHash != localHash + if !needsPush { + continue + } + if err := r.gitPush(name, pushRefSpec); err != nil { + return "", nil, fmt.Errorf("push to %s failed: %w", name, err) + } + pushedCount++ } - // Merge had conflicts — fall back to intent replay. - // Capture the pre-reset local hash so the caller can expose it to - // intent replay for attachment-blob recovery. Git keeps the blob - // objects in the ODB until gc, even after the ref is reset. - r.preReplayHash = localHash - if err := r.tfs.Reset(remoteHash); err != nil { - return "", nil, fmt.Errorf("reset to remote: %w", err) + switch { + case didMerge && pushedCount > 0: + return "rebased and pushed", nil, nil + case pushedCount > 0: + return "pushed", nil, nil + case didFastForward: + return "up to date", nil, nil + default: + return "up to date", nil, nil } - return "needs replay", localMsgs, nil } -// Push pushes the beadwork branch to the configured remote. -func (r *Repo) Push() error { - return r.push() +// Push pushes the beadwork branch to every remote that has the branch, or +// to a single resolved remote if none of them do. See Sync for the +// resolver semantics. +func (r *Repo) Push(resolve RemoteResolver) error { + if !r.hasRemote() { + return fmt.Errorf("no remote configured") + } + remotes, err := r.targetRemotes(resolve) + if err != nil { + return err + } + if len(remotes) == 0 { + return fmt.Errorf("no remote configured") + } + return r.pushTo(remotes) } -func (r *Repo) push() error { +func (r *Repo) pushTo(remotes []string) error { refSpec := config.RefSpec(refLocal + ":" + refLocal) - return r.gitPush(r.RemoteName(), refSpec) + for _, name := range remotes { + if err := r.gitPush(name, refSpec); err != nil { + return fmt.Errorf("push to %s failed: %w", name, err) + } + } + return nil } // RepoDir returns the repository root (the parent of the .git directory). diff --git a/internal/repo/sync_test.go b/internal/repo/sync_test.go index 087e4097..0752ce5e 100644 --- a/internal/repo/sync_test.go +++ b/internal/repo/sync_test.go @@ -3,6 +3,7 @@ package repo_test import ( "os" "os/exec" + "strings" "testing" "github.com/jallum/beadwork/internal/intent" @@ -16,7 +17,7 @@ func TestSyncNoRemote(t *testing.T) { env := testutil.NewEnv(t) defer env.Cleanup() - status, intents, err := env.Repo.Sync() + status, intents, err := env.Repo.Sync(nil) if err != nil { t.Fatalf("Sync: %v", err) } @@ -38,7 +39,7 @@ func TestSyncPushToEmpty(t *testing.T) { env.Store.Create("Test issue", issue.CreateOpts{}) env.CommitIntent("create test") - status, _, err := env.Repo.Sync() + status, _, err := env.Repo.Sync(nil) if err != nil { t.Fatalf("Sync: %v", err) } @@ -56,7 +57,7 @@ func TestSyncCleanRebase(t *testing.T) { // Push initial state env.Store.Create("Initial", issue.CreateOpts{}) env.CommitIntent("create initial") - env.Repo.Sync() + env.Repo.Sync(nil) // Clone and make a change on the remote side env2 := env.CloneEnv(bare) @@ -65,14 +66,14 @@ func TestSyncCleanRebase(t *testing.T) { env2.SwitchTo() iss2, _ := env2.Store.Create("Remote issue", issue.CreateOpts{Priority: intPtr(1)}) env2.CommitIntent("create " + iss2.ID) - env2.Repo.Sync() + env2.Repo.Sync(nil) // Back to original, make a non-conflicting change env.SwitchTo() iss1, _ := env.Store.Create("Local issue", issue.CreateOpts{Priority: intPtr(2)}) env.CommitIntent("create " + iss1.ID) - status, _, err := env.Repo.Sync() + status, _, err := env.Repo.Sync(nil) if err != nil { t.Fatalf("Sync: %v", err) } @@ -96,7 +97,7 @@ func TestSyncDirtyRebaseIntentReplay(t *testing.T) { // Create a shared issue and push shared, _ := env.Store.Create("Shared issue", issue.CreateOpts{}) env.CommitIntent("create " + shared.ID + " p3 task \"Shared issue\"") - env.Repo.Sync() + env.Repo.Sync(nil) // Clone and modify the shared issue env2 := env.CloneEnv(bare) @@ -106,7 +107,7 @@ func TestSyncDirtyRebaseIntentReplay(t *testing.T) { statusIP := "in_progress" env2.Store.Update(shared.ID, issue.UpdateOpts{Status: &statusIP}) env2.CommitIntent("update " + shared.ID + " status=in_progress") - env2.Repo.Sync() + env2.Repo.Sync(nil) // Back to original, also modify the same issue (will conflict) env.SwitchTo() @@ -114,7 +115,7 @@ func TestSyncDirtyRebaseIntentReplay(t *testing.T) { env.Store.Update(shared.ID, issue.UpdateOpts{Assignee: &assignee}) env.CommitIntent("update " + shared.ID + " assignee=local-agent") - status, intents, err := env.Repo.Sync() + status, intents, err := env.Repo.Sync(nil) if err != nil { t.Fatalf("Sync: %v", err) } @@ -130,7 +131,7 @@ func TestSyncDirtyRebaseIntentReplay(t *testing.T) { t.Logf("replay error: %v", e) } } - if err := env.Repo.Push(); err != nil { + if err := env.Repo.Push(nil); err != nil { t.Fatalf("Push after replay: %v", err) } @@ -163,7 +164,7 @@ func TestSyncMultipleIntentsReplay(t *testing.T) { bare := env.NewBareRemote() // Push initial state - env.Repo.Sync() + env.Repo.Sync(nil) // Clone and make a change env2 := env.CloneEnv(bare) @@ -172,7 +173,7 @@ func TestSyncMultipleIntentsReplay(t *testing.T) { env2.SwitchTo() blocker, _ := env2.Store.Create("Remote blocker", issue.CreateOpts{Priority: intPtr(1)}) env2.CommitIntent("create " + blocker.ID + " p1 task \"Remote blocker\"") - env2.Repo.Sync() + env2.Repo.Sync(nil) // Local side: create multiple issues, link them, label one env.SwitchTo() @@ -185,7 +186,7 @@ func TestSyncMultipleIntentsReplay(t *testing.T) { env.Store.Label(a.ID, []string{"urgent"}, nil) env.CommitIntent("label " + a.ID + " +urgent") - status, intents, err := env.Repo.Sync() + status, intents, err := env.Repo.Sync(nil) if err != nil { t.Fatalf("Sync: %v", err) } @@ -195,7 +196,7 @@ func TestSyncMultipleIntentsReplay(t *testing.T) { for _, e := range errs { t.Logf("replay error: %v", e) } - env.Repo.Push() + env.Repo.Push(nil) } // Verify everything landed: remote blocker + local A + local B + link + label @@ -222,10 +223,10 @@ func TestSyncUpToDate(t *testing.T) { env.Store.Create("Test", issue.CreateOpts{}) env.CommitIntent("create test") - env.Repo.Sync() // push + env.Repo.Sync(nil) // push // Sync again with no new changes - status, _, err := env.Repo.Sync() + status, _, err := env.Repo.Sync(nil) if err != nil { t.Fatalf("Sync: %v", err) } @@ -239,7 +240,7 @@ func TestSyncPicksUpRemoteChanges(t *testing.T) { defer env.Cleanup() bare := env.NewBareRemote() - env.Repo.Sync() // push initial + env.Repo.Sync(nil) // push initial // Clone, create issue, push env2 := env.CloneEnv(bare) @@ -248,11 +249,11 @@ func TestSyncPicksUpRemoteChanges(t *testing.T) { env2.SwitchTo() remote, _ := env2.Store.Create("From remote", issue.CreateOpts{}) env2.CommitIntent("create " + remote.ID) - env2.Repo.Sync() + env2.Repo.Sync(nil) // Original syncs — should pick up the remote issue env.SwitchTo() - env.Repo.Sync() + env.Repo.Sync(nil) got, err := env.Store.Get(remote.ID) if err != nil { @@ -299,13 +300,13 @@ func TestPush(t *testing.T) { // Push initial (to create remote branch) env.Store.Create("Initial", issue.CreateOpts{}) env.CommitIntent("create initial") - env.Repo.Sync() + env.Repo.Sync(nil) // Create another issue and push env.Store.Create("Push test", issue.CreateOpts{Priority: intPtr(1)}) env.CommitIntent("create push test") - if err := env.Repo.Push(); err != nil { + if err := env.Repo.Push(nil); err != nil { t.Fatalf("Push: %v", err) } @@ -325,7 +326,7 @@ func TestSyncFetchOnly(t *testing.T) { defer env.Cleanup() bare := env.NewBareRemote() - env.Repo.Sync() // push initial + env.Repo.Sync(nil) // push initial // Clone, create issue, push env2 := env.CloneEnv(bare) @@ -334,11 +335,11 @@ func TestSyncFetchOnly(t *testing.T) { env2.SwitchTo() remote, _ := env2.Store.Create("Remote only", issue.CreateOpts{Priority: intPtr(1)}) env2.CommitIntent("create " + remote.ID) - env2.Repo.Sync() + env2.Repo.Sync(nil) // Original: no local changes, should fast-forward env.SwitchTo() - status, _, err := env.Repo.Sync() + status, _, err := env.Repo.Sync(nil) if err != nil { t.Fatalf("Sync: %v", err) } @@ -376,13 +377,13 @@ func TestSyncLocalAheadNoDiverge(t *testing.T) { // Push initial env.Store.Create("Initial", issue.CreateOpts{}) env.CommitIntent("create initial") - env.Repo.Sync() + env.Repo.Sync(nil) // Create another issue locally (remote hasn't changed) env.Store.Create("Local only", issue.CreateOpts{Priority: intPtr(2)}) env.CommitIntent("create local only") - status, _, err := env.Repo.Sync() + status, _, err := env.Repo.Sync(nil) if err != nil { t.Fatalf("Sync: %v", err) } @@ -401,7 +402,7 @@ func TestSyncMultipleLocalCommits(t *testing.T) { // Push initial env.Store.Create("Initial", issue.CreateOpts{}) env.CommitIntent("create initial") - env.Repo.Sync() + env.Repo.Sync(nil) // Create several local issues for i := 0; i < 3; i++ { @@ -409,7 +410,7 @@ func TestSyncMultipleLocalCommits(t *testing.T) { env.CommitIntent("create " + iss.ID) } - status, _, err := env.Repo.Sync() + status, _, err := env.Repo.Sync(nil) if err != nil { t.Fatalf("Sync: %v", err) } @@ -430,16 +431,16 @@ func TestPushNoRemote(t *testing.T) { env.Store.Create("No remote", issue.CreateOpts{}) env.CommitIntent("create no remote") - err := env.Repo.Push() + err := env.Repo.Push(nil) if err == nil { t.Error("expected error when pushing with no remote") } } -// TestSyncWithNonOriginRemoteViaBwConfig exercises the case where the only -// git remote is named "upstream" (no origin exists) and beadwork has been -// told to use it via .bwconfig. -func TestSyncWithNonOriginRemoteViaBwConfig(t *testing.T) { +// TestSyncWithNonOriginRemoteViaGitConfig exercises the case where the +// only git remote is named "upstream" (no origin) and beadwork has been +// told to use it via git config beadwork.remote. +func TestSyncWithNonOriginRemoteViaGitConfig(t *testing.T) { env := testutil.NewEnv(t) defer env.Cleanup() @@ -447,15 +448,12 @@ func TestSyncWithNonOriginRemoteViaBwConfig(t *testing.T) { gitRun(t, env.Dir, "init", "--bare", bare) gitRun(t, env.Dir, "remote", "add", "upstream", bare) - if err := env.Repo.SetConfig("remote", "upstream"); err != nil { - t.Fatalf("SetConfig: %v", err) - } - env.CommitIntent("config remote=upstream") + gitRun(t, env.Dir, "config", "beadwork.remote", "upstream") env.Store.Create("Upstream issue", issue.CreateOpts{}) env.CommitIntent("create upstream issue") - status, _, err := env.Repo.Sync() + status, _, err := env.Repo.Sync(nil) if err != nil { t.Fatalf("Sync against upstream: %v", err) } @@ -463,40 +461,208 @@ func TestSyncWithNonOriginRemoteViaBwConfig(t *testing.T) { t.Errorf("status = %q, want 'pushed'", status) } - // Confirm the remote actually received the beadwork branch. out, err := exec.Command("git", "-C", bare, "rev-parse", "refs/heads/beadwork").CombinedOutput() if err != nil { t.Fatalf("remote beadwork ref missing: %s: %v", out, err) } } -// TestSyncWithNonOriginRemoteViaGitConfig exercises the same case but with -// the remote name set through git config (beadwork.remote), which should -// take precedence over anything in .bwconfig. -func TestSyncWithNonOriginRemoteViaGitConfig(t *testing.T) { +// beadworkTip reads the beadwork branch tip from a bare repo, returning +// an empty string if the branch doesn't exist. Used by multi-remote tests +// to assert which bare repos have the branch after sync. +func beadworkTip(t *testing.T, bare string) string { + t.Helper() + out, err := exec.Command("git", "-C", bare, "rev-parse", "refs/heads/beadwork").CombinedOutput() + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) +} + +// TestSyncMultiRemoteBothHaveBeadwork covers the core new behavior: when +// more than one remote has the beadwork branch, sync pushes new commits +// to every one of them. +func TestSyncMultiRemoteBothHaveBeadwork(t *testing.T) { env := testutil.NewEnv(t) defer env.Cleanup() - bare := env.Dir + "/remote.git" - gitRun(t, env.Dir, "init", "--bare", bare) - gitRun(t, env.Dir, "remote", "add", "upstream", bare) + bare1 := env.Dir + "/bare1.git" + bare2 := env.Dir + "/bare2.git" + gitRun(t, env.Dir, "init", "--bare", bare1) + gitRun(t, env.Dir, "init", "--bare", bare2) + gitRun(t, env.Dir, "remote", "add", "alpha", bare1) + gitRun(t, env.Dir, "remote", "add", "beta", bare2) - gitRun(t, env.Dir, "config", "beadwork.remote", "upstream") + // Initial push via git config beadwork.remote; only alpha has beadwork now. + gitRun(t, env.Dir, "config", "beadwork.remote", "alpha") + env.Store.Create("Seed", issue.CreateOpts{}) + env.CommitIntent("create seed") + if _, _, err := env.Repo.Sync(nil); err != nil { + t.Fatalf("initial sync: %v", err) + } + // Seed beta so both have beadwork. + gitRun(t, env.Dir, "push", "beta", "refs/heads/beadwork:refs/heads/beadwork") + seedTip := beadworkTip(t, bare1) + if seedTip == "" || seedTip != beadworkTip(t, bare2) { + t.Fatalf("pre-test seed failed: alpha=%q beta=%q", seedTip, beadworkTip(t, bare2)) + } - env.Store.Create("Upstream issue", issue.CreateOpts{}) - env.CommitIntent("create upstream issue") + // New commit locally, then sync — should push to both remotes. + env.Store.Create("Multi", issue.CreateOpts{}) + env.CommitIntent("create multi") - status, _, err := env.Repo.Sync() + status, _, err := env.Repo.Sync(nil) if err != nil { - t.Fatalf("Sync against upstream: %v", err) + t.Fatalf("multi-remote sync: %v", err) } if status != "pushed" { t.Errorf("status = %q, want 'pushed'", status) } - out, err := exec.Command("git", "-C", bare, "rev-parse", "refs/heads/beadwork").CombinedOutput() + tip1, tip2 := beadworkTip(t, bare1), beadworkTip(t, bare2) + if tip1 == seedTip { + t.Errorf("alpha tip did not advance: still %q", tip1) + } + if tip2 == seedTip { + t.Errorf("beta tip did not advance: still %q", tip2) + } + if tip1 != tip2 { + t.Errorf("tips diverged after sync: alpha=%q beta=%q", tip1, tip2) + } +} + +// TestSyncMultiRemoteOnlyOneHasBeadwork covers the mixed case: two +// remotes exist, one has beadwork and one does not. Sync must push only +// to the one that has it; the bare remote without beadwork stays empty. +func TestSyncMultiRemoteOnlyOneHasBeadwork(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + bare1 := env.Dir + "/bare1.git" + bare2 := env.Dir + "/bare2.git" + gitRun(t, env.Dir, "init", "--bare", bare1) + gitRun(t, env.Dir, "init", "--bare", bare2) + gitRun(t, env.Dir, "remote", "add", "alpha", bare1) + gitRun(t, env.Dir, "remote", "add", "beta", bare2) + + gitRun(t, env.Dir, "config", "beadwork.remote", "alpha") + env.Store.Create("Only alpha", issue.CreateOpts{}) + env.CommitIntent("create only alpha") + if _, _, err := env.Repo.Sync(nil); err != nil { + t.Fatalf("initial sync: %v", err) + } + if beadworkTip(t, bare1) == "" { + t.Fatal("alpha did not receive beadwork on initial sync") + } + if beadworkTip(t, bare2) != "" { + t.Fatal("beta unexpectedly has beadwork after initial sync") + } + + // Add another commit. alpha has beadwork, beta still doesn't — sync + // should push only to alpha. + env.Store.Create("Second", issue.CreateOpts{}) + env.CommitIntent("create second") + + if _, _, err := env.Repo.Sync(nil); err != nil { + t.Fatalf("second sync: %v", err) + } + if beadworkTip(t, bare2) != "" { + t.Errorf("beta received beadwork but it had none before sync") + } +} + +// TestSyncMultiRemoteConflictFansOutAfterReplay confirms that when a +// conflict on one remote triggers `needs replay`, replaying and +// re-invoking sync fans the merged state out to every target remote. +func TestSyncMultiRemoteConflictFansOutAfterReplay(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + bare1 := env.Dir + "/bare1.git" + bare2 := env.Dir + "/bare2.git" + gitRun(t, env.Dir, "init", "--bare", bare1) + gitRun(t, env.Dir, "init", "--bare", bare2) + gitRun(t, env.Dir, "remote", "add", "alpha", bare1) + gitRun(t, env.Dir, "remote", "add", "beta", bare2) + gitRun(t, env.Dir, "config", "beadwork.remote", "alpha") + + // Seed a shared issue on both remotes. + shared, _ := env.Store.Create("Shared", issue.CreateOpts{}) + env.CommitIntent("create " + shared.ID + " p3 task \"Shared\"") + if _, _, err := env.Repo.Sync(nil); err != nil { + t.Fatalf("seed sync: %v", err) + } + gitRun(t, env.Dir, "push", "beta", "refs/heads/beadwork:refs/heads/beadwork") + + // Clone the alpha bare, modify the shared issue, push back to alpha. + // This makes alpha diverge from local. + env2 := env.CloneEnv(bare1) + defer env2.Cleanup() + env2.SwitchTo() + statusIP := "in_progress" + env2.Store.Update(shared.ID, issue.UpdateOpts{Status: &statusIP}) + env2.CommitIntent("update " + shared.ID + " status=in_progress") + if _, _, err := env2.Repo.Sync(nil); err != nil { + t.Fatalf("clone sync: %v", err) + } + + // Back to original, make a conflicting edit locally. + env.SwitchTo() + assignee := "local-agent" + env.Store.Update(shared.ID, issue.UpdateOpts{Assignee: &assignee}) + env.CommitIntent("update " + shared.ID + " assignee=local-agent") + + // First sync: alpha diverged → needs replay (or clean rebase if git + // happens to auto-merge). Either way, after handling it the state + // must fan out to beta too. + status, intents, err := env.Repo.Sync(nil) if err != nil { - t.Fatalf("remote beadwork ref missing: %s: %v", out, err) + t.Fatalf("first sync: %v", err) + } + env.Store.ClearCache() + + if status == "needs replay" { + errs := intent.Replay(env.Store, intents) + for _, e := range errs { + t.Logf("replay error: %v", e) + } + if _, _, err := env.Repo.Sync(nil); err != nil { + t.Fatalf("post-replay sync: %v", err) + } + } + + // After everything settles, alpha and beta should agree. + if a, b := beadworkTip(t, bare1), beadworkTip(t, bare2); a != b || a == "" { + t.Errorf("remotes did not converge after replay: alpha=%q beta=%q", a, b) + } +} + +// TestSyncStaleBeadworkRemoteConfig ensures that a beadwork.remote value +// pointing at a non-existent remote produces a clear error, not a +// misleading git-level failure. +func TestSyncStaleBeadworkRemoteConfig(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + bare1 := env.Dir + "/bare1.git" + bare2 := env.Dir + "/bare2.git" + gitRun(t, env.Dir, "init", "--bare", bare1) + gitRun(t, env.Dir, "init", "--bare", bare2) + gitRun(t, env.Dir, "remote", "add", "alpha", bare1) + gitRun(t, env.Dir, "remote", "add", "beta", bare2) + + // Config names a remote that doesn't exist. + gitRun(t, env.Dir, "config", "beadwork.remote", "ghost") + + env.Store.Create("Stale", issue.CreateOpts{}) + env.CommitIntent("create stale") + + _, _, err := env.Repo.Sync(nil) + if err == nil { + t.Fatal("expected error for stale beadwork.remote") + } + if !strings.Contains(err.Error(), "ghost") { + t.Errorf("error should name the missing remote: %v", err) } } diff --git a/internal/treefs/treefs.go b/internal/treefs/treefs.go index ba12460f..fdbc0e73 100644 --- a/internal/treefs/treefs.go +++ b/internal/treefs/treefs.go @@ -826,6 +826,20 @@ func (t *TreeFS) HasRemotes() (bool, error) { return len(remotes) > 0, nil } +// RemoteNames returns the configured git remote names, sorted alphabetically. +func (t *TreeFS) RemoteNames() ([]string, error) { + remotes, err := t.repo.Remotes() + if err != nil { + return nil, err + } + names := make([]string, 0, len(remotes)) + for _, r := range remotes { + names = append(names, r.Config().Name) + } + sort.Strings(names) + return names, nil +} + // SetRef directly sets a reference. Used by Init to create tracking branches. func (t *TreeFS) SetRef(name string, hash plumbing.Hash) error { ref := plumbing.NewHashReference(plumbing.ReferenceName(name), hash) From fb030f7e0819fb0aeee724150838c8d7bdca6e34 Mon Sep 17 00:00:00 2001 From: Mac Morgan Date: Wed, 22 Apr 2026 11:40:49 -0400 Subject: [PATCH 5/9] Prompt for default remote during bw init MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `bw init` runs in a repo with 2+ remotes and none have the beadwork branch yet, init now asks the user which remote bw should sync with going forward and persists the choice to git config beadwork.remote. Short-circuits match sync: single-remote and existing git config / origin-by-name skip the prompt silently (no persist needed — sync re-applies the same rules). Unlike sync, init's prompt is not gated on term.IsTerminal. `bw init` is a human-initiated setup step; callers who really can't interact can set git config beadwork.remote beforehand and the resolver is never invoked. Init also now probes every remote for the beadwork branch rather than only the configured one, so fresh clones with a non-origin seed remote bootstrap correctly without needing git config. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/bw/init.go | 36 ++++++- cmd/bw/init_test.go | 198 ++++++++++++++++++++++++++++++++++ cmd/bw/sync.go | 10 +- internal/repo/hook_test.go | 6 +- internal/repo/init_test.go | 22 ++-- internal/repo/migrate_test.go | 8 +- internal/repo/repo.go | 135 +++++++++++++++++------ internal/repo/repo_test.go | 6 +- internal/repo/sync_test.go | 2 +- internal/testutil/testutil.go | 4 +- 10 files changed, 365 insertions(+), 62 deletions(-) diff --git a/cmd/bw/init.go b/cmd/bw/init.go index 6491b539..9b8446c3 100644 --- a/cmd/bw/init.go +++ b/cmd/bw/init.go @@ -2,13 +2,23 @@ package main import ( "fmt" + "io" + "os" "strings" "github.com/jallum/beadwork/internal/config" "github.com/jallum/beadwork/internal/issue" + "github.com/jallum/beadwork/internal/repo" ) +// initStdin is the input source for the init remote-selection prompt. +// Tests override it with a strings.Reader. Unlike sync's syncStdin, the +// init prompt is never gated on a TTY check: `bw init` is a +// user-initiated setup command, so we always prompt when there's an +// ambiguous seed-remote choice. +var initStdin io.Reader = os.Stdin + type InitArgs struct { Prefix string Force bool @@ -35,8 +45,9 @@ func cmdInit(_ *issue.Store, args []string, w Writer, _ *config.Config) (*config if err != nil { return nil, err } + resolver := makeInitRemoteResolver(r, w) if ia.Force { - if err := r.ForceReinit(ia.Prefix); err != nil { + if err := r.ForceReinit(ia.Prefix, resolver); err != nil { return nil, err } fmt.Fprintln(w, initMessage("reinitialized", r.Prefix)) @@ -45,13 +56,34 @@ func cmdInit(_ *issue.Store, args []string, w Writer, _ *config.Config) (*config if r.IsInitialized() { return nil, fmt.Errorf("beadwork already initialized") } - if err := r.Init(ia.Prefix); err != nil { + if err := r.Init(ia.Prefix, resolver); err != nil { return nil, err } fmt.Fprintln(w, initMessage("initialized", r.Prefix)) return nil, nil } +// makeInitRemoteResolver returns a RemoteResolver that unconditionally +// prompts (no TTY check) and persists the choice via git config. Init +// is a human-driven setup step, so there's no non-interactive path to +// guard against — if someone really can't interact, they can set +// git config beadwork.remote before running bw init and the +// resolver will never be called. +func makeInitRemoteResolver(r *repo.Repo, w Writer) repo.RemoteResolver { + return func(candidates []string) (string, error) { + chosen, err := promptForRemoteWithReader(candidates, w, initStdin) + if err != nil { + return "", err + } + if err := gitConfigSet(r.RepoDir(), "beadwork.remote", chosen); err != nil { + return "", fmt.Errorf("set beadwork.remote: %w", err) + } + fmt.Fprintf(w, "saved: git config beadwork.remote=%s\n", chosen) + return chosen, nil + } +} + + func initMessage(verb, prefix string) string { var b strings.Builder fmt.Fprintf(&b, "%s beadwork (prefix: %s)\n", verb, prefix) diff --git a/cmd/bw/init_test.go b/cmd/bw/init_test.go index fa6bfd7d..96a1fecb 100644 --- a/cmd/bw/init_test.go +++ b/cmd/bw/init_test.go @@ -144,6 +144,204 @@ func TestCmdInitForceDefaultPrefix(t *testing.T) { } } +// setupFreshRepoWithRemotes creates a temp dir, inits a git repo with +// one commit, adds the given named bare remotes, and chdirs into the +// repo. Returns the repo dir and a cleanup function to restore cwd. +// Used by the multi-remote init tests below. +func setupFreshRepoWithRemotes(t *testing.T, remotes ...string) (string, func()) { + t.Helper() + dir := t.TempDir() + run := func(args ...string) { + t.Helper() + cmd := exec.Command(args[0], args[1:]...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("%v: %s: %v", args, out, err) + } + } + run("git", "init") + run("git", "config", "user.email", "test@test.com") + run("git", "config", "user.name", "Test") + os.WriteFile(dir+"/README", []byte("test"), 0644) + run("git", "add", ".") + run("git", "commit", "-m", "initial") + for _, name := range remotes { + bare := dir + "/" + name + ".git" + run("git", "init", "--bare", bare) + run("git", "remote", "add", name, bare) + } + orig, _ := os.Getwd() + os.Chdir(dir) + return dir, func() { os.Chdir(orig) } +} + +// TestCmdInitMultiRemotePrompts exercises the new init prompt: multiple +// remotes exist, none have the beadwork branch yet, no origin, no git +// config — init must ask the user which remote to seed and persist the +// choice. Unlike sync, init never consults isInteractiveStdin. +func TestCmdInitMultiRemotePrompts(t *testing.T) { + dir, cleanup := setupFreshRepoWithRemotes(t, "alpha", "beta") + defer cleanup() + + origStdin := initStdin + defer func() { initStdin = origStdin }() + initStdin = strings.NewReader("2\n") // pick beta + + var buf bytes.Buffer + if err := cmdInit(nil, []string{"--prefix", "multi"}, PlainWriter(&buf)); err != nil { + t.Fatalf("cmdInit: %v: %s", err, buf.String()) + } + if !strings.Contains(buf.String(), "initialized") { + t.Errorf("output missing 'initialized': %q", buf.String()) + } + + chosen := getGitConfig(t, dir, "beadwork.remote") + if chosen != "beta" { + t.Errorf("git config beadwork.remote = %q, want 'beta'", chosen) + } +} + +// TestCmdInitMultiRemoteIgnoresTTYStub confirms the init prompt fires +// even when isInteractiveStdin (sync's gate) is stubbed to return false. +// Init is human-initiated; the TTY check is sync's concern, not init's. +func TestCmdInitMultiRemoteIgnoresTTYStub(t *testing.T) { + _, cleanup := setupFreshRepoWithRemotes(t, "alpha", "beta") + defer cleanup() + + origStdin := initStdin + origInteractive := isInteractiveStdin + defer func() { + initStdin = origStdin + isInteractiveStdin = origInteractive + }() + initStdin = strings.NewReader("1\n") + isInteractiveStdin = func() bool { return false } + + var buf bytes.Buffer + if err := cmdInit(nil, []string{"--prefix", "alpha"}, PlainWriter(&buf)); err != nil { + t.Fatalf("cmdInit: %v: %s", err, buf.String()) + } + if !strings.Contains(buf.String(), "initialized") { + t.Errorf("output missing 'initialized': %q", buf.String()) + } +} + +// TestCmdInitMultiRemoteOriginShortcut verifies origin is auto-selected +// without prompting, even when initStdin has primed input. +func TestCmdInitMultiRemoteOriginShortcut(t *testing.T) { + dir, cleanup := setupFreshRepoWithRemotes(t, "origin", "upstream") + defer cleanup() + + origStdin := initStdin + defer func() { initStdin = origStdin }() + primed := strings.NewReader("2\n") + initStdin = primed + + var buf bytes.Buffer + if err := cmdInit(nil, []string{"--prefix", "o"}, PlainWriter(&buf)); err != nil { + t.Fatalf("cmdInit: %v: %s", err, buf.String()) + } + if primed.Len() == 0 { + t.Error("init prompted despite origin being present") + } + // Should not have persisted a choice either, since the origin rule + // triggers inside resolveSingleRemote without calling the resolver. + if v := getGitConfig(t, dir, "beadwork.remote"); v != "" { + t.Errorf("git config beadwork.remote = %q, want unset (origin picked silently)", v) + } +} + +// TestCmdInitMultiRemoteGitConfigShortcut verifies a pre-existing +// beadwork.remote git config short-circuits the prompt during init. +func TestCmdInitMultiRemoteGitConfigShortcut(t *testing.T) { + dir, cleanup := setupFreshRepoWithRemotes(t, "alpha", "beta") + defer cleanup() + + if out, err := exec.Command("git", "-C", dir, "config", "beadwork.remote", "beta").CombinedOutput(); err != nil { + t.Fatalf("git config: %s: %v", out, err) + } + + origStdin := initStdin + defer func() { initStdin = origStdin }() + primed := strings.NewReader("1\n") + initStdin = primed + + var buf bytes.Buffer + if err := cmdInit(nil, []string{"--prefix", "g"}, PlainWriter(&buf)); err != nil { + t.Fatalf("cmdInit: %v: %s", err, buf.String()) + } + if primed.Len() == 0 { + t.Error("init prompted despite git config beadwork.remote being set") + } +} + +// TestCmdInitBootstrapsFromRemoteWithBeadwork confirms that if some +// remote already has the beadwork branch, init silently fetches from it +// (no prompt, regardless of initStdin). +func TestCmdInitBootstrapsFromRemoteWithBeadwork(t *testing.T) { + // Seed a bare repo by doing a full init-and-push in a source repo. + srcDir, cleanupSrc := setupFreshRepoWithRemotes(t, "alpha") + var buf bytes.Buffer + if err := cmdInit(nil, []string{"--prefix", "seed"}, PlainWriter(&buf)); err != nil { + cleanupSrc() + t.Fatalf("seed cmdInit: %v: %s", err, buf.String()) + } + // Push beadwork to alpha so that bare has the branch. + alphaBare := srcDir + "/alpha.git" + if out, err := exec.Command("git", "-C", srcDir, "push", "alpha", "refs/heads/beadwork:refs/heads/beadwork").CombinedOutput(); err != nil { + cleanupSrc() + t.Fatalf("seed push: %s: %v", out, err) + } + cleanupSrc() + + // Fresh repo, add the same bare (with beadwork) as "beta" alongside + // a second empty bare "alpha2". Init should auto-pick beta (the one + // with beadwork) with no prompt, no persistence. + dir := t.TempDir() + run := func(args ...string) { + t.Helper() + cmd := exec.Command(args[0], args[1:]...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("%v: %s: %v", args, out, err) + } + } + run("git", "init") + run("git", "config", "user.email", "clone@test.com") + run("git", "config", "user.name", "Clone") + os.WriteFile(dir+"/README", []byte("x"), 0644) + run("git", "add", ".") + run("git", "commit", "-m", "initial") + run("git", "init", "--bare", dir+"/alpha2.git") + run("git", "remote", "add", "alpha2", dir+"/alpha2.git") + run("git", "remote", "add", "beta", alphaBare) + + orig, _ := os.Getwd() + os.Chdir(dir) + defer os.Chdir(orig) + + origStdin := initStdin + defer func() { initStdin = origStdin }() + primed := strings.NewReader("1\n") + initStdin = primed + + var buf2 bytes.Buffer + if err := cmdInit(nil, []string{}, PlainWriter(&buf2)); err != nil { + t.Fatalf("cmdInit: %v: %s", err, buf2.String()) + } + if primed.Len() == 0 { + t.Error("init prompted despite beta already having beadwork") + } + if !strings.Contains(buf2.String(), "initialized") { + t.Errorf("output = %q", buf2.String()) + } + // Prefix came from the seed (the beadwork branch carries .bwconfig + // with prefix=seed), not the caller. + if !strings.Contains(buf2.String(), "seed") { + t.Errorf("expected prefix 'seed' (bootstrapped from remote); output=%q", buf2.String()) + } +} + func TestCmdInitInvalidPrefix(t *testing.T) { dir := t.TempDir() runInDir := func(args ...string) { diff --git a/cmd/bw/sync.go b/cmd/bw/sync.go index 6fc21188..1c39b6c6 100644 --- a/cmd/bw/sync.go +++ b/cmd/bw/sync.go @@ -99,7 +99,15 @@ func makeRemoteResolver(r *repo.Repo, w Writer) repo.RemoteResolver { // promptForRemote presents a numbered menu and reads a selection from // syncStdin. Re-prompts up to 3 times on invalid input. func promptForRemote(candidates []string, w Writer) (string, error) { - reader := bufio.NewReader(syncStdin) + return promptForRemoteWithReader(candidates, w, syncStdin) +} + +// promptForRemoteWithReader is the shared implementation behind both the +// sync and init prompts. The caller supplies the input source so init +// can use initStdin and sync can use syncStdin without either depending +// on the other's package-level variable. +func promptForRemoteWithReader(candidates []string, w Writer, source io.Reader) (string, error) { + reader := bufio.NewReader(source) for attempt := 0; attempt < 3; attempt++ { fmt.Fprintln(w, "multiple remotes — pick one for bw to sync with:") for i, name := range candidates { diff --git a/internal/repo/hook_test.go b/internal/repo/hook_test.go index 4fffc5c9..51915582 100644 --- a/internal/repo/hook_test.go +++ b/internal/repo/hook_test.go @@ -43,7 +43,7 @@ func TestInitWithPreCommitHook(t *testing.T) { if err != nil { t.Fatalf("FindRepo: %v", err) } - if err := r.Init("test"); err != nil { + if err := r.Init("test", nil); err != nil { t.Fatalf("Init should succeed despite pre-commit hook: %v", err) } } @@ -66,7 +66,7 @@ func TestCommitWithPreCommitHook(t *testing.T) { if err != nil { t.Fatalf("FindRepo: %v", err) } - if err := r.Init("test"); err != nil { + if err := r.Init("test", nil); err != nil { t.Fatalf("Init: %v", err) } @@ -101,7 +101,7 @@ func TestCommitNoop(t *testing.T) { if err != nil { t.Fatalf("FindRepo: %v", err) } - if err := r.Init("test"); err != nil { + if err := r.Init("test", nil); err != nil { t.Fatalf("Init: %v", err) } diff --git a/internal/repo/init_test.go b/internal/repo/init_test.go index faf65609..3bb38e11 100644 --- a/internal/repo/init_test.go +++ b/internal/repo/init_test.go @@ -28,7 +28,7 @@ func TestForceReinit(t *testing.T) { } // First init - if err := r.Init("old"); err != nil { + if err := r.Init("old", nil); err != nil { t.Fatalf("Init: %v", err) } if r.Prefix != "old" { @@ -37,13 +37,13 @@ func TestForceReinit(t *testing.T) { // Regular init should fail r2, _ := repo.FindRepo() - if err := r2.Init("new"); err == nil { + if err := r2.Init("new", nil); err == nil { t.Fatal("Init should fail when already initialized") } // Force reinit with new prefix r3, _ := repo.FindRepo() - if err := r3.ForceReinit("new"); err != nil { + if err := r3.ForceReinit("new", nil); err != nil { t.Fatalf("ForceReinit: %v", err) } if r3.Prefix != "new" { @@ -75,11 +75,11 @@ func TestForceReinitKeepsPrefix(t *testing.T) { defer os.Chdir(orig) r, _ := repo.FindRepo() - r.Init("keep") + r.Init("keep", nil) // Force reinit with empty prefix should derive a new one (not keep old) r2, _ := repo.FindRepo() - if err := r2.ForceReinit(""); err != nil { + if err := r2.ForceReinit("", nil); err != nil { t.Fatalf("ForceReinit: %v", err) } // Should derive prefix from dir name, not be empty @@ -106,7 +106,7 @@ func TestDerivePrefixLength(t *testing.T) { defer os.Chdir(orig) r, _ := repo.FindRepo() - if err := r.Init(""); err != nil { + if err := r.Init("", nil); err != nil { t.Fatalf("Init: %v", err) } // Should truncate to 8 chars max @@ -136,7 +136,7 @@ func TestDerivePrefixPreservesCase(t *testing.T) { defer os.Chdir(orig) r, _ := repo.FindRepo() - if err := r.Init(""); err != nil { + if err := r.Init("", nil); err != nil { t.Fatalf("Init: %v", err) } if r.Prefix != "MyApp" { @@ -161,7 +161,7 @@ func TestDerivePrefixStripsInvalidChars(t *testing.T) { defer os.Chdir(orig) r, _ := repo.FindRepo() - if err := r.Init(""); err != nil { + if err := r.Init("", nil); err != nil { t.Fatalf("Init: %v", err) } // Dots and spaces stripped, should be "mycoolap" (8 char truncation of "mycoolapp") @@ -192,7 +192,7 @@ func TestInitNoStatusGitkeeps(t *testing.T) { if err != nil { t.Fatalf("FindRepo: %v", err) } - if err := r.Init(""); err != nil { + if err := r.Init("", nil); err != nil { t.Fatalf("Init: %v", err) } @@ -231,7 +231,7 @@ func TestInitWithNonOriginRemoteViaGitConfig(t *testing.T) { os.Chdir(orig) t.Fatalf("FindRepo src: %v", err) } - if err := srcRepo.Init("test"); err != nil { + if err := srcRepo.Init("test", nil); err != nil { os.Chdir(orig) t.Fatalf("Init src: %v", err) } @@ -259,7 +259,7 @@ func TestInitWithNonOriginRemoteViaGitConfig(t *testing.T) { if err != nil { t.Fatalf("FindRepo clone: %v", err) } - if err := r.Init("test"); err != nil { + if err := r.Init("test", nil); err != nil { t.Fatalf("Init against upstream-only remote: %v", err) } diff --git a/internal/repo/migrate_test.go b/internal/repo/migrate_test.go index 6edf79c2..62ca0a69 100644 --- a/internal/repo/migrate_test.go +++ b/internal/repo/migrate_test.go @@ -55,7 +55,7 @@ func TestVersionUnset(t *testing.T) { func TestVersionAfterInit(t *testing.T) { r := initTestRepo(t) - if err := r.Init("test"); err != nil { + if err := r.Init("test", nil); err != nil { t.Fatalf("Init: %v", err) } if v := r.Version(); v != CurrentVersion { @@ -65,7 +65,7 @@ func TestVersionAfterInit(t *testing.T) { func TestVersionRoundTrip(t *testing.T) { r := initTestRepo(t) - if err := r.Init("test"); err != nil { + if err := r.Init("test", nil); err != nil { t.Fatalf("Init: %v", err) } r.SetConfig("version", "42") @@ -115,7 +115,7 @@ func TestUpgradeFromV0(t *testing.T) { func TestUpgradeAlreadyCurrent(t *testing.T) { r := initTestRepo(t) - if err := r.Init("test"); err != nil { + if err := r.Init("test", nil); err != nil { t.Fatalf("Init: %v", err) } @@ -200,7 +200,7 @@ func TestUpgradeV1ToV2PriorityMigration(t *testing.T) { func TestInitStampsVersion(t *testing.T) { r := initTestRepo(t) - if err := r.Init("test"); err != nil { + if err := r.Init("test", nil); err != nil { t.Fatalf("Init: %v", err) } diff --git a/internal/repo/repo.go b/internal/repo/repo.go index 0320641a..d6153427 100644 --- a/internal/repo/repo.go +++ b/internal/repo/repo.go @@ -133,7 +133,7 @@ func ValidatePrefix(prefix string) error { } // ForceReinit destroys the existing beadwork branch and reinitializes. -func (r *Repo) ForceReinit(prefix string) error { +func (r *Repo) ForceReinit(prefix string, resolve RemoteResolver) error { if err := ValidatePrefix(prefix); err != nil { return err } @@ -152,10 +152,24 @@ func (r *Repo) ForceReinit(prefix string) error { } r.tfs = tfs - return r.Init(prefix) + return r.Init(prefix, resolve) } -func (r *Repo) Init(prefix string) error { +// Init creates (or bootstraps from a remote) the beadwork branch. +// +// When any git remote already has the beadwork branch, Init fetches from +// one of them (selected via the same sync precedence: single-only, then +// git config beadwork.remote, then a remote named origin, then +// alphabetically first) and creates the local tracking branch from that +// tip. +// +// When no remote has the branch yet and multiple remotes exist, Init +// invokes resolve to let the caller pick which remote should be the +// project's default going forward; the selection is persisted to +// git config beadwork.remote. Passing nil skips that step — useful for +// tests and for the single-remote common case where no choice is +// needed. +func (r *Repo) Init(prefix string, resolve RemoteResolver) error { if r.initialized { return fmt.Errorf("beadwork already initialized") } @@ -164,19 +178,32 @@ func (r *Repo) Init(prefix string) error { return err } - remoteExists := r.remoteBranchExists() + allRemotes, err := r.tfs.RemoteNames() + if err != nil { + return fmt.Errorf("list remotes: %w", err) + } + var hasBW []string + for _, name := range allRemotes { + if r.remoteHasBeadwork(name) { + hasBW = append(hasBW, name) + } + } + sort.Strings(hasBW) + localExists := r.localBranchExists() - if remoteExists { - // Fetch remote branch - refSpec := config.RefSpec(fmt.Sprintf("+%s:%s", refLocal, r.refRemote())) - if err := r.fetch(r.RemoteName(), refSpec); err != nil { + if len(hasBW) > 0 { + // At least one remote has beadwork — bootstrap from it. Init + // never prompts in this branch; it picks deterministically. + fetchFrom := initFetchRemote(r, hasBW) + remoteRef := "refs/remotes/" + fetchFrom + "/" + BranchName + refSpec := config.RefSpec(fmt.Sprintf("+%s:%s", refLocal, remoteRef)) + if err := r.fetch(fetchFrom, refSpec); err != nil { return fmt.Errorf("fetch failed: %w", err) } if !localExists { - // Create local branch from remote - remoteHash, err := r.tfs.LookupRef(r.refRemote()) + remoteHash, err := r.tfs.LookupRef(remoteRef) if err != nil { return fmt.Errorf("lookup remote ref: %w", err) } @@ -193,6 +220,22 @@ func (r *Repo) Init(prefix string) error { r.tfs = tfs r.Prefix = r.readPrefix() } else if !localExists { + // No remote has beadwork yet and we're about to seed a new + // local branch. If multiple remotes exist AND sync's deterministic + // short-circuits (existing git config, a remote named origin) + // wouldn't already pick one, ask the user now so future + // `bw sync` / `bw push` runs don't have to. Short-circuit cases + // aren't persisted — sync re-applies the same rules on its own. + if len(allRemotes) >= 2 && resolve != nil && initNeedsPrompt(r, allRemotes) { + chosen, err := resolve(allRemotes) + if err != nil { + return err + } + if _, err := execGit(r.RepoDir(), "config", "beadwork.remote", chosen); err != nil { + return fmt.Errorf("persist beadwork.remote: %w", err) + } + } + // No branch anywhere — TreeFS will create it on first Commit // (baseRef is zero, so Commit creates the ref) if prefix == "" { @@ -228,6 +271,53 @@ func (r *Repo) Init(prefix string) error { return nil } +// initNeedsPrompt returns true when the "seed a fresh local branch in a +// multi-remote repo" path actually needs to ask the user. Returns false +// when git config beadwork.remote is already set to one of the remotes +// or when a remote named "origin" exists — in both cases sync will +// make the same pick on its own, so there's nothing useful to persist. +func initNeedsPrompt(r *Repo, allRemotes []string) bool { + if out, err := execGit(r.RepoDir(), "config", "--get", "beadwork.remote"); err == nil { + if cfg := strings.TrimSpace(out); cfg != "" { + for _, name := range allRemotes { + if name == cfg { + return false + } + } + } + } + for _, name := range allRemotes { + if name == "origin" { + return false + } + } + return true +} + +// initFetchRemote picks one remote from a non-empty list of remotes that +// have the beadwork branch, using the sync precedence but silently +// falling back to alphabetically-first when neither git config nor +// origin is a usable match. Never prompts. +func initFetchRemote(r *Repo, hasBW []string) string { + if len(hasBW) == 1 { + return hasBW[0] + } + if out, err := execGit(r.RepoDir(), "config", "--get", "beadwork.remote"); err == nil { + cfg := strings.TrimSpace(out) + for _, name := range hasBW { + if name == cfg { + return cfg + } + } + } + for _, name := range hasBW { + if name == "origin" { + return "origin" + } + } + return hasBW[0] +} + // AllCommits returns all commits on the beadwork branch, newest-first. func (r *Repo) AllCommits() ([]treefs.CommitInfo, error) { return r.tfs.AllCommits() @@ -237,25 +327,6 @@ func (r *Repo) Commit(message string) error { return r.tfs.Commit(message) } -func (r *Repo) remoteBranchExists() bool { - _, err := r.tfs.LookupRef(r.refRemote()) - if err == nil { - return true - } - // Also check via ls-remote for freshly cloned repos where we haven't - // fetched yet but the remote has the branch - has, _ := r.tfs.HasRemotes() - if !has { - return false - } - // Use git CLI for ls-remote since go-git remote.List requires network - out, err := execGit(r.RepoDir(), "ls-remote", "--heads", r.RemoteName(), BranchName) - if err != nil { - return false - } - return strings.TrimSpace(out) != "" -} - func (r *Repo) localBranchExists() bool { _, err := r.tfs.LookupRef(refLocal) return err == nil @@ -313,12 +384,6 @@ func (r *Repo) RemoteName() string { return "origin" } -// refRemote returns the local ref path for the fetched remote beadwork -// branch, e.g. "refs/remotes/upstream/beadwork". -func (r *Repo) refRemote() string { - return "refs/remotes/" + r.RemoteName() + "/" + BranchName -} - // GetConfig reads a single key from .bwconfig. func (r *Repo) GetConfig(key string) (string, bool) { cfg := r.ListConfig() diff --git a/internal/repo/repo_test.go b/internal/repo/repo_test.go index b31d66ce..cb065fbb 100644 --- a/internal/repo/repo_test.go +++ b/internal/repo/repo_test.go @@ -198,7 +198,7 @@ func TestInitInvalidPrefix(t *testing.T) { defer os.Chdir(orig) r, _ := repo.FindRepo() - err := r.Init("has space") + err := r.Init("has space", nil) if err == nil { t.Error("expected error for invalid prefix") } @@ -222,7 +222,7 @@ func TestDerivePrefixFallback(t *testing.T) { defer os.Chdir(orig) r, _ := repo.FindRepo() - if err := r.Init(""); err != nil { + if err := r.Init("", nil); err != nil { t.Fatalf("Init: %v", err) } // Should fallback to "bw" when dir name has no valid chars @@ -246,7 +246,7 @@ func TestCommitWithChanges(t *testing.T) { defer os.Chdir(orig) r, _ := repo.FindRepo() - r.Init("test") + r.Init("test", nil) // Create a file and commit r.TreeFS().WriteFile("issues/test.json", []byte(`{"id":"test"}`)) diff --git a/internal/repo/sync_test.go b/internal/repo/sync_test.go index 0752ce5e..cd00120a 100644 --- a/internal/repo/sync_test.go +++ b/internal/repo/sync_test.go @@ -361,7 +361,7 @@ func TestForceReinitInvalidPrefix(t *testing.T) { env := testutil.NewEnv(t) defer env.Cleanup() - err := env.Repo.ForceReinit("has space") + err := env.Repo.ForceReinit("has space", nil) if err == nil { t.Error("expected error for invalid prefix") } diff --git a/internal/testutil/testutil.go b/internal/testutil/testutil.go index 005a6414..4816288c 100644 --- a/internal/testutil/testutil.go +++ b/internal/testutil/testutil.go @@ -40,7 +40,7 @@ func NewEnv(t *testing.T) *Env { if err != nil { t.Fatalf("FindRepo: %v", err) } - if err := r.Init("test"); err != nil { + if err := r.Init("test", nil); err != nil { t.Fatalf("Init: %v", err) } @@ -86,7 +86,7 @@ func (e *Env) CloneEnv(barePath string) *Env { if err != nil { e.T.Fatalf("FindRepo in clone: %v", err) } - if err := r.Init("test"); err != nil { + if err := r.Init("test", nil); err != nil { e.T.Fatalf("Init in clone: %v", err) } From ab435c84c275af3c4cbcee172c688d9afc8fdfb9 Mon Sep 17 00:00:00 2001 From: Mac Morgan Date: Wed, 22 Apr 2026 11:49:30 -0400 Subject: [PATCH 6/9] Extract shared remote-prompt helpers into cmd/bw/remote_prompt.go promptForRemoteWithReader and gitConfigSet are used by both the sync and init resolvers; putting them in sync.go was misleading. Move them to their own file and inline the single-use promptForRemote wrapper into the sync resolver. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/bw/remote_prompt.go | 54 +++++++++++++++++++++++++++++++++++++++++ cmd/bw/sync.go | 52 +-------------------------------------- 2 files changed, 55 insertions(+), 51 deletions(-) create mode 100644 cmd/bw/remote_prompt.go diff --git a/cmd/bw/remote_prompt.go b/cmd/bw/remote_prompt.go new file mode 100644 index 00000000..e83c0eb2 --- /dev/null +++ b/cmd/bw/remote_prompt.go @@ -0,0 +1,54 @@ +package main + +import ( + "bufio" + "fmt" + "io" + "os/exec" + "strconv" + "strings" +) + +// promptForRemoteWithReader presents a numbered menu of candidate +// remotes and reads a selection from the given reader. Empty input +// defaults to the first candidate. Retries up to 3 times on invalid +// input before giving up. Shared between the sync and init prompts — +// each command passes its own stdin variable (syncStdin or initStdin) +// so the two prompts don't share input state. +func promptForRemoteWithReader(candidates []string, w Writer, source io.Reader) (string, error) { + reader := bufio.NewReader(source) + for attempt := 0; attempt < 3; attempt++ { + fmt.Fprintln(w, "multiple remotes — pick one for bw to sync with:") + for i, name := range candidates { + fmt.Fprintf(w, " %d) %s\n", i+1, name) + } + fmt.Fprint(w, "select [1]: ") + + line, err := reader.ReadString('\n') + if err != nil && line == "" { + return "", fmt.Errorf("read input: %w", err) + } + line = strings.TrimSpace(line) + if line == "" { + line = "1" + } + n, convErr := strconv.Atoi(line) + if convErr != nil || n < 1 || n > len(candidates) { + fmt.Fprintf(w, "invalid selection %q; expected a number between 1 and %d\n", line, len(candidates)) + continue + } + return candidates[n-1], nil + } + return "", fmt.Errorf("too many invalid selections") +} + +// gitConfigSet writes a git config key to the repo at repoDir. +func gitConfigSet(repoDir, key, value string) error { + cmd := exec.Command("git", "config", key, value) + cmd.Dir = repoDir + out, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("git config %s %s: %s: %w", key, value, strings.TrimSpace(string(out)), err) + } + return nil +} diff --git a/cmd/bw/sync.go b/cmd/bw/sync.go index 1c39b6c6..e7d09681 100644 --- a/cmd/bw/sync.go +++ b/cmd/bw/sync.go @@ -1,12 +1,9 @@ package main import ( - "bufio" "fmt" "io" "os" - "os/exec" - "strconv" "strings" "github.com/jallum/beadwork/internal/config" @@ -84,7 +81,7 @@ func makeRemoteResolver(r *repo.Repo, w Writer) repo.RemoteResolver { return "", fmt.Errorf("no default remote — multiple remotes, none have the %q branch, no remote is named \"origin\", and git config beadwork.remote is unset. Set one with: git config beadwork.remote (remotes: %s)", "beadwork", strings.Join(candidates, ", ")) } - chosen, err := promptForRemote(candidates, w) + chosen, err := promptForRemoteWithReader(candidates, w, syncStdin) if err != nil { return "", err } @@ -95,50 +92,3 @@ func makeRemoteResolver(r *repo.Repo, w Writer) repo.RemoteResolver { return chosen, nil } } - -// promptForRemote presents a numbered menu and reads a selection from -// syncStdin. Re-prompts up to 3 times on invalid input. -func promptForRemote(candidates []string, w Writer) (string, error) { - return promptForRemoteWithReader(candidates, w, syncStdin) -} - -// promptForRemoteWithReader is the shared implementation behind both the -// sync and init prompts. The caller supplies the input source so init -// can use initStdin and sync can use syncStdin without either depending -// on the other's package-level variable. -func promptForRemoteWithReader(candidates []string, w Writer, source io.Reader) (string, error) { - reader := bufio.NewReader(source) - for attempt := 0; attempt < 3; attempt++ { - fmt.Fprintln(w, "multiple remotes — pick one for bw to sync with:") - for i, name := range candidates { - fmt.Fprintf(w, " %d) %s\n", i+1, name) - } - fmt.Fprint(w, "select [1]: ") - - line, err := reader.ReadString('\n') - if err != nil && line == "" { - return "", fmt.Errorf("read input: %w", err) - } - line = strings.TrimSpace(line) - if line == "" { - line = "1" - } - n, convErr := strconv.Atoi(line) - if convErr != nil || n < 1 || n > len(candidates) { - fmt.Fprintf(w, "invalid selection %q; expected a number between 1 and %d\n", line, len(candidates)) - continue - } - return candidates[n-1], nil - } - return "", fmt.Errorf("too many invalid selections") -} - -func gitConfigSet(repoDir, key, value string) error { - cmd := exec.Command("git", "config", key, value) - cmd.Dir = repoDir - out, err := cmd.CombinedOutput() - if err != nil { - return fmt.Errorf("git config %s %s: %s: %w", key, value, strings.TrimSpace(string(out)), err) - } - return nil -} From abab0dba41837b74d45a27bc0094b0dc2d77fe44 Mon Sep 17 00:00:00 2001 From: Mac Morgan Date: Wed, 22 Apr 2026 12:00:04 -0400 Subject: [PATCH 7/9] Consolidate sync/init remote resolvers into one constructor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit makeRemoteResolver now takes a stdin source and is shared by both commands; makeInitRemoteResolver is gone. Init therefore inherits the same isInteractiveStdin gate as sync: non-interactive runs that would otherwise need to prompt now error out with the same "no default remote — ..." message. Callers who can't interact set git config beadwork.remote before running bw init. Tests: TestCmdInitMultiRemotePrompts now stubs isInteractiveStdin to true; the former TestCmdInitMultiRemoteIgnoresTTYStub is inverted to TestCmdInitNonInteractiveFailsWithoutPrompt, mirroring sync's gate test. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/bw/init.go | 30 +++--------------------------- cmd/bw/init_test.go | 41 ++++++++++++++++++++++++++++------------- cmd/bw/sync.go | 15 ++++++++------- 3 files changed, 39 insertions(+), 47 deletions(-) diff --git a/cmd/bw/init.go b/cmd/bw/init.go index 9b8446c3..f4ebcb91 100644 --- a/cmd/bw/init.go +++ b/cmd/bw/init.go @@ -9,14 +9,11 @@ import ( "github.com/jallum/beadwork/internal/config" "github.com/jallum/beadwork/internal/issue" - "github.com/jallum/beadwork/internal/repo" ) // initStdin is the input source for the init remote-selection prompt. -// Tests override it with a strings.Reader. Unlike sync's syncStdin, the -// init prompt is never gated on a TTY check: `bw init` is a -// user-initiated setup command, so we always prompt when there's an -// ambiguous seed-remote choice. +// Tests override it with a strings.Reader. Kept separate from syncStdin +// so init and sync tests can drive their prompts independently. var initStdin io.Reader = os.Stdin type InitArgs struct { @@ -45,7 +42,7 @@ func cmdInit(_ *issue.Store, args []string, w Writer, _ *config.Config) (*config if err != nil { return nil, err } - resolver := makeInitRemoteResolver(r, w) + resolver := makeRemoteResolver(r, w, initStdin) if ia.Force { if err := r.ForceReinit(ia.Prefix, resolver); err != nil { return nil, err @@ -63,27 +60,6 @@ func cmdInit(_ *issue.Store, args []string, w Writer, _ *config.Config) (*config return nil, nil } -// makeInitRemoteResolver returns a RemoteResolver that unconditionally -// prompts (no TTY check) and persists the choice via git config. Init -// is a human-driven setup step, so there's no non-interactive path to -// guard against — if someone really can't interact, they can set -// git config beadwork.remote before running bw init and the -// resolver will never be called. -func makeInitRemoteResolver(r *repo.Repo, w Writer) repo.RemoteResolver { - return func(candidates []string) (string, error) { - chosen, err := promptForRemoteWithReader(candidates, w, initStdin) - if err != nil { - return "", err - } - if err := gitConfigSet(r.RepoDir(), "beadwork.remote", chosen); err != nil { - return "", fmt.Errorf("set beadwork.remote: %w", err) - } - fmt.Fprintf(w, "saved: git config beadwork.remote=%s\n", chosen) - return chosen, nil - } -} - - func initMessage(verb, prefix string) string { var b strings.Builder fmt.Fprintf(&b, "%s beadwork (prefix: %s)\n", verb, prefix) diff --git a/cmd/bw/init_test.go b/cmd/bw/init_test.go index 96a1fecb..96387b0c 100644 --- a/cmd/bw/init_test.go +++ b/cmd/bw/init_test.go @@ -175,16 +175,21 @@ func setupFreshRepoWithRemotes(t *testing.T, remotes ...string) (string, func()) return dir, func() { os.Chdir(orig) } } -// TestCmdInitMultiRemotePrompts exercises the new init prompt: multiple +// TestCmdInitMultiRemotePrompts exercises the init prompt: multiple // remotes exist, none have the beadwork branch yet, no origin, no git // config — init must ask the user which remote to seed and persist the -// choice. Unlike sync, init never consults isInteractiveStdin. +// choice. func TestCmdInitMultiRemotePrompts(t *testing.T) { dir, cleanup := setupFreshRepoWithRemotes(t, "alpha", "beta") defer cleanup() origStdin := initStdin - defer func() { initStdin = origStdin }() + origInteractive := isInteractiveStdin + defer func() { + initStdin = origStdin + isInteractiveStdin = origInteractive + }() + isInteractiveStdin = func() bool { return true } initStdin = strings.NewReader("2\n") // pick beta var buf bytes.Buffer @@ -201,11 +206,13 @@ func TestCmdInitMultiRemotePrompts(t *testing.T) { } } -// TestCmdInitMultiRemoteIgnoresTTYStub confirms the init prompt fires -// even when isInteractiveStdin (sync's gate) is stubbed to return false. -// Init is human-initiated; the TTY check is sync's concern, not init's. -func TestCmdInitMultiRemoteIgnoresTTYStub(t *testing.T) { - _, cleanup := setupFreshRepoWithRemotes(t, "alpha", "beta") +// TestCmdInitNonInteractiveFailsWithoutPrompt confirms that init honors +// the same TTY gate as sync: when isInteractiveStdin returns false and +// no short-circuit rule resolves the remote, init errors out instead +// of consuming piped input. The caller can set +// `git config beadwork.remote ` beforehand to skip the prompt. +func TestCmdInitNonInteractiveFailsWithoutPrompt(t *testing.T) { + dir, cleanup := setupFreshRepoWithRemotes(t, "alpha", "beta") defer cleanup() origStdin := initStdin @@ -214,15 +221,23 @@ func TestCmdInitMultiRemoteIgnoresTTYStub(t *testing.T) { initStdin = origStdin isInteractiveStdin = origInteractive }() - initStdin = strings.NewReader("1\n") isInteractiveStdin = func() bool { return false } + primed := strings.NewReader("1\n") + initStdin = primed var buf bytes.Buffer - if err := cmdInit(nil, []string{"--prefix", "alpha"}, PlainWriter(&buf)); err != nil { - t.Fatalf("cmdInit: %v: %s", err, buf.String()) + err := cmdInit(nil, []string{"--prefix", "alpha"}, PlainWriter(&buf)) + if err == nil { + t.Fatalf("expected error in non-interactive multi-remote init; output=%q", buf.String()) } - if !strings.Contains(buf.String(), "initialized") { - t.Errorf("output missing 'initialized': %q", buf.String()) + if !strings.Contains(err.Error(), "no default remote") { + t.Errorf("error = %v, want a 'no default remote' message", err) + } + if primed.Len() == 0 { + t.Error("resolver consumed initStdin input despite non-interactive stdin") + } + if v := getGitConfig(t, dir, "beadwork.remote"); v != "" { + t.Errorf("git config beadwork.remote = %q, want unset", v) } } diff --git a/cmd/bw/sync.go b/cmd/bw/sync.go index e7d09681..4a08ff21 100644 --- a/cmd/bw/sync.go +++ b/cmd/bw/sync.go @@ -32,7 +32,7 @@ func cmdSync(store *issue.Store, args []string, w Writer, _ *config.Config) (*co _ = args r := store.Committer.(*repo.Repo) - resolver := makeRemoteResolver(r, w) + resolver := makeRemoteResolver(r, w, syncStdin) status, intents, err := r.Sync(resolver) if err != nil { @@ -71,17 +71,18 @@ func cmdSync(store *issue.Store, args []string, w Writer, _ *config.Config) (*co return nil, nil } -// makeRemoteResolver returns a RemoteResolver closed over the repo and -// the CLI writer. When stdin is interactive it prompts; otherwise it -// returns the same non-interactive error that a nil resolver would -// produce from the repo layer. -func makeRemoteResolver(r *repo.Repo, w Writer) repo.RemoteResolver { +// makeRemoteResolver returns a RemoteResolver closed over the repo, the +// CLI writer, and a stdin source. Shared by sync and init; each command +// passes its own stdin var so tests can drive them independently. When +// stdin isn't interactive we never prompt — the error message mirrors +// what a nil resolver would produce from the repo layer. +func makeRemoteResolver(r *repo.Repo, w Writer, source io.Reader) repo.RemoteResolver { return func(candidates []string) (string, error) { if !isInteractiveStdin() { return "", fmt.Errorf("no default remote — multiple remotes, none have the %q branch, no remote is named \"origin\", and git config beadwork.remote is unset. Set one with: git config beadwork.remote (remotes: %s)", "beadwork", strings.Join(candidates, ", ")) } - chosen, err := promptForRemoteWithReader(candidates, w, syncStdin) + chosen, err := promptForRemoteWithReader(candidates, w, source) if err != nil { return "", err } From cc2d35669298101d023d95e720a0ca77c0f7ed99 Mon Sep 17 00:00:00 2001 From: Jason Allum <21417+jallum@users.noreply.github.com> Date: Thu, 7 May 2026 10:56:35 -0400 Subject: [PATCH 8/9] Sync with first beadwork remote only; warn on multiple Replace multi-remote fan-out with single-target sync: targetRemotes now returns the first remote (alphabetically) that has the beadwork branch rather than all of them, emitting a warning to stderr when more than one is found. This removes the three-phase multi-remote loop from syncTo, collapsing it to a straightforward fetch/merge/push against one remote. pushTo is removed; Push inlines the single-call. Tests updated to assert the new behaviour: the primary remote advances and secondary remotes are left untouched. --- cmd/bw/init_test.go | 12 +-- cmd/bw/sync_test.go | 10 +- internal/repo/repo.go | 185 ++++++++++++++----------------------- internal/repo/sync_test.go | 50 +++++----- 4 files changed, 106 insertions(+), 151 deletions(-) diff --git a/cmd/bw/init_test.go b/cmd/bw/init_test.go index 96387b0c..ec64b188 100644 --- a/cmd/bw/init_test.go +++ b/cmd/bw/init_test.go @@ -193,7 +193,7 @@ func TestCmdInitMultiRemotePrompts(t *testing.T) { initStdin = strings.NewReader("2\n") // pick beta var buf bytes.Buffer - if err := cmdInit(nil, []string{"--prefix", "multi"}, PlainWriter(&buf)); err != nil { + if _, err := cmdInit(nil, []string{"--prefix", "multi"}, PlainWriter(&buf), nil); err != nil { t.Fatalf("cmdInit: %v: %s", err, buf.String()) } if !strings.Contains(buf.String(), "initialized") { @@ -226,7 +226,7 @@ func TestCmdInitNonInteractiveFailsWithoutPrompt(t *testing.T) { initStdin = primed var buf bytes.Buffer - err := cmdInit(nil, []string{"--prefix", "alpha"}, PlainWriter(&buf)) + _, err := cmdInit(nil, []string{"--prefix", "alpha"}, PlainWriter(&buf), nil) if err == nil { t.Fatalf("expected error in non-interactive multi-remote init; output=%q", buf.String()) } @@ -253,7 +253,7 @@ func TestCmdInitMultiRemoteOriginShortcut(t *testing.T) { initStdin = primed var buf bytes.Buffer - if err := cmdInit(nil, []string{"--prefix", "o"}, PlainWriter(&buf)); err != nil { + if _, err := cmdInit(nil, []string{"--prefix", "o"}, PlainWriter(&buf), nil); err != nil { t.Fatalf("cmdInit: %v: %s", err, buf.String()) } if primed.Len() == 0 { @@ -282,7 +282,7 @@ func TestCmdInitMultiRemoteGitConfigShortcut(t *testing.T) { initStdin = primed var buf bytes.Buffer - if err := cmdInit(nil, []string{"--prefix", "g"}, PlainWriter(&buf)); err != nil { + if _, err := cmdInit(nil, []string{"--prefix", "g"}, PlainWriter(&buf), nil); err != nil { t.Fatalf("cmdInit: %v: %s", err, buf.String()) } if primed.Len() == 0 { @@ -297,7 +297,7 @@ func TestCmdInitBootstrapsFromRemoteWithBeadwork(t *testing.T) { // Seed a bare repo by doing a full init-and-push in a source repo. srcDir, cleanupSrc := setupFreshRepoWithRemotes(t, "alpha") var buf bytes.Buffer - if err := cmdInit(nil, []string{"--prefix", "seed"}, PlainWriter(&buf)); err != nil { + if _, err := cmdInit(nil, []string{"--prefix", "seed"}, PlainWriter(&buf), nil); err != nil { cleanupSrc() t.Fatalf("seed cmdInit: %v: %s", err, buf.String()) } @@ -341,7 +341,7 @@ func TestCmdInitBootstrapsFromRemoteWithBeadwork(t *testing.T) { initStdin = primed var buf2 bytes.Buffer - if err := cmdInit(nil, []string{}, PlainWriter(&buf2)); err != nil { + if _, err := cmdInit(nil, []string{}, PlainWriter(&buf2), nil); err != nil { t.Fatalf("cmdInit: %v: %s", err, buf2.String()) } if primed.Len() == 0 { diff --git a/cmd/bw/sync_test.go b/cmd/bw/sync_test.go index 1925369a..6800017c 100644 --- a/cmd/bw/sync_test.go +++ b/cmd/bw/sync_test.go @@ -132,7 +132,7 @@ func TestCmdSyncNonInteractiveFailsWithoutPrompt(t *testing.T) { env.Repo.Commit("create needs resolution") var buf bytes.Buffer - err := cmdSync(env.Store, []string{}, PlainWriter(&buf)) + _, err := cmdSync(env.Store, []string{}, PlainWriter(&buf), nil) if err == nil { t.Fatalf("expected error in non-interactive multi-remote run; output=%q", buf.String()) } @@ -175,7 +175,7 @@ func TestCmdSyncInteractivePromptPersists(t *testing.T) { env.Repo.Commit("create interactive") var buf bytes.Buffer - if err := cmdSync(env.Store, []string{}, PlainWriter(&buf)); err != nil { + if _, err := cmdSync(env.Store, []string{}, PlainWriter(&buf), nil); err != nil { t.Fatalf("cmdSync: %v: %s", err, buf.String()) } if !strings.Contains(buf.String(), "pushed") { @@ -194,7 +194,7 @@ func TestCmdSyncInteractivePromptPersists(t *testing.T) { env.Repo.Commit("create second") var buf2 bytes.Buffer - if err := cmdSync(env.Store, []string{}, PlainWriter(&buf2)); err != nil { + if _, err := cmdSync(env.Store, []string{}, PlainWriter(&buf2), nil); err != nil { t.Fatalf("second cmdSync: %v: %s", err, buf2.String()) } if strings.Contains(buf2.String(), "multiple remotes") { @@ -230,7 +230,7 @@ func TestCmdSyncGitConfigShortcutSkipsPrompt(t *testing.T) { env.Repo.Commit("create config shortcut") var buf bytes.Buffer - if err := cmdSync(env.Store, []string{}, PlainWriter(&buf)); err != nil { + if _, err := cmdSync(env.Store, []string{}, PlainWriter(&buf), nil); err != nil { t.Fatalf("cmdSync: %v: %s", err, buf.String()) } if primed.Len() == 0 { @@ -269,7 +269,7 @@ func TestCmdSyncOriginShortcutSkipsPrompt(t *testing.T) { env.Repo.Commit("create origin shortcut") var buf bytes.Buffer - if err := cmdSync(env.Store, []string{}, PlainWriter(&buf)); err != nil { + if _, err := cmdSync(env.Store, []string{}, PlainWriter(&buf), nil); err != nil { t.Fatalf("cmdSync: %v: %s", err, buf.String()) } if primed.Len() == 0 { diff --git a/internal/repo/repo.go b/internal/repo/repo.go index d6153427..51f01a2a 100644 --- a/internal/repo/repo.go +++ b/internal/repo/repo.go @@ -471,7 +471,10 @@ func (r *Repo) targetRemotes(resolve RemoteResolver) ([]string, error) { } if len(hasBW) > 0 { sort.Strings(hasBW) - return hasBW, nil + if len(hasBW) > 1 { + fmt.Fprintf(os.Stderr, "warning: multiple remotes have the beadwork branch (%s); using %q\n", strings.Join(hasBW, ", "), hasBW[0]) + } + return []string{hasBW[0]}, nil } chosen, err := r.resolveSingleRemote(all, resolve) if err != nil { @@ -507,16 +510,15 @@ func (r *Repo) resolveSingleRemote(all []string, resolve RemoteResolver) (string return "", fmt.Errorf("no default remote — multiple remotes, none have the %q branch, no remote is named \"origin\", and git config beadwork.remote is unset. Set one with: git config beadwork.remote (remotes: %s)", BranchName, strings.Join(all, ", ")) } -// Sync fetches from every remote that has the beadwork branch, merges -// their tips into local, and pushes the result back to every one of them. -// If no remote has the branch yet, it resolves a single remote via the -// precedence rules (single-remote auto-pick, git config beadwork.remote, -// "origin" by name, or the resolver callback for interactive selection) -// and pushes to just that one. +// Sync fetches from the target remote, merges its tip into local, and +// pushes the result back. The target is the first remote (alphabetically) +// that already has the beadwork branch; if none do, a single remote is +// resolved via the precedence rules (single-remote auto-pick, git config +// beadwork.remote, "origin" by name, resolver callback). // -// On merge conflict against any remote, returns ("needs replay", -// conflicting-local-commits, nil) with preReplayHash captured. Callers run -// intent.Replay then re-invoke Sync to finish the fan-out. +// On merge conflict returns ("needs replay", conflicting-local-commits, +// nil) with preReplayHash captured. Callers run intent.Replay then +// re-invoke Sync to finish the push. func (r *Repo) Sync(resolve RemoteResolver) (status string, replayed []string, err error) { if !r.hasRemote() { return "no remote configured", nil, nil @@ -530,119 +532,81 @@ func (r *Repo) Sync(resolve RemoteResolver) (status string, replayed []string, e return "no remote configured", nil, nil } - return r.syncTo(remotes) + return r.syncTo(remotes[0]) } -// syncTo runs the three-phase multi-remote sync across the given remotes: -// fetch each, merge each into local in sorted order (stopping on conflict), -// then push local to each. -func (r *Repo) syncTo(remotes []string) (string, []string, error) { - // Phase A: fetch every target remote into its own tracking ref. - // A failure on one remote (e.g. network, missing beadwork branch) - // is warned-and-skipped so the rest of the sync can proceed. - fetched := make([]string, 0, len(remotes)) - for _, name := range remotes { - refSpec := config.RefSpec(fmt.Sprintf("+%s:refs/remotes/%s/%s", refLocal, name, BranchName)) - if err := r.fetch(name, refSpec); err != nil { - fmt.Fprintf(os.Stderr, "warning: fetch from %s failed: %v\n", name, err) - continue +func (r *Repo) syncTo(remote string) (string, []string, error) { + refSpec := config.RefSpec(fmt.Sprintf("+%s:refs/remotes/%s/%s", refLocal, remote, BranchName)) + _ = r.fetch(remote, refSpec) // failure OK — remote may not have beadwork yet + + remoteRef := "refs/remotes/" + remote + "/" + BranchName + pushRef := config.RefSpec(refLocal + ":" + refLocal) + + remoteHash, err := r.tfs.LookupRef(remoteRef) + if err != nil { + // Remote has no beadwork branch — push to seed it. + if err := r.gitPush(remote, pushRef); err != nil { + return "", nil, fmt.Errorf("push to %s: %w", remote, err) } - fetched = append(fetched, name) + return "pushed", nil, nil } - sort.Strings(fetched) - // Phase B: merge each fetched remote's tip into local. - didMerge := false - didFastForward := false - for _, name := range fetched { - remoteRef := "refs/remotes/" + name + "/" + BranchName - remoteHash, err := r.tfs.LookupRef(remoteRef) - if err != nil { - // Tracking ref missing despite successful fetch — skip. - continue - } - localHash := r.tfs.RefHash() + localHash := r.tfs.RefHash() + localCommits, err := r.tfs.CommitsBetween(localHash, remoteHash) + if err != nil { + return "", nil, err + } - localCommits, err := r.tfs.CommitsBetween(localHash, remoteHash) - if err != nil { - return "", nil, err - } - if len(localCommits) == 0 { - if localHash != remoteHash { - if err := r.tfs.Reset(remoteHash); err != nil { - return "", nil, fmt.Errorf("fast-forward from %s: %w", name, err) - } - didFastForward = true + if len(localCommits) == 0 { + // Local is at or behind remote. + if localHash != remoteHash { + if err := r.tfs.Reset(remoteHash); err != nil { + return "", nil, fmt.Errorf("fast-forward from %s: %w", remote, err) } - continue } + return "up to date", nil, nil + } - remoteCommits, err := r.tfs.CommitsBetween(remoteHash, localHash) - if err != nil { - return "", nil, err - } - if len(remoteCommits) == 0 { - // Local strictly ahead of this remote — Phase C will push. - continue - } + remoteCommits, err := r.tfs.CommitsBetween(remoteHash, localHash) + if err != nil { + return "", nil, err + } - // Diverged — attempt 3-way tree merge. - localMsgs := make([]string, 0, len(localCommits)) - for _, c := range localCommits { - localMsgs = append(localMsgs, c.Message) - } - merged, err := r.tfs.MergeCommit(localHash, remoteHash, localMsgs) - if err != nil { - return "", nil, fmt.Errorf("merge with %s: %w", name, err) - } - if merged { - didMerge = true - continue + if len(remoteCommits) == 0 { + // Local strictly ahead — push. + if err := r.gitPush(remote, pushRef); err != nil { + return "", nil, fmt.Errorf("push to %s: %w", remote, err) } + return "pushed", nil, nil + } - // Conflict: capture pre-reset hash for attachment-blob recovery - // (same as the single-remote path previously did), reset to this - // remote's tip, and bubble the conflicting intents out for replay. - r.preReplayHash = localHash - if err := r.tfs.Reset(remoteHash); err != nil { - return "", nil, fmt.Errorf("reset to %s: %w", name, err) - } - return "needs replay", localMsgs, nil - } - - // Phase C: push local to every target remote. Only counts as "pushed" - // when local is actually ahead of (or unrelated to) the remote's tip. - pushRefSpec := config.RefSpec(refLocal + ":" + refLocal) - pushedCount := 0 - for _, name := range remotes { - remoteRef := "refs/remotes/" + name + "/" + BranchName - localHash := r.tfs.RefHash() - remoteHash, refErr := r.tfs.LookupRef(remoteRef) - needsPush := refErr != nil || remoteHash != localHash - if !needsPush { - continue - } - if err := r.gitPush(name, pushRefSpec); err != nil { - return "", nil, fmt.Errorf("push to %s failed: %w", name, err) + // Diverged — attempt 3-way tree merge. + localMsgs := make([]string, 0, len(localCommits)) + for _, c := range localCommits { + localMsgs = append(localMsgs, c.Message) + } + merged, err := r.tfs.MergeCommit(localHash, remoteHash, localMsgs) + if err != nil { + return "", nil, fmt.Errorf("merge with %s: %w", remote, err) + } + if merged { + if err := r.gitPush(remote, pushRef); err != nil { + return "", nil, fmt.Errorf("push after merge: %w", err) } - pushedCount++ + return "rebased and pushed", nil, nil } - switch { - case didMerge && pushedCount > 0: - return "rebased and pushed", nil, nil - case pushedCount > 0: - return "pushed", nil, nil - case didFastForward: - return "up to date", nil, nil - default: - return "up to date", nil, nil + // Conflict — capture pre-reset hash for attachment-blob recovery, + // reset to the remote tip, and bubble the local intents out for replay. + r.preReplayHash = localHash + if err := r.tfs.Reset(remoteHash); err != nil { + return "", nil, fmt.Errorf("reset to %s: %w", remote, err) } + return "needs replay", localMsgs, nil } -// Push pushes the beadwork branch to every remote that has the branch, or -// to a single resolved remote if none of them do. See Sync for the -// resolver semantics. +// Push pushes the beadwork branch to the target remote. See Sync for +// the target-remote selection semantics. func (r *Repo) Push(resolve RemoteResolver) error { if !r.hasRemote() { return fmt.Errorf("no remote configured") @@ -654,15 +618,8 @@ func (r *Repo) Push(resolve RemoteResolver) error { if len(remotes) == 0 { return fmt.Errorf("no remote configured") } - return r.pushTo(remotes) -} - -func (r *Repo) pushTo(remotes []string) error { - refSpec := config.RefSpec(refLocal + ":" + refLocal) - for _, name := range remotes { - if err := r.gitPush(name, refSpec); err != nil { - return fmt.Errorf("push to %s failed: %w", name, err) - } + if err := r.gitPush(remotes[0], config.RefSpec(refLocal+":"+refLocal)); err != nil { + return fmt.Errorf("push to %s failed: %w", remotes[0], err) } return nil } diff --git a/internal/repo/sync_test.go b/internal/repo/sync_test.go index cd00120a..22cda646 100644 --- a/internal/repo/sync_test.go +++ b/internal/repo/sync_test.go @@ -479,10 +479,10 @@ func beadworkTip(t *testing.T, bare string) string { return strings.TrimSpace(string(out)) } -// TestSyncMultiRemoteBothHaveBeadwork covers the core new behavior: when -// more than one remote has the beadwork branch, sync pushes new commits -// to every one of them. -func TestSyncMultiRemoteBothHaveBeadwork(t *testing.T) { +// TestSyncMultiRemoteUsesFirst verifies that when multiple remotes have +// the beadwork branch, sync uses only the first one alphabetically and +// leaves the others untouched. +func TestSyncMultiRemoteUsesFirst(t *testing.T) { env := testutil.NewEnv(t) defer env.Cleanup() @@ -493,21 +493,21 @@ func TestSyncMultiRemoteBothHaveBeadwork(t *testing.T) { gitRun(t, env.Dir, "remote", "add", "alpha", bare1) gitRun(t, env.Dir, "remote", "add", "beta", bare2) - // Initial push via git config beadwork.remote; only alpha has beadwork now. + // Initial push via git config; only alpha has beadwork now. gitRun(t, env.Dir, "config", "beadwork.remote", "alpha") env.Store.Create("Seed", issue.CreateOpts{}) env.CommitIntent("create seed") if _, _, err := env.Repo.Sync(nil); err != nil { t.Fatalf("initial sync: %v", err) } - // Seed beta so both have beadwork. + // Manually seed beta so both have beadwork. gitRun(t, env.Dir, "push", "beta", "refs/heads/beadwork:refs/heads/beadwork") seedTip := beadworkTip(t, bare1) if seedTip == "" || seedTip != beadworkTip(t, bare2) { t.Fatalf("pre-test seed failed: alpha=%q beta=%q", seedTip, beadworkTip(t, bare2)) } - // New commit locally, then sync — should push to both remotes. + // New commit locally, then sync — should push only to alpha (first alphabetically). env.Store.Create("Multi", issue.CreateOpts{}) env.CommitIntent("create multi") @@ -523,11 +523,8 @@ func TestSyncMultiRemoteBothHaveBeadwork(t *testing.T) { if tip1 == seedTip { t.Errorf("alpha tip did not advance: still %q", tip1) } - if tip2 == seedTip { - t.Errorf("beta tip did not advance: still %q", tip2) - } - if tip1 != tip2 { - t.Errorf("tips diverged after sync: alpha=%q beta=%q", tip1, tip2) + if tip2 != seedTip { + t.Errorf("beta was updated but should have been left at seed tip: %q → %q", seedTip, tip2) } } @@ -571,10 +568,10 @@ func TestSyncMultiRemoteOnlyOneHasBeadwork(t *testing.T) { } } -// TestSyncMultiRemoteConflictFansOutAfterReplay confirms that when a -// conflict on one remote triggers `needs replay`, replaying and -// re-invoking sync fans the merged state out to every target remote. -func TestSyncMultiRemoteConflictFansOutAfterReplay(t *testing.T) { +// TestSyncConflictReplayIgnoresSecondaryRemote confirms that conflict → +// replay → sync works correctly against the primary remote (alpha) while +// a secondary remote with beadwork (beta) is left untouched. +func TestSyncConflictReplayIgnoresSecondaryRemote(t *testing.T) { env := testutil.NewEnv(t) defer env.Cleanup() @@ -586,16 +583,16 @@ func TestSyncMultiRemoteConflictFansOutAfterReplay(t *testing.T) { gitRun(t, env.Dir, "remote", "add", "beta", bare2) gitRun(t, env.Dir, "config", "beadwork.remote", "alpha") - // Seed a shared issue on both remotes. + // Seed alpha, then manually push to beta so both have beadwork. shared, _ := env.Store.Create("Shared", issue.CreateOpts{}) env.CommitIntent("create " + shared.ID + " p3 task \"Shared\"") if _, _, err := env.Repo.Sync(nil); err != nil { t.Fatalf("seed sync: %v", err) } gitRun(t, env.Dir, "push", "beta", "refs/heads/beadwork:refs/heads/beadwork") + betaSeedTip := beadworkTip(t, bare2) - // Clone the alpha bare, modify the shared issue, push back to alpha. - // This makes alpha diverge from local. + // Clone alpha, push a diverging commit back to alpha. env2 := env.CloneEnv(bare1) defer env2.Cleanup() env2.SwitchTo() @@ -606,15 +603,13 @@ func TestSyncMultiRemoteConflictFansOutAfterReplay(t *testing.T) { t.Fatalf("clone sync: %v", err) } - // Back to original, make a conflicting edit locally. + // Back to original, make a conflicting local edit. env.SwitchTo() assignee := "local-agent" env.Store.Update(shared.ID, issue.UpdateOpts{Assignee: &assignee}) env.CommitIntent("update " + shared.ID + " assignee=local-agent") - // First sync: alpha diverged → needs replay (or clean rebase if git - // happens to auto-merge). Either way, after handling it the state - // must fan out to beta too. + // Sync with alpha (the primary): expect conflict or clean rebase. status, intents, err := env.Repo.Sync(nil) if err != nil { t.Fatalf("first sync: %v", err) @@ -631,9 +626,12 @@ func TestSyncMultiRemoteConflictFansOutAfterReplay(t *testing.T) { } } - // After everything settles, alpha and beta should agree. - if a, b := beadworkTip(t, bare1), beadworkTip(t, bare2); a != b || a == "" { - t.Errorf("remotes did not converge after replay: alpha=%q beta=%q", a, b) + // Alpha should have the resolved state; beta should still be at its seed tip. + if a := beadworkTip(t, bare1); a == "" { + t.Error("alpha has no beadwork tip after replay") + } + if b := beadworkTip(t, bare2); b != betaSeedTip { + t.Errorf("beta was modified but should have been left at seed tip: was %q, now %q", betaSeedTip, b) } } From a9be8c08c0ca4fb0860b09873fb2f861af4f1013 Mon Sep 17 00:00:00 2001 From: Jason Allum <21417+jallum@users.noreply.github.com> Date: Thu, 7 May 2026 14:09:03 -0400 Subject: [PATCH 9/9] gofmt cmd/bw/sync_test.go --- cmd/bw/sync_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/bw/sync_test.go b/cmd/bw/sync_test.go index 6800017c..8bf513c9 100644 --- a/cmd/bw/sync_test.go +++ b/cmd/bw/sync_test.go @@ -252,7 +252,7 @@ func TestCmdSyncOriginShortcutSkipsPrompt(t *testing.T) { env := testutil.NewEnv(t) defer env.Cleanup() - env.NewBareRemote() // adds "origin" + env.NewBareRemote() // adds "origin" addBareRemote(t, env, "upstream") // second remote, without beadwork origInteractive := isInteractiveStdin