Add command to show divergence from upstream

[stefanhaller]

• PR Description

This adds a new command to the "upstream" menu (available by pressing u in the branches panel) that lets you see what git calls the "symmetric difference" of a branch against its upstream. I find this useful in two situations:

• when a branch has diverged from its upstream (i.e. it shows mybranch ↑3↓5) it allows you to tell whether this is because you and your coworker have both added commits (in which case you want to pull), or because you rebased the branch locally, in which case you want to force-push. Or both, in which case you want to maybe cherry-pick the upstream commit (which is easy from this new view), and then force-push.

• when master has new commits (i.e. it shows master ↓7), this lets you see which commits you would fetch before actually doing it.

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.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 (specifically docs/Config.md) have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[jesseduffield]

This is a sorely missing feature right now. I'm in a similar spot with the rebase-onto PR where there's a tension between a unified UX and a tailored UX. On one side I'd like a unified UX for diffing, but the overwhelming majority of symmetric diffing is between a branch and its upstream, so I think having a bespoke UI for that makes sense.

I agree about the keyboard awkwardness. How about adding an entry to the 'upstream' menu (invoked with 'u'). I think that would be a good place for something like this.

With those up arrows: I think we also should include them in the regular view, but in the place of the existing commit icon. That way it'll be clear what they represent when you're going between the views (and it'll also clear up confusion for those who don't get the meaning of red commit hashes in the first place).

It would be good if there was some easy way to look at a summary of the file changes rather than having to go commit-by-commit. VSCode has it as the first entry in a list of commits:

It would also be good to have headers for the two sets of changes i.e. 'changes to pull' and 'changes to push'. It would also be good if you could collapse those headers to easily get from the set of commits you care about (not super important here because if all you cared about was the commits to push you could look at them in the regular commits view). At any rate, there's no real precedent for having items in these views that represent something completely different from the other things, and I don't think it's a good pattern to add a new field in the commits model called 'IsHeader': it's something that should happen at the view-model/view level rather than the model level. But this is all just wishlist stuff: If we can get this PR across the line with the existing UI patterns in place then that's good enough for me.

FWIW I haven't read the code yet

[jesseduffield]

This controller is attached to a few contexts:

		gui.State.Contexts.Branches,
		gui.State.Contexts.RemoteBranches,
		gui.State.Contexts.Tags,
		gui.State.Contexts.ReflogCommits,

But we only want this keybinding for branches right?

Might need to create a 'switch_to_subcommits_helper' which is a reflection of a poor architecture here: often controller code needs to be used from somewhere else and the only solution is to extract that code out into a helper.

[stefanhaller]

Right. In the implementation we check whether the model is a models.Branch and only do the left-right thing in that case, falling back to the normal log if not. We also fall back to normal log if the branch has no upstream, so we need those if statements anyway.

[stefanhaller]

I agree about the keyboard awkwardness. How about adding an entry to the 'upstream' menu (invoked with 'u').

Good idea, that might work. (Except that I really use this command so often that I would prefer it to be a single-key shortcut. But I could try if I can get used to it.)

What would you call it?

With those up arrows: I think we also should include them in the regular view, but in the place of the existing commit icon. That way it'll be clear what they represent when you're going between the views (and it'll also clear up confusion for those who don't get the meaning of red commit hashes in the first place).

I disagree; the red/yellow colors in this new left-right view mean something very different from the red/yellow in the normal view, so I think it would be very confusing to add arrows in the normal view and make it look like they mean something similar.

It would be good if there was some easy way to look at a summary of the file changes rather than having to go commit-by-commit.

I would see this as an orthogonal feature. Right now I do this by going to the first commit of the branch (or one below), typing W, and moving to the head commit. It would be nice if there was an easier way to do this, but I see this as independent from the left/right log.

It would also be good to have headers for the two sets of changes i.e. 'changes to pull' and 'changes to push'. It would also be good if you could collapse those headers to easily get from the set of commits you care about

This is tricky because the red/yellow commits can be intermingled depending on log order. Which is arguably confusing when it happens, so we might sort the commits by type. Anyway, I don't miss those headers myself.

[stefanhaller]

the red/yellow colors in this new left-right view mean something very different from the red/yellow in the normal view,

