From 69153acfdb6539b515bcb13a262bd38b12f27e9f Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 6 Apr 2024 15:05:41 +0200 Subject: [PATCH 1/4] Remove TODO.* from .gitignore It was added in 2018 (700f8c7e796), but I don't know for what purpose. It just took me 15 minutes to figure out why my new file todo.go wasn't added, so I'm removing this entry as I find it more harmful than helpful. --- .gitignore | 3 --- 1 file changed, 3 deletions(-) diff --git a/.gitignore b/.gitignore index 2053b9898..2fc9940dc 100644 --- a/.gitignore +++ b/.gitignore @@ -6,9 +6,6 @@ # Hidden .* -# TODO -TODO.* - # Notes *.notes From 7270dea48d5325089561ee6e6b6ba15ac1c6fa68 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 6 Apr 2024 14:45:49 +0200 Subject: [PATCH 2/4] Switch git-todo-parser from fsmiamoto original repo to stefanhaller's fork Sometimes it takes a while to get PRs accepted upstream, and this blocks our progress. Since I'm pretty much the only one making changes there anyway, it makes sense to point to my fork directly. --- go.mod | 2 +- go.sum | 4 ++-- pkg/app/daemon/daemon.go | 2 +- pkg/app/daemon/rebase.go | 2 +- pkg/commands/git_commands/commit_loader.go | 2 +- pkg/commands/git_commands/commit_loader_test.go | 2 +- pkg/commands/git_commands/patch.go | 2 +- pkg/commands/git_commands/rebase.go | 2 +- pkg/commands/models/commit.go | 2 +- pkg/gui/controllers/local_commits_controller.go | 2 +- pkg/gui/presentation/commits.go | 2 +- pkg/gui/presentation/commits_test.go | 2 +- pkg/gui/services/custom_commands/session_state_loader.go | 2 +- pkg/utils/rebase_todo.go | 2 +- pkg/utils/rebase_todo_test.go | 2 +- .../{fsmiamoto => stefanhaller}/git-todo-parser/LICENSE | 0 .../git-todo-parser/todo/parse.go | 2 +- .../git-todo-parser/todo/todo.go | 0 .../git-todo-parser/todo/write.go | 1 - vendor/modules.txt | 6 +++--- 20 files changed, 20 insertions(+), 21 deletions(-) rename vendor/github.com/{fsmiamoto => stefanhaller}/git-todo-parser/LICENSE (100%) rename vendor/github.com/{fsmiamoto => stefanhaller}/git-todo-parser/todo/parse.go (98%) rename vendor/github.com/{fsmiamoto => stefanhaller}/git-todo-parser/todo/todo.go (100%) rename vendor/github.com/{fsmiamoto => stefanhaller}/git-todo-parser/todo/write.go (99%) diff --git a/go.mod b/go.mod index 2768d0815..e084a0741 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,6 @@ require ( github.com/aybabtme/humanlog v0.4.1 github.com/cloudfoundry/jibber_jabber v0.0.0-20151120183258-bcc4c8345a21 github.com/creack/pty v1.1.11 - github.com/fsmiamoto/git-todo-parser v0.0.5 github.com/gdamore/tcell/v2 v2.7.4 github.com/go-errors/errors v1.5.1 github.com/gookit/color v1.4.2 @@ -34,6 +33,7 @@ require ( github.com/sirupsen/logrus v1.4.2 github.com/spf13/afero v1.9.5 github.com/spkg/bom v0.0.0-20160624110644-59b7046e48ad + github.com/stefanhaller/git-todo-parser v0.0.7-0.20240406123903-fd957137b6e2 github.com/stretchr/testify v1.8.1 github.com/xo/terminfo v0.0.0-20210125001918-ca9a967f8778 golang.org/x/exp v0.0.0-20220318154914-8dddf5d87bd8 diff --git a/go.sum b/go.sum index e9b355692..cdd64127c 100644 --- a/go.sum +++ b/go.sum @@ -84,8 +84,6 @@ github.com/fatih/color v1.7.1-0.20180516100307-2d684516a886/go.mod h1:Zm6kSWBoL9 github.com/fatih/color v1.9.0 h1:8xPHl4/q1VyqGIPif1F+1V3Y3lSmrq01EabUW3CoW5s= github.com/fatih/color v1.9.0/go.mod h1:eQcE1qtQxscV5RaZvpXrrb8Drkc3/DdQ+uUYCNjL+zU= github.com/flynn/go-shlex v0.0.0-20150515145356-3f9db97f8568/go.mod h1:xEzjJPgXI435gkrCt3MPfRiAkVrwSbHsst4LCFVfpJc= -github.com/fsmiamoto/git-todo-parser v0.0.5 h1:Bhzd/vz/6Qm3udfkd6NO9fWfD3TpwR9ucp3N75/J5I8= -github.com/fsmiamoto/git-todo-parser v0.0.5/go.mod h1:B+AgTbNE2BARvJqzXygThzqxLIaEWvwr2sxKYYb0Fas= github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo= github.com/gdamore/encoding v1.0.0/go.mod h1:alR0ol34c49FCSBLjhosxzcPHQbf2trDkoo5dl+VrEg= github.com/gdamore/encoding v1.0.1 h1:YzKZckdBL6jVt2Gc+5p82qhrGiqMdG/eNs6Wy0u3Uhw= @@ -281,6 +279,8 @@ github.com/spf13/afero v1.9.5 h1:stMpOSZFs//0Lv29HduCmli3GUfpFoF3Y1Q/aXj/wVM= github.com/spf13/afero v1.9.5/go.mod h1:UBogFpq8E9Hx+xc5CNTTEpTnuHVmXDwZcZcE1eb/UhQ= github.com/spkg/bom v0.0.0-20160624110644-59b7046e48ad h1:fiWzISvDn0Csy5H0iwgAuJGQTUpVfEMJJd4nRFXogbc= github.com/spkg/bom v0.0.0-20160624110644-59b7046e48ad/go.mod h1:qLr4V1qq6nMqFKkMo8ZTx3f+BZEkzsRUY10Xsm2mwU0= +github.com/stefanhaller/git-todo-parser v0.0.7-0.20240406123903-fd957137b6e2 h1:RTNWemd/9z9A5L/AggEP3OdhRz5dXETB/wdAlYF0SuM= +github.com/stefanhaller/git-todo-parser v0.0.7-0.20240406123903-fd957137b6e2/go.mod h1:HFt9hGqMzgQ+gVxMKcvTvGaFz4Y0yYycqqAp2V3wcJY= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= diff --git a/pkg/app/daemon/daemon.go b/pkg/app/daemon/daemon.go index e815b6e82..16dcce4a2 100644 --- a/pkg/app/daemon/daemon.go +++ b/pkg/app/daemon/daemon.go @@ -8,11 +8,11 @@ import ( "os/exec" "strconv" - "github.com/fsmiamoto/git-todo-parser/todo" "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/common" "github.com/jesseduffield/lazygit/pkg/utils" "github.com/samber/lo" + "github.com/stefanhaller/git-todo-parser/todo" ) // Sometimes lazygit will be invoked in daemon mode from a parent lazygit process. diff --git a/pkg/app/daemon/rebase.go b/pkg/app/daemon/rebase.go index 0ca323c7d..1ab91de8e 100644 --- a/pkg/app/daemon/rebase.go +++ b/pkg/app/daemon/rebase.go @@ -5,11 +5,11 @@ import ( "path/filepath" "strings" - "github.com/fsmiamoto/git-todo-parser/todo" "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/common" "github.com/jesseduffield/lazygit/pkg/env" "github.com/samber/lo" + "github.com/stefanhaller/git-todo-parser/todo" ) type TodoLine struct { diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index 50d9db709..48545edb9 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -11,13 +11,13 @@ import ( "strings" "sync" - "github.com/fsmiamoto/git-todo-parser/todo" "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/commands/oscommands" "github.com/jesseduffield/lazygit/pkg/commands/types/enums" "github.com/jesseduffield/lazygit/pkg/common" "github.com/jesseduffield/lazygit/pkg/utils" "github.com/samber/lo" + "github.com/stefanhaller/git-todo-parser/todo" ) // context: diff --git a/pkg/commands/git_commands/commit_loader_test.go b/pkg/commands/git_commands/commit_loader_test.go index 38c03625f..7f60209a4 100644 --- a/pkg/commands/git_commands/commit_loader_test.go +++ b/pkg/commands/git_commands/commit_loader_test.go @@ -5,13 +5,13 @@ import ( "strings" "testing" - "github.com/fsmiamoto/git-todo-parser/todo" "github.com/go-errors/errors" "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/commands/oscommands" "github.com/jesseduffield/lazygit/pkg/commands/types/enums" "github.com/jesseduffield/lazygit/pkg/config" "github.com/jesseduffield/lazygit/pkg/utils" + "github.com/stefanhaller/git-todo-parser/todo" "github.com/stretchr/testify/assert" ) diff --git a/pkg/commands/git_commands/patch.go b/pkg/commands/git_commands/patch.go index fa8436101..3d18bf3e2 100644 --- a/pkg/commands/git_commands/patch.go +++ b/pkg/commands/git_commands/patch.go @@ -4,12 +4,12 @@ import ( "path/filepath" "time" - "github.com/fsmiamoto/git-todo-parser/todo" "github.com/go-errors/errors" "github.com/jesseduffield/lazygit/pkg/app/daemon" "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/commands/patch" "github.com/jesseduffield/lazygit/pkg/commands/types/enums" + "github.com/stefanhaller/git-todo-parser/todo" ) type PatchCommands struct { diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index f7b8b43dc..ff7ecec09 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/pkg/commands/git_commands/rebase.go @@ -5,13 +5,13 @@ import ( "path/filepath" "strings" - "github.com/fsmiamoto/git-todo-parser/todo" "github.com/go-errors/errors" "github.com/jesseduffield/lazygit/pkg/app/daemon" "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/commands/oscommands" "github.com/jesseduffield/lazygit/pkg/utils" "github.com/samber/lo" + "github.com/stefanhaller/git-todo-parser/todo" ) type RebaseCommands struct { diff --git a/pkg/commands/models/commit.go b/pkg/commands/models/commit.go index 64b89db8f..95e3b9b18 100644 --- a/pkg/commands/models/commit.go +++ b/pkg/commands/models/commit.go @@ -3,8 +3,8 @@ package models import ( "fmt" - "github.com/fsmiamoto/git-todo-parser/todo" "github.com/jesseduffield/lazygit/pkg/utils" + "github.com/stefanhaller/git-todo-parser/todo" ) // Special commit hash for empty tree object diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 1336f5237..1121e141e 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -4,7 +4,6 @@ import ( "fmt" "strings" - "github.com/fsmiamoto/git-todo-parser/todo" "github.com/go-errors/errors" "github.com/jesseduffield/gocui" "github.com/jesseduffield/lazygit/pkg/commands/models" @@ -16,6 +15,7 @@ import ( "github.com/jesseduffield/lazygit/pkg/gui/types" "github.com/jesseduffield/lazygit/pkg/utils" "github.com/samber/lo" + "github.com/stefanhaller/git-todo-parser/todo" ) // after selecting the 200th commit, we'll load in all the rest diff --git a/pkg/gui/presentation/commits.go b/pkg/gui/presentation/commits.go index 1fc2c7d91..827648dda 100644 --- a/pkg/gui/presentation/commits.go +++ b/pkg/gui/presentation/commits.go @@ -5,7 +5,6 @@ import ( "strings" "time" - "github.com/fsmiamoto/git-todo-parser/todo" "github.com/jesseduffield/generics/set" "github.com/jesseduffield/lazygit/pkg/commands/git_commands" "github.com/jesseduffield/lazygit/pkg/commands/models" @@ -19,6 +18,7 @@ import ( "github.com/kyokomi/emoji/v2" "github.com/samber/lo" "github.com/sasha-s/go-deadlock" + "github.com/stefanhaller/git-todo-parser/todo" ) type pipeSetCacheKey struct { diff --git a/pkg/gui/presentation/commits_test.go b/pkg/gui/presentation/commits_test.go index 8f43da50d..0438b105c 100644 --- a/pkg/gui/presentation/commits_test.go +++ b/pkg/gui/presentation/commits_test.go @@ -6,12 +6,12 @@ import ( "testing" "time" - "github.com/fsmiamoto/git-todo-parser/todo" "github.com/gookit/color" "github.com/jesseduffield/generics/set" "github.com/jesseduffield/lazygit/pkg/commands/git_commands" "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/utils" + "github.com/stefanhaller/git-todo-parser/todo" "github.com/stretchr/testify/assert" "github.com/xo/terminfo" ) diff --git a/pkg/gui/services/custom_commands/session_state_loader.go b/pkg/gui/services/custom_commands/session_state_loader.go index 93ce9d179..6a3068df8 100644 --- a/pkg/gui/services/custom_commands/session_state_loader.go +++ b/pkg/gui/services/custom_commands/session_state_loader.go @@ -1,9 +1,9 @@ package custom_commands import ( - "github.com/fsmiamoto/git-todo-parser/todo" "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/gui/controllers/helpers" + "github.com/stefanhaller/git-todo-parser/todo" ) // loads the session state at the time that a custom command is invoked, for use diff --git a/pkg/utils/rebase_todo.go b/pkg/utils/rebase_todo.go index 216d56f26..20c2e750e 100644 --- a/pkg/utils/rebase_todo.go +++ b/pkg/utils/rebase_todo.go @@ -6,8 +6,8 @@ import ( "os" "strings" - "github.com/fsmiamoto/git-todo-parser/todo" "github.com/samber/lo" + "github.com/stefanhaller/git-todo-parser/todo" ) type Todo struct { diff --git a/pkg/utils/rebase_todo_test.go b/pkg/utils/rebase_todo_test.go index 913b055cd..8265cf4cf 100644 --- a/pkg/utils/rebase_todo_test.go +++ b/pkg/utils/rebase_todo_test.go @@ -4,7 +4,7 @@ import ( "errors" "testing" - "github.com/fsmiamoto/git-todo-parser/todo" + "github.com/stefanhaller/git-todo-parser/todo" "github.com/stretchr/testify/assert" ) diff --git a/vendor/github.com/fsmiamoto/git-todo-parser/LICENSE b/vendor/github.com/stefanhaller/git-todo-parser/LICENSE similarity index 100% rename from vendor/github.com/fsmiamoto/git-todo-parser/LICENSE rename to vendor/github.com/stefanhaller/git-todo-parser/LICENSE diff --git a/vendor/github.com/fsmiamoto/git-todo-parser/todo/parse.go b/vendor/github.com/stefanhaller/git-todo-parser/todo/parse.go similarity index 98% rename from vendor/github.com/fsmiamoto/git-todo-parser/todo/parse.go rename to vendor/github.com/stefanhaller/git-todo-parser/todo/parse.go index 51efd305d..12bec439b 100644 --- a/vendor/github.com/fsmiamoto/git-todo-parser/todo/parse.go +++ b/vendor/github.com/stefanhaller/git-todo-parser/todo/parse.go @@ -71,7 +71,7 @@ func parseLine(line string, commentChar byte) (Todo, error) { return todo, ErrUnexpectedCommand } - if todo.Command == Break { + if todo.Command == Break || todo.Command == NoOp { return todo, nil } diff --git a/vendor/github.com/fsmiamoto/git-todo-parser/todo/todo.go b/vendor/github.com/stefanhaller/git-todo-parser/todo/todo.go similarity index 100% rename from vendor/github.com/fsmiamoto/git-todo-parser/todo/todo.go rename to vendor/github.com/stefanhaller/git-todo-parser/todo/todo.go diff --git a/vendor/github.com/fsmiamoto/git-todo-parser/todo/write.go b/vendor/github.com/stefanhaller/git-todo-parser/todo/write.go similarity index 99% rename from vendor/github.com/fsmiamoto/git-todo-parser/todo/write.go rename to vendor/github.com/stefanhaller/git-todo-parser/todo/write.go index 949db420a..279de48a0 100644 --- a/vendor/github.com/fsmiamoto/git-todo-parser/todo/write.go +++ b/vendor/github.com/stefanhaller/git-todo-parser/todo/write.go @@ -23,7 +23,6 @@ func writeTodo(f io.Writer, todo Todo, commentChar byte) error { switch todo.Command { case NoOp: - return nil case Comment: sb.WriteByte(commentChar) diff --git a/vendor/modules.txt b/vendor/modules.txt index 9bedc28a2..425c1705b 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -34,9 +34,6 @@ github.com/emirpasic/gods/utils # github.com/fatih/color v1.9.0 ## explicit; go 1.13 github.com/fatih/color -# github.com/fsmiamoto/git-todo-parser v0.0.5 -## explicit; go 1.13 -github.com/fsmiamoto/git-todo-parser/todo # github.com/gdamore/encoding v1.0.1 ## explicit; go 1.9 github.com/gdamore/encoding @@ -269,6 +266,9 @@ github.com/spf13/afero/mem # github.com/spkg/bom v0.0.0-20160624110644-59b7046e48ad ## explicit github.com/spkg/bom +# github.com/stefanhaller/git-todo-parser v0.0.7-0.20240406123903-fd957137b6e2 +## explicit; go 1.13 +github.com/stefanhaller/git-todo-parser/todo # github.com/stretchr/testify v1.8.1 ## explicit; go 1.13 github.com/stretchr/testify/assert From af6d072cc62f6f1bdb153d2669655d5d87802fac Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 27 Mar 2024 07:23:44 +0100 Subject: [PATCH 3/4] Add tests demonstrating undesired behavior with update-ref todos for copied branches These tests succeed here, but have comments explaining which bits are undesired. See next commit for a more detailed explanation why. --- .../tests/branch/rebase_copied_branch.go | 69 +++++++++++++++++++ ...commit_in_copied_branch_with_update_ref.go | 55 +++++++++++++++ .../interactive_rebase_of_copied_branch.go | 41 +++++++++++ pkg/integration/tests/test_list.go | 3 + 4 files changed, 168 insertions(+) create mode 100644 pkg/integration/tests/branch/rebase_copied_branch.go create mode 100644 pkg/integration/tests/interactive_rebase/drop_commit_in_copied_branch_with_update_ref.go create mode 100644 pkg/integration/tests/interactive_rebase/interactive_rebase_of_copied_branch.go diff --git a/pkg/integration/tests/branch/rebase_copied_branch.go b/pkg/integration/tests/branch/rebase_copied_branch.go new file mode 100644 index 000000000..162c69af7 --- /dev/null +++ b/pkg/integration/tests/branch/rebase_copied_branch.go @@ -0,0 +1,69 @@ +package branch + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var RebaseCopiedBranch = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Make a copy of a branch, rebase it, check that the original branch is unaffected", + ExtraCmdArgs: []string{}, + Skip: false, + GitVersion: AtLeast("2.38.0"), + SetupConfig: func(config *config.AppConfig) { + config.AppState.GitLogShowGraph = "never" + }, + SetupRepo: func(shell *Shell) { + shell. + EmptyCommit("master 1"). + EmptyCommit("master 2"). + NewBranchFrom("branch1", "master^"). + EmptyCommit("branch 1"). + EmptyCommit("branch 2"). + NewBranch("branch2") + + shell.SetConfig("rebase.updateRefs", "true") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits().Lines( + Contains("CI * branch 2"), + Contains("CI branch 1"), + Contains("CI master 1"), + ) + + t.Views().Branches(). + Focus(). + Lines( + Contains("branch2").IsSelected(), + Contains("branch1"), + Contains("master"), + ). + NavigateToLine(Contains("master")). + Press(keys.Branches.RebaseBranch). + Tap(func() { + t.ExpectPopup().Menu(). + Title(Equals("Rebase 'branch2' onto 'master'")). + Select(Contains("Simple rebase")). + Confirm() + }) + + t.Views().Commits().Lines( + Contains("CI * branch 2"), // wrong, don't want a star here + Contains("CI branch 1"), + Contains("CI master 2"), + Contains("CI master 1"), + ) + + t.Views().Branches(). + Focus(). + NavigateToLine(Contains("branch1")). + PressPrimaryAction() + + t.Views().Commits().Lines( + Contains("CI * branch 2"), // wrong, don't want a star here + Contains("CI branch 1"), + Contains("CI master 2"), // wrong, don't want this commit + Contains("CI master 1"), + ) + }, +}) diff --git a/pkg/integration/tests/interactive_rebase/drop_commit_in_copied_branch_with_update_ref.go b/pkg/integration/tests/interactive_rebase/drop_commit_in_copied_branch_with_update_ref.go new file mode 100644 index 000000000..567e746a5 --- /dev/null +++ b/pkg/integration/tests/interactive_rebase/drop_commit_in_copied_branch_with_update_ref.go @@ -0,0 +1,55 @@ +package interactive_rebase + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var DropCommitInCopiedBranchWithUpdateRef = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Drops a commit in a branch that is a copy of another branch, and verify that the other branch is left alone", + ExtraCmdArgs: []string{}, + Skip: false, + GitVersion: AtLeast("2.38.0"), + SetupConfig: func(config *config.AppConfig) { + config.AppState.GitLogShowGraph = "never" + }, + SetupRepo: func(shell *Shell) { + shell. + NewBranch("branch1"). + CreateNCommits(3). + NewBranch("branch2") + + shell.SetConfig("rebase.updateRefs", "true") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("CI * commit 03").IsSelected(), + Contains("CI commit 02"), + Contains("CI commit 01"), + ). + NavigateToLine(Contains("commit 02")). + Press(keys.Universal.Remove). + Tap(func() { + t.ExpectPopup().Confirmation(). + Title(Equals("Drop commit")). + Content(Equals("Are you sure you want to drop the selected commit(s)?")). + Confirm() + }). + Lines( + Contains("CI * commit 03"), // don't want a star here because branch1 should no longer be pointing to it + Contains("CI commit 01"), + ) + + t.Views().Branches(). + Focus(). + NavigateToLine(Contains("branch1")). + PressPrimaryAction() + + t.Views().Commits().Lines( + Contains("CI * commit 03"), // branch1 has changed like branch2, but shouldn't have + Contains("CI commit 01"), + ) + }, +}) diff --git a/pkg/integration/tests/interactive_rebase/interactive_rebase_of_copied_branch.go b/pkg/integration/tests/interactive_rebase/interactive_rebase_of_copied_branch.go new file mode 100644 index 000000000..d29cbe102 --- /dev/null +++ b/pkg/integration/tests/interactive_rebase/interactive_rebase_of_copied_branch.go @@ -0,0 +1,41 @@ +package interactive_rebase + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var InteractiveRebaseOfCopiedBranch = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Check that interactively rebasing a branch that is a copy of another branch doesn't affect the original branch", + ExtraCmdArgs: []string{}, + Skip: false, + GitVersion: AtLeast("2.38.0"), + SetupConfig: func(config *config.AppConfig) { + config.AppState.GitLogShowGraph = "never" + }, + SetupRepo: func(shell *Shell) { + shell. + NewBranch("branch1"). + CreateNCommits(3). + NewBranch("branch2") + + shell.SetConfig("rebase.updateRefs", "true") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("CI * commit 03"), + Contains("CI commit 02"), + Contains("CI commit 01"), + ). + NavigateToLine(Contains("commit 01")). + Press(keys.Universal.Edit). + Lines( + Contains("update-ref").Contains("branch1"), // we don't want this + Contains("pick").Contains("CI commit 03"), + Contains("pick").Contains("CI commit 02"), + Contains("CI <-- YOU ARE HERE --- commit 01"), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index cb1a66ac5..cdbcc7871 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -47,6 +47,7 @@ var tests = []*components.IntegrationTest{ branch.RebaseAbortOnConflict, branch.RebaseAndDrop, branch.RebaseCancelOnConflict, + branch.RebaseCopiedBranch, branch.RebaseDoesNotAutosquash, branch.RebaseFromMarkedBase, branch.RebaseToUpstream, @@ -170,6 +171,7 @@ var tests = []*components.IntegrationTest{ interactive_rebase.AmendNonHeadCommitDuringRebase, interactive_rebase.DeleteUpdateRefTodo, interactive_rebase.DontShowBranchHeadsForTodoItems, + interactive_rebase.DropCommitInCopiedBranchWithUpdateRef, interactive_rebase.DropTodoCommitWithUpdateRef, interactive_rebase.DropWithCustomCommentChar, interactive_rebase.EditFirstCommit, @@ -178,6 +180,7 @@ var tests = []*components.IntegrationTest{ interactive_rebase.EditTheConflCommit, interactive_rebase.FixupFirstCommit, interactive_rebase.FixupSecondCommit, + interactive_rebase.InteractiveRebaseOfCopiedBranch, interactive_rebase.MidRebaseRangeSelect, interactive_rebase.Move, interactive_rebase.MoveInRebase, From 8b99a3c949c5397bb9c7efe4d2258e467c00be4b Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 24 Mar 2024 16:59:47 +0100 Subject: [PATCH 4/4] Drop update-ref commands at the top of the rebase-todo file The rebase.updateRefs feature of git is very useful to rebase a stack of branches and keep everything nicely stacked; however, it is usually in the way when you make a copy of a branch and want to rebase it "away" from the original branch in some way or other. For example, the original branch might sit on main, and you want to rebase the copy onto devel to see if things still compile there. Or you want to do some heavy history rewriting experiments on the copy, but keep the original branch in case the experiments fail. Or you want to split a branch in two because it contains two unrelated sets of changes; so you make a copy, and drop half of the commits from the copy, then check out the original branch and drop the other half of the commits from it. In all these cases, git's updateRefs feature insists on moving the original branch along with the copy in the first rebase that you make on the copy. I think this is a bug in git, it should create update-ref todos only for branches that point into the middle of your branch (because only then do they form a stack), not when they point at the head (because then it's a copy). I had a long discussion about this on the git mailing list [1], but people either don't agree or don't care enough. So we fix this on our side: whenever we start a rebase for whatever reason, be it interactive, non-interactive, or behind-the-scenes, we drop any update-ref todos that are at the very top of the todo list, which fixes all the above-mentioned scenarios nicely. I will admit that there's one scenario where git's behavior is the desired one, and the fix in this PR makes it worse: when you create a new branch off of an existing one, with the intention of creating a stack of branches, but before you make the first commit on the new branch you realize some problem with the first branch (e.g. a commit that needs to be reworded or dropped). It this case you do want both branches to be affected by the change. In my experience this scenario is much rarer than the other ones that I described above, and it's also much easier to recover from: just check out the other branch again and hard-reset it to the rebased one. [1] https://public-inbox.org/git/354f9fed-567f-42c8-9da9-148a5e223022@haller-berlin.de/ --- pkg/app/daemon/daemon.go | 38 +++++++++++++++---- pkg/app/daemon/rebase.go | 5 +++ pkg/commands/git_commands/rebase.go | 2 +- .../tests/branch/rebase_copied_branch.go | 5 +-- ...commit_in_copied_branch_with_update_ref.go | 5 ++- .../interactive_rebase_of_copied_branch.go | 2 +- pkg/utils/rebase_todo.go | 26 +++++++++++++ 7 files changed, 68 insertions(+), 15 deletions(-) diff --git a/pkg/app/daemon/daemon.go b/pkg/app/daemon/daemon.go index 16dcce4a2..045491fce 100644 --- a/pkg/app/daemon/daemon.go +++ b/pkg/app/daemon/daemon.go @@ -33,6 +33,7 @@ const ( DaemonKindUnknown DaemonKind = iota DaemonKindExitImmediately + DaemonKindRemoveUpdateRefsForCopiedBranch DaemonKindCherryPick DaemonKindMoveTodosUp DaemonKindMoveTodosDown @@ -53,14 +54,15 @@ func getInstruction() Instruction { jsonData := os.Getenv(DaemonInstructionEnvKey) mapping := map[DaemonKind]func(string) Instruction{ - DaemonKindExitImmediately: deserializeInstruction[*ExitImmediatelyInstruction], - DaemonKindCherryPick: deserializeInstruction[*CherryPickCommitsInstruction], - DaemonKindChangeTodoActions: deserializeInstruction[*ChangeTodoActionsInstruction], - DaemonKindMoveFixupCommitDown: deserializeInstruction[*MoveFixupCommitDownInstruction], - DaemonKindMoveTodosUp: deserializeInstruction[*MoveTodosUpInstruction], - DaemonKindMoveTodosDown: deserializeInstruction[*MoveTodosDownInstruction], - DaemonKindInsertBreak: deserializeInstruction[*InsertBreakInstruction], - DaemonKindWriteRebaseTodo: deserializeInstruction[*WriteRebaseTodoInstruction], + DaemonKindExitImmediately: deserializeInstruction[*ExitImmediatelyInstruction], + DaemonKindRemoveUpdateRefsForCopiedBranch: deserializeInstruction[*RemoveUpdateRefsForCopiedBranchInstruction], + DaemonKindCherryPick: deserializeInstruction[*CherryPickCommitsInstruction], + DaemonKindChangeTodoActions: deserializeInstruction[*ChangeTodoActionsInstruction], + DaemonKindMoveFixupCommitDown: deserializeInstruction[*MoveFixupCommitDownInstruction], + DaemonKindMoveTodosUp: deserializeInstruction[*MoveTodosUpInstruction], + DaemonKindMoveTodosDown: deserializeInstruction[*MoveTodosDownInstruction], + DaemonKindInsertBreak: deserializeInstruction[*InsertBreakInstruction], + DaemonKindWriteRebaseTodo: deserializeInstruction[*WriteRebaseTodoInstruction], } return mapping[getDaemonKind()](jsonData) @@ -157,6 +159,26 @@ func NewExitImmediatelyInstruction() Instruction { return &ExitImmediatelyInstruction{} } +type RemoveUpdateRefsForCopiedBranchInstruction struct{} + +func (self *RemoveUpdateRefsForCopiedBranchInstruction) Kind() DaemonKind { + return DaemonKindRemoveUpdateRefsForCopiedBranch +} + +func (self *RemoveUpdateRefsForCopiedBranchInstruction) SerializedInstructions() string { + return serializeInstruction(self) +} + +func (self *RemoveUpdateRefsForCopiedBranchInstruction) run(common *common.Common) error { + return handleInteractiveRebase(common, func(path string) error { + return nil + }) +} + +func NewRemoveUpdateRefsForCopiedBranchInstruction() Instruction { + return &RemoveUpdateRefsForCopiedBranchInstruction{} +} + type CherryPickCommitsInstruction struct { Todo string } diff --git a/pkg/app/daemon/rebase.go b/pkg/app/daemon/rebase.go index 1ab91de8e..8cc16d3b1 100644 --- a/pkg/app/daemon/rebase.go +++ b/pkg/app/daemon/rebase.go @@ -8,6 +8,7 @@ import ( "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/common" "github.com/jesseduffield/lazygit/pkg/env" + "github.com/jesseduffield/lazygit/pkg/utils" "github.com/samber/lo" "github.com/stefanhaller/git-todo-parser/todo" ) @@ -44,6 +45,10 @@ func handleInteractiveRebase(common *common.Common, f func(path string) error) e path := os.Args[1] if strings.HasSuffix(path, "git-rebase-todo") { + err := utils.RemoveUpdateRefsForCopiedBranch(path, getCommentChar()) + if err != nil { + return err + } return f(path) } else if strings.HasSuffix(path, filepath.Join(gitDir(), "COMMIT_EDITMSG")) { // TODO: test // if we are rebasing and squashing, we'll see a COMMIT_EDITMSG diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index ff7ecec09..040df0070 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/pkg/commands/git_commands/rebase.go @@ -233,7 +233,7 @@ func (self *RebaseCommands) PrepareInteractiveRebaseCommand(opts PrepareInteract if opts.instruction != nil { cmdObj.AddEnvVars(daemon.ToEnvVars(opts.instruction)...) } else { - gitSequenceEditor = "true" + cmdObj.AddEnvVars(daemon.ToEnvVars(daemon.NewRemoveUpdateRefsForCopiedBranchInstruction())...) } cmdObj.AddEnvVars( diff --git a/pkg/integration/tests/branch/rebase_copied_branch.go b/pkg/integration/tests/branch/rebase_copied_branch.go index 162c69af7..faa31093e 100644 --- a/pkg/integration/tests/branch/rebase_copied_branch.go +++ b/pkg/integration/tests/branch/rebase_copied_branch.go @@ -48,7 +48,7 @@ var RebaseCopiedBranch = NewIntegrationTest(NewIntegrationTestArgs{ }) t.Views().Commits().Lines( - Contains("CI * branch 2"), // wrong, don't want a star here + Contains("CI branch 2"), Contains("CI branch 1"), Contains("CI master 2"), Contains("CI master 1"), @@ -60,9 +60,8 @@ var RebaseCopiedBranch = NewIntegrationTest(NewIntegrationTestArgs{ PressPrimaryAction() t.Views().Commits().Lines( - Contains("CI * branch 2"), // wrong, don't want a star here + Contains("CI branch 2"), Contains("CI branch 1"), - Contains("CI master 2"), // wrong, don't want this commit Contains("CI master 1"), ) }, diff --git a/pkg/integration/tests/interactive_rebase/drop_commit_in_copied_branch_with_update_ref.go b/pkg/integration/tests/interactive_rebase/drop_commit_in_copied_branch_with_update_ref.go index 567e746a5..8ea13c6e2 100644 --- a/pkg/integration/tests/interactive_rebase/drop_commit_in_copied_branch_with_update_ref.go +++ b/pkg/integration/tests/interactive_rebase/drop_commit_in_copied_branch_with_update_ref.go @@ -38,7 +38,7 @@ var DropCommitInCopiedBranchWithUpdateRef = NewIntegrationTest(NewIntegrationTes Confirm() }). Lines( - Contains("CI * commit 03"), // don't want a star here because branch1 should no longer be pointing to it + Contains("CI commit 03"), // no start on this commit because branch1 is no longer pointing to it Contains("CI commit 01"), ) @@ -48,7 +48,8 @@ var DropCommitInCopiedBranchWithUpdateRef = NewIntegrationTest(NewIntegrationTes PressPrimaryAction() t.Views().Commits().Lines( - Contains("CI * commit 03"), // branch1 has changed like branch2, but shouldn't have + Contains("CI commit 03"), + Contains("CI commit 02"), Contains("CI commit 01"), ) }, diff --git a/pkg/integration/tests/interactive_rebase/interactive_rebase_of_copied_branch.go b/pkg/integration/tests/interactive_rebase/interactive_rebase_of_copied_branch.go index d29cbe102..4bb3b86c7 100644 --- a/pkg/integration/tests/interactive_rebase/interactive_rebase_of_copied_branch.go +++ b/pkg/integration/tests/interactive_rebase/interactive_rebase_of_copied_branch.go @@ -32,7 +32,7 @@ var InteractiveRebaseOfCopiedBranch = NewIntegrationTest(NewIntegrationTestArgs{ NavigateToLine(Contains("commit 01")). Press(keys.Universal.Edit). Lines( - Contains("update-ref").Contains("branch1"), // we don't want this + // No update-ref todo for branch1 here, even though command-line git would have added it Contains("pick").Contains("CI commit 03"), Contains("pick").Contains("CI commit 02"), Contains("CI <-- YOU ARE HERE --- commit 01"), diff --git a/pkg/utils/rebase_todo.go b/pkg/utils/rebase_todo.go index 20c2e750e..e678a8f4e 100644 --- a/pkg/utils/rebase_todo.go +++ b/pkg/utils/rebase_todo.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "os" + "slices" "strings" "github.com/samber/lo" @@ -262,6 +263,31 @@ func moveFixupCommitDown(todos []todo.Todo, originalHash string, fixupHash strin return newTodos, nil } +func RemoveUpdateRefsForCopiedBranch(fileName string, commentChar byte) error { + todos, err := ReadRebaseTodoFile(fileName, commentChar) + if err != nil { + return err + } + + // Filter out comments + todos = lo.Filter(todos, func(t todo.Todo, _ int) bool { + return t.Command != todo.Comment + }) + + // Delete any update-ref todos at the end of the todo list. These are not + // part of a stack of branches, and so shouldn't be updated. This makes it + // possible to create a copy of a branch and rebase the copy without + // affecting the original branch. + if _, i, found := lo.FindLastIndexOf(todos, func(t todo.Todo) bool { + return t.Command != todo.UpdateRef + }); found && i < len(todos)-1 { + todos = slices.Delete(todos, i+1, len(todos)) + return WriteRebaseTodoFile(fileName, todos, commentChar) + } + + return nil +} + // We render a todo in the commits view if it's a commit or if it's an // update-ref. We don't render label, reset, or comment lines. func isRenderedTodo(t todo.Todo) bool {