Skip to content

fix sync --all failing when deleted branch exists in tree #685

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
willschnicke-silkline wants to merge 1 commit into
base: master
Choose a base branch
from
+50 −2
Open

fix sync --all failing when deleted branch exists in tree #685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions e2e_tests/sync_all_deleted_branch_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package e2e_tests

import (
"testing"

"github.com/aviator-co/av/internal/git/gittest"
)

func TestSyncAllWithDeletedBranch(t *testing.T) {
server := RunMockGitHubServer(t)
defer server.Close()
repo := gittest.NewTempRepoWithGitHubServer(t, server.URL)
Chdir(t, repo.RepoDir)

// Create two independent stacks off main.
repo.Git(t, "switch", "main")
RequireAv(t, "branch", "stack-1")
repo.CommitFile(t, "my-file", "1a\n", gittest.WithMessage("Commit 1a"))

repo.Git(t, "switch", "main")
RequireAv(t, "branch", "stack-2")
repo.CommitFile(t, "my-file-2", "2a\n", gittest.WithMessage("Commit 2a"))

// Delete stack-1's git ref without tidying av metadata.
// This simulates a branch deleted externally (e.g., on GitHub) but not yet
// cleaned up in av's internal database.
repo.Git(t, "switch", "stack-2")
repo.Git(t, "branch", "-D", "stack-1")

// Push a new commit to main so there's something to sync.
repo.WithCheckoutBranch(t, "refs/heads/main", func() {
repo.CommitFile(t, "other-file", "X2\n", gittest.WithMessage("Commit X2"))
repo.Git(t, "push", "origin", "main")
})

// This should succeed, skipping the deleted branch rather than failing
// with "error: git merge-base: exit status 128".
RequireAv(t, "sync", "--all", "--rebase-to-trunk")

// stack-2 should have been synced onto the latest main.
repo.Git(t, "merge-base", "--is-ancestor", "main", "stack-2")
}
10 changes: 8 additions & 2 deletions internal/sequencer/planner/targets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,17 @@ func GetTargetBranches(
continue
}
if includeStackRoots {
ret = append(ret, plumbing.NewBranchReferenceName(br.Name))
ref := plumbing.NewBranchReferenceName(br.Name)
if exists, _ := repo.DoesRefExist(ctx, ref.String()); exists {
ret = append(ret, ref)
}
Comment on lines +46 to +49
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The error returned by repo.DoesRefExist is being ignored. If DoesRefExist encounters an error (e.g., a problem communicating with Git), exists will be false, and the branch will be silently skipped. This can lead to incorrect behavior where branches are skipped due to an error rather than actual deletion. Errors should be handled explicitly, either by propagating them or logging them appropriately, to prevent silent failures.

ref := plumbing.NewBranchReferenceName(br.Name)
if exists, err := repo.DoesRefExist(ctx, ref.String()); err != nil {
	return nil, errors.Wrapf(err, "failed to check existence of branch %q", br.Name)
} else if exists {
	ret = append(ret, ref)
}

}
// Use filtered traversal to skip excluded branches and their descendants
for _, n := range meta.SubsequentBranchesFiltered(tx, br.Name, true) {
ret = append(ret, plumbing.NewBranchReferenceName(n))
ref := plumbing.NewBranchReferenceName(n)
if exists, _ := repo.DoesRefExist(ctx, ref.String()); exists {
ret = append(ret, ref)
}
Comment on lines +53 to +56
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to the previous comment, the error returned by repo.DoesRefExist is ignored here. This can lead to branches being incorrectly skipped if there's an underlying issue with Git communication, rather than the branch truly not existing. It's important to handle errors explicitly to ensure the correct behavior of the sync --all operation.

ref := plumbing.NewBranchReferenceName(n)
if exists, err := repo.DoesRefExist(ctx, ref.String()); err != nil {
	return nil, errors.Wrapf(err, "failed to check existence of branch %q", n)
} else if exists {
	ret = append(ret, ref)
}

}
}
return ret, nil
Expand Down