Skip to content

Commit ff465e2

Browse files
stefanhallerstefanhaller
committed
Show original todo action instead of "conflict", and show <-- CONFLICT instead
It is useful to see if the conflicted commit was a "pick" or an "edit". What's more, we're about to add support for showing cherry-picks and reverts, and seeing that a conflicted commit was a revert is important because its diff is backwards compared to the diff of the conflicting files in the Files panel.
1 parent 9c8f987 commit ff465e2

19 files changed

+81
-63
lines changed

‎pkg/commands/git_commands/commit_loader.go

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -322,13 +322,8 @@ func (self *CommitLoader) getRebasingCommits() []*models.Commit {
322322

323323
// See if the current commit couldn't be applied because it conflicted; if
324324
// so, add a fake entry for it
325-
if conflictedCommitHash := self.getConflictedCommit(todos); conflictedCommitHash != "" {
326-
commits = append(commits, &models.Commit{
327-
Hash: conflictedCommitHash,
328-
Name: "",
329-
Status: models.StatusRebasing,
330-
Action: models.ActionConflict,
331-
})
325+
if conflictedCommit := self.getConflictedCommit(todos); conflictedCommit != nil {
326+
commits = append(commits, conflictedCommit)
332327
}
333328

334329
for _, t := range todos {
@@ -351,17 +346,17 @@ func (self *CommitLoader) getRebasingCommits() []*models.Commit {
351346
return commits
352347
}
353348

354-
func (self *CommitLoader) getConflictedCommit(todos []todo.Todo) string {
349+
func (self *CommitLoader) getConflictedCommit(todos []todo.Todo) *models.Commit {
355350
bytesContent, err := self.readFile(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "rebase-merge/done"))
356351
if err != nil {
357352
self.Log.Error(fmt.Sprintf("error occurred reading rebase-merge/done: %s", err.Error()))
358-
return ""
353+
return nil
359354
}
360355

361356
doneTodos, err := todo.Parse(bytes.NewBuffer(bytesContent), self.config.GetCoreCommentChar())
362357
if err != nil {
363358
self.Log.Error(fmt.Sprintf("error occurred while parsing rebase-merge/done file: %s", err.Error()))
364-
return ""
359+
return nil
365360
}
366361

367362
amendFileExists := false
@@ -372,15 +367,15 @@ func (self *CommitLoader) getConflictedCommit(todos []todo.Todo) string {
372367
return self.getConflictedCommitImpl(todos, doneTodos, amendFileExists)
373368
}
374369

375-
func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos []todo.Todo, amendFileExists bool) string {
370+
func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos []todo.Todo, amendFileExists bool) *models.Commit {
376371
// Should never be possible, but just to be safe:
377372
if len(doneTodos) == 0 {
378373
self.Log.Error("no done entries in rebase-merge/done file")
379-
return ""
374+
return nil
380375
}
381376
lastTodo := doneTodos[len(doneTodos)-1]
382377
if lastTodo.Command == todo.Break || lastTodo.Command == todo.Exec || lastTodo.Command == todo.Reword {
383-
return ""
378+
return nil
384379
}
385380

386381
// In certain cases, git reschedules commands that failed. One example is if
@@ -391,7 +386,7 @@ func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos [
391386
// same, the command was rescheduled.
392387
if len(doneTodos) > 0 && len(todos) > 0 && doneTodos[len(doneTodos)-1] == todos[0] {
393388
// Command was rescheduled, no need to display it
394-
return ""
389+
return nil
395390
}
396391

397392
// Older versions of git have a bug whereby, if a command is rescheduled,
@@ -416,26 +411,30 @@ func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos [
416411
if len(doneTodos) >= 3 && len(todos) > 0 && doneTodos[len(doneTodos)-2] == todos[0] &&
417412
doneTodos[len(doneTodos)-1] == doneTodos[len(doneTodos)-3] {
418413
// Command was rescheduled, no need to display it
419-
return ""
414+
return nil
420415
}
421416

422417
if lastTodo.Command == todo.Edit {
423418
if amendFileExists {
424419
// Special case for "edit": if the "amend" file exists, the "edit"
425420
// command was successful, otherwise it wasn't
426-
return ""
421+
return nil
427422
}
428423
}
429424

430425
// I don't think this is ever possible, but again, just to be safe:
431426
if lastTodo.Commit == "" {
432427
self.Log.Error("last command in rebase-merge/done file doesn't have a commit")
433-
return ""
428+
return nil
434429
}
435430

436431
// Any other todo that has a commit associated with it must have failed with
437432
// a conflict, otherwise we wouldn't have stopped the rebase:
438-
return lastTodo.Commit
433+
return &models.Commit{
434+
Hash: lastTodo.Commit,
435+
Action: lastTodo.Command,
436+
Status: models.StatusConflicted,
437+
}
439438
}
440439

441440
func setCommitMergedStatuses(ancestor string, commits []*models.Commit) {

‎pkg/commands/git_commands/commit_loader_test.go

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -332,14 +332,14 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
332332
todos []todo.Todo
333333
doneTodos []todo.Todo
334334
amendFileExists bool
335-
expectedHash string
335+
expectedResult *models.Commit
336336
}{
337337
{
338338
testName: "no done todos",
339339
todos: []todo.Todo{},
340340
doneTodos: []todo.Todo{},
341341
amendFileExists: false,
342-
expectedHash: "",
342+
expectedResult: nil,
343343
},
344344
{
345345
testName: "common case (conflict)",
@@ -355,7 +355,11 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
355355
},
356356
},
357357
amendFileExists: false,
358-
expectedHash: "fa1afe1",
358+
expectedResult: &models.Commit{
359+
Hash: "fa1afe1",
360+
Action: todo.Pick,
361+
Status: models.StatusConflicted,
362+
},
359363
},
360364
{
361365
testName: "last command was 'break'",
@@ -364,7 +368,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
364368
{Command: todo.Break},
365369
},
366370
amendFileExists: false,
367-
expectedHash: "",
371+
expectedResult: nil,
368372
},
369373
{
370374
testName: "last command was 'exec'",
@@ -376,7 +380,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
376380
},
377381
},
378382
amendFileExists: false,
379-
expectedHash: "",
383+
expectedResult: nil,
380384
},
381385
{
382386
testName: "last command was 'reword'",
@@ -385,7 +389,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
385389
{Command: todo.Reword},
386390
},
387391
amendFileExists: false,
388-
expectedHash: "",
392+
expectedResult: nil,
389393
},
390394
{
391395
testName: "'pick' was rescheduled",
@@ -402,7 +406,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
402406
},
403407
},
404408
amendFileExists: false,
405-
expectedHash: "",
409+
expectedResult: nil,
406410
},
407411
{
408412
testName: "'pick' was rescheduled, buggy git version",
@@ -427,7 +431,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
427431
},
428432
},
429433
amendFileExists: false,
430-
expectedHash: "",
434+
expectedResult: nil,
431435
},
432436
{
433437
testName: "conflicting 'pick' after 'exec'",
@@ -452,7 +456,11 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
452456
},
453457
},
454458
amendFileExists: false,
455-
expectedHash: "fa1afe1",
459+
expectedResult: &models.Commit{
460+
Hash: "fa1afe1",
461+
Action: todo.Pick,
462+
Status: models.StatusConflicted,
463+
},
456464
},
457465
{
458466
testName: "'edit' with amend file",
@@ -464,7 +472,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
464472
},
465473
},
466474
amendFileExists: true,
467-
expectedHash: "",
475+
expectedResult: nil,
468476
},
469477
{
470478
testName: "'edit' without amend file",
@@ -476,7 +484,11 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
476484
},
477485
},
478486
amendFileExists: false,
479-
expectedHash: "fa1afe1",
487+
expectedResult: &models.Commit{
488+
Hash: "fa1afe1",
489+
Action: todo.Edit,
490+
Status: models.StatusConflicted,
491+
},
480492
},
481493
}
482494
for _, scenario := range scenarios {
@@ -497,7 +509,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
497509
}
498510

