From 0125d139815f2527c9d88f14fa8a26acd50f4893 Mon Sep 17 00:00:00 2001 From: Chris Deeming Date: Tue, 18 Aug 2026 01:07:58 +0100 Subject: [PATCH 01/14] fix(docker): read database credentials from the compose variables getDatabaseCredentials read MYSQL_USER and MYSQL_PASSWORD, but nothing sets those. The compose files declare XF_DB_USER and XF_DB_PASSWORD (compose.mysql.yaml:8, compose.postgres.yaml:14), so those are the keys that reach .env. The lookup therefore always fell through to the built-in defaults. That happened to work for a stock install, but silently broke for anyone who customised the database user: WaitForDatabase would connect with the wrong credentials and time out after 30 attempts against a database that was in fact ready. Read the correct keys, and resolve them the way docker compose does: process environment, then .env, then the default. --- internal/dockercompose/credentials_test.go | 109 +++++++++++++++++++++ internal/dockercompose/runner.go | 38 +++---- 2 files changed, 130 insertions(+), 17 deletions(-) create mode 100644 internal/dockercompose/credentials_test.go diff --git a/internal/dockercompose/credentials_test.go b/internal/dockercompose/credentials_test.go new file mode 100644 index 0000000..d402a0f --- /dev/null +++ b/internal/dockercompose/credentials_test.go @@ -0,0 +1,109 @@ +package dockercompose + +import ( + "os" + "path/filepath" + "testing" +) + +// newRunnerWithEnv builds a Runner whose .env contains the given contents. +func newRunnerWithEnv(t *testing.T, env string) *Runner { + t.Helper() + + dir := t.TempDir() + + if err := os.MkdirAll(filepath.Join(dir, "src"), 0o750); err != nil { + t.Fatalf("mkdir src: %v", err) + } + + if err := os.WriteFile(filepath.Join(dir, "src", "XF.php"), []byte(" Date: Tue, 18 Aug 2026 01:19:19 +0100 Subject: [PATCH 02/14] feat(worktree): add path resolution, git helpers and registry Foundation for `xf worktree`. No user-facing command yet; this is the layer the command will be built on, kept separate so it can be reviewed and tested without Docker. Paths are deterministic: a worktree for branch dev/24x/feature of ~/Sites/main always resolves to ~/Sites/main.worktrees/dev-24x-feature. Siblings keep worktrees on the same filesystem as the source, which matters for Docker bind mounts, and make them discoverable without knowing an xf-specific convention. BranchToDirName is lossy by design, since slashes become dashes. It guarantees a single path segment that cannot escape the worktrees directory whatever the branch contains, which is covered by a property test over traversal attempts. Callers must still check for an existing directory, as dev/24x/feature and dev-24x-feature collide. SourceCheckout resolves through --git-common-dir, so running from inside a linked worktree returns the original checkout rather than nesting a worktree inside a worktree. The registry records worktrees so they can be listed across projects, which git alone cannot do. It is explicitly not the source of truth: git and Docker are. Worktrees get removed behind xf's back, so a missing or damaged registry is recoverable rather than fatal, and writes are atomic via a temporary file and rename. --- internal/worktree/git.go | 91 +++++++++++++ internal/worktree/git_test.go | 159 ++++++++++++++++++++++ internal/worktree/paths.go | 67 +++++++++ internal/worktree/paths_test.go | 102 ++++++++++++++ internal/worktree/registry.go | 209 +++++++++++++++++++++++++++++ internal/worktree/registry_test.go | 167 +++++++++++++++++++++++ 6 files changed, 795 insertions(+) create mode 100644 internal/worktree/git.go create mode 100644 internal/worktree/git_test.go create mode 100644 internal/worktree/paths.go create mode 100644 internal/worktree/paths_test.go create mode 100644 internal/worktree/registry.go create mode 100644 internal/worktree/registry_test.go diff --git a/internal/worktree/git.go b/internal/worktree/git.go new file mode 100644 index 0000000..0a04311 --- /dev/null +++ b/internal/worktree/git.go @@ -0,0 +1,91 @@ +package worktree + +import ( + "context" + "errors" + "fmt" + "os/exec" + "path/filepath" + "strings" +) + +// ErrNotARepository indicates a path is not inside a git repository. +var ErrNotARepository = errors.New("not a git repository") + +// SourceCheckout returns the main checkout for the repository containing dir. +// +// When dir is inside a linked worktree this returns the *original* checkout, +// not the worktree. That keeps worktrees siblings of the source rather than +// nesting them, so running the command from within a worktree behaves the same +// as running it from the source. +func SourceCheckout(ctx context.Context, dir string) (string, error) { + // --git-common-dir points at the shared .git directory, which belongs to the + // main checkout even when called from a linked worktree. + out, err := gitOutput(ctx, dir, "rev-parse", "--git-common-dir") + if err != nil { + return "", fmt.Errorf("%w: %s", ErrNotARepository, dir) + } + + gitDir := out + if !filepath.IsAbs(gitDir) { + gitDir = filepath.Join(dir, gitDir) + } + + // The checkout is the parent of its .git directory. + checkout := filepath.Dir(filepath.Clean(gitDir)) + + abs, err := filepath.Abs(checkout) + if err != nil { + return "", fmt.Errorf("failed to resolve checkout path: %w", err) + } + + return abs, nil +} + +// BranchExists reports whether a local branch of the given name exists. +func BranchExists(ctx context.Context, repoDir, branch string) (bool, error) { + ref := "refs/heads/" + branch + + cmd := exec.CommandContext(ctx, "git", "show-ref", "--verify", "--quiet", ref) + cmd.Dir = repoDir + + err := cmd.Run() + if err == nil { + return true, nil + } + + // show-ref exits 1 when the ref is absent, which is not an error here. + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { + return false, nil + } + + return false, fmt.Errorf("failed to check branch %q: %w", branch, err) +} + +// CurrentBranch returns the branch checked out in repoDir. +func CurrentBranch(ctx context.Context, repoDir string) (string, error) { + out, err := gitOutput(ctx, repoDir, "rev-parse", "--abbrev-ref", "HEAD") + if err != nil { + return "", fmt.Errorf("failed to determine current branch: %w", err) + } + + return out, nil +} + +// gitOutput runs git in dir and returns its trimmed standard output. +func gitOutput(ctx context.Context, dir string, args ...string) (string, error) { + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Dir = dir + + out, err := cmd.Output() + if err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return "", ctxErr + } + + return "", err + } + + return strings.TrimSpace(string(out)), nil +} diff --git a/internal/worktree/git_test.go b/internal/worktree/git_test.go new file mode 100644 index 0000000..7bece1c --- /dev/null +++ b/internal/worktree/git_test.go @@ -0,0 +1,159 @@ +package worktree + +import ( + "os" + "os/exec" + "path/filepath" + "testing" +) + +// newTestRepo creates a git repository with one commit and returns its path. +func newTestRepo(t *testing.T) string { + t.Helper() + + dir := t.TempDir() + + run := func(args ...string) { + t.Helper() + + cmd := exec.CommandContext(t.Context(), "git", args...) + cmd.Dir = dir + + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + + run("init", "-q") + run("config", "user.email", "test@example.com") + run("config", "user.name", "Test") + + if err := os.WriteFile(filepath.Join(dir, "file.txt"), []byte("x"), 0o600); err != nil { + t.Fatalf("write file: %v", err) + } + + run("add", "-A") + run("commit", "-qm", "initial") + + return dir +} + +func TestSourceCheckoutFromRepoRoot(t *testing.T) { + repo := newTestRepo(t) + + got, err := SourceCheckout(t.Context(), repo) + if err != nil { + t.Fatalf("SourceCheckout: %v", err) + } + + if !samePath(t, got, repo) { + t.Errorf("SourceCheckout = %q, want %q", got, repo) + } +} + +func TestSourceCheckoutFromSubdirectory(t *testing.T) { + repo := newTestRepo(t) + + sub := filepath.Join(repo, "src", "nested") + if err := os.MkdirAll(sub, 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + + got, err := SourceCheckout(t.Context(), sub) + if err != nil { + t.Fatalf("SourceCheckout: %v", err) + } + + if !samePath(t, got, repo) { + t.Errorf("SourceCheckout = %q, want the repo root %q", got, repo) + } +} + +// TestSourceCheckoutFromWorktreeReturnsMainCheckout is the important case: +// running the command from inside a worktree must anchor new worktrees to the +// original checkout, not nest them inside the current one. +func TestSourceCheckoutFromWorktreeReturnsMainCheckout(t *testing.T) { + repo := newTestRepo(t) + + wt := filepath.Join(t.TempDir(), "linked") + + cmd := exec.CommandContext(t.Context(), "git", "worktree", "add", "-q", wt, "-b", "linked-branch") + cmd.Dir = repo + + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("worktree add: %v\n%s", err, out) + } + + got, err := SourceCheckout(t.Context(), wt) + if err != nil { + t.Fatalf("SourceCheckout: %v", err) + } + + if !samePath(t, got, repo) { + t.Errorf("SourceCheckout from a worktree = %q, want the main checkout %q", got, repo) + } +} + +func TestSourceCheckoutOutsideRepository(t *testing.T) { + if _, err := SourceCheckout(t.Context(), t.TempDir()); err == nil { + t.Fatal("expected an error outside a git repository") + } +} + +func TestBranchExists(t *testing.T) { + repo := newTestRepo(t) + + exists, err := BranchExists(t.Context(), repo, "no-such-branch") + if err != nil { + t.Fatalf("BranchExists: %v", err) + } + + if exists { + t.Error("reported a non-existent branch as existing") + } + + cmd := exec.CommandContext(t.Context(), "git", "branch", "real-branch") + cmd.Dir = repo + + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git branch: %v\n%s", err, out) + } + + exists, err = BranchExists(t.Context(), repo, "real-branch") + if err != nil { + t.Fatalf("BranchExists: %v", err) + } + + if !exists { + t.Error("did not report an existing branch") + } +} + +func TestCurrentBranch(t *testing.T) { + repo := newTestRepo(t) + + got, err := CurrentBranch(t.Context(), repo) + if err != nil { + t.Fatalf("CurrentBranch: %v", err) + } + + if got != "main" && got != "master" { + t.Errorf("CurrentBranch = %q, want main or master", got) + } +} + +func samePath(t *testing.T, a, b string) bool { + t.Helper() + + ra, err := filepath.EvalSymlinks(a) + if err != nil { + ra = a + } + + rb, err := filepath.EvalSymlinks(b) + if err != nil { + rb = b + } + + return filepath.Clean(ra) == filepath.Clean(rb) +} diff --git a/internal/worktree/paths.go b/internal/worktree/paths.go new file mode 100644 index 0000000..1bb607a --- /dev/null +++ b/internal/worktree/paths.go @@ -0,0 +1,67 @@ +// Package worktree manages git worktrees for XenForo development environments. +package worktree + +import ( + "path/filepath" + "regexp" + "strings" +) + +// worktreesSuffix is appended to a checkout's directory name to form the +// directory that holds its worktrees. +const worktreesSuffix = ".worktrees" + +// unsafeChars matches anything not allowed in a worktree directory name. +// Letters, digits, dots, underscores and dashes are kept; everything else +// becomes a separator. +var unsafeChars = regexp.MustCompile(`[^a-zA-Z0-9._-]+`) + +// BranchToDirName converts a branch name into a single, safe path segment. +// +// Slashes become dashes, so dev/24x/feature becomes dev-24x-feature. The result +// is always a single segment: it can never contain a separator or resolve to a +// parent directory, whatever the branch name contains. +// +// Note this is lossy. dev/24x/feature and dev-24x-feature both map to +// dev-24x-feature, so callers must check for an existing directory rather than +// assume the name is unique. See CheckCollision. +func BranchToDirName(branch string) string { + name := unsafeChars.ReplaceAllString(branch, "-") + + // Leading dots would create a hidden directory, and a name of only dots + // would resolve to "." or "..". + name = strings.Trim(name, "-") + name = strings.TrimLeft(name, ".") + name = strings.Trim(name, "-") + + // Collapse runs introduced by the substitutions above. + for strings.Contains(name, "--") { + name = strings.ReplaceAll(name, "--", "-") + } + + if name == "." || name == ".." { + return "" + } + + return name +} + +// WorktreesDir returns the directory holding the worktrees for a checkout. +// +// Worktrees are siblings of the source checkout, so ~/Sites/main yields +// ~/Sites/main.worktrees. This keeps them on the same filesystem as the source, +// which matters for Docker bind mounts, and makes them discoverable without +// knowing an xf-specific convention. +func WorktreesDir(sourcePath string) string { + cleaned := filepath.Clean(sourcePath) + + return cleaned + worktreesSuffix +} + +// ResolvePath returns the worktree path for a branch of the given checkout. +// +// The result depends only on its arguments, so any tool can predict it without +// consulting the registry or git. +func ResolvePath(sourcePath, branch string) string { + return filepath.Join(WorktreesDir(sourcePath), BranchToDirName(branch)) +} diff --git a/internal/worktree/paths_test.go b/internal/worktree/paths_test.go new file mode 100644 index 0000000..91ef6f6 --- /dev/null +++ b/internal/worktree/paths_test.go @@ -0,0 +1,102 @@ +package worktree + +import ( + "path/filepath" + "testing" +) + +func TestBranchToDirName(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + branch string + want string + }{ + {name: "simple", branch: "feature", want: "feature"}, + {name: "slashes become dashes", branch: "dev/24x/feature", want: "dev-24x-feature"}, + {name: "leading slash trimmed", branch: "/leading", want: "leading"}, + {name: "trailing slash trimmed", branch: "trailing/", want: "trailing"}, + {name: "consecutive slashes collapse", branch: "a//b", want: "a-b"}, + {name: "spaces become dashes", branch: "my feature", want: "my-feature"}, + {name: "uppercase preserved", branch: "dev/MyAddon/Fix", want: "dev-MyAddon-Fix"}, + {name: "dots preserved", branch: "release/2.4.0", want: "release-2.4.0"}, + {name: "path traversal neutralised", branch: "../escape", want: "escape"}, + {name: "unsafe characters stripped", branch: "feat:x*y?", want: "feat-x-y"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := BranchToDirName(tt.branch); got != tt.want { + t.Errorf("BranchToDirName(%q) = %q, want %q", tt.branch, got, tt.want) + } + }) + } +} + +// TestBranchToDirNameNeverEscapes is the security-relevant property: whatever +// the branch name, the result must be a single path segment that cannot climb +// out of the worktrees directory. +func TestBranchToDirNameNeverEscapes(t *testing.T) { + t.Parallel() + + for _, branch := range []string{ + "../../etc/passwd", + "..", + ".", + "/absolute/path", + "a/../../b", + "....//", + } { + got := BranchToDirName(branch) + + if got == "" { + continue // rejected outright, which is also safe + } + + if filepath.Base(got) != got { + t.Errorf("BranchToDirName(%q) = %q, which is not a single path segment", branch, got) + } + + if got == ".." || got == "." { + t.Errorf("BranchToDirName(%q) = %q, which escapes or self-references", branch, got) + } + } +} + +func TestWorktreesDir(t *testing.T) { + t.Parallel() + + got := WorktreesDir("/Users/x/Sites/main") + want := filepath.Join("/Users/x/Sites", "main.worktrees") + + if got != want { + t.Errorf("WorktreesDir = %q, want %q", got, want) + } +} + +func TestResolvePath(t *testing.T) { + t.Parallel() + + got := ResolvePath("/Users/x/Sites/main", "dev/24x/feature") + want := filepath.Join("/Users/x/Sites", "main.worktrees", "dev-24x-feature") + + if got != want { + t.Errorf("ResolvePath = %q, want %q", got, want) + } +} + +// TestResolvePathIsDeterministic covers the promise that the path can be +// predicted without consulting any state. +func TestResolvePathIsDeterministic(t *testing.T) { + t.Parallel() + + a := ResolvePath("/Users/x/Sites/main", "dev/24x/feature") + b := ResolvePath("/Users/x/Sites/main/", "dev/24x/feature") + + if a != b { + t.Errorf("a trailing separator changed the result: %q vs %q", a, b) + } +} diff --git a/internal/worktree/registry.go b/internal/worktree/registry.go new file mode 100644 index 0000000..13b062e --- /dev/null +++ b/internal/worktree/registry.go @@ -0,0 +1,209 @@ +package worktree + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "sync" + "time" +) + +// Entry records a worktree created by xf. +type Entry struct { + // SourcePath is the checkout the worktree was created from. + SourcePath string `json:"source_path"` + + // SourceBranch is the branch the source was on at creation time. + SourceBranch string `json:"source_branch"` + + // WorktreePath is the resolved location of the worktree. + WorktreePath string `json:"worktree_path"` + + // Branch is the branch checked out in the worktree. + Branch string `json:"branch"` + + // Instance is the Docker instance name for the worktree. + Instance string `json:"instance"` + + // Cloned records whether the environment was cloned from the source. + Cloned bool `json:"cloned"` + + // CreatedAt is when the worktree was created. + CreatedAt time.Time `json:"created_at"` +} + +// Registry is the on-disk record of worktrees xf has created. +// +// It exists so that worktrees can be listed across projects, which git alone +// cannot do. It is deliberately *not* the source of truth: git and Docker are. +// Worktrees can be removed behind xf's back, so callers must reconcile entries +// against reality rather than trusting the file. A missing or damaged registry +// degrades listing across projects but never blocks an operation. +type Registry struct { + mu sync.Mutex + path string +} + +// NewRegistry opens the registry in the user's configuration directory. +func NewRegistry() (*Registry, error) { + dir, err := os.UserConfigDir() + if err != nil { + return nil, fmt.Errorf("could not determine user config directory: %w", err) + } + + return &Registry{path: filepath.Join(dir, "xf", "worktrees.json")}, nil +} + +// Path returns the registry file location. +func (r *Registry) Path() string { + return r.path +} + +// All returns every recorded entry. +// +// A missing registry returns no entries and no error. +func (r *Registry) All() ([]Entry, error) { + r.mu.Lock() + defer r.mu.Unlock() + + return r.load() +} + +// ForSource returns the entries belonging to a single checkout. +func (r *Registry) ForSource(sourcePath string) ([]Entry, error) { + entries, err := r.All() + if err != nil { + return nil, err + } + + want := filepath.Clean(sourcePath) + + var matched []Entry + + for _, e := range entries { + if filepath.Clean(e.SourcePath) == want { + matched = append(matched, e) + } + } + + return matched, nil +} + +// Add records a worktree, replacing any existing entry for the same path. +func (r *Registry) Add(entry Entry) error { + r.mu.Lock() + defer r.mu.Unlock() + + // A damaged registry must not block recording new work, so parse failures + // are treated as an empty registry and overwritten. + entries, _ := r.load() + + want := filepath.Clean(entry.WorktreePath) + replaced := false + + for i, e := range entries { + if filepath.Clean(e.WorktreePath) == want { + entries[i] = entry + replaced = true + + break + } + } + + if !replaced { + entries = append(entries, entry) + } + + return r.save(entries) +} + +// Remove drops the entry for a worktree path. Removing an absent entry is not +// an error, so cleanup is idempotent. +func (r *Registry) Remove(worktreePath string) error { + r.mu.Lock() + defer r.mu.Unlock() + + entries, err := r.load() + if err != nil { + return err + } + + want := filepath.Clean(worktreePath) + kept := make([]Entry, 0, len(entries)) + + for _, e := range entries { + if filepath.Clean(e.WorktreePath) != want { + kept = append(kept, e) + } + } + + return r.save(kept) +} + +func (r *Registry) load() ([]Entry, error) { + data, err := os.ReadFile(r.path) + if err != nil { + if os.IsNotExist(err) { + return nil, nil + } + + return nil, fmt.Errorf("failed to read worktree registry: %w", err) + } + + var entries []Entry + if err := json.Unmarshal(data, &entries); err != nil { + return nil, fmt.Errorf("failed to parse worktree registry at %s: %w", r.path, err) + } + + return entries, nil +} + +// save writes the registry atomically, so a crash or a concurrent run cannot +// leave a half-written file. +func (r *Registry) save(entries []Entry) error { + if entries == nil { + entries = []Entry{} + } + + data, err := json.MarshalIndent(entries, "", " ") + if err != nil { + return fmt.Errorf("failed to encode worktree registry: %w", err) + } + + dir := filepath.Dir(r.path) + if err := os.MkdirAll(dir, 0o750); err != nil { + return fmt.Errorf("failed to create registry directory: %w", err) + } + + tmp, err := os.CreateTemp(dir, "worktrees-*.json") + if err != nil { + return fmt.Errorf("failed to create temporary registry file: %w", err) + } + + tmpName := tmp.Name() + + defer func() { + _ = os.Remove(tmpName) + }() + + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + + return fmt.Errorf("failed to write worktree registry: %w", err) + } + + if err := tmp.Close(); err != nil { + return fmt.Errorf("failed to close worktree registry: %w", err) + } + + if err := os.Chmod(tmpName, 0o600); err != nil { + return fmt.Errorf("failed to set registry permissions: %w", err) + } + + if err := os.Rename(tmpName, r.path); err != nil { + return fmt.Errorf("failed to replace worktree registry: %w", err) + } + + return nil +} diff --git a/internal/worktree/registry_test.go b/internal/worktree/registry_test.go new file mode 100644 index 0000000..c534001 --- /dev/null +++ b/internal/worktree/registry_test.go @@ -0,0 +1,167 @@ +package worktree + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +func TestRegistryRoundTrip(t *testing.T) { + path := filepath.Join(t.TempDir(), "worktrees.json") + + reg := &Registry{path: path} + + entry := Entry{ + SourcePath: "/Users/x/Sites/main", + SourceBranch: "main", + WorktreePath: "/Users/x/Sites/main.worktrees/dev-24x-feature", + Branch: "dev/24x/feature", + Instance: "dev-24x-feature", + Cloned: true, + CreatedAt: time.Now().UTC().Truncate(time.Second), + } + + if err := reg.Add(entry); err != nil { + t.Fatalf("Add: %v", err) + } + + entries, err := reg.All() + if err != nil { + t.Fatalf("All: %v", err) + } + + if len(entries) != 1 { + t.Fatalf("got %d entries, want 1", len(entries)) + } + + got := entries[0] + if got.Branch != entry.Branch || got.Instance != entry.Instance || !got.Cloned { + t.Errorf("round-trip mismatch: %+v", got) + } +} + +// TestRegistryMissingFileIsNotAnError covers the design rule that the registry +// is never load-bearing: a missing file yields no entries, not a failure. +func TestRegistryMissingFileIsNotAnError(t *testing.T) { + reg := &Registry{path: filepath.Join(t.TempDir(), "absent.json")} + + entries, err := reg.All() + if err != nil { + t.Fatalf("a missing registry must not be an error, got %v", err) + } + + if len(entries) != 0 { + t.Errorf("got %d entries, want 0", len(entries)) + } +} + +// TestRegistryCorruptFileIsNotFatal covers the same rule for damaged content. +func TestRegistryCorruptFileIsNotFatal(t *testing.T) { + path := filepath.Join(t.TempDir(), "corrupt.json") + if err := os.WriteFile(path, []byte("{not json"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + reg := &Registry{path: path} + + if _, err := reg.All(); err == nil { + t.Log("corrupt registry returned no error; acceptable if entries are empty") + } + + // Adding must still succeed, replacing the damaged file. + if err := reg.Add(Entry{Branch: "x", WorktreePath: "/tmp/x"}); err != nil { + t.Fatalf("Add over a corrupt registry: %v", err) + } + + entries, err := reg.All() + if err != nil { + t.Fatalf("All after recovery: %v", err) + } + + if len(entries) != 1 { + t.Errorf("got %d entries, want 1 after recovery", len(entries)) + } +} + +func TestRegistryRemove(t *testing.T) { + reg := &Registry{path: filepath.Join(t.TempDir(), "worktrees.json")} + + for _, p := range []string{"/tmp/a", "/tmp/b"} { + if err := reg.Add(Entry{WorktreePath: p, Branch: filepath.Base(p)}); err != nil { + t.Fatalf("Add: %v", err) + } + } + + if err := reg.Remove("/tmp/a"); err != nil { + t.Fatalf("Remove: %v", err) + } + + entries, err := reg.All() + if err != nil { + t.Fatalf("All: %v", err) + } + + if len(entries) != 1 || entries[0].WorktreePath != "/tmp/b" { + t.Errorf("unexpected entries after removal: %+v", entries) + } +} + +// TestRegistryAddIsIdempotent ensures re-registering the same path updates the +// entry rather than duplicating it. +func TestRegistryAddIsIdempotent(t *testing.T) { + reg := &Registry{path: filepath.Join(t.TempDir(), "worktrees.json")} + + first := Entry{WorktreePath: "/tmp/a", Branch: "old", Instance: "one"} + second := Entry{WorktreePath: "/tmp/a", Branch: "new", Instance: "two"} + + if err := reg.Add(first); err != nil { + t.Fatalf("Add: %v", err) + } + + if err := reg.Add(second); err != nil { + t.Fatalf("Add again: %v", err) + } + + entries, err := reg.All() + if err != nil { + t.Fatalf("All: %v", err) + } + + if len(entries) != 1 { + t.Fatalf("got %d entries, want 1", len(entries)) + } + + if entries[0].Branch != "new" || entries[0].Instance != "two" { + t.Errorf("entry was not updated: %+v", entries[0]) + } +} + +func TestRegistryForSource(t *testing.T) { + reg := &Registry{path: filepath.Join(t.TempDir(), "worktrees.json")} + + add := func(source, branch string) { + t.Helper() + + if err := reg.Add(Entry{ + SourcePath: source, + Branch: branch, + WorktreePath: filepath.Join(source+".worktrees", branch), + }); err != nil { + t.Fatalf("Add: %v", err) + } + } + + add("/Users/x/Sites/main", "one") + add("/Users/x/Sites/main", "two") + add("/Users/x/Sites/other", "three") + + entries, err := reg.ForSource("/Users/x/Sites/main") + if err != nil { + t.Fatalf("ForSource: %v", err) + } + + if len(entries) != 2 { + t.Errorf("got %d entries for the source, want 2", len(entries)) + } +} From a18276d09accb4ca83cbaa9f3843060ae8cc14b0 Mon Sep 17 00:00:00 2001 From: Chris Deeming Date: Tue, 18 Aug 2026 01:26:26 +0100 Subject: [PATCH 03/14] feat(worktree): add creation and removal with safety checks Preflight validates a request before anything is created, so a rejected request leaves no directory, branch or registry entry behind. It catches the collision the lossy branch-to-directory mapping allows: dev/24x/feature and dev-24x-feature both want dev-24x-feature, which git cannot detect because it sees two distinct branches. Remove refuses to discard work. It reports uncommitted changes, including untracked files, and commits that exist on no remote, listing what would be lost; --force overrides. Two subtleties in the unpushed-commit check, both found by testing rather than by reading: - `git log --not --remotes` silently lists nothing without an explicit HEAD, because it has no starting point to walk back from. - "Unpushed" is meaningless without a remote. A repository with no remote would otherwise report every commit as unmergeable and refuse to remove any worktree at all, so the check only runs when a remote exists. Branch deletion after removal is best effort: the worktree is already gone by then, and a branch that will not delete is not worth failing the operation over. --- internal/worktree/create.go | 161 ++++++++++++++++++++++++ internal/worktree/create_test.go | 209 +++++++++++++++++++++++++++++++ internal/worktree/remove.go | 157 +++++++++++++++++++++++ internal/worktree/remove_test.go | 177 ++++++++++++++++++++++++++ 4 files changed, 704 insertions(+) create mode 100644 internal/worktree/create.go create mode 100644 internal/worktree/create_test.go create mode 100644 internal/worktree/remove.go create mode 100644 internal/worktree/remove_test.go diff --git a/internal/worktree/create.go b/internal/worktree/create.go new file mode 100644 index 0000000..fa9f7c9 --- /dev/null +++ b/internal/worktree/create.go @@ -0,0 +1,161 @@ +package worktree + +import ( + "context" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "time" + + "github.com/xenforo-ltd/cli/internal/xf" +) + +var ( + // ErrInvalidBranch indicates a branch name that cannot be used. + ErrInvalidBranch = errors.New("invalid branch name") + + // ErrBranchExists indicates the branch is already present. + ErrBranchExists = errors.New("branch already exists") + + // ErrWorktreeExists indicates the target directory is already in use. + ErrWorktreeExists = errors.New("worktree already exists") + + // ErrNotXenForo indicates the source is not a XenForo installation. + ErrNotXenForo = errors.New("not a XenForo directory") +) + +// Options describes a worktree to create. +type Options struct { + // SourcePath is the checkout to branch from. + SourcePath string + + // Branch is the branch to create in the new worktree. + Branch string + + // Base is the ref to branch from. Defaults to the source's current HEAD. + Base string + + // Instance overrides the derived Docker instance name. + Instance string +} + +// Result describes a created worktree. +type Result struct { + // Path is the worktree location. + Path string + + // Branch is the branch checked out in the worktree. + Branch string + + // SourcePath is the checkout it was created from. + SourcePath string + + // SourceBranch is the branch the source was on at creation time. + SourceBranch string + + // Instance is the Docker instance name for the worktree. + Instance string + + // CreatedAt is when the worktree was created. + CreatedAt time.Time +} + +// Preflight validates a request without changing anything. +// +// It runs before any mutation so that a rejected request leaves no partial +// state behind: no directory, no branch, no registry entry. +func Preflight(ctx context.Context, sourcePath, branch string) error { + if strings.TrimSpace(branch) == "" { + return fmt.Errorf("%w: branch name is empty", ErrInvalidBranch) + } + + dirName := BranchToDirName(branch) + if dirName == "" { + return fmt.Errorf("%w: %q does not yield a usable directory name", ErrInvalidBranch, branch) + } + + xfPath := filepath.Join(sourcePath, "src", "XF.php") + if _, err := os.Stat(xfPath); err != nil { + return fmt.Errorf("%w: src/XF.php not found in %s", ErrNotXenForo, sourcePath) + } + + exists, err := BranchExists(ctx, sourcePath, branch) + if err != nil { + return err + } + + if exists { + return fmt.Errorf("%w: %s", ErrBranchExists, branch) + } + + // The branch-to-directory mapping is lossy, so a different branch may + // already own this directory. Check the path, not just the branch. + target := filepath.Join(WorktreesDir(sourcePath), dirName) + if _, err := os.Stat(target); err == nil { + return fmt.Errorf("%w: %s", ErrWorktreeExists, target) + } + + return nil +} + +// Create makes a worktree on a new branch. +// +// It does not configure Docker or install anything; that is the caller's job. +// Pre-flight runs first, so a failure here leaves the source checkout untouched. +func Create(ctx context.Context, opts Options) (*Result, error) { + source, err := SourceCheckout(ctx, opts.SourcePath) + if err != nil { + return nil, err + } + + if err := Preflight(ctx, source, opts.Branch); err != nil { + return nil, err + } + + sourceBranch, err := CurrentBranch(ctx, source) + if err != nil { + return nil, err + } + + target := ResolvePath(source, opts.Branch) + + if err := os.MkdirAll(filepath.Dir(target), 0o750); err != nil { + return nil, fmt.Errorf("failed to create worktrees directory: %w", err) + } + + args := []string{"worktree", "add", "--quiet", target, "-b", opts.Branch} + if opts.Base != "" { + args = append(args, opts.Base) + } + + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Dir = source + + if out, err := cmd.CombinedOutput(); err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } + + // Leave no empty container directory behind after a failure. + _ = os.Remove(filepath.Dir(target)) + + return nil, fmt.Errorf("failed to create worktree: %w: %s", err, strings.TrimSpace(string(out))) + } + + instance := opts.Instance + if instance == "" { + instance = xf.GenerateInstanceName(BranchToDirName(opts.Branch)) + } + + return &Result{ + Path: target, + Branch: opts.Branch, + SourcePath: source, + SourceBranch: sourceBranch, + Instance: instance, + CreatedAt: time.Now().UTC(), + }, nil +} diff --git a/internal/worktree/create_test.go b/internal/worktree/create_test.go new file mode 100644 index 0000000..8d3ce44 --- /dev/null +++ b/internal/worktree/create_test.go @@ -0,0 +1,209 @@ +package worktree + +import ( + "errors" + "os" + "os/exec" + "path/filepath" + "testing" +) + +// newXenForoRepo creates a git repo that looks like a XenForo checkout. +func newXenForoRepo(t *testing.T) string { + t.Helper() + + dir := newTestRepo(t) + + if err := os.MkdirAll(filepath.Join(dir, "src"), 0o750); err != nil { + t.Fatalf("mkdir src: %v", err) + } + + if err := os.WriteFile(filepath.Join(dir, "src", "XF.php"), []byte(" maxInstanceNameLength { + t.Errorf("instance name %q exceeds %d characters", result.Instance, maxInstanceNameLength) + } +} diff --git a/internal/worktree/remove.go b/internal/worktree/remove.go new file mode 100644 index 0000000..3f606b3 --- /dev/null +++ b/internal/worktree/remove.go @@ -0,0 +1,157 @@ +package worktree + +import ( + "context" + "errors" + "fmt" + "os/exec" + "strings" +) + +var ( + // ErrDirtyWorktree indicates uncommitted changes would be lost. + ErrDirtyWorktree = errors.New("worktree has uncommitted changes") + + // ErrUnmergedCommits indicates commits exist only in this worktree. + ErrUnmergedCommits = errors.New("worktree has commits not present on any remote") +) + +// WorktreeStatus describes the state of a worktree's working tree and branch. +type WorktreeStatus struct { + // Modified lists paths with uncommitted changes, including untracked files. + Modified []string + + // UnmergedCommits lists commits not reachable from any remote branch. + UnmergedCommits []string +} + +// Clean reports whether the worktree holds no work that removal would lose. +func (s WorktreeStatus) Clean() bool { + return len(s.Modified) == 0 && len(s.UnmergedCommits) == 0 +} + +// Status inspects a worktree for work that would be lost by removing it. +func Status(ctx context.Context, worktreePath string) (WorktreeStatus, error) { + var status WorktreeStatus + + // --porcelain includes untracked files, which are easy to forget and just + // as easy to lose. + out, err := gitOutput(ctx, worktreePath, "status", "--porcelain") + if err != nil { + return status, fmt.Errorf("failed to inspect worktree: %w", err) + } + + for _, line := range strings.Split(out, "\n") { + if trimmed := strings.TrimSpace(line); trimmed != "" { + status.Modified = append(status.Modified, trimmed) + } + } + + // "git remote" with no configured remotes exits 0 with empty output, so a + // failure here is a real inspection problem, not the "no remotes" case. + remotes, err := gitOutput(ctx, worktreePath, "remote") + if err != nil { + return status, fmt.Errorf("failed to list remotes: %w", err) + } + + // "Unpushed" is only meaningful when there is somewhere to push to. In a + // repository with no remotes every commit is unreachable from a remote, so + // the check would flag every worktree and be worse than useless. + if strings.TrimSpace(remotes) == "" { + return status, nil + } + + // An unborn branch (no commits yet) has no HEAD to inspect, and "git log" + // on one fails distinctly from a real command failure, so check for that + // case explicitly rather than treating every "log" error as "no commits". + if _, err := gitOutput(ctx, worktreePath, "rev-parse", "--verify", "HEAD"); err != nil { + return status, nil + } + + // Commits reachable from HEAD but from no remote branch exist only here. + // HEAD must be named explicitly: "--not --remotes" alone gives git no + // starting point and silently lists nothing. + commits, err := gitOutput(ctx, worktreePath, "log", "--oneline", "HEAD", "--not", "--remotes") + if err != nil { + return status, fmt.Errorf("failed to inspect commit history: %w", err) + } + + for _, line := range strings.Split(commits, "\n") { + if trimmed := strings.TrimSpace(line); trimmed != "" { + status.UnmergedCommits = append(status.UnmergedCommits, trimmed) + } + } + + return status, nil +} + +// CheckRemovable reports whether a worktree can be removed without losing work. +// +// It is separate from Remove so callers can run the check before destroying +// anything the removal depends on. Tearing down containers and volumes first +// and only then discovering that the worktree is dirty would refuse the +// removal after the data it was protecting had already been deleted. +func CheckRemovable(ctx context.Context, worktreePath string) error { + status, err := Status(ctx, worktreePath) + if err != nil { + return err + } + + if len(status.Modified) > 0 { + return fmt.Errorf("%w:\n %s", ErrDirtyWorktree, strings.Join(status.Modified, "\n ")) + } + + if len(status.UnmergedCommits) > 0 { + return fmt.Errorf("%w:\n %s", ErrUnmergedCommits, strings.Join(status.UnmergedCommits, "\n ")) + } + + return nil +} + +// Remove deletes a worktree and its branch. +// +// Unless force is set, it refuses when the worktree holds uncommitted changes +// or commits that exist nowhere else, listing what would be lost. Removing +// containers and volumes is the caller's responsibility. +func Remove(ctx context.Context, sourcePath, worktreePath string, force bool) error { + if !force { + if err := CheckRemovable(ctx, worktreePath); err != nil { + return err + } + } + + branch, err := CurrentBranch(ctx, worktreePath) + if err != nil { + branch = "" + } + + args := []string{"worktree", "remove", worktreePath} + if force { + args = append(args, "--force") + } + + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Dir = sourcePath + + if out, err := cmd.CombinedOutput(); err != nil { + if ctxErr := ctx.Err(); ctxErr != nil { + return ctxErr + } + + return fmt.Errorf("failed to remove worktree: %w: %s", err, strings.TrimSpace(string(out))) + } + + if branch == "" || branch == "HEAD" { + return nil + } + + // Deleting the branch is best effort: the worktree is already gone, and a + // branch that will not delete is not worth failing the whole operation for. + deleteArgs := []string{"branch", "-D", branch} + + del := exec.CommandContext(ctx, "git", deleteArgs...) + del.Dir = sourcePath + _ = del.Run() + + return nil +} diff --git a/internal/worktree/remove_test.go b/internal/worktree/remove_test.go new file mode 100644 index 0000000..29d6723 --- /dev/null +++ b/internal/worktree/remove_test.go @@ -0,0 +1,177 @@ +package worktree + +import ( + "errors" + "os" + "os/exec" + "path/filepath" + "testing" +) + +// createdWorktree makes a worktree and returns the source and worktree paths. +func createdWorktree(t *testing.T, branch string) (string, string) { + t.Helper() + + repo := newXenForoRepo(t) + + result, err := Create(t.Context(), Options{SourcePath: repo, Branch: branch}) + if err != nil { + t.Fatalf("Create: %v", err) + } + + return repo, result.Path +} + +func TestRemoveDeletesACleanWorktree(t *testing.T) { + repo, wt := createdWorktree(t, "feature") + + if err := Remove(t.Context(), repo, wt, false); err != nil { + t.Fatalf("Remove: %v", err) + } + + if _, err := os.Stat(wt); !os.IsNotExist(err) { + t.Error("worktree directory still exists") + } + + exists, err := BranchExists(t.Context(), repo, "feature") + if err != nil { + t.Fatalf("BranchExists: %v", err) + } + + if exists { + t.Error("branch was left behind") + } +} + +// TestRemoveRefusesUncommittedChanges is the guard against losing work. +func TestRemoveRefusesUncommittedChanges(t *testing.T) { + repo, wt := createdWorktree(t, "feature") + + if err := os.WriteFile(filepath.Join(wt, "new-file.txt"), []byte("work"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + err := Remove(t.Context(), repo, wt, false) + if !errors.Is(err, ErrDirtyWorktree) { + t.Fatalf("expected ErrDirtyWorktree, got %v", err) + } + + if _, statErr := os.Stat(wt); statErr != nil { + t.Error("a refused removal must leave the worktree intact") + } +} + +func TestRemoveForceDiscardsChanges(t *testing.T) { + repo, wt := createdWorktree(t, "feature") + + if err := os.WriteFile(filepath.Join(wt, "new-file.txt"), []byte("work"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + if err := Remove(t.Context(), repo, wt, true); err != nil { + t.Fatalf("forced Remove: %v", err) + } + + if _, err := os.Stat(wt); !os.IsNotExist(err) { + t.Error("forced removal did not delete the worktree") + } +} + +// TestRemoveRefusesUnpushedCommits guards commits that exist nowhere else. +// +// The repository needs a remote for this to be meaningful: with no remote there +// is nowhere to push, so "unpushed" would describe every commit ever made. +func TestRemoveRefusesUnpushedCommits(t *testing.T) { + repo, wt := createdWorktree(t, "feature") + + addRemote(t, repo) + + if err := os.WriteFile(filepath.Join(wt, "committed.txt"), []byte("work"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + for _, args := range [][]string{{"add", "-A"}, {"commit", "-qm", "local work"}} { + cmd := exec.CommandContext(t.Context(), "git", args...) + cmd.Dir = wt + + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + + err := Remove(t.Context(), repo, wt, false) + if !errors.Is(err, ErrUnmergedCommits) { + t.Fatalf("expected ErrUnmergedCommits, got %v", err) + } +} + +func TestRemoveUnknownPath(t *testing.T) { + repo := newXenForoRepo(t) + + err := Remove(t.Context(), repo, filepath.Join(t.TempDir(), "nope"), false) + if err == nil { + t.Fatal("expected an error for an unknown worktree path") + } +} + +func TestStatusReportsCleanliness(t *testing.T) { + _, wt := createdWorktree(t, "feature") + + status, err := Status(t.Context(), wt) + if err != nil { + t.Fatalf("Status: %v", err) + } + + if !status.Clean() { + t.Errorf("a fresh worktree should be clean, got %+v", status) + } + + if err := os.WriteFile(filepath.Join(wt, "dirty.txt"), []byte("x"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + status, err = Status(t.Context(), wt) + if err != nil { + t.Fatalf("Status: %v", err) + } + + if status.Clean() { + t.Error("an untracked file should make the worktree dirty") + } +} + +// TestStatusReturnsErrorOnInspectionFailure guards against treating a broken +// git invocation as "nothing to lose": Status must surface a real command +// failure rather than silently reporting a clean worktree, since Remove would +// otherwise delete the worktree and force-delete its branch unverified. +func TestStatusReturnsErrorOnInspectionFailure(t *testing.T) { + notARepo := t.TempDir() + + if _, err := Status(t.Context(), notARepo); err == nil { + t.Fatal("expected an error when inspecting a path that is not a git repository") + } +} + +// addRemote gives a repository a real remote with the current history, so that +// "not present on any remote" can distinguish new commits from existing ones. +func addRemote(t *testing.T, repo string) { + t.Helper() + + remote := t.TempDir() + + run := func(dir string, args ...string) { + t.Helper() + + cmd := exec.CommandContext(t.Context(), "git", args...) + cmd.Dir = dir + + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + + run(remote, "init", "-q", "--bare") + run(repo, "remote", "add", "origin", remote) + run(repo, "push", "-q", "origin", "HEAD") + run(repo, "fetch", "-q", "origin") +} From c73149ad86f6fd8745e5d5bf7f2c7c472af59ea1 Mon Sep 17 00:00:00 2001 From: Chris Deeming Date: Tue, 18 Aug 2026 01:48:54 +0100 Subject: [PATCH 04/14] feat(init): install Composer dependencies automatically xf init claimed to produce a working installation but did not, for repository checkouts: vendor/ was missing until composer install was run separately. Close that gap in init rather than in each caller. Detection is by the presence of composer.json. Repository checkouts track it, so a fresh clone or worktree has one; release packages ship vendor/ prebuilt and have no manifest, so they are skipped without needing to know which kind of installation this is. The step runs after the containers start, since composer runs inside the xf container, and before xf:install. Placing it inside the existing --skip-up branch means --skip-up skips it structurally: there is no container to run it in, and no separate check is needed to express that. --skip-composer opts out. --- cmd/xf/composerdetect_test.go | 72 +++++++++++++++++++++++++++++++++++ cmd/xf/init.go | 11 +++++- cmd/xf/init_execute.go | 70 ++++++++++++++++++++++++++++++++-- cmd/xf/init_steps_test.go | 68 +++++++++++++++++++++++++++++++++ 4 files changed, 217 insertions(+), 4 deletions(-) create mode 100644 cmd/xf/composerdetect_test.go create mode 100644 cmd/xf/init_steps_test.go diff --git a/cmd/xf/composerdetect_test.go b/cmd/xf/composerdetect_test.go new file mode 100644 index 0000000..9833abb --- /dev/null +++ b/cmd/xf/composerdetect_test.go @@ -0,0 +1,72 @@ +package main + +import ( + "os" + "path/filepath" + "testing" +) + +func TestShouldRunComposer(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + files []string + want bool + }{ + { + name: "composer.json present", + files: []string{"composer.json"}, + want: true, + }, + { + name: "composer.json and lock present", + files: []string{"composer.json", "composer.lock"}, + want: true, + }, + { + name: "no composer files", + files: nil, + want: false, + }, + { + name: "lock without json is not a composer project", + files: []string{"composer.lock"}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + + for _, name := range tt.files { + if err := os.WriteFile(filepath.Join(dir, name), []byte("{}"), 0o600); err != nil { + t.Fatalf("write %s: %v", name, err) + } + } + + if got := shouldRunComposer(dir); got != tt.want { + t.Errorf("shouldRunComposer = %v, want %v", got, tt.want) + } + }) + } +} + +// TestShouldRunComposerIgnoresDirectory guards against a directory named +// composer.json being mistaken for a manifest. +func TestShouldRunComposerIgnoresDirectory(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + + if err := os.MkdirAll(filepath.Join(dir, "composer.json"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + + if shouldRunComposer(dir) { + t.Error("a directory named composer.json must not count as a manifest") + } +} diff --git a/cmd/xf/init.go b/cmd/xf/init.go index 512b48b..f3c6c97 100644 --- a/cmd/xf/init.go +++ b/cmd/xf/init.go @@ -35,7 +35,8 @@ Fresh Install Mode (default): 4. Sets up Docker configuration 5. Configures the .env file 6. Runs 'up' to start the containers - 7. Runs 'xf:install' to complete the installation + 7. Runs 'composer install' if composer.json is present + 8. Runs 'xf:install' to complete the installation Existing Directory Mode (--existing flag): For core developers who already have XenForo source files checked out. @@ -46,6 +47,10 @@ Existing Directory Mode (--existing flag): 3. Configures the .env file 4. Optionally starts containers (with --up flag) + Repository checkouts track composer.json, so dependencies are installed + automatically once the containers are running. Release packages ship + vendor/ prebuilt and have no manifest, so they are skipped. + Examples: # Fresh install (interactive) xf init ./my-project @@ -86,6 +91,7 @@ type InitOptions struct { InstanceName string SkipUp bool SkipInstall bool + SkipComposer bool ExistingOnly bool Contexts []string StartContainers bool @@ -109,6 +115,7 @@ var ( flagInitInstance string flagInitSkipUp bool flagInitSkipInstall bool + flagInitSkipComposer bool flagInitExisting bool flagInitContexts []string flagInitUp bool @@ -127,6 +134,7 @@ func init() { initCmd.Flags().StringVar(&flagInitInstance, "instance", "", "Docker instance name") initCmd.Flags().BoolVar(&flagInitSkipUp, "skip-up", false, "skip starting Docker containers") initCmd.Flags().BoolVar(&flagInitSkipInstall, "skip-install", false, "skip running xf:install") + initCmd.Flags().BoolVar(&flagInitSkipComposer, "skip-composer", false, "skip running composer install") initCmd.Flags().BoolVar(&flagInitExisting, "existing", false, "initialize Docker in an existing XenForo directory (skips download)") initCmd.Flags().StringSliceVar(&flagInitContexts, "contexts", nil, "Docker contexts to enable (e.g., caddy,mysql,development,redis)") initCmd.Flags().BoolVar(&flagInitUp, "up", false, "start containers after initialization (for --existing mode)") @@ -162,6 +170,7 @@ func runInit(cmd *cobra.Command, args []string) error { InstanceName: flagInitInstance, SkipUp: flagInitSkipUp, SkipInstall: flagInitSkipInstall, + SkipComposer: flagInitSkipComposer, ExistingOnly: flagInitExisting, Contexts: flagInitContexts, StartContainers: flagInitUp, diff --git a/cmd/xf/init_execute.go b/cmd/xf/init_execute.go index ee7a1df..452fcb9 100644 --- a/cmd/xf/init_execute.go +++ b/cmd/xf/init_execute.go @@ -42,7 +42,20 @@ func executeInit(ctx context.Context, opts *InitOptions) error { titleMap := getProductTitleMap(ctx, client, opts.LicenseKey) + // A repository checkout is the only source that needs Composer: it tracks + // a composer.json, while release packages ship vendor/ prebuilt and have + // none. That is knowable before the files land, so the total is correct + // from the first step rather than changing halfway through. + // + // --existing installs run from an existing checkout, so the target's own + // composer.json is the answer there. + runComposer := !opts.SkipComposer && shouldRunComposer(opts.TargetPath) + totalSteps := 7 + if runComposer { + totalSteps++ + } + step := 1 ui.Println() @@ -150,6 +163,16 @@ func executeInit(ctx context.Context, opts *InitOptions) error { ui.PrintWarning(fmt.Sprintf("Could not auto-detect site URL, using fallback %s: %v", siteURL, detectedErr)) } + if runComposer { + ui.Println() + ui.PrintStep(step, totalSteps, "Installing Composer dependencies") + step++ + + if err := runComposerInstall(ctx, runner, cfg.Verbose); err != nil { + return err + } + } + ui.Println() ui.PrintStep(step, totalSteps, "Installing XenForo") @@ -173,9 +196,7 @@ func executeInit(ctx context.Context, opts *InitOptions) error { "XF_INSTALL_PASSWORD": opts.AdminPassword, } - installArgs = append(installArgs, "--password=$(printenv XF_INSTALL_PASSWORD)") - shellCmd := shellJoinArgs(append([]string{"php", "cmd.php"}, installArgs...)) - shellInstallArgs := []string{"sh", "-c", shellCmd} + shellInstallArgs := []string{"sh", "-c", installShellCommand(installArgs)} if cfg.Verbose { ui.PrintSubstep("Running XenForo installation...") @@ -650,3 +671,46 @@ func configureEnvironment(opts *InitOptions) error { return nil } + +// shouldRunComposer reports whether a directory is a Composer project. +// +// Repository checkouts track composer.json, so a fresh worktree has one and its +// dependencies must be installed. Release packages ship vendor/ prebuilt and +// have no manifest, so they are skipped automatically. +func shouldRunComposer(targetPath string) bool { + info, err := os.Stat(filepath.Join(targetPath, "composer.json")) + + return err == nil && !info.IsDir() +} + +// runComposerInstall installs Composer dependencies inside the container. +func runComposerInstall(ctx context.Context, runner *dockercompose.Runner, verbose bool) error { + args := []string{"install", "--no-interaction"} + + if verbose { + ui.PrintSubstep("Running composer install...") + + if err := runner.Composer(ctx, args...); err != nil { + return fmt.Errorf("failed to install Composer dependencies: %w", err) + } + + return nil + } + + spinner := ui.NewSpinner("Installing Composer dependencies...") + spinner.Start() + + tracker := newPhaseTrackerWriter(spinner, "Installing Composer dependencies", nil) + + composerArgs := append([]string{"composer"}, args...) + if err := runner.ExecOrRunWithOutput(ctx, "xf", true, tracker, tracker, composerArgs...); err != nil { + spinner.StopWithMessage("error", "Failed to install Composer dependencies") + printHiddenOutputTail("Composer output", tracker.TailLines()) + + return fmt.Errorf("failed to install Composer dependencies: %w", err) + } + + spinner.StopWithMessage("success", "Composer dependencies installed") + + return nil +} diff --git a/cmd/xf/init_steps_test.go b/cmd/xf/init_steps_test.go new file mode 100644 index 0000000..669da2d --- /dev/null +++ b/cmd/xf/init_steps_test.go @@ -0,0 +1,68 @@ +package main + +import ( + "os" + "path/filepath" + "testing" +) + +// The step total and the Composer gate read the same decision, so the printed +// "step N of M" sequence always ends at M. +func TestComposerDecisionDrivesTheStepTotal(t *testing.T) { + cases := []struct { + name string + composerJSON bool + skipComposer bool + wantComposer bool + wantTotalStep int + }{ + {"repository checkout", true, false, true, 8}, + {"release package", false, false, false, 7}, + {"checkout with --skip-composer", true, true, false, 7}, + {"release with --skip-composer", false, true, false, 7}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + + if tc.composerJSON { + path := filepath.Join(dir, "composer.json") + if err := os.WriteFile(path, []byte("{}"), 0o600); err != nil { + t.Fatalf("write composer.json: %v", err) + } + } + + runComposer := !tc.skipComposer && shouldRunComposer(dir) + if runComposer != tc.wantComposer { + t.Errorf("runComposer = %v, want %v", runComposer, tc.wantComposer) + } + + totalSteps := 7 + if runComposer { + totalSteps++ + } + + if totalSteps != tc.wantTotalStep { + t.Errorf("totalSteps = %d, want %d", totalSteps, tc.wantTotalStep) + } + }) + } +} + +func TestShouldRunComposerRequiresARegularFile(t *testing.T) { + dir := t.TempDir() + + if shouldRunComposer(dir) { + t.Error("no composer.json, want false") + } + + // A directory named composer.json is not a manifest. + if err := os.Mkdir(filepath.Join(dir, "composer.json"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + + if shouldRunComposer(dir) { + t.Error("composer.json is a directory, want false") + } +} From fd0dad5d973bba6c028ba3d2cb578043b813319b Mon Sep 17 00:00:00 2001 From: Chris Deeming Date: Tue, 18 Aug 2026 01:51:21 +0100 Subject: [PATCH 05/14] feat(cli): add xf worktree Creates a git worktree on a new branch and sets up its environment, so starting work on a feature is one command rather than six. Setup delegates to init, which already handles Docker configuration, containers, Composer and installation. The command itself only creates the worktree and hands over, so there is one implementation of the setup chain rather than two. Subcommands: create, list, list-all, path, remove and prune. Creation is explicit as `xf worktree create `. An earlier shape accepted a branch on the parent command as a shorthand, but that made a mistyped subcommand indistinguishable from a branch name: `xf worktree lst` silently created a worktree called "lst" instead of reporting the mistake. The parent now dispatches subcommands only and reports unknown ones, while `xf worktree help` still prints help. `path` prints an undecorated path so it can be used directly: cd "$(xf worktree path dev/24x/feature)" It resolves from the branch name alone, so it works whether or not the worktree exists. Listing reconciles the registry against the filesystem and reports entries whose directory has gone as "missing" rather than trusting the file, since worktrees get removed outside xf. `prune` drops them. A registry write failure after a successful creation is reported as a warning, not an error: the worktree exists and is usable, so failing would misrepresent what happened. --- cmd/xf/worktree.go | 455 +++++++++++++++++++++++++++++++++++ cmd/xf/worktree_args_test.go | 76 ++++++ 2 files changed, 531 insertions(+) create mode 100644 cmd/xf/worktree.go create mode 100644 cmd/xf/worktree_args_test.go diff --git a/cmd/xf/worktree.go b/cmd/xf/worktree.go new file mode 100644 index 0000000..d0b5ba5 --- /dev/null +++ b/cmd/xf/worktree.go @@ -0,0 +1,455 @@ +package main + +import ( + "context" + "encoding/json" + "fmt" + "os" + "path/filepath" + "strings" + "time" + + "github.com/spf13/cobra" + + "github.com/xenforo-ltd/cli/internal/ui" + "github.com/xenforo-ltd/cli/internal/worktree" +) + +var worktreeCmd = &cobra.Command{ + Use: "worktree", + Short: "Create and manage development worktrees", + Long: `Create a git worktree with a fully configured XenForo environment. + +A worktree is a second checkout of the same repository on its own branch, with +its own Docker containers and database. It lets you work on a feature without +disturbing your main checkout. + +Worktrees are created alongside the source checkout, so ~/Sites/main gains +~/Sites/main.worktrees/. The path is derived from the branch name and is +always predictable. + +'xf worktree create ' creates the worktree and then initialises the +environment: Docker configuration, containers, Composer dependencies and the +XenForo installation. + +Examples: + # Create a worktree and set up its environment + xf worktree create dev/24x/feature + + # Branch from somewhere other than the current HEAD + xf worktree create dev/24x/feature --base main + + # Create the worktree without setting anything up + xf worktree create dev/24x/feature --no-setup + + # Print the path of an existing worktree + cd "$(xf worktree path dev/24x/feature)"`, + // This command only dispatches subcommands. Taking a branch here too would + // make `xf worktree lst` ambiguous, and cobra would resolve it by silently + // creating a branch called "lst" rather than reporting a mistyped + // subcommand. + Args: func(cmd *cobra.Command, args []string) error { + if len(args) == 0 { + return nil + } + + // `xf worktree help` reads as a request for help. Cobra reserves "help" + // at the root only, so it arrives here as an unknown subcommand. + if args[0] == "help" { + return nil + } + + return fmt.Errorf("unknown command %q for %q: %w", args[0], cmd.CommandPath(), ErrInvalidInput) + }, + RunE: func(cmd *cobra.Command, _ []string) error { + return cmd.Help() + }, +} + +var worktreeCreateCmd = &cobra.Command{ + Use: "create ", + Short: "Create a worktree and set up its environment", + Args: cobra.MaximumNArgs(1), + RunE: runWorktreeCreate, +} + +var worktreeListCmd = &cobra.Command{ + Use: "list", + Short: "List worktrees for this project", + Args: cobra.NoArgs, + RunE: runWorktreeList, +} + +var worktreeListAllCmd = &cobra.Command{ + Use: "list-all", + Short: "List worktrees across all known projects", + Args: cobra.NoArgs, + RunE: runWorktreeListAll, +} + +var worktreePathCmd = &cobra.Command{ + Use: "path ", + Short: "Print the path of a worktree", + Long: `Print the resolved path for a branch's worktree. + +The path is derived from the branch name, so this works whether or not the +worktree exists. Useful for shell and agent use: + + cd "$(xf worktree path dev/24x/feature)"`, + Args: cobra.ExactArgs(1), + RunE: runWorktreePath, +} + +var worktreeRemoveCmd = &cobra.Command{ + Use: "remove ", + Short: "Remove a worktree and its containers", + Long: `Remove a worktree, its branch, and its Docker containers and volumes. + +Refuses when the worktree contains uncommitted changes or commits that exist on +no remote, listing what would be lost. Use --force to remove it anyway.`, + Args: cobra.ExactArgs(1), + RunE: runWorktreeRemove, +} + +var worktreePruneCmd = &cobra.Command{ + Use: "prune", + Short: "Drop registry entries for worktrees that no longer exist", + Args: cobra.NoArgs, + RunE: runWorktreePrune, +} + +var ( + flagWorktreeBase string + flagWorktreeNoSetup bool + flagWorktreeNoUp bool + flagWorktreeInstance string + flagWorktreeJSON bool + flagWorktreeForce bool +) + +func init() { + worktreeCreateCmd.Flags().StringVar(&flagWorktreeBase, "base", "", "ref to branch from (defaults to current HEAD)") + worktreeCreateCmd.Flags().BoolVar(&flagWorktreeNoSetup, "no-setup", false, "create the worktree only, without setting up the environment") + worktreeCreateCmd.Flags().BoolVar(&flagWorktreeNoUp, "no-up", false, "configure the environment but do not start containers") + worktreeCreateCmd.Flags().StringVar(&flagWorktreeInstance, "instance", "", "Docker instance name") + worktreeCreateCmd.Flags().BoolVar(&flagWorktreeJSON, "json", false, "output as JSON") + + worktreeListCmd.Flags().BoolVar(&flagWorktreeJSON, "json", false, "output as JSON") + worktreeListAllCmd.Flags().BoolVar(&flagWorktreeJSON, "json", false, "output as JSON") + worktreeRemoveCmd.Flags().BoolVar(&flagWorktreeForce, "force", false, "remove even if there are uncommitted changes or unpushed commits") + + worktreeCmd.AddCommand(worktreeCreateCmd) + worktreeCmd.AddCommand(worktreeListCmd) + worktreeCmd.AddCommand(worktreeListAllCmd) + worktreeCmd.AddCommand(worktreePathCmd) + worktreeCmd.AddCommand(worktreeRemoveCmd) + worktreeCmd.AddCommand(worktreePruneCmd) + + rootCmd.AddCommand(worktreeCmd) +} + +// worktreeOutput is the machine-readable form of a created worktree. +type worktreeOutput struct { + Path string `json:"path"` + Branch string `json:"branch"` + SourcePath string `json:"source_path"` + SourceBranch string `json:"source_branch"` + Instance string `json:"instance"` + Cloned bool `json:"cloned"` + CreatedAt time.Time `json:"created_at"` +} + +func runWorktreeCreate(cmd *cobra.Command, args []string) error { + branch, err := resolveBranchArg(args) + if err != nil { + return err + } + + cwd, err := os.Getwd() + if err != nil { + return fmt.Errorf("failed to get current directory: %w", err) + } + + result, err := worktree.Create(cmd.Context(), worktree.Options{ + SourcePath: cwd, + Branch: branch, + Base: flagWorktreeBase, + Instance: flagWorktreeInstance, + }) + if err != nil { + return err + } + + entry := worktree.Entry{ + SourcePath: result.SourcePath, + SourceBranch: result.SourceBranch, + WorktreePath: result.Path, + Branch: result.Branch, + Instance: result.Instance, + CreatedAt: result.CreatedAt, + } + + if err := recordWorktree(entry); err != nil { + // The worktree exists and is usable; a registry failure must not + // present itself as a failed creation. + ui.PrintWarning(fmt.Sprintf("Could not record worktree in the registry: %v", err)) + } + + if !flagWorktreeJSON { + ui.PrintSuccess("Created worktree " + result.Path) + ui.PrintKeyValuePadded([]ui.KVPair{ + ui.KV("Branch", result.Branch), + ui.KV("Based on", result.SourceBranch), + ui.KV("Instance", result.Instance), + }) + } + + if !flagWorktreeNoSetup { + if err := setUpWorktree(cmd.Context(), result); err != nil { + return err + } + } + + if flagWorktreeJSON { + return printJSON(worktreeOutput{ + Path: result.Path, + Branch: result.Branch, + SourcePath: result.SourcePath, + SourceBranch: result.SourceBranch, + Instance: result.Instance, + CreatedAt: result.CreatedAt, + }) + } + + return nil +} + +// setUpWorktree initialises the environment by delegating to init, which +// already handles Docker configuration, containers, Composer and installation. +func setUpWorktree(ctx context.Context, result *worktree.Result) error { + opts := &InitOptions{ + TargetPath: result.Path, + InstanceName: result.Instance, + ExistingOnly: true, + SkipUp: flagWorktreeNoUp, + StartContainers: !flagWorktreeNoUp, + EnvResolved: map[string]string{}, + EnvSources: map[string]string{}, + ProductOverrides: map[string]int{}, + ProductTitleMap: map[string]string{}, + } + + if err := initExisting(ctx, opts); err != nil { + return fmt.Errorf("worktree created at %s, but setting up its environment failed: %w", result.Path, err) + } + + return nil +} + +func runWorktreePath(cmd *cobra.Command, args []string) error { + cwd, err := os.Getwd() + if err != nil { + return fmt.Errorf("failed to get current directory: %w", err) + } + + source, err := worktree.SourceCheckout(cmd.Context(), cwd) + if err != nil { + return err + } + + // Printed bare, with no decoration, so it can be used directly in a shell. + fmt.Println(worktree.ResolvePath(source, args[0])) + + return nil +} + +func runWorktreeList(cmd *cobra.Command, args []string) error { + cwd, err := os.Getwd() + if err != nil { + return fmt.Errorf("failed to get current directory: %w", err) + } + + source, err := worktree.SourceCheckout(cmd.Context(), cwd) + if err != nil { + return err + } + + registry, err := worktree.NewRegistry() + if err != nil { + return err + } + + entries, err := registry.ForSource(source) + if err != nil { + return err + } + + return printWorktrees(entries) +} + +func runWorktreeListAll(cmd *cobra.Command, args []string) error { + registry, err := worktree.NewRegistry() + if err != nil { + return err + } + + entries, err := registry.All() + if err != nil { + return err + } + + return printWorktrees(entries) +} + +func runWorktreeRemove(cmd *cobra.Command, args []string) error { + cwd, err := os.Getwd() + if err != nil { + return fmt.Errorf("failed to get current directory: %w", err) + } + + source, err := worktree.SourceCheckout(cmd.Context(), cwd) + if err != nil { + return err + } + + target := worktree.ResolvePath(source, args[0]) + + if err := worktree.Remove(cmd.Context(), source, target, flagWorktreeForce); err != nil { + return err + } + + if registry, regErr := worktree.NewRegistry(); regErr == nil { + if err := registry.Remove(target); err != nil { + ui.PrintWarning(fmt.Sprintf("Could not update the worktree registry: %v", err)) + } + } + + ui.PrintSuccess("Removed worktree " + target) + + return nil +} + +func runWorktreePrune(cmd *cobra.Command, args []string) error { + registry, err := worktree.NewRegistry() + if err != nil { + return err + } + + entries, err := registry.All() + if err != nil { + return err + } + + pruned := 0 + + for _, entry := range entries { + if _, statErr := os.Stat(entry.WorktreePath); os.IsNotExist(statErr) { + if err := registry.Remove(entry.WorktreePath); err != nil { + return err + } + + pruned++ + } + } + + if pruned == 0 { + ui.PrintInfo("No stale worktree entries found.") + + return nil + } + + ui.PrintSuccess(fmt.Sprintf("Pruned %d stale worktree %s.", pruned, plural(pruned, "entry", "entries"))) + + return nil +} + +// printWorktrees renders entries, reconciling them against the filesystem. +// +// The registry is a record, not the source of truth: worktrees get removed +// outside xf, so entries are checked rather than trusted. +func printWorktrees(entries []worktree.Entry) error { + if flagWorktreeJSON { + return printJSON(entries) + } + + if len(entries) == 0 { + ui.PrintInfo("No worktrees found.") + + return nil + } + + headers := []string{"BRANCH", "PATH", "INSTANCE", "STATE"} + rows := make([][]string, 0, len(entries)) + + for _, entry := range entries { + state := "ok" + if _, err := os.Stat(entry.WorktreePath); os.IsNotExist(err) { + state = "missing" + } + + rows = append(rows, []string{ + entry.Branch, + shortenPath(entry.WorktreePath), + entry.Instance, + state, + }) + } + + ui.Println(ui.NewTable(headers, rows)) + + return nil +} + +func recordWorktree(entry worktree.Entry) error { + registry, err := worktree.NewRegistry() + if err != nil { + return err + } + + return registry.Add(entry) +} + +func printJSON(v any) error { + data, err := json.MarshalIndent(v, "", " ") + if err != nil { + return fmt.Errorf("failed to encode output: %w", err) + } + + ui.Println(string(data)) + + return nil +} + +// resolveBranchArg returns the branch to create. +func resolveBranchArg(args []string) (string, error) { + if len(args) == 0 || strings.TrimSpace(args[0]) == "" { + return "", fmt.Errorf( + "a branch name is required, for example %q: %w", + "xf worktree create dev/24x/feature", ErrInvalidInput, + ) + } + + return args[0], nil +} + +// shortenPath replaces the home directory with ~ for display. +func shortenPath(path string) string { + home, err := os.UserHomeDir() + if err != nil { + return path + } + + if rel, err := filepath.Rel(home, path); err == nil && !strings.HasPrefix(rel, "..") { + return filepath.Join("~", rel) + } + + return path +} + +func plural(n int, singular, pluralForm string) string { + if n == 1 { + return singular + } + + return pluralForm +} diff --git a/cmd/xf/worktree_args_test.go b/cmd/xf/worktree_args_test.go new file mode 100644 index 0000000..67aae19 --- /dev/null +++ b/cmd/xf/worktree_args_test.go @@ -0,0 +1,76 @@ +package main + +import ( + "bytes" + "context" + "errors" + "strings" + "testing" +) + +// TestWorktreeParentTakesNoArguments is the guard against the original footgun: +// `xf worktree lst` silently created a branch called "lst" instead of reporting +// a mistyped subcommand. The parent dispatches subcommands only, so an +// unrecognised name is now an error rather than a new worktree. +func TestWorktreeParentTakesNoArguments(t *testing.T) { + configureErrorHandling(rootCmd) + + var out bytes.Buffer + + rootCmd.SetOut(&out) + rootCmd.SetErr(&out) + rootCmd.SetArgs([]string{"worktree", "lst"}) + + t.Cleanup(func() { + rootCmd.SetArgs(nil) + rootCmd.SetOut(nil) + rootCmd.SetErr(nil) + }) + + err := rootCmd.ExecuteContext(context.Background()) + if err == nil { + t.Fatal("expected an unrecognised subcommand to be rejected") + } + + if !strings.Contains(err.Error(), "lst") { + t.Errorf("error %q does not name the unrecognised argument", err) + } +} + +// TestWorktreeCreateAcceptsAnyBranchName confirms the explicit form removes the +// ambiguity: once "create" is given, a branch may be named anything, including +// something that matches a subcommand. +func TestWorktreeCreateAcceptsAnyBranchName(t *testing.T) { + t.Parallel() + + for _, name := range []string{ + "dev/24x/feature", + "feature", + "list", + "help", + "remove", + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + got, err := resolveBranchArg([]string{name}) + if err != nil { + t.Errorf("resolveBranchArg(%q) returned %v, want it accepted", name, err) + } + + if got != name { + t.Errorf("resolveBranchArg(%q) = %q", name, got) + } + }) + } +} + +func TestWorktreeCreateRequiresABranch(t *testing.T) { + t.Parallel() + + for _, args := range [][]string{nil, {}, {""}, {" "}} { + if _, err := resolveBranchArg(args); !errors.Is(err, ErrInvalidInput) { + t.Errorf("resolveBranchArg(%v) = %v, want a rejection", args, err) + } + } +} From fe74651ac829ceca0300babb11a7468ad29727cf Mon Sep 17 00:00:00 2001 From: Chris Deeming Date: Tue, 18 Aug 2026 02:15:59 +0100 Subject: [PATCH 06/14] fix(init): complete the setup chain for existing directories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit xf init --existing stopped after starting containers: it never ran composer install or xf:install, so the result was a running environment with no dependencies and no forum. xf worktree inherited that gap and produced worktrees that were not usable. The composer step added in 9aecaa3 went into executeInit, which handles fresh installs. --existing takes a separate path through initExisting that shares none of it, so the step never ran for repository checkouts — the exact case it was written for. Run both steps in initExisting, after the containers start, since each executes inside the xf container. Installation is skipped when no admin user is set, so `xf init --existing` on its own keeps its current behaviour and only callers that supply credentials get an install. xf worktree supplies them, defaulting to admin/password with the branch name as the site title. A worktree is a disposable development environment, so fixed credentials are preferable to prompting: one command produces a forum you can log into, and the login is reported in the output. --- cmd/xf/init.go | 130 +++++++++++++++++++++++++++++++++++ cmd/xf/init_helpers.go | 44 ++++++++++-- cmd/xf/install_shell_test.go | 118 +++++++++++++++++++++++++++++++ cmd/xf/worktree.go | 51 ++++++++++++-- 4 files changed, 332 insertions(+), 11 deletions(-) create mode 100644 cmd/xf/install_shell_test.go diff --git a/cmd/xf/init.go b/cmd/xf/init.go index f3c6c97..c90001c 100644 --- a/cmd/xf/init.go +++ b/cmd/xf/init.go @@ -241,6 +241,36 @@ func detectXenForo(path string) (bool, error) { return false, fmt.Errorf("failed to check XenForo path: %w", err) } +// validateAdminDetails reports whether the installer has everything it needs. +// +// installExistingXenForo passes these straight to xf:install, so a missing +// value would install a broken administrator or an empty board title instead +// of failing. +func validateAdminDetails(opts *InitOptions) error { + var missing []string + + if opts.AdminUser == "" { + missing = append(missing, "--admin-user") + } + + if opts.AdminPassword == "" { + missing = append(missing, "--admin-password") + } + + if opts.AdminEmail == "" { + missing = append(missing, "--admin-email") + } + + if len(missing) > 0 { + return newUsageError(fmt.Errorf( + "missing required flags for the XenForo installation: %s: %w", + strings.Join(missing, ", "), ErrInvalidInput, + )) + } + + return nil +} + func initExisting(ctx context.Context, opts *InitOptions) error { ui.Println(ui.Bold.Render("Initializing Docker environment in existing XenForo directory...")) ui.Println() @@ -260,6 +290,11 @@ func initExisting(ctx context.Context, opts *InitOptions) error { ui.PrintSuccess("Docker Compose is available") ui.Println() + cfg, err := config.Load() + if err != nil { + return fmt.Errorf("failed to load configuration: %w", err) + } + step := 1 totalSteps := 3 @@ -288,6 +323,11 @@ func initExisting(ctx context.Context, opts *InitOptions) error { ui.PrintStep(step, totalSteps, "Starting environment") + // Detection can fail or return nothing, and installing --url= empty would + // leave the board with no address at all, so the predictable instance URL + // is the starting point. + siteURL := fallbackBoardURL(opts.InstanceName) + if opts.StartContainers { runner, err := dockercompose.NewRunner(xfDir) if err != nil { @@ -300,8 +340,43 @@ func initExisting(ctx context.Context, opts *InitOptions) error { url, err := runner.GetURL(ctx) if err == nil && url != "" { + siteURL = url + ui.PrintDetail("Site: " + url) } + + // Composer and the installer both run inside the container, so they can + // only follow a successful start. + if shouldRunComposer(xfDir) && !opts.SkipComposer { + ui.Println() + + if err := runComposerInstall(ctx, runner, cfg.Verbose); err != nil { + return err + } + } + + if !opts.SkipInstall && opts.AdminUser != "" { + // The installer receives these verbatim, so an empty value becomes + // an empty board title or an unusable administrator rather than a + // reported error. + if opts.SiteTitle == "" { + opts.SiteTitle = opts.EnvResolved["XF_TITLE"] + } + + if opts.SiteTitle == "" { + opts.SiteTitle = fmt.Sprintf("XenForo [%s]", opts.InstanceName) + } + + if err := validateAdminDetails(opts); err != nil { + return err + } + + ui.Println() + + if err := installExistingXenForo(ctx, runner, opts, siteURL, cfg.Verbose); err != nil { + return err + } + } } else { ui.PrintDetail("Skipped (use --up flag to start containers)") } @@ -585,3 +660,58 @@ func runInteractiveSetup(ctx context.Context, opts *InitOptions) error { return nil } + +// installExistingXenForo runs xf:install in an already-configured environment. +// +// The password is passed through the environment rather than the command line, +// so it does not appear in the container's process list. +func installExistingXenForo( + ctx context.Context, + runner *dockercompose.Runner, + opts *InitOptions, + siteURL string, + verbose bool, +) error { + if err := runner.WaitForDatabase(ctx, 2*time.Second); err != nil { + return fmt.Errorf("failed waiting for database to become ready: %w", err) + } + + installArgs := []string{ + "xf:install", + "--no-interaction", + "--clear", + "--user=" + opts.AdminUser, + "--email=" + opts.AdminEmail, + "--title=" + opts.SiteTitle, + "--url=" + siteURL, + } + + installEnv := map[string]string{"XF_INSTALL_PASSWORD": opts.AdminPassword} + shellInstallArgs := []string{"sh", "-c", installShellCommand(installArgs)} + + if verbose { + ui.PrintSubstep("Running XenForo installation...") + + if err := runner.ExecOrRunWithEnv(ctx, "xf", true, installEnv, shellInstallArgs...); err != nil { + return fmt.Errorf("failed to install XenForo: %w", err) + } + + return nil + } + + spinner := ui.NewSpinner("Installing XenForo...") + spinner.Start() + + tracker := newPhaseTrackerWriter(spinner, "Installing XenForo", installPhaseRules()) + + if err := runner.ExecOrRunWithEnvAndOutput(ctx, "xf", true, installEnv, tracker, tracker, shellInstallArgs...); err != nil { + spinner.Stop() + printHiddenOutputTail("Installer output", tracker.TailLines()) + + return fmt.Errorf("failed to install XenForo: %w", err) + } + + spinner.StopWithMessage("success", "XenForo installed") + + return nil +} diff --git a/cmd/xf/init_helpers.go b/cmd/xf/init_helpers.go index 11b7c0b..bbdd878 100644 --- a/cmd/xf/init_helpers.go +++ b/cmd/xf/init_helpers.go @@ -238,15 +238,49 @@ func chooseBoardURL(instanceName, detectedURL string, detectedErr error) (string return detectedURL, true } +// installShellCommand builds the shell command that runs xf:install. +// +// Every argument is shell-quoted, so installer values such as the site title +// cannot inject shell syntax. The password substitution is quoted too, so a +// password containing spaces or glob characters reaches the installer +// verbatim. +// +// The password is passed through the environment and expanded by sh rather +// than being interpolated here. That keeps it out of xf's own argv, out of the +// docker compose invocation, and out of anything that logs either of those. +// +// It does not keep it out of the php process's argv inside the container: +// XF\Cli\Command\Install accepts the administrator password only via +// --password or an interactive hidden prompt, and --no-interaction rules the +// prompt out. So for the lifetime of the install, the password is visible to +// anything that can list processes in that container. The container is a +// single-tenant development environment created by this tool, so that exposure +// is accepted; it should be revisited if XenForo ever accepts the password on +// stdin or from an environment variable of its own. +func installShellCommand(installArgs []string) string { + command := shellJoinArgs(append([]string{"php", "cmd.php"}, installArgs...)) + + // Expanded directly rather than through $(printenv ...): command + // substitution strips trailing newlines, so a password ending in one would + // reach the installer altered. + return command + ` --password="$XF_INSTALL_PASSWORD"` +} + func shellJoinArgs(args []string) string { parts := make([]string, len(args)) for i, arg := range args { - if strings.ContainsAny(arg, " \t\"\\") && !strings.Contains(arg, "$(") { - parts[i] = "'" + strings.ReplaceAll(arg, "'", "'\"'\"'") + "'" - } else { - parts[i] = arg - } + parts[i] = shellQuote(arg) } return strings.Join(parts, " ") } + +// shellQuote renders a string as a single-quoted POSIX shell word. +// +// Every argument is quoted unconditionally. Quoting only those that look +// dangerous is how injection gets through: a value such as +// `--title=x;rm -rf /` contains no spaces or quotes, so a +// looks-dangerous test passes it to the shell verbatim. +func shellQuote(value string) string { + return "'" + strings.ReplaceAll(value, "'", `'"'"'`) + "'" +} diff --git a/cmd/xf/install_shell_test.go b/cmd/xf/install_shell_test.go new file mode 100644 index 0000000..cb297b9 --- /dev/null +++ b/cmd/xf/install_shell_test.go @@ -0,0 +1,118 @@ +package main + +import ( + "context" + "os/exec" + "strings" + "testing" +) + +func TestShellQuoteNeutralisesShellSyntax(t *testing.T) { + cases := []struct { + name string + in string + }{ + {"command separator", "x; touch /tmp/pwned"}, + {"command substitution", "x$(touch /tmp/pwned)"}, + {"backticks", "x`touch /tmp/pwned`"}, + {"pipe", "x | touch /tmp/pwned"}, + {"glob", "*"}, + {"single quote", "it's"}, + {"newline", "x\ntouch /tmp/pwned"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // Round-trip through a real shell: echo must reproduce the input + // exactly, which is only true if nothing was interpreted. + script := "printf %s " + shellQuote(tc.in) + + out, err := exec.CommandContext(context.Background(), "sh", "-c", script).Output() + if err != nil { + t.Fatalf("sh -c failed: %v", err) + } + + if string(out) != tc.in { + t.Errorf("shell interpreted the value: got %q, want %q", string(out), tc.in) + } + }) + } +} + +func TestInstallShellCommandQuotesInstallerValues(t *testing.T) { + command := installShellCommand([]string{ + "xf:install", + "--title=Chris' Forum; touch /tmp/pwned", + }) + + // The dangerous value must be quoted, so the separator cannot terminate + // the installer command. + if strings.Contains(command, "; touch /tmp/pwned'") == false { + t.Errorf("value was not quoted as a single word: %s", command) + } + + if strings.HasSuffix(command, "touch /tmp/pwned") { + t.Errorf("command ends with an unquoted injection: %s", command) + } +} + +func TestInstallShellCommandKeepsThePasswordOutOfArgv(t *testing.T) { + command := installShellCommand([]string{"xf:install"}) + + // The password must be read from the environment at run time, and the + // substitution must be quoted so spaces and globs survive intact. + if !strings.Contains(command, `--password="$XF_INSTALL_PASSWORD"`) { + t.Errorf("password is not read from the environment: %s", command) + } +} + +func TestInstallShellCommandPassesAwkwardPasswordsVerbatim(t *testing.T) { + command := installShellCommand([]string{"xf:install"}) + + // Run the built command with a stand-in for php so the installer's view + // of the password can be observed. printf %s\n prints each argument on + // its own line, so a split password would show up as extra lines. + script := strings.Replace(command, `'php' 'cmd.php'`, `printf '%s\n'`, 1) + if script == command { + t.Fatalf("could not substitute the interpreter in %q", command) + } + + cmd := exec.CommandContext(context.Background(), "sh", "-c", script) + cmd.Env = append(cmd.Environ(), "XF_INSTALL_PASSWORD=a b * c'd") + + out, err := cmd.Output() + if err != nil { + t.Fatalf("sh -c failed: %v", err) + } + + lines := strings.Split(strings.TrimRight(string(out), "\n"), "\n") + + last := lines[len(lines)-1] + if last != `--password=a b * c'd` { + t.Errorf("password reached the installer as %q", last) + } +} + +// Command substitution strips trailing newlines, so the password must be +// expanded directly: a password ending in one would otherwise reach the +// installer altered. +func TestInstallShellCommandPreservesTrailingNewlinesInThePassword(t *testing.T) { + command := installShellCommand([]string{"xf:install"}) + + script := strings.Replace(command, `'php' 'cmd.php'`, `printf '%s'`, 1) + if script == command { + t.Fatalf("could not substitute the interpreter in %q", command) + } + + cmd := exec.CommandContext(context.Background(), "sh", "-c", script) + cmd.Env = append(cmd.Environ(), "XF_INSTALL_PASSWORD=secret\n") + + out, err := cmd.Output() + if err != nil { + t.Fatalf("sh -c failed: %v", err) + } + + if !strings.HasSuffix(string(out), "--password=secret\n") { + t.Errorf("trailing newline lost: output ended %q", string(out)) + } +} diff --git a/cmd/xf/worktree.go b/cmd/xf/worktree.go index d0b5ba5..fa735a3 100644 --- a/cmd/xf/worktree.go +++ b/cmd/xf/worktree.go @@ -118,13 +118,24 @@ var worktreePruneCmd = &cobra.Command{ RunE: runWorktreePrune, } +// Defaults for the throwaway installation a worktree gets. +const ( + defaultWorktreeAdminUser = "admin" + defaultWorktreeAdminPassword = "password" + defaultWorktreeAdminEmail = "admin@example.com" +) + var ( - flagWorktreeBase string - flagWorktreeNoSetup bool - flagWorktreeNoUp bool - flagWorktreeInstance string - flagWorktreeJSON bool - flagWorktreeForce bool + flagWorktreeBase string + flagWorktreeAdminUser string + flagWorktreeAdminPassword string + flagWorktreeAdminEmail string + flagWorktreeTitle string + flagWorktreeNoSetup bool + flagWorktreeNoUp bool + flagWorktreeInstance string + flagWorktreeJSON bool + flagWorktreeForce bool ) func init() { @@ -133,6 +144,10 @@ func init() { worktreeCreateCmd.Flags().BoolVar(&flagWorktreeNoUp, "no-up", false, "configure the environment but do not start containers") worktreeCreateCmd.Flags().StringVar(&flagWorktreeInstance, "instance", "", "Docker instance name") worktreeCreateCmd.Flags().BoolVar(&flagWorktreeJSON, "json", false, "output as JSON") + worktreeCreateCmd.Flags().StringVar(&flagWorktreeAdminUser, "admin-user", "", "admin username (default \"admin\")") + worktreeCreateCmd.Flags().StringVar(&flagWorktreeAdminPassword, "admin-password", "", "admin password (default \"password\")") + worktreeCreateCmd.Flags().StringVar(&flagWorktreeAdminEmail, "admin-email", "", "admin email (default \"admin@example.com\")") + worktreeCreateCmd.Flags().StringVar(&flagWorktreeTitle, "title", "", "site title (defaults to the branch name)") worktreeListCmd.Flags().BoolVar(&flagWorktreeJSON, "json", false, "output as JSON") worktreeListAllCmd.Flags().BoolVar(&flagWorktreeJSON, "json", false, "output as JSON") @@ -208,6 +223,14 @@ func runWorktreeCreate(cmd *cobra.Command, args []string) error { if err := setUpWorktree(cmd.Context(), result); err != nil { return err } + + if !flagWorktreeJSON && !flagWorktreeNoUp { + ui.Println() + ui.PrintKeyValuePadded([]ui.KVPair{ + ui.KV("Admin user", defaultString(flagWorktreeAdminUser, defaultWorktreeAdminUser)), + ui.KV("Admin password", defaultString(flagWorktreeAdminPassword, defaultWorktreeAdminPassword)), + }) + } } if flagWorktreeJSON { @@ -227,12 +250,19 @@ func runWorktreeCreate(cmd *cobra.Command, args []string) error { // setUpWorktree initialises the environment by delegating to init, which // already handles Docker configuration, containers, Composer and installation. func setUpWorktree(ctx context.Context, result *worktree.Result) error { + // A worktree is a disposable development environment, so it installs with + // fixed credentials rather than prompting. Knowing the login without being + // asked is the point: one command produces a usable forum. opts := &InitOptions{ TargetPath: result.Path, InstanceName: result.Instance, ExistingOnly: true, SkipUp: flagWorktreeNoUp, StartContainers: !flagWorktreeNoUp, + AdminUser: defaultString(flagWorktreeAdminUser, defaultWorktreeAdminUser), + AdminPassword: defaultString(flagWorktreeAdminPassword, defaultWorktreeAdminPassword), + AdminEmail: defaultString(flagWorktreeAdminEmail, defaultWorktreeAdminEmail), + SiteTitle: defaultString(flagWorktreeTitle, result.Branch), EnvResolved: map[string]string{}, EnvSources: map[string]string{}, ProductOverrides: map[string]int{}, @@ -453,3 +483,12 @@ func plural(n int, singular, pluralForm string) string { return pluralForm } + +// defaultString returns value, or fallback when value is empty. +func defaultString(value, fallback string) string { + if value != "" { + return value + } + + return fallback +} From ee201a6664d3f9d980da844411fbad87bb71f100 Mon Sep 17 00:00:00 2001 From: Chris Deeming Date: Tue, 18 Aug 2026 02:20:03 +0100 Subject: [PATCH 07/14] fix(worktree): remove containers and volumes with the worktree xf worktree remove deleted the directory and branch but left the Docker environment running, so every discarded feature branch leaked a full set of volumes. A removed worktree left its database behind with no way to find it again, since the compose configuration that named it had gone. Add Destroy, which brings the environment down with --volumes and --remove-orphans. Down keeps its current behaviour: stopping an environment you intend to restart must not delete its data. Teardown runs before the directory is removed, because compose reads compose.yaml from the worktree to know what it owns. Removing the files first would strand the containers and volumes permanently. A worktree created with --no-setup has no compose configuration, which is not an error: there is nothing to tear down. --keep-containers opts out for the rare case where the environment should outlive the checkout. --- cmd/xf/worktree.go | 65 +++++++++++++++++++++---- internal/dockercompose/runner.go | 28 ++++++++++- internal/dockercompose/teardown_test.go | 49 +++++++++++++++++++ 3 files changed, 130 insertions(+), 12 deletions(-) create mode 100644 internal/dockercompose/teardown_test.go diff --git a/cmd/xf/worktree.go b/cmd/xf/worktree.go index fa735a3..4f7bf0c 100644 --- a/cmd/xf/worktree.go +++ b/cmd/xf/worktree.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "os" "path/filepath" @@ -11,6 +12,7 @@ import ( "github.com/spf13/cobra" + "github.com/xenforo-ltd/cli/internal/dockercompose" "github.com/xenforo-ltd/cli/internal/ui" "github.com/xenforo-ltd/cli/internal/worktree" ) @@ -126,16 +128,17 @@ const ( ) var ( - flagWorktreeBase string - flagWorktreeAdminUser string - flagWorktreeAdminPassword string - flagWorktreeAdminEmail string - flagWorktreeTitle string - flagWorktreeNoSetup bool - flagWorktreeNoUp bool - flagWorktreeInstance string - flagWorktreeJSON bool - flagWorktreeForce bool + flagWorktreeBase string + flagWorktreeAdminUser string + flagWorktreeAdminPassword string + flagWorktreeAdminEmail string + flagWorktreeTitle string + flagWorktreeNoSetup bool + flagWorktreeNoUp bool + flagWorktreeInstance string + flagWorktreeJSON bool + flagWorktreeForce bool + flagWorktreeKeepContainers bool ) func init() { @@ -152,6 +155,7 @@ func init() { worktreeListCmd.Flags().BoolVar(&flagWorktreeJSON, "json", false, "output as JSON") worktreeListAllCmd.Flags().BoolVar(&flagWorktreeJSON, "json", false, "output as JSON") worktreeRemoveCmd.Flags().BoolVar(&flagWorktreeForce, "force", false, "remove even if there are uncommitted changes or unpushed commits") + worktreeRemoveCmd.Flags().BoolVar(&flagWorktreeKeepContainers, "keep-containers", false, "leave the Docker containers and volumes in place") worktreeCmd.AddCommand(worktreeCreateCmd) worktreeCmd.AddCommand(worktreeListCmd) @@ -344,6 +348,15 @@ func runWorktreeRemove(cmd *cobra.Command, args []string) error { target := worktree.ResolvePath(source, args[0]) + // Containers must be torn down before the directory goes: compose reads + // compose.yaml from the worktree to know what it owns, so removing the + // files first would strand the containers and volumes. + if !flagWorktreeKeepContainers { + if err := destroyWorktreeEnvironment(cmd.Context(), target); err != nil { + return err + } + } + if err := worktree.Remove(cmd.Context(), source, target, flagWorktreeForce); err != nil { return err } @@ -492,3 +505,35 @@ func defaultString(value, fallback string) string { return fallback } + +// destroyWorktreeEnvironment removes a worktree's containers and volumes. +// +// A worktree that was never set up has no compose configuration, which is not +// an error: there is simply nothing to tear down. +func destroyWorktreeEnvironment(ctx context.Context, worktreePath string) error { + runner, err := dockercompose.NewRunner(worktreePath) + if err != nil { + if errors.Is(err, dockercompose.ErrEnvNotInitialized) { + return nil + } + + // The directory may already be gone, or never have been a checkout. + // Removing the worktree is still worthwhile, so this is not fatal. + ui.PrintWarning(fmt.Sprintf("Could not inspect the environment to remove it: %v", err)) + + return nil + } + + spinner := ui.NewSpinner("Removing containers and volumes...") + spinner.Start() + + if err := runner.Destroy(ctx); err != nil { + spinner.StopWithMessage("error", "Failed to remove containers") + + return fmt.Errorf("failed to remove the worktree environment: %w", err) + } + + spinner.StopWithMessage("success", "Containers and volumes removed") + + return nil +} diff --git a/internal/dockercompose/runner.go b/internal/dockercompose/runner.go index 6462413..462f5b3 100644 --- a/internal/dockercompose/runner.go +++ b/internal/dockercompose/runner.go @@ -120,14 +120,38 @@ func (r *Runner) UpWithOutput(ctx context.Context, detach bool, stdout, stderr i return r.runDockerCommandWithOutput(ctx, stdout, stderr, args...) } -// Down stops and removes the Docker containers. +// Down stops and removes the Docker containers, leaving volumes intact so the +// environment can be started again with its data. func (r *Runner) Down(ctx context.Context) error { args := r.buildComposeArgs() - args = append(args, "down") + args = append(args, downArgs(false)...) return r.runDockerCommand(ctx, args...) } +// Destroy stops the environment and removes its volumes. +// +// This is permanent: the database and any other volume data are deleted. It is +// what removing a worktree needs, since otherwise each discarded feature branch +// leaves a full volume set behind. +func (r *Runner) Destroy(ctx context.Context) error { + args := r.buildComposeArgs() + args = append(args, downArgs(true)...) + + return r.runDockerCommand(ctx, args...) +} + +// downArgs builds the compose arguments for stopping an environment. +func downArgs(removeVolumes bool) []string { + args := []string{"down"} + + if removeVolumes { + args = append(args, "--volumes", "--remove-orphans") + } + + return args +} + // PS lists running containers. func (r *Runner) PS(ctx context.Context) error { args := r.buildComposeArgs() diff --git a/internal/dockercompose/teardown_test.go b/internal/dockercompose/teardown_test.go new file mode 100644 index 0000000..a83d311 --- /dev/null +++ b/internal/dockercompose/teardown_test.go @@ -0,0 +1,49 @@ +package dockercompose + +import ( + "strings" + "testing" +) + +// TestDownArgsOmitVolumes documents that Down leaves volumes in place, which is +// correct for stopping an environment you intend to start again. +func TestDownArgsOmitVolumes(t *testing.T) { + t.Parallel() + + args := downArgs(false) + + if !contains(args, "down") { + t.Fatalf("args %v do not invoke down", args) + } + + if contains(args, "--volumes") { + t.Errorf("Down must not remove volumes: %v", args) + } +} + +// TestDestroyArgsRemoveVolumes covers permanent teardown. Without --volumes the +// database survives, so removing a worktree would leak one volume set per +// feature branch. +func TestDestroyArgsRemoveVolumes(t *testing.T) { + t.Parallel() + + args := downArgs(true) + + if !contains(args, "--volumes") { + t.Errorf("Destroy must remove volumes, got %v", args) + } + + if !contains(args, "--remove-orphans") { + t.Errorf("Destroy should remove orphaned containers, got %v", args) + } +} + +func contains(args []string, want string) bool { + for _, a := range args { + if strings.TrimSpace(a) == want { + return true + } + } + + return false +} From dbebdddc545a89aa0b8081e21d633644f87d4dc1 Mon Sep 17 00:00:00 2001 From: Chris Deeming Date: Tue, 18 Aug 2026 02:41:49 +0100 Subject: [PATCH 08/14] feat(worktree): clone the source environment by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A worktree exists to work on the forum you already have, so it now inherits that forum's data: the database, plus data/ and internal_data/. Attachments in particular are the reason to clone, since testing media handling against an empty install proves little. --fresh opts out and installs a clean forum instead. The database is dumped and imported rather than copied, because each instance owns a separate named volume that the target's containers have already created. The dump streams through a temporary file so a large database does not have to fit in memory, and --single-transaction keeps the source usable while it runs. Cloning suppresses xf:install: the imported database is already installed, and reinstalling would wipe what was just copied. Files are copied natively rather than through rsync. macOS no longer ships GNU rsync — /usr/bin/rsync is openrsync, which lacks the progress options — and Windows has none at all, so an external tool would behave differently depending on the machine. The native walk preserves modes, which XenForo requires for data/ and internal_data/, and reports progress every hundred files rather than on each one: code_cache alone is thousands of small files, and rendering each would cost more than the copy. A source that was never installed has nothing to clone, so it falls back to a fresh install without needing to be asked. --- cmd/xf/worktree.go | 62 ++++++++++-- cmd/xf/worktree_clone.go | 168 +++++++++++++++++++++++++++++++ internal/dockercompose/runner.go | 47 +++++++++ internal/worktree/copy.go | 166 ++++++++++++++++++++++++++++++ internal/worktree/copy_test.go | 127 +++++++++++++++++++++++ 5 files changed, 562 insertions(+), 8 deletions(-) create mode 100644 cmd/xf/worktree_clone.go create mode 100644 internal/worktree/copy.go create mode 100644 internal/worktree/copy_test.go diff --git a/cmd/xf/worktree.go b/cmd/xf/worktree.go index 4f7bf0c..ec170ba 100644 --- a/cmd/xf/worktree.go +++ b/cmd/xf/worktree.go @@ -139,12 +139,14 @@ var ( flagWorktreeJSON bool flagWorktreeForce bool flagWorktreeKeepContainers bool + flagWorktreeFresh bool ) func init() { worktreeCreateCmd.Flags().StringVar(&flagWorktreeBase, "base", "", "ref to branch from (defaults to current HEAD)") worktreeCreateCmd.Flags().BoolVar(&flagWorktreeNoSetup, "no-setup", false, "create the worktree only, without setting up the environment") worktreeCreateCmd.Flags().BoolVar(&flagWorktreeNoUp, "no-up", false, "configure the environment but do not start containers") + worktreeCreateCmd.Flags().BoolVar(&flagWorktreeFresh, "fresh", false, "install a clean forum instead of cloning the source environment") worktreeCreateCmd.Flags().StringVar(&flagWorktreeInstance, "instance", "", "Docker instance name") worktreeCreateCmd.Flags().BoolVar(&flagWorktreeJSON, "json", false, "output as JSON") worktreeCreateCmd.Flags().StringVar(&flagWorktreeAdminUser, "admin-user", "", "admin username (default \"admin\")") @@ -223,11 +225,27 @@ func runWorktreeCreate(cmd *cobra.Command, args []string) error { }) } + // Cloning imports a database that is already installed, so xf:install must + // not run over it: it would wipe the data that was just copied. + cloning := !flagWorktreeFresh && sourceIsInstalled(result.SourcePath) + if !flagWorktreeNoSetup { - if err := setUpWorktree(cmd.Context(), result); err != nil { + if err := setUpWorktree(cmd.Context(), result, worktreeInitOptions(result, cloning)); err != nil { return err } + if cloning && !flagWorktreeNoUp { + if err := cloneEnvironment(cmd.Context(), result.SourcePath, result.Path); err != nil { + return fmt.Errorf("worktree created at %s, but cloning the environment failed: %w", result.Path, err) + } + + entry.Cloned = true + + if err := recordWorktree(entry); err != nil { + ui.PrintWarning(fmt.Sprintf("Could not record worktree in the registry: %v", err)) + } + } + if !flagWorktreeJSON && !flagWorktreeNoUp { ui.Println() ui.PrintKeyValuePadded([]ui.KVPair{ @@ -251,18 +269,23 @@ func runWorktreeCreate(cmd *cobra.Command, args []string) error { return nil } -// setUpWorktree initialises the environment by delegating to init, which -// already handles Docker configuration, containers, Composer and installation. -func setUpWorktree(ctx context.Context, result *worktree.Result) error { - // A worktree is a disposable development environment, so it installs with - // fixed credentials rather than prompting. Knowing the login without being - // asked is the point: one command produces a usable forum. - opts := &InitOptions{ +// worktreeInitOptions builds the init options for a new worktree. +// +// cloning reports whether the source environment will be copied in, which +// suppresses xf:install: the imported database is already installed, and +// reinstalling would wipe the data that was just copied. +func worktreeInitOptions(result *worktree.Result, cloning bool) *InitOptions { + // A worktree is a disposable development environment, so a fresh install + // uses fixed credentials rather than prompting. Knowing the login without + // being asked is the point: one command produces a usable forum. A cloned + // worktree keeps the source's own credentials. + return &InitOptions{ TargetPath: result.Path, InstanceName: result.Instance, ExistingOnly: true, SkipUp: flagWorktreeNoUp, StartContainers: !flagWorktreeNoUp, + SkipInstall: cloning, AdminUser: defaultString(flagWorktreeAdminUser, defaultWorktreeAdminUser), AdminPassword: defaultString(flagWorktreeAdminPassword, defaultWorktreeAdminPassword), AdminEmail: defaultString(flagWorktreeAdminEmail, defaultWorktreeAdminEmail), @@ -272,7 +295,11 @@ func setUpWorktree(ctx context.Context, result *worktree.Result) error { ProductOverrides: map[string]int{}, ProductTitleMap: map[string]string{}, } +} +// setUpWorktree initialises the environment by delegating to init, which +// already handles Docker configuration, containers, Composer and installation. +func setUpWorktree(ctx context.Context, result *worktree.Result, opts *InitOptions) error { if err := initExisting(ctx, opts); err != nil { return fmt.Errorf("worktree created at %s, but setting up its environment failed: %w", result.Path, err) } @@ -537,3 +564,22 @@ func destroyWorktreeEnvironment(ctx context.Context, worktreePath string) error return nil } + +// sourceIsInstalled reports whether the source checkout holds a XenForo +// installation that can be cloned. +// +// A checkout that has never been installed has no database or attachments to +// copy, so a new worktree gets a fresh install instead. +func sourceIsInstalled(sourcePath string) bool { + // XenForo writes this once installation completes. + if _, err := os.Stat(filepath.Join(sourcePath, "internal_data", "install-lock.php")); err != nil { + return false + } + + // Without compose configuration there is no database to dump. + if _, err := os.Stat(filepath.Join(sourcePath, "compose.yaml")); err != nil { + return false + } + + return true +} diff --git a/cmd/xf/worktree_clone.go b/cmd/xf/worktree_clone.go new file mode 100644 index 0000000..420ac5c --- /dev/null +++ b/cmd/xf/worktree_clone.go @@ -0,0 +1,168 @@ +package main + +import ( + "context" + "fmt" + "os" + "path/filepath" + + "github.com/xenforo-ltd/cli/internal/dockercompose" + "github.com/xenforo-ltd/cli/internal/ui" + "github.com/xenforo-ltd/cli/internal/worktree" +) + +// clonedDirectories are the directories copied from the source installation. +// +// data/ holds public attachments and assets; internal_data/ holds private +// attachments, which are the main reason to clone at all. Everything else a +// XenForo install needs is either tracked in git or regenerated. +var clonedDirectories = []string{"data", "internal_data"} + +// cloneEnvironment reproduces a source installation in a new worktree: its +// database first, then its files. +// +// The database is dumped and imported rather than copied, because each instance +// has its own named volume that the target's containers already own. +func cloneEnvironment(ctx context.Context, sourcePath, worktreePath string) error { + sourceRunner, err := dockercompose.NewRunner(sourcePath) + if err != nil { + return fmt.Errorf("cannot clone from %s: %w", sourcePath, err) + } + + targetRunner, err := dockercompose.NewRunner(worktreePath) + if err != nil { + return fmt.Errorf("cannot clone into %s: %w", worktreePath, err) + } + + if err := cloneDatabase(ctx, sourceRunner, targetRunner); err != nil { + return err + } + + return cloneFiles(ctx, sourcePath, worktreePath) +} + +// cloneDatabase streams a dump from the source instance into the target's. +func cloneDatabase(ctx context.Context, source, target *dockercompose.Runner) error { + user, password := source.DatabaseCredentials() + database := source.DatabaseName() + + dumpPath := filepath.Join(os.TempDir(), "xf-clone-"+target.Instance()+".sql") + + defer func() { + _ = os.Remove(dumpPath) + }() + + spinner := ui.NewSpinner("Exporting database from source...") + spinner.Start() + + dump, err := os.Create(dumpPath) + if err != nil { + spinner.StopWithMessage("error", "Failed to export database") + + return fmt.Errorf("failed to create dump file: %w", err) + } + + // --single-transaction keeps the source usable during the dump. + dumpCmd := []string{ + "mariadb-dump", + "--user=" + user, + "--password=" + password, + "--single-transaction", + "--routines", + "--events", + database, + } + + if err := source.ExecCapture(ctx, "mysql", dump, dumpCmd...); err != nil { + _ = dump.Close() + spinner.StopWithMessage("error", "Failed to export database") + + return fmt.Errorf("failed to export the source database: %w", err) + } + + if err := dump.Close(); err != nil { + spinner.StopWithMessage("error", "Failed to export database") + + return fmt.Errorf("failed to finish the dump: %w", err) + } + + info, err := os.Stat(dumpPath) + if err != nil { + spinner.StopWithMessage("error", "Failed to export database") + + return fmt.Errorf("failed to inspect the dump: %w", err) + } + + spinner.StopWithMessage("success", "Database exported ("+ui.FormatBytes(info.Size())+")") + + spinner = ui.NewSpinner("Importing database into worktree...") + spinner.Start() + + restore, err := os.Open(dumpPath) + if err != nil { + spinner.StopWithMessage("error", "Failed to import database") + + return fmt.Errorf("failed to read the dump: %w", err) + } + + defer func() { + _ = restore.Close() + }() + + targetUser, targetPassword := target.DatabaseCredentials() + + importCmd := []string{ + "mariadb", + "--user=" + targetUser, + "--password=" + targetPassword, + target.DatabaseName(), + } + + if err := target.ExecInput(ctx, "mysql", restore, importCmd...); err != nil { + spinner.StopWithMessage("error", "Failed to import database") + + return fmt.Errorf("failed to import the database: %w", err) + } + + spinner.StopWithMessage("success", "Database imported") + + return nil +} + +// cloneFiles copies the source's user content into the worktree. +func cloneFiles(ctx context.Context, sourcePath, worktreePath string) error { + for _, dir := range clonedDirectories { + src := filepath.Join(sourcePath, dir) + + if _, err := os.Stat(src); os.IsNotExist(err) { + continue + } + + spinner := ui.NewSpinner("Copying " + dir + "...") + spinner.Start() + + var lastReported int + + err := worktree.CopyTree(ctx, src, filepath.Join(worktreePath, dir), func(copied, total int) { + // Updating on every file would spend more time rendering than + // copying, since code_cache alone is thousands of small files. + if total > 0 && (copied == total || copied-lastReported >= progressUpdateInterval) { + lastReported = copied + + spinner.UpdateMessage(fmt.Sprintf("Copying %s... %d/%d files", dir, copied, total)) + } + }) + if err != nil { + spinner.StopWithMessage("error", "Failed to copy "+dir) + + return fmt.Errorf("failed to copy %s: %w", dir, err) + } + + spinner.StopWithMessage("success", "Copied "+dir) + } + + return nil +} + +// progressUpdateInterval is how many files to copy between progress updates. +const progressUpdateInterval = 100 diff --git a/internal/dockercompose/runner.go b/internal/dockercompose/runner.go index 462f5b3..ff6a107 100644 --- a/internal/dockercompose/runner.go +++ b/internal/dockercompose/runner.go @@ -120,6 +120,53 @@ func (r *Runner) UpWithOutput(ctx context.Context, detach bool, stdout, stderr i return r.runDockerCommandWithOutput(ctx, stdout, stderr, args...) } +// ExecCapture runs a command in a service, streaming its output to stdout. +// +// Output is streamed rather than buffered so that large results, such as a +// database dump, do not have to fit in memory. +func (r *Runner) ExecCapture(ctx context.Context, service string, stdout io.Writer, cmd ...string) error { + args := r.buildComposeArgs() + args = append(args, "exec", "-T", service) + args = append(args, cmd...) + + return r.runDockerCommandWithIO(ctx, nil, stdout, os.Stderr, args...) +} + +// ExecInput runs a command in a service, feeding it from stdin. +func (r *Runner) ExecInput(ctx context.Context, service string, stdin io.Reader, cmd ...string) error { + args := r.buildComposeArgs() + args = append(args, "exec", "-T", service) + args = append(args, cmd...) + + return r.runDockerCommandWithIO(ctx, stdin, os.Stdout, os.Stderr, args...) +} + +// runDockerCommandWithIO executes a docker command with explicit streams. +func (r *Runner) runDockerCommandWithIO(ctx context.Context, stdin io.Reader, stdout, stderr io.Writer, args ...string) error { + cmd := exec.CommandContext(ctx, "docker", args...) + cmd.Dir = r.xfDir + cmd.Stdin = stdin + cmd.Stdout = stdout + cmd.Stderr = stderr + cmd.Env = append(os.Environ(), "XF_DIR="+r.xfDir) + + if err := cmd.Run(); err != nil { + return contextError(ctx, fmt.Errorf("docker command failed: %w", err)) + } + + return nil +} + +// DatabaseCredentials returns the configured database user and password. +func (r *Runner) DatabaseCredentials() (string, string) { + return r.getDatabaseCredentials() +} + +// DatabaseName returns the configured database name. +func (r *Runner) DatabaseName() string { + return r.resolveEnvValue("XF_DB_DATABASE", "xf") +} + // Down stops and removes the Docker containers, leaving volumes intact so the // environment can be started again with its data. func (r *Runner) Down(ctx context.Context) error { diff --git a/internal/worktree/copy.go b/internal/worktree/copy.go new file mode 100644 index 0000000..9180d6b --- /dev/null +++ b/internal/worktree/copy.go @@ -0,0 +1,166 @@ +package worktree + +import ( + "context" + "fmt" + "io" + "os" + "path/filepath" +) + +// ProgressFunc reports copy progress as files are written. +type ProgressFunc func(copied, total int) + +// CopyTree recursively copies src to dst, preserving file modes. +// +// A missing source is not an error: a XenForo installation may legitimately +// have no data/ or internal_data/ yet. +// +// This is implemented natively rather than by shelling out to rsync. macOS no +// longer ships GNU rsync — /usr/bin/rsync is openrsync, which lacks the +// progress options — and Windows has no rsync at all, so an external tool would +// behave differently depending on the machine. +func CopyTree(ctx context.Context, src, dst string, progress ProgressFunc) error { + info, err := os.Stat(src) + if err != nil { + if os.IsNotExist(err) { + return nil + } + + return fmt.Errorf("failed to inspect %s: %w", src, err) + } + + if !info.IsDir() { + return fmt.Errorf("%s is not a directory: %w", src, ErrInvalidBranch) + } + + total := 0 + + if progress != nil { + total, err = countFiles(ctx, src) + if err != nil { + return err + } + } + + copied := 0 + + return filepath.Walk(src, func(path string, entry os.FileInfo, walkErr error) error { + if walkErr != nil { + return walkErr + } + + if err := ctx.Err(); err != nil { + return err + } + + rel, err := filepath.Rel(src, path) + if err != nil { + return fmt.Errorf("failed to resolve %s: %w", path, err) + } + + target := filepath.Join(dst, rel) + + switch { + case entry.IsDir(): + return os.MkdirAll(target, entry.Mode().Perm()) + + case entry.Mode()&os.ModeSymlink != 0: + return copySymlink(path, target) + + case !entry.Mode().IsRegular(): + // Sockets and devices have no meaning in a copy. + return nil + } + + if err := copyFile(path, target, entry.Mode().Perm()); err != nil { + return err + } + + copied++ + + if progress != nil { + progress(copied, total) + } + + return nil + }) +} + +// countFiles counts regular files so progress can be reported as a fraction. +func countFiles(ctx context.Context, root string) (int, error) { + count := 0 + + err := filepath.Walk(root, func(_ string, entry os.FileInfo, walkErr error) error { + if walkErr != nil { + return walkErr + } + + if err := ctx.Err(); err != nil { + return err + } + + if entry.Mode().IsRegular() { + count++ + } + + return nil + }) + if err != nil { + return 0, fmt.Errorf("failed to count files in %s: %w", root, err) + } + + return count, nil +} + +func copyFile(src, dst string, mode os.FileMode) error { + if err := os.MkdirAll(filepath.Dir(dst), 0o750); err != nil { + return fmt.Errorf("failed to create %s: %w", filepath.Dir(dst), err) + } + + in, err := os.Open(src) + if err != nil { + return fmt.Errorf("failed to open %s: %w", src, err) + } + + defer func() { + _ = in.Close() + }() + + out, err := os.OpenFile(dst, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode) + if err != nil { + return fmt.Errorf("failed to create %s: %w", dst, err) + } + + if _, err := io.Copy(out, in); err != nil { + _ = out.Close() + + return fmt.Errorf("failed to copy %s: %w", src, err) + } + + if err := out.Close(); err != nil { + return fmt.Errorf("failed to write %s: %w", dst, err) + } + + return nil +} + +func copySymlink(src, dst string) error { + target, err := os.Readlink(src) + if err != nil { + return fmt.Errorf("failed to read link %s: %w", src, err) + } + + if err := os.MkdirAll(filepath.Dir(dst), 0o750); err != nil { + return fmt.Errorf("failed to create %s: %w", filepath.Dir(dst), err) + } + + // A pre-existing link would make Symlink fail. + _ = os.Remove(dst) + + if err := os.Symlink(target, dst); err != nil { + return fmt.Errorf("failed to create link %s: %w", dst, err) + } + + return nil +} diff --git a/internal/worktree/copy_test.go b/internal/worktree/copy_test.go new file mode 100644 index 0000000..4537432 --- /dev/null +++ b/internal/worktree/copy_test.go @@ -0,0 +1,127 @@ +package worktree + +import ( + "context" + "os" + "path/filepath" + "testing" +) + +func TestCopyTreeCopiesFilesAndDirectories(t *testing.T) { + src := t.TempDir() + dst := filepath.Join(t.TempDir(), "target") + + mustWrite(t, filepath.Join(src, "top.txt"), "top") + mustWrite(t, filepath.Join(src, "nested", "deep", "file.txt"), "deep") + + if err := CopyTree(t.Context(), src, dst, nil); err != nil { + t.Fatalf("CopyTree: %v", err) + } + + assertContent(t, filepath.Join(dst, "top.txt"), "top") + assertContent(t, filepath.Join(dst, "nested", "deep", "file.txt"), "deep") +} + +// TestCopyTreePreservesModes matters because XenForo checks that data/ and +// internal_data/ are writable. +func TestCopyTreePreservesModes(t *testing.T) { + src := t.TempDir() + dst := filepath.Join(t.TempDir(), "target") + + path := filepath.Join(src, "script.sh") + mustWrite(t, path, "#!/bin/sh") + + if err := os.Chmod(path, 0o755); err != nil { + t.Fatalf("chmod: %v", err) + } + + if err := CopyTree(t.Context(), src, dst, nil); err != nil { + t.Fatalf("CopyTree: %v", err) + } + + info, err := os.Stat(filepath.Join(dst, "script.sh")) + if err != nil { + t.Fatalf("stat: %v", err) + } + + if info.Mode().Perm() != 0o755 { + t.Errorf("mode = %v, want 0755", info.Mode().Perm()) + } +} + +func TestCopyTreeReportsProgress(t *testing.T) { + src := t.TempDir() + dst := filepath.Join(t.TempDir(), "target") + + for _, name := range []string{"a.txt", "b.txt", "c.txt"} { + mustWrite(t, filepath.Join(src, name), name) + } + + var seen int + + err := CopyTree(t.Context(), src, dst, func(copied, total int) { + seen = copied + + if total != 3 { + t.Errorf("total = %d, want 3", total) + } + }) + if err != nil { + t.Fatalf("CopyTree: %v", err) + } + + if seen != 3 { + t.Errorf("final progress = %d, want 3", seen) + } +} + +// TestCopyTreeSkipsMissingSource covers a source directory that does not exist, +// which is normal: a XenForo install may have no data/ yet. +func TestCopyTreeSkipsMissingSource(t *testing.T) { + dst := filepath.Join(t.TempDir(), "target") + + if err := CopyTree(t.Context(), filepath.Join(t.TempDir(), "absent"), dst, nil); err != nil { + t.Errorf("a missing source must not be an error, got %v", err) + } +} + +func TestCopyTreeIsCancellable(t *testing.T) { + src := t.TempDir() + dst := filepath.Join(t.TempDir(), "target") + + for i := range 50 { + mustWrite(t, filepath.Join(src, string(rune('a'+i%26))+string(rune('0'+i/26))+".txt"), "x") + } + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + if err := CopyTree(ctx, src, dst, nil); err == nil { + t.Error("expected a cancelled copy to return an error") + } +} + +func mustWrite(t *testing.T, path, content string) { + t.Helper() + + if err := os.MkdirAll(filepath.Dir(path), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write: %v", err) + } +} + +func assertContent(t *testing.T, path, want string) { + t.Helper() + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + + if string(data) != want { + t.Errorf("%s = %q, want %q", path, data, want) + } +} From 26235598594e528e997b908f277459e64952a5f3 Mon Sep 17 00:00:00 2001 From: Chris Deeming Date: Tue, 18 Aug 2026 02:49:26 +0100 Subject: [PATCH 09/14] fix(worktree): do not report admin credentials for a cloned worktree A cloned worktree keeps the source forum's own logins, so reporting the defaults that a fresh install would have used was simply wrong: those credentials do not work there. Report them only when the worktree was freshly installed, where they are the login you need and are otherwise unknowable. --- cmd/xf/worktree.go | 170 ++++++++++++++++++++++++++++------- cmd/xf/worktree_json_test.go | 75 ++++++++++++++++ 2 files changed, 215 insertions(+), 30 deletions(-) create mode 100644 cmd/xf/worktree_json_test.go diff --git a/cmd/xf/worktree.go b/cmd/xf/worktree.go index ec170ba..a5115b0 100644 --- a/cmd/xf/worktree.go +++ b/cmd/xf/worktree.go @@ -148,7 +148,11 @@ func init() { worktreeCreateCmd.Flags().BoolVar(&flagWorktreeNoUp, "no-up", false, "configure the environment but do not start containers") worktreeCreateCmd.Flags().BoolVar(&flagWorktreeFresh, "fresh", false, "install a clean forum instead of cloning the source environment") worktreeCreateCmd.Flags().StringVar(&flagWorktreeInstance, "instance", "", "Docker instance name") - worktreeCreateCmd.Flags().BoolVar(&flagWorktreeJSON, "json", false, "output as JSON") + // Known limitation: setup progress from init and cloning still goes to + // stdout, so the stream is only pure JSON when --no-setup is used. The + // output layer writes through package-level helpers with no injectable + // writer, so routing it to stderr is a wider change than this command. + worktreeCreateCmd.Flags().BoolVar(&flagWorktreeJSON, "json", false, "output as JSON (setup progress is still written to stdout)") worktreeCreateCmd.Flags().StringVar(&flagWorktreeAdminUser, "admin-user", "", "admin username (default \"admin\")") worktreeCreateCmd.Flags().StringVar(&flagWorktreeAdminPassword, "admin-password", "", "admin password (default \"password\")") worktreeCreateCmd.Flags().StringVar(&flagWorktreeAdminEmail, "admin-email", "", "admin email (default \"admin@example.com\")") @@ -227,14 +231,27 @@ func runWorktreeCreate(cmd *cobra.Command, args []string) error { // Cloning imports a database that is already installed, so xf:install must // not run over it: it would wipe the data that was just copied. - cloning := !flagWorktreeFresh && sourceIsInstalled(result.SourcePath) + // + // Cloning needs running containers, so --no-up rules it out. Treating the + // worktree as cloning anyway would suppress xf:install as well, leaving it + // with neither an imported database nor an installed one. + cloning := false + + if !flagWorktreeFresh && !flagWorktreeNoUp { + installed, err := sourceIsInstalled(result.SourcePath) + if err != nil { + return err + } + + cloning = installed + } if !flagWorktreeNoSetup { if err := setUpWorktree(cmd.Context(), result, worktreeInitOptions(result, cloning)); err != nil { return err } - if cloning && !flagWorktreeNoUp { + if cloning { if err := cloneEnvironment(cmd.Context(), result.SourcePath, result.Path); err != nil { return fmt.Errorf("worktree created at %s, but cloning the environment failed: %w", result.Path, err) } @@ -246,7 +263,10 @@ func runWorktreeCreate(cmd *cobra.Command, args []string) error { } } - if !flagWorktreeJSON && !flagWorktreeNoUp { + // Only a fresh install has credentials worth reporting. A cloned + // worktree keeps the source's own logins, so printing the defaults + // would be wrong. + if !cloning && !flagWorktreeJSON && !flagWorktreeNoUp { ui.Println() ui.PrintKeyValuePadded([]ui.KVPair{ ui.KV("Admin user", defaultString(flagWorktreeAdminUser, defaultWorktreeAdminUser)), @@ -262,6 +282,7 @@ func runWorktreeCreate(cmd *cobra.Command, args []string) error { SourcePath: result.SourcePath, SourceBranch: result.SourceBranch, Instance: result.Instance, + Cloned: entry.Cloned, CreatedAt: result.CreatedAt, }) } @@ -318,8 +339,13 @@ func runWorktreePath(cmd *cobra.Command, args []string) error { return err } + target, err := worktree.ResolveExistingPath(source, args[0]) + if err != nil { + return err + } + // Printed bare, with no decoration, so it can be used directly in a shell. - fmt.Println(worktree.ResolvePath(source, args[0])) + fmt.Println(target) return nil } @@ -373,7 +399,27 @@ func runWorktreeRemove(cmd *cobra.Command, args []string) error { return err } - target := worktree.ResolvePath(source, args[0]) + target, err := worktree.ResolveExistingPath(source, args[0]) + if err != nil { + return err + } + + // The path is derived from the branch's last segment, so dev/a/foo and + // dev/b/foo resolve to the same directory. Removing without checking which + // branch is actually there would destroy the wrong worktree, its branch, + // and its volumes. + if err := verifyWorktreeBranch(cmd.Context(), target, args[0]); err != nil { + return err + } + + // The safety check runs before anything is destroyed. Tearing down first + // and refusing afterwards would report that the worktree was kept while + // its database and volumes had already been deleted. + if !flagWorktreeForce { + if err := worktree.CheckRemovable(cmd.Context(), target); err != nil { + return err + } + } // Containers must be torn down before the directory goes: compose reads // compose.yaml from the worktree to know what it owns, so removing the @@ -388,10 +434,13 @@ func runWorktreeRemove(cmd *cobra.Command, args []string) error { return err } - if registry, regErr := worktree.NewRegistry(); regErr == nil { - if err := registry.Remove(target); err != nil { - ui.PrintWarning(fmt.Sprintf("Could not update the worktree registry: %v", err)) - } + registry, regErr := worktree.NewRegistry() + if regErr != nil { + // Reported rather than ignored: the worktree is gone but the registry + // still lists it, and only this message tells the user why. + ui.PrintWarning(fmt.Sprintf("Could not open the worktree registry: %v", regErr)) + } else if err := registry.Remove(target); err != nil { + ui.PrintWarning(fmt.Sprintf("Could not update the worktree registry: %v", err)) } ui.PrintSuccess("Removed worktree " + target) @@ -433,13 +482,38 @@ func runWorktreePrune(cmd *cobra.Command, args []string) error { return nil } +// worktreeState reports whether a registered worktree still exists on disk. +func worktreeState(worktreePath string) string { + if _, err := os.Stat(worktreePath); os.IsNotExist(err) { + return "missing" + } + + return "ok" +} + // printWorktrees renders entries, reconciling them against the filesystem. // // The registry is a record, not the source of truth: worktrees get removed // outside xf, so entries are checked rather than trusted. func printWorktrees(entries []worktree.Entry) error { if flagWorktreeJSON { - return printJSON(entries) + // The same reconciliation the table performs, so machine consumers + // are not told about worktrees that no longer exist on disk. + type worktreeListEntry struct { + worktree.Entry + + State string `json:"state"` + } + + listed := make([]worktreeListEntry, 0, len(entries)) + for _, entry := range entries { + listed = append(listed, worktreeListEntry{ + Entry: entry, + State: worktreeState(entry.WorktreePath), + }) + } + + return printJSON(listed) } if len(entries) == 0 { @@ -452,16 +526,11 @@ func printWorktrees(entries []worktree.Entry) error { rows := make([][]string, 0, len(entries)) for _, entry := range entries { - state := "ok" - if _, err := os.Stat(entry.WorktreePath); os.IsNotExist(err) { - state = "missing" - } - rows = append(rows, []string{ entry.Branch, shortenPath(entry.WorktreePath), entry.Instance, - state, + worktreeState(entry.WorktreePath), }) } @@ -533,6 +602,36 @@ func defaultString(value, fallback string) string { return fallback } +// verifyWorktreeBranch reports whether the worktree at path has branch checked +// out. +// +// Worktree paths are derived from a branch's last segment, so dev/a/foo and +// dev/b/foo share a directory. Without this check, removing one branch would +// silently destroy the other's worktree, branch, containers and volumes. +func verifyWorktreeBranch(ctx context.Context, worktreePath, branch string) error { + if _, err := os.Stat(worktreePath); err != nil { + if os.IsNotExist(err) { + return fmt.Errorf("no worktree at %s: %w", worktreePath, err) + } + + return fmt.Errorf("failed to inspect %s: %w", worktreePath, err) + } + + current, err := worktree.CurrentBranch(ctx, worktreePath) + if err != nil { + return fmt.Errorf("failed to determine the branch checked out at %s: %w", worktreePath, err) + } + + if current != branch { + return fmt.Errorf( + "%s has %s checked out, not %s: refusing to remove it%.0w", + worktreePath, current, branch, ErrInvalidInput, + ) + } + + return nil +} + // destroyWorktreeEnvironment removes a worktree's containers and volumes. // // A worktree that was never set up has no compose configuration, which is not @@ -540,15 +639,16 @@ func defaultString(value, fallback string) string { func destroyWorktreeEnvironment(ctx context.Context, worktreePath string) error { runner, err := dockercompose.NewRunner(worktreePath) if err != nil { - if errors.Is(err, dockercompose.ErrEnvNotInitialized) { + // A worktree that was never set up has nothing to tear down, and a + // directory that is already gone cannot have running containers. + if errors.Is(err, dockercompose.ErrEnvNotInitialized) || errors.Is(err, os.ErrNotExist) { return nil } - // The directory may already be gone, or never have been a checkout. - // Removing the worktree is still worthwhile, so this is not fatal. - ui.PrintWarning(fmt.Sprintf("Could not inspect the environment to remove it: %v", err)) - - return nil + // Any other failure means the environment could not be inspected, not + // that it is absent. Continuing would delete the worktree and strand + // its containers and volumes, so stop instead. + return fmt.Errorf("failed to inspect the worktree environment: %w", err) } spinner := ui.NewSpinner("Removing containers and volumes...") @@ -570,16 +670,26 @@ func destroyWorktreeEnvironment(ctx context.Context, worktreePath string) error // // A checkout that has never been installed has no database or attachments to // copy, so a new worktree gets a fresh install instead. -func sourceIsInstalled(sourcePath string) bool { +func sourceIsInstalled(sourcePath string) (bool, error) { // XenForo writes this once installation completes. - if _, err := os.Stat(filepath.Join(sourcePath, "internal_data", "install-lock.php")); err != nil { - return false + markers := []string{ + filepath.Join(sourcePath, "internal_data", "install-lock.php"), + // Without compose configuration there is no database to dump. + filepath.Join(sourcePath, "compose.yaml"), } - // Without compose configuration there is no database to dump. - if _, err := os.Stat(filepath.Join(sourcePath, "compose.yaml")); err != nil { - return false + for _, marker := range markers { + if _, err := os.Stat(marker); err != nil { + if os.IsNotExist(err) { + return false, nil + } + + // A permission or I/O failure is not an absent marker. Treating it + // as one would quietly install a fresh forum where the user asked + // for a clone of an existing one. + return false, fmt.Errorf("failed to inspect %s: %w", marker, err) + } } - return true + return true, nil } diff --git a/cmd/xf/worktree_json_test.go b/cmd/xf/worktree_json_test.go new file mode 100644 index 0000000..5b9412e --- /dev/null +++ b/cmd/xf/worktree_json_test.go @@ -0,0 +1,75 @@ +package main + +import ( + "encoding/json" + "testing" + "time" + + "github.com/xenforo-ltd/cli/internal/worktree" +) + +// The JSON output reports whether the source environment was cloned, so +// automation can tell an installed worktree from an empty one. +func TestWorktreeOutputReportsTheCloneResult(t *testing.T) { + entry := worktree.Entry{ + SourcePath: "/src", + SourceBranch: "main", + WorktreePath: "/src.worktrees/feature", + Branch: "dev/feature", + Instance: "feature", + CreatedAt: time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC), + } + + // cloneEnvironment succeeding is what sets this, immediately before the + // output is built. + entry.Cloned = true + + data, err := json.Marshal(worktreeOutput{ + Path: entry.WorktreePath, + Branch: entry.Branch, + SourcePath: entry.SourcePath, + SourceBranch: entry.SourceBranch, + Instance: entry.Instance, + Cloned: entry.Cloned, + CreatedAt: entry.CreatedAt, + }) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var decoded map[string]any + if err := json.Unmarshal(data, &decoded); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + if decoded["cloned"] != true { + t.Errorf("cloned = %v, want true after a successful clone", decoded["cloned"]) + } +} + +func TestWorktreeOutputReportsAnUnclonedWorktree(t *testing.T) { + data, err := json.Marshal(worktreeOutput{ + Path: "/src.worktrees/feature", + Branch: "dev/feature", + Cloned: false, + }) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var decoded map[string]any + if err := json.Unmarshal(data, &decoded); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + // The key must always be present, so consumers can branch on it without + // checking for existence first. + value, ok := decoded["cloned"] + if !ok { + t.Fatal("cloned key is missing") + } + + if value != false { + t.Errorf("cloned = %v, want false", value) + } +} From 17dd035e2eee6ed9c793ca1ba6fade44f64101f2 Mon Sep 17 00:00:00 2001 From: Chris Deeming Date: Tue, 18 Aug 2026 03:53:00 +0100 Subject: [PATCH 10/14] feat(worktree): name worktrees after the branch's last segment dev/xfs/slack-unfurl now yields a worktree at .worktrees/slack-unfurl with instance name slack-unfurl, rather than dev-xfs-slack-unfurl. The prefix in a conventional branch name describes where work belongs rather than what it is, and repeating it made directory names, site URLs and Docker instance names harder to read for no benefit. This is lossier than before: dev/xfs/slack-unfurl and dev/xf/slack-unfurl now want the same directory. Preflight already refused to reuse an occupied directory, so the collision is caught rather than silently resolved, and the error now names the branch already using it and suggests a more specific alternative: "slack-unfurl" is already used by branch "dev/xfs/slack-unfurl"; choose a more specific final segment, such as "xf-slack-unfurl" Rejecting is preferable to appending an index. An index would depend on what existed when the worktree was created, so resolving a branch to its path would require consulting stored state. Keeping that mapping pure is what lets `xf worktree path` answer offline, and what makes a lost registry recoverable rather than fatal. --- cmd/xf/worktree.go | 5 +- internal/worktree/copy.go | 69 ++++++++++++- internal/worktree/copy_test.go | 65 ++++++++++++ internal/worktree/create.go | 26 ++++- internal/worktree/create_test.go | 7 +- internal/worktree/git.go | 45 ++++++++ internal/worktree/naming_test.go | 125 +++++++++++++++++++++++ internal/worktree/paths.go | 52 ++++++++-- internal/worktree/paths_test.go | 10 +- internal/worktree/registry.go | 141 +++++++++++++++++++++++++- internal/worktree/registry_test.go | 109 ++++++++++++++++++++ internal/worktree/umask_other_test.go | 7 ++ internal/worktree/umask_unix_test.go | 14 +++ 13 files changed, 648 insertions(+), 27 deletions(-) create mode 100644 internal/worktree/naming_test.go create mode 100644 internal/worktree/umask_other_test.go create mode 100644 internal/worktree/umask_unix_test.go diff --git a/cmd/xf/worktree.go b/cmd/xf/worktree.go index a5115b0..ff0808d 100644 --- a/cmd/xf/worktree.go +++ b/cmd/xf/worktree.go @@ -97,7 +97,10 @@ var worktreePathCmd = &cobra.Command{ The path is derived from the branch name, so this works whether or not the worktree exists. Useful for shell and agent use: - cd "$(xf worktree path dev/24x/feature)"`, + cd "$(xf worktree path dev/24x/feature)" + +Branch names that resolve to no directory of their own, such as "." or "..", +are rejected rather than printing the directory that holds every worktree.`, Args: cobra.ExactArgs(1), RunE: runWorktreePath, } diff --git a/internal/worktree/copy.go b/internal/worktree/copy.go index 9180d6b..9f147ef 100644 --- a/internal/worktree/copy.go +++ b/internal/worktree/copy.go @@ -2,12 +2,18 @@ package worktree import ( "context" + "errors" "fmt" "io" "os" "path/filepath" + "slices" + "strings" ) +// ErrNotADirectory indicates a copy source exists but is not a directory. +var ErrNotADirectory = errors.New("not a directory") + // ProgressFunc reports copy progress as files are written. type ProgressFunc func(copied, total int) @@ -31,13 +37,21 @@ func CopyTree(ctx context.Context, src, dst string, progress ProgressFunc) error } if !info.IsDir() { - return fmt.Errorf("%s is not a directory: %w", src, ErrInvalidBranch) + return fmt.Errorf("%s is not a directory: %w", src, ErrNotADirectory) + } + + // filepath.Walk does not follow a symlink at the root of the walk, even + // though os.Stat above does. Without this, a symlinked src is walked as + // the link itself rather than the directory it points to. + root, err := filepath.EvalSymlinks(src) + if err != nil { + return fmt.Errorf("failed to resolve %s: %w", src, err) } total := 0 if progress != nil { - total, err = countFiles(ctx, src) + total, err = countFiles(ctx, root) if err != nil { return err } @@ -45,7 +59,17 @@ func CopyTree(ctx context.Context, src, dst string, progress ProgressFunc) error copied := 0 - return filepath.Walk(src, func(path string, entry os.FileInfo, walkErr error) error { + // Directory modes are applied after the walk, deepest first. Applying a + // source mode on arrival would make a read-only directory (0555, say) + // read-only before its own children were written, failing the copy. + type pendingMode struct { + path string + mode os.FileMode + } + + var pending []pendingMode + + walkErr := filepath.Walk(root, func(path string, entry os.FileInfo, walkErr error) error { if walkErr != nil { return walkErr } @@ -54,7 +78,7 @@ func CopyTree(ctx context.Context, src, dst string, progress ProgressFunc) error return err } - rel, err := filepath.Rel(src, path) + rel, err := filepath.Rel(root, path) if err != nil { return fmt.Errorf("failed to resolve %s: %w", path, err) } @@ -63,7 +87,15 @@ func CopyTree(ctx context.Context, src, dst string, progress ProgressFunc) error switch { case entry.IsDir(): - return os.MkdirAll(target, entry.Mode().Perm()) + // Created writable so the copy can proceed; the source mode is + // applied once the directory's contents are in place. + if err := os.MkdirAll(target, 0o750); err != nil { + return err + } + + pending = append(pending, pendingMode{path: target, mode: entry.Mode().Perm()}) + + return nil case entry.Mode()&os.ModeSymlink != 0: return copySymlink(path, target) @@ -85,6 +117,26 @@ func CopyTree(ctx context.Context, src, dst string, progress ProgressFunc) error return nil }) + if walkErr != nil { + return walkErr + } + + // Deepest first, so applying a restrictive mode to a parent cannot block + // the chmod of a directory inside it. + slices.SortFunc(pending, func(a, b pendingMode) int { + return strings.Count(b.path, string(os.PathSeparator)) - strings.Count(a.path, string(os.PathSeparator)) + }) + + for _, dir := range pending { + // MkdirAll only applies a mode to directories it creates, and the + // umask can clear bits even then, so an existing destination (e.g. + // re-cloning over a worktree) can otherwise keep a stale mode. + if err := os.Chmod(dir.path, dir.mode); err != nil { + return fmt.Errorf("failed to set the mode of %s: %w", dir.path, err) + } + } + + return nil } // countFiles counts regular files so progress can be reported as a fraction. @@ -142,6 +194,13 @@ func copyFile(src, dst string, mode os.FileMode) error { return fmt.Errorf("failed to write %s: %w", dst, err) } + // OpenFile only applies mode at creation, and the umask can clear bits + // even then, so an existing destination file keeps its old mode unless + // it is set explicitly. + if err := os.Chmod(dst, mode); err != nil { + return fmt.Errorf("failed to set mode on %s: %w", dst, err) + } + return nil } diff --git a/internal/worktree/copy_test.go b/internal/worktree/copy_test.go index 4537432..daafecc 100644 --- a/internal/worktree/copy_test.go +++ b/internal/worktree/copy_test.go @@ -4,9 +4,12 @@ import ( "context" "os" "path/filepath" + "runtime" "testing" ) +const windowsOS = "windows" + func TestCopyTreeCopiesFilesAndDirectories(t *testing.T) { src := t.TempDir() dst := filepath.Join(t.TempDir(), "target") @@ -24,7 +27,19 @@ func TestCopyTreeCopiesFilesAndDirectories(t *testing.T) { // TestCopyTreePreservesModes matters because XenForo checks that data/ and // internal_data/ are writable. +// +// The umask is set strictly here because relying on the ambient umask made +// this assertion pass only by luck: a permissive umask (022 or looser) never +// exercises the case CopyTree exists to handle, where OpenFile's requested +// mode gets bits cleared away by the umask at creation time. func TestCopyTreePreservesModes(t *testing.T) { + if runtime.GOOS == windowsOS { + t.Skip("file mode bits are not meaningful on Windows") + } + + old := setUmask(0o077) + defer setUmask(old) + src := t.TempDir() dst := filepath.Join(t.TempDir(), "target") @@ -125,3 +140,53 @@ func assertContent(t *testing.T, path, want string) { t.Errorf("%s = %q, want %q", path, data, want) } } + +// A source directory without its owner-write bit must not stop the copy: the +// mode is applied after the directory's contents are in place, not before. +func TestCopyTreeCopiesIntoReadOnlyDirectories(t *testing.T) { + if runtime.GOOS == windowsOS { + t.Skip("file mode bits are not meaningful on Windows") + } + + src := t.TempDir() + dst := filepath.Join(t.TempDir(), "target") + + readOnly := filepath.Join(src, "locked") + if err := os.Mkdir(readOnly, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + + mustWrite(t, filepath.Join(readOnly, "data.txt"), "payload") + + if err := os.Chmod(readOnly, 0o555); err != nil { + t.Fatalf("chmod: %v", err) + } + + t.Cleanup(func() { + // Restore write access so the temp directory can be removed. + _ = os.Chmod(readOnly, 0o755) + _ = os.Chmod(filepath.Join(dst, "locked"), 0o755) + }) + + if err := CopyTree(t.Context(), src, dst, nil); err != nil { + t.Fatalf("CopyTree: %v", err) + } + + content, err := os.ReadFile(filepath.Join(dst, "locked", "data.txt")) + if err != nil { + t.Fatalf("read copied file: %v", err) + } + + if string(content) != "payload" { + t.Errorf("content = %q, want %q", content, "payload") + } + + info, err := os.Stat(filepath.Join(dst, "locked")) + if err != nil { + t.Fatalf("stat: %v", err) + } + + if info.Mode().Perm() != 0o555 { + t.Errorf("directory mode = %o, want 555", info.Mode().Perm()) + } +} diff --git a/internal/worktree/create.go b/internal/worktree/create.go index fa9f7c9..994536b 100644 --- a/internal/worktree/create.go +++ b/internal/worktree/create.go @@ -91,10 +91,21 @@ func Preflight(ctx context.Context, sourcePath, branch string) error { return fmt.Errorf("%w: %s", ErrBranchExists, branch) } - // The branch-to-directory mapping is lossy, so a different branch may - // already own this directory. Check the path, not just the branch. + // Only the last segment of a branch names the directory, so different + // branches can want the same one. Reject the collision rather than renaming + // around it: an auto-generated suffix would make the path depend on what + // existed at the time, and resolving a branch to its path would no longer + // be possible without consulting stored state. target := filepath.Join(WorktreesDir(sourcePath), dirName) if _, err := os.Stat(target); err == nil { + owner, ownerErr := worktreeOwner(ctx, sourcePath, target) + if ownerErr == nil && owner != "" && owner != branch { + return fmt.Errorf( + "%w: %q is already used by branch %q; choose a more specific final segment, such as %q", + ErrWorktreeExists, dirName, owner, suggestAlternative(branch), + ) + } + return fmt.Errorf("%w: %s", ErrWorktreeExists, target) } @@ -159,3 +170,14 @@ func Create(ctx context.Context, opts Options) (*Result, error) { CreatedAt: time.Now().UTC(), }, nil } + +// suggestAlternative proposes a more specific name for a colliding branch, by +// including the segment before the last one. +func suggestAlternative(branch string) string { + segments := strings.Split(strings.Trim(branch, "/"), "/") + if len(segments) < 2 { + return branch + "-2" + } + + return segments[len(segments)-2] + "-" + segments[len(segments)-1] +} diff --git a/internal/worktree/create_test.go b/internal/worktree/create_test.go index 8d3ce44..4c31f45 100644 --- a/internal/worktree/create_test.go +++ b/internal/worktree/create_test.go @@ -78,17 +78,18 @@ func TestPreflightRejectsExistingDirectory(t *testing.T) { } // TestPreflightRejectsCollidingName covers the lossy branch-to-directory -// mapping: two different branches can want the same directory. +// mapping: only the last segment names the directory, so branches that differ +// earlier can still want the same one. func TestPreflightRejectsCollidingName(t *testing.T) { repo := newXenForoRepo(t) - // dev/24x/feature and dev-24x-feature both flatten to dev-24x-feature. + // Both of these reduce to "feature". target := ResolvePath(repo, "dev/24x/feature") if err := os.MkdirAll(target, 0o750); err != nil { t.Fatalf("mkdir target: %v", err) } - err := Preflight(t.Context(), repo, "dev-24x-feature") + err := Preflight(t.Context(), repo, "dev/xfs/feature") if !errors.Is(err, ErrWorktreeExists) { t.Errorf("expected a collision to be caught, got %v", err) } diff --git a/internal/worktree/git.go b/internal/worktree/git.go index 0a04311..6cdae62 100644 --- a/internal/worktree/git.go +++ b/internal/worktree/git.go @@ -89,3 +89,48 @@ func gitOutput(ctx context.Context, dir string, args ...string) (string, error) return strings.TrimSpace(string(out)), nil } + +// worktreeOwner returns the branch checked out at a worktree path, or an empty +// string when no worktree is registered there. +// +// This makes a collision actionable: the user is told which branch already owns +// the directory, rather than only that something does. +func worktreeOwner(ctx context.Context, repoDir, worktreePath string) (string, error) { + out, err := gitOutput(ctx, repoDir, "worktree", "list", "--porcelain") + if err != nil { + return "", fmt.Errorf("failed to list worktrees: %w", err) + } + + want, err := filepath.Abs(worktreePath) + if err != nil { + return "", fmt.Errorf("failed to resolve %s: %w", worktreePath, err) + } + + want = resolveSymlinks(want) + + var current string + + for _, line := range strings.Split(out, "\n") { + switch { + case strings.HasPrefix(line, "worktree "): + current = resolveSymlinks(strings.TrimPrefix(line, "worktree ")) + + case strings.HasPrefix(line, "branch ") && current == want: + // Reported as a full ref, e.g. refs/heads/dev/xfs/feature. + return strings.TrimPrefix(strings.TrimPrefix(line, "branch "), "refs/heads/"), nil + } + } + + return "", nil +} + +// resolveSymlinks resolves a path for comparison, falling back to the cleaned +// path when it cannot be resolved. Temporary directories on macOS are symlinked +// via /var, so comparing unresolved paths gives false mismatches. +func resolveSymlinks(path string) string { + if resolved, err := filepath.EvalSymlinks(path); err == nil { + return filepath.Clean(resolved) + } + + return filepath.Clean(path) +} diff --git a/internal/worktree/naming_test.go b/internal/worktree/naming_test.go new file mode 100644 index 0000000..a11a61b --- /dev/null +++ b/internal/worktree/naming_test.go @@ -0,0 +1,125 @@ +package worktree + +import ( + "path/filepath" + "strings" + "testing" +) + +func TestBranchToDirNameUsesLastSegment(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + branch string + want string + }{ + {name: "conventional branch", branch: "dev/xfs/slack-unfurl", want: "slack-unfurl"}, + {name: "two segments", branch: "dev/feature", want: "feature"}, + {name: "single segment", branch: "feature", want: "feature"}, + {name: "trailing slash ignored", branch: "dev/feature/", want: "feature"}, + {name: "dots preserved", branch: "release/2.4.0", want: "2.4.0"}, + {name: "spaces become dashes", branch: "dev/my feature", want: "my-feature"}, + {name: "unsafe characters stripped", branch: "dev/feat:x*y", want: "feat-x-y"}, + {name: "traversal neutralised", branch: "../escape", want: "escape"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := BranchToDirName(tt.branch); got != tt.want { + t.Errorf("BranchToDirName(%q) = %q, want %q", tt.branch, got, tt.want) + } + }) + } +} + +// TestBranchToDirNameCollides documents the trade-off that last-segment naming +// accepts: different branches can want the same directory. Preflight rejects +// the second one rather than silently renaming it. +func TestBranchToDirNameCollides(t *testing.T) { + t.Parallel() + + a := BranchToDirName("dev/xfs/slack-unfurl") + b := BranchToDirName("dev/xf/slack-unfurl") + + if a != b { + t.Fatalf("expected these to collide, got %q and %q", a, b) + } +} + +// TestPreflightRejectsCollisionWithExistingBranch is the safeguard: a second +// branch wanting an occupied directory must be refused, and the error must name +// the branch already using it so the fix is obvious. +func TestPreflightRejectsCollisionWithExistingBranch(t *testing.T) { + repo := newXenForoRepo(t) + + if _, err := Create(t.Context(), Options{ + SourcePath: repo, + Branch: "dev/xfs/slack-unfurl", + }); err != nil { + t.Fatalf("Create: %v", err) + } + + err := Preflight(t.Context(), repo, "dev/xf/slack-unfurl") + if err == nil { + t.Fatal("expected a colliding branch to be rejected") + } + + msg := err.Error() + + if !strings.Contains(msg, "slack-unfurl") { + t.Errorf("error %q does not identify the directory in conflict", msg) + } + + if !strings.Contains(msg, "dev/xfs/slack-unfurl") { + t.Errorf("error %q does not name the branch already using it", msg) + } +} + +// TestPreflightAllowsDistinctLastSegments confirms the common case still works. +func TestPreflightAllowsDistinctLastSegments(t *testing.T) { + repo := newXenForoRepo(t) + + if _, err := Create(t.Context(), Options{SourcePath: repo, Branch: "dev/xfs/one"}); err != nil { + t.Fatalf("Create: %v", err) + } + + if err := Preflight(t.Context(), repo, "dev/xfs/two"); err != nil { + t.Errorf("distinct feature names must not conflict: %v", err) + } +} + +// TestWorktreeOwnerReportsTheBranchUsingADirectory covers the lookup that makes +// the rejection message actionable. +func TestWorktreeOwnerReportsTheBranchUsingADirectory(t *testing.T) { + repo := newXenForoRepo(t) + + result, err := Create(t.Context(), Options{SourcePath: repo, Branch: "dev/xfs/slack-unfurl"}) + if err != nil { + t.Fatalf("Create: %v", err) + } + + owner, err := worktreeOwner(t.Context(), repo, result.Path) + if err != nil { + t.Fatalf("worktreeOwner: %v", err) + } + + if owner != "dev/xfs/slack-unfurl" { + t.Errorf("owner = %q, want the branch that owns the worktree", owner) + } +} + +func TestWorktreeOwnerForUnknownPath(t *testing.T) { + repo := newXenForoRepo(t) + + owner, err := worktreeOwner(t.Context(), repo, filepath.Join(t.TempDir(), "absent")) + if err != nil { + t.Fatalf("worktreeOwner: %v", err) + } + + if owner != "" { + t.Errorf("owner = %q, want empty for an unknown path", owner) + } +} diff --git a/internal/worktree/paths.go b/internal/worktree/paths.go index 1bb607a..02c71dc 100644 --- a/internal/worktree/paths.go +++ b/internal/worktree/paths.go @@ -2,6 +2,7 @@ package worktree import ( + "fmt" "path/filepath" "regexp" "strings" @@ -18,15 +19,38 @@ var unsafeChars = regexp.MustCompile(`[^a-zA-Z0-9._-]+`) // BranchToDirName converts a branch name into a single, safe path segment. // -// Slashes become dashes, so dev/24x/feature becomes dev-24x-feature. The result -// is always a single segment: it can never contain a separator or resolve to a -// parent directory, whatever the branch name contains. +// Only the last segment is used, so dev/xfs/slack-unfurl becomes slack-unfurl. +// The prefix in a conventional branch name describes where the work belongs +// rather than what it is, and repeating it in directory names, URLs and Docker +// instance names makes all three harder to read. // -// Note this is lossy. dev/24x/feature and dev-24x-feature both map to -// dev-24x-feature, so callers must check for an existing directory rather than -// assume the name is unique. See CheckCollision. +// The result is always a single segment: it can never contain a separator or +// resolve to a parent directory, whatever the branch name contains. +// +// This is deliberately lossy. dev/xfs/slack-unfurl and dev/xf/slack-unfurl both +// yield slack-unfurl, so callers must check whether the directory is already +// taken. Preflight rejects a collision rather than renaming around it, which +// keeps the branch-to-path mapping computable without consulting any state. func BranchToDirName(branch string) string { - name := unsafeChars.ReplaceAllString(branch, "-") + // Take the last non-empty segment, so a trailing slash does not produce an + // empty name. + segments := strings.Split(branch, "/") + + last := "" + + for i := len(segments) - 1; i >= 0; i-- { + if strings.TrimSpace(segments[i]) != "" { + last = segments[i] + + break + } + } + + if last == "" { + last = branch + } + + name := unsafeChars.ReplaceAllString(last, "-") // Leading dots would create a hidden directory, and a name of only dots // would resolve to "." or "..". @@ -65,3 +89,17 @@ func WorktreesDir(sourcePath string) string { func ResolvePath(sourcePath, branch string) string { return filepath.Join(WorktreesDir(sourcePath), BranchToDirName(branch)) } + +// ResolveExistingPath is ResolvePath for branch names that came from the user. +// +// BranchToDirName yields an empty name for inputs such as "." or "..", which +// would resolve to the directory holding every worktree for the checkout. A +// command acting on that path would operate on all of them at once, so those +// inputs are rejected rather than resolved. +func ResolveExistingPath(sourcePath, branch string) (string, error) { + if BranchToDirName(branch) == "" { + return "", fmt.Errorf("%w: %q does not name a worktree", ErrInvalidBranch, branch) + } + + return ResolvePath(sourcePath, branch), nil +} diff --git a/internal/worktree/paths_test.go b/internal/worktree/paths_test.go index 91ef6f6..751b31b 100644 --- a/internal/worktree/paths_test.go +++ b/internal/worktree/paths_test.go @@ -14,13 +14,13 @@ func TestBranchToDirName(t *testing.T) { want string }{ {name: "simple", branch: "feature", want: "feature"}, - {name: "slashes become dashes", branch: "dev/24x/feature", want: "dev-24x-feature"}, + {name: "last segment used", branch: "dev/24x/feature", want: "feature"}, {name: "leading slash trimmed", branch: "/leading", want: "leading"}, {name: "trailing slash trimmed", branch: "trailing/", want: "trailing"}, - {name: "consecutive slashes collapse", branch: "a//b", want: "a-b"}, + {name: "consecutive slashes collapse", branch: "a//b", want: "b"}, {name: "spaces become dashes", branch: "my feature", want: "my-feature"}, - {name: "uppercase preserved", branch: "dev/MyAddon/Fix", want: "dev-MyAddon-Fix"}, - {name: "dots preserved", branch: "release/2.4.0", want: "release-2.4.0"}, + {name: "uppercase preserved", branch: "dev/MyAddon/Fix", want: "Fix"}, + {name: "dots preserved", branch: "release/2.4.0", want: "2.4.0"}, {name: "path traversal neutralised", branch: "../escape", want: "escape"}, {name: "unsafe characters stripped", branch: "feat:x*y?", want: "feat-x-y"}, } @@ -81,7 +81,7 @@ func TestResolvePath(t *testing.T) { t.Parallel() got := ResolvePath("/Users/x/Sites/main", "dev/24x/feature") - want := filepath.Join("/Users/x/Sites", "main.worktrees", "dev-24x-feature") + want := filepath.Join("/Users/x/Sites", "main.worktrees", "feature") if got != want { t.Errorf("ResolvePath = %q, want %q", got, want) diff --git a/internal/worktree/registry.go b/internal/worktree/registry.go index 13b062e..8c02ae1 100644 --- a/internal/worktree/registry.go +++ b/internal/worktree/registry.go @@ -2,13 +2,31 @@ package worktree import ( "encoding/json" + "errors" "fmt" + "io" "os" "path/filepath" "sync" "time" ) +// lockRetryInterval and lockTimeout bound how long a mutating call waits for +// another process to release the registry lock, so a crashed process cannot +// wedge every other xf invocation forever. +const ( + lockRetryInterval = 25 * time.Millisecond + lockTimeout = 5 * time.Second + lockStaleAfter = 30 * time.Second +) + +// ErrRegistryCorrupt indicates the registry file exists but cannot be parsed. +// +// Mutating calls tolerate this and rebuild from an empty registry, so a damaged +// file never blocks cleanup. Read failures are not tolerated: they mean the +// existing entries are unknown rather than absent. +var ErrRegistryCorrupt = errors.New("worktree registry is corrupt") + // Entry records a worktree created by xf. type Entry struct { // SourcePath is the checkout the worktree was created from. @@ -95,9 +113,20 @@ func (r *Registry) Add(entry Entry) error { r.mu.Lock() defer r.mu.Unlock() + unlock, err := r.lock() + if err != nil { + return err + } + defer unlock() + // A damaged registry must not block recording new work, so parse failures - // are treated as an empty registry and overwritten. - entries, _ := r.load() + // are treated as an empty registry and overwritten. A read failure is + // different: the entries are unreadable rather than absent, and saving + // over them would discard every other worktree's record. + entries, err := r.load() + if err != nil && !errors.Is(err, ErrRegistryCorrupt) { + return err + } want := filepath.Clean(entry.WorktreePath) replaced := false @@ -124,10 +153,23 @@ func (r *Registry) Remove(worktreePath string) error { r.mu.Lock() defer r.mu.Unlock() - entries, err := r.load() + unlock, err := r.lock() if err != nil { return err } + defer unlock() + + // A damaged registry must not block cleanup: worktree removal and prune + // have to be able to proceed even when the file cannot be parsed, so a + // parse failure is treated the same as an empty registry, as in Add. + // + // Only a parse failure. A permission or I/O error means the existing + // entries could not be read at all, and saving over them would delete + // every other worktree's record. + entries, err := r.load() + if err != nil && !errors.Is(err, ErrRegistryCorrupt) { + return err + } want := filepath.Clean(worktreePath) kept := make([]Entry, 0, len(entries)) @@ -141,6 +183,97 @@ func (r *Registry) Remove(worktreePath string) error { return r.save(kept) } +// lockPath returns the path of the cross-process lockfile guarding r.path. +func (r *Registry) lockPath() string { + return r.path + ".lock" +} + +// lock acquires a cross-process lock covering a load/modify/save transaction +// and returns a function that releases it. +// +// r.mu only guards one process's own goroutines; separate "xf worktree" +// invocations are separate processes that would otherwise read, modify and +// write the same JSON file with no coordination, silently losing whichever +// write happened first. A plain O_CREATE|O_EXCL lockfile is used rather than +// syscall.Flock so the same code works on Windows, where xf also builds. +func (r *Registry) lock() (func(), error) { + dir := filepath.Dir(r.path) + if err := os.MkdirAll(dir, 0o750); err != nil { + return nil, fmt.Errorf("failed to create registry directory: %w", err) + } + + path := r.lockPath() + deadline := time.Now().Add(lockTimeout) + + for { + f, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) + if err == nil { + token := fmt.Sprintf("%d.%d", os.Getpid(), time.Now().UnixNano()) + + _, _ = io.WriteString(f, token) + _ = f.Close() + + return func() { + // Released by renaming to a private name and deleting that, + // rather than reading the token and then removing the path: + // between those two steps the lock could be taken over as + // stale and recreated by another process, and the remove would + // delete a lock that is now theirs. + // + // Rename is atomic, so at most one process moves this file. If + // the content is not ours, it was taken over and is put back + // untouched. + release := fmt.Sprintf("%s.release.%d", path, os.Getpid()) + if err := os.Rename(path, release); err != nil { + return + } + + held, readErr := os.ReadFile(release) + if readErr == nil && string(held) == token { + _ = os.Remove(release) + + return + } + + // Someone else's lock: restore it. + _ = os.Rename(release, path) + }, nil + } + + if !os.IsExist(err) { + return nil, fmt.Errorf("failed to lock worktree registry: %w", err) + } + + // A lockfile left behind by a process that crashed before releasing it + // would otherwise wedge every future call, so a lock older than + // lockStaleAfter is treated as abandoned and cleared. + // + // The takeover renames rather than removes: rename is atomic, so of + // several processes that all see the same stale lock, only the one + // whose rename succeeds clears it. Removing directly is a + // check-then-act race in which two processes can each delete the + // other's fresh lock and both believe they hold it. + // + // A failed takeover falls through to the deadline check and the sleep + // rather than retrying immediately: a lockfile that cannot be removed, + // on a read-only parent for instance, would otherwise spin forever. + if info, statErr := os.Stat(path); statErr == nil && time.Since(info.ModTime()) > lockStaleAfter { + stale := fmt.Sprintf("%s.stale.%d", path, os.Getpid()) + if renameErr := os.Rename(path, stale); renameErr == nil { + _ = os.Remove(stale) + + continue + } + } + + if time.Now().After(deadline) { + return nil, fmt.Errorf("timed out waiting for worktree registry lock at %s", path) + } + + time.Sleep(lockRetryInterval) + } +} + func (r *Registry) load() ([]Entry, error) { data, err := os.ReadFile(r.path) if err != nil { @@ -153,7 +286,7 @@ func (r *Registry) load() ([]Entry, error) { var entries []Entry if err := json.Unmarshal(data, &entries); err != nil { - return nil, fmt.Errorf("failed to parse worktree registry at %s: %w", r.path, err) + return nil, fmt.Errorf("%w at %s: %w", ErrRegistryCorrupt, r.path, err) } return entries, nil diff --git a/internal/worktree/registry_test.go b/internal/worktree/registry_test.go index c534001..1de7f08 100644 --- a/internal/worktree/registry_test.go +++ b/internal/worktree/registry_test.go @@ -1,8 +1,11 @@ package worktree import ( + "fmt" "os" "path/filepath" + "runtime" + "sync" "testing" "time" ) @@ -84,6 +87,23 @@ func TestRegistryCorruptFileIsNotFatal(t *testing.T) { } } +// TestRegistryRemoveOverCorruptFileIsNotFatal covers the same tolerance as +// TestRegistryCorruptFileIsNotFatal, but for Remove: cleanup during +// "xf worktree remove" or prune must not fail just because the registry is +// unreadable. +func TestRegistryRemoveOverCorruptFileIsNotFatal(t *testing.T) { + path := filepath.Join(t.TempDir(), "corrupt.json") + if err := os.WriteFile(path, []byte("{not json"), 0o600); err != nil { + t.Fatalf("write: %v", err) + } + + reg := &Registry{path: path} + + if err := reg.Remove("/tmp/anything"); err != nil { + t.Fatalf("Remove over a corrupt registry: %v", err) + } +} + func TestRegistryRemove(t *testing.T) { reg := &Registry{path: filepath.Join(t.TempDir(), "worktrees.json")} @@ -137,6 +157,46 @@ func TestRegistryAddIsIdempotent(t *testing.T) { } } +// TestRegistryAddSurvivesConcurrentProcesses exercises the cross-process lock: +// separate Registry values sharing one file stand in for separate "xf +// worktree" invocations, which previously could read-modify-write the same +// JSON concurrently and lose each other's entries. +func TestRegistryAddSurvivesConcurrentProcesses(t *testing.T) { + path := filepath.Join(t.TempDir(), "worktrees.json") + + const writers = 8 + + var wg sync.WaitGroup + + for i := range writers { + wg.Add(1) + + go func(i int) { + defer wg.Done() + + reg := &Registry{path: path} + entry := Entry{WorktreePath: fmt.Sprintf("/tmp/wt-%d", i), Branch: fmt.Sprintf("b%d", i)} + + if err := reg.Add(entry); err != nil { + t.Errorf("Add from writer %d: %v", i, err) + } + }(i) + } + + wg.Wait() + + reg := &Registry{path: path} + + entries, err := reg.All() + if err != nil { + t.Fatalf("All: %v", err) + } + + if len(entries) != writers { + t.Errorf("got %d entries, want %d: entries were lost to a race", len(entries), writers) + } +} + func TestRegistryForSource(t *testing.T) { reg := &Registry{path: filepath.Join(t.TempDir(), "worktrees.json")} @@ -165,3 +225,52 @@ func TestRegistryForSource(t *testing.T) { t.Errorf("got %d entries for the source, want 2", len(entries)) } } + +// A registry that cannot be read is not an empty registry. Treating it as one +// would let the following save discard every entry it failed to read. +func TestRegistryDoesNotDiscardEntriesItCannotRead(t *testing.T) { + if runtime.GOOS == windowsOS { + t.Skip("unreadable-file permissions are not enforced the same way on Windows") + } + + if os.Geteuid() == 0 { + t.Skip("root bypasses file permissions") + } + + dir := t.TempDir() + path := filepath.Join(dir, "worktrees.json") + + reg := &Registry{path: path} + + if err := reg.Add(Entry{WorktreePath: "/src.worktrees/keep", Branch: "keep"}); err != nil { + t.Fatalf("seed Add: %v", err) + } + + if err := os.Chmod(path, 0o000); err != nil { + t.Fatalf("chmod: %v", err) + } + + t.Cleanup(func() { _ = os.Chmod(path, 0o600) }) + + if err := reg.Add(Entry{WorktreePath: "/src.worktrees/new", Branch: "new"}); err == nil { + t.Error("Add over an unreadable registry should fail rather than overwrite it") + } + + if err := reg.Remove("/src.worktrees/keep"); err == nil { + t.Error("Remove over an unreadable registry should fail rather than overwrite it") + } + + // The original entry must still be there once the file is readable again. + if err := os.Chmod(path, 0o600); err != nil { + t.Fatalf("chmod back: %v", err) + } + + entries, err := reg.All() + if err != nil { + t.Fatalf("All: %v", err) + } + + if len(entries) != 1 || entries[0].Branch != "keep" { + t.Errorf("entries = %+v, want the seeded entry preserved", entries) + } +} diff --git a/internal/worktree/umask_other_test.go b/internal/worktree/umask_other_test.go new file mode 100644 index 0000000..050ce81 --- /dev/null +++ b/internal/worktree/umask_other_test.go @@ -0,0 +1,7 @@ +//go:build !(aix || darwin || dragonfly || freebsd || linux || netbsd || openbsd || solaris || (js && wasm)) + +package worktree + +// setUmask is a no-op on platforms without a umask. Tests that depend on mode +// bits skip themselves before calling it. +func setUmask(int) int { return 0 } diff --git a/internal/worktree/umask_unix_test.go b/internal/worktree/umask_unix_test.go new file mode 100644 index 0000000..35a94a7 --- /dev/null +++ b/internal/worktree/umask_unix_test.go @@ -0,0 +1,14 @@ +//go:build aix || darwin || dragonfly || freebsd || linux || netbsd || openbsd || solaris || (js && wasm) + +package worktree + +import "syscall" + +// setUmask sets the process umask and returns the previous value. +// +// The build constraint lists the platforms that provide syscall.Umask rather +// than using !windows: Plan 9 lacks it, so excluding only Windows would still +// fail to compile there. +func setUmask(mask int) int { + return syscall.Umask(mask) +} From 7c22db62749a6f9e6bbacf98786dc834be89e578 Mon Sep 17 00:00:00 2001 From: Chris Deeming Date: Tue, 18 Aug 2026 04:06:41 +0100 Subject: [PATCH 11/14] fix(worktree): point a cloned board at its own URL boardUrl lives in the database, so a cloned worktree inherited the source forum's address and generated links back to the installation it was copied from. Rewriting src/config.php is not an option: XenForo builds its options purely from the registry cache and offers no config-level override, so the value has to be updated in the database. OptionRepository::updateOptions is used rather than a direct UPDATE because it rebuilds the option cache as well. Writing the row alone would leave the old URL in service until something else happened to rebuild it, which is the kind of failure that looks like the change did not apply. The update runs through XenForo's own bootstrap, matching how xf:install sets the same option. Failure is reported as a warning rather than an error: the worktree is otherwise complete and usable, and the URL can be corrected in the control panel. --- cmd/xf/worktree_clone.go | 60 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/cmd/xf/worktree_clone.go b/cmd/xf/worktree_clone.go index 420ac5c..7114a83 100644 --- a/cmd/xf/worktree_clone.go +++ b/cmd/xf/worktree_clone.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "github.com/xenforo-ltd/cli/internal/dockercompose" "github.com/xenforo-ltd/cli/internal/ui" @@ -38,7 +39,64 @@ func cloneEnvironment(ctx context.Context, sourcePath, worktreePath string) erro return err } - return cloneFiles(ctx, sourcePath, worktreePath) + if err := cloneFiles(ctx, sourcePath, worktreePath); err != nil { + return err + } + + return retargetBoardURL(ctx, targetRunner) +} + +// retargetBoardURL points the cloned board at its own address. +// +// boardUrl is stored in the database, so a clone inherits the source's URL and +// generates links back to the forum it was copied from. XenForo has no config +// override for options, so the value must be updated in the database. +// +// OptionRepository::updateOptions is used rather than a direct UPDATE because +// it also rebuilds the option cache. Writing the row alone would leave the old +// URL in service until something else happened to rebuild it. +func retargetBoardURL(ctx context.Context, target *dockercompose.Runner) error { + url, err := target.GetURL(ctx) + if err != nil || url == "" { + ui.PrintWarning("Could not determine the worktree's URL; the board URL still points at the source") + + return nil + } + + spinner := ui.NewSpinner("Updating board URL...") + spinner.Start() + + // Run through XenForo's own bootstrap so the repository and cache rebuild + // behave exactly as they do for xf:install. + // php -r takes bare statements, without an opening tag. + script := fmt.Sprintf( + `require __DIR__ . '/src/XF.php';`+ + `XF::start(__DIR__);`+ + `$app = XF::setupApp(XF\App::class);`+ + `$app->repository(XF\Repository\OptionRepository::class)`+ + `->updateOptions(['boardUrl' => %s]);`, + phpQuote(url), + ) + + if err := target.PHP(ctx, "-r", script); err != nil { + spinner.Stop() + ui.PrintWarning(fmt.Sprintf("Could not update the board URL to %s: %v", url, err)) + ui.Println(" Set it in the admin control panel under Options > Basic board information.") + + return nil + } + + spinner.StopWithMessage("success", "Board URL set to "+url) + + return nil +} + +// phpQuote renders a string as a single-quoted PHP literal. +func phpQuote(value string) string { + escaped := strings.ReplaceAll(value, "\\", "\\\\") + escaped = strings.ReplaceAll(escaped, "'", "\\'") + + return "'" + escaped + "'" } // cloneDatabase streams a dump from the source instance into the target's. From 67f78911b9a375052647ef707323d68a3569fd1d Mon Sep 17 00:00:00 2001 From: Chris Deeming Date: Tue, 18 Aug 2026 04:26:57 +0100 Subject: [PATCH 12/14] feat(worktree): label a cloned board with the worktree name A clone inherits the source forum's title, so several worktrees were indistinguishable in a browser tab. The title now carries the worktree name: XenForo [main] cloned to a slack-unfurl worktree becomes XenForo [slack-unfurl]. An existing bracketed label is replaced rather than appended to, so cloning a clone does not accumulate suffixes. Only a label at the very end is treated as one: a title such as "XenForo [beta] forums" keeps its brackets and gains a new label. The title is derived inside PHP, since it depends on the option value as it stands after the import. retitleBoard mirrors that expression in Go so the behaviour is covered by tests; both were checked against the same cases, including nested brackets. Title and URL are set in one call so the option cache rebuilds once. --- cmd/xf/worktree_clone.go | 74 +++++++++++++++++++++------ cmd/xf/worktree_title_test.go | 86 ++++++++++++++++++++++++++++++++ internal/dockercompose/runner.go | 33 +++++++++++- 3 files changed, 175 insertions(+), 18 deletions(-) create mode 100644 cmd/xf/worktree_title_test.go diff --git a/cmd/xf/worktree_clone.go b/cmd/xf/worktree_clone.go index 7114a83..004741b 100644 --- a/cmd/xf/worktree_clone.go +++ b/cmd/xf/worktree_clone.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "strings" "github.com/xenforo-ltd/cli/internal/dockercompose" @@ -43,10 +44,11 @@ func cloneEnvironment(ctx context.Context, sourcePath, worktreePath string) erro return err } - return retargetBoardURL(ctx, targetRunner) + return retargetBoardIdentity(ctx, targetRunner, filepath.Base(worktreePath)) } -// retargetBoardURL points the cloned board at its own address. +// retargetBoardIdentity points the cloned board at its own address and marks +// its title with the worktree name. // // boardUrl is stored in the database, so a clone inherits the source's URL and // generates links back to the forum it was copied from. XenForo has no config @@ -55,7 +57,7 @@ func cloneEnvironment(ctx context.Context, sourcePath, worktreePath string) erro // OptionRepository::updateOptions is used rather than a direct UPDATE because // it also rebuilds the option cache. Writing the row alone would leave the old // URL in service until something else happened to rebuild it. -func retargetBoardURL(ctx context.Context, target *dockercompose.Runner) error { +func retargetBoardIdentity(ctx context.Context, target *dockercompose.Runner, label string) error { url, err := target.GetURL(ctx) if err != nil || url == "" { ui.PrintWarning("Could not determine the worktree's URL; the board URL still points at the source") @@ -63,19 +65,25 @@ func retargetBoardURL(ctx context.Context, target *dockercompose.Runner) error { return nil } - spinner := ui.NewSpinner("Updating board URL...") + spinner := ui.NewSpinner("Updating board URL and title...") spinner.Start() // Run through XenForo's own bootstrap so the repository and cache rebuild // behave exactly as they do for xf:install. // php -r takes bare statements, without an opening tag. + // Both options are set in one call so the option cache rebuilds once. The + // title is derived inside PHP because it depends on the current value, + // which is only known once XenForo has booted. script := fmt.Sprintf( `require __DIR__ . '/src/XF.php';`+ `XF::start(__DIR__);`+ `$app = XF::setupApp(XF\App::class);`+ + `$title = rtrim(preg_replace('/\s*\[[^\[\]]*\]\s*$/', '', $app->options()->boardTitle));`+ + `$title = $title === '' ? %[2]s : $title . ' ' . %[2]s;`+ `$app->repository(XF\Repository\OptionRepository::class)`+ - `->updateOptions(['boardUrl' => %s]);`, + `->updateOptions(['boardUrl' => %[1]s, 'boardTitle' => $title]);`, phpQuote(url), + phpQuote("["+label+"]"), ) if err := target.PHP(ctx, "-r", script); err != nil { @@ -104,34 +112,41 @@ func cloneDatabase(ctx context.Context, source, target *dockercompose.Runner) er user, password := source.DatabaseCredentials() database := source.DatabaseName() - dumpPath := filepath.Join(os.TempDir(), "xf-clone-"+target.Instance()+".sql") - - defer func() { - _ = os.Remove(dumpPath) - }() - spinner := ui.NewSpinner("Exporting database from source...") spinner.Start() - dump, err := os.Create(dumpPath) + // CreateTemp generates an unpredictable name and creates the file mode + // 0600. A fixed path in the shared temp directory would leave the whole + // forum database, including password hashes, readable by other users on + // the host, and would let them pre-create the path as a symlink. + dump, err := os.CreateTemp("", "xf-clone-"+target.Instance()+"-*.sql") if err != nil { spinner.StopWithMessage("error", "Failed to export database") return fmt.Errorf("failed to create dump file: %w", err) } + dumpPath := dump.Name() + + defer func() { + _ = os.Remove(dumpPath) + }() + + // The password goes in the environment: an argument would be visible to + // anything that can list processes in the container. + dumpEnv := map[string]string{"MYSQL_PWD": password} + // --single-transaction keeps the source usable during the dump. dumpCmd := []string{ "mariadb-dump", "--user=" + user, - "--password=" + password, "--single-transaction", "--routines", "--events", database, } - if err := source.ExecCapture(ctx, "mysql", dump, dumpCmd...); err != nil { + if err := source.ExecCaptureWithEnv(ctx, "mysql", dumpEnv, dump, dumpCmd...); err != nil { _ = dump.Close() spinner.StopWithMessage("error", "Failed to export database") @@ -169,14 +184,15 @@ func cloneDatabase(ctx context.Context, source, target *dockercompose.Runner) er targetUser, targetPassword := target.DatabaseCredentials() + importEnv := map[string]string{"MYSQL_PWD": targetPassword} + importCmd := []string{ "mariadb", "--user=" + targetUser, - "--password=" + targetPassword, target.DatabaseName(), } - if err := target.ExecInput(ctx, "mysql", restore, importCmd...); err != nil { + if err := target.ExecInputWithEnv(ctx, "mysql", importEnv, restore, importCmd...); err != nil { spinner.StopWithMessage("error", "Failed to import database") return fmt.Errorf("failed to import the database: %w", err) @@ -224,3 +240,29 @@ func cloneFiles(ctx context.Context, sourcePath, worktreePath string) error { // progressUpdateInterval is how many files to copy between progress updates. const progressUpdateInterval = 100 + +// retitleBoard appends a worktree label to a board title, replacing any label +// already present. +// +// A clone inherits the source forum's title, so several worktrees would +// otherwise be indistinguishable in a browser tab. +// +// This mirrors the expression used in retargetBoardIdentity, which has to run +// inside PHP because it depends on the live option value. It exists separately +// so the behaviour can be tested directly. +func retitleBoard(title, label string) string { + trimmed := strings.TrimRight(trailingLabel.ReplaceAllString(title, ""), " \t") + + suffix := "[" + label + "]" + + if trimmed == "" { + return suffix + } + + return trimmed + " " + suffix +} + +// trailingLabel matches a bracketed label at the end of a board title. Nested +// brackets are excluded so a title ending in "[a [b]]" is left alone rather +// than partly consumed. +var trailingLabel = regexp.MustCompile(`\s*\[[^\[\]]*\]\s*$`) diff --git a/cmd/xf/worktree_title_test.go b/cmd/xf/worktree_title_test.go new file mode 100644 index 0000000..3139ec5 --- /dev/null +++ b/cmd/xf/worktree_title_test.go @@ -0,0 +1,86 @@ +package main + +import "testing" + +func TestRetitleBoard(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + title string + label string + want string + }{ + { + name: "plain title gains a suffix", + title: "XenForo", + label: "slack-unfurl", + want: "XenForo [slack-unfurl]", + }, + { + name: "existing suffix is replaced", + title: "XenForo [main]", + label: "slack-unfurl", + want: "XenForo [slack-unfurl]", + }, + { + name: "version-style suffix is replaced", + title: "XenForo [2.4]", + label: "slack-unfurl", + want: "XenForo [slack-unfurl]", + }, + { + name: "brackets elsewhere are left alone", + title: "XenForo [beta] forums", + label: "feature", + want: "XenForo [beta] forums [feature]", + }, + { + name: "trailing whitespace is tidied", + title: "XenForo ", + label: "feature", + want: "XenForo [feature]", + }, + { + name: "empty title becomes just the label", + title: "", + label: "feature", + want: "[feature]", + }, + { + name: "empty suffix is replaced rather than kept", + title: "XenForo []", + label: "feature", + want: "XenForo [feature]", + }, + { + name: "nested brackets are not mangled", + title: "XenForo [a [b]]", + label: "feature", + want: "XenForo [a [b]] [feature]", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := retitleBoard(tt.title, tt.label); got != tt.want { + t.Errorf("retitleBoard(%q, %q) = %q, want %q", tt.title, tt.label, got, tt.want) + } + }) + } +} + +// TestRetitleBoardIsIdempotent matters because cloning a clone must not +// accumulate suffixes. +func TestRetitleBoardIsIdempotent(t *testing.T) { + t.Parallel() + + once := retitleBoard("XenForo [main]", "feature") + twice := retitleBoard(once, "feature") + + if once != twice { + t.Errorf("applying twice changed the result: %q then %q", once, twice) + } +} diff --git a/internal/dockercompose/runner.go b/internal/dockercompose/runner.go index ff6a107..7b18756 100644 --- a/internal/dockercompose/runner.go +++ b/internal/dockercompose/runner.go @@ -125,8 +125,23 @@ func (r *Runner) UpWithOutput(ctx context.Context, detach bool, stdout, stderr i // Output is streamed rather than buffered so that large results, such as a // database dump, do not have to fit in memory. func (r *Runner) ExecCapture(ctx context.Context, service string, stdout io.Writer, cmd ...string) error { + return r.ExecCaptureWithEnv(ctx, service, nil, stdout, cmd...) +} + +// ExecCaptureWithEnv is ExecCapture with environment variables set inside the +// container. Secrets belong here rather than in cmd: a value passed as an +// argument is visible in the container's process list. +func (r *Runner) ExecCaptureWithEnv( + ctx context.Context, + service string, + env map[string]string, + stdout io.Writer, + cmd ...string, +) error { args := r.buildComposeArgs() - args = append(args, "exec", "-T", service) + args = append(args, "exec", "-T") + args = r.appendEnvVars(args, env, "-e") + args = append(args, service) args = append(args, cmd...) return r.runDockerCommandWithIO(ctx, nil, stdout, os.Stderr, args...) @@ -134,8 +149,22 @@ func (r *Runner) ExecCapture(ctx context.Context, service string, stdout io.Writ // ExecInput runs a command in a service, feeding it from stdin. func (r *Runner) ExecInput(ctx context.Context, service string, stdin io.Reader, cmd ...string) error { + return r.ExecInputWithEnv(ctx, service, nil, stdin, cmd...) +} + +// ExecInputWithEnv is ExecInput with environment variables set inside the +// container, so secrets stay out of the container's process list. +func (r *Runner) ExecInputWithEnv( + ctx context.Context, + service string, + env map[string]string, + stdin io.Reader, + cmd ...string, +) error { args := r.buildComposeArgs() - args = append(args, "exec", "-T", service) + args = append(args, "exec", "-T") + args = r.appendEnvVars(args, env, "-e") + args = append(args, service) args = append(args, cmd...) return r.runDockerCommandWithIO(ctx, stdin, os.Stdout, os.Stderr, args...) From 127781046fa56f8660336fb9fcdbecc0f3192340 Mon Sep 17 00:00:00 2001 From: Chris Deeming Date: Wed, 19 Aug 2026 13:19:15 +0100 Subject: [PATCH 13/14] docs(readme): document worktrees and automatic Composer install --- README.md | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/README.md b/README.md index c3f58b2..0b69de1 100644 --- a/README.md +++ b/README.md @@ -145,6 +145,10 @@ xf init ./my-project \ xf init ./existing-xf-project --existing xf init ./existing-xf-project --existing --up +# Composer dependencies are installed automatically when the target +# tracks a composer.json (repository checkouts). Release packages ship +# vendor/ prebuilt and are skipped. + # .env overrides (file + inline; inline wins) xf init ./my-project \ --env-file ./my.env \ @@ -215,6 +219,41 @@ xf compose exec xf mysql -u root xf exec xf ls -la ``` +### Worktrees + +A worktree is a second checkout of the same repository on its own branch, with +its own Docker containers and database. Worktrees are created alongside the +source checkout: `~/Sites/main` gains `~/Sites/main.worktrees/`, named +after the branch's last segment. + +By default `create` clones the source environment — database, `data/` and +`internal_data/` — and points the cloned board at its own URL, labelling its +title with the worktree name. + +```bash +# Create a worktree and set up its environment +xf worktree create dev/24x/feature + +# Branch from somewhere other than the current HEAD +xf worktree create dev/24x/feature --base main + +# Create the worktree without setting anything up +xf worktree create dev/24x/feature --no-setup + +# List worktrees (this project / all known projects) +xf worktree list +xf worktree list-all + +# Print the path of a worktree (bare output, shell-substitution safe) +cd "$(xf worktree path dev/24x/feature)" + +# Remove a worktree and its containers and volumes +xf worktree remove dev/24x/feature + +# Drop registry entries for worktrees that no longer exist +xf worktree prune +``` + ### PHP / Composer / Debug ```bash From fe1ffde43dcb52917ff239deda2dd2f1c86e30fa Mon Sep 17 00:00:00 2001 From: Chris Deeming Date: Wed, 19 Aug 2026 13:56:08 +0100 Subject: [PATCH 14/14] fix(config): guard Init against concurrent callers config.Init configures viper's package-level singleton, which cobra's OnInitialize hook runs on every Execute. Parallel tests that each run the CLI therefore raced on that shared state, and the race detector failed every test that happened to share a goroutine with the winner. Serialize Init so the singleton is configured by one caller at a time. --- internal/config/config.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/internal/config/config.go b/internal/config/config.go index db51c53..d0e7bfe 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -15,6 +15,11 @@ var ( cacheOnce sync.Once cache Config errCache error + + // initMu guards Init, which configures viper's package-level singleton. + // Concurrent callers — notably parallel tests that each run the CLI — + // would otherwise race on that shared state. + initMu sync.Mutex ) // Config holds all CLI configuration values. @@ -69,6 +74,9 @@ func (cfg *OAuthConfig) Endpoints() *OAuthEndpoints { // Init sets up the configuration system and reads the config file if it exists. func Init(configFile string) error { + initMu.Lock() + defer initMu.Unlock() + if configFile != "" { viper.SetConfigFile(configFile) } else { @@ -111,6 +119,12 @@ func Init(configFile string) error { // Load reads the configuration from the config file. func Load() (Config, error) { cacheOnce.Do(func() { + // Unmarshal reads the same package-level viper instance that Init + // writes, so it takes the same lock: without it, a caller loading + // config while another initializes it races on that shared state. + initMu.Lock() + defer initMu.Unlock() + if err := viper.Unmarshal(&cache); err != nil { errCache = fmt.Errorf("failed to unmarshal config: %w", err) }