Always rebase on the distant version.

cmd/rebase.go

@@ -4,9 +4,11 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"slices"
 
 	"github.com/cupcicm/opp/core"
 	"github.com/go-git/go-git/v5/plumbing"
+	"github.com/go-git/go-git/v5/plumbing/object"
 	"github.com/urfave/cli/v2"
 )
 
@@ -59,24 +61,42 @@ func rebase(ctx context.Context, repo *core.Repo, pr *core.LocalPr, first bool)
 			fmt.Errorf(".opp/state/pr/%d is invalid, not sure what to rebase on", pr.PrNumber), 1)
 	}
 
+	ancestorCommit, err := FirstAncestorCommit(repo, pr)
+	if err != nil {
+		return false, err
+	}
+
 	if ancestor.IsPr() {
-		return rebaseOnDependentPr(ctx, repo, pr, ancestor.(*core.LocalPr), first)
+		return rebaseOnDependentPr(ctx, repo, pr, ancestor.(*core.LocalPr), ancestorCommit, first)
 	} else {
-		return rebaseOnBaseBranch(ctx, repo, pr, first)
+		return rebaseOnBaseBranch(ctx, repo, pr, ancestorCommit, first)
 	}
 }
 
 // Return true when the current PR has been merged and does not actually exist anymore.
