From 98cbc067390dbdd4911da4fedb478bbf4635e4a7 Mon Sep 17 00:00:00 2001 From: Jonatan Dahl Date: Wed, 21 Jan 2026 14:43:49 -0500 Subject: [PATCH] fix: auto-configure missing stackparent for branches in stack chain When a branch in the stack chain doesn't have stackparent configured (e.g., created manually without `stack new`), sync now: - Detects the missing config by comparing chain vs stackBranches - Infers the parent from chain order - Auto-configures stackparent so the branch gets synced - Notifies user with "Auto-configured X with parent Y" This fixes sync silently skipping parent branches and rebasing onto stale remote versions instead of freshly-rebased local versions. Co-Authored-By: Claude Opus 4.5 --- CLAUDE.md | 5 ++-- cmd/sync.go | 50 ++++++++++++++++++++++++++++--- cmd/sync_test.go | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 127 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f5a18f7..a99ba33 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -95,10 +95,11 @@ git config stack.baseBranch develop # Default is "main" ## Testing -Currently no test files exist. When adding tests: +**Always use `./scripts/test` to run tests** (handles CGO_ENABLED=0 for macOS compatibility). +Test patterns: - Use table-driven tests for topological sort and tree building -- Mock git/gh command execution for unit tests +- Mock git/gh command execution for unit tests using `testutil.MockGitClient` and `testutil.MockGitHubClient` - Consider integration tests that use temporary git repos **IMPORTANT**: When testing git operations (creating branches, stashing, etc.), always use `./tests/test-repo` directory, NOT the main repository. This keeps the main repo clean and prevents pollution from test branches. diff --git a/cmd/sync.go b/cmd/sync.go index 0d32d60..db0dc0d 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -335,6 +335,48 @@ func runSync(gitClient git.GitClient, githubClient github.GitHubClient) error { } } + // Detect branches in chain that don't have stackparent configured + // and auto-configure them with inferred parents + existingBranchNames := make(map[string]bool) + for _, b := range stackBranches { + existingBranchNames[b.Name] = true + } + + // Walk the chain and add missing branches + for i, branchName := range chain { + if branchName == baseBranch { + continue // Skip base branch + } + if existingBranchNames[branchName] { + continue // Already in stackBranches + } + + // Infer parent from chain (previous branch in the chain) + var inferredParent string + if i > 0 { + inferredParent = chain[i-1] + } else { + inferredParent = baseBranch + } + + // Check if branch exists locally before adding + if gitClient.BranchExists(branchName) { + stackBranches = append(stackBranches, stack.StackBranch{ + Name: branchName, + Parent: inferredParent, + }) + existingBranchNames[branchName] = true + + // Configure stackparent so future syncs work correctly + configKey := fmt.Sprintf("branch.%s.stackparent", branchName) + if err := gitClient.SetConfig(configKey, inferredParent); err != nil { + fmt.Fprintf(os.Stderr, "Warning: failed to set stackparent for %s: %v\n", branchName, err) + } else { + fmt.Printf("Auto-configured %s with parent %s\n", branchName, inferredParent) + } + } + } + // Sort branches in topological order (bottom to top) sorted, err := stack.TopologicalSort(stackBranches) if err != nil { @@ -438,7 +480,7 @@ func runSync(gitClient git.GitClient, githubClient github.GitHubClient) error { grandparent = stack.GetBaseBranch(gitClient) } - fmt.Printf(" Updating parent from %s to %s\n", branch.Parent, grandparent) + fmt.Printf(" ✓ Updated parent from %s to %s\n", branch.Parent, grandparent) configKey := fmt.Sprintf("branch.%s.stackparent", branch.Name) if err := gitClient.SetConfig(configKey, grandparent); err != nil { fmt.Fprintf(os.Stderr, " Warning: failed to update parent config: %v\n", err) @@ -668,7 +710,7 @@ func runSync(gitClient git.GitClient, githubClient github.GitHubClient) error { return fmt.Errorf("failed to restore stackparent config: %w", err) } - fmt.Printf("✓ Rebuilt %s (backup saved as %s)\n", branch.Name, backupBranch) + fmt.Printf(" ✓ Rebuilt %s (backup saved as %s)\n", branch.Name, backupBranch) fmt.Printf(" To delete backup later: git branch -D %s\n", backupBranch) // Branch is now clean - no need to rebase, just return nil @@ -788,10 +830,10 @@ func runSync(gitClient git.GitClient, githubClient github.GitHubClient) error { if err := githubClient.UpdatePRBase(pr.Number, branch.Parent); err != nil { fmt.Fprintf(os.Stderr, " Warning: failed to update PR base: %v\n", err) } else { - fmt.Printf("✓ PR #%d updated\n", pr.Number) + fmt.Printf(" ✓ PR #%d updated\n", pr.Number) } } else { - fmt.Printf("✓ PR #%d base is already correct (%s)\n", pr.Number, pr.Base) + fmt.Printf(" ✓ PR #%d base is already correct (%s)\n", pr.Number, pr.Base) } } else { fmt.Printf(" No PR found (create one with 'gh pr create')\n") diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 3f52931..16da740 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -664,6 +664,84 @@ func TestRunSyncResume(t *testing.T) { }) } +func TestRunSyncAutoConfiguresMissingStackparent(t *testing.T) { + testutil.SetupTest() + defer testutil.TeardownTest() + + t.Run("auto-configures parent branch missing stackparent", func(t *testing.T) { + mockGit := new(testutil.MockGitClient) + mockGH := new(testutil.MockGitHubClient) + + // Setup: feature-b has stackparent=feature-a, but feature-a has NO stackparent + mockGit.On("GetConfig", "stack.sync.stashed").Return("") + mockGit.On("GetConfig", "stack.sync.originalBranch").Return("") + mockGit.On("GetCurrentBranch").Return("feature-b", nil) + mockGit.On("SetConfig", "stack.sync.originalBranch", "feature-b").Return(nil) + mockGit.On("IsWorkingTreeClean").Return(true, nil) + mockGit.On("GetConfig", "branch.feature-b.stackparent").Return("feature-a") + mockGit.On("GetConfig", "stack.baseBranch").Return("").Maybe() + mockGit.On("GetDefaultBranch").Return("main").Maybe() + + // Key difference: feature-a is NOT in stackParents (no stackparent configured) + stackParents := map[string]string{ + "feature-b": "feature-a", // feature-a is missing! + } + mockGit.On("GetAllStackParents").Return(stackParents, nil).Maybe() + + // The fix should auto-configure feature-a with parent=main + mockGit.On("BranchExists", "feature-a").Return(true) + mockGit.On("SetConfig", "branch.feature-a.stackparent", "main").Return(nil) + + // Parallel operations + mockGit.On("Fetch").Return(nil) + mockGH.On("GetAllPRs").Return(make(map[string]*github.PRInfo), nil) + + // Worktree checks + mockGit.On("GetWorktreeBranches").Return(make(map[string]string), nil) + mockGit.On("GetCurrentWorktreePath").Return("/Users/test/repo", nil) + mockGit.On("GetRemoteBranchesSet").Return(map[string]bool{ + "main": true, + "feature-a": true, + "feature-b": true, + }) + + // Process feature-a first (auto-configured) + mockGit.On("CheckoutBranch", "feature-a").Return(nil) + mockGit.On("GetCommitHash", "feature-a").Return("abc123", nil) + mockGit.On("GetCommitHash", "origin/feature-a").Return("abc123", nil) + mockGit.On("FetchBranch", "main").Return(nil) + mockGit.On("GetUniqueCommitsByPatch", "origin/main", "feature-a").Return([]string{"abc123"}, nil) + mockGit.On("GetMergeBase", "feature-a", "origin/main").Return("main123", nil) + mockGit.On("GetCommitHash", "origin/main").Return("main123", nil) + mockGit.On("Rebase", "origin/main").Return(nil) + mockGit.On("FetchBranch", "feature-a").Return(nil) + mockGit.On("PushWithExpectedRemote", "feature-a", "abc123").Return(nil) + + // Process feature-b second + mockGit.On("CheckoutBranch", "feature-b").Return(nil) + mockGit.On("GetCommitHash", "feature-b").Return("def456", nil) + mockGit.On("GetCommitHash", "origin/feature-b").Return("def456", nil) + mockGit.On("GetUniqueCommitsByPatch", "feature-a", "feature-b").Return([]string{"def456"}, nil) + mockGit.On("GetMergeBase", "feature-b", "feature-a").Return("abc123", nil) + mockGit.On("GetCommitHash", "feature-a").Return("abc123", nil) + mockGit.On("Rebase", "feature-a").Return(nil) + mockGit.On("FetchBranch", "feature-b").Return(nil) + mockGit.On("PushWithExpectedRemote", "feature-b", "def456").Return(nil) + + // Return to original branch + mockGit.On("CheckoutBranch", "feature-b").Return(nil) + // Clean up sync state + mockGit.On("UnsetConfig", "stack.sync.stashed").Return(nil) + mockGit.On("UnsetConfig", "stack.sync.originalBranch").Return(nil) + + err := runSync(mockGit, mockGH) + + assert.NoError(t, err) + mockGit.AssertExpectations(t) + mockGH.AssertExpectations(t) + }) +} + func TestRunSyncAbort(t *testing.T) { testutil.SetupTest() defer testutil.TeardownTest()