fix: Stack sync rebase properly after parent is merged (#63)

<!-- av pr metadata
This information is embedded by the av CLI when creating PRs to track the status of stacks when using Aviator. Please do not delete or edit this section of the PR.
```
{"parent":"master","parentHead":"","trunk":"master"}
```
-->

e2e_tests/stack_sync_merge_commit_test.go

@@ -0,0 +1,68 @@
+package e2e_tests
+
+import (
+	"github.com/aviator-co/av/internal/git"
+	"github.com/aviator-co/av/internal/git/gittest"
+	"github.com/aviator-co/av/internal/meta"
+	"github.com/stretchr/testify/require"
+	"testing"
+)
+
+func TestStackSyncMergeCommit(t *testing.T) {
+	repo := gittest.NewTempRepo(t)
+	Chdir(t, repo.Dir())
+
+	// To start, we create a simple two-stack where each stack has a single commit.
+	// Our stack looks like:
+	//     stack-1: main -> 1a -> 2b
+	//     stack-2:                \ -> 2a -> 2b
+	require.Equal(t, 0, Cmd(t, "git", "checkout", "-b", "stack-1").ExitCode)
+	gittest.CommitFile(t, repo, "my-file", []byte("1a\n"), gittest.WithMessage("Commit 1a"))
+	gittest.CommitFile(t, repo, "my-file", []byte("1a\n1b\n"), gittest.WithMessage("Commit 1b"))
+	RequireAv(t, "stack", "branch", "stack-2")
+	gittest.CommitFile(t, repo, "my-file", []byte("1a\n1b\n2a\n"), gittest.WithMessage("Commit 2a"))
+	gittest.CommitFile(t, repo, "my-file", []byte("1a\n1b\n2a\n2b\n"), gittest.WithMessage("Commit 2b"))
+
+	// Everything up to date now, so this should be a no-op.
+	require.Equal(t, 0, Av(t, "stack", "sync", "--no-fetch", "--no-push").ExitCode)
+
+	// We simulate a merge here so that our history looks like:
+	//     main:    X            / -> 1S
+	//     stack-1:  \ -> 1a -> 2b
+	//     stack-2:              \ -> 2a -> 2b
+	// where 1S is the squash-merge commit of 2b onto main. Note that since it's
+	// a squash commit, 1S is not a *merge commit* in the Git definition.
+	var squashCommit string
+	gittest.WithCheckoutBranch(t, repo, "main", func() {
+		oldHead, err := repo.RevParse(&git.RevParse{Rev: "HEAD"})
+		require.NoError(t, err, "failed to get HEAD")
+
+		RequireCmd(t, "git", "merge", "--squash", "stack-1")
+		// `git merge --squash` doesn't actually create the commit, so we have to
+		// do that separately.
+		RequireCmd(t, "git", "commit", "--no-edit")
+		squashCommit, err = repo.RevParse(&git.RevParse{Rev: "HEAD"})
+		require.NoError(t, err, "failed to get squash commit")
+		require.NotEqual(t, oldHead, squashCommit, "squash commit should be different from old HEAD")
+	})
+
+	stack1Meta, _ := meta.ReadBranch(repo, "stack-1")
+	stack1Meta.MergeCommit = squashCommit
+	require.NoError(t, meta.WriteBranch(repo, stack1Meta))
+
+	require.Equal(t, 0,
+		Cmd(t, "git", "merge-base", "--is-ancestor", "stack-1", "stack-2").ExitCode,
+		"HEAD of stack-1 should be an ancestor of HEAD of stack-2 before running sync",
+	)
+	require.NotEqual(t, 0,
+		Cmd(t, "git", "merge-base", "--is-ancestor", squashCommit, "stack-2").ExitCode,
+		"squash commit of stack-1 should not be an ancestor of HEAD of stack-1 before running sync",
+	)
+
+	RequireAv(t, "stack", "sync", "--no-fetch", "--no-push")
+
+	require.Equal(t, 0,
+		Cmd(t, "git", "merge-base", "--is-ancestor", squashCommit, "stack-2").ExitCode,
+		"squash commit of stack-1 should be an ancestor of HEAD of stack-1 after running sync",
+	)
+}

internal/actions/sync_branch.go

@@ -71,11 +71,11 @@ func SyncBranch(
 	} else {
 		if !opts.NoFetch {
 			update, err := UpdatePullRequestState(ctx, repo, client, repoMeta, branch.Name)
-			branch = update.Branch
 			if err != nil {
 				_, _ = fmt.Fprint(os.Stderr, colors.Failure("      - error: ", err.Error()), "\n")
 				return nil, errors.Wrap(err, "failed to fetch latest PR info")
 			}
+			branch = update.Branch
 			pull = update.Pull
 			if update.Changed {
 				_, _ = fmt.Fprint(os.Stderr, "      - found updated pull request: ", colors.UserInput(update.Pull.Permalink), "\n")
@@ -211,8 +211,10 @@ func syncBranchRebase(
 			"  - rebasing ", colors.UserInput(branch.Name),
 			" on top of merge commit ", colors.UserInput(short), "\n",
 		)
-		if _, err := repo.Git("fetch", "origin", branch.MergeCommit); err != nil {
-			return nil, errors.WrapIff(err, "failed to fetch merge commit %q from origin", short)
+		if !opts.NoFetch {
+			if _, err := repo.Git("fetch", "origin", branch.MergeCommit); err != nil {
+				return nil, errors.WrapIff(err, "failed to fetch merge commit %q from origin", short)
+			}
 		}
 
 		rebase, err := repo.RebaseParse(git.RebaseOpts{

internal/meta/branch.go

@@ -60,21 +60,21 @@ func (b *Branch) UnmarshalJSON(bytes []byte) error {
 		return err
 	}
 
-	// Everything except name (since that is set externally) and parent is set
-	// on the actual Branch object we're unmarshalling into
-	b.Children = d.Children
-	b.PullRequest = d.PullRequest
+	// Copy over all the data that we unmarshalled into BranchAlias. This is
+	// everything except since Parent which we'll handle next. We need to take
+	// special care to copy name since it won't be defined on BranchAlias (since
+	// Name is not serialized to JSON). Instead we expect that the struct is
+	// always initialized with the name defined, so we have to copy it over here.
+	// (Doing just "*b = ..." will result in us erasing the name.)
+	d.BranchAlias.Name = b.Name
+	*b = Branch(d.BranchAlias)
 
 	// Parse the parent information (which can either be a string or a JSON)
 	var err error
 	b.Parent, err = unmarshalBranchState(d.Parent)
 	if err != nil {
 		return err
 	}
-	// can't do this because sometimes we read an uninitialized branch
-	//if b.Parent.Name == "" {
-	//	return errors.Errorf("cannot unmarshal Branch from JSON: parent branch of %q is unset", b.Name)
-	//}
 
 	logrus.Debugf("parsed branch metadata: %s => %#+v %#+v", bytes, d, b)
 	return nil