From 3928d0ebda6952975059405544183bb1e63c0e1f Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 26 Feb 2023 11:51:02 +0100 Subject: [PATCH] Insert fake todo entry for a conflicting commit that is being applied When stopping in a rebase because of a conflict, it is nice to see the commit that git is trying to apply. Create a fake todo entry labelled "conflict" for this, and show the "<-- YOU ARE HERE ---" string for that one (in red) instead of for the real current head. --- pkg/commands/git_commands/commit_loader.go | 98 ++++++++++ .../git_commands/commit_loader_test.go | 177 ++++++++++++++++++ pkg/commands/models/commit.go | 2 + .../controllers/local_commits_controller.go | 6 +- pkg/gui/presentation/commits.go | 14 +- .../tests/branch/rebase_and_drop.go | 6 +- .../amend_commit_with_conflict.go | 8 +- .../edit_the_confl_commit.go | 47 +++++ .../tests/interactive_rebase/shared.go | 4 +- .../sync/pull_rebase_interactive_conflict.go | 3 +- .../pull_rebase_interactive_conflict_drop.go | 6 +- pkg/integration/tests/test_list.go | 1 + 12 files changed, 357 insertions(+), 15 deletions(-) create mode 100644 pkg/integration/tests/interactive_rebase/edit_the_confl_commit.go diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index 777f13d5c..e5b9ab0f4 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -310,6 +310,17 @@ func (self *CommitLoader) getInteractiveRebasingCommits() ([]*models.Commit, err return nil, nil } + // See if the current commit couldn't be applied because it conflicted; if + // so, add a fake entry for it + if conflictedCommitSha := self.getConflictedCommit(todos); conflictedCommitSha != "" { + commits = append(commits, &models.Commit{ + Sha: conflictedCommitSha, + Name: "", + Status: models.StatusRebasing, + Action: models.ActionConflict, + }) + } + for _, t := range todos { if t.Command == todo.UpdateRef { t.Msg = strings.TrimPrefix(t.Ref, "refs/heads/") @@ -328,6 +339,93 @@ func (self *CommitLoader) getInteractiveRebasingCommits() ([]*models.Commit, err return commits, nil } +func (self *CommitLoader) getConflictedCommit(todos []todo.Todo) string { + bytesContent, err := self.readFile(filepath.Join(self.dotGitDir, "rebase-merge/done")) + if err != nil { + self.Log.Error(fmt.Sprintf("error occurred reading rebase-merge/done: %s", err.Error())) + return "" + } + + doneTodos, err := todo.Parse(bytes.NewBuffer(bytesContent)) + if err != nil { + self.Log.Error(fmt.Sprintf("error occurred while parsing rebase-merge/done file: %s", err.Error())) + return "" + } + + amendFileExists := false + if _, err := os.Stat(filepath.Join(self.dotGitDir, "rebase-merge/amend")); err == nil { + amendFileExists = true + } + + return self.getConflictedCommitImpl(todos, doneTodos, amendFileExists) +} + +func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos []todo.Todo, amendFileExists bool) string { + // Should never be possible, but just to be safe: + if len(doneTodos) == 0 { + self.Log.Error("no done entries in rebase-merge/done file") + return "" + } + lastTodo := doneTodos[len(doneTodos)-1] + if lastTodo.Command == todo.Break || lastTodo.Command == todo.Exec || lastTodo.Command == todo.Reword { + return "" + } + + // In certain cases, git reschedules commands that failed. One example is if + // a patch would overwrite an untracked file (another one is an "exec" that + // failed, but we don't care about that here because we dealt with exec + // already above). To detect this, compare the last command of the "done" + // file against the first command of "git-rebase-todo"; if they are the + // same, the command was rescheduled. + if len(doneTodos) > 0 && len(todos) > 0 && doneTodos[len(doneTodos)-1] == todos[0] { + // Command was rescheduled, no need to display it + return "" + } + + // Older versions of git have a bug whereby, if a command is rescheduled, + // the last successful command is appended to the "done" file again. To + // detect this, we need to compare the second-to-last done entry against the + // first todo entry, and also compare the last done entry against the + // last-but-two done entry; this latter check is needed for the following + // case: + // pick A + // exec make test + // pick B + // exec make test + // If pick B fails with conflicts, then the "done" file contains + // pick A + // exec make test + // pick B + // and git-rebase-todo contains + // exec make test + // Without the last condition we would erroneously treat this as the exec + // command being rescheduled, so we wouldn't display our fake entry for + // "pick B". + if len(doneTodos) >= 3 && len(todos) > 0 && doneTodos[len(doneTodos)-2] == todos[0] && + doneTodos[len(doneTodos)-1] == doneTodos[len(doneTodos)-3] { + // Command was rescheduled, no need to display it + return "" + } + + if lastTodo.Command == todo.Edit { + if amendFileExists { + // Special case for "edit": if the "amend" file exists, the "edit" + // command was successful, otherwise it wasn't + return "" + } + } + + // I don't think this is ever possible, but again, just to be safe: + if lastTodo.Commit == "" { + self.Log.Error("last command in rebase-merge/done file doesn't have a commit") + return "" + } + + // Any other todo that has a commit associated with it must have failed with + // a conflict, otherwise we wouldn't have stopped the rebase: + return lastTodo.Commit +} + // assuming the file starts like this: // From e93d4193e6dd45ca9cf3a5a273d7ba6cd8b8fb20 Mon Sep 17 00:00:00 2001 // From: Lazygit Tester diff --git a/pkg/commands/git_commands/commit_loader_test.go b/pkg/commands/git_commands/commit_loader_test.go index a1a2c7e41..5c0102964 100644 --- a/pkg/commands/git_commands/commit_loader_test.go +++ b/pkg/commands/git_commands/commit_loader_test.go @@ -6,6 +6,7 @@ import ( "strings" "testing" + "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" @@ -319,3 +320,179 @@ func TestGetCommits(t *testing.T) { }) } } + +func TestCommitLoader_getConflictedCommitImpl(t *testing.T) { + scenarios := []struct { + testName string + todos []todo.Todo + doneTodos []todo.Todo + amendFileExists bool + expectedSha string + }{ + { + testName: "no done todos", + todos: []todo.Todo{}, + doneTodos: []todo.Todo{}, + amendFileExists: false, + expectedSha: "", + }, + { + testName: "common case (conflict)", + todos: []todo.Todo{}, + doneTodos: []todo.Todo{ + { + Command: todo.Pick, + Commit: "deadbeef", + }, + { + Command: todo.Pick, + Commit: "fa1afe1", + }, + }, + amendFileExists: false, + expectedSha: "fa1afe1", + }, + { + testName: "last command was 'break'", + todos: []todo.Todo{}, + doneTodos: []todo.Todo{ + {Command: todo.Break}, + }, + amendFileExists: false, + expectedSha: "", + }, + { + testName: "last command was 'exec'", + todos: []todo.Todo{}, + doneTodos: []todo.Todo{ + { + Command: todo.Exec, + ExecCommand: "make test", + }, + }, + amendFileExists: false, + expectedSha: "", + }, + { + testName: "last command was 'reword'", + todos: []todo.Todo{}, + doneTodos: []todo.Todo{ + {Command: todo.Reword}, + }, + amendFileExists: false, + expectedSha: "", + }, + { + testName: "'pick' was rescheduled", + todos: []todo.Todo{ + { + Command: todo.Pick, + Commit: "fa1afe1", + }, + }, + doneTodos: []todo.Todo{ + { + Command: todo.Pick, + Commit: "fa1afe1", + }, + }, + amendFileExists: false, + expectedSha: "", + }, + { + testName: "'pick' was rescheduled, buggy git version", + todos: []todo.Todo{ + { + Command: todo.Pick, + Commit: "fa1afe1", + }, + }, + doneTodos: []todo.Todo{ + { + Command: todo.Pick, + Commit: "deadbeaf", + }, + { + Command: todo.Pick, + Commit: "fa1afe1", + }, + { + Command: todo.Pick, + Commit: "deadbeaf", + }, + }, + amendFileExists: false, + expectedSha: "", + }, + { + testName: "conflicting 'pick' after 'exec'", + todos: []todo.Todo{ + { + Command: todo.Exec, + ExecCommand: "make test", + }, + }, + doneTodos: []todo.Todo{ + { + Command: todo.Pick, + Commit: "deadbeaf", + }, + { + Command: todo.Exec, + ExecCommand: "make test", + }, + { + Command: todo.Pick, + Commit: "fa1afe1", + }, + }, + amendFileExists: false, + expectedSha: "fa1afe1", + }, + { + testName: "'edit' with amend file", + todos: []todo.Todo{}, + doneTodos: []todo.Todo{ + { + Command: todo.Edit, + Commit: "fa1afe1", + }, + }, + amendFileExists: true, + expectedSha: "", + }, + { + testName: "'edit' without amend file", + todos: []todo.Todo{}, + doneTodos: []todo.Todo{ + { + Command: todo.Edit, + Commit: "fa1afe1", + }, + }, + amendFileExists: false, + expectedSha: "fa1afe1", + }, + } + for _, scenario := range scenarios { + t.Run(scenario.testName, func(t *testing.T) { + common := utils.NewDummyCommon() + + builder := &CommitLoader{ + Common: common, + cmd: oscommands.NewDummyCmdObjBuilder(oscommands.NewFakeRunner(t)), + getRebaseMode: func() (enums.RebaseMode, error) { return enums.REBASE_MODE_INTERACTIVE, nil }, + dotGitDir: ".git", + readFile: func(filename string) ([]byte, error) { + return []byte(""), nil + }, + walkFiles: func(root string, fn filepath.WalkFunc) error { + return nil + }, + } + + sha := builder.getConflictedCommitImpl(scenario.todos, scenario.doneTodos, scenario.amendFileExists) + assert.Equal(t, scenario.expectedSha, sha) + }) + } +} diff --git a/pkg/commands/models/commit.go b/pkg/commands/models/commit.go index e0ff875a0..73ed523d2 100644 --- a/pkg/commands/models/commit.go +++ b/pkg/commands/models/commit.go @@ -26,6 +26,8 @@ const ( // Conveniently for us, the todo package starts the enum at 1, and given // that it doesn't have a "none" value, we're setting ours to 0 ActionNone todo.TodoCommand = 0 + // "Comment" is the last one of the todo package's enum entries + ActionConflict = todo.Comment + 1 ) // Commit : A git commit diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 46c3ad6b5..0ba80c768 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -385,6 +385,10 @@ func (self *LocalCommitsController) interactiveRebase(action todo.TodoCommand) e // commit meaning you are trying to edit the todo file rather than actually // begin a rebase. It then updates the todo file with that action func (self *LocalCommitsController) handleMidRebaseCommand(action todo.TodoCommand, commit *models.Commit) (bool, error) { + if commit.Action == models.ActionConflict { + return true, self.c.ErrorMsg(self.c.Tr.ChangingThisActionIsNotAllowed) + } + if !commit.IsTODO() { if self.c.Git().Status.WorkingTreeState() != enums.REBASE_MODE_NONE { // If we are in a rebase, the only action that is allowed for @@ -434,7 +438,7 @@ func (self *LocalCommitsController) moveDown(commit *models.Commit) error { } if commit.IsTODO() { - if !commits[index+1].IsTODO() { + if !commits[index+1].IsTODO() || commits[index+1].Action == models.ActionConflict { return nil } diff --git a/pkg/gui/presentation/commits.go b/pkg/gui/presentation/commits.go index a2bba30ed..4da03f02f 100644 --- a/pkg/gui/presentation/commits.go +++ b/pkg/gui/presentation/commits.go @@ -17,6 +17,7 @@ import ( "github.com/jesseduffield/lazygit/pkg/theme" "github.com/jesseduffield/lazygit/pkg/utils" "github.com/kyokomi/emoji/v2" + "github.com/samber/lo" "github.com/sasha-s/go-deadlock" ) @@ -103,7 +104,11 @@ func GetCommitListDisplayStrings( for i, commit := range filteredCommits { unfilteredIdx := i + startIdx bisectStatus = getBisectStatus(unfilteredIdx, commit.Sha, bisectInfo, bisectBounds) - isYouAreHereCommit := showYouAreHereLabel && unfilteredIdx == rebaseOffset + isYouAreHereCommit := false + if showYouAreHereLabel && (commit.Action == models.ActionConflict || unfilteredIdx == rebaseOffset) { + isYouAreHereCommit = true + showYouAreHereLabel = false + } lines = append(lines, displayCommit( common, commit, @@ -272,7 +277,7 @@ func displayCommit( actionString := "" if commit.Action != models.ActionNone { - todoString := commit.Action.String() + todoString := lo.Ternary(commit.Action == models.ActionConflict, "conflict", commit.Action.String()) actionString = actionColorMap(commit.Action).Sprint(todoString) + " " } @@ -295,7 +300,8 @@ func displayCommit( } if isYouAreHereCommit { - youAreHere := style.FgYellow.Sprintf("<-- %s ---", common.Tr.YouAreHere) + color := lo.Ternary(commit.Action == models.ActionConflict, style.FgRed, style.FgYellow) + youAreHere := color.Sprintf("<-- %s ---", common.Tr.YouAreHere) name = fmt.Sprintf("%s %s", youAreHere, name) } @@ -391,6 +397,8 @@ func actionColorMap(action todo.TodoCommand) style.TextStyle { return style.FgGreen case todo.Fixup: return style.FgMagenta + case models.ActionConflict: + return style.FgRed default: return style.FgYellow } diff --git a/pkg/integration/tests/branch/rebase_and_drop.go b/pkg/integration/tests/branch/rebase_and_drop.go index 9c95e40e1..4c6712f23 100644 --- a/pkg/integration/tests/branch/rebase_and_drop.go +++ b/pkg/integration/tests/branch/rebase_and_drop.go @@ -53,7 +53,8 @@ var RebaseAndDrop = NewIntegrationTest(NewIntegrationTestArgs{ TopLines( MatchesRegexp(`pick.*to keep`).IsSelected(), MatchesRegexp(`pick.*to remove`), - MatchesRegexp("YOU ARE HERE.*second-change-branch unrelated change"), + MatchesRegexp(`conflict.*YOU ARE HERE.*first change`), + MatchesRegexp("second-change-branch unrelated change"), MatchesRegexp("second change"), MatchesRegexp("original"), ). @@ -62,7 +63,8 @@ var RebaseAndDrop = NewIntegrationTest(NewIntegrationTestArgs{ TopLines( MatchesRegexp(`pick.*to keep`), MatchesRegexp(`drop.*to remove`).IsSelected(), - MatchesRegexp("YOU ARE HERE.*second-change-branch unrelated change"), + MatchesRegexp(`conflict.*YOU ARE HERE.*first change`), + MatchesRegexp("second-change-branch unrelated change"), MatchesRegexp("second change"), MatchesRegexp("original"), ) diff --git a/pkg/integration/tests/interactive_rebase/amend_commit_with_conflict.go b/pkg/integration/tests/interactive_rebase/amend_commit_with_conflict.go index 0483487f3..2226b5196 100644 --- a/pkg/integration/tests/interactive_rebase/amend_commit_with_conflict.go +++ b/pkg/integration/tests/interactive_rebase/amend_commit_with_conflict.go @@ -35,8 +35,8 @@ var AmendCommitWithConflict = NewIntegrationTest(NewIntegrationTestArgs{ }). Lines( Contains("pick").Contains("three"), - // Would be nice to see "fixup! two" here, because that's what git is trying to apply right now - Contains("<-- YOU ARE HERE --- two"), + Contains("conflict").Contains("<-- YOU ARE HERE --- fixup! two"), + Contains("two"), Contains("one"), ) @@ -66,8 +66,8 @@ var AmendCommitWithConflict = NewIntegrationTest(NewIntegrationTestArgs{ t.Views().Commits(). Lines( - // Would be nice to see "three" here, because that's what git is trying to apply right now - Contains("<-- YOU ARE HERE --- two"), + Contains("<-- YOU ARE HERE --- three"), + Contains("two"), Contains("one"), ) }, diff --git a/pkg/integration/tests/interactive_rebase/edit_the_confl_commit.go b/pkg/integration/tests/interactive_rebase/edit_the_confl_commit.go new file mode 100644 index 000000000..96ec81c74 --- /dev/null +++ b/pkg/integration/tests/interactive_rebase/edit_the_confl_commit.go @@ -0,0 +1,47 @@ +package interactive_rebase + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var EditTheConflCommit = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Swap two commits, causing a conflict; then try to interact with the 'confl' commit, which results in an error.", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.CreateFileAndAdd("myfile", "one") + shell.Commit("commit one") + shell.UpdateFileAndAdd("myfile", "two") + shell.Commit("commit two") + shell.UpdateFileAndAdd("myfile", "three") + shell.Commit("commit three") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("commit three").IsSelected(), + Contains("commit two"), + Contains("commit one"), + ). + Press(keys.Commits.MoveDownCommit). + Tap(func() { + t.Common().AcknowledgeConflicts() + }). + Focus(). + Lines( + Contains("pick").Contains("commit two"), + Contains("conflict").Contains("<-- YOU ARE HERE --- commit three"), + Contains("commit one"), + ). + NavigateToLine(Contains("<-- YOU ARE HERE --- commit three")). + Press(keys.Commits.RenameCommit) + + t.ExpectPopup().Alert(). + Title(Equals("Error")). + Content(Contains("Changing this kind of rebase todo entry is not allowed")). + Confirm() + }, +}) diff --git a/pkg/integration/tests/interactive_rebase/shared.go b/pkg/integration/tests/interactive_rebase/shared.go index 2d3746af4..2e42c7f76 100644 --- a/pkg/integration/tests/interactive_rebase/shared.go +++ b/pkg/integration/tests/interactive_rebase/shared.go @@ -10,8 +10,8 @@ func handleConflictsFromSwap(t *TestDriver) { t.Views().Commits(). Lines( Contains("pick").Contains("commit two"), - // Would be nice to see "three" here, it's the one that conflicted - Contains("<-- YOU ARE HERE --- commit one"), + Contains("conflict").Contains("<-- YOU ARE HERE --- commit three"), + Contains("commit one"), ) t.Views().Files(). diff --git a/pkg/integration/tests/sync/pull_rebase_interactive_conflict.go b/pkg/integration/tests/sync/pull_rebase_interactive_conflict.go index 896c5cc45..9e66f3bcd 100644 --- a/pkg/integration/tests/sync/pull_rebase_interactive_conflict.go +++ b/pkg/integration/tests/sync/pull_rebase_interactive_conflict.go @@ -47,7 +47,8 @@ var PullRebaseInteractiveConflict = NewIntegrationTest(NewIntegrationTestArgs{ t.Views().Commits(). Lines( Contains("pick").Contains("five"), - Contains("YOU ARE HERE").Contains("three"), + Contains("conflict").Contains("YOU ARE HERE").Contains("four"), + Contains("three"), Contains("two"), Contains("one"), ) diff --git a/pkg/integration/tests/sync/pull_rebase_interactive_conflict_drop.go b/pkg/integration/tests/sync/pull_rebase_interactive_conflict_drop.go index 1950571b3..9ca3a0ebe 100644 --- a/pkg/integration/tests/sync/pull_rebase_interactive_conflict_drop.go +++ b/pkg/integration/tests/sync/pull_rebase_interactive_conflict_drop.go @@ -48,14 +48,16 @@ var PullRebaseInteractiveConflictDrop = NewIntegrationTest(NewIntegrationTestArg Focus(). Lines( Contains("pick").Contains("five").IsSelected(), - Contains("YOU ARE HERE").Contains("three"), + Contains("conflict").Contains("YOU ARE HERE").Contains("four"), + Contains("three"), Contains("two"), Contains("one"), ). Press(keys.Universal.Remove). Lines( Contains("drop").Contains("five").IsSelected(), - Contains("YOU ARE HERE").Contains("three"), + Contains("conflict").Contains("YOU ARE HERE").Contains("four"), + Contains("three"), Contains("two"), Contains("one"), ) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index aa303a44c..02d4f8432 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -105,6 +105,7 @@ var tests = []*components.IntegrationTest{ interactive_rebase.DropTodoCommitWithUpdateRefShowBranchHeads, interactive_rebase.EditFirstCommit, interactive_rebase.EditNonTodoCommitDuringRebase, + interactive_rebase.EditTheConflCommit, interactive_rebase.FixupFirstCommit, interactive_rebase.FixupSecondCommit, interactive_rebase.Move,