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

@@ -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 <test@example.com>

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)
+		})
+	}
+}

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

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
 		}
 

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
 	}

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"),
 			)

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("conflict").Contains("<-- YOU ARE HERE --- fixup! two"),
-				Contains("<-- YOU ARE HERE --- 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 --- three"),
-				Contains("<-- YOU ARE HERE --- two"),
+				Contains("two"),
 				Contains("one"),
 			)
 	},

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()
+	},
+})

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("conflict").Contains("<-- YOU ARE HERE --- commit three"),
-			Contains("<-- YOU ARE HERE --- commit one"),
+			Contains("commit one"),
 		)
 
 	t.Views().Files().

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"),
 			)

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"),
 			)

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,