Skip to content

Fix issue 4997 - Rebase/Fixup/Reword/Squash/Patch/Drop onto wrong commit (sibling branch) when git.log.showWholeGraph=true #5008

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix issue 4997 - Rebase/Fixup/Reword/Squash/Patch/Drop onto wrong commit (sibling branch) when git.log.showWholeGraph=true #5008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions pkg/commands/git_commands/patch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@ func (self *PatchCommands) SaveTemporaryPatch(patch string) (string, error) {
}

// DeletePatchesFromCommit applies a patch in reverse for a commit
func (self *PatchCommands) DeletePatchesFromCommit(commits []*models.Commit, commitIndex int) error {
if err := self.rebase.BeginInteractiveRebaseForCommit(commits, commitIndex, false); err != nil {
func (self *PatchCommands) DeletePatchesFromCommit(commits []*models.Commit, commitIndex int, parentIdx int) error {
if err := self.rebase.BeginInteractiveRebaseForCommit(commits, commitIndex, parentIdx, false); err != nil {
return err
}

Expand All @@ -113,12 +113,12 @@ func (self *PatchCommands) DeletePatchesFromCommit(commits []*models.Commit, com
return self.rebase.ContinueRebase()
}

func (self *PatchCommands) MovePatchToSelectedCommit(commits []*models.Commit, sourceCommitIdx int, destinationCommitIdx int) error {
func (self *PatchCommands) MovePatchToSelectedCommit(commits []*models.Commit, sourceCommitIdx int, destinationCommitIdx int, parentIdx int) error {
if sourceCommitIdx < destinationCommitIdx {
// Passing true for keepCommitsThatBecomeEmpty: if the moved-from
// commit becomes empty, we want to keep it, mainly for consistency with
// moving the patch to a *later* commit, which behaves the same.
if err := self.rebase.BeginInteractiveRebaseForCommit(commits, destinationCommitIdx, true); err != nil {
if err := self.rebase.BeginInteractiveRebaseForCommit(commits, destinationCommitIdx, parentIdx, true); err != nil {
return err
}

Expand Down Expand Up @@ -217,14 +217,14 @@ func (self *PatchCommands) MovePatchToSelectedCommit(commits []*models.Commit, s
return self.rebase.ContinueRebase()
}

func (self *PatchCommands) MovePatchIntoIndex(commits []*models.Commit, commitIdx int, stash bool) error {
func (self *PatchCommands) MovePatchIntoIndex(commits []*models.Commit, commitIdx int, parentIdx int, stash bool) error {
if stash {
if err := self.stash.Push(fmt.Sprintf(self.Tr.AutoStashForMovingPatchToIndex, commits[commitIdx].ShortHash())); err != nil {
return err
}
}

if err := self.rebase.BeginInteractiveRebaseForCommit(commits, commitIdx, false); err != nil {
if err := self.rebase.BeginInteractiveRebaseForCommit(commits, commitIdx, parentIdx, false); err != nil {
return err
}

Expand Down Expand Up @@ -275,10 +275,11 @@ func (self *PatchCommands) MovePatchIntoIndex(commits []*models.Commit, commitId
func (self *PatchCommands) PullPatchIntoNewCommit(
commits []*models.Commit,
commitIdx int,
parentIdx int,
commitSummary string,
commitDescription string,
) error {
if err := self.rebase.BeginInteractiveRebaseForCommit(commits, commitIdx, false); err != nil {
if err := self.rebase.BeginInteractiveRebaseForCommit(commits, commitIdx, parentIdx, false); err != nil {
return err
}

Expand Down Expand Up @@ -318,10 +319,11 @@ func (self *PatchCommands) PullPatchIntoNewCommit(
func (self *PatchCommands) PullPatchIntoNewCommitBefore(
commits []*models.Commit,
commitIdx int,
parentIdx int,
commitSummary string,
commitDescription string,
) error {
if err := self.rebase.BeginInteractiveRebaseForCommit(commits, commitIdx+1, true); err != nil {
if err := self.rebase.BeginInteractiveRebaseForCommit(commits, commitIdx+1, parentIdx, true); err != nil {
return err
}

Expand Down
49 changes: 22 additions & 27 deletions pkg/commands/git_commands/rebase.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@ func NewRebaseCommands(
}
}

func (self *RebaseCommands) RewordCommit(commits []*models.Commit, index int, summary string, description string) error {
func (self *RebaseCommands) RewordCommit(commits []*models.Commit, index int, parentIdx int, summary string, description string) error {
// This check is currently unreachable (handled in LocalCommitsController.reword),
// but kept as a safeguard in case this method is used elsewhere.
if self.config.NeedsGpgSubprocessForCommit() {
return errors.New(self.Tr.DisabledForGPG)
}

err := self.BeginInteractiveRebaseForCommit(commits, index, false)
err := self.BeginInteractiveRebaseForCommit(commits, index, parentIdx, false)
if err != nil {
return err
}
Expand All @@ -55,44 +55,44 @@ func (self *RebaseCommands) RewordCommit(commits []*models.Commit, index int, su
return self.ContinueRebase()
}

func (self *RebaseCommands) RewordCommitInEditor(commits []*models.Commit, index int) (*oscommands.CmdObj, error) {
func (self *RebaseCommands) RewordCommitInEditor(commits []*models.Commit, index int, parentIdx int) (*oscommands.CmdObj, error) {
changes := []daemon.ChangeTodoAction{{
Hash: commits[index].Hash(),
NewAction: todo.Reword,
}}
self.os.LogCommand(logTodoChanges(changes), false)

return self.PrepareInteractiveRebaseCommand(PrepareInteractiveRebaseCommandOpts{
baseHashOrRoot: getBaseHashOrRoot(commits, index+1),
baseHashOrRoot: getBaseHashOrRoot(commits, parentIdx),
instruction: daemon.NewChangeTodoActionsInstruction(changes),
}), nil
}

func (self *RebaseCommands) ResetCommitAuthor(commits []*models.Commit, start, end int) error {
return self.GenericAmend(commits, start, end, func(_ *models.Commit) error {
func (self *RebaseCommands) ResetCommitAuthor(commits []*models.Commit, start, end int, parentIdx int) error {
return self.GenericAmend(commits, start, end, parentIdx, func(_ *models.Commit) error {
return self.commit.ResetAuthor()
})
}

func (self *RebaseCommands) SetCommitAuthor(commits []*models.Commit, start, end int, value string) error {
return self.GenericAmend(commits, start, end, func(_ *models.Commit) error {
func (self *RebaseCommands) SetCommitAuthor(commits []*models.Commit, start, end int, parentIdx int, value string) error {
return self.GenericAmend(commits, start, end, parentIdx, func(_ *models.Commit) error {
return self.commit.SetAuthor(value)
})
}

func (self *RebaseCommands) AddCommitCoAuthor(commits []*models.Commit, start, end int, value string) error {
return self.GenericAmend(commits, start, end, func(commit *models.Commit) error {
func (self *RebaseCommands) AddCommitCoAuthor(commits []*models.Commit, start, end int, parentIdx int, value string) error {
return self.GenericAmend(commits, start, end, parentIdx, func(commit *models.Commit) error {
return self.commit.AddCoAuthor(commit.Hash(), value)
})
}

func (self *RebaseCommands) GenericAmend(commits []*models.Commit, start, end int, f func(commit *models.Commit) error) error {
func (self *RebaseCommands) GenericAmend(commits []*models.Commit, start, end int, parentIdx int, f func(commit *models.Commit) error) error {
if start == end && models.IsHeadCommit(commits, start) {
// we've selected the top commit so no rebase is required
return f(commits[start])
}

err := self.BeginInteractiveRebaseForCommitRange(commits, start, end, false)
err := self.BeginInteractiveRebaseForCommitRange(commits, start, end, parentIdx, false)
if err != nil {
return err
}
Expand Down Expand Up @@ -139,13 +139,8 @@ func (self *RebaseCommands) MoveCommitsUp(commits []*models.Commit, startIdx int
}).Run()
}

func (self *RebaseCommands) InteractiveRebase(commits []*models.Commit, startIdx int, endIdx int, action todo.TodoCommand) error {
baseIndex := endIdx + 1
if action == todo.Squash || action == todo.Fixup {
baseIndex++
}

baseHashOrRoot := getBaseHashOrRoot(commits, baseIndex)
func (self *RebaseCommands) InteractiveRebase(commits []*models.Commit, startIdx int, endIdx int, parentIdx int, action todo.TodoCommand) error {
baseHashOrRoot := getBaseHashOrRoot(commits, parentIdx)

changes := lo.FilterMap(commits[startIdx:endIdx+1], func(commit *models.Commit, _ int) (daemon.ChangeTodoAction, bool) {
return daemon.ChangeTodoAction{
Expand Down Expand Up @@ -294,7 +289,7 @@ func (self *RebaseCommands) getHashOfLastCommitMade() (string, error) {
}

// AmendTo amends the given commit with whatever files are staged
func (self *RebaseCommands) AmendTo(commits []*models.Commit, commitIndex int) error {
func (self *RebaseCommands) AmendTo(commits []*models.Commit, commitIndex int, parentIdx int) error {
commit := commits[commitIndex]

if err := self.commit.CreateFixupCommit(commit.Hash()); err != nil {
Expand All @@ -307,7 +302,7 @@ func (self *RebaseCommands) AmendTo(commits []*models.Commit, commitIndex int) e
}

return self.PrepareInteractiveRebaseCommand(PrepareInteractiveRebaseCommandOpts{
baseHashOrRoot: getBaseHashOrRoot(commits, commitIndex+1),
baseHashOrRoot: getBaseHashOrRoot(commits, parentIdx),
overrideEditor: true,
instruction: daemon.NewMoveFixupCommitDownInstruction(commit.Hash(), fixupHash, true),
}).Run()
Expand Down Expand Up @@ -401,7 +396,7 @@ func (self *RebaseCommands) SquashAllAboveFixupCommits(commit *models.Commit) er
// BeginInteractiveRebaseForCommit starts an interactive rebase to edit the current
// commit and pick all others. After this you'll want to call `self.ContinueRebase()
func (self *RebaseCommands) BeginInteractiveRebaseForCommit(
commits []*models.Commit, commitIndex int, keepCommitsThatBecomeEmpty bool,
commits []*models.Commit, commitIndex int, parentIdx int, keepCommitsThatBecomeEmpty bool,
) error {
if commitIndex < len(commits) && commits[commitIndex].IsMerge() {
if self.config.NeedsGpgSubprocessForCommit() {
Expand All @@ -415,11 +410,11 @@ func (self *RebaseCommands) BeginInteractiveRebaseForCommit(
}).Run()
}

return self.BeginInteractiveRebaseForCommitRange(commits, commitIndex, commitIndex, keepCommitsThatBecomeEmpty)
return self.BeginInteractiveRebaseForCommitRange(commits, commitIndex, commitIndex, parentIdx, keepCommitsThatBecomeEmpty)
}

func (self *RebaseCommands) BeginInteractiveRebaseForCommitRange(
commits []*models.Commit, start, end int, keepCommitsThatBecomeEmpty bool,
commits []*models.Commit, start, end int, parentIdx int, keepCommitsThatBecomeEmpty bool,
) error {
if len(commits)-1 < end {
return errors.New("index outside of range of commits")
Expand All @@ -442,7 +437,7 @@ func (self *RebaseCommands) BeginInteractiveRebaseForCommitRange(
self.os.LogCommand(logTodoChanges(changes), false)

return self.PrepareInteractiveRebaseCommand(PrepareInteractiveRebaseCommandOpts{
baseHashOrRoot: getBaseHashOrRoot(commits, end+1),
baseHashOrRoot: getBaseHashOrRoot(commits, parentIdx),
overrideEditor: true,
keepCommitsThatBecomeEmpty: keepCommitsThatBecomeEmpty,
instruction: daemon.NewChangeTodoActionsInstruction(changes),
Expand Down Expand Up @@ -515,8 +510,8 @@ func (self *RebaseCommands) runSkipEditorCommand(cmdObj *oscommands.CmdObj) erro
}

// DiscardOldFileChanges discards changes to a file from an old commit
func (self *RebaseCommands) DiscardOldFileChanges(commits []*models.Commit, commitIndex int, filePaths []string) error {
if err := self.BeginInteractiveRebaseForCommit(commits, commitIndex, false); err != nil {
func (self *RebaseCommands) DiscardOldFileChanges(commits []*models.Commit, commitIndex int, parentIdx int, filePaths []string) error {
if err := self.BeginInteractiveRebaseForCommit(commits, commitIndex, parentIdx, false); err != nil {
return err
}

Expand Down
6 changes: 5 additions & 1 deletion pkg/commands/git_commands/rebase_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ func TestRebaseDiscardOldFileChanges(t *testing.T) {
gitConfigMockResponses map[string]string
commitOpts []models.NewCommitOpts
commitIndex int
parentIndex int
fileName []string
runner *oscommands.FakeCmdObjRunner
test func(error)
Expand All @@ -111,6 +112,7 @@ func TestRebaseDiscardOldFileChanges(t *testing.T) {
gitConfigMockResponses: nil,
commitOpts: []models.NewCommitOpts{},
commitIndex: 0,
parentIndex: 0,
fileName: []string{"test999.txt"},
runner: oscommands.NewFakeRunner(t),
test: func(err error) {
Expand All @@ -122,6 +124,7 @@ func TestRebaseDiscardOldFileChanges(t *testing.T) {
gitConfigMockResponses: map[string]string{"commit.gpgSign": "true"},
commitOpts: []models.NewCommitOpts{{Name: "commit", Hash: "123456"}},
commitIndex: 0,
parentIndex: 0,
fileName: []string{"test999.txt"},
runner: oscommands.NewFakeRunner(t),
test: func(err error) {
Expand All @@ -136,6 +139,7 @@ func TestRebaseDiscardOldFileChanges(t *testing.T) {
{Name: "commit2", Hash: "abcdef"},
},
commitIndex: 0,
parentIndex: 1,
fileName: []string{"test999.txt"},
runner: oscommands.NewFakeRunner(t).
ExpectGitArgs([]string{"rebase", "--interactive", "--autostash", "--keep-empty", "--no-autosquash", "--rebase-merges", "abcdef"}, "", nil).
Expand Down Expand Up @@ -163,7 +167,7 @@ func TestRebaseDiscardOldFileChanges(t *testing.T) {
commits := lo.Map(s.commitOpts,
func(opts models.NewCommitOpts, _ int) *models.Commit { return models.NewCommit(hashPool, opts) })

s.test(instance.DiscardOldFileChanges(commits, s.commitIndex, s.fileName))
s.test(instance.DiscardOldFileChanges(commits, s.commitIndex, s.parentIndex, s.fileName))
s.runner.CheckForMissingCalls()
})
}
Expand Down
5 changes: 4 additions & 1 deletion pkg/gui/controllers/commits_files_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,10 @@ func (self *CommitFilesController) discard(selectedNodes []*filetree.CommitFileN
})
}

err := self.c.Git().Rebase.DiscardOldFileChanges(self.c.Model().Commits, self.c.Contexts().LocalCommits.GetSelectedLineIdx(), filePaths)
selectedCommits, _, endIdx := self.c.Contexts().LocalCommits.GetSelectedItems()
_, parentIdx := self.c.Helpers().Commits.GetParentCommit(selectedCommits, endIdx, 1)

err := self.c.Git().Rebase.DiscardOldFileChanges(self.c.Model().Commits, self.c.Contexts().LocalCommits.GetSelectedLineIdx(), parentIdx, filePaths)
if err := self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err); err != nil {
return err
}
Expand Down
20 changes: 15 additions & 5 deletions pkg/gui/controllers/custom_patch_options_menu_action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,10 @@ func (self *CustomPatchOptionsMenuAction) handleDeletePatchFromCommit() error {

return self.c.WithWaitingStatus(self.c.Tr.RebasingStatus, func(gocui.Task) error {
commitIndex := self.getPatchCommitIndex()
selectedCommits, _, endIdx := self.c.Contexts().LocalCommits.GetSelectedItems()
_, parentIdx := self.c.Helpers().Commits.GetParentCommit(selectedCommits, endIdx, 1)
self.c.LogAction(self.c.Tr.Actions.RemovePatchFromCommit)
err := self.c.Git().Patch.DeletePatchesFromCommit(self.c.Model().Commits, commitIndex)
err := self.c.Git().Patch.DeletePatchesFromCommit(self.c.Model().Commits, commitIndex, parentIdx)
return self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err)
})
}
Expand All @@ -160,8 +162,10 @@ func (self *CustomPatchOptionsMenuAction) handleMovePatchToSelectedCommit() erro

return self.c.WithWaitingStatus(self.c.Tr.RebasingStatus, func(gocui.Task) error {
commitIndex := self.getPatchCommitIndex()
selectedCommits, _, endIdx := self.c.Contexts().LocalCommits.GetSelectedItems()
_, parentIdx := self.c.Helpers().Commits.GetParentCommit(selectedCommits, endIdx, 1)
self.c.LogAction(self.c.Tr.Actions.MovePatchToSelectedCommit)
err := self.c.Git().Patch.MovePatchToSelectedCommit(self.c.Model().Commits, commitIndex, self.c.Contexts().LocalCommits.GetSelectedLineIdx())
err := self.c.Git().Patch.MovePatchToSelectedCommit(self.c.Model().Commits, commitIndex, self.c.Contexts().LocalCommits.GetSelectedLineIdx(), parentIdx)
return self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err)
})
}
Expand All @@ -180,8 +184,10 @@ func (self *CustomPatchOptionsMenuAction) handleMovePatchIntoWorkingTree() error
HandleConfirm: func() error {
return self.c.WithWaitingStatus(self.c.Tr.RebasingStatus, func(gocui.Task) error {
commitIndex := self.getPatchCommitIndex()
selectedCommits, _, endIdx := self.c.Contexts().LocalCommits.GetSelectedItems()
_, parentIdx := self.c.Helpers().Commits.GetParentCommit(selectedCommits, endIdx, 1)
self.c.LogAction(self.c.Tr.Actions.MovePatchIntoIndex)
err := self.c.Git().Patch.MovePatchIntoIndex(self.c.Model().Commits, commitIndex, mustStash)
err := self.c.Git().Patch.MovePatchIntoIndex(self.c.Model().Commits, commitIndex, parentIdx, mustStash)
return self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err)
})
},
Expand All @@ -196,6 +202,8 @@ func (self *CustomPatchOptionsMenuAction) handlePullPatchIntoNewCommit() error {
self.returnFocusFromPatchExplorerIfNecessary()

commitIndex := self.getPatchCommitIndex()
selectedCommits, _, endIdx := self.c.Contexts().LocalCommits.GetSelectedItems()
_, parentIdx := self.c.Helpers().Commits.GetParentCommit(selectedCommits, endIdx, 1)
self.c.Helpers().Commits.OpenCommitMessagePanel(
&helpers.OpenCommitMessagePanelOpts{
// Pass a commit index of one less than the moved-from commit, so that
Expand All @@ -209,7 +217,7 @@ func (self *CustomPatchOptionsMenuAction) handlePullPatchIntoNewCommit() error {
return self.c.WithWaitingStatus(self.c.Tr.RebasingStatus, func(gocui.Task) error {
self.c.Helpers().Commits.CloseCommitMessagePanel()
self.c.LogAction(self.c.Tr.Actions.MovePatchIntoNewCommit)
err := self.c.Git().Patch.PullPatchIntoNewCommit(self.c.Model().Commits, commitIndex, summary, description)
err := self.c.Git().Patch.PullPatchIntoNewCommit(self.c.Model().Commits, commitIndex, parentIdx, summary, description)
if err := self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err); err != nil {
return err
}
Expand All @@ -231,6 +239,8 @@ func (self *CustomPatchOptionsMenuAction) handlePullPatchIntoNewCommitBefore() e
self.returnFocusFromPatchExplorerIfNecessary()

commitIndex := self.getPatchCommitIndex()
selectedCommits, _, endIdx := self.c.Contexts().LocalCommits.GetSelectedItems()
_, parentIdx := self.c.Helpers().Commits.GetParentCommit(selectedCommits, endIdx, 2)
self.c.Helpers().Commits.OpenCommitMessagePanel(
&helpers.OpenCommitMessagePanelOpts{
// Pass a commit index of one less than the moved-from commit, so that
Expand All @@ -244,7 +254,7 @@ func (self *CustomPatchOptionsMenuAction) handlePullPatchIntoNewCommitBefore() e
return self.c.WithWaitingStatus(self.c.Tr.RebasingStatus, func(gocui.Task) error {
self.c.Helpers().Commits.CloseCommitMessagePanel()
self.c.LogAction(self.c.Tr.Actions.MovePatchIntoNewCommit)
err := self.c.Git().Patch.PullPatchIntoNewCommitBefore(self.c.Model().Commits, commitIndex, summary, description)
err := self.c.Git().Patch.PullPatchIntoNewCommitBefore(self.c.Model().Commits, commitIndex, parentIdx, summary, description)
if err := self.c.Helpers().MergeAndRebase.CheckMergeOrRebase(err); err != nil {
return err
}
Expand Down
34 changes: 34 additions & 0 deletions pkg/gui/controllers/helpers/commits_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/jesseduffield/gocui"
"github.com/jesseduffield/lazygit/pkg/commands/git_commands"
"github.com/jesseduffield/lazygit/pkg/commands/models"
"github.com/jesseduffield/lazygit/pkg/gui/types"
"github.com/samber/lo"
)
Expand Down Expand Up @@ -232,6 +233,39 @@ func (self *CommitsHelper) OpenCommitMenu(suggestionFunc func(string) []*types.S
})
}

func (self *CommitsHelper) GetParentCommit(selectedCommits []*models.Commit, endIdx int, depth int) (*models.Commit, int) {
commits := self.c.Model().Commits

currentCommit := selectedCommits[len(selectedCommits)-1]
parentIndex := endIdx
var parentCommit *models.Commit

for d := 0; d < depth; d++ {
selectedParents := currentCommit.Parents()
if len(selectedParents) == 0 {
self.c.Log.Warn("Selected commit has no parents, the nearest commit is used")
return commits[parentIndex], parentIndex + 1
}
parentHash := selectedParents[0]
found := false
for i, commit := range commits {
if commit.Hash() == parentHash {
parentIndex = i
parentCommit = commits[parentIndex]
currentCommit = parentCommit
found = true
break
}
}
if !found {
self.c.Log.Warnf("Parent commit %s not found in visible commit list, the nearest commit is used ", parentHash)
return commits[parentIndex], parentIndex + 1
}
}

return parentCommit, parentIndex
}

func (self *CommitsHelper) addCoAuthor(suggestionFunc func(string) []*types.Suggestion) error {
self.c.Prompt(types.PromptOpts{
Title: self.c.Tr.AddCoAuthorPromptTitle,
Expand Down
Loading