Skip to content

Make opening git difftool more consistent #3691

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stefanhaller merged 1 commit into from
Jun 30, 2024
Merged

Make opening git difftool more consistent #3691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/keybindings/Keybindings_en.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` g `` | Reset | |
| `` R `` | Rename branch | |
| `` u `` | View upstream options | View options relating to the branch's upstream e.g. setting/unsetting the upstream and resetting to the upstream. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | View commits | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down Expand Up @@ -271,6 +272,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` u `` | Set as upstream | Set the selected remote branch as the upstream of the checked-out branch. |
| `` s `` | Sort order | |
| `` g `` | Reset | View reset options (soft/mixed/hard) for resetting onto selected item. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | View commits | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down Expand Up @@ -349,6 +351,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` d `` | Delete | View delete options for local/remote tag. |
| `` P `` | Push tag | Push the selected tag to a remote. You'll be prompted to select a remote. |
| `` g `` | Reset | View reset options (soft/mixed/hard) for resetting onto selected item. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | View commits | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down
3 changes: 3 additions & 0 deletions docs/keybindings/Keybindings_ja.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` d `` | Delete | View delete options for local/remote tag. |
| `` P `` | タグをpush | Push the selected tag to a remote. You'll be prompted to select a remote. |
| `` g `` | Reset | View reset options (soft/mixed/hard) for resetting onto selected item. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | コミットを閲覧 | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down Expand Up @@ -239,6 +240,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` g `` | Reset | |
| `` R `` | ブランチ名を変更 | |
| `` u `` | View upstream options | View options relating to the branch's upstream e.g. setting/unsetting the upstream and resetting to the upstream. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | コミットを閲覧 | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down Expand Up @@ -335,6 +337,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` u `` | Set as upstream | Set the selected remote branch as the upstream of the checked-out branch. |
| `` s `` | 並び替え | |
| `` g `` | Reset | View reset options (soft/mixed/hard) for resetting onto selected item. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | コミットを閲覧 | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down
3 changes: 3 additions & 0 deletions docs/keybindings/Keybindings_ko.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ _Legend: `<c-b>` means ctrl+b, `<a-b>` means alt+b, `B` means shift+b_
| `` g `` | View reset options | |
| `` R `` | 브랜치 이름 변경 | |
| `` u `` | View upstream options | View options relating to the branch's upstream e.g. setting/unsetting the upstream and resetting to the upstream. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | 커밋 보기 | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down Expand Up @@ -248,6 +249,7 @@ _Legend: `<c-b>` means ctrl+b, `<a-b>` means alt+b, `B` means shift+b_
| `` u `` | Set as upstream | Set the selected remote branch as the upstream of the checked-out branch. |
| `` s `` | Sort order | |
| `` g `` | View reset options | View reset options (soft/mixed/hard) for resetting onto selected item. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | 커밋 보기 | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down Expand Up @@ -322,6 +324,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` d `` | Delete | View delete options for local/remote tag. |
| `` P `` | 태그를 push | Push the selected tag to a remote. You'll be prompted to select a remote. |
| `` g `` | Reset | View reset options (soft/mixed/hard) for resetting onto selected item. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | 커밋 보기 | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down
3 changes: 3 additions & 0 deletions docs/keybindings/Keybindings_nl.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ _Legend: `<c-b>` means ctrl+b, `<a-b>` means alt+b, `B` means shift+b_
| `` g `` | Bekijk reset opties | |
| `` R `` | Hernoem branch | |
| `` u `` | View upstream options | View options relating to the branch's upstream e.g. setting/unsetting the upstream and resetting to the upstream. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | Bekijk commits | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down Expand Up @@ -249,6 +250,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` u `` | Set as upstream | Stel in als upstream van uitgecheckte branch |
| `` s `` | Sort order | |
| `` g `` | Bekijk reset opties | View reset options (soft/mixed/hard) for resetting onto selected item. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | Bekijk commits | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down Expand Up @@ -349,6 +351,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` d `` | Delete | View delete options for local/remote tag. |
| `` P `` | Push tag | Push the selected tag to a remote. You'll be prompted to select a remote. |
| `` g `` | Reset | View reset options (soft/mixed/hard) for resetting onto selected item. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | Bekijk commits | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down
3 changes: 3 additions & 0 deletions docs/keybindings/Keybindings_pl.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ Jeśli chcesz zamiast tego rozpocząć interaktywny rebase od wybranego commita,
| `` g `` | Reset | |
| `` R `` | Zmień nazwę gałęzi | |
| `` u `` | Pokaż opcje upstream | Pokaż opcje dotyczące upstream gałęzi, np. ustawianie/usuwanie upstream i resetowanie do upstream. |
| `` <c-t> `` | Otwórz zewnętrzne narzędzie różnic (git difftool) | |
| `` <enter> `` | Pokaż commity | |
| `` w `` | Zobacz opcje drzewa pracy | |
| `` / `` | Filtruj bieżący widok po tekście | |
Expand Down Expand Up @@ -331,6 +332,7 @@ Jeśli chcesz zamiast tego rozpocząć interaktywny rebase od wybranego commita,
| `` d `` | Usuń | Wyświetl opcje usuwania lokalnego/odległego tagu. |
| `` P `` | Wyślij tag | Wyślij wybrany tag do zdalnego. Zostaniesz poproszony o wybranie zdalnego. |
| `` g `` | Reset | Wyświetl opcje resetu (miękki/mieszany/twardy) do wybranego elementu. |
| `` <c-t> `` | Otwórz zewnętrzne narzędzie różnic (git difftool) | |
| `` <enter> `` | Pokaż commity | |
| `` w `` | Zobacz opcje drzewa pracy | |
| `` / `` | Filtruj bieżący widok po tekście | |
Expand Down Expand Up @@ -359,6 +361,7 @@ Jeśli chcesz zamiast tego rozpocząć interaktywny rebase od wybranego commita,
| `` u `` | Ustaw jako upstream | Ustaw wybraną gałąź zdalną jako upstream sprawdzonej gałęzi. |
| `` s `` | Kolejność sortowania | |
| `` g `` | Reset | Wyświetl opcje resetu (miękki/mieszany/twardy) do wybranego elementu. |
| `` <c-t> `` | Otwórz zewnętrzne narzędzie różnic (git difftool) | |
| `` <enter> `` | Pokaż commity | |
| `` w `` | Zobacz opcje drzewa pracy | |
| `` / `` | Filtruj bieżący widok po tekście | |
3 changes: 3 additions & 0 deletions docs/keybindings/Keybindings_ru.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` g `` | Просмотреть параметры сброса | |
| `` R `` | Переименовать ветку | |
| `` u `` | View upstream options | View options relating to the branch's upstream e.g. setting/unsetting the upstream and resetting to the upstream. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | Просмотреть коммиты | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down Expand Up @@ -288,6 +289,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` d `` | Delete | View delete options for local/remote tag. |
| `` P `` | Отправить тег | Push the selected tag to a remote. You'll be prompted to select a remote. |
| `` g `` | Reset | View reset options (soft/mixed/hard) for resetting onto selected item. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | Просмотреть коммиты | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand All @@ -305,6 +307,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` u `` | Set as upstream | Установить как upstream-ветку переключённую ветку |
| `` s `` | Порядок сортировки | |
| `` g `` | Просмотреть параметры сброса | View reset options (soft/mixed/hard) for resetting onto selected item. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | Просмотреть коммиты | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down
3 changes: 3 additions & 0 deletions docs/keybindings/Keybindings_zh-CN.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ _Legend: `<c-b>` means ctrl+b, `<a-b>` means alt+b, `B` means shift+b_
| `` g `` | 查看重置选项 | |
| `` R `` | 重命名分支 | |
| `` u `` | View upstream options | View options relating to the branch's upstream e.g. setting/unsetting the upstream and resetting to the upstream. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | 查看提交 | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down Expand Up @@ -248,6 +249,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` d `` | Delete | View delete options for local/remote tag. |
| `` P `` | 推送标签 | Push the selected tag to a remote. You'll be prompted to select a remote. |
| `` g `` | Reset | View reset options (soft/mixed/hard) for resetting onto selected item. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | 查看提交 | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down Expand Up @@ -348,6 +350,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` u `` | Set as upstream | 设置为检出分支的上游 |
| `` s `` | Sort order | |
| `` g `` | 查看重置选项 | View reset options (soft/mixed/hard) for resetting onto selected item. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | 查看提交 | |
| `` w `` | View worktree options | |
| `` / `` | Filter the current view by text | |
Expand Down
3 changes: 3 additions & 0 deletions docs/keybindings/Keybindings_zh-TW.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` g `` | 檢視重設選項 | |
| `` R `` | 重新命名分支 | |
| `` u `` | 檢視上游設定 | 檢視有關上游分支的設定（例如重設至上游） |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | 檢視提交 | |
| `` w `` | 檢視工作目錄選項 | |
| `` / `` | 搜尋 | |
Expand All @@ -284,6 +285,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` d `` | Delete | View delete options for local/remote tag. |
| `` P `` | 推送標籤 | Push the selected tag to a remote. You'll be prompted to select a remote. |
| `` g `` | Reset | View reset options (soft/mixed/hard) for resetting onto selected item. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | 檢視提交 | |
| `` w `` | 檢視工作目錄選項 | |
| `` / `` | 搜尋 | |
Expand Down Expand Up @@ -359,6 +361,7 @@ If you would instead like to start an interactive rebase from the selected commi
| `` u `` | Set as upstream | 將此分支設為當前分支之上游 |
| `` s `` | Sort order | |
| `` g `` | 檢視重設選項 | View reset options (soft/mixed/hard) for resetting onto selected item. |
| `` <c-t> `` | Open external diff tool (git difftool) | |
| `` <enter> `` | 檢視提交 | |
| `` w `` | 檢視工作目錄選項 | |
| `` / `` | 搜尋 | |
8 changes: 8 additions & 0 deletions pkg/gui/controllers/branches_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,14 @@ func (self *BranchesController) GetKeybindings(opts types.KeybindingsOpts) []*ty
OpensMenu: true,
DisplayOnScreen: true,
},
{
Key: opts.GetKey(opts.Config.Universal.OpenDiffTool),
Handler: self.withItem(func(selectedBranch *models.Branch) error {
return self.c.Helpers().Diff.OpenDiffToolForRef(selectedBranch)
}),
GetDisabledReason: self.require(self.singleItemSelected()),
Description: self.c.Tr.OpenDiffTool,
},
}
}

Expand Down
16 changes: 16 additions & 0 deletions pkg/gui/controllers/helpers/diff_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package helpers

import (
"github.com/jesseduffield/lazygit/pkg/commands/git_commands"
"github.com/jesseduffield/lazygit/pkg/gui/context"
"github.com/jesseduffield/lazygit/pkg/gui/modes/diffing"
"github.com/jesseduffield/lazygit/pkg/gui/types"
Expand Down Expand Up @@ -119,3 +120,18 @@ func (self *DiffHelper) IgnoringWhitespaceSubTitle() string {

return ""
}

func (self *DiffHelper) OpenDiffToolForRef(selectedRef types.Ref) error {
to := selectedRef.RefName()
from, reverse := self.c.Modes().Diffing.GetFromAndReverseArgsForDiff("")
_, err := self.c.RunSubprocess(self.c.Git().Diff.OpenDiffToolCmdObj(
git_commands.DiffToolCmdOptions{
Filepath: ".",
FromCommit: from,
ToCommit: to,
Reverse: reverse,
IsDirectory: true,
Staged: false,
}))
return err
}
Copy link
Collaborator

@stefanhaller stefanhaller Jun 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is close, but not quite the same as git difftool <selectedRef> on the command line does. You are diffing the selected ref against the checked-out branch, but git difftool <selectedRef> diffs it against the working tree. This is different when there are modified files in the working tree.

The easiest way to do this seems to be

func (self *DiffHelper) DefaultOpenDiffTool(selectedRef types.Ref) error {
	to := selectedRef.RefName()
	from, reverse := self.c.Modes().Diffing.GetFromAndReverseArgsForDiff("")
	_, err := self.c.RunSubprocess(self.c.Git().Diff.OpenDiffToolCmdObj(
		git_commands.DiffToolCmdOptions{
			Filepath:    ".",
			FromCommit:  from,
			ToCommit:    to,
			Reverse:     reverse,
			IsDirectory: true,
			Staged:      false,
		}))
	return err
}

Side note: I'm usually a fan of ternaries, but when you have to use tuples to pack and unpack multiple return values, then that's taking it too far for my taste. Also, my preference for ternaries probably comes from C++ where it makes it possible to declare the variable as const, which is always nice; go doesn't have this concept, so there's not as much of a benefit. And the more go code I read, the more I get the feeling that

x := "first choice"
if condition {
  x = "other choice"
}

is simply the idiomatic way of expressing this in go, so I'm probably going to reduce my use of lo.Ternary.

And finally: the name DefaultOpenDiffTool doesn't say much about what this is; how about OpenDiffToolForRef?

Copy link
Contributor Author

@part22 part22 Jun 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I have changed the behaviour as per your suggestion so the diff includes the changes in the working tree.

I have also simplified the use of the ternary; I was also perviously wondering if the tradeoff was worth it.

fromRefName := lo.Ternary(self.c.Modes().Diffing.Active(), selectedRef.ParentRefName(), "")
from, reverse := self.c.Modes().Diffing.GetFromAndReverseArgsForDiff(fromRefName)

Yeah, I also like using IIFE to be able to run logic before assigning a value to a const variable but as you mentioned Go does not have the const concept. ¯\_(ツ)_/¯

I have renamed the function DefaultOpenDiffTool as per your suggestion to OpenDifftoolForRef.

Thank you for feedback.

Copy link
Collaborator

@stefanhaller stefanhaller Jun 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need the ternary at all, GetFromAndReverseArgsForDiff has the if inside already. The code I posted above is the one you want. (Yours does the same, but it's unnecessarily complex.)

Spelling nitpick: we say DiffTool everywhere, which an upper case "T", so it should be OpenDiffToolForRef.

And finally, your cheatsheets are out of date, please run go generate ./... (as the PR template says).

Copy link
Contributor Author

@part22 part22 Jun 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, that's been fixed as well as the spelling.

I ran go generate ./... but it seems to fail. I ran the command both from this branch and a clean version of master.

I am on Windows and I am using Powershell; this is the output of go generate ./...

image

Copy link
Collaborator

@stefanhaller stefanhaller Jun 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generating cheatsheets is indeed broken on Windows right now (among other things). I'll fix that, I updated them here for now.

Copy link
Collaborator

@stefanhaller stefanhaller Jun 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed by #3706, if you would like to test that. (It won't do anything of course, as all generated files are up to date, but at least it shouldn't error.)

8 changes: 8 additions & 0 deletions pkg/gui/controllers/remote_branches_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,14 @@ func (self *RemoteBranchesController) GetKeybindings(opts types.KeybindingsOpts)
Tooltip: self.c.Tr.ResetTooltip,
OpensMenu: true,
},
{
Key: opts.GetKey(opts.Config.Universal.OpenDiffTool),
Handler: self.withItem(func(selectedBranch *models.RemoteBranch) error {
return self.c.Helpers().Diff.OpenDiffToolForRef(selectedBranch)
}),
GetDisabledReason: self.require(self.singleItemSelected()),
Description: self.c.Tr.OpenDiffTool,
},
}
}

Expand Down
8 changes: 8 additions & 0 deletions pkg/gui/controllers/tags_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,14 @@ func (self *TagsController) GetKeybindings(opts types.KeybindingsOpts) []*types.
DisplayOnScreen: true,
OpensMenu: true,
},
{
Key: opts.GetKey(opts.Config.Universal.OpenDiffTool),
Handler: self.withItem(func(selectedTag *models.Tag) error {
return self.c.Helpers().Diff.OpenDiffToolForRef(selectedTag)
}),
GetDisabledReason: self.require(self.singleItemSelected()),
Description: self.c.Tr.OpenDiffTool,
},
}

return bindings
Expand Down