Fix the bug described in the previous commit

What happens here is that when stopping on an "edit" todo entry, we rely on the
assumption that if the .git/rebase-merge/amend file exists, the command was
successful, and if it doesn't, there was a conflict. The problem is that when
you stop on an edit command, and then run a multi-commit cherry-pick or rebase,
this will delete the amend file. You may or may not consider this a bug in git;
to work around it, we also check the existence of the rebase-merge/message file,
which will be deleted as well by the cherry-pick or revert.
This commit is contained in:
Stefan Haller 2024-06-14 14:36:31 +02:00
parent 9b88052a4e
commit 5ba8d42c80
4 changed files with 35 additions and 35 deletions

View file

@ -388,15 +388,13 @@ func (self *CommitLoader) getConflictedCommit(todos []todo.Todo) *models.Commit
return nil return nil
} }
amendFileExists := false amendFileExists, _ := self.os.FileExists(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "rebase-merge/amend"))
if _, err := os.Stat(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "rebase-merge/amend")); err == nil { messageFileExists, _ := self.os.FileExists(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "rebase-merge/message"))
amendFileExists = true
}
return self.getConflictedCommitImpl(todos, doneTodos, amendFileExists) return self.getConflictedCommitImpl(todos, doneTodos, amendFileExists, messageFileExists)
} }
func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos []todo.Todo, amendFileExists bool) *models.Commit { func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos []todo.Todo, amendFileExists bool, messageFileExists bool) *models.Commit {
// Should never be possible, but just to be safe: // Should never be possible, but just to be safe:
if len(doneTodos) == 0 { if len(doneTodos) == 0 {
self.Log.Error("no done entries in rebase-merge/done file") self.Log.Error("no done entries in rebase-merge/done file")
@ -449,6 +447,14 @@ func (self *CommitLoader) getConflictedCommitImpl(todos []todo.Todo, doneTodos [
// command was successful, otherwise it wasn't // command was successful, otherwise it wasn't
return nil return nil
} }
if !messageFileExists {
// As an additional check, see if the "message" file exists; if it
// doesn't, it must be because a multi-commit cherry-pick or revert
// was performed in the meantime, which deleted both the amend file
// and the message file.
return nil
}
} }
// I don't think this is ever possible, but again, just to be safe: // I don't think this is ever possible, but again, just to be safe:

View file

@ -332,6 +332,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
todos []todo.Todo todos []todo.Todo
doneTodos []todo.Todo doneTodos []todo.Todo
amendFileExists bool amendFileExists bool
messageFileExists bool
expectedResult *models.Commit expectedResult *models.Commit
}{ }{
{ {
@ -475,7 +476,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
expectedResult: nil, expectedResult: nil,
}, },
{ {
testName: "'edit' without amend file", testName: "'edit' without amend file but message file",
todos: []todo.Todo{}, todos: []todo.Todo{},
doneTodos: []todo.Todo{ doneTodos: []todo.Todo{
{ {
@ -484,12 +485,26 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
}, },
}, },
amendFileExists: false, amendFileExists: false,
messageFileExists: true,
expectedResult: &models.Commit{ expectedResult: &models.Commit{
Hash: "fa1afe1", Hash: "fa1afe1",
Action: todo.Edit, Action: todo.Edit,
Status: models.StatusConflicted, Status: models.StatusConflicted,
}, },
}, },
{
testName: "'edit' without amend and without message file",
todos: []todo.Todo{},
doneTodos: []todo.Todo{
{
Command: todo.Edit,
Commit: "fa1afe1",
},
},
amendFileExists: false,
messageFileExists: false,
expectedResult: nil,
},
} }
for _, scenario := range scenarios { for _, scenario := range scenarios {
t.Run(scenario.testName, func(t *testing.T) { t.Run(scenario.testName, func(t *testing.T) {
@ -508,7 +523,7 @@ func TestCommitLoader_getConflictedCommitImpl(t *testing.T) {
}, },
} }
hash := builder.getConflictedCommitImpl(scenario.todos, scenario.doneTodos, scenario.amendFileExists) hash := builder.getConflictedCommitImpl(scenario.todos, scenario.doneTodos, scenario.amendFileExists, scenario.messageFileExists)
assert.Equal(t, scenario.expectedResult, hash) assert.Equal(t, scenario.expectedResult, hash)
}) })
} }

View file

@ -45,7 +45,6 @@ var RevertDuringRebaseWhenStoppedOnEdit = NewIntegrationTest(NewIntegrationTestA
). ).
Press("X"). Press("X").
Lines( Lines(
/* EXPECTED:
Contains("pick").Contains("commit 04"), Contains("pick").Contains("commit 04"),
Contains(`<-- YOU ARE HERE --- Revert "commit 01"`).IsSelected(), Contains(`<-- YOU ARE HERE --- Revert "commit 01"`).IsSelected(),
Contains(`Revert "commit 02"`), Contains(`Revert "commit 02"`),
@ -53,15 +52,6 @@ var RevertDuringRebaseWhenStoppedOnEdit = NewIntegrationTest(NewIntegrationTestA
Contains("commit 02"), Contains("commit 02"),
Contains("commit 01"), Contains("commit 01"),
Contains("master commit"), Contains("master commit"),
ACTUAL: */
Contains("pick").Contains("commit 04"),
Contains("edit").Contains("<-- CONFLICT --- commit 03").IsSelected(),
Contains(`Revert "commit 01"`),
Contains(`Revert "commit 02"`),
Contains("commit 03"),
Contains("commit 02"),
Contains("commit 01"),
Contains("master commit"),
) )
}, },
}) })

View file

@ -84,7 +84,6 @@ var RevertMultipleCommitsInInteractiveRebase = NewIntegrationTest(NewIntegration
t.Views().Commits(). t.Views().Commits().
Lines( Lines(
/* EXPECTED:
Contains("pick").Contains("CI unrelated change 3"), Contains("pick").Contains("CI unrelated change 3"),
Contains("pick").Contains("CI unrelated change 2"), Contains("pick").Contains("CI unrelated change 2"),
Contains(`CI ◯ <-- YOU ARE HERE --- Revert "unrelated change 1"`), Contains(`CI ◯ <-- YOU ARE HERE --- Revert "unrelated change 1"`),
@ -93,16 +92,6 @@ var RevertMultipleCommitsInInteractiveRebase = NewIntegrationTest(NewIntegration
Contains("CI ◯ add first line"), Contains("CI ◯ add first line"),
Contains("CI ◯ unrelated change 1"), Contains("CI ◯ unrelated change 1"),
Contains("CI ◯ add empty file"), Contains("CI ◯ add empty file"),
ACTUAL: */
Contains("pick").Contains("CI unrelated change 3"),
Contains("pick").Contains("CI unrelated change 2"),
Contains("edit CI <-- CONFLICT --- add second line"),
Contains(`CI ◯ Revert "unrelated change 1"`),
Contains(`CI ◯ Revert "add first line"`),
Contains("CI ◯ add second line"),
Contains("CI ◯ add first line"),
Contains("CI ◯ unrelated change 1"),
Contains("CI ◯ add empty file"),
) )
t.Views().Options().Content(Contains("View rebase options: m")) t.Views().Options().Content(Contains("View rebase options: m"))