So maybe we should use entirely different colors for the left-right view to avoid this confusion. Maybe magenta for outgoing, and blue for incoming? (Trying this in a fixup.)

This is tricky because the red/yellow commits can be intermingled depending on log order. Which is arguably confusing when it happens, so we might sort the commits by type.

This only happened to me because I had git.log.order: "default" in my config. (I don't remember why, I don't like that. 😄) With the default of topo-order they never appear intermingled, so maybe we should use that explicitly in this view, overriding the config. However, the order of incoming vs. outgoing commits is still not deterministic, it depends on the commit dates. I wonder if it would be easier to understand if incoming commits would always come first.

Added a fixup to sort incoming commits before outgoing ones, this helps a lot, I find.

[stefanhaller]

OK, as for the arrows and the colors: I was a bit slow here, I get it now. We should show the up arrow for red commits in the normal view too (only for those), and make the outgoing commits in the left-right view red as well. We then only need a different color for the incoming commits, I picked blue for now.

This also simplifies the implementation a bit, because we can make the incoming state just another models.Status, so we don't need an extra model field for the left/right thing and lots of special cases around it.

The only things that's a bit confusing now is when you rebase your local branch onto a newer master, which I just did with this branch. In that case, some of the commits that are shown in green in the normal commits view are shown in red in the left-right view, which is confusing. But I don't want to show them in green in the left-right view (I think).

I also added a commit that moves the command to the upstream menu as you suggested. If we make it the first entry so that you can hit u <enter>, that would work for me.

[jesseduffield]

The only things that's a bit confusing now is when you rebase your local branch onto a newer master, which I just did with this branch. In that case, some of the commits that are shown in green in the normal commits view are shown in red in the left-right view, which is confusing. But I don't want to show them in green in the left-right view (I think).

I agree they should just appear as red in the left-right view. It would be cool if we could show the up arrow in the local commits view against commits that are both merged-to-master and unpushed. That would require splitting out pushed status (or merged status) from the commit's Status field. Up to you if you think that's worth doing here.

[jesseduffield]

Looking really good, left some comments

[stefanhaller]

I'm still struggling with the concepts here, and what's the best way to visualize them.

Arrows: they really mean different things in the normal commits view and the left-right view, and I'm no longer sure if I would show them in the normal view for that reason. In the left-right view, they just mark the "mine vs. theirs" distinction, this has nothing to do with local vs. remote. In the normal view they would mark commits that the remote doesn't have yet, which is something very different. As for the suggestion to show arrows for green commits if they are both merged and unpushed: I bet people would find this confusing and would ask what that means.

Colors: I think I changed my mind about showing green for master commits in the left/right view. It would actually be very useful to be able to tell where a branch starts if it has been rebased. Here's an example of a branch that was rebased locally onto a newer master:

In this contrived case you can easily tell from the commit subjects where the branch starts, but in the real world it might not be so easy.

On the other hand, using just two colors for "mine vs. theirs" helps make the two sections more clear, as in the image above; the arrows alone are not distinct enough for that purpose. So if we lose this clear color coding, we might really need the section headers that you suggested earlier. I'm just not keen on going that route because it seems like so much work.

So, unsure what to do. Currently I lean towards:

• not show arrows in normal view
• show red/blue/green colors in left-right view, and see if that's good enough without section headers.

[stefanhaller]

So, I changed it in that way. Which means we're back to the previous implementation with an extra field in the model (an enum this time instead of a string). I'm pretty happy with this now.

Here's what it looked like for this very branch right before I force-pushed:

I still think adding section headers would make it clearer, but I'd be ok with going without them for now.

[jesseduffield]

I agree with that UX. I also would like headers there but happy to merge without them. I'll take another look at the code now

[jesseduffield]

Actually one thing that would be good is to add the 'view divergence from upstream' menu item to the diffing menu as well (shift+w). Users may think to look for the option in the upstream menu or the diffing menu and I think we should support both angles.

[jesseduffield]

Patch here based on tweaking things locally which does a view things:

• removes the need for a type assertion
• explicitly checks if the upstream is tracked locally

diff --git a/pkg/commands/models/branch.go b/pkg/commands/models/branch.go
index b4dcc0a79..5b49fcdc7 100644
--- a/pkg/commands/models/branch.go
+++ b/pkg/commands/models/branch.go
@@ -1,5 +1,7 @@
 package models
 
