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,