From cdbec3997dac0801483f19382a0f0382fcea48f5 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 23 Mar 2024 15:34:39 +0100 Subject: [PATCH 1/4] Cleanup: fix typo in test comment --- .../tests/interactive_rebase/delete_update_ref_todo.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 d08f518ec..51691fb99 100644 --- a/pkg/integration/tests/interactive_rebase/delete_update_ref_todo.go +++ b/pkg/integration/tests/interactive_rebase/delete_update_ref_todo.go @@ -57,7 +57,7 @@ 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 03"), // No star on this commit, so there's no branch head here Contains("CI ◯ commit 02"), Contains("CI ◯ commit 01"), ) From ba85f93fb343d86a00686911411b139fcc613497 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 23 Mar 2024 15:40:07 +0100 Subject: [PATCH 2/4] Extend delete_update_ref_todo test to actually test what it was supposed to In the test we simply removed the update-ref todo but didn't make any other changes to the todos. This should really have kept everything the way it was, including the other branch head. The fact that the star was gone was really because of the bug that we are going to fix later in the branch. Change the test so that it also makes a change before the update-ref todo; this way we test that the star is gone because we deleted the update-ref, not because of the bug. To guard against the bug, we add another assertion for the branches view to test that both branches are still there. This currently fails. --- .../interactive_rebase/delete_update_ref_todo.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) 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 51691fb99..1969be58e 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() }). @@ -58,8 +60,15 @@ var DeleteUpdateRefTodo = NewIntegrationTest(NewIntegrationTestArgs{ Contains("CI ◯ commit 05"), Contains("CI ◯ commit 04"), Contains("CI ◯ commit 03"), // No star on this commit, so there's no branch head here - Contains("CI ◯ commit 02"), Contains("CI ◯ commit 01"), ) + + t.Views().Branches(). + Lines( + Contains("branch2"), + /* branch1 was deleted, which is wrong: + Contains("branch1"), + */ + ) }, }) From 6da9d55e47b54dabc226d9549ddb62ce7dc3c0d4 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 23 Mar 2024 14:41:46 +0100 Subject: [PATCH 3/4] Cleanup: update outdated comment We used to pass the todo string, but this has changed to instructions a while ago. --- pkg/commands/git_commands/rebase.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index eeae66167..874cbd809 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() From 1fdcc29277d48fddaa6d02cabba10552eac7ef14 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 23 Mar 2024 15:45:42 +0100 Subject: [PATCH 4/4] Fix deleting update-ref todos It is a bad idea to read a git-rebase-todo file, remove some update-ref todos, and write it back out behind git's back. This will cause git to actually remove the branches referenced by those update-ref todos when the rebase is continued. The reason is that git remembers the refs affected by update-ref todos at the beginning of the rebase, and remembers information about them in the file .git/rebase-merge/update-refs. Then, whenever the user performs a "git rebase --edit-todo" command, it updates that file based on whether update-ref todos were added or removed by that edit. If we rewrite the git-rebase-todo file behind git's back, this updating doesn't happen. Fix this by not updating the git-rebase-todo file directly in this case, but performing a "git rebase --edit-todo" command where we set ourselves as the editor and change the file in there. This makes git update the bookkeeping information properly. Ideally we would use this method for all cases where we change the git-rebase-todo file (e.g. moving todos up/down, or changing the type of a todo); this would be cleaner because we wouldn't mess with git's private implementation details. I tried this, but unfortunately it isn't fast enough. Right now, moving a todo up or down takes between 1 and 2ms on my machine; changing it to do a "git rebase --edit-todo" slows it down to over 100ms, which is unacceptable. --- pkg/app/daemon/daemon.go | 26 +++++++++++++ pkg/commands/git_commands/rebase.go | 37 ++++++++++++++++++- .../delete_update_ref_todo.go | 2 - pkg/utils/rebase_todo.go | 20 ++++++++-- 4 files changed, 78 insertions(+), 7 deletions(-) diff --git a/pkg/app/daemon/daemon.go b/pkg/app/daemon/daemon.go index 70490aef0..2f1cded7c 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 874cbd809..2d1de707f 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/pkg/commands/git_commands/rebase.go @@ -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/integration/tests/interactive_rebase/delete_update_ref_todo.go b/pkg/integration/tests/interactive_rebase/delete_update_ref_todo.go index 1969be58e..9bb216cfe 100644 --- a/pkg/integration/tests/interactive_rebase/delete_update_ref_todo.go +++ b/pkg/integration/tests/interactive_rebase/delete_update_ref_todo.go @@ -66,9 +66,7 @@ var DeleteUpdateRefTodo = NewIntegrationTest(NewIntegrationTestArgs{ t.Views().Branches(). Lines( Contains("branch2"), - /* branch1 was deleted, which is wrong: Contains("branch1"), - */ ) }, }) diff --git a/pkg/utils/rebase_todo.go b/pkg/utils/rebase_todo.go index 3403eef97..f2b60a097 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) {