Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
44 changes: 44 additions & 0 deletions cmd/av/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,3 +165,47 @@ func branchNameArgs(
branches, _ := allBranches(cmd.Context())
return branches, cobra.ShellCompDirectiveNoSpace
}

// validateBranchSync checks if the current branch is in sync with its parent branch.
// Only enforces this check for non-trunk branches when we have a stored parent head.
// This validation prevents commands from operating on branches that are out of sync
// with their parent, which could lead to unexpected behavior or loss of commits.
func validateBranchSync(ctx context.Context, repo *git.Repo, branch meta.Branch) error {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure the philosophy around how much this CLI should act as a wizard. I.e. rather than throwing an error maybe the cli should prompt for "do you want to run av sync?".

Happy to update depending on what you prefer

if branch.Parent.Trunk {
// If there's no parent or the branch is a trunk branch, we don't need to check sync
return nil
}

// Get the current head of the parent branch
currentParentHead, err := repo.RevParse(ctx, &git.RevParse{Rev: branch.Parent.Name})
if err != nil {
return errors.Errorf(
"cannot get current head of parent branch %s: %v",
branch.Parent.Name,
err,
)
}

// If the current parent head differs from the stored parent head,
// then the parent has moved and we need to sync before proceeding
if currentParentHead != branch.Parent.Head {
return errors.Errorf(
"branch is not in sync with parent branch %s, please run 'av sync' first",
branch.Parent.Name,
)
}

// Additional check: ensure the child branch is actually based on the stored parent head
mergeBase, err := repo.MergeBase(ctx, branch.Parent.Head, branch.Name)
if err != nil {
return errors.Errorf("cannot find merge base with parent branch: %v", err)
}

if mergeBase != branch.Parent.Head {
return errors.Errorf(
"branch is not in sync with parent branch %s, please run 'av sync' first",
branch.Parent.Name,
)
}
return nil
}
17 changes: 16 additions & 1 deletion cmd/av/squash.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,23 @@ func runSquash(ctx context.Context, repo *git.Repo, db meta.DB) error {
return errors.New("this branch has already been merged, squashing is not allowed")
}

// Check if branch is in sync with parent before squashing
if err := validateBranchSync(ctx, repo, branch); err != nil {
return err
}

// Use the parent's head commit hash if available, otherwise fall back to branch name
// Since we've already validated sync, we can safely use the stored parent head
var parentRef string
if branch.Parent.Head != "" {
parentRef = branch.Parent.Head
} else {
// Fallback to branch name when no head commit is stored
parentRef = branch.Parent.Name
}

commitIDs, err := repo.RevList(ctx, git.RevListOpts{
Specifiers: []string{currentBranchName, "^" + branch.Parent.Name},
Specifiers: []string{currentBranchName, "^" + parentRef},
Reverse: true,
})
if err != nil {
Expand Down
106 changes: 106 additions & 0 deletions e2e_tests/squash_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package e2e_tests

import (
"strings"
"testing"

"github.com/aviator-co/av/internal/git/gittest"
Expand Down Expand Up @@ -321,3 +322,108 @@ func TestSquashStackedBranchBDoesNotAffectBranchA(t *testing.T) {
require.FileExists(t, repo.RepoDir+"/b2.txt")
require.FileExists(t, repo.RepoDir+"/b3.txt")
}

func TestSquashWithModifiedParentBranch(t *testing.T) {
repo := gittest.NewTempRepo(t)
Chdir(t, repo.RepoDir)
repo.Git(t, "fetch")

// Create parent branch
RequireAv(t, "branch", "parent-branch")
repo.CreateFile(t, "parent1.txt", "parent content 1")
repo.AddFile(t, "parent1.txt")
RequireAv(t, "commit", "-m", "parent commit 1")

repo.CreateFile(t, "parent2.txt", "parent content 2")
repo.AddFile(t, "parent2.txt")
RequireAv(t, "commit", "-m", "parent commit 2")

// Create child branch from parent
RequireAv(t, "branch", "child-branch")
repo.CreateFile(t, "child1.txt", "child content 1")
repo.AddFile(t, "child1.txt")
RequireAv(t, "commit", "-m", "child commit 1")

repo.CreateFile(t, "child2.txt", "child content 2")
repo.AddFile(t, "child2.txt")
RequireAv(t, "commit", "-m", "child commit 2")

// Simulate external changes to parent branch (force push scenario)
repo.Git(t, "checkout", "parent-branch")
repo.Git(t, "reset", "--hard", "HEAD~1") // Reset to parent commit 1
repo.CreateFile(t, "parent_new.txt", "new parent content")
repo.AddFile(t, "parent_new.txt")
repo.Git(t, "commit", "-m", "parent new commit")

// Switch back to child branch and attempt squash
RequireAv(t, "switch", "child-branch")

// Squash should now fail because branch is not in sync with parent
output := Av(t, "squash")
require.NotEqual(t, 0, output.ExitCode)
require.Contains(t, output.Stderr, "branch is not in sync with parent branch")
require.Contains(t, output.Stderr, "please run 'av sync' first")

// Verify child files are still present (no partial squash happened)
require.FileExists(t, repo.RepoDir+"/child1.txt")
require.FileExists(t, repo.RepoDir+"/child2.txt")

// Verify we still have the expected commits (no squashing occurred)
commitCount := strings.TrimSpace(
repo.Git(t, "rev-list", "--count", "child-branch", "^parent-branch"),
)
// After the parent branch was force-pushed (reset and new commits), the child branch
// now has more commits relative to the new parent head, which is expected behavior
require.NotEqual(
t,
"1",
commitCount,
"Should have more than 1 commit, confirming squash was prevented",
)

// The key achievement: squash was prevented, avoiding the bug where we would
// squash into a parent commit due to the force-pushed parent branch
}

func TestSquashFailsWhenBranchOutOfSync(t *testing.T) {
repo := gittest.NewTempRepo(t)
Chdir(t, repo.RepoDir)
repo.Git(t, "fetch")

// Create parent branch
RequireAv(t, "branch", "parent-branch")
repo.CreateFile(t, "parent1.txt", "parent content 1")
repo.AddFile(t, "parent1.txt")
RequireAv(t, "commit", "-m", "parent commit 1")

// Create child branch from parent
RequireAv(t, "branch", "child-branch")
repo.CreateFile(t, "child1.txt", "child content 1")
repo.AddFile(t, "child1.txt")
RequireAv(t, "commit", "-m", "child commit 1")

repo.CreateFile(t, "child2.txt", "child content 2")
repo.AddFile(t, "child2.txt")
RequireAv(t, "commit", "-m", "child commit 2")

// Modify parent branch externally to create out-of-sync condition
repo.Git(t, "checkout", "parent-branch")
repo.CreateFile(t, "parent2.txt", "parent content 2")
repo.AddFile(t, "parent2.txt")
repo.Git(t, "commit", "-m", "parent commit 2")

// Switch back to child branch
RequireAv(t, "switch", "child-branch")

// Squash should fail because branch is not in sync with parent
output := Av(t, "squash")
require.NotEqual(t, 0, output.ExitCode)
require.Contains(t, output.Stderr, "branch is not in sync with parent branch parent-branch")
require.Contains(t, output.Stderr, "please run 'av sync' first")

// Verify no changes were made (both child commits should still exist)
commitCount := strings.TrimSpace(
repo.Git(t, "rev-list", "--count", "child-branch", "^parent-branch"),
)
require.Equal(t, "2", commitCount)
}