From 8515c74722a15c6205c7c3f26b7da6e139d28a9e Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 16 Jul 2025 13:14:08 +0200 Subject: [PATCH 01/11] Add test that demonstrates problem with showing filtered history of file with renames The test shows that selecting a commit before the rename shows an empty diff, and selecting the rename itself shows an added file rather than a rename. --- pkg/integration/components/shell.go | 4 + .../show_diffs_for_renamed_file.go | 128 ++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + 3 files changed, 133 insertions(+) create mode 100644 pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go diff --git a/pkg/integration/components/shell.go b/pkg/integration/components/shell.go index 6b711e5a8..4c833f2ab 100644 --- a/pkg/integration/components/shell.go +++ b/pkg/integration/components/shell.go @@ -235,6 +235,10 @@ func (self *Shell) DeleteFileAndAdd(fileName string) *Shell { GitAdd(fileName) } +func (self *Shell) RenameFileInGit(oldName string, newName string) *Shell { + return self.RunCommand([]string{"git", "mv", oldName, newName}) +} + // creates commits 01, 02, 03, ..., n with a new file in each // The reason for padding with zeroes is so that it's easier to do string // matches on the commit messages when there are many of them diff --git a/pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go b/pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go new file mode 100644 index 000000000..3d91e391c --- /dev/null +++ b/pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go @@ -0,0 +1,128 @@ +package filter_by_path + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var ShowDiffsForRenamedFile = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Filter commits by file path for a file that was renamed, and verify that it shows the diffs correctly", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) { + }, + SetupRepo: func(shell *Shell) { + shell.CreateFileAndAdd("oldFile", "a\nb\nc\n") + shell.Commit("add old file") + shell.UpdateFileAndAdd("oldFile", "x\nb\nc\n") + shell.Commit("update old file") + shell.CreateFileAndAdd("unrelatedFile", "content of unrelated file\n") + shell.Commit("add unrelated file") + shell.RenameFileInGit("oldFile", "newFile") + shell.Commit("rename file") + shell.UpdateFileAndAdd("newFile", "y\nb\nc\n") + shell.UpdateFileAndAdd("unrelatedFile", "updated content of unrelated file\n") + shell.Commit("update both files") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + Lines( + Contains("update both files").IsSelected(), + Contains("rename file"), + Contains("add unrelated file"), + Contains("update old file"), + Contains("add old file"), + ). + PressEnter() + + t.Views().CommitFiles(). + IsFocused(). + Lines( + Equals("▼ /").IsSelected(), + Equals(" M newFile"), + Equals(" M unrelatedFile"), + ). + SelectNextItem(). + Press(keys.Universal.FilteringMenu) + + t.ExpectPopup().Menu().Title(Equals("Filtering")). + Select(Contains("Filter by 'newFile'")).Confirm() + + t.Views().Commits(). + IsFocused(). + Lines( + Contains("update both files").IsSelected(), + Contains("rename file"), + Contains("update old file"), + Contains("add old file"), + ) + + t.Views().Main().ContainsLines( + Equals(" update both files"), + Equals("---"), + Equals(" newFile | 2 +-"), + Equals(" 1 file changed, 1 insertion(+), 1 deletion(-)"), + Equals(""), + Equals("diff --git a/newFile b/newFile"), + Contains("index"), + Equals("--- a/newFile"), + Equals("+++ b/newFile"), + Equals("@@ -1,3 +1,3 @@"), + Equals("-x"), + Equals("+y"), + Equals(" b"), + Equals(" c"), + ) + + t.Views().Commits().SelectNextItem() + + t.Views().Main().ContainsLines( + Equals(" rename file"), + Equals("---"), + /* EXPECTED: + Equals(" oldFile => newFile | 0"), + Equals(" 1 file changed, 0 insertions(+), 0 deletions(-)"), + Equals(""), + Equals("diff --git a/oldFile b/newFile"), + Equals("similarity index 100%"), + Equals("rename from oldFile"), + Equals("rename to newFile"), + ACTUAL: */ + Equals(" newFile | 3 +++"), + Equals(" 1 file changed, 3 insertions(+)"), + Equals(""), + Equals("diff --git a/newFile b/newFile"), + Equals("new file mode 100644"), + Contains("index"), + Equals("--- /dev/null"), + Equals("+++ b/newFile"), + Equals("@@ -0,0 +1,3 @@"), + Equals("+x"), + Equals("+b"), + Equals("+c"), + ) + + t.Views().Commits().SelectNextItem() + + /* EXPECTED: + t.Views().Main().ContainsLines( + Equals(" update old file"), + Equals("---"), + Equals(" oldFile | 2 +-"), + Equals(" 1 file changed, 1 insertion(+), 1 deletion(-)"), + Equals(""), + Equals("diff --git a/oldFile b/oldFile"), + Contains("index"), + Equals("--- a/oldFile"), + Equals("+++ b/oldFile"), + Equals("@@ -1,3 +1,3 @@"), + Equals("-a"), + Equals("+x"), + Equals(" b"), + Equals(" c"), + ) + ACTUAL: */ + t.Views().Main().IsEmpty() + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 4a8754284..1c5f5d75f 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -235,6 +235,7 @@ var tests = []*components.IntegrationTest{ filter_by_path.CliArg, filter_by_path.KeepSameCommitSelectedOnExit, filter_by_path.SelectFile, + filter_by_path.ShowDiffsForRenamedFile, filter_by_path.TypeFile, interactive_rebase.AdvancedInteractiveRebase, interactive_rebase.AmendCommitWithConflict, From 92b5bad29d889945b5d2f63adc687723dbdcba4a Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 11 Jul 2025 17:19:14 +0200 Subject: [PATCH 02/11] Cleanup: simplify git arguments for reflog loading The combination of --abbrev=40 and %h can be shortened to %H. --- pkg/commands/git_commands/reflog_commit_loader.go | 3 +-- .../git_commands/reflog_commit_loader_test.go | 12 ++++++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/pkg/commands/git_commands/reflog_commit_loader.go b/pkg/commands/git_commands/reflog_commit_loader.go index 1e981b1e1..036f74504 100644 --- a/pkg/commands/git_commands/reflog_commit_loader.go +++ b/pkg/commands/git_commands/reflog_commit_loader.go @@ -30,8 +30,7 @@ func (self *ReflogCommitLoader) GetReflogCommits(hashPool *utils.StringPool, las cmdArgs := NewGitCmd("log"). Config("log.showSignature=false"). Arg("-g"). - Arg("--abbrev=40"). - Arg("--format=%h%x00%ct%x00%gs%x00%P"). + Arg("--format=%H%x00%ct%x00%gs%x00%P"). ArgIf(filterAuthor != "", "--author="+filterAuthor). ArgIf(filterPath != "", "--follow", "--", filterPath). ToArgv() diff --git a/pkg/commands/git_commands/reflog_commit_loader_test.go b/pkg/commands/git_commands/reflog_commit_loader_test.go index 01e479e5c..468f6ca06 100644 --- a/pkg/commands/git_commands/reflog_commit_loader_test.go +++ b/pkg/commands/git_commands/reflog_commit_loader_test.go @@ -39,7 +39,7 @@ func TestGetReflogCommits(t *testing.T) { { testName: "no reflog entries", runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--abbrev=40", "--format=%h%x00%ct%x00%gs%x00%P"}, "", nil), + ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=%H%x00%ct%x00%gs%x00%P"}, "", nil), lastReflogCommit: nil, expectedCommitOpts: []models.NewCommitOpts{}, @@ -49,7 +49,7 @@ func TestGetReflogCommits(t *testing.T) { { testName: "some reflog entries", runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--abbrev=40", "--format=%h%x00%ct%x00%gs%x00%P"}, reflogOutput, nil), + ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=%H%x00%ct%x00%gs%x00%P"}, reflogOutput, nil), lastReflogCommit: nil, expectedCommitOpts: []models.NewCommitOpts{ @@ -95,7 +95,7 @@ func TestGetReflogCommits(t *testing.T) { { testName: "some reflog entries where last commit is given", runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--abbrev=40", "--format=%h%x00%ct%x00%gs%x00%P"}, reflogOutput, nil), + ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=%H%x00%ct%x00%gs%x00%P"}, reflogOutput, nil), lastReflogCommit: models.NewCommit(hashPool, models.NewCommitOpts{ Hash: "c3c4b66b64c97ffeecde", @@ -119,7 +119,7 @@ func TestGetReflogCommits(t *testing.T) { { testName: "when passing filterPath", runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--abbrev=40", "--format=%h%x00%ct%x00%gs%x00%P", "--follow", "--", "path"}, reflogOutput, nil), + ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=%H%x00%ct%x00%gs%x00%P", "--follow", "--", "path"}, reflogOutput, nil), lastReflogCommit: models.NewCommit(hashPool, models.NewCommitOpts{ Hash: "c3c4b66b64c97ffeecde", @@ -144,7 +144,7 @@ func TestGetReflogCommits(t *testing.T) { { testName: "when passing filterAuthor", runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--abbrev=40", "--format=%h%x00%ct%x00%gs%x00%P", "--author=John Doe "}, reflogOutput, nil), + ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=%H%x00%ct%x00%gs%x00%P", "--author=John Doe "}, reflogOutput, nil), lastReflogCommit: models.NewCommit(hashPool, models.NewCommitOpts{ Hash: "c3c4b66b64c97ffeecde", @@ -169,7 +169,7 @@ func TestGetReflogCommits(t *testing.T) { { testName: "when command returns error", runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--abbrev=40", "--format=%h%x00%ct%x00%gs%x00%P"}, "", errors.New("haha")), + ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=%H%x00%ct%x00%gs%x00%P"}, "", errors.New("haha")), lastReflogCommit: nil, filterPath: "", From d99ceb91eefa424b6e3831b5c65011b0b5a41eee Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 11 Jul 2025 18:05:24 +0200 Subject: [PATCH 03/11] Don't get reflog commits twice unnecessarily in filtering mode I can only guess what happened here: in aa750c08192d, this code to manually load the reflog commits was added, to make sorting branches by recency work when the reflog is filtered by path. At that time we didn't have separate ReflogCommits and FilteredReflogCommits models yet. Then, FilteredReflogCommits was introduced (in 8822c409e24), probably for the very purpose of being able to get rid of this again; but then it was forgotton to actually get rid of it. The funny thing is that the introduction of FilteredReflogCommits happened in the very next commit, 15 minutes later. --- pkg/gui/controllers/helpers/refresh_helper.go | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index dc396be3f..7c27225e5 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -449,21 +449,8 @@ func (self *RefreshHelper) refreshBranches(refreshWorktrees bool, keepBranchSele self.c.Mutexes().RefreshingBranchesMutex.Lock() defer self.c.Mutexes().RefreshingBranchesMutex.Unlock() - reflogCommits := self.c.Model().FilteredReflogCommits - if self.c.Modes().Filtering.Active() && self.c.UserConfig().Git.LocalBranchSortOrder == "recency" { - // in filter mode we filter our reflog commits to just those containing the path - // however we need all the reflog entries to populate the recencies of our branches - // which allows us to order them correctly. So if we're filtering we'll just - // manually load all the reflog commits here - var err error - reflogCommits, _, err = self.c.Git().Loaders.ReflogCommitLoader.GetReflogCommits(self.c.Model().HashPool, nil, "", "") - if err != nil { - self.c.Log.Error(err) - } - } - branches, err := self.c.Git().Loaders.BranchLoader.Load( - reflogCommits, + self.c.Model().ReflogCommits, self.c.Model().MainBranches, self.c.Model().Branches, loadBehindCounts, From 934276ac401cd51bd05b750d1b6f27ba94dd393a Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 10 Jul 2025 14:00:01 +0200 Subject: [PATCH 04/11] Use the non-filtered reflog for undoing Using the filtered one is probably not a good idea. It didn't do much harm because the split of ReflogCommits and FilteredReflogCommits doesn't really work right now (FilteredReflogCommits is always the same as ReflogCommits, even in filtering mode), but we'll fix this in the next commit. --- pkg/gui/controllers/undo_controller.go | 2 +- pkg/gui/types/common.go | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/gui/controllers/undo_controller.go b/pkg/gui/controllers/undo_controller.go index ecff5bea8..cd3b04e4f 100644 --- a/pkg/gui/controllers/undo_controller.go +++ b/pkg/gui/controllers/undo_controller.go @@ -199,7 +199,7 @@ func (self *UndoController) reflogRedo() error { // Though we might support this later, hence the use of the CURRENT_REBASE action kind. func (self *UndoController) parseReflogForActions(onUserAction func(counter int, action reflogAction) (bool, error)) error { counter := 0 - reflogCommits := self.c.Model().FilteredReflogCommits + reflogCommits := self.c.Model().ReflogCommits rebaseFinishCommitHash := "" var action *reflogAction for reflogCommitIdx, reflogCommit := range reflogCommits { diff --git a/pkg/gui/types/common.go b/pkg/gui/types/common.go index bf7267a8c..7d8528ee8 100644 --- a/pkg/gui/types/common.go +++ b/pkg/gui/types/common.go @@ -289,10 +289,11 @@ type Model struct { Worktrees []*models.Worktree // FilteredReflogCommits are the ones that appear in the reflog panel. - // when in filtering mode we only include the ones that match the given path + // When in filtering mode we only include the ones that match the given path FilteredReflogCommits []*models.Commit - // ReflogCommits are the ones used by the branches panel to obtain recency values - // if we're not in filtering mode, CommitFiles and FilteredReflogCommits will be + // ReflogCommits are the ones used by the branches panel to obtain recency values, + // and for the undo functionality. + // If we're not in filtering mode, CommitFiles and FilteredReflogCommits will be // one and the same ReflogCommits []*models.Commit From e1d728ee5ebde56ed165c0f5b3f1b6520d77334a Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 11 Jul 2025 17:56:28 +0200 Subject: [PATCH 05/11] Fix showing only filtered reflog entries when filtering by path Recycle reflog commits only for the non-filtered ones. If we're not filtering, FilteredReflogCommits and ReflogCommits are the same. If we then enter filtering and get reflog entries again, and pass a lastReflogCommit, we'd recycle the previous FilteredReflogCommits, which are unfiltered, so that's no good. Work around this by simply never using the recycle mechanism when getting the filtered reflog commits. We could do better by remembering what the last filter path or author was, and only suppressing the recycling when it changed; but that's more complexity than I want to add, so hopefully this is good enough. --- pkg/gui/controllers/helpers/refresh_helper.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 7c27225e5..24958c65d 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -607,12 +607,13 @@ func (self *RefreshHelper) refreshReflogCommits() error { // pulling state into its own variable in case it gets swapped out for another state // and we get an out of bounds exception model := self.c.Model() - var lastReflogCommit *models.Commit - if len(model.ReflogCommits) > 0 { - lastReflogCommit = model.ReflogCommits[0] - } refresh := func(stateCommits *[]*models.Commit, filterPath string, filterAuthor string) error { + var lastReflogCommit *models.Commit + if filterPath == "" && filterAuthor == "" && len(*stateCommits) > 0 { + lastReflogCommit = (*stateCommits)[0] + } + commits, onlyObtainedNewReflogCommits, err := self.c.Git().Loaders.ReflogCommitLoader. GetReflogCommits(self.c.Model().HashPool, lastReflogCommit, filterPath, filterAuthor) if err != nil { From 33a4fdf0eed3bbe1c1a710a0d86034e2bb950920 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 10 Jul 2025 13:59:52 +0200 Subject: [PATCH 06/11] Remove unnecessary setSubCommits indirection I don't know why this function argument was added, but I don't like unnecessary indirections, so I'm removing it as SubCommitsHelper has access to everything it needs to do it itself. --- pkg/gui/controllers.go | 8 +------- pkg/gui/controllers/helpers/sub_commits_helper.go | 10 +++++++--- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/pkg/gui/controllers.go b/pkg/gui/controllers.go index dec0ab23a..ba116b9ed 100644 --- a/pkg/gui/controllers.go +++ b/pkg/gui/controllers.go @@ -90,12 +90,6 @@ func (gui *Gui) resetHelpersAndControllers() { modeHelper, ) - setSubCommits := func(commits []*models.Commit) { - gui.Mutexes.SubCommitsMutex.Lock() - defer gui.Mutexes.SubCommitsMutex.Unlock() - - gui.State.Model.SubCommits = commits - } gui.helpers = &helpers.Helpers{ Refs: refsHelper, Host: helpers.NewHostHelper(helperCommon), @@ -135,7 +129,7 @@ func (gui *Gui) resetHelpersAndControllers() { ), Search: searchHelper, Worktree: worktreeHelper, - SubCommits: helpers.NewSubCommitsHelper(helperCommon, refreshHelper, setSubCommits), + SubCommits: helpers.NewSubCommitsHelper(helperCommon, refreshHelper), } gui.CustomCommandsClient = custom_commands.NewClient( diff --git a/pkg/gui/controllers/helpers/sub_commits_helper.go b/pkg/gui/controllers/helpers/sub_commits_helper.go index cffd8e8ed..e61c4fee6 100644 --- a/pkg/gui/controllers/helpers/sub_commits_helper.go +++ b/pkg/gui/controllers/helpers/sub_commits_helper.go @@ -11,18 +11,15 @@ type SubCommitsHelper struct { c *HelperCommon refreshHelper *RefreshHelper - setSubCommits func([]*models.Commit) } func NewSubCommitsHelper( c *HelperCommon, refreshHelper *RefreshHelper, - setSubCommits func([]*models.Commit), ) *SubCommitsHelper { return &SubCommitsHelper{ c: c, refreshHelper: refreshHelper, - setSubCommits: setSubCommits, } } @@ -73,3 +70,10 @@ func (self *SubCommitsHelper) ViewSubCommits(opts ViewSubCommitsOpts) error { self.c.Context().Push(self.c.Contexts().SubCommits, types.OnFocusOpts{}) return nil } + +func (self *SubCommitsHelper) setSubCommits(commits []*models.Commit) { + self.c.Mutexes().SubCommitsMutex.Lock() + defer self.c.Mutexes().SubCommitsMutex.Unlock() + + self.c.Model().SubCommits = commits +} From e7c33a7a65ecafd323bd522a607378b7f9062cc5 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sun, 1 Jun 2025 20:25:05 +0200 Subject: [PATCH 07/11] Always append -- to git show command It is good practice to use a -- argument even if no further arguments follow. Doesn't really make a difference for this particular command though, I think. --- pkg/commands/git_commands/commit.go | 3 ++- pkg/commands/git_commands/commit_test.go | 10 +++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/commands/git_commands/commit.go b/pkg/commands/git_commands/commit.go index 11b75c8f0..2927a3038 100644 --- a/pkg/commands/git_commands/commit.go +++ b/pkg/commands/git_commands/commit.go @@ -270,7 +270,8 @@ func (self *CommitCommands) ShowCmdObj(hash string, filterPath string) *oscomman Arg(hash). ArgIf(self.UserConfig().Git.IgnoreWhitespaceInDiffView, "--ignore-all-space"). Arg(fmt.Sprintf("--find-renames=%d%%", self.UserConfig().Git.RenameSimilarityThreshold)). - ArgIf(filterPath != "", "--", filterPath). + Arg("--"). + ArgIf(filterPath != "", filterPath). Dir(self.repoPaths.worktreePath). ToArgv() diff --git a/pkg/commands/git_commands/commit_test.go b/pkg/commands/git_commands/commit_test.go index 112cf6b77..6abe12128 100644 --- a/pkg/commands/git_commands/commit_test.go +++ b/pkg/commands/git_commands/commit_test.go @@ -267,7 +267,7 @@ func TestCommitShowCmdObj(t *testing.T) { similarityThreshold: 50, ignoreWhitespace: false, extDiffCmd: "", - expected: []string{"-C", "/path/to/worktree", "-c", "diff.noprefix=false", "show", "--no-ext-diff", "--submodule", "--color=always", "--unified=3", "--stat", "--decorate", "-p", "1234567890", "--find-renames=50%"}, + expected: []string{"-C", "/path/to/worktree", "-c", "diff.noprefix=false", "show", "--no-ext-diff", "--submodule", "--color=always", "--unified=3", "--stat", "--decorate", "-p", "1234567890", "--find-renames=50%", "--"}, }, { testName: "Default case with filter path", @@ -285,7 +285,7 @@ func TestCommitShowCmdObj(t *testing.T) { similarityThreshold: 50, ignoreWhitespace: false, extDiffCmd: "", - expected: []string{"-C", "/path/to/worktree", "-c", "diff.noprefix=false", "show", "--no-ext-diff", "--submodule", "--color=always", "--unified=77", "--stat", "--decorate", "-p", "1234567890", "--find-renames=50%"}, + expected: []string{"-C", "/path/to/worktree", "-c", "diff.noprefix=false", "show", "--no-ext-diff", "--submodule", "--color=always", "--unified=77", "--stat", "--decorate", "-p", "1234567890", "--find-renames=50%", "--"}, }, { testName: "Show diff with custom similarity threshold", @@ -294,7 +294,7 @@ func TestCommitShowCmdObj(t *testing.T) { similarityThreshold: 33, ignoreWhitespace: false, extDiffCmd: "", - expected: []string{"-C", "/path/to/worktree", "-c", "diff.noprefix=false", "show", "--no-ext-diff", "--submodule", "--color=always", "--unified=3", "--stat", "--decorate", "-p", "1234567890", "--find-renames=33%"}, + expected: []string{"-C", "/path/to/worktree", "-c", "diff.noprefix=false", "show", "--no-ext-diff", "--submodule", "--color=always", "--unified=3", "--stat", "--decorate", "-p", "1234567890", "--find-renames=33%", "--"}, }, { testName: "Show diff, ignoring whitespace", @@ -303,7 +303,7 @@ func TestCommitShowCmdObj(t *testing.T) { similarityThreshold: 50, ignoreWhitespace: true, extDiffCmd: "", - expected: []string{"-C", "/path/to/worktree", "-c", "diff.noprefix=false", "show", "--no-ext-diff", "--submodule", "--color=always", "--unified=77", "--stat", "--decorate", "-p", "1234567890", "--ignore-all-space", "--find-renames=50%"}, + expected: []string{"-C", "/path/to/worktree", "-c", "diff.noprefix=false", "show", "--no-ext-diff", "--submodule", "--color=always", "--unified=77", "--stat", "--decorate", "-p", "1234567890", "--ignore-all-space", "--find-renames=50%", "--"}, }, { testName: "Show diff with external diff command", @@ -312,7 +312,7 @@ func TestCommitShowCmdObj(t *testing.T) { similarityThreshold: 50, ignoreWhitespace: false, extDiffCmd: "difft --color=always", - expected: []string{"-C", "/path/to/worktree", "-c", "diff.external=difft --color=always", "-c", "diff.noprefix=false", "show", "--ext-diff", "--submodule", "--color=always", "--unified=3", "--stat", "--decorate", "-p", "1234567890", "--find-renames=50%"}, + expected: []string{"-C", "/path/to/worktree", "-c", "diff.external=difft --color=always", "-c", "diff.noprefix=false", "show", "--ext-diff", "--submodule", "--color=always", "--unified=3", "--stat", "--decorate", "-p", "1234567890", "--find-renames=50%", "--"}, }, } From 88dae1d8b955b593fac909ec726cd67ed65c509d Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Wed, 16 Jul 2025 18:25:22 +0200 Subject: [PATCH 08/11] Refactor commit loading and reflog commit loading to extract shared code These are very similar in that they call RunAndProcessLines on a git log command and then process lines to create commits. Extract this into a common helper function. At the moment this doesn't gain us much, but in the next commit we will extend this helper function considerably with filter path logic, which would otherwise have to be duplicated in both places. --- pkg/commands/git_commands/commit_loader.go | 10 ++++---- .../git_commands/commit_loading_shared.go | 23 +++++++++++++++++++ .../git_commands/reflog_commit_loader.go | 12 ++++------ 3 files changed, 34 insertions(+), 11 deletions(-) create mode 100644 pkg/commands/git_commands/commit_loading_shared.go diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index 7cdac4c33..9fff74cdc 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -89,11 +89,13 @@ func (self *CommitLoader) GetCommits(opts GetCommitsOptions) ([]*models.Commit, go utils.Safe(func() { defer wg.Done() - logErr = self.getLogCmd(opts).RunAndProcessLines(func(line string) (bool, error) { - commit := self.extractCommitFromLine(opts.HashPool, line, opts.RefToShowDivergenceFrom != "") - commits = append(commits, commit) - return false, nil + var realCommits []*models.Commit + realCommits, logErr = loadCommits(self.getLogCmd(opts), func(line string) (*models.Commit, bool) { + return self.extractCommitFromLine(opts.HashPool, line, opts.RefToShowDivergenceFrom != ""), false }) + if logErr == nil { + commits = append(commits, realCommits...) + } }) var ancestor string diff --git a/pkg/commands/git_commands/commit_loading_shared.go b/pkg/commands/git_commands/commit_loading_shared.go new file mode 100644 index 000000000..9bf774186 --- /dev/null +++ b/pkg/commands/git_commands/commit_loading_shared.go @@ -0,0 +1,23 @@ +package git_commands + +import ( + "github.com/jesseduffield/lazygit/pkg/commands/models" + "github.com/jesseduffield/lazygit/pkg/commands/oscommands" +) + +func loadCommits( + cmd *oscommands.CmdObj, + parseLogLine func(string) (*models.Commit, bool), +) ([]*models.Commit, error) { + commits := []*models.Commit{} + + err := cmd.RunAndProcessLines(func(line string) (bool, error) { + commit, stop := parseLogLine(line) + if stop { + return true, nil + } + commits = append(commits, commit) + return false, nil + }) + return commits, err +} diff --git a/pkg/commands/git_commands/reflog_commit_loader.go b/pkg/commands/git_commands/reflog_commit_loader.go index 036f74504..da0d1c3e0 100644 --- a/pkg/commands/git_commands/reflog_commit_loader.go +++ b/pkg/commands/git_commands/reflog_commit_loader.go @@ -25,8 +25,6 @@ func NewReflogCommitLoader(common *common.Common, cmd oscommands.ICmdObjBuilder) // GetReflogCommits only returns the new reflog commits since the given lastReflogCommit // if none is passed (i.e. it's value is nil) then we get all the reflog commits func (self *ReflogCommitLoader) GetReflogCommits(hashPool *utils.StringPool, lastReflogCommit *models.Commit, filterPath string, filterAuthor string) ([]*models.Commit, bool, error) { - commits := make([]*models.Commit, 0) - cmdArgs := NewGitCmd("log"). Config("log.showSignature=false"). Arg("-g"). @@ -38,10 +36,11 @@ func (self *ReflogCommitLoader) GetReflogCommits(hashPool *utils.StringPool, las cmdObj := self.cmd.New(cmdArgs).DontLog() onlyObtainedNewReflogCommits := false - err := cmdObj.RunAndProcessLines(func(line string) (bool, error) { + + commits, err := loadCommits(cmdObj, func(line string) (*models.Commit, bool) { commit, ok := self.parseLine(hashPool, line) if !ok { - return false, nil + return nil, false } // note that the unix timestamp here is the timestamp of the COMMIT, not the reflog entry itself, @@ -51,11 +50,10 @@ func (self *ReflogCommitLoader) GetReflogCommits(hashPool *utils.StringPool, las if lastReflogCommit != nil && self.sameReflogCommit(commit, lastReflogCommit) { onlyObtainedNewReflogCommits = true // after this point we already have these reflogs loaded so we'll simply return the new ones - return true, nil + return nil, true } - commits = append(commits, commit) - return false, nil + return commit, false }) if err != nil { return nil, false, err From 0f7f1a56df4b0785d2c202182e0d97443ecf51a0 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 30 May 2025 10:08:13 +0200 Subject: [PATCH 09/11] Fix showing diffs for renamed file when filtering by path When filtering for a file path we use the --follow option for "git log", so it will follow renames of the file, which is great. However, if you then selected one of the commits before a rename, you didn't see a diff, because we would pass the original filter path to the "git show" call. To fix this, call git log with the --name-status option when filtering by path, so that each commit reports which file paths are touched in this commit; remember these in the commit object, so that we can pass them to the "git show" call later. Be careful not to store too many such paths unnecessarily. When filtering by folder rather than file, all these paths will necessarily be inside the original filter path, so detect this and don't store them in this case. There is some unfortunate code duplication between loading commits and loading reflog commits, which I am too lazy to clean up right now. --- pkg/commands/git_commands/commit.go | 4 +- pkg/commands/git_commands/commit_loader.go | 11 ++-- .../git_commands/commit_loader_test.go | 32 +++++----- .../git_commands/commit_loading_shared.go | 61 +++++++++++++++++-- pkg/commands/git_commands/commit_test.go | 16 ++--- .../git_commands/reflog_commit_loader.go | 6 +- .../git_commands/reflog_commit_loader_test.go | 22 +++---- pkg/commands/models/commit.go | 4 ++ pkg/gui/controllers/helpers/diff_helper.go | 13 +++- .../controllers/reflog_commits_controller.go | 2 +- .../show_diffs_for_renamed_file.go | 17 ------ 11 files changed, 120 insertions(+), 68 deletions(-) diff --git a/pkg/commands/git_commands/commit.go b/pkg/commands/git_commands/commit.go index 2927a3038..0abb98168 100644 --- a/pkg/commands/git_commands/commit.go +++ b/pkg/commands/git_commands/commit.go @@ -253,7 +253,7 @@ func (self *CommitCommands) AmendHeadCmdObj() *oscommands.CmdObj { return self.cmd.New(cmdArgs) } -func (self *CommitCommands) ShowCmdObj(hash string, filterPath string) *oscommands.CmdObj { +func (self *CommitCommands) ShowCmdObj(hash string, filterPaths []string) *oscommands.CmdObj { contextSize := self.UserConfig().Git.DiffContextSize extDiffCmd := self.UserConfig().Git.Paging.ExternalDiffCommand @@ -271,7 +271,7 @@ func (self *CommitCommands) ShowCmdObj(hash string, filterPath string) *oscomman ArgIf(self.UserConfig().Git.IgnoreWhitespaceInDiffView, "--ignore-all-space"). Arg(fmt.Sprintf("--find-renames=%d%%", self.UserConfig().Git.RenameSimilarityThreshold)). Arg("--"). - ArgIf(filterPath != "", filterPath). + Arg(filterPaths...). Dir(self.repoPaths.worktreePath). ToArgv() diff --git a/pkg/commands/git_commands/commit_loader.go b/pkg/commands/git_commands/commit_loader.go index 9fff74cdc..e8480ec1b 100644 --- a/pkg/commands/git_commands/commit_loader.go +++ b/pkg/commands/git_commands/commit_loader.go @@ -90,7 +90,7 @@ func (self *CommitLoader) GetCommits(opts GetCommitsOptions) ([]*models.Commit, defer wg.Done() var realCommits []*models.Commit - realCommits, logErr = loadCommits(self.getLogCmd(opts), func(line string) (*models.Commit, bool) { + realCommits, logErr = loadCommits(self.getLogCmd(opts), opts.FilterPath, func(line string) (*models.Commit, bool) { return self.extractCommitFromLine(opts.HashPool, line, opts.RefToShowDivergenceFrom != ""), false }) if logErr == nil { @@ -294,7 +294,10 @@ func (self *CommitLoader) getHydratedTodoCommits(hashPool *utils.StringPool, tod fullCommits := map[string]*models.Commit{} err := cmdObj.RunAndProcessLines(func(line string) (bool, error) { - commit := self.extractCommitFromLine(hashPool, line, false) + if line == "" || line[0] != '+' { + return false, nil + } + commit := self.extractCommitFromLine(hashPool, line[1:], false) fullCommits[commit.Hash()] = commit return false, nil }) @@ -601,7 +604,7 @@ func (self *CommitLoader) getLogCmd(opts GetCommitsOptions) *oscommands.CmdObj { Arg("--abbrev=40"). ArgIf(opts.FilterAuthor != "", "--author="+opts.FilterAuthor). ArgIf(opts.Limit, "-300"). - ArgIf(opts.FilterPath != "", "--follow"). + ArgIf(opts.FilterPath != "", "--follow", "--name-status"). Arg("--no-show-signature"). ArgIf(opts.RefToShowDivergenceFrom != "", "--left-right"). Arg("--"). @@ -611,4 +614,4 @@ func (self *CommitLoader) getLogCmd(opts GetCommitsOptions) *oscommands.CmdObj { return self.cmd.New(cmdArgs).DontLog() } -const prettyFormat = `--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s` +const prettyFormat = `--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s` diff --git a/pkg/commands/git_commands/commit_loader_test.go b/pkg/commands/git_commands/commit_loader_test.go index 6f20e7cc6..ba0cd9d84 100644 --- a/pkg/commands/git_commands/commit_loader_test.go +++ b/pkg/commands/git_commands/commit_loader_test.go @@ -15,16 +15,16 @@ import ( "github.com/stretchr/testify/assert" ) -var commitsOutput = strings.ReplaceAll(`0eea75e8c631fba6b58135697835d58ba4c18dbc|1640826609|Jesse Duffield|jessedduffield@gmail.com|HEAD -> better-tests|b21997d6b4cbdf84b149|>|better typing for rebase mode -b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164|1640824515|Jesse Duffield|jessedduffield@gmail.com|origin/better-tests|e94e8fc5b6fab4cb755f|>|fix logging -e94e8fc5b6fab4cb755f29f1bdb3ee5e001df35c|1640823749|Jesse Duffield|jessedduffield@gmail.com|tag: 123, tag: 456|d8084cd558925eb7c9c3|>|refactor -d8084cd558925eb7c9c38afeed5725c21653ab90|1640821426|Jesse Duffield|jessedduffield@gmail.com||65f910ebd85283b5cce9|>|WIP -65f910ebd85283b5cce9bf67d03d3f1a9ea3813a|1640821275|Jesse Duffield|jessedduffield@gmail.com||26c07b1ab33860a1a759|>|WIP -26c07b1ab33860a1a7591a0638f9925ccf497ffa|1640750752|Jesse Duffield|jessedduffield@gmail.com||3d4470a6c072208722e5|>|WIP -3d4470a6c072208722e5ae9a54bcb9634959a1c5|1640748818|Jesse Duffield|jessedduffield@gmail.com||053a66a7be3da43aacdc|>|WIP -053a66a7be3da43aacdc7aa78e1fe757b82c4dd2|1640739815|Jesse Duffield|jessedduffield@gmail.com||985fe482e806b172aea4|>|refactoring the config struct`, "|", "\x00") +var commitsOutput = strings.ReplaceAll(`+0eea75e8c631fba6b58135697835d58ba4c18dbc|1640826609|Jesse Duffield|jessedduffield@gmail.com|HEAD -> better-tests|b21997d6b4cbdf84b149|>|better typing for rebase mode ++b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164|1640824515|Jesse Duffield|jessedduffield@gmail.com|origin/better-tests|e94e8fc5b6fab4cb755f|>|fix logging ++e94e8fc5b6fab4cb755f29f1bdb3ee5e001df35c|1640823749|Jesse Duffield|jessedduffield@gmail.com|tag: 123, tag: 456|d8084cd558925eb7c9c3|>|refactor ++d8084cd558925eb7c9c38afeed5725c21653ab90|1640821426|Jesse Duffield|jessedduffield@gmail.com||65f910ebd85283b5cce9|>|WIP ++65f910ebd85283b5cce9bf67d03d3f1a9ea3813a|1640821275|Jesse Duffield|jessedduffield@gmail.com||26c07b1ab33860a1a759|>|WIP ++26c07b1ab33860a1a7591a0638f9925ccf497ffa|1640750752|Jesse Duffield|jessedduffield@gmail.com||3d4470a6c072208722e5|>|WIP ++3d4470a6c072208722e5ae9a54bcb9634959a1c5|1640748818|Jesse Duffield|jessedduffield@gmail.com||053a66a7be3da43aacdc|>|WIP ++053a66a7be3da43aacdc7aa78e1fe757b82c4dd2|1640739815|Jesse Duffield|jessedduffield@gmail.com||985fe482e806b172aea4|>|refactoring the config struct`, "|", "\x00") -var singleCommitOutput = strings.ReplaceAll(`0eea75e8c631fba6b58135697835d58ba4c18dbc|1640826609|Jesse Duffield|jessedduffield@gmail.com|HEAD -> better-tests|b21997d6b4cbdf84b149|>|better typing for rebase mode`, "|", "\x00") +var singleCommitOutput = strings.ReplaceAll(`+0eea75e8c631fba6b58135697835d58ba4c18dbc|1640826609|Jesse Duffield|jessedduffield@gmail.com|HEAD -> better-tests|b21997d6b4cbdf84b149|>|better typing for rebase mode`, "|", "\x00") func TestGetCommits(t *testing.T) { type scenario struct { @@ -44,7 +44,7 @@ func TestGetCommits(t *testing.T) { opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", IncludeRebaseCommits: false}, runner: oscommands.NewFakeRunner(t). ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). - ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, "", nil), + ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, "", nil), expectedCommitOpts: []models.NewCommitOpts{}, expectedError: nil, @@ -55,7 +55,7 @@ func TestGetCommits(t *testing.T) { opts: GetCommitsOptions{RefName: "refs/heads/mybranch", RefForPushedStatus: "refs/heads/mybranch", IncludeRebaseCommits: false}, runner: oscommands.NewFakeRunner(t). ExpectGitArgs([]string{"merge-base", "refs/heads/mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). - ExpectGitArgs([]string{"log", "refs/heads/mybranch", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, "", nil), + ExpectGitArgs([]string{"log", "refs/heads/mybranch", "--topo-order", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, "", nil), expectedCommitOpts: []models.NewCommitOpts{}, expectedError: nil, @@ -69,7 +69,7 @@ func TestGetCommits(t *testing.T) { // here it's seeing which commits are yet to be pushed ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). // here it's actually getting all the commits in a formatted form, one per line - ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, commitsOutput, nil). + ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, commitsOutput, nil). // here it's testing which of the configured main branches have an upstream ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "master@{u}"}, "refs/remotes/origin/master", nil). // this one does ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "main@{u}"}, "", errors.New("error")). // this one doesn't, so it checks origin instead @@ -205,7 +205,7 @@ func TestGetCommits(t *testing.T) { // here it's seeing which commits are yet to be pushed ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). // here it's actually getting all the commits in a formatted form, one per line - ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, singleCommitOutput, nil). + ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, singleCommitOutput, nil). // here it's testing which of the configured main branches exist; neither does ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "master@{u}"}, "", errors.New("error")). ExpectGitArgs([]string{"rev-parse", "--verify", "--quiet", "refs/remotes/origin/master"}, "", errors.New("error")). @@ -241,7 +241,7 @@ func TestGetCommits(t *testing.T) { // here it's seeing which commits are yet to be pushed ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). // here it's actually getting all the commits in a formatted form, one per line - ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, singleCommitOutput, nil). + ExpectGitArgs([]string{"log", "HEAD", "--topo-order", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, singleCommitOutput, nil). // here it's testing which of the configured main branches exist ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "master@{u}"}, "refs/remotes/origin/master", nil). ExpectGitArgs([]string{"rev-parse", "--symbolic-full-name", "main@{u}"}, "", errors.New("error")). @@ -276,7 +276,7 @@ func TestGetCommits(t *testing.T) { opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", IncludeRebaseCommits: false}, runner: oscommands.NewFakeRunner(t). ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). - ExpectGitArgs([]string{"log", "HEAD", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, "", nil), + ExpectGitArgs([]string{"log", "HEAD", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s", "--abbrev=40", "--no-show-signature", "--"}, "", nil), expectedCommitOpts: []models.NewCommitOpts{}, expectedError: nil, @@ -287,7 +287,7 @@ func TestGetCommits(t *testing.T) { opts: GetCommitsOptions{RefName: "HEAD", RefForPushedStatus: "mybranch", FilterPath: "src"}, runner: oscommands.NewFakeRunner(t). ExpectGitArgs([]string{"merge-base", "mybranch", "mybranch@{u}"}, "b21997d6b4cbdf84b149d8e6a2c4d06a8e9ec164", nil). - ExpectGitArgs([]string{"log", "HEAD", "--oneline", "--pretty=format:%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s", "--abbrev=40", "--follow", "--no-show-signature", "--", "src"}, "", nil), + ExpectGitArgs([]string{"log", "HEAD", "--oneline", "--pretty=format:+%H%x00%at%x00%aN%x00%ae%x00%D%x00%P%x00%m%x00%s", "--abbrev=40", "--follow", "--name-status", "--no-show-signature", "--", "src"}, "", nil), expectedCommitOpts: []models.NewCommitOpts{}, expectedError: nil, diff --git a/pkg/commands/git_commands/commit_loading_shared.go b/pkg/commands/git_commands/commit_loading_shared.go index 9bf774186..f1a14698e 100644 --- a/pkg/commands/git_commands/commit_loading_shared.go +++ b/pkg/commands/git_commands/commit_loading_shared.go @@ -1,23 +1,74 @@ package git_commands import ( + "strings" + "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/commands/oscommands" + "github.com/samber/lo" ) func loadCommits( cmd *oscommands.CmdObj, + filterPath string, parseLogLine func(string) (*models.Commit, bool), ) ([]*models.Commit, error) { commits := []*models.Commit{} - err := cmd.RunAndProcessLines(func(line string) (bool, error) { - commit, stop := parseLogLine(line) - if stop { - return true, nil + var commit *models.Commit + var filterPaths []string + // A string pool that stores interned strings to reduce memory usage + pool := make(map[string]string) + + finishLastCommit := func() { + if commit != nil { + // Only set the filter paths if we have one that is not contained in the original + // filter path. When filtering on a directory, all file paths will start with that + // directory, so we needn't bother storing the individual paths. Likewise, if we + // filter on a file and the file path hasn't changed, we needn't store it either. + // Only if a file has been moved or renamed do we need to store the paths, but then + // we need them all so that we can properly render a diff for the rename. + if lo.SomeBy(filterPaths, func(path string) bool { + return !strings.HasPrefix(path, filterPath) + }) { + commit.FilterPaths = lo.Map(filterPaths, func(path string, _ int) string { + if v, ok := pool[path]; ok { + return v + } + pool[path] = path + return path + }) + } + commits = append(commits, commit) + commit = nil + filterPaths = nil + } + } + err := cmd.RunAndProcessLines(func(line string) (bool, error) { + if line == "" { + return false, nil + } + + if line[0] == '+' { + finishLastCommit() + var stop bool + commit, stop = parseLogLine(line[1:]) + if stop { + commit = nil + return true, nil + } + } else if commit != nil && filterPath != "" { + // We are filtering by path, and this line is the output of the --name-status flag + fields := strings.Split(line, "\t") + // We don't bother looking at the first field (it will be 'A', 'M', 'R072' or a bunch of others). + // All we care about is the path(s), and there will be one for 'M' and 'A', and two for 'R' or 'C', + // in which case we want them both so that we can show the diff between the two. + if len(fields) > 1 { + filterPaths = append(filterPaths, fields[1:]...) + } } - commits = append(commits, commit) return false, nil }) + finishLastCommit() return commits, err } diff --git a/pkg/commands/git_commands/commit_test.go b/pkg/commands/git_commands/commit_test.go index 6abe12128..84d3126b5 100644 --- a/pkg/commands/git_commands/commit_test.go +++ b/pkg/commands/git_commands/commit_test.go @@ -251,7 +251,7 @@ func TestCommitCreateAmendCommit(t *testing.T) { func TestCommitShowCmdObj(t *testing.T) { type scenario struct { testName string - filterPath string + filterPaths []string contextSize uint64 similarityThreshold int ignoreWhitespace bool @@ -262,7 +262,7 @@ func TestCommitShowCmdObj(t *testing.T) { scenarios := []scenario{ { testName: "Default case without filter path", - filterPath: "", + filterPaths: []string{}, contextSize: 3, similarityThreshold: 50, ignoreWhitespace: false, @@ -271,7 +271,7 @@ func TestCommitShowCmdObj(t *testing.T) { }, { testName: "Default case with filter path", - filterPath: "file.txt", + filterPaths: []string{"file.txt"}, contextSize: 3, similarityThreshold: 50, ignoreWhitespace: false, @@ -280,7 +280,7 @@ func TestCommitShowCmdObj(t *testing.T) { }, { testName: "Show diff with custom context size", - filterPath: "", + filterPaths: []string{}, contextSize: 77, similarityThreshold: 50, ignoreWhitespace: false, @@ -289,7 +289,7 @@ func TestCommitShowCmdObj(t *testing.T) { }, { testName: "Show diff with custom similarity threshold", - filterPath: "", + filterPaths: []string{}, contextSize: 3, similarityThreshold: 33, ignoreWhitespace: false, @@ -298,7 +298,7 @@ func TestCommitShowCmdObj(t *testing.T) { }, { testName: "Show diff, ignoring whitespace", - filterPath: "", + filterPaths: []string{}, contextSize: 77, similarityThreshold: 50, ignoreWhitespace: true, @@ -307,7 +307,7 @@ func TestCommitShowCmdObj(t *testing.T) { }, { testName: "Show diff with external diff command", - filterPath: "", + filterPaths: []string{}, contextSize: 3, similarityThreshold: 50, ignoreWhitespace: false, @@ -330,7 +330,7 @@ func TestCommitShowCmdObj(t *testing.T) { } instance := buildCommitCommands(commonDeps{userConfig: userConfig, appState: &config.AppState{}, runner: runner, repoPaths: &repoPaths}) - assert.NoError(t, instance.ShowCmdObj("1234567890", s.filterPath).Run()) + assert.NoError(t, instance.ShowCmdObj("1234567890", s.filterPaths).Run()) runner.CheckForMissingCalls() }) } diff --git a/pkg/commands/git_commands/reflog_commit_loader.go b/pkg/commands/git_commands/reflog_commit_loader.go index da0d1c3e0..053fc4598 100644 --- a/pkg/commands/git_commands/reflog_commit_loader.go +++ b/pkg/commands/git_commands/reflog_commit_loader.go @@ -28,16 +28,16 @@ func (self *ReflogCommitLoader) GetReflogCommits(hashPool *utils.StringPool, las cmdArgs := NewGitCmd("log"). Config("log.showSignature=false"). Arg("-g"). - Arg("--format=%H%x00%ct%x00%gs%x00%P"). + Arg("--format=+%H%x00%ct%x00%gs%x00%P"). ArgIf(filterAuthor != "", "--author="+filterAuthor). - ArgIf(filterPath != "", "--follow", "--", filterPath). + ArgIf(filterPath != "", "--follow", "--name-status", "--", filterPath). ToArgv() cmdObj := self.cmd.New(cmdArgs).DontLog() onlyObtainedNewReflogCommits := false - commits, err := loadCommits(cmdObj, func(line string) (*models.Commit, bool) { + commits, err := loadCommits(cmdObj, filterPath, func(line string) (*models.Commit, bool) { commit, ok := self.parseLine(hashPool, line) if !ok { return nil, false diff --git a/pkg/commands/git_commands/reflog_commit_loader_test.go b/pkg/commands/git_commands/reflog_commit_loader_test.go index 468f6ca06..839533f3f 100644 --- a/pkg/commands/git_commands/reflog_commit_loader_test.go +++ b/pkg/commands/git_commands/reflog_commit_loader_test.go @@ -14,11 +14,11 @@ import ( "github.com/stretchr/testify/assert" ) -var reflogOutput = strings.ReplaceAll(`c3c4b66b64c97ffeecde|1643150483|checkout: moving from A to B|51baa8c1 -c3c4b66b64c97ffeecde|1643150483|checkout: moving from B to A|51baa8c1 -c3c4b66b64c97ffeecde|1643150483|checkout: moving from A to B|51baa8c1 -c3c4b66b64c97ffeecde|1643150483|checkout: moving from master to A|51baa8c1 -f4ddf2f0d4be4ccc7efa|1643149435|checkout: moving from A to master|51baa8c1 +var reflogOutput = strings.ReplaceAll(`+c3c4b66b64c97ffeecde|1643150483|checkout: moving from A to B|51baa8c1 ++c3c4b66b64c97ffeecde|1643150483|checkout: moving from B to A|51baa8c1 ++c3c4b66b64c97ffeecde|1643150483|checkout: moving from A to B|51baa8c1 ++c3c4b66b64c97ffeecde|1643150483|checkout: moving from master to A|51baa8c1 ++f4ddf2f0d4be4ccc7efa|1643149435|checkout: moving from A to master|51baa8c1 `, "|", "\x00") func TestGetReflogCommits(t *testing.T) { @@ -39,7 +39,7 @@ func TestGetReflogCommits(t *testing.T) { { testName: "no reflog entries", runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=%H%x00%ct%x00%gs%x00%P"}, "", nil), + ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=+%H%x00%ct%x00%gs%x00%P"}, "", nil), lastReflogCommit: nil, expectedCommitOpts: []models.NewCommitOpts{}, @@ -49,7 +49,7 @@ func TestGetReflogCommits(t *testing.T) { { testName: "some reflog entries", runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=%H%x00%ct%x00%gs%x00%P"}, reflogOutput, nil), + ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=+%H%x00%ct%x00%gs%x00%P"}, reflogOutput, nil), lastReflogCommit: nil, expectedCommitOpts: []models.NewCommitOpts{ @@ -95,7 +95,7 @@ func TestGetReflogCommits(t *testing.T) { { testName: "some reflog entries where last commit is given", runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=%H%x00%ct%x00%gs%x00%P"}, reflogOutput, nil), + ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=+%H%x00%ct%x00%gs%x00%P"}, reflogOutput, nil), lastReflogCommit: models.NewCommit(hashPool, models.NewCommitOpts{ Hash: "c3c4b66b64c97ffeecde", @@ -119,7 +119,7 @@ func TestGetReflogCommits(t *testing.T) { { testName: "when passing filterPath", runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=%H%x00%ct%x00%gs%x00%P", "--follow", "--", "path"}, reflogOutput, nil), + ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=+%H%x00%ct%x00%gs%x00%P", "--follow", "--name-status", "--", "path"}, reflogOutput, nil), lastReflogCommit: models.NewCommit(hashPool, models.NewCommitOpts{ Hash: "c3c4b66b64c97ffeecde", @@ -144,7 +144,7 @@ func TestGetReflogCommits(t *testing.T) { { testName: "when passing filterAuthor", runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=%H%x00%ct%x00%gs%x00%P", "--author=John Doe "}, reflogOutput, nil), + ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=+%H%x00%ct%x00%gs%x00%P", "--author=John Doe "}, reflogOutput, nil), lastReflogCommit: models.NewCommit(hashPool, models.NewCommitOpts{ Hash: "c3c4b66b64c97ffeecde", @@ -169,7 +169,7 @@ func TestGetReflogCommits(t *testing.T) { { testName: "when command returns error", runner: oscommands.NewFakeRunner(t). - ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=%H%x00%ct%x00%gs%x00%P"}, "", errors.New("haha")), + ExpectGitArgs([]string{"-c", "log.showSignature=false", "log", "-g", "--format=+%H%x00%ct%x00%gs%x00%P"}, "", errors.New("haha")), lastReflogCommit: nil, filterPath: "", diff --git a/pkg/commands/models/commit.go b/pkg/commands/models/commit.go index eca66b3ac..8dfec4d4b 100644 --- a/pkg/commands/models/commit.go +++ b/pkg/commands/models/commit.go @@ -54,6 +54,10 @@ type Commit struct { // Hashes of parent commits (will be multiple if it's a merge commit) parents []*string + // When filtering by path, this contains the paths that were changed in this + // commit; nil when not filtering by path. + FilterPaths []string + Status CommitStatus Action todo.TodoCommand Divergence Divergence // set to DivergenceNone unless we are showing the divergence view diff --git a/pkg/gui/controllers/helpers/diff_helper.go b/pkg/gui/controllers/helpers/diff_helper.go index 9a08f15ec..0ef23de36 100644 --- a/pkg/gui/controllers/helpers/diff_helper.go +++ b/pkg/gui/controllers/helpers/diff_helper.go @@ -65,10 +65,21 @@ func (self *DiffHelper) GetUpdateTaskForRenderingCommitsDiff(commit *models.Comm return types.NewRunPtyTaskWithPrefix(cmdObj.GetCmd(), prefix) } - cmdObj := self.c.Git().Commit.ShowCmdObj(commit.Hash(), self.c.Modes().Filtering.GetPath()) + cmdObj := self.c.Git().Commit.ShowCmdObj(commit.Hash(), self.FilterPathsForCommit(commit)) return types.NewRunPtyTask(cmdObj.GetCmd()) } +func (self *DiffHelper) FilterPathsForCommit(commit *models.Commit) []string { + filterPath := self.c.Modes().Filtering.GetPath() + if filterPath != "" { + if len(commit.FilterPaths) > 0 { + return commit.FilterPaths + } + return []string{filterPath} + } + return nil +} + func (self *DiffHelper) ExitDiffMode() error { self.c.Modes().Diffing = diffing.New() self.c.Refresh(types.RefreshOptions{Mode: types.ASYNC}) diff --git a/pkg/gui/controllers/reflog_commits_controller.go b/pkg/gui/controllers/reflog_commits_controller.go index ea35eca44..2d0751a0b 100644 --- a/pkg/gui/controllers/reflog_commits_controller.go +++ b/pkg/gui/controllers/reflog_commits_controller.go @@ -45,7 +45,7 @@ func (self *ReflogCommitsController) GetOnRenderToMain() func() { if commit == nil { task = types.NewRenderStringTask("No reflog history") } else { - cmdObj := self.c.Git().Commit.ShowCmdObj(commit.Hash(), self.c.Modes().Filtering.GetPath()) + cmdObj := self.c.Git().Commit.ShowCmdObj(commit.Hash(), self.c.Helpers().Diff.FilterPathsForCommit(commit)) task = types.NewRunPtyTask(cmdObj.GetCmd()) } diff --git a/pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go b/pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go index 3d91e391c..b0d574056 100644 --- a/pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go +++ b/pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go @@ -80,7 +80,6 @@ var ShowDiffsForRenamedFile = NewIntegrationTest(NewIntegrationTestArgs{ t.Views().Main().ContainsLines( Equals(" rename file"), Equals("---"), - /* EXPECTED: Equals(" oldFile => newFile | 0"), Equals(" 1 file changed, 0 insertions(+), 0 deletions(-)"), Equals(""), @@ -88,24 +87,10 @@ var ShowDiffsForRenamedFile = NewIntegrationTest(NewIntegrationTestArgs{ Equals("similarity index 100%"), Equals("rename from oldFile"), Equals("rename to newFile"), - ACTUAL: */ - Equals(" newFile | 3 +++"), - Equals(" 1 file changed, 3 insertions(+)"), - Equals(""), - Equals("diff --git a/newFile b/newFile"), - Equals("new file mode 100644"), - Contains("index"), - Equals("--- /dev/null"), - Equals("+++ b/newFile"), - Equals("@@ -0,0 +1,3 @@"), - Equals("+x"), - Equals("+b"), - Equals("+c"), ) t.Views().Commits().SelectNextItem() - /* EXPECTED: t.Views().Main().ContainsLines( Equals(" update old file"), Equals("---"), @@ -122,7 +107,5 @@ var ShowDiffsForRenamedFile = NewIntegrationTest(NewIntegrationTestArgs{ Equals(" b"), Equals(" c"), ) - ACTUAL: */ - t.Views().Main().IsEmpty() }, }) From e716617a8269e13b169544b71762e854ec39b53b Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 17 Jul 2025 21:19:45 +0200 Subject: [PATCH 10/11] Add test for range diff across rename When shift-selecting a range of commits across a file rename in filtering-by-path mode, the diff currently shows an added file rather than a renamed file. Add a test that demonstrates this, we'll fix this in the next commit. --- .../show_diffs_for_renamed_file.go | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go b/pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go index b0d574056..eae09acd2 100644 --- a/pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go +++ b/pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go @@ -107,5 +107,43 @@ var ShowDiffsForRenamedFile = NewIntegrationTest(NewIntegrationTestArgs{ Equals(" b"), Equals(" c"), ) + + t.Views().Commits(). + Press(keys.Universal.RangeSelectUp). + Press(keys.Universal.RangeSelectUp) + + t.Views().Main().ContainsLines( + /* EXPECTED: + Contains("Showing diff for range"), + Equals(""), + Equals(" oldFile => newFile | 2 +-"), + Equals(" 1 file changed, 1 insertion(+), 1 deletion(-)"), + Equals(""), + Equals("diff --git a/oldFile b/newFile"), + Equals("similarity index 66%"), + Equals("rename from oldFile"), + Equals("rename to newFile"), + Contains("index"), + Equals("--- a/oldFile"), + Equals("+++ b/newFile"), + Equals("@@ -1,3 +1,3 @@"), + Equals("-a"), + Equals("+y"), + Equals(" b"), + Equals(" c"), + ACTUAL: */ + Equals(" newFile | 3 +++"), + Equals(" 1 file changed, 3 insertions(+)"), + Equals(""), + Equals("diff --git a/newFile b/newFile"), + Equals("new file mode 100644"), + Contains("index"), + Equals("--- /dev/null"), + Equals("+++ b/newFile"), + Equals("@@ -0,0 +1,3 @@"), + Equals("+y"), + Equals("+b"), + Equals("+c"), + ) }, }) From a1aeedd5d559b079ff9c1978d9349738e41596d5 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 17 Jul 2025 20:52:46 +0200 Subject: [PATCH 11/11] Fix showing range diff across a rename when filtering by path We need to pass the union of filter paths at both ends of the range. --- pkg/gui/controllers/helpers/diff_helper.go | 16 ++++++++++++++-- .../show_diffs_for_renamed_file.go | 14 -------------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/pkg/gui/controllers/helpers/diff_helper.go b/pkg/gui/controllers/helpers/diff_helper.go index 0ef23de36..1fd73081a 100644 --- a/pkg/gui/controllers/helpers/diff_helper.go +++ b/pkg/gui/controllers/helpers/diff_helper.go @@ -57,8 +57,20 @@ func (self *DiffHelper) GetUpdateTaskForRenderingCommitsDiff(commit *models.Comm from, to := refRange.From, refRange.To args := []string{from.ParentRefName(), to.RefName(), "--stat", "-p"} args = append(args, "--") - if path := self.c.Modes().Filtering.GetPath(); path != "" { - args = append(args, path) + if filterPath := self.c.Modes().Filtering.GetPath(); filterPath != "" { + // If both refs are commits, filter by the union of their paths. This is useful for + // example when diffing a range of commits in filter-by-path mode across a rename. + fromCommit, ok1 := from.(*models.Commit) + toCommit, ok2 := to.(*models.Commit) + if ok1 && ok2 { + paths := append(self.FilterPathsForCommit(fromCommit), self.FilterPathsForCommit(toCommit)...) + args = append(args, lo.Uniq(paths)...) + } else { + // If either ref is not a commit (which is possible in sticky diff mode, when + // diffing against a branch or tag), we just filter by the filter path; that's the + // best we can do in this case. + args = append(args, filterPath) + } } cmdObj := self.c.Git().Diff.DiffCmdObj(args) prefix := style.FgYellow.Sprintf("%s %s-%s\n\n", self.c.Tr.ShowingDiffForRange, from.ShortRefName(), to.ShortRefName()) diff --git a/pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go b/pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go index eae09acd2..720a681e1 100644 --- a/pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go +++ b/pkg/integration/tests/filter_by_path/show_diffs_for_renamed_file.go @@ -113,7 +113,6 @@ var ShowDiffsForRenamedFile = NewIntegrationTest(NewIntegrationTestArgs{ Press(keys.Universal.RangeSelectUp) t.Views().Main().ContainsLines( - /* EXPECTED: Contains("Showing diff for range"), Equals(""), Equals(" oldFile => newFile | 2 +-"), @@ -131,19 +130,6 @@ var ShowDiffsForRenamedFile = NewIntegrationTest(NewIntegrationTestArgs{ Equals("+y"), Equals(" b"), Equals(" c"), - ACTUAL: */ - Equals(" newFile | 3 +++"), - Equals(" 1 file changed, 3 insertions(+)"), - Equals(""), - Equals("diff --git a/newFile b/newFile"), - Equals("new file mode 100644"), - Contains("index"), - Equals("--- /dev/null"), - Equals("+++ b/newFile"), - Equals("@@ -0,0 +1,3 @@"), - Equals("+y"), - Equals("+b"), - Equals("+c"), ) }, })