499511
hash := builder.getConflictedCommitImpl(scenario.todos, scenario.doneTodos, scenario.amendFileExists)
500-
assert.Equal(t, scenario.expectedHash, hash)
512+
assert.Equal(t, scenario.expectedResult, hash)
501513
})
502514
}
503515
}

‎pkg/commands/models/commit.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,14 @@ const (
1818
StatusPushed
1919
StatusMerged
2020
StatusRebasing
21+
StatusConflicted
2122
StatusReflog
2223
)
2324

2425
const (
2526
// Conveniently for us, the todo package starts the enum at 1, and given
2627
// that it doesn't have a "none" value, we're setting ours to 0
2728
ActionNone todo.TodoCommand = 0
28-
// "Comment" is the last one of the todo package's enum entries
29-
ActionConflict = todo.Comment + 1
3029
)
3130

3231
type Divergence int

‎pkg/gui/controllers/files_controller.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -829,12 +829,12 @@ func (self *FilesController) handleAmendCommitPress() error {
829829
func (self *FilesController) isResolvingConflicts() bool {
830830
commits := self.c.Model().Commits
831831
for _, c := range commits {
832+
if c.Status == models.StatusConflicted {
833+
return true
834+
}
832835
if !c.IsTODO() {
833836
break
834837
}
835-
if c.Action == models.ActionConflict {
836-
return true
837-
}
838838
}
839839
return false
840840
}

‎pkg/gui/controllers/local_commits_controller.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1389,7 +1389,7 @@ func (self *LocalCommitsController) canMoveDown(selectedCommits []*models.Commit
13891389
if self.isRebasing() {
13901390
commits := self.c.Model().Commits
13911391

1392-
if !commits[endIdx+1].IsTODO() || commits[endIdx+1].Action == models.ActionConflict {
1392+
if !commits[endIdx+1].IsTODO() || commits[endIdx+1].Status == models.StatusConflicted {
13931393
return &types.DisabledReason{Text: self.c.Tr.CannotMoveAnyFurther}
13941394
}
13951395
}
@@ -1405,7 +1405,7 @@ func (self *LocalCommitsController) canMoveUp(selectedCommits []*models.Commit,
14051405
if self.isRebasing() {
14061406
commits := self.c.Model().Commits
14071407

1408-
if !commits[startIdx-1].IsTODO() || commits[startIdx-1].Action == models.ActionConflict {
1408+
if !commits[startIdx-1].IsTODO() || commits[startIdx-1].Status == models.StatusConflicted {
14091409
return &types.DisabledReason{Text: self.c.Tr.CannotMoveAnyFurther}
14101410
}
14111411
}

‎pkg/gui/presentation/commits.go

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ func GetCommitListDisplayStrings(
186186
unfilteredIdx := i + startIdx
187187
bisectStatus = getBisectStatus(unfilteredIdx, commit.Hash, bisectInfo, bisectBounds)
188188
isYouAreHereCommit := false
189-
if showYouAreHereLabel && (commit.Action == models.ActionConflict || unfilteredIdx == rebaseOffset) {
189+
if showYouAreHereLabel && (commit.Status == models.StatusConflicted || unfilteredIdx == rebaseOffset) {
190190
isYouAreHereCommit = true
191191
showYouAreHereLabel = false
192192
}
@@ -395,8 +395,7 @@ func displayCommit(
395395

396396
actionString := ""
397397
if commit.Action != models.ActionNone {
398-
todoString := lo.Ternary(commit.Action == models.ActionConflict, "conflict", commit.Action.String())
399-
actionString = actionColorMap(commit.Action).Sprint(todoString) + " "
398+
actionString = actionColorMap(commit.Action, commit.Status).Sprint(commit.Action.String()) + " "
400399
}
401400

402401
tagString := ""
@@ -429,8 +428,13 @@ func displayCommit(
429428

430429
mark := ""
431430
if isYouAreHereCommit {
432-
color := lo.Ternary(commit.Action == models.ActionConflict, style.FgRed, style.FgYellow)
433-
youAreHere := color.Sprintf("<-- %s ---", common.Tr.YouAreHere)
431+
color := style.FgYellow
432+
text := common.Tr.YouAreHere
433+
if commit.Status == models.StatusConflicted {
434+
color = style.FgRed
435+
text = common.Tr.ConflictLabel
436+
}
437+
youAreHere := color.Sprintf("<-- %s ---", text)
434438
mark = fmt.Sprintf("%s ", youAreHere)
435439
} else if isMarkedBaseCommit {
436440
rebaseFromHere := style.FgYellow.Sprint(common.Tr.MarkedCommitMarker)
@@ -501,7 +505,7 @@ func getHashColor(
501505
hashColor = style.FgYellow
502506
case models.StatusMerged:
503507
hashColor = style.FgGreen
504-
case models.StatusRebasing:
508+
case models.StatusRebasing, models.StatusConflicted:
505509
hashColor = style.FgBlue
506510
case models.StatusReflog:
507511
hashColor = style.FgBlue
@@ -519,7 +523,11 @@ func getHashColor(
519523
return hashColor
520524
}
521525

522-
func actionColorMap(action todo.TodoCommand) style.TextStyle {
526+
func actionColorMap(action todo.TodoCommand, status models.CommitStatus) style.TextStyle {
527+
if status == models.StatusConflicted {
528+
return style.FgRed
529+
}
530+
523531
switch action {
524532
case todo.Pick:
525533
return style.FgCyan
@@ -529,8 +537,6 @@ func actionColorMap(action todo.TodoCommand) style.TextStyle {
529537
return style.FgGreen
530538
case todo.Fixup:
531539
return style.FgMagenta
532-
case models.ActionConflict:
533-
return style.FgRed
534540
default:
535541
return style.FgYellow
536542
}

‎pkg/i18n/english.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,7 @@ type TranslationSet struct {
349349
ErrorOccurred string
350350
NoRoom string
351351
YouAreHere string
352+
ConflictLabel string
352353
YouDied string
353354
RewordNotSupported string
354355
ChangingThisActionIsNotAllowed string
@@ -1416,6 +1417,7 @@ func EnglishTranslationSet() *TranslationSet {
14161417
ErrorOccurred: "An error occurred! Please create an issue at",
14171418
NoRoom: "Not enough room",
14181419
YouAreHere: "YOU ARE HERE",
1420+
ConflictLabel: "CONFLICT",
14191421
YouDied: "YOU DIED!",
14201422
RewordNotSupported: "Rewording commits while interactively rebasing is not currently supported",
14211423
ChangingThisActionIsNotAllowed: "Changing this kind of rebase todo entry is not allowed",

‎pkg/integration/tests/branch/rebase_and_drop.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ var RebaseAndDrop = NewIntegrationTest(NewIntegrationTestArgs{
5353
TopLines(
5454
MatchesRegexp(`pick.*to keep`).IsSelected(),
5555
MatchesRegexp(`pick.*to remove`),
56-
MatchesRegexp(`conflict.*YOU ARE HERE.*first change`),
56+
MatchesRegexp(`pick.*CONFLICT.*first change`),
5757
MatchesRegexp("second-change-branch unrelated change"),
5858
MatchesRegexp("second change"),
5959
MatchesRegexp("original"),
@@ -63,7 +63,7 @@ var RebaseAndDrop = NewIntegrationTest(NewIntegrationTestArgs{
6363
TopLines(
6464
MatchesRegexp(`pick.*to keep`),
6565
MatchesRegexp(`drop.*to remove`).IsSelected(),
66-
MatchesRegexp(`conflict.*YOU ARE HERE.*first change`),
66+
MatchesRegexp(`pick.*CONFLICT.*first change`),
6767
MatchesRegexp("second-change-branch unrelated change"),
6868
MatchesRegexp("second change"),
6969
MatchesRegexp("original"),

‎pkg/integration/tests/commit/amend_when_there_are_conflicts_and_amend.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ var AmendWhenThereAreConflictsAndAmend = NewIntegrationTest(NewIntegrationTestAr
3030
Focus().
3131
Lines(
3232
Contains("pick").Contains("commit three"),
33-
Contains("conflict").Contains("<-- YOU ARE HERE --- file1 changed in branch"),
33+
Contains("pick").Contains("<-- CONFLICT --- file1 changed in branch"),
3434
Contains("commit two"),
3535
Contains("file1 changed in master"),
3636
Contains("base commit"),

‎pkg/integration/tests/commit/amend_when_there_are_conflicts_and_cancel.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ var AmendWhenThereAreConflictsAndCancel = NewIntegrationTest(NewIntegrationTestA
3434
Focus().
3535
Lines(
3636
Contains("pick").Contains("commit three"),
37-
Contains("conflict").Contains("<-- YOU ARE HERE --- file1 changed in branch"),
37+
Contains("pick").Contains("<-- CONFLICT --- file1 changed in branch"),
3838
Contains("commit two"),
3939
Contains("file1 changed in master"),
4040
Contains("base commit"),

0 commit comments

Comments
 (0)