From f7e26fec4eb7a2acf04166d6613bc6250ec6ce54 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 24 Jun 2026 13:17:15 +0200 Subject: [PATCH] Make the patch-building secondary pane's space remove the right lines, disable its discard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- pkg/commands/patch/patch_builder.go | 25 ++++ .../controllers/commits_files_controller.go | 6 +- pkg/gui/controllers/files_controller.go | 5 +- pkg/gui/controllers/helpers/staging_helper.go | 42 +++++++ pkg/gui/controllers/main_view_controller.go | 2 +- .../patch_building_from_main_view.go | 81 ++++++++++++- .../switch_to_diff_files_controller.go | 6 +- pkg/gui/types/context.go | 2 +- pkg/i18n/english.go | 2 + .../remove_lines_from_main_view_secondary.go | 110 ++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + 11 files changed, 272 insertions(+), 10 deletions(-) create mode 100644 pkg/integration/tests/patch_building/remove_lines_from_main_view_secondary.go diff --git a/pkg/commands/patch/patch_builder.go b/pkg/commands/patch/patch_builder.go index 890a53518..9aaa3c268 100644 --- a/pkg/commands/patch/patch_builder.go +++ b/pkg/commands/patch/patch_builder.go @@ -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. diff --git a/pkg/gui/controllers/commits_files_controller.go b/pkg/gui/controllers/commits_files_controller.go index 51843c1b6..6f1b4725a 100644 --- a/pkg/gui/controllers/commits_files_controller.go +++ b/pkg/gui/controllers/commits_files_controller.go @@ -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 diff --git a/pkg/gui/controllers/files_controller.go b/pkg/gui/controllers/files_controller.go index 6310e8c69..cde2a3424 100644 --- a/pkg/gui/controllers/files_controller.go +++ b/pkg/gui/controllers/files_controller.go @@ -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 } diff --git a/pkg/gui/controllers/helpers/staging_helper.go b/pkg/gui/controllers/helpers/staging_helper.go index 95ac396ce..bd015e24f 100644 --- a/pkg/gui/controllers/helpers/staging_helper.go +++ b/pkg/gui/controllers/helpers/staging_helper.go @@ -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, diff --git a/pkg/gui/controllers/main_view_controller.go b/pkg/gui/controllers/main_view_controller.go index 77a3b697c..aa3746b5b 100644 --- a/pkg/gui/controllers/main_view_controller.go +++ b/pkg/gui/controllers/main_view_controller.go @@ -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 diff --git a/pkg/gui/controllers/patch_building_from_main_view.go b/pkg/gui/controllers/patch_building_from_main_view.go index 4fba9054f..f446cd78f 100644 --- a/pkg/gui/controllers/patch_building_from_main_view.go +++ b/pkg/gui/controllers/patch_building_from_main_view.go @@ -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} } diff --git a/pkg/gui/controllers/switch_to_diff_files_controller.go b/pkg/gui/controllers/switch_to_diff_files_controller.go index 28e0594e2..79d489a1e 100644 --- a/pkg/gui/controllers/switch_to_diff_files_controller.go +++ b/pkg/gui/controllers/switch_to_diff_files_controller.go @@ -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 { diff --git a/pkg/gui/types/context.go b/pkg/gui/types/context.go index e8196cea6..82ef1ce9d 100644 --- a/pkg/gui/types/context.go +++ b/pkg/gui/types/context.go @@ -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 { diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index acfc0b111..15df07786 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -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.", diff --git a/pkg/integration/tests/patch_building/remove_lines_from_main_view_secondary.go b/pkg/integration/tests/patch_building/remove_lines_from_main_view_secondary.go new file mode 100644 index 000000000..9a720c9c2 --- /dev/null +++ b/pkg/integration/tests/patch_building/remove_lines_from_main_view_secondary.go @@ -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")) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 23bfff2de..c539f2a71 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -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,