Repository: jesseduffield/lazygit (language: go)

Issue:
Deleting an update-ref entry in a rebase will cause the branch to be deleted
**Describe the bug**
Scenario: you want to make a copy of a branch, and rebase it away from the original branch (e.g. the original branch is off of `main`, and you want to make a copy to see if it also compiles on `devel`).

If you have `rebase.updateRefs` set to true in your git config (which you will if you often work with stacked branches), then rebasing the copy onto `devel` will also rebase the original branch there; not what you want. I consider this a bug in git, and posted [lengthy messages](https://public-inbox.org/git/354f9fed-567f-42c8-9da9-148a5e223022@haller-berlin.de/) about this in the git mailing list, but folks either don't agree or don't care enough. (Read the whole thread if you are interested.)

So, to work around it, you make an interactive rebase onto `devel`, and delete the `update-ref` entry for the original branch. That should make the original branch stay where it was, right? Now here's the bug: it will instead _delete_ the original branch. Pretty bad.

The reason for this is the house-keeping that git does around update-ref commands. When starting a rebase, it remembers which refs are affected, and when changing the todo list using `git rebase --edit-todo` it updates that info based on whether update-ref commands where added or removed. The problem is that lazygit simply removes an update-ref command behind git's back, so this housekeeping doesn't happen.

The solution could be, instead of writing our changed git-rebase-todo file back manually, to call `git rebase --edit-todo` and set lazygit as an editor that provides the updated todo file via our daemon mechanism.

**Version info:**
0.41.0


Discussion / hints:

Fix patch:
diff --git a/pkg/app/daemon/daemon.go b/pkg/app/daemon/daemon.go
index 70490aef024..2f1cded7c23 100644
--- a/pkg/app/daemon/daemon.go
+++ b/pkg/app/daemon/daemon.go
@@ -39,6 +39,7 @@ const (
 	DaemonKindInsertBreak
 	DaemonKindChangeTodoActions
 	DaemonKindMoveFixupCommitDown
+	DaemonKindWriteRebaseTodo
 )
 
 const (
@@ -59,6 +60,7 @@ func getInstruction() Instruction {
 		DaemonKindMoveTodosUp:         deserializeInstruction[*MoveTodosUpInstruction],
 		DaemonKindMoveTodosDown:       deserializeInstruction[*MoveTodosDownInstruction],
 		DaemonKindInsertBreak:         deserializeInstruction[*InsertBreakInstruction],
+		DaemonKindWriteRebaseTodo:     deserializeInstruction[*WriteRebaseTodoInstruction],
 	}
 
 	return mapping[getDaemonKind()](jsonData)
@@ -330,3 +332,27 @@ func (self *InsertBreakInstruction) run(common *common.Common) error {
 		return utils.PrependStrToTodoFile(path, []byte("break\n"))
 	})
 }
