From 8ca78412acdc73526248ff10c332f98ceba39fd2 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Tue, 1 Aug 2023 14:54:56 +0200 Subject: [PATCH 1/2] Add command to find base commit for creating a fixup --- docs/Config.md | 1 + docs/Fixup_Commits.md | 64 +++++++ docs/keybindings/Keybindings_en.md | 1 + docs/keybindings/Keybindings_ja.md | 1 + docs/keybindings/Keybindings_ko.md | 1 + docs/keybindings/Keybindings_nl.md | 1 + docs/keybindings/Keybindings_pl.md | 1 + docs/keybindings/Keybindings_ru.md | 1 + docs/keybindings/Keybindings_zh-CN.md | 1 + docs/keybindings/Keybindings_zh-TW.md | 1 + pkg/commands/git.go | 3 + pkg/commands/git_commands/blame.go | 33 ++++ pkg/commands/git_commands/commit.go | 14 ++ pkg/commands/git_commands/diff.go | 8 + pkg/config/user_config.go | 2 + pkg/gui/controllers.go | 1 + pkg/gui/controllers/files_controller.go | 6 + pkg/gui/controllers/helpers/fixup_helper.go | 178 ++++++++++++++++++ pkg/gui/controllers/helpers/helpers.go | 2 + pkg/i18n/english.go | 16 ++ .../commit/find_base_commit_for_fixup.go | 79 ++++++++ pkg/integration/tests/test_list.go | 1 + schema/config.json | 4 + 23 files changed, 420 insertions(+) create mode 100644 docs/Fixup_Commits.md create mode 100644 pkg/commands/git_commands/blame.go create mode 100644 pkg/gui/controllers/helpers/fixup_helper.go create mode 100644 pkg/integration/tests/commit/find_base_commit_for_fixup.go diff --git a/docs/Config.md b/docs/Config.md index 741a2c2b90a..8b37c6aaa65 100644 --- a/docs/Config.md +++ b/docs/Config.md @@ -209,6 +209,7 @@ keybinding: commitChangesWithoutHook: 'w' # commit changes without pre-commit hook amendLastCommit: 'A' commitChangesWithEditor: 'C' + findBaseCommitForFixup: '' confirmDiscard: 'x' ignoreFile: 'i' refreshFiles: 'r' diff --git a/docs/Fixup_Commits.md b/docs/Fixup_Commits.md new file mode 100644 index 00000000000..1d148c546c3 --- /dev/null +++ b/docs/Fixup_Commits.md @@ -0,0 +1,64 @@ +# Fixup Commits + +## Background + +There's this common scenario that you have a PR in review, the reviewer is +requesting some changes, and you make those changes and would normally simply +squash them into the original commit that they came from. If you do that, +however, there's no way for the reviewer to see what you changed. You could just +make a separate commit with those changes at the end of the branch, but this is +not ideal because it results in a git history that is not very clean. + +To help with this, git has a concept of fixup commits: you do make a separate +commit, but the subject of this commit is the string "fixup! " followed by the +original commit subject. This both tells the reviewer what's going on (you are +making a change that you later will squash into the designated commit), and it +provides an easy way to actually perform this squash operation when you are +ready to do that (before merging). + +## Creating fixup commits + +You could of course create fixup commits manually by typing in the commit +message with the prefix yourself. But lazygit has an easier way to do that: +in the Commits view, select the commit that you want to create a fixup for, and +press shift-F (for "Create fixup commit for this commit"). This automatically +creates a commit with the appropriate subject line. + +Don't confuse this with the lowercase "f" command ("Fixup commit"); that one +squashes the selected commit into its parent, this is not what we want here. + +## Squashing fixup commits + +When you're ready to merge the branch and want to squash all these fixup commits +that you created, that's very easy to do: select the first commit of your branch +and hit shift-S (for "Squash all 'fixup!' commits above selected commit +(autosquash)"). Boom, done. + +## Finding the commit to create a fixup for + +When you are making changes to code that you changed earlier in a long branch, +it can be tedious to find the commit to squash it into. Lazygit has a command to +help you with this, too: in the Files view, press ctrl-f to select the right +base commit in the Commits view automatically. From there, you can either press +shift-F to create a fixup commit for it, or shift-A to amend your changes into +the commit if you haven't published your branch yet. + +This command works in many cases, and when it does it almost feels like magic, +but it's important to understand its limitations because it doesn't always work. +The way it works is that it looks at the deleted lines of your current +modifications, blames them to find out which commit those lines come from, and +if they all come from the same commit, it selects it. So here are cases where it +doesn't work: + +- Your current diff has only added lines, but no deleted lines. In this case + there's no way for lazygit to know which commit you want to add them to. +- The deleted lines belong to multiple different commits. In this case you can + help lazygit by staging a set of files or hunks that all belong to the same + commit; if some changes are staged, the ctrl-f command works only on those. +- The found commit is already on master; in this case, lazygit refuses to select + it, because it doesn't make sense to create fixups for it, let alone amend to + it. + +To sum it up: the command works great if you are changing code again that you +changed or added earlier in the same branch. This is a common enough case to +make the command useful. diff --git a/docs/keybindings/Keybindings_en.md b/docs/keybindings/Keybindings_en.md index f7aaa46c817..ef6299613bf 100644 --- a/docs/keybindings/Keybindings_en.md +++ b/docs/keybindings/Keybindings_en.md @@ -123,6 +123,7 @@ _Legend: `` means ctrl+b, `` means alt+b, `B` means shift+b_ w: Commit changes without pre-commit hook A: Amend last commit C: Commit changes using git editor + <c-f>: Find base commit for fixup e: Edit file o: Open file i: Ignore or exclude file diff --git a/docs/keybindings/Keybindings_ja.md b/docs/keybindings/Keybindings_ja.md index a5a54b872ca..36389ecc111 100644 --- a/docs/keybindings/Keybindings_ja.md +++ b/docs/keybindings/Keybindings_ja.md @@ -196,6 +196,7 @@ _Legend: `` means ctrl+b, `` means alt+b, `B` means shift+b_ w: pre-commitフックを実行せずに変更をコミット A: 最新のコミットにamend C: gitエディタを使用して変更をコミット + <c-f>: Find base commit for fixup e: ファイルを編集 o: ファイルを開く i: ファイルをignore diff --git a/docs/keybindings/Keybindings_ko.md b/docs/keybindings/Keybindings_ko.md index fc7291c4ba8..d9628319205 100644 --- a/docs/keybindings/Keybindings_ko.md +++ b/docs/keybindings/Keybindings_ko.md @@ -336,6 +336,7 @@ _Legend: `` means ctrl+b, `` means alt+b, `B` means shift+b_ w: Commit changes without pre-commit hook A: 마지맛 커밋 수정 C: Git 편집기를 사용하여 변경 내용을 커밋합니다. + <c-f>: Find base commit for fixup e: 파일 편집 o: 파일 닫기 i: Ignore file diff --git a/docs/keybindings/Keybindings_nl.md b/docs/keybindings/Keybindings_nl.md index c2d81a06d47..23bf0a47715 100644 --- a/docs/keybindings/Keybindings_nl.md +++ b/docs/keybindings/Keybindings_nl.md @@ -56,6 +56,7 @@ _Legend: `` means ctrl+b, `` means alt+b, `B` means shift+b_ w: Commit veranderingen zonder pre-commit hook A: Wijzig laatste commit C: Commit veranderingen met de git editor + <c-f>: Find base commit for fixup e: Verander bestand o: Open bestand i: Ignore or exclude file diff --git a/docs/keybindings/Keybindings_pl.md b/docs/keybindings/Keybindings_pl.md index 2136a5cdf69..601ec72f593 100644 --- a/docs/keybindings/Keybindings_pl.md +++ b/docs/keybindings/Keybindings_pl.md @@ -157,6 +157,7 @@ _Legend: `` means ctrl+b, `` means alt+b, `B` means shift+b_ w: Zatwierdź zmiany bez skryptu pre-commit A: Zmień ostatni commit C: Zatwierdź zmiany używając edytora + <c-f>: Find base commit for fixup e: Edytuj plik o: Otwórz plik i: Ignore or exclude file diff --git a/docs/keybindings/Keybindings_ru.md b/docs/keybindings/Keybindings_ru.md index ab82515ad5a..6277f3db8d4 100644 --- a/docs/keybindings/Keybindings_ru.md +++ b/docs/keybindings/Keybindings_ru.md @@ -330,6 +330,7 @@ _Связки клавиш_ w: Закоммитить изменения без предварительного хука коммита A: Правка последнего коммита C: Сохранить изменения с помощью редактора git + <c-f>: Find base commit for fixup e: Редактировать файл o: Открыть файл i: Игнорировать или исключить файл diff --git a/docs/keybindings/Keybindings_zh-CN.md b/docs/keybindings/Keybindings_zh-CN.md index 49bce5ece13..4b4a8e028da 100644 --- a/docs/keybindings/Keybindings_zh-CN.md +++ b/docs/keybindings/Keybindings_zh-CN.md @@ -204,6 +204,7 @@ _Legend: `` means ctrl+b, `` means alt+b, `B` means shift+b_ w: 提交更改而无需预先提交钩子 A: 修补最后一次提交 C: 提交更改（使用编辑器编辑提交信息） + <c-f>: Find base commit for fixup e: 编辑文件 o: 打开文件 i: 忽略文件 diff --git a/docs/keybindings/Keybindings_zh-TW.md b/docs/keybindings/Keybindings_zh-TW.md index 201f7955500..8fd6a274bda 100644 --- a/docs/keybindings/Keybindings_zh-TW.md +++ b/docs/keybindings/Keybindings_zh-TW.md @@ -299,6 +299,7 @@ _說明：`` 表示 Ctrl+B、`` 表示 Alt+B，`B`表示 Shift+B_ w: 沒有預提交 hook 就提交更改 A: 修正上次提交 C: 使用 git 編輯器提交變更 + <c-f>: Find base commit for fixup e: 編輯檔案 o: 開啟檔案 i: 忽略或排除檔案 diff --git a/pkg/commands/git.go b/pkg/commands/git.go index 51066103416..b1b04a72fa0 100644 --- a/pkg/commands/git.go +++ b/pkg/commands/git.go @@ -21,6 +21,7 @@ import ( // GitCommand is our main git interface type GitCommand struct { + Blame *git_commands.BlameCommands Branch *git_commands.BranchCommands Commit *git_commands.CommitCommands Config *git_commands.ConfigCommands @@ -160,6 +161,7 @@ func NewGitCommandAux( patchCommands := git_commands.NewPatchCommands(gitCommon, rebaseCommands, commitCommands, statusCommands, stashCommands, patchBuilder) bisectCommands := git_commands.NewBisectCommands(gitCommon) worktreeCommands := git_commands.NewWorktreeCommands(gitCommon) + blameCommands := git_commands.NewBlameCommands(gitCommon) branchLoader := git_commands.NewBranchLoader(cmn, cmd, branchCommands.CurrentBranchInfo, configCommands) commitFileLoader := git_commands.NewCommitFileLoader(cmn, cmd) @@ -171,6 +173,7 @@ func NewGitCommandAux( tagLoader := git_commands.NewTagLoader(cmn, cmd) return &GitCommand{ + Blame: blameCommands, Branch: branchCommands, Commit: commitCommands, Config: configCommands, diff --git a/pkg/commands/git_commands/blame.go b/pkg/commands/git_commands/blame.go new file mode 100644 index 00000000000..aba1c63fe84 --- /dev/null +++ b/pkg/commands/git_commands/blame.go @@ -0,0 +1,33 @@ +package git_commands + +import ( + "fmt" +) + +type BlameCommands struct { + *GitCommon +} + +func NewBlameCommands(gitCommon *GitCommon) *BlameCommands { + return &BlameCommands{ + GitCommon: gitCommon, + } +} + +// Blame a range of lines. For each line, output the hash of the commit where +// the line last changed, then a space, then a description of the commit (author +// and date), another space, and then the line. For example: +// +// ac90ebac688fe8bc2ffd922157a9d2c54681d2aa (Stefan Haller 2023-08-01 14:54:56 +0200 11) func NewBlameCommands(gitCommon *GitCommon) *BlameCommands { +// ac90ebac688fe8bc2ffd922157a9d2c54681d2aa (Stefan Haller 2023-08-01 14:54:56 +0200 12) return &BlameCommands{ +// ac90ebac688fe8bc2ffd922157a9d2c54681d2aa (Stefan Haller 2023-08-01 14:54:56 +0200 13) GitCommon: gitCommon, +func (self *BlameCommands) BlameLineRange(filename string, commit string, firstLine int, numLines int) (string, error) { + cmdArgs := NewGitCmd("blame"). + Arg("-l"). + Arg(fmt.Sprintf("-L%d,+%d", firstLine, numLines)). + Arg(commit). + Arg("--"). + Arg(filename) + + return self.cmd.New(cmdArgs.ToArgv()).RunWithOutput() +} diff --git a/pkg/commands/git_commands/commit.go b/pkg/commands/git_commands/commit.go index 443289f6ea8..e0b5b8a9aeb 100644 --- a/pkg/commands/git_commands/commit.go +++ b/pkg/commands/git_commands/commit.go @@ -198,6 +198,20 @@ func (self *CommitCommands) GetCommitMessagesFirstLine(shas []string) (string, e return self.cmd.New(cmdArgs).DontLog().RunWithOutput() } +// Example output: +// +// cd50c79ae Preserve the commit message correctly even if the description has blank lines +// 3ebba5f32 Add test demonstrating a bug with preserving the commit message +// 9a423c388 Remove unused function +func (self *CommitCommands) GetShasAndCommitMessagesFirstLine(shas []string) (string, error) { + cmdArgs := NewGitCmd("show"). + Arg("--no-patch", "--pretty=format:%h %s"). + Arg(shas...). + ToArgv() + + return self.cmd.New(cmdArgs).DontLog().RunWithOutput() +} + func (self *CommitCommands) GetCommitsOneline(shas []string) (string, error) { cmdArgs := NewGitCmd("show"). Arg("--no-patch", "--oneline"). diff --git a/pkg/commands/git_commands/diff.go b/pkg/commands/git_commands/diff.go index 41e71e94116..3729390241a 100644 --- a/pkg/commands/git_commands/diff.go +++ b/pkg/commands/git_commands/diff.go @@ -78,3 +78,11 @@ func (self *DiffCommands) OpenDiffToolCmdObj(opts DiffToolCmdOptions) oscommands Arg("--", opts.Filepath). ToArgv()) } + +func (self *DiffCommands) DiffIndexCmdObj(diffArgs ...string) oscommands.ICmdObj { + return self.cmd.New( + NewGitCmd("diff-index"). + Arg("--submodule", "--no-ext-diff", "--no-color", "--patch"). + Arg(diffArgs...).ToArgv(), + ) +} diff --git a/pkg/config/user_config.go b/pkg/config/user_config.go index ffc27eb88d6..d0d5184551f 100644 --- a/pkg/config/user_config.go +++ b/pkg/config/user_config.go @@ -366,6 +366,7 @@ type KeybindingFilesConfig struct { CommitChangesWithoutHook string `yaml:"commitChangesWithoutHook"` AmendLastCommit string `yaml:"amendLastCommit"` CommitChangesWithEditor string `yaml:"commitChangesWithEditor"` + FindBaseCommitForFixup string `yaml:"findBaseCommitForFixup"` ConfirmDiscard string `yaml:"confirmDiscard"` IgnoreFile string `yaml:"ignoreFile"` RefreshFiles string `yaml:"refreshFiles"` @@ -762,6 +763,7 @@ func GetDefaultConfig() *UserConfig { CommitChangesWithoutHook: "w", AmendLastCommit: "A", CommitChangesWithEditor: "C", + FindBaseCommitForFixup: "", IgnoreFile: "i", RefreshFiles: "r", StashAllChanges: "s", diff --git a/pkg/gui/controllers.go b/pkg/gui/controllers.go index 37c54a655a5..b4cbec79e88 100644 --- a/pkg/gui/controllers.go +++ b/pkg/gui/controllers.go @@ -103,6 +103,7 @@ func (gui *Gui) resetHelpersAndControllers() { CherryPick: cherryPickHelper, Upstream: helpers.NewUpstreamHelper(helperCommon, suggestionsHelper.GetRemoteBranchesSuggestionsFunc), AmendHelper: helpers.NewAmendHelper(helperCommon, gpgHelper), + FixupHelper: helpers.NewFixupHelper(helperCommon), Commits: commitsHelper, Snake: helpers.NewSnakeHelper(helperCommon), Diff: diffHelper, diff --git a/pkg/gui/controllers/files_controller.go b/pkg/gui/controllers/files_controller.go index 0db509ca3b0..d60773ec14d 100644 --- a/pkg/gui/controllers/files_controller.go +++ b/pkg/gui/controllers/files_controller.go @@ -64,6 +64,12 @@ func (self *FilesController) GetKeybindings(opts types.KeybindingsOpts) []*types Handler: self.c.Helpers().WorkingTree.HandleCommitEditorPress, Description: self.c.Tr.CommitChangesWithEditor, }, + { + Key: opts.GetKey(opts.Config.Files.FindBaseCommitForFixup), + Handler: self.c.Helpers().FixupHelper.HandleFindBaseCommitForFixupPress, + Description: self.c.Tr.FindBaseCommitForFixup, + Tooltip: self.c.Tr.FindBaseCommitForFixupTooltip, + }, { Key: opts.GetKey(opts.Config.Universal.Edit), Handler: self.checkSelectedFileNode(self.edit), diff --git a/pkg/gui/controllers/helpers/fixup_helper.go b/pkg/gui/controllers/helpers/fixup_helper.go new file mode 100644 index 00000000000..2198d11cb2b --- /dev/null +++ b/pkg/gui/controllers/helpers/fixup_helper.go @@ -0,0 +1,178 @@ +package helpers + +import ( + "regexp" + "strings" + "sync" + + "github.com/jesseduffield/generics/set" + "github.com/jesseduffield/lazygit/pkg/commands/models" + "github.com/jesseduffield/lazygit/pkg/gui/types" + "github.com/jesseduffield/lazygit/pkg/utils" + "github.com/samber/lo" +) + +type FixupHelper struct { + c *HelperCommon +} + +func NewFixupHelper( + c *HelperCommon, +) *FixupHelper { + return &FixupHelper{ + c: c, + } +} + +type deletedLineInfo struct { + filename string + startLineIdx int + numLines int +} + +func (self *FixupHelper) HandleFindBaseCommitForFixupPress() error { + diff, hasStagedChanges, err := self.getDiff() + if err != nil { + return err + } + if diff == "" { + return self.c.ErrorMsg(self.c.Tr.NoChangedFiles) + } + + deletedLineInfos := self.parseDiff(diff) + if len(deletedLineInfos) == 0 { + return self.c.ErrorMsg(self.c.Tr.NoDeletedLinesInDiff) + } + + shas := self.blameDeletedLines(deletedLineInfos) + + if len(shas) == 0 { + // This should never happen + return self.c.ErrorMsg(self.c.Tr.NoBaseCommitsFound) + } + if len(shas) > 1 { + subjects, err := self.c.Git().Commit.GetShasAndCommitMessagesFirstLine(shas) + if err != nil { + return err + } + message := lo.Ternary(hasStagedChanges, + self.c.Tr.MultipleBaseCommitsFoundStaged, + self.c.Tr.MultipleBaseCommitsFoundUnstaged) + return self.c.ErrorMsg(message + "\n\n" + subjects) + } + + commit, index, ok := lo.FindIndexOf(self.c.Model().Commits, func(commit *models.Commit) bool { + return commit.Sha == shas[0] + }) + if !ok { + commits := self.c.Model().Commits + if commits[len(commits)-1].Status == models.StatusMerged { + // If the commit is not found, it's most likely because it's already + // merged, and more than 300 commits away. Check if the last known + // commit is already merged; if so, show the "already merged" error. + return self.c.ErrorMsg(self.c.Tr.BaseCommitIsAlreadyOnMainBranch) + } + // If we get here, the current branch must have more then 300 commits. Unlikely... + return self.c.ErrorMsg(self.c.Tr.BaseCommitIsNotInCurrentView) + } + if commit.Status == models.StatusMerged { + return self.c.ErrorMsg(self.c.Tr.BaseCommitIsAlreadyOnMainBranch) + } + + if !useIndex { + if err := self.c.Git().WorkingTree.StageAll(); err != nil { + return err + } + _ = self.c.Refresh(types.RefreshOptions{Mode: types.SYNC, Scope: []types.RefreshableView{types.FILES}}) + } + + self.c.Contexts().LocalCommits.SetSelectedLineIdx(index) + return self.c.PushContext(self.c.Contexts().LocalCommits) +} + +func (self *FixupHelper) getDiff() (string, bool, error) { + args := []string{"-U0", "--ignore-submodules=all", "HEAD", "--"} + + // Try staged changes first + hasStagedChanges := true + diff, err := self.c.Git().Diff.DiffIndexCmdObj(append([]string{"--cached"}, args...)...).RunWithOutput() + + if err == nil && diff == "" { + hasStagedChanges = false + // If there are no staged changes, try unstaged changes + diff, err = self.c.Git().Diff.DiffIndexCmdObj(args...).RunWithOutput() + } + + return diff, hasStagedChanges, err +} + +func (self *FixupHelper) parseDiff(diff string) []*deletedLineInfo { + lines := strings.Split(strings.TrimSuffix(diff, "\n"), "\n") + + deletedLineInfos := []*deletedLineInfo{} + + hunkHeaderRegexp := regexp.MustCompile(`@@ -(\d+)(?:,\d+)? \+\d+(?:,\d+)? @@`) + + var filename string + var currentLineInfo *deletedLineInfo + finishHunk := func() { + if currentLineInfo != nil && currentLineInfo.numLines > 0 { + deletedLineInfos = append(deletedLineInfos, currentLineInfo) + } + } + for _, line := range lines { + if strings.HasPrefix(line, "diff --git") { + finishHunk() + currentLineInfo = nil + } else if strings.HasPrefix(line, "--- ") { + // For some reason, the line ends with a tab character if the file + // name contains spaces + filename = strings.TrimRight(line[6:], "\t") + } else if strings.HasPrefix(line, "@@ ") { + finishHunk() + match := hunkHeaderRegexp.FindStringSubmatch(line) + startIdx := utils.MustConvertToInt(match[1]) + currentLineInfo = &deletedLineInfo{filename, startIdx, 0} + } else if currentLineInfo != nil && line[0] == '-' { + currentLineInfo.numLines++ + } + } + finishHunk() + + return deletedLineInfos +} + +// returns the list of commit hashes that introduced the lines which have now been deleted +func (self *FixupHelper) blameDeletedLines(deletedLineInfos []*deletedLineInfo) []string { + var wg sync.WaitGroup + shaChan := make(chan string) + + for _, info := range deletedLineInfos { + wg.Add(1) + go func(info *deletedLineInfo) { + defer wg.Done() + + blameOutput, err := self.c.Git().Blame.BlameLineRange(info.filename, "HEAD", info.startLineIdx, info.numLines) + if err != nil { + self.c.Log.Errorf("Error blaming file '%s': %v", info.filename, err) + return + } + blameLines := strings.Split(strings.TrimSuffix(blameOutput, "\n"), "\n") + for _, line := range blameLines { + shaChan <- strings.Split(line, " ")[0] + } + }(info) + } + + go func() { + wg.Wait() + close(shaChan) + }() + + result := set.New[string]() + for sha := range shaChan { + result.Add(sha) + } + + return result.ToSlice() +} diff --git a/pkg/gui/controllers/helpers/helpers.go b/pkg/gui/controllers/helpers/helpers.go index 9e05ba16374..1f1050dc974 100644 --- a/pkg/gui/controllers/helpers/helpers.go +++ b/pkg/gui/controllers/helpers/helpers.go @@ -33,6 +33,7 @@ type Helpers struct { GPG *GpgHelper Upstream *UpstreamHelper AmendHelper *AmendHelper + FixupHelper *FixupHelper Commits *CommitsHelper Snake *SnakeHelper // lives in context package because our contexts need it to render to main @@ -70,6 +71,7 @@ func NewStubHelpers() *Helpers { GPG: &GpgHelper{}, Upstream: &UpstreamHelper{}, AmendHelper: &AmendHelper{}, + FixupHelper: &FixupHelper{}, Commits: &CommitsHelper{}, Snake: &SnakeHelper{}, Diff: &DiffHelper{}, diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index 34df4aba187..e2559f3d21a 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -39,6 +39,14 @@ type TranslationSet struct { SureToAmend string NoCommitToAmend string CommitChangesWithEditor string + FindBaseCommitForFixup string + FindBaseCommitForFixupTooltip string + NoDeletedLinesInDiff string + NoBaseCommitsFound string + MultipleBaseCommitsFoundStaged string + MultipleBaseCommitsFoundUnstaged string + BaseCommitIsAlreadyOnMainBranch string + BaseCommitIsNotInCurrentView string StatusTitle string GlobalTitle string Menu string @@ -858,6 +866,14 @@ func EnglishTranslationSet() TranslationSet { SureToAmend: "Are you sure you want to amend last commit? Afterwards, you can change the commit message from the commits panel.", NoCommitToAmend: "There's no commit to amend.", CommitChangesWithEditor: "Commit changes using git editor", + FindBaseCommitForFixup: "Find base commit for fixup", + FindBaseCommitForFixupTooltip: "Find the commit that your current changes are building upon, for the sake of amending/fixing up the commit. This spares you from having to look through your branch's commits one-by-one to see which commit should be amended/fixed up. See docs: ", + NoDeletedLinesInDiff: "No deleted lines in diff", + NoBaseCommitsFound: "No base commits found", + MultipleBaseCommitsFoundStaged: "Multiple base commits found. (Try staging fewer changes at once)", + MultipleBaseCommitsFoundUnstaged: "Multiple base commits found. (Try staging some of the changes)", + BaseCommitIsAlreadyOnMainBranch: "The base commit for this change is already on the main branch", + BaseCommitIsNotInCurrentView: "Base commit is not in current view", StatusTitle: "Status", Menu: "Menu", Execute: "Execute", diff --git a/pkg/integration/tests/commit/find_base_commit_for_fixup.go b/pkg/integration/tests/commit/find_base_commit_for_fixup.go new file mode 100644 index 00000000000..4440932e909 --- /dev/null +++ b/pkg/integration/tests/commit/find_base_commit_for_fixup.go @@ -0,0 +1,79 @@ +package commit + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var FindBaseCommitForFixup = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Finds the base commit to create a fixup for", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.NewBranch("mybranch"). + EmptyCommit("1st commit"). + CreateFileAndAdd("file1", "file1 content\n"). + Commit("2nd commit"). + CreateFileAndAdd("file2", "file2 content\n"). + Commit("3rd commit"). + UpdateFile("file1", "file1 changed content"). + UpdateFile("file2", "file2 changed content") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Lines( + Contains("3rd commit"), + Contains("2nd commit"), + Contains("1st commit"), + ) + + // Two changes from different commits: this fails + t.Views().Files(). + Focus(). + Press(keys.Files.FindBaseCommitForFixup) + + t.ExpectPopup().Alert(). + Title(Equals("Error")). + Content( + Contains("Multiple base commits found"). + Contains("2nd commit"). + Contains("3rd commit"), + ). + Confirm() + + // Stage only one of the files: this succeeds + t.Views().Files(). + IsFocused(). + NavigateToLine(Contains("file1")). + PressPrimaryAction(). + Press(keys.Files.FindBaseCommitForFixup) + + t.Views().Commits(). + IsFocused(). + Lines( + Contains("3rd commit"), + Contains("2nd commit").IsSelected(), + Contains("1st commit"), + ). + Press(keys.Commits.AmendToCommit) + + t.ExpectPopup().Confirmation(). + Title(Equals("Amend commit")). + Content(Contains("Are you sure you want to amend this commit with your staged files?")). + Confirm() + + // Now only the other file is modified (and unstaged); this works now + t.Views().Files(). + Focus(). + Press(keys.Files.FindBaseCommitForFixup) + + t.Views().Commits(). + IsFocused(). + Lines( + Contains("3rd commit").IsSelected(), + Contains("2nd commit"), + Contains("1st commit"), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index ccefd1dd23e..c018d84e78c 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -70,6 +70,7 @@ var tests = []*components.IntegrationTest{ commit.CommitWithPrefix, commit.CreateTag, commit.DiscardOldFileChange, + commit.FindBaseCommitForFixup, commit.Highlight, commit.History, commit.HistoryComplex, diff --git a/schema/config.json b/schema/config.json index a5cbc0b6726..1251240a1d5 100644 --- a/schema/config.json +++ b/schema/config.json @@ -914,6 +914,10 @@ "type": "string", "default": "C" }, + "findBaseCommitForFixup": { + "type": "string", + "default": "\u003cc-f\u003e" + }, "confirmDiscard": { "type": "string", "default": "x" From b35f8776e1044414071d828010d2314665b81d82 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 7 Jan 2024 16:22:46 +0100 Subject: [PATCH 2/2] Warn when there are hunks with only added lines The algorithm works by blaming the deleted lines, so if a hunk contains only added lines, we can only hope that it also belongs in the same commit. Warn the user about this. Note: the warning might be overly agressive, we'll have to see if this is annoying. The reason is that it depends on the diff context size whether added lines go into their own hunk or are grouped together with other added or deleted lines into one hunk. However, our algorithm uses a diff context size of 0, because that makes it easiest to parse the diff; this results in hunks having only added lines more often than what the user sees. For example, moving a line of code down by two lines will likely result in a single hunk for the user, but in two hunks for our algorithm. On the other hand, being this strict makes the warning consistent. We could consider using the user's diff context size in the algorithm, but then it would depend on the current context size whether the warning appears, which could be confusing. Plus, it would make the algorithm quite a bit more complicated. --- pkg/gui/controllers/helpers/fixup_helper.go | 41 +++++++++++----- pkg/i18n/english.go | 2 + ...ommit_for_fixup_warning_for_added_lines.go | 48 +++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + 4 files changed, 81 insertions(+), 11 deletions(-) create mode 100644 pkg/integration/tests/commit/find_base_commit_for_fixup_warning_for_added_lines.go diff --git a/pkg/gui/controllers/helpers/fixup_helper.go b/pkg/gui/controllers/helpers/fixup_helper.go index 2198d11cb2b..35c8233b8ca 100644 --- a/pkg/gui/controllers/helpers/fixup_helper.go +++ b/pkg/gui/controllers/helpers/fixup_helper.go @@ -39,7 +39,7 @@ func (self *FixupHelper) HandleFindBaseCommitForFixupPress() error { return self.c.ErrorMsg(self.c.Tr.NoChangedFiles) } - deletedLineInfos := self.parseDiff(diff) + deletedLineInfos, hasHunksWithOnlyAddedLines := self.parseDiff(diff) if len(deletedLineInfos) == 0 { return self.c.ErrorMsg(self.c.Tr.NoDeletedLinesInDiff) } @@ -79,15 +79,29 @@ func (self *FixupHelper) HandleFindBaseCommitForFixupPress() error { return self.c.ErrorMsg(self.c.Tr.BaseCommitIsAlreadyOnMainBranch) } - if !useIndex { - if err := self.c.Git().WorkingTree.StageAll(); err != nil { - return err + doIt := func() error { + if !hasStagedChanges { + if err := self.c.Git().WorkingTree.StageAll(); err != nil { + return err + } + _ = self.c.Refresh(types.RefreshOptions{Mode: types.SYNC, Scope: []types.RefreshableView{types.FILES}}) } - _ = self.c.Refresh(types.RefreshOptions{Mode: types.SYNC, Scope: []types.RefreshableView{types.FILES}}) + + self.c.Contexts().LocalCommits.SetSelectedLineIdx(index) + return self.c.PushContext(self.c.Contexts().LocalCommits) } - self.c.Contexts().LocalCommits.SetSelectedLineIdx(index) - return self.c.PushContext(self.c.Contexts().LocalCommits) + if hasHunksWithOnlyAddedLines { + return self.c.Confirm(types.ConfirmOpts{ + Title: self.c.Tr.FindBaseCommitForFixup, + Prompt: self.c.Tr.HunksWithOnlyAddedLinesWarning, + HandleConfirm: func() error { + return doIt() + }, + }) + } + + return doIt() } func (self *FixupHelper) getDiff() (string, bool, error) { @@ -106,18 +120,23 @@ func (self *FixupHelper) getDiff() (string, bool, error) { return diff, hasStagedChanges, err } -func (self *FixupHelper) parseDiff(diff string) []*deletedLineInfo { +func (self *FixupHelper) parseDiff(diff string) ([]*deletedLineInfo, bool) { lines := strings.Split(strings.TrimSuffix(diff, "\n"), "\n") deletedLineInfos := []*deletedLineInfo{} + hasHunksWithOnlyAddedLines := false hunkHeaderRegexp := regexp.MustCompile(`@@ -(\d+)(?:,\d+)? \+\d+(?:,\d+)? @@`) var filename string var currentLineInfo *deletedLineInfo finishHunk := func() { - if currentLineInfo != nil && currentLineInfo.numLines > 0 { - deletedLineInfos = append(deletedLineInfos, currentLineInfo) + if currentLineInfo != nil { + if currentLineInfo.numLines > 0 { + deletedLineInfos = append(deletedLineInfos, currentLineInfo) + } else { + hasHunksWithOnlyAddedLines = true + } } } for _, line := range lines { @@ -139,7 +158,7 @@ func (self *FixupHelper) parseDiff(diff string) []*deletedLineInfo { } finishHunk() - return deletedLineInfos + return deletedLineInfos, hasHunksWithOnlyAddedLines } // returns the list of commit hashes that introduced the lines which have now been deleted diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go index e2559f3d21a..b645291e701 100644 --- a/pkg/i18n/english.go +++ b/pkg/i18n/english.go @@ -47,6 +47,7 @@ type TranslationSet struct { MultipleBaseCommitsFoundUnstaged string BaseCommitIsAlreadyOnMainBranch string BaseCommitIsNotInCurrentView string + HunksWithOnlyAddedLinesWarning string StatusTitle string GlobalTitle string Menu string @@ -874,6 +875,7 @@ func EnglishTranslationSet() TranslationSet { MultipleBaseCommitsFoundUnstaged: "Multiple base commits found. (Try staging some of the changes)", BaseCommitIsAlreadyOnMainBranch: "The base commit for this change is already on the main branch", BaseCommitIsNotInCurrentView: "Base commit is not in current view", + HunksWithOnlyAddedLinesWarning: "There are ranges of only added lines in the diff; be careful to check that these belong in the found base commit.\n\nProceed?", StatusTitle: "Status", Menu: "Menu", Execute: "Execute", diff --git a/pkg/integration/tests/commit/find_base_commit_for_fixup_warning_for_added_lines.go b/pkg/integration/tests/commit/find_base_commit_for_fixup_warning_for_added_lines.go new file mode 100644 index 00000000000..315b757dbdf --- /dev/null +++ b/pkg/integration/tests/commit/find_base_commit_for_fixup_warning_for_added_lines.go @@ -0,0 +1,48 @@ +package commit + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var FindBaseCommitForFixupWarningForAddedLines = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Finds the base commit to create a fixup for, and warns that there are hunks with only added lines", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.NewBranch("mybranch"). + EmptyCommit("1st commit"). + CreateFileAndAdd("file1", "file1 content\n"). + Commit("2nd commit"). + CreateFileAndAdd("file2", "file2 content\n"). + Commit("3rd commit"). + UpdateFile("file1", "file1 changed content"). + UpdateFile("file2", "file2 content\nadded content") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Lines( + Contains("3rd commit").IsSelected(), + Contains("2nd commit"), + Contains("1st commit"), + ) + + t.Views().Files(). + Focus(). + Press(keys.Files.FindBaseCommitForFixup) + + t.ExpectPopup().Confirmation(). + Title(Equals("Find base commit for fixup")). + Content(Contains("There are ranges of only added lines in the diff; be careful to check that these belong in the found base commit.")). + Confirm() + + t.Views().Commits(). + IsFocused(). + Lines( + Contains("3rd commit"), + Contains("2nd commit").IsSelected(), + Contains("1st commit"), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index c018d84e78c..1b6ab75e982 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -71,6 +71,7 @@ var tests = []*components.IntegrationTest{ commit.CreateTag, commit.DiscardOldFileChange, commit.FindBaseCommitForFixup, + commit.FindBaseCommitForFixupWarningForAddedLines, commit.Highlight, commit.History, commit.HistoryComplex,