diff --git a/AGENTS.md b/AGENTS.md index 96c5ba75871..d621604be0e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -390,12 +390,32 @@ Avoid phrasings like: - "cleaner than the previous approach" - "we used to ... but ..." - "after trying X, we found Y" +- "X rather than Y", where Y is what the code did before the change The iteration story is sometimes worth preserving — but it belongs in the commit message, which is the durable record of *why this change was made*. The code comment should make sense to someone who has never seen any prior version and is just trying to understand the file as it currently exists. +The tell is subtler than an explicit "we used to". A comment that justifies the +code against an alternative — "run it on a worker rather than blocking the UI", +"switch panels in `Then` rather than a moment earlier" — is history in disguise +whenever that alternative is what the code did before the change. It reads as +ordinary rationale, but the reader has no way to know the contrast is with a +version that no longer exists. + +So the check to apply is: would you have written this comment if you were +writing the file from scratch, with no diff in mind? If not, the sentence +belongs in the commit message. + +## Don't justify routine call sites + +If the codebase calls a helper in twenty places without explanation, your +twenty-first call site doesn't need one either. A comment there says "something +here is unusual"; when nothing is, it's noise — and it invites exactly the kind +of before/after justification the section above warns about. Look at the +neighboring call sites before writing one: if they're bare, match them. + ## Don't present "live with the bug" as an option When you're investigating a defect and laying out fix options for the user, diff --git a/pkg/gui/controllers/files_controller.go b/pkg/gui/controllers/files_controller.go index 567b0b6e5b6..be8e9447708 100644 --- a/pkg/gui/controllers/files_controller.go +++ b/pkg/gui/controllers/files_controller.go @@ -1508,13 +1508,20 @@ func (self *FilesController) handleStashSave(stashFunc func(message string) erro self.c.Prompt(types.PromptOpts{ Title: self.c.Tr.StashChanges, HandleConfirm: func(stashComment string) error { - self.c.LogAction(action) + return self.c.WithWaitingStatusBlockingInput( + types.WaitingStatusOpts{Message: self.c.Tr.StashingStatus}, + func(gocui.Task) error { + self.c.LogAction(action) - if err := stashFunc(stashComment); err != nil { - return err - } - self.c.Refresh(types.RefreshOptions{Scope: []types.RefreshableView{types.STASH, types.FILES}}) - return nil + if err := stashFunc(stashComment); err != nil { + return err + } + self.c.RefreshFromWorker(types.RefreshOptions{ + BatchUIUpdates: true, + Scope: []types.RefreshableView{types.STASH, types.FILES}, + }) + return nil + }) }, AllowEmptyInput: true, }) diff --git a/pkg/gui/controllers/stash_controller.go b/pkg/gui/controllers/stash_controller.go index d01fc8dbf31..03011e421c2 100644 --- a/pkg/gui/controllers/stash_controller.go +++ b/pkg/gui/controllers/stash_controller.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/jesseduffield/lazygit/pkg/commands/models" + "github.com/jesseduffield/lazygit/pkg/gocui" "github.com/jesseduffield/lazygit/pkg/gui/context" "github.com/jesseduffield/lazygit/pkg/gui/style" "github.com/jesseduffield/lazygit/pkg/gui/types" @@ -120,33 +121,29 @@ func (self *StashController) handleStashApply(stashEntry *models.StashEntry) err Title: self.c.Tr.StashApply, Prompt: self.c.Tr.SureApplyStashEntry, HandleConfirm: func() error { - self.c.LogAction(self.c.Tr.Actions.ApplyStash) - err := self.c.Git().Stash.Apply(stashEntry.Index) - self.postStashRefresh() - if err != nil { - return err - } - if self.c.UserConfig().Gui.SwitchToFilesAfterStashApply { - self.c.Context().Push(self.c.Contexts().Files, types.OnFocusOpts{}) - } - return nil + return self.c.WithWaitingStatusBlockingInput( + types.WaitingStatusOpts{Message: self.c.Tr.ApplyingStashStatus}, + func(gocui.Task) error { + self.c.LogAction(self.c.Tr.Actions.ApplyStash) + err := self.c.Git().Stash.Apply(stashEntry.Index) + self.postStashRefresh(err == nil && self.c.UserConfig().Gui.SwitchToFilesAfterStashApply) + return err + }) }, }) } func (self *StashController) handleStashPop(stashEntry *models.StashEntry) error { pop := func() error { - self.c.LogAction(self.c.Tr.Actions.PopStash) - self.c.LogCommand(fmt.Sprintf(self.c.Tr.Log.PoppingStash, stashEntry.Hash), false) - err := self.c.Git().Stash.Pop(stashEntry.Index) - self.postStashRefresh() - if err != nil { - return err - } - if self.c.UserConfig().Gui.SwitchToFilesAfterStashPop { - self.c.Context().Push(self.c.Contexts().Files, types.OnFocusOpts{}) - } - return nil + return self.c.WithWaitingStatusBlockingInput( + types.WaitingStatusOpts{Message: self.c.Tr.PoppingStashStatus}, + func(gocui.Task) error { + self.c.LogAction(self.c.Tr.Actions.PopStash) + self.c.LogCommand(fmt.Sprintf(self.c.Tr.Log.PoppingStash, stashEntry.Hash), false) + err := self.c.Git().Stash.Pop(stashEntry.Index) + self.postStashRefresh(err == nil && self.c.UserConfig().Gui.SwitchToFilesAfterStashPop) + return err + }) } if self.c.UserConfig().Gui.SkipStashWarning { @@ -175,31 +172,60 @@ func (self *StashController) handleStashDrop(stashEntries []*models.StashEntry) // iteration lets the workers race and an earlier, stale result can // land last. The indices are captured up front and we drop // highest-first, so the remaining lower indices stay valid without - // an intervening refresh. Block input until the refresh has - // landed, so that dropping the next entry in quick succession - // (confirming and pressing the key again right away) sees the - // refreshed list and not the stale, pre-drop indices. - defer self.c.RefreshBlockingInput(types.RefreshOptions{Scope: []types.RefreshableView{types.STASH}}) + // an intervening refresh. + var dropErr error for i := len(stashEntries) - 1; i >= 0; i-- { self.c.LogCommand(fmt.Sprintf(self.c.Tr.Log.DroppingStash, stashEntries[i].Hash), false) - if err := self.c.Git().Stash.Drop(stashEntries[i].Index); err != nil { - return err + if dropErr = self.c.Git().Stash.Drop(stashEntries[i].Index); dropErr != nil { + break } } - self.context().CollapseRangeSelectionToTop() - return nil + // Block input until the refresh has landed, so that dropping the + // next entry in quick succession (confirming and pressing the key + // again right away) sees the refreshed list and not the stale, + // pre-drop indices. + self.c.RefreshBlockingInput(types.RefreshOptions{ + Scope: []types.RefreshableView{types.STASH}, + Then: func() error { + // Collapse the range selection from here, so that it lands + // in the same frame as the shortened list. The refresh has + // painted the list by the time Then runs, so the new + // selection needs a focus update of its own. + if dropErr == nil { + self.context().CollapseRangeSelectionToTop() + self.context().HandleFocus(types.OnFocusOpts{}) + } + return nil + }, + }) + return dropErr }, }) return nil } -func (self *StashController) postStashRefresh() { - // Block input until the refresh has landed: popping shifts the indices of - // the remaining stash entries, and acting on the next entry in quick - // succession (confirming the popup and pressing the key again right away) - // must see the refreshed list, or it would target the wrong stash. - self.c.RefreshBlockingInput(types.RefreshOptions{Scope: []types.RefreshableView{types.STASH, types.FILES}}) +// postStashRefresh refreshes the panels that applying or popping a stash +// affects, moving the focus to the files panel if switchToFiles is set. +// +// Call it from the worker that ran the stash command, from inside a +// WithWaitingStatusBlockingInput: popping shifts the indices of the remaining +// stash entries, so acting on the next entry in quick succession (confirming +// the popup and pressing the key again right away) has to be held off until +// the refreshed list is in place, or it would target the wrong stash. +func (self *StashController) postStashRefresh(switchToFiles bool) { + self.c.RefreshFromWorker(types.RefreshOptions{ + BatchUIUpdates: true, + Scope: []types.RefreshableView{types.STASH, types.FILES}, + Then: func() error { + // Switch panels from here, so that the focus change lands in the + // same frame as the refreshed panel contents. + if switchToFiles { + self.c.Context().Push(self.c.Contexts().Files, types.OnFocusOpts{}) + } + return nil + }, + }) } func (self *StashController) handleNewBranchOffStashEntry(stashEntry *models.StashEntry) error { diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index 845155aedc0..009a78d334b 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -443,6 +443,9 @@ type TranslationSet struct { MovingCommitsToNewBranchStatus string ApplyingFilterStatus string RemovingFilterStatus string + StashingStatus string + ApplyingStashStatus string + PoppingStashStatus string CommitFiles string SubCommitsDynamicTitle string CommitFilesDynamicTitle string @@ -1602,6 +1605,9 @@ func EnglishTranslationSet() *TranslationSet { MovingCommitsToNewBranchStatus: "Moving commits to new branch", ApplyingFilterStatus: "Applying filter", RemovingFilterStatus: "Removing filter", + StashingStatus: "Stashing", + ApplyingStashStatus: "Applying stash", + PoppingStashStatus: "Popping stash", CommitFiles: "Commit files", SubCommitsDynamicTitle: "Commits (%s)", CommitFilesDynamicTitle: "Diff files (%s)",