Skip to content
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.

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

fix: Handle the original parent is a trunk case #117

Merged
merged 1 commit into from
May 16, 2023
Merged
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
37 changes: 34 additions & 3 deletions internal/actions/sync_branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,12 +179,21 @@ func syncBranchRebase(
}, nil
}

var origUpstream string
if origParent.Trunk {
// We do not know the original trunk commit hashes. Use the current one as
// an approximation.
origUpstream = newUpstreamCommitHash
Copy link
Contributor

Choose a reason for hiding this comment

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

why not do a revparse here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

newUpstreamCommitHash is either a result of the revparse or the merge commit which is close to the original upstream.

} else {
origUpstream = origParent.Head
}

continuation := SyncBranchContinuation{
NewParentName: parentState.Name,
}
rebase, err := repo.RebaseParse(git.RebaseOpts{
Branch: branch.Name,
Upstream: origParent.Head,
Upstream: origUpstream,
Onto: newUpstreamCommitHash,
})
if err != nil {
Expand Down Expand Up @@ -248,6 +257,16 @@ func syncBranchRebase(
// Note that we've introduced B into the history of stacked-2, but
// not C or D since those commits come after M.
newUpstreamCommitHash := origParentBranch.MergeCommit
var origUpstream string
if origParent.Trunk {
var err error
origUpstream, err = repo.RevParse(&git.RevParse{Rev: "origin/" + origParent.Name})
Copy link
Contributor

Choose a reason for hiding this comment

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

for my understanding, if i read the BranchState correctly, the BranchState.Head represents the SHA of the parent branch, not the SHA of the current branch. But git rev-parse gives you SHA of the current branch.

Is that an intended change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BranchState.Head is empty if Trunk = true. Hence this change.

if err != nil {
return nil, errors.WrapIff(err, "failed to get HEAD of %q", origParent.Name)
}
} else {
origUpstream = origParent.Head
}
continuation := SyncBranchContinuation{
NewParentName: parentState.Name,
}
Expand All @@ -260,7 +279,7 @@ func syncBranchRebase(
}
rebase, err := repo.RebaseParse(git.RebaseOpts{
Branch: branch.Name,
Upstream: origParent.Head,
Upstream: origUpstream,
Onto: newUpstreamCommitHash,
})
if err != nil {
Expand Down Expand Up @@ -305,6 +324,18 @@ func syncBranchRebase(
" of parent branch ", colors.UserInput(parentState.Name),
"\n",
)
var origUpstream string
if origParent.Trunk {
// This can happen if the branch is originally a stack root and reparented to
// another branch (and became non-stack-root).
var err error
origUpstream, err = repo.RevParse(&git.RevParse{Rev: "origin/" + origParent.Name})
if err != nil {
return nil, errors.WrapIff(err, "failed to get HEAD of %q", origParent.Name)
}
} else {
origUpstream = origParent.Head
}
// We need to use `rebase --onto` here and be very careful about how we
// determine the commits that are being rebased on top of parentHead.
// Suppose we have a history like
Expand Down Expand Up @@ -339,7 +370,7 @@ func syncBranchRebase(
}
rebase, err := repo.RebaseParse(git.RebaseOpts{
Branch: branch.Name,
Upstream: origParent.Head,
Upstream: origUpstream,
Onto: parentHead,
})
if err != nil {
Expand Down