-func rebaseOnBaseBranch(ctx context.Context, repo *core.Repo, pr *core.LocalPr, first bool) (bool, error) {
+func rebaseOnBaseBranch(
+	ctx context.Context,
+	repo *core.Repo,
+	pr *core.LocalPr,
+	parent *object.Commit,
+	first bool,
+) (bool, error) {
 	if !first {
 		fmt.Printf("Rebasing dependent PR %s...\n", pr.LocalBranch())
 	}
 
 	if err := repo.Checkout(pr); err != nil {
 		return false, fmt.Errorf("error during checkout: %w", err)
 	}
-	if err := repo.Rebase(ctx, repo.BaseBranch(), true); err != nil {
-		return false, fmt.Errorf("error during rebase: %w", err)
+	base := repo.BaseBranch()
+	if !repo.TryRebaseBranchOnto(ctx, parent.Hash, base) {
+		fmt.Printf("%s cannot be cleanly rebased on top of %s.\n", pr.LocalBranch(), base.LocalName())
+		fmt.Printf("This PR depended on another PR, and you merged a version that conflicts with this PR.\n")
+		fmt.Printf("Here is an editor where you need to choose how to correctly rebase %s on top of the new %s\n", pr.LocalBranch(), base.RemoteName())
+		err := repo.InteractiveRebase(ctx, base)
+		if err != nil {
+			return false, errors.New("please finish the interactive rebase then re-run")
+		}
 	}
 	remoteBaseBranchTip := core.Must(repo.GetRemoteTip(repo.BaseBranch()))
 	localPrTip := core.Must(repo.GetLocalTip(pr))
@@ -95,14 +115,21 @@ func rebaseOnBaseBranch(ctx context.Context, repo *core.Repo, pr *core.LocalPr,
 // if the dependant branch has been modified git doesn't know where
 // the current PR starts exactly. The user will know.
 // Return true when the current PR has been merged and does not actually exist anymore.
-func rebaseOnDependentPr(ctx context.Context, repo *core.Repo, pr *core.LocalPr, ancestor *core.LocalPr, first bool) (bool, error) {
+func rebaseOnDependentPr(
+	ctx context.Context,
+	repo *core.Repo,
+	pr *core.LocalPr,
+	ancestor *core.LocalPr,
+	parent *object.Commit,
+	first bool,
+) (bool, error) {
 	hasBeenMerged, err := rebase(ctx, repo, ancestor, false)
 	if err != nil {
 		return false, err
 	}
 	if hasBeenMerged {
 		pr.ReloadState()
-		return rebaseOnBaseBranch(ctx, repo, pr, first)
+		return rebaseOnBaseBranch(ctx, repo, pr, parent, first)
 	}
 
 	if !first {
@@ -114,7 +141,7 @@ func rebaseOnDependentPr(ctx context.Context, repo *core.Repo, pr *core.LocalPr,
 		return false, fmt.Errorf("error during checkout: %w", err)
 	}
 	// Try to rebase silently once.
-	if !repo.TryRebaseCurrentBranchSilently(ctx, ancestor) {
+	if !repo.TryRebaseBranchOnto(ctx, parent.Hash, ancestor) {
 		fmt.Printf("%s cannot be cleanly rebased on top of %s.\n", pr.LocalBranch(), ancestor.LocalBranch())
 		fmt.Printf("This usually happens when you modified (e.g. amended) some commits in %s.\n", ancestor.LocalBranch())
 		fmt.Printf("Here is an editor window where you need to pick only the commits in %s.\n", pr.LocalBranch())
@@ -127,3 +154,35 @@ func rebaseOnDependentPr(ctx context.Context, repo *core.Repo, pr *core.LocalPr,
 	pr.RememberCurrentTip()
 	return false, nil
 }
+
+// Returns the first commit in the history of pr that belongs to its ancestor, and does
+// not belong to the PR.
+func FirstAncestorCommit(repo *core.Repo, pr *core.LocalPr) (*object.Commit, error) {
+	tip := core.Must(repo.GetLocalTip(pr))
+	commits, err := repo.GetCommitsNotInBaseBranch(tip.Hash)
+	if err != nil {
+		return nil, fmt.Errorf("%s does not descend from %s", pr.LocalBranch(), repo.BaseBranch().LocalName())
+	}
+
+	ancestorKnownTips := pr.AncestorTips()
+	ancestor, err := pr.GetAncestor()
+	if err != nil {
+		return nil, err
+	}
+	remoteTip, err := repo.GetRemoteTip(ancestor)
+	if err != nil {
+		return nil, err
+	}
+	ancestorKnownTips = append(ancestorKnownTips, remoteTip.Hash.String())
+
+	slices.Reverse(ancestorKnownTips)
+	for _, commit := range commits {
+		if slices.Contains(ancestorKnownTips, commit.Hash.String()) {
+			// The current commit was once the tip of the ancestor branch for this PR.
+			// This means that the current PR contains all commits after this, and
+			// the commits before this were the commits of the ancestor branch or PR.
+			return commit, nil
+		}
+	}
+	return remoteTip, nil
+}

cmd/rebase_test.go

@@ -2,10 +2,15 @@ package cmd_test
 
 import (
 	"context"
+	"fmt"
+	"os"
+	"path"
+	"strings"
 	"testing"
 
 	"github.com/cupcicm/opp/core"
 	"github.com/cupcicm/opp/core/tests"
+	"github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/plumbing"
 	"github.com/stretchr/testify/assert"
 )
@@ -33,3 +38,90 @@ func TestRebaseCleansDependentBranches(t *testing.T) {
 
 	assert.Equal(t, "master", core.Must(pr3.GetAncestor()).LocalName())
 }
+
+func TestRebaseFindsPreviousTips(t *testing.T) {
+	r := tests.NewTestRepo(t)
+
+	pr2 := r.CreatePr(t, "HEAD^", 2)
+	pr3 := r.CreatePr(t, "HEAD", 3)
+
+	r.Checkout(pr2)
+
+	os.WriteFile(path.Join(r.Path(), "3"), []byte("amended 3"), 0644)
+	r.Worktree().Add("3")
+	r.RewriteLastCommit("amended 3")
+	assert.NoError(t, r.Run("push"))
+	r.Checkout(pr3)
+
+	// The status now is that pr/3 depends on an old version of pr/2
+	// the commit in pr/2 has been amended to a new one.
+	// However, pr/2 still remembers its old tip, so rebasing pr/3 will
+	// see pr/3 point to commit "4" on top of the "amended 3" commit
+	assert.NoError(t, r.Run("rebase"))
+	commits := core.Must(r.Log(&git.LogOptions{}))
+	expectedCommitMessages := []string{"4", "amended 3", "2", "1", "0"}
+	for i := 0; i < 5; i++ {
+		c := core.Must(commits.Next())
+		assert.Equal(t, expectedCommitMessages[i], strings.TrimSpace(c.Message))
+	}
+	r.Checkout(pr2)
+	pr2Commits := core.Must(r.Log(&git.LogOptions{}))
+	for i := 0; i < 4; i++ {
+		c := core.Must(pr2Commits.Next())
+		assert.Equal(t, expectedCommitMessages[i+1], strings.TrimSpace(c.Message))
+	}
+}
+
+func TestRebaseAbandonsGracefully(t *testing.T) {
+	r := tests.NewTestRepo(t)
+
+	pr2 := r.CreatePr(t, "HEAD^", 2)
+	pr3 := r.CreatePr(t, "HEAD", 3)
+
+	r.Checkout(pr2)
+
+	os.WriteFile(path.Join(r.Path(), "4"), []byte("conflicts with 4"), 0644)
+	r.Worktree().Add("4")
+	r.RewriteLastCommit("conflicts with 4")
+	r.MergePr(t, pr2)
+	r.Checkout(pr3)
+	fmt.Println(r.Path())
+
+	// The new version of commit 3 modifies file 4, so it will conflict with commit 4
+	// Assert the rebase command fails, and that a rebase is in progress.
+	assert.Error(t, r.Run("rebase"))
+	assert.DirExists(t, path.Join(r.Path(), ".git", "rebase-merge"))
+	rebaseTodo := string(core.Must(os.ReadFile(path.Join(r.Path(), ".git", "rebase-merge", "git-rebase-todo.backup"))))
+
+	// Also, the file should contain "pick commit 3" and "pick commit 4"
+	assert.Regexp(t, "pick [0-9a-z]+ 3", rebaseTodo)
+	assert.Regexp(t, "pick [0-9a-z]+ 4", rebaseTodo)
+}
+
+func TestRebaseFindsTipWhenMerged(t *testing.T) {
+	r := tests.NewTestRepo(t)
+
+	pr2 := r.CreatePr(t, "HEAD^", 2)
+	pr3 := r.CreatePr(t, "HEAD", 3)
+
+	r.Checkout(pr2)
+
+	os.WriteFile(path.Join(r.Path(), "3"), []byte("amended 3"), 0644)
+	r.Worktree().Add("3")
+	r.RewriteLastCommit("amended 3")
+	r.MergePr(t, pr2)
+	r.Checkout(pr3)
+	fmt.Println(r.Path())
+
+	// The status now is that pr/3 depends on an old version of pr/2
+	// the commit in pr/2 has been amended to a new one, then pr/2 was merged.
+	// However, pr/3 remembers the old tip of pr/2, so rebasing pr/3 will
+	// see pr/3 point to commit "4" on top of the "amended 3" commit
+	assert.NoError(t, r.Run("rebase"))
+	commits := core.Must(r.Log(&git.LogOptions{}))
+	expectedCommitMessages := []string{"4", "amended 3", "2", "1", "0"}
+	for i := 0; i < 5; i++ {
+		c := core.Must(commits.Next())
+		assert.Equal(t, expectedCommitMessages[i], strings.TrimSpace(c.Message))
+	}
+}

core/repo.go

@@ -194,21 +194,16 @@ func (r *Repo) Fetch(ctx context.Context) error {
 
 // When remote is true, rebase on the distant version of the branch. When false,
 // rebase on the local version.
-func (r *Repo) Rebase(ctx context.Context, branch Branch, remote bool) error {
-	var cmd *exec.Cmd
-	if remote {
-		cmd = r.GitExec(ctx, "rebase %s/%s", GetRemoteName(), branch.RemoteName())
-	} else {
-		cmd = r.GitExec(ctx, "rebase %s", branch.LocalName())
-	}
+func (r *Repo) Rebase(ctx context.Context, branch Branch) error {
+	cmd := r.GitExec(ctx, "rebase %s/%s", GetRemoteName(), branch.RemoteName())
 	cmd.Stderr = os.Stderr
 	cmd.Stdout = os.Stdout
 	cmd.Stdin = os.Stdin
 	return cmd.Run()
 }
 
 func (r *Repo) TryRebaseCurrentBranchSilently(ctx context.Context, branch Branch) bool {
-	cmd := r.GitExec(ctx, "rebase %s", branch.LocalName())
+	cmd := r.GitExec(ctx, "rebase %s/%s", GetRemoteName(), branch.RemoteName())
 	err := cmd.Run()
 	if err == nil {
 		return true
@@ -237,10 +232,27 @@ func (r *Repo) TryRebaseOntoSilently(ctx context.Context, first plumbing.Hash, l
 	return false
 }
 
+func (r *Repo) TryRebaseBranchOnto(ctx context.Context, parent plumbing.Hash, onto Branch) bool {
+	ontoName := onto.LocalName()
+	if !onto.IsPr() {
+		ontoName = fmt.Sprintf("%s/%s", GetRemoteName(), onto.RemoteName())
+	}
+	cmd := r.GitExec(ctx, "rebase --onto %s %s", ontoName, parent.String())
+	err := cmd.Run()
+	if err == nil {
+		return true
+	}
+	abort := r.GitExec(ctx, "rebase --abort")
+	if err := abort.Run(); err != nil {
+		panic(fmt.Errorf("tried to abort the rebase but failed: %w", err))
+	}
+	return false
+}
+
 // When remote is true, rebase on the distant version of the branch. When false,
 // rebase on the local version.
 func (r *Repo) InteractiveRebase(ctx context.Context, branch Branch) error {
-	cmd := r.GitExec(ctx, "rebase --no-fork-point -i %s", branch.LocalName())
+	cmd := r.GitExec(ctx, "rebase --no-fork-point -i %s/%s", GetRemoteName(), branch.RemoteName())
 	cmd.Stderr = os.Stderr
 	cmd.Stdout = os.Stdout
 	cmd.Stdin = os.Stdin

core/tests/testrepo.go

@@ -71,6 +71,7 @@ func NewTestRepo(t *testing.T) *TestRepo {
 		}),
 	}
 	testRepo.PrepareSource()
+	testRepo.AlwaysFailingEditor()
 	return &testRepo
 }
 
@@ -84,7 +85,20 @@ func (r *TestRepo) Run(command string, args ...string) error {
 
 func (r *TestRepo) Commit(msg string) plumbing.Hash {
 	wt := core.Must(r.Source.Worktree())
-	return core.Must(wt.Commit("msg", &git.CommitOptions{}))
+	return core.Must(wt.Commit(msg, &git.CommitOptions{}))
+}
+
+func (r *TestRepo) RewriteLastCommit(msg string) {
+	cmd := r.GitExec(context.Background(), "commit --amend -m \"%s\"", msg)
+	cmd.Run()
+}
+
+func (r *TestRepo) AlwaysFailingEditor() {
+	cmd := r.GitExec(context.Background(), "config core.editor true")
+	err := cmd.Run()
+	if err != nil {
+		panic(err)
+	}
 }
 
 func (r *TestRepo) PrepareSource() {
@@ -127,6 +141,17 @@ func (r *TestRepo) CreatePr(t *testing.T, ref string, prNumber int, args ...stri
 	return r.AssertHasPr(t, prNumber)
 }
 
+func (r *TestRepo) MergePr(t *testing.T, pr *core.LocalPr) error {
+	tip := core.Must(r.GetLocalTip(pr))
+	r.GithubMock.CallGetAndReturnMergeable(pr.PrNumber, true)
+	r.GithubMock.CallMerge(pr.PrNumber, tip.Hash.String())
+	err := r.Run("merge", fmt.Sprintf("pr/%d", pr.PrNumber))
+	if err != nil {
+		return err
+	}
+	return r.Push(context.Background(), tip.Hash, r.BaseBranch().RemoteName())
+}
+
 type GithubMock struct {
 	mock.Mock
 }