diff --git a/pkg/commands/git_commands/rebase.go b/pkg/commands/git_commands/rebase.go index e166687a9..c5e289e49 100644 --- a/pkg/commands/git_commands/rebase.go +++ b/pkg/commands/git_commands/rebase.go @@ -501,6 +501,12 @@ func (self *RebaseCommands) GenericMergeOrRebaseAction(commandType string, comma } } + if commandType == "rebase" && command == "continue" { + if err := self.commitPickThatRebaseStoppedAt(); err != nil { + return err + } + } + cmdObj := skipEditor(self.GenericMergeOrRebaseActionCmdObj(commandType, command)) if command == "continue" || command == "skip" { switch commandType { @@ -535,6 +541,19 @@ func (self *RebaseCommands) GenericMergeOrRebaseAction(commandType string, comma return nil } +// commitPickThatRebaseStoppedAt commits the changes of a pick if the rebase +// stopped after it applied them, but before it committed them (see +// rebaseStoppedBeforeCommittingPick). "git rebase --continue" refuses to commit +// them in that case. "git cherry-pick --continue" commits them with the author +// and message of the pick, like the rebase would have. +func (self *RebaseCommands) commitPickThatRebaseStoppedAt() error { + if !rebaseStoppedBeforeCommittingPick(self.repoPaths.WorktreeGitDirPath(), self.config.GetCoreCommentChar()) { + return nil + } + + return skipEditor(self.GenericMergeOrRebaseActionCmdObj("cherry-pick", "continue")).Run() +} + // resumeAfterLockError makes cmdObj, a command that starts or continues a // rebase, resume the rebase if a lock error stops it partway through; see // resumeRebaseAfterLockError. diff --git a/pkg/commands/git_commands/status.go b/pkg/commands/git_commands/status.go index cdec190c4..f6b1fd460 100644 --- a/pkg/commands/git_commands/status.go +++ b/pkg/commands/git_commands/status.go @@ -59,35 +59,56 @@ func (self *StatusCommands) IsInCherryPick() (bool, error) { } func (self *StatusCommands) hasCherryPickHead() (bool, error) { - exists, err := self.os.FileExists(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "CHERRY_PICK_HEAD")) + gitDir := self.repoPaths.WorktreeGitDirPath() + exists, err := self.os.FileExists(filepath.Join(gitDir, "CHERRY_PICK_HEAD")) if err != nil || !exists { return exists, err } - // Sometimes, CHERRY_PICK_HEAD is present during rebases even if no - // cherry-pick is in progress. I suppose this is because rebase used to be - // implemented as a series of cherry-picks, so this could be remnants of - // code that is shared between cherry-pick and rebase, or something. The way - // to tell if this is the case is to check for the presence of the - // stopped-sha file, which records the sha of the last pick that was - // executed before the rebase stopped, and seeing if the sha in that file is - // the same as the one in CHERRY_PICK_HEAD. - cherryPickHead, err := os.ReadFile(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "CHERRY_PICK_HEAD")) + cherryPickHead, err := os.ReadFile(filepath.Join(gitDir, "CHERRY_PICK_HEAD")) if err != nil { return false, err } - stoppedSha, err := os.ReadFile(filepath.Join(self.repoPaths.WorktreeGitDirPath(), "rebase-merge", "stopped-sha")) + return !cherryPickHeadBelongsToRebase(gitDir, strings.TrimSpace(string(cherryPickHead)), self.config.GetCoreCommentChar()), nil +} + +// cherryPickHeadBelongsToRebase tells whether CHERRY_PICK_HEAD, whose value is +// cherryPickHead, belongs to a pick of the rebase that is in progress in +// gitDir, rather than to a cherry-pick. A rebase writes CHERRY_PICK_HEAD when it +// has applied the changes of a pick, and removes it when it has committed them. +// So it is still there if the rebase stopped because the pick conflicted; git +// records the pick in rebase-merge/stopped-sha then. It is also still there if +// git couldn't commit the changes, for example because it couldn't take the +// index lock. Git exits without writing stopped-sha in that case, but the pick +// is the last line of rebase-merge/done. +func cherryPickHeadBelongsToRebase(gitDir string, cherryPickHead string, commentChar byte) bool { + // The hashes in these files may be abbreviated + isPrefixOfCherryPickHead := func(hash string) bool { + return hash != "" && strings.HasPrefix(cherryPickHead, hash) + } + + stoppedSha, err := os.ReadFile(filepath.Join(gitDir, "rebase-merge", "stopped-sha")) + if err == nil && isPrefixOfCherryPickHead(strings.TrimSpace(string(stoppedSha))) { + return true + } + + doneTodos, err := utils.ReadRebaseTodoFile(filepath.Join(gitDir, "rebase-merge", "done"), commentChar) + return err == nil && len(doneTodos) > 0 && isPrefixOfCherryPickHead(doneTodos[len(doneTodos)-1].Commit) +} + +// rebaseStoppedBeforeCommittingPick tells whether the rebase that is in +// progress in gitDir has applied the changes of a pick, but exited before it +// could commit them (see cherryPickHeadBelongsToRebase). "git rebase +// --continue" refuses to commit the changes in that case, because git didn't +// write the message file that it writes when a pick conflicts. +func rebaseStoppedBeforeCommittingPick(gitDir string, commentChar byte) bool { + cherryPickHead, err := os.ReadFile(filepath.Join(gitDir, "CHERRY_PICK_HEAD")) if err != nil { - // If we get an error we assume the file doesn't exist - return true, nil + return false } - cherryPickHeadStr := strings.TrimSpace(string(cherryPickHead)) - stoppedShaStr := strings.TrimSpace(string(stoppedSha)) - // Need to use HasPrefix here because the cherry-pick HEAD is a full sha1, - // but stopped-sha is an abbreviated sha1 - if strings.HasPrefix(cherryPickHeadStr, stoppedShaStr) { - return false, nil + if _, err := os.Stat(filepath.Join(gitDir, "rebase-merge", "message")); err == nil { + return false } - return true, nil + return cherryPickHeadBelongsToRebase(gitDir, strings.TrimSpace(string(cherryPickHead)), commentChar) } func (self *StatusCommands) IsInRevert() (bool, error) { diff --git a/pkg/commands/git_commands/status_test.go b/pkg/commands/git_commands/status_test.go index 0ec71123a..45987f912 100644 --- a/pkg/commands/git_commands/status_test.go +++ b/pkg/commands/git_commands/status_test.go @@ -113,6 +113,31 @@ func TestStatusWorkingTreeStateOfCherryPickOrRevert(t *testing.T) { files: map[string]string{"REVERT_HEAD": "1234567\n"}, expectedReverting: true, }, + { + testName: "CHERRY_PICK_HEAD of a rebase that stopped because of conflicts", + files: map[string]string{ + "CHERRY_PICK_HEAD": "2345678abcdef\n", + "rebase-merge/stopped-sha": "2345678\n", + "rebase-merge/done": "pick 1234567abcdef one\npick 2345678abcdef two\n", + }, + expectedCherryPicking: false, + }, + { + testName: "CHERRY_PICK_HEAD of a rebase that couldn't commit a pick", + files: map[string]string{ + "CHERRY_PICK_HEAD": "2345678abcdef\n", + "rebase-merge/done": "pick 1234567abcdef one\npick 2345678abcdef two\n", + }, + expectedCherryPicking: false, + }, + { + testName: "cherry-pick while a rebase is stopped", + files: map[string]string{ + "CHERRY_PICK_HEAD": "3456789abcdef\n", + "rebase-merge/done": "pick 1234567abcdef one\nedit 2345678abcdef two\n", + }, + expectedCherryPicking: true, + }, { testName: "cherry-pick of several commits that stopped before applying one", files: map[string]string{"sequencer/todo": "pick 1234567 one\npick 2345678 two\n"}, @@ -142,3 +167,58 @@ func TestStatusWorkingTreeStateOfCherryPickOrRevert(t *testing.T) { }) } } + +func TestRebaseStoppedBeforeCommittingPick(t *testing.T) { + scenarios := []struct { + testName string + files map[string]string + expected bool + }{ + { + testName: "the rebase couldn't commit a pick", + files: map[string]string{ + "CHERRY_PICK_HEAD": "2345678abcdef\n", + "rebase-merge/done": "pick 1234567abcdef one\npick 2345678abcdef two\n", + }, + expected: true, + }, + { + testName: "the rebase stopped because a pick conflicted", + files: map[string]string{ + "CHERRY_PICK_HEAD": "2345678abcdef\n", + "rebase-merge/stopped-sha": "2345678\n", + "rebase-merge/message": "two\n", + "rebase-merge/done": "pick 1234567abcdef one\npick 2345678abcdef two\n", + }, + expected: false, + }, + { + testName: "no CHERRY_PICK_HEAD", + files: map[string]string{ + "rebase-merge/done": "pick 1234567abcdef one\nedit 2345678abcdef two\n", + }, + expected: false, + }, + { + testName: "CHERRY_PICK_HEAD of a cherry-pick while a rebase is stopped", + files: map[string]string{ + "CHERRY_PICK_HEAD": "3456789abcdef\n", + "rebase-merge/done": "pick 1234567abcdef one\nedit 2345678abcdef two\n", + }, + expected: false, + }, + } + + for _, s := range scenarios { + t.Run(s.testName, func(t *testing.T) { + gitDir := t.TempDir() + for path, content := range s.files { + fullPath := filepath.Join(gitDir, path) + assert.NoError(t, os.MkdirAll(filepath.Dir(fullPath), 0o755)) + assert.NoError(t, os.WriteFile(fullPath, []byte(content), 0o644)) + } + + assert.Equal(t, s.expected, rebaseStoppedBeforeCommittingPick(gitDir, '#')) + }) + } +} diff --git a/pkg/integration/tests/interactive_rebase/lock_error_partway_through_skip.go b/pkg/integration/tests/interactive_rebase/lock_error_partway_through_skip.go index c1d0bfdd3..f78bd9300 100644 --- a/pkg/integration/tests/interactive_rebase/lock_error_partway_through_skip.go +++ b/pkg/integration/tests/interactive_rebase/lock_error_partway_through_skip.go @@ -65,14 +65,6 @@ var LockErrorPartwayThroughSkip = NewIntegrationTest(NewIntegrationTestArgs{ Contains("A").Contains("file-y"), ) - t.Views().Status().Content(Contains("(cherry-picking)")) - - t.GlobalPress(keys.Universal.CreateRebaseOptionsMenu) - t.ExpectPopup().Menu(). - Title(Equals("Cherry-pick options")). - Select(Contains("continue")). - Confirm() - t.Views().Status().Content(Contains("(rebasing)")) t.GlobalPress(keys.Universal.CreateRebaseOptionsMenu)