Don't keep commits that become empty during a rebase

The only exception is when moving a custom patch for an entire commit to an
earlier commit; in this case the source commit becomes empty, but we want to
keep it, mainly for consistency with moving the patch to a later commit, which
behaves the same.

In all other cases where we rebase, it's confusing when empty commits are kept;
the most common example is rebasing a branch onto master, where master already
contains some of the commits of our branch. In this case we simply want to drop
these.

3 files changed, 25 insertions, 18 deletions

diff --git a/pkg/commands/git_commands/patch.go b/pkg/commands/git_commands/patch.go
index 2dd7c4490..3ee8fa362 100644
--- a/pkg/commands/git_commands/patch.go
+++ b/pkg/commands/git_commands/patch.go
@@ -91,7 +91,7 @@ func (self *PatchCommands) SaveTemporaryPatch(patch string) (string, error) {
 
 // DeletePatchesFromCommit applies a patch in reverse for a commit
 func (self *PatchCommands) DeletePatchesFromCommit(commits []*models.Commit, commitIndex int) error {
-	if err := self.rebase.BeginInteractiveRebaseForCommit(commits, commitIndex); err != nil {
+	if err := self.rebase.BeginInteractiveRebaseForCommit(commits, commitIndex, false); err != nil {
 		return err
 	}
 
@@ -117,7 +117,10 @@ func (self *PatchCommands) DeletePatchesFromCommit(commits []*models.Commit, com
 
 func (self *PatchCommands) MovePatchToSelectedCommit(commits []*models.Commit, sourceCommitIdx int, destinationCommitIdx int) error {
 	if sourceCommitIdx < destinationCommitIdx {
-		if err := self.rebase.BeginInteractiveRebaseForCommit(commits, destinationCommitIdx); err != nil {
+		// Passing true for keepCommitsThatBecomeEmpty: if the moved-from
+		// commit becomes empty, we want to keep it, mainly for consistency with
+		// moving the patch to a *later* commit, which behaves the same.
+		if err := self.rebase.BeginInteractiveRebaseForCommit(commits, destinationCommitIdx, true); err != nil {
 			return err
 		}
 
@@ -223,7 +226,7 @@ func (self *PatchCommands) MovePatchIntoIndex(commits []*models.Commit, commitId
 		}
 	}
 
-	if err := self.rebase.BeginInteractiveRebaseForCommit(commits, commitIdx); err != nil {
+	if err := self.rebase.BeginInteractiveRebaseForCommit(commits, commitIdx, false); err != nil {
 		return err
 	}
 
@@ -272,7 +275,7 @@ func (self *PatchCommands) MovePatchIntoIndex(commits []*models.Commit, commitId
 }
 
 func (self *PatchCommands) PullPatchIntoNewCommit(commits []*models.Commit, commitIdx int) error {
-	if err := self.rebase.BeginInteractiveRebaseForCommit(commits, commitIdx); err != nil {
+	if err := self.rebase.BeginInteractiveRebaseForCommit(commits, commitIdx, false); err != nil {
 		return err
 	}
 
diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go
index cbda4fb1f..60386b2ed 100644
--- a/pkg/commands/git_commands/rebase.go
+++ b/pkg/commands/git_commands/rebase.go
@@ -41,7 +41,7 @@ func (self *RebaseCommands) RewordCommit(commits []*models.Commit, index int, me
 		return self.commit.RewordLastCommit(message)
 	}
 
-	err := self.BeginInteractiveRebaseForCommit(commits, index)
+	err := self.BeginInteractiveRebaseForCommit(commits, index, false)
 	if err != nil {
 		return err
 	}
@@ -86,7 +86,7 @@ func (self *RebaseCommands) GenericAmend(commits []*models.Commit, index int, f
 		return f()
 	}
 
-	err := self.BeginInteractiveRebaseForCommit(commits, index)
+	err := self.BeginInteractiveRebaseForCommit(commits, index, false)
 	if err != nil {
 		return err
 	}
@@ -165,9 +165,10 @@ func logTodoChanges(changes []daemon.ChangeTodoAction) string {
 }
 
 type PrepareInteractiveRebaseCommandOpts struct {
-	baseShaOrRoot  string
-	instruction    daemon.Instruction
-	overrideEditor bool
+	baseShaOrRoot              string
+	instruction                daemon.Instruction
+	overrideEditor             bool
+	keepCommitsThatBecomeEmpty bool
 }
 
 // PrepareInteractiveRebaseCommand returns the cmd for an interactive rebase
@@ -180,7 +181,7 @@ func (self *RebaseCommands) PrepareInteractiveRebaseCommand(opts PrepareInteract
 		Arg("--interactive").
 		Arg("--autostash").
 		Arg("--keep-empty").
-		ArgIf(!self.version.IsOlderThan(2, 26, 0), "--empty=keep").
+		ArgIf(opts.keepCommitsThatBecomeEmpty && !self.version.IsOlderThan(2, 26, 0), "--empty=keep").
 		Arg("--no-autosquash").
 		ArgIf(!self.version.IsOlderThan(2, 22, 0), "--rebase-merges").
 		Arg(opts.baseShaOrRoot).
@@ -273,7 +274,9 @@ func (self *RebaseCommands) SquashAllAboveFixupCommits(commit *models.Commit) er
 
 // BeginInteractiveRebaseForCommit starts an interactive rebase to edit the current
 // commit and pick all others. After this you'll want to call `self.ContinueRebase()
-func (self *RebaseCommands) BeginInteractiveRebaseForCommit(commits []*models.Commit, commitIndex int) error {
+func (self *RebaseCommands) BeginInteractiveRebaseForCommit(
+	commits []*models.Commit, commitIndex int, keepCommitsThatBecomeEmpty bool,
+) error {
 	if len(commits)-1 < commitIndex {
 		return errors.New("index outside of range of commits")
 	}
@@ -292,9 +295,10 @@ func (self *RebaseCommands) BeginInteractiveRebaseForCommit(commits []*models.Co
 	self.os.LogCommand(logTodoChanges(changes), false)
 
 	return self.PrepareInteractiveRebaseCommand(PrepareInteractiveRebaseCommandOpts{
-		baseShaOrRoot:  getBaseShaOrRoot(commits, commitIndex+1),
-		overrideEditor: true,
-		instruction:    daemon.NewChangeTodoActionsInstruction(changes),
+		baseShaOrRoot:              getBaseShaOrRoot(commits, commitIndex+1),
+		overrideEditor:             true,
+		keepCommitsThatBecomeEmpty: keepCommitsThatBecomeEmpty,
+		instruction:                daemon.NewChangeTodoActionsInstruction(changes),
 	}).Run()
 }
 
@@ -358,7 +362,7 @@ func (self *RebaseCommands) runSkipEditorCommand(cmdObj oscommands.ICmdObj) erro
 
 // DiscardOldFileChanges discards changes to a file from an old commit
 func (self *RebaseCommands) DiscardOldFileChanges(commits []*models.Commit, commitIndex int, fileName string) error {
-	if err := self.BeginInteractiveRebaseForCommit(commits, commitIndex); err != nil {
+	if err := self.BeginInteractiveRebaseForCommit(commits, commitIndex, false); err != nil {
 		return err
 	}
 
diff --git a/pkg/commands/git_commands/rebase_test.go b/pkg/commands/git_commands/rebase_test.go
index f865468fb..dd768d120 100644
--- a/pkg/commands/git_commands/rebase_test.go
+++ b/pkg/commands/git_commands/rebase_test.go
@@ -29,7 +29,7 @@ func TestRebaseRebaseBranch(t *testing.T) {
 			arg:        "master",
 			gitVersion: &GitVersion{2, 26, 0, ""},
 			runner: oscommands.NewFakeRunner(t).
-				Expect(`git rebase --interactive --autostash --keep-empty --empty=keep --no-autosquash --rebase-merges master`, "", nil),
+				Expect(`git rebase --interactive --autostash --keep-empty --no-autosquash --rebase-merges master`, "", nil),
 			test: func(err error) {
 				assert.NoError(t, err)
 			},
@@ -39,7 +39,7 @@ func TestRebaseRebaseBranch(t *testing.T) {
 			arg:        "master",
 			gitVersion: &GitVersion{2, 26, 0, ""},
 			runner: oscommands.NewFakeRunner(t).
-				Expect(`git rebase --interactive --autostash --keep-empty --empty=keep --no-autosquash --rebase-merges master`, "", errors.New("error")),
+				Expect(`git rebase --interactive --autostash --keep-empty --no-autosquash --rebase-merges master`, "", errors.New("error")),
 			test: func(err error) {
 				assert.Error(t, err)
 			},
@@ -149,7 +149,7 @@ func TestRebaseDiscardOldFileChanges(t *testing.T) {
 			commitIndex: 0,
 			fileName:    "test999.txt",
 			runner: oscommands.NewFakeRunner(t).
-				Expect(`git rebase --interactive --autostash --keep-empty --empty=keep --no-autosquash --rebase-merges abcdef`, "", nil).
+				Expect(`git rebase --interactive --autostash --keep-empty --no-autosquash --rebase-merges abcdef`, "", nil).
 				Expect(`git cat-file -e HEAD^:"test999.txt"`, "", nil).
 				Expect(`git checkout HEAD^ -- "test999.txt"`, "", nil).
 				Expect(`git commit --amend --no-edit --allow-empty`, "", nil).