From 46eda77205d1c0ca3295f626d79e0f8fb46eae0e Mon Sep 17 00:00:00 2001 From: Jonatan Dahl Date: Mon, 16 Mar 2026 10:58:46 -0400 Subject: [PATCH 1/2] fix(sync): don't treat branches with no commits as merged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Branches with no unique commits point to the same commit as main, so IsAncestor() trivially returns true — falsely detecting them as merged and switching to main. Fix by adding a reverse ancestry check: if both directions are true, the commits are identical (new branch, not merged). Co-Authored-By: Claude Opus 4.6 --- cmd/sync.go | 37 ++++++++++++++++++++++--------------- cmd/sync_test.go | 3 +++ 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/cmd/sync.go b/cmd/sync.go index 3ac1e81..c324b6c 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -512,19 +512,23 @@ func runSync(gitClient git.GitClient, githubClient github.GitHubClient, syncRemo // No PR found - check if branch was merged via git history remoteBase := syncRemote + "/" + baseBranch if merged, err := gitClient.IsAncestor(branch.Name, remoteBase); err == nil && merged { - fmt.Printf("%s Skipping %s (merged into %s, detected via git history)...\n", progress, ui.Branch(branch.Name), ui.Branch(baseBranch)) - fmt.Printf(" Removing from stack tracking...\n") - configKey := fmt.Sprintf("branch.%s.stackparent", branch.Name) - if err := gitClient.UnsetConfig(configKey); err != nil { - fmt.Fprintf(os.Stderr, " Warning: failed to remove stack config: %v\n", err) - } else { - fmt.Printf(" %s Removed. You can delete this branch with: %s\n", ui.SuccessIcon(), ui.Command(fmt.Sprintf("git branch -d %s", branch.Name))) - } - if branch.Name == originalBranch { - originalBranchMerged = true + // Also check reverse: if remote base is ancestor of branch, they point to + // the same commit — this is a new branch with no commits, not a merged one + if sameCommit, err2 := gitClient.IsAncestor(remoteBase, branch.Name); err2 != nil || !sameCommit { + fmt.Printf("%s Skipping %s (merged into %s, detected via git history)...\n", progress, ui.Branch(branch.Name), ui.Branch(baseBranch)) + fmt.Printf(" Removing from stack tracking...\n") + configKey := fmt.Sprintf("branch.%s.stackparent", branch.Name) + if err := gitClient.UnsetConfig(configKey); err != nil { + fmt.Fprintf(os.Stderr, " Warning: failed to remove stack config: %v\n", err) + } else { + fmt.Printf(" %s Removed. You can delete this branch with: %s\n", ui.SuccessIcon(), ui.Command(fmt.Sprintf("git branch -d %s", branch.Name))) + } + if branch.Name == originalBranch { + originalBranchMerged = true + } + fmt.Println() + continue } - fmt.Println() - continue } } @@ -541,9 +545,12 @@ func runSync(gitClient git.GitClient, githubClient github.GitHubClient, syncRemo // No PR found for parent - check if parent was merged via git history remoteBase := syncRemote + "/" + baseBranch if merged, err := gitClient.IsAncestor(branch.Parent, remoteBase); err == nil && merged { - fmt.Printf(" Parent %s appears merged into %s (detected via git history)\n", ui.Branch(branch.Parent), ui.Branch(baseBranch)) - oldParent = branch.Parent - parentMergedViaGit = true + // Also check reverse: if same commit, parent is just a new branch, not merged + if sameCommit, err2 := gitClient.IsAncestor(remoteBase, branch.Parent); err2 != nil || !sameCommit { + fmt.Printf(" Parent %s appears merged into %s (detected via git history)\n", ui.Branch(branch.Parent), ui.Branch(baseBranch)) + oldParent = branch.Parent + parentMergedViaGit = true + } } } diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 17bafee..2f4baba 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -1236,6 +1236,8 @@ func TestRunSyncGitBasedMergeDetection(t *testing.T) { // feature-a: no PR, but IsAncestor returns true → merged via git history mockGit.On("IsAncestor", "feature-a", "origin/main").Return(true, nil) + // Reverse check: origin/main is NOT ancestor of feature-a (branch has diverged, truly merged) + mockGit.On("IsAncestor", "origin/main", "feature-a").Return(false, nil) // Remove feature-a from stack mockGit.On("UnsetConfig", "branch.feature-a.stackparent").Return(nil) @@ -1244,6 +1246,7 @@ func TestRunSyncGitBasedMergeDetection(t *testing.T) { // feature-b's parent (feature-a) has no PR, parent merged via git // IsAncestor("feature-a", "origin/main") already mocked above → true + // IsAncestor("origin/main", "feature-a") already mocked above → false (truly merged) // Reparent feature-b from feature-a to main mockGit.On("GetConfig", "branch.feature-a.stackparent").Return("main") From 8035eac8326ecb25e4febb2e543f67df463e8201 Mon Sep 17 00:00:00 2001 From: Jonatan Dahl Date: Mon, 16 Mar 2026 11:05:08 -0400 Subject: [PATCH 2/2] fix(sync): improve reverse ancestry check and add test for no-commits case Co-Authored-By: Claude Opus 4.6 --- cmd/sync.go | 53 +++++++++++++++++++++++------------------- cmd/sync_test.go | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 24 deletions(-) diff --git a/cmd/sync.go b/cmd/sync.go index c324b6c..adc15d4 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -511,24 +511,27 @@ func runSync(gitClient git.GitClient, githubClient github.GitHubClient, syncRemo } else if _, exists := prCache[branch.Name]; !exists { // No PR found - check if branch was merged via git history remoteBase := syncRemote + "/" + baseBranch - if merged, err := gitClient.IsAncestor(branch.Name, remoteBase); err == nil && merged { - // Also check reverse: if remote base is ancestor of branch, they point to - // the same commit — this is a new branch with no commits, not a merged one - if sameCommit, err2 := gitClient.IsAncestor(remoteBase, branch.Name); err2 != nil || !sameCommit { - fmt.Printf("%s Skipping %s (merged into %s, detected via git history)...\n", progress, ui.Branch(branch.Name), ui.Branch(baseBranch)) - fmt.Printf(" Removing from stack tracking...\n") - configKey := fmt.Sprintf("branch.%s.stackparent", branch.Name) - if err := gitClient.UnsetConfig(configKey); err != nil { - fmt.Fprintf(os.Stderr, " Warning: failed to remove stack config: %v\n", err) - } else { - fmt.Printf(" %s Removed. You can delete this branch with: %s\n", ui.SuccessIcon(), ui.Command(fmt.Sprintf("git branch -d %s", branch.Name))) - } - if branch.Name == originalBranch { - originalBranchMerged = true - } - fmt.Println() - continue + merged, err := gitClient.IsAncestor(branch.Name, remoteBase) + // If branch is ancestor of remote base, also check reverse: if both are ancestors + // of each other, they point to the same commit — a new branch with no commits, not merged + sameCommit := false + if err == nil && merged { + sameCommit, _ = gitClient.IsAncestor(remoteBase, branch.Name) + } + if err == nil && merged && !sameCommit { + fmt.Printf("%s Skipping %s (merged into %s, detected via git history)...\n", progress, ui.Branch(branch.Name), ui.Branch(baseBranch)) + fmt.Printf(" Removing from stack tracking...\n") + configKey := fmt.Sprintf("branch.%s.stackparent", branch.Name) + if err := gitClient.UnsetConfig(configKey); err != nil { + fmt.Fprintf(os.Stderr, " Warning: failed to remove stack config: %v\n", err) + } else { + fmt.Printf(" %s Removed. You can delete this branch with: %s\n", ui.SuccessIcon(), ui.Command(fmt.Sprintf("git branch -d %s", branch.Name))) } + if branch.Name == originalBranch { + originalBranchMerged = true + } + fmt.Println() + continue } } @@ -544,13 +547,15 @@ func runSync(gitClient git.GitClient, githubClient github.GitHubClient, syncRemo } else if parentPR == nil && branch.Parent != baseBranch { // No PR found for parent - check if parent was merged via git history remoteBase := syncRemote + "/" + baseBranch - if merged, err := gitClient.IsAncestor(branch.Parent, remoteBase); err == nil && merged { - // Also check reverse: if same commit, parent is just a new branch, not merged - if sameCommit, err2 := gitClient.IsAncestor(remoteBase, branch.Parent); err2 != nil || !sameCommit { - fmt.Printf(" Parent %s appears merged into %s (detected via git history)\n", ui.Branch(branch.Parent), ui.Branch(baseBranch)) - oldParent = branch.Parent - parentMergedViaGit = true - } + merged, err := gitClient.IsAncestor(branch.Parent, remoteBase) + sameCommit := false + if err == nil && merged { + sameCommit, _ = gitClient.IsAncestor(remoteBase, branch.Parent) + } + if err == nil && merged && !sameCommit { + fmt.Printf(" Parent %s appears merged into %s (detected via git history)\n", ui.Branch(branch.Parent), ui.Branch(baseBranch)) + oldParent = branch.Parent + parentMergedViaGit = true } } diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 2f4baba..081d2d1 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -1278,6 +1278,66 @@ func TestRunSyncGitBasedMergeDetection(t *testing.T) { mockGH.AssertExpectations(t) }) + t.Run("branch with no commits is not treated as merged", func(t *testing.T) { + mockGit := new(testutil.MockGitClient) + mockGH := new(testutil.MockGitHubClient) + + // Setup: no existing sync state + mockGit.On("GetConfig", "stack.sync.stashed").Return("") + mockGit.On("GetConfig", "stack.sync.originalBranch").Return("") + mockGit.On("GetCurrentBranch").Return("feature-a", nil) + mockGit.On("SetConfig", "stack.sync.originalBranch", "feature-a").Return(nil) + mockGit.On("IsWorkingTreeClean").Return(true, nil) + mockGit.On("GetConfig", "branch.feature-a.stackparent").Return("main") + mockGit.On("GetConfig", "stack.baseBranch").Return("").Maybe() + mockGit.On("GetDefaultBranch").Return("main").Maybe() + + stackParents := map[string]string{ + "feature-a": "main", + } + mockGit.On("GetAllStackParents").Return(stackParents, nil).Maybe() + + // No PRs found + mockGit.On("FetchRemote", "origin").Return(nil) + mockGH.On("GetPRsForBranches", mock.Anything).Return(make(map[string]*github.PRInfo)) + + 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-a has no commits: both IsAncestor directions return true (same commit) + mockGit.On("IsAncestor", "feature-a", "origin/main").Return(true, nil) + mockGit.On("IsAncestor", "origin/main", "feature-a").Return(true, nil) + + // Branch should NOT be removed — should proceed to normal processing + mockGit.On("CheckoutBranch", "feature-a").Return(nil) + mockGit.On("GetCommitHash", "feature-a").Return("main123", nil) + mockGit.On("GetCommitHash", "origin/feature-a").Return("main123", nil) + mockGit.On("FetchBranchFromRemote", "origin", "main").Return(nil) + mockGit.On("GetUniqueCommitsByPatch", "origin/main", "feature-a").Return([]string{}, nil) + mockGit.On("Rebase", "origin/main").Return(nil) + mockGit.On("FetchBranch", "feature-a").Return(nil) + mockGit.On("PushWithExpectedRemote", "feature-a", "main123").Return(nil) + + // Return to original branch + mockGit.On("CheckoutBranch", "feature-a").Return(nil) + // Clean up sync state + mockGit.On("UnsetConfig", "stack.sync.stashed").Return(nil) + mockGit.On("UnsetConfig", "stack.sync.originalBranch").Return(nil) + mockGit.On("GetConfig", "stack.postSyncInstall").Return("false").Maybe() + + err := runSync(mockGit, mockGH, "origin") + + assert.NoError(t, err) + // Verify branch was NOT removed from stack tracking + mockGit.AssertNotCalled(t, "UnsetConfig", "branch.feature-a.stackparent") + mockGit.AssertExpectations(t) + mockGH.AssertExpectations(t) + }) + t.Run("branch not merged via git is processed normally", func(t *testing.T) { mockGit := new(testutil.MockGitClient) mockGH := new(testutil.MockGitHubClient)