mirror of
https://github.com/jesseduffield/lazygit.git
synced 2026-09-10 23:56:24 -04:00
Make the patch-building secondary pane's space remove the right lines, disable its discard
The focused-main-view rework made the secondary pane actionable, but the old patch-building explorer's secondary was inert, so its actions were never thought through. Pressing space there routed through the same toggle handler as the main pane, resolving the selection against the secondary's diff and mapping it to patch-builder indices by line number. But the secondary shows the *aggregated* custom patch, which renumbers included additions whenever an earlier addition in the same hunk is excluded (Transform recomputes each hunk's +start). So the shifted number resolved to the wrong line in the original diff — often adding an unrelated line instead of removing the selected one. Resolve the secondary selection by its *ordinal* among the change lines shown instead: the custom-patch view renders exactly the included change lines in order, so the k-th change line of a file is that file's k-th included change line (PatchBuilder.IncludedChangeLineIndices), independent of the renumbering. Space in the secondary now only ever removes, mirroring how space in the staging view's staged pane unstages. Discarding from the commit (the remove key) makes no sense in the custom-patch preview — it would act on lines shown only as the patch, and space already removes them — so it's disabled there. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
6f83f02d92
commit
f7e26fec4e
|
|
@ -348,6 +348,31 @@ func (p *PatchBuilder) PatchLineIndicesForLines(filename string, lines []LineIde
|
|||
return indices, nil
|
||||
}
|
||||
|
||||
// IncludedChangeLineIndices returns the patch-line indices of the change lines (additions
|
||||
// and deletions) currently included in the patch for filename, in ascending order. These
|
||||
// are exactly the change lines the aggregated patch renders for the file, in the same
|
||||
// order, so the k-th change line shown in the custom-patch (secondary) view corresponds to
|
||||
// the k-th index here. That correspondence lets the focused main view remove a selection
|
||||
// from the patch by its ordinal among the shown change lines, sidestepping the line-number
|
||||
// renumbering the aggregated patch applies (which makes matching by identity unreliable for
|
||||
// additions). Empty when the file isn't part of the patch.
|
||||
func (p *PatchBuilder) IncludedChangeLineIndices(filename string) []int {
|
||||
info, ok := p.fileInfoMap[filename]
|
||||
if !ok || info.mode == UNSELECTED {
|
||||
return nil
|
||||
}
|
||||
lines := Parse(info.diff).Lines()
|
||||
included := append([]int{}, info.includedLineIndices...)
|
||||
sort.Ints(included)
|
||||
result := make([]int, 0, len(included))
|
||||
for _, idx := range included {
|
||||
if idx >= 0 && idx < len(lines) && (lines[idx].IsAddition() || lines[idx].IsDeletion()) {
|
||||
result = append(result, idx)
|
||||
}
|
||||
}
|
||||
return result
|
||||
}
|
||||
|
||||
// IncludedLineIdentities returns the identities of the change lines currently included
|
||||
// in the patch for filename — the identity space the inclusion gutter matches rendered
|
||||
// rows against. Empty when the file isn't part of the patch.
|
||||
|
|
|
|||
|
|
@ -608,7 +608,7 @@ func (self *CommitFilesController) OnClick(mainViewName string, clickedLineIdx i
|
|||
func (self *CommitFilesController) PrimaryAction(mainViewName string, firstLineIdx int, lastLineIdx int) error {
|
||||
from, to, reverse := self.c.Helpers().CommitFiles.CurrentFromToReverseForPatchBuilding()
|
||||
canRebase := self.context().GetCanRebase()
|
||||
return togglePatchFromFocusedMainView(self.c, mainViewName, firstLineIdx, lastLineIdx,
|
||||
return primaryPatchActionFromFocusedMainView(self.c, mainViewName, firstLineIdx, lastLineIdx,
|
||||
from, to, reverse, canRebase,
|
||||
func() {
|
||||
self.c.Refresh(types.RefreshOptions{Scope: []types.RefreshableView{types.COMMIT_FILES}})
|
||||
|
|
@ -623,8 +623,8 @@ func (self *CommitFilesController) DiscardSelection(mainViewName string, firstLi
|
|||
return discardSelectionFromCommit(self.c, mainViewName, firstLineIdx, lastLineIdx, from, to, reverse, canRebase)
|
||||
}
|
||||
|
||||
func (self *CommitFilesController) DiscardSelectionDisabledReason() *types.DisabledReason {
|
||||
return discardFromCommitDisabledReason(self.c, self.context().GetCanRebase())
|
||||
func (self *CommitFilesController) DiscardSelectionDisabledReason(mainViewName string) *types.DisabledReason {
|
||||
return discardFromCommitDisabledReason(self.c, mainViewName, self.context().GetCanRebase())
|
||||
}
|
||||
|
||||
// pathsForDiff returns the file paths to use for a diff command. When a text
|
||||
|
|
|
|||
|
|
@ -496,7 +496,10 @@ func (self *FilesController) PrimaryAction(mainViewName string, firstLineIdx int
|
|||
// DiscardSelectionDisabledReason: discarding from the working tree is always available
|
||||
// (a zero-context diff is reported as an error from DiscardSelection itself, matching the
|
||||
// staging view).
|
||||
func (self *FilesController) DiscardSelectionDisabledReason() *types.DisabledReason {
|
||||
// DiscardSelectionDisabledReason ignores which pane the action came from: discarding from
|
||||
// the working tree is meaningful in both the unstaged and staged (secondary) panes (the
|
||||
// staged side just unstages), unlike the commit panels' custom-patch preview.
|
||||
func (self *FilesController) DiscardSelectionDisabledReason(mainViewName string) *types.DisabledReason {
|
||||
return nil
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -217,6 +217,48 @@ func (self *StagingHelper) ChangeLinesInViewRange(windowName string, first int,
|
|||
return infos
|
||||
}
|
||||
|
||||
// ChangeLineOrdinalsInViewRange resolves the selected view-line range [first, last] of a
|
||||
// custom-patch (secondary) diff to, per file, the ordinals of the selected change lines
|
||||
// among that file's change lines (0-based, in displayed order), keyed by the file's
|
||||
// absolute path. The custom-patch view shows exactly the change lines included in the
|
||||
// patch, in order, so the k-th change line of a file maps to that file's k-th included
|
||||
// change line — which PatchBuilder.IncludedChangeLineIndices turns into a patch-line index
|
||||
// to remove. This is how the focused main view removes lines from the custom patch without
|
||||
// matching by line number, which the aggregated patch renumbers (unreliable for additions).
|
||||
//
|
||||
// The whole buffer is scanned to count each file's change lines from its start; only the
|
||||
// lines inside the selected range are reported. View lines that wrap to the same buffer
|
||||
// line are de-duplicated by the buffer-line walk.
|
||||
func (self *StagingHelper) ChangeLineOrdinalsInViewRange(windowName string, first int, last int) map[string][]int {
|
||||
v, _ := self.c.GocuiGui().View(self.windowHelper.GetViewNameForWindow(windowName))
|
||||
if v == nil {
|
||||
return nil
|
||||
}
|
||||
firstBuffer, ok := v.BufferLineForViewLine(first)
|
||||
if !ok {
|
||||
return nil
|
||||
}
|
||||
lastBuffer, ok := v.BufferLineForViewLine(last)
|
||||
if !ok {
|
||||
return nil
|
||||
}
|
||||
|
||||
resolved := self.resolveDiffLines(v.DiffLineContents())
|
||||
countByFile := map[string]int{}
|
||||
result := map[string][]int{}
|
||||
for i, r := range resolved {
|
||||
if !r.ok || !r.info.IsChange() {
|
||||
continue
|
||||
}
|
||||
ordinal := countByFile[r.info.Path]
|
||||
countByFile[r.info.Path]++
|
||||
if i >= firstBuffer && i <= lastBuffer {
|
||||
result[r.info.Path] = append(result[r.info.Path], ordinal)
|
||||
}
|
||||
}
|
||||
return result
|
||||
}
|
||||
|
||||
// GetDiffLineInfoForView is GetDiffLineInfo against a specific view rather than
|
||||
// one looked up by window. It is used to read the identity of the line the patch
|
||||
// explorer currently has selected when escaping back to the focused main view,
|
||||
|
|
|
|||
|
|
@ -417,7 +417,7 @@ func (self *MainViewController) discardSelectionDisabledReason() *types.Disabled
|
|||
if actions == nil {
|
||||
return nil
|
||||
}
|
||||
return actions.DiscardSelectionDisabledReason()
|
||||
return actions.DiscardSelectionDisabledReason(self.context.GetViewName())
|
||||
}
|
||||
|
||||
// copySelection copies the selected diff line(s) to the clipboard. Unlike the primary
|
||||
|
|
|
|||
|
|
@ -10,6 +10,78 @@ import (
|
|||
"github.com/samber/lo"
|
||||
)
|
||||
|
||||
// primaryPatchActionFromFocusedMainView routes the primary action (space) in a commit
|
||||
// panel's focused main view by which pane it was pressed in: the main pane toggles the
|
||||
// selection into or out of the custom patch, while the secondary pane — which previews the
|
||||
// patch being built — removes the selection from the patch (mirroring how space in the
|
||||
// staging view's staged pane unstages). Both commit-panel controllers go through here so
|
||||
// the routing lives in one place.
|
||||
func primaryPatchActionFromFocusedMainView(
|
||||
c *ControllerCommon,
|
||||
mainViewName string,
|
||||
firstLineIdx int,
|
||||
lastLineIdx int,
|
||||
from string,
|
||||
to string,
|
||||
reverse bool,
|
||||
canRebase bool,
|
||||
refresh func(),
|
||||
) error {
|
||||
if mainViewName == c.Contexts().NormalSecondary.GetViewName() {
|
||||
return removePatchLinesFromFocusedMainView(c, mainViewName, firstLineIdx, lastLineIdx, refresh)
|
||||
}
|
||||
return togglePatchFromFocusedMainView(c, mainViewName, firstLineIdx, lastLineIdx,
|
||||
from, to, reverse, canRebase, refresh)
|
||||
}
|
||||
|
||||
// removePatchLinesFromFocusedMainView removes the selected change line(s) from the custom
|
||||
// patch when space is pressed in the secondary pane (the custom-patch preview). Every line
|
||||
// shown there is already in the patch, so this only ever removes — unlike the main pane's
|
||||
// toggle. The selection is mapped to patch-line indices by its ordinal among the shown
|
||||
// change lines (see ChangeLineOrdinalsInViewRange / IncludedChangeLineIndices) rather than
|
||||
// by line number, which the aggregated patch renumbers (so matching additions by number is
|
||||
// unreliable). Refresh then re-renders the smaller patch and the selection is re-established
|
||||
// by its change-line ordinal, like the toggle path.
|
||||
func removePatchLinesFromFocusedMainView(
|
||||
c *ControllerCommon,
|
||||
mainViewName string,
|
||||
firstLineIdx int,
|
||||
lastLineIdx int,
|
||||
refresh func(),
|
||||
) error {
|
||||
ordinalsByFile := c.Helpers().Staging.ChangeLineOrdinalsInViewRange(mainViewName, firstLineIdx, lastLineIdx)
|
||||
if len(ordinalsByFile) == 0 {
|
||||
return nil
|
||||
}
|
||||
|
||||
patchBuilder := c.Git().Patch.PatchBuilder
|
||||
for absPath, ordinals := range ordinalsByFile {
|
||||
filename := patchFilename(c, absPath)
|
||||
if filename == "" {
|
||||
continue
|
||||
}
|
||||
included := patchBuilder.IncludedChangeLineIndices(filename)
|
||||
indices := make([]int, 0, len(ordinals))
|
||||
for _, ordinal := range ordinals {
|
||||
if ordinal >= 0 && ordinal < len(included) {
|
||||
indices = append(indices, included[ordinal])
|
||||
}
|
||||
}
|
||||
if len(indices) == 0 {
|
||||
continue
|
||||
}
|
||||
if err := patchBuilder.RemoveFileLineRange(filename, "", indices); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
||||
refresh()
|
||||
// A removal doesn't change the diff command, so source and target are the same pane;
|
||||
// the re-render still moves the selection in view-line space, so re-establish it.
|
||||
revealSelectionAfterPrimaryAction(c, mainViewName, mainViewName, firstLineIdx)
|
||||
return nil
|
||||
}
|
||||
|
||||
// togglePatchFromFocusedMainView toggles the selected diff line(s) — a single line, a
|
||||
// range, or a hunk — into or out of the custom patch being built for (from, to,
|
||||
// reverse). It is the patch-building counterpart of the files panel's staging handler,
|
||||
|
|
@ -136,7 +208,14 @@ func togglePatchLines(c *ControllerCommon, infos []types.DiffLineInfo) error {
|
|||
// progress, and the diff context size must be non-zero (a patch can't be built from a
|
||||
// zero-context diff). Stash and sub-commits of another branch are never rebaseable, so
|
||||
// they always get the local-commits reason.
|
||||
func discardFromCommitDisabledReason(c *ControllerCommon, canRebase bool) *types.DisabledReason {
|
||||
//
|
||||
// Discard is never offered in the secondary pane: it previews the custom patch, so
|
||||
// discarding "from the commit" there would act on lines shown only as the patch — and
|
||||
// space already removes them from the patch — which would be confusing.
|
||||
func discardFromCommitDisabledReason(c *ControllerCommon, mainViewName string, canRebase bool) *types.DisabledReason {
|
||||
if mainViewName == c.Contexts().NormalSecondary.GetViewName() {
|
||||
return &types.DisabledReason{Text: c.Tr.CannotDiscardFromCustomPatchView}
|
||||
}
|
||||
if !canRebase {
|
||||
return &types.DisabledReason{Text: c.Tr.CanOnlyDiscardFromLocalCommits}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -180,7 +180,7 @@ func (self *SwitchToDiffFilesController) PrimaryAction(mainViewName string, firs
|
|||
from, reverse := self.c.Modes().Diffing.GetFromAndReverseArgsForDiff(from)
|
||||
canRebase := self.canRebase(ref, refsRange)
|
||||
|
||||
return togglePatchFromFocusedMainView(self.c, mainViewName, firstLineIdx, lastLineIdx,
|
||||
return primaryPatchActionFromFocusedMainView(self.c, mainViewName, firstLineIdx, lastLineIdx,
|
||||
from, to, reverse, canRebase,
|
||||
func() {
|
||||
self.c.OnUIThread(func() error {
|
||||
|
|
@ -208,12 +208,12 @@ func (self *SwitchToDiffFilesController) DiscardSelection(mainViewName string, f
|
|||
return discardSelectionFromCommit(self.c, mainViewName, firstLineIdx, lastLineIdx, from, to, reverse, canRebase)
|
||||
}
|
||||
|
||||
func (self *SwitchToDiffFilesController) DiscardSelectionDisabledReason() *types.DisabledReason {
|
||||
func (self *SwitchToDiffFilesController) DiscardSelectionDisabledReason(mainViewName string) *types.DisabledReason {
|
||||
canRebase := false
|
||||
if ref := self.context.GetSelectedRef(); ref != nil {
|
||||
canRebase = self.canRebase(ref, self.context.GetSelectedRefRangeForDiffFiles())
|
||||
}
|
||||
return discardFromCommitDisabledReason(self.c, canRebase)
|
||||
return discardFromCommitDisabledReason(self.c, mainViewName, canRebase)
|
||||
}
|
||||
|
||||
func (self *SwitchToDiffFilesController) canEnter() *types.DisabledReason {
|
||||
|
|
|
|||
|
|
@ -380,7 +380,7 @@ type FocusedMainViewActions interface {
|
|||
// is unavailable here — e.g. the diff isn't a local commit's, or a rebase is in
|
||||
// progress — or nil when it's available.
|
||||
DiscardSelection(mainViewName string, firstLineIdx int, lastLineIdx int) error
|
||||
DiscardSelectionDisabledReason() *DisabledReason
|
||||
DiscardSelectionDisabledReason(mainViewName string) *DisabledReason
|
||||
}
|
||||
|
||||
type IController interface {
|
||||
|
|
|
|||
|
|
@ -457,6 +457,7 @@ type TranslationSet struct {
|
|||
CheckoutCommitFileTooltip string
|
||||
CannotCheckoutWithModifiedFilesErr string
|
||||
CanOnlyDiscardFromLocalCommits string
|
||||
CannotDiscardFromCustomPatchView string
|
||||
CannotDiscardFromMultipleCommits string
|
||||
Remove string
|
||||
DiscardOldFileChangeTooltip string
|
||||
|
|
@ -1624,6 +1625,7 @@ func EnglishTranslationSet() *TranslationSet {
|
|||
CheckoutCommitFileTooltip: "Checkout file. This replaces the file in your working tree with the version from the selected commit.",
|
||||
CannotCheckoutWithModifiedFilesErr: "You have local modifications for the file(s) you are trying to check out. You need to stash or discard these first.",
|
||||
CanOnlyDiscardFromLocalCommits: "Changes can only be discarded from local commits",
|
||||
CannotDiscardFromCustomPatchView: "Cannot discard from the custom patch view; press space to remove lines from the patch instead",
|
||||
CannotDiscardFromMultipleCommits: "Changes cannot be discarded from a multiselection of commits",
|
||||
Remove: "Remove",
|
||||
DiscardOldFileChangeTooltip: "Discard this commit's changes to this file. This runs an interactive rebase in the background, so you may get a merge conflict if a later commit also changes this file.",
|
||||
|
|
|
|||
|
|
@ -0,0 +1,110 @@
|
|||
package patch_building
|
||||
|
||||
import (
|
||||
"github.com/jesseduffield/lazygit/pkg/config"
|
||||
. "github.com/jesseduffield/lazygit/pkg/integration/components"
|
||||
)
|
||||
|
||||
var RemoveLinesFromMainViewSecondary = NewIntegrationTest(NewIntegrationTestArgs{
|
||||
Description: "Remove a line from the custom patch by pressing space on it in the secondary (custom-patch) pane of the focused main view",
|
||||
ExtraCmdArgs: []string{},
|
||||
Skip: false,
|
||||
SetupConfig: func(config *config.AppConfig) {
|
||||
// Line mode, so we can include only some of a hunk's additions and exercise the
|
||||
// renumbering the aggregated patch applies to the included ones.
|
||||
config.GetUserConfig().Gui.UseHunkModeInStagingView = false
|
||||
},
|
||||
SetupRepo: func(shell *Shell) {
|
||||
shell.NewBranch("branch-a")
|
||||
shell.CreateFileAndAdd("file1", "one\ntwo\nthree\n")
|
||||
shell.Commit("first commit")
|
||||
|
||||
// Three consecutive additions in a single hunk.
|
||||
shell.NewBranch("branch-b")
|
||||
shell.UpdateFileAndAdd("file1", "one\nADDED1\nADDED2\nADDED3\ntwo\nthree\n")
|
||||
shell.Commit("update")
|
||||
|
||||
shell.Checkout("branch-a")
|
||||
},
|
||||
Run: func(t *TestDriver, keys config.KeybindingConfig) {
|
||||
t.Views().Branches().
|
||||
Focus().
|
||||
Press(keys.Universal.NextItem).
|
||||
PressEnter()
|
||||
|
||||
t.Views().SubCommits().
|
||||
IsFocused().
|
||||
Lines(
|
||||
Contains("update").IsSelected(),
|
||||
Contains("first commit"),
|
||||
).
|
||||
Press(keys.Universal.FocusMainView)
|
||||
|
||||
// Include ADDED2 and ADDED3 in the patch but not ADDED1. Excluding ADDED1 shifts
|
||||
// the new-file line numbers of ADDED2/ADDED3 in the aggregated patch, which is what
|
||||
// used to make removing them by line number from the secondary act on the wrong line.
|
||||
t.Views().Main().
|
||||
IsFocused().
|
||||
SelectedLines(
|
||||
Contains("+ADDED1"),
|
||||
).
|
||||
Press(keys.Universal.NextItem).
|
||||
SelectedLines(
|
||||
Contains("+ADDED2"),
|
||||
).
|
||||
Press(keys.Universal.RangeSelectDown).
|
||||
SelectedLines(
|
||||
Contains("+ADDED2"),
|
||||
Contains("+ADDED3"),
|
||||
).
|
||||
PressPrimaryAction()
|
||||
|
||||
// The cumulative patch holds ADDED2 and ADDED3 only.
|
||||
t.Views().Secondary().
|
||||
Content(Contains("+ADDED2")).
|
||||
Content(Contains("+ADDED3")).
|
||||
Content(DoesNotContain("ADDED1"))
|
||||
|
||||
// Tab into the secondary (custom-patch) pane; the selection lands on its first
|
||||
// change line, ADDED2.
|
||||
t.Views().Main().
|
||||
Press(keys.Universal.TogglePanel)
|
||||
|
||||
t.Views().Secondary().
|
||||
IsFocused().
|
||||
SelectedLines(
|
||||
Contains("+ADDED2"),
|
||||
).
|
||||
// Discarding from the commit makes no sense in the custom-patch preview, so it's
|
||||
// disabled here (you remove from the patch with space instead).
|
||||
Press(keys.Universal.Remove)
|
||||
|
||||
t.ExpectToast(Contains("Cannot discard from the custom patch view"))
|
||||
|
||||
t.Views().Secondary().
|
||||
// Space removes the selected line from the patch — and removes ADDED2, not some
|
||||
// other line resolved from its shifted line number.
|
||||
PressPrimaryAction().
|
||||
// The selection lands on the next surviving change, ADDED3.
|
||||
SelectedLines(
|
||||
Contains("+ADDED3"),
|
||||
).
|
||||
Content(Contains("+ADDED3")).
|
||||
Content(DoesNotContain("ADDED2")).
|
||||
Content(DoesNotContain("ADDED1"))
|
||||
|
||||
// Applying confirms only ADDED3 was ever in the patch.
|
||||
t.Common().SelectPatchOption(MatchesRegexp(`Apply patch$`))
|
||||
|
||||
t.Views().Files().
|
||||
Focus().
|
||||
Lines(
|
||||
Contains("file1").IsSelected(),
|
||||
)
|
||||
|
||||
t.Views().Main().
|
||||
Content(Contains("ADDED3")).
|
||||
Content(DoesNotContain("ADDED1")).
|
||||
Content(DoesNotContain("ADDED2"))
|
||||
},
|
||||
})
|
||||
|
|
@ -389,6 +389,7 @@ var tests = []*components.IntegrationTest{
|
|||
patch_building.MoveToNewCommitInLastCommitOfStackedBranch,
|
||||
patch_building.MoveToNewCommitPartialHunk,
|
||||
patch_building.RemoveFromCommit,
|
||||
patch_building.RemoveLinesFromMainViewSecondary,
|
||||
patch_building.RemovePartsOfAddedFile,
|
||||
patch_building.RenameSimilarityThresholdChange,
|
||||
patch_building.RenamedFilePartial,
|
||||
|
|
|
|||
Loading…
Reference in a new issue