Make opening git difftool more consistent

[part22]

The default shortcut to open git difftool (ctrl+t) is not available on the "Local Branches" window. It is available when selecting a commit from a local branch, a remote branch, or a tag from the "Local Branches" window.

This is inconsistent since branches or tags are also commits, the shortcut should also work on them directly.

This commit remedies this inconsistency by allowing the use of the shortcut directly on a branch or a tag. The shortcut works both in the "standard" mode and the "diffing" mode.

Current Behaviour	New Behaviour
Select a branch Press enter to see the commits Select the head commit Press ctrl+t to open git difftool	Select a branch Press ctrl+t to open git difftool

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[stefanhaller]

I'm not convinced that this behavior makes sense when not in diffing mode. In diffing mode it works as expected, opening difftool with the same diff that I'm seeing in the main window, which is a nice addition.

But outside of diffing mode it opens difftool with the diff of the last commit of the selected branch or tag, which doesn't make sense to me. What do users expect to see in this case? I can see two possibilities:

• "the diff of this branch". This could mean the diff of the commit range of the yellow commits of the branch, or in other words the diff of the branch head against the merge base with the closest main branch. The downside is that it only makes sense for branches, not for tags.
• "the diff of where I am compared to the branch". This is what you get when typing git difftool <somebranch> on the command line: it gives you the diff of the working tree against that branch.

I suppose I would prefer either the second of the options above (calling difftool with only one argument), or disabling the command when not in diffing mode.

Curious about your thoughts on this.

[part22]

Hi Stefan,

I think you are right, it is more consistent to have the same behaviour as the command line.
We should chose the solution that will be the least surprising to git users.

I have modfied the behaviour so it matches the behaviour of git difftool <somebranch>.

Thanks for your feedback!

[stefanhaller]

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?

[part22]

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.

[stefanhaller]

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).

[part22]

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 ./...

[stefanhaller]

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

[stefanhaller]

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.)

[stefanhaller]

Great work, thanks for bearing with my nitpicking. Looks very good now, merging.