+
+type WriteRebaseTodoInstruction struct {
+	TodosFileContent []byte
+}
+
+func NewWriteRebaseTodoInstruction(todosFileContent []byte) Instruction {
+	return &WriteRebaseTodoInstruction{
+		TodosFileContent: todosFileContent,
+	}
+}
+
+func (self *WriteRebaseTodoInstruction) Kind() DaemonKind {
+	return DaemonKindWriteRebaseTodo
+}
+
+func (self *WriteRebaseTodoInstruction) SerializedInstructions() string {
+	return serializeInstruction(self)
+}
+
+func (self *WriteRebaseTodoInstruction) run(common *common.Common) error {
+	return handleInteractiveRebase(common, func(path string) error {
+		return os.WriteFile(path, self.TodosFileContent, 0o644)
+	})
+}
diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go
index eeae661678a..2d1de707f38 100644
--- a/pkg/commands/git_commands/rebase.go
+++ b/pkg/commands/git_commands/rebase.go
@@ -204,7 +204,7 @@ type PrepareInteractiveRebaseCommandOpts struct {
 
 // PrepareInteractiveRebaseCommand returns the cmd for an interactive rebase
 // we tell git to run lazygit to edit the todo list, and we pass the client
-// lazygit a todo string to write to the todo file
+// lazygit instructions what to do with the todo file
 func (self *RebaseCommands) PrepareInteractiveRebaseCommand(opts PrepareInteractiveRebaseCommandOpts) oscommands.ICmdObj {
 	ex := oscommands.GetLazygitPath()
 
@@ -250,6 +250,36 @@ func (self *RebaseCommands) PrepareInteractiveRebaseCommand(opts PrepareInteract
 	return cmdObj
 }
 
+// GitRebaseEditTodo runs "git rebase --edit-todo", saving the given todosFileContent to the file
+func (self *RebaseCommands) GitRebaseEditTodo(todosFileContent []byte) error {
+	ex := oscommands.GetLazygitPath()
+
+	cmdArgs := NewGitCmd("rebase").
+		Arg("--edit-todo").
+		ToArgv()
+
+	debug := "FALSE"
+	if self.Debug {
+		debug = "TRUE"
+	}
+
+	self.Log.WithField("command", cmdArgs).Debug("RunCommand")
+
+	cmdObj := self.cmd.New(cmdArgs)
+
+	cmdObj.AddEnvVars(daemon.ToEnvVars(daemon.NewWriteRebaseTodoInstruction(todosFileContent))...)
+
+	cmdObj.AddEnvVars(
+		"DEBUG="+debug,
+		"LANG=en_US.UTF-8",   // Force using EN as language
+		"LC_ALL=en_US.UTF-8", // Force using EN as language
+		"GIT_EDITOR="+ex,
+		"GIT_SEQUENCE_EDITOR="+ex,
+	)
+
+	return cmdObj.Run()
+}
+
 // AmendTo amends the given commit with whatever files are staged
 func (self *RebaseCommands) AmendTo(commits []*models.Commit, commitIndex int) error {
 	commit := commits[commitIndex]
@@ -302,11 +332,16 @@ func (self *RebaseCommands) DeleteUpdateRefTodos(commits []*models.Commit) error
 		return todoFromCommit(commit)
 	})
 
-	return utils.DeleteTodos(
+	todosFileContent, err := utils.DeleteTodos(
 		filepath.Join(self.repoPaths.WorktreeGitDirPath(), "rebase-merge/git-rebase-todo"),
 		todosToDelete,
 		self.config.GetCoreCommentChar(),
 	)
+	if err != nil {
+		return err
+	}
+
+	return self.GitRebaseEditTodo(todosFileContent)
 }
 
 func (self *RebaseCommands) MoveTodosDown(commits []*models.Commit) error {
diff --git a/pkg/utils/rebase_todo.go b/pkg/utils/rebase_todo.go
index 3403eef9700..f2b60a09769 100644
--- a/pkg/utils/rebase_todo.go
+++ b/pkg/utils/rebase_todo.go
@@ -1,6 +1,7 @@
 package utils
 
 import (
+	"bytes"
 	"fmt"
 	"os"
 	"strings"
@@ -96,6 +97,12 @@ func WriteRebaseTodoFile(fileName string, todos []todo.Todo, commentChar byte) e
 	return err
 }
 
+func todosToString(todos []todo.Todo, commentChar byte) ([]byte, error) {
+	buffer := bytes.Buffer{}
+	err := todo.Write(&buffer, todos, commentChar)
+	return buffer.Bytes(), err
+}
+
 func PrependStrToTodoFile(filePath string, linesToPrepend []byte) error {
 	existingContent, err := os.ReadFile(filePath)
 	if err != nil {
@@ -106,16 +113,21 @@ func PrependStrToTodoFile(filePath string, linesToPrepend []byte) error {
 	return os.WriteFile(filePath, linesToPrepend, 0o644)
 }
 
-func DeleteTodos(fileName string, todosToDelete []Todo, commentChar byte) error {
+// Unlike the other functions in this file, which write the changed todos file
+// back to disk, this one returns the new content as a byte slice. This is
+// because when deleting update-ref todos, we must perform a "git rebase
+// --edit-todo" command to pass the changed todos to git so that it can do some
+// housekeeping around the deleted todos. This can only be done by our caller.
+func DeleteTodos(fileName string, todosToDelete []Todo, commentChar byte) ([]byte, error) {
 	todos, err := ReadRebaseTodoFile(fileName, commentChar)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	rearrangedTodos, err := deleteTodos(todos, todosToDelete)
 	if err != nil {
-		return err
+		return nil, err
 	}
-	return WriteRebaseTodoFile(fileName, rearrangedTodos, commentChar)
+	return todosToString(rearrangedTodos, commentChar)
 }
 
 func deleteTodos(todos []todo.Todo, todosToDelete []Todo) ([]todo.Todo, error) {


Test patch:
diff --git a/pkg/integration/tests/interactive_rebase/delete_update_ref_todo.go b/pkg/integration/tests/interactive_rebase/delete_update_ref_todo.go
index d08f518ecf9..9bb216cfe2a 100644
--- a/pkg/integration/tests/interactive_rebase/delete_update_ref_todo.go
+++ b/pkg/integration/tests/interactive_rebase/delete_update_ref_todo.go
@@ -50,6 +50,8 @@ var DeleteUpdateRefTodo = NewIntegrationTest(NewIntegrationTestArgs{
 				Contains("pick").Contains("CI commit 02"),
 				Contains("CI ◯ <-- YOU ARE HERE --- commit 01"),
 			).
+			NavigateToLine(Contains("commit 02")).
+			Press(keys.Universal.Remove).
 			Tap(func() {
 				t.Common().ContinueRebase()
 			}).
@@ -57,9 +59,14 @@ var DeleteUpdateRefTodo = NewIntegrationTest(NewIntegrationTestArgs{
 				Contains("CI ◯ commit 06"),
 				Contains("CI ◯ commit 05"),
 				Contains("CI ◯ commit 04"),
-				Contains("CI ◯ commit 03"), // No start on this commit, so there's no branch head here
-				Contains("CI ◯ commit 02"),
+				Contains("CI ◯ commit 03"), // No star on this commit, so there's no branch head here
 				Contains("CI ◯ commit 01"),
 			)
+
+		t.Views().Branches().
+			Lines(
+				Contains("branch2"),
+				Contains("branch1"),
+			)
 	},
 })