From fe6bb98c4d57ce3d98855bef309cb215229928eb Mon Sep 17 00:00:00 2001 From: Travis DePrato <773453+travigd@users.noreply.github.com> Date: Tue, 4 Oct 2022 13:47:28 -0700 Subject: [PATCH] fix: Stack sync rebase properly after parent is merged (#63) --- e2e_tests/stack_sync_merge_commit_test.go | 68 +++++++++++++++++++++++ internal/actions/sync_branch.go | 8 ++- internal/meta/branch.go | 16 +++--- 3 files changed, 81 insertions(+), 11 deletions(-) create mode 100644 e2e_tests/stack_sync_merge_commit_test.go diff --git a/e2e_tests/stack_sync_merge_commit_test.go b/e2e_tests/stack_sync_merge_commit_test.go new file mode 100644 index 00000000..96001358 --- /dev/null +++ b/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", + ) +} diff --git a/internal/actions/sync_branch.go b/internal/actions/sync_branch.go index 47b64610..511cc05d 100644 --- a/internal/actions/sync_branch.go +++ b/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{ diff --git a/internal/meta/branch.go b/internal/meta/branch.go index 81293926..34ecb099 100644 --- a/internal/meta/branch.go +++ b/internal/meta/branch.go @@ -60,10 +60,14 @@ 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 @@ -71,10 +75,6 @@ func (b *Branch) UnmarshalJSON(bytes []byte) error { 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