+import "fmt"
+
 // Branch : A git branch
 // duplicating this for now
 type Branch struct {
@@ -43,6 +45,14 @@ func (b *Branch) ParentRefName() string {
 	return b.RefName() + "^"
 }
 
+func (b *Branch) FullUpstreamRefName() string {
+	if b.UpstreamRemote == "" || b.UpstreamBranch == "" {
+		return ""
+	}
+
+	return fmt.Sprintf("refs/remotes/%s/%s", b.UpstreamRemote, b.UpstreamBranch)
+}
+
 func (b *Branch) ID() string {
 	return b.RefName()
 }
diff --git a/pkg/gui/controllers/branches_controller.go b/pkg/gui/controllers/branches_controller.go
index 7f0a0f9fb..24139fa09 100644
--- a/pkg/gui/controllers/branches_controller.go
+++ b/pkg/gui/controllers/branches_controller.go
@@ -144,20 +144,15 @@ func (self *BranchesController) setUpstream(selectedBranch *models.Branch) error
 			{
 				LabelColumns: []string{self.c.Tr.ViewDivergenceFromUpstream},
 				OnPress: func() error {
-					ref := self.context().GetSelectedRef()
-					if ref == nil {
+					branch := self.context().GetSelected()
+					if branch == nil {
 						return nil
 					}
 
-					upstreamRefName := ""
-					if branch, ok := ref.(*models.Branch); ok {
-						if branch.UpstreamRemote != "" && branch.UpstreamBranch != "" {
-							upstreamRefName = fmt.Sprintf("refs/remotes/%s/%s", branch.UpstreamRemote, branch.UpstreamBranch)
-						} else {
-							return self.c.ErrorMsg(self.c.Tr.DivergenceNoUpstream)
-						}
+					if !branch.RemoteBranchStoredLocally() {
+						return self.c.ErrorMsg(self.c.Tr.DivergenceNoUpstream)
 					}
-					return self.c.Helpers().SubCommits.ViewSubCommits(ref, upstreamRefName, self.context(), false)
+					return self.c.Helpers().SubCommits.ViewSubCommits(branch, branch.FullUpstreamRefName(), self.context(), false)
 				},
 				Key: 'v',
 			},
diff --git a/pkg/i18n/english.go b/pkg/i18n/english.go
index 072e5b44b..bfa9e3ed6 100644
--- a/pkg/i18n/english.go
+++ b/pkg/i18n/english.go
@@ -1123,7 +1123,7 @@ func EnglishTranslationSet() TranslationSet {
 		SetUpstream:                         "Set upstream of selected branch",
 		UnsetUpstream:                       "Unset upstream of selected branch",
 		ViewDivergenceFromUpstream:          "View divergence from upstream",
-		DivergenceNoUpstream:                "Cannot show divergence of a branch that has no upstream",
+		DivergenceNoUpstream:                "Cannot show divergence of a branch that has no (locally tracked) upstream",
 		SetUpstreamTitle:                    "Set upstream branch",
 		SetUpstreamMessage:                  "Are you sure you want to set the upstream branch of '{{.checkedOut}}' to '{{.selected}}'",
 		EditRemote:                          "Edit remote",

[jesseduffield]

can we rename these to 'DivergenceAhead' and 'DivergenceBehind' to generalise for the sake of diffing any two refs?

[stefanhaller]

Personally I don't like the terms "ahead" and "behind" much, they are not an intuitive description of what's happening. For example, for a branch that my co-worker rebased onto a newer master, my divergence view shows my older branch commits as "ahead", but the upstream commits (which are really "newer") as "behind". I don't find this intuitive at all.

I find terms like "local/remote" (or "local/upstream") or "mine/theirs" more useful to describe the situation.

[stefanhaller]

This is still unresolved; which one should I pick?

1. DivergenceLocal/DivergenceUpstream (the current version)
2. DivergenceBehind/DivergenceAhead (your preference)
3. DivergenceMine/DivergenceTheirs
4. DivergenceLocal/DivergenceRemote

I can live with each of these, but my preference is either 3 or 4.

(We should make these names the same as what we show in the section headers.)

[jesseduffield]

Sorry, forgot to respond.

It's gonna have to be either 2 or 3 because we could be comparing any two refs. Ahead/Behind is used in vscode when it shows divergence, so there's precedence for it. The downside is that it invokes time.

Mine/Theirs is intuitive when comparing your branch to somebody else's branch, but it's confusing when you own both branches. It invokes interpersonal comparison

ChatGPT just suggested source/target. That sounds pretty neutral to me, what do you think?

[jesseduffield]

BTW as for labels: I'm happy to have the labels be something like 'Commits unique to A', and 'Commits unique to B' where we don't need to use the same terminology we're using here.

[stefanhaller]

source/target sounds too neutral to me, I find it hard to relate the terms to the feature.

I agree that 'Commits unique to A' (or maybe just 'A') makes sense when we allow comparing any two refs. We don't have that feature yet, and for the specific case of comparing against upstream I prefer Local/Remote. Can we make this configurable (later, when we need it) and show Local/Remote in the headers for now?

In that case, what we use for the variable names doesn't matter too much to me. Maybe DivergenceThis/DivergenceOther. Or even DivergenceLeft/DivergenceRight?

[jesseduffield]

For sure: when comparing to upstream we can have specific headers for that.

Happy to go with DivergenceLeft/DivergenceRight as it aligns with the flag that we're passing to git log.

[jesseduffield]

I'd add a comment explaining that this is only set when comparing refs

[stefanhaller]

Added one in 8d163ca.

[jesseduffield]

I'd have this title passed in as an argument so we can say 'mybranch vs origin/mybranch'

[stefanhaller]

Changed in 56531a8. I went with mybranch <-> origin/mybranch instead of "vs", thinking that it will be easier to translate (in German we don't have a word for "vs" that is as short).

[jesseduffield]

I'd make this an opts struct rather than use positional args given that it's getting long

[stefanhaller]

Changed in e2acae1.

[stefanhaller]

Actually one thing that would be good is to add the 'view divergence from upstream' menu item to the diffing menu as well (shift+w). Users may think to look for the option in the upstream menu or the diffing menu and I think we should support both angles.

Note sure I agree; as explained elsewhere, I see diffing and viewing left/right logs as very different concepts, and I'm not sure it's good to blur the line between them. That's not a very strong opinion though, happy to put it in if you really want.

[stefanhaller]

I changed my mind about the section headers. In the beginning we had only two colors, for local/remote, and I didn't think the headers were necessary then. But now we have three colors in total (remote can be blue/green, or local can be red/green), and this additional color coding is way more useful than I thought, but now I do think the headers are needed to make it clear enough.

Unfortunately it's trickier than I thought for two reasons:

• The current mechanism for providing section headers using the getNonModelItems func doesn't work for the refreshViewportOnChange case, i.e. when getDisplayStrings is called with a startIdx and length that's not the entire model. The reason is that getDisplayStrings expects these to be model indices, however we really want to provide view indices there, so we need to convert from view to model, but we need to call getNonModelItems before so that we can do that. However, it currently takes the column positions, which we only have after calling getNonModelItems. The solution is to split getNonModelItems in two, one for getting the positions, and one for rendering the headers. This needs a pretty heavy rework of what was done in Section headers in keybindings menu #2911, and I think I want to do it in that PR so that it's right from the start.
• The graph needs to be rendered in two separate sections. Without a section header I thought it's acceptable to render it in one piece, resulting in two separate unconnected branches of the graph; but with a header in between this looks too ugly. I haven't looked into how hard this is going to be; hopefully not too bad, but all this means it's going to take a bit longer. Will work more on this next week.

[jesseduffield]

Note sure I agree; as explained elsewhere, I see diffing and viewing left/right logs as very different concepts, and I'm not sure it's good to blur the line between them. That's not a very strong opinion though, happy to put it in if you really want.

Happy to leave that out of this PR

[stefanhaller]

New version here, too. This now sits on top of #2911.

[stefanhaller]

Happy to take suggestions for a better UI, for now I went with the simplest possible design. I wasn't sure if we want to align this with the sha column or maybe the subject column.

[stefanhaller]

This is really too bad; I'd like to have the graph in the left/right view, too. But I tried, and it's too hard to do; not only would we have to render the graph in two sections, but we'd actually have to calculate two separate sets of pipe sets; and then there's this global cache for these. I'll leave this as a future improvement.

[jesseduffield]

LGTM. Really looking forward